| Message ID | 20170911142056.15643-3-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hmp: fix "dump-quest-memory" segfault | expand |
On 11 September 2017 at 15:20, Laurent Vivier <lvivier@redhat.com> wrote: > Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) > fixes the problem for i386, do the same for arm. > > Running QEMU with > qemu-system-aarch64 -M none -nographic -m 256 > and executing > dump-guest-memory /dev/null 0 8192 > results in segfault > > Fix by checking if we have CPU. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> It seems a little arbitrary to assume that if there's no CPU what you wanted was a 32-bit little-endian dump. Why do we have a machine without a CPU anyway ? thanks -- PMM
On 11.09.2017 16:39, Peter Maydell wrote: > On 11 September 2017 at 15:20, Laurent Vivier <lvivier@redhat.com> wrote: >> Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) >> fixes the problem for i386, do the same for arm. >> >> Running QEMU with >> qemu-system-aarch64 -M none -nographic -m 256 >> and executing >> dump-guest-memory /dev/null 0 8192 >> results in segfault >> >> Fix by checking if we have CPU. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > It seems a little arbitrary to assume that if there's no > CPU what you wanted was a 32-bit little-endian dump. > > Why do we have a machine without a CPU anyway ? The "none" machine is always started without a default CPU. Thomas
On Mon, 11 Sep 2017 16:20:55 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) > fixes the problem for i386, do the same for arm. > > Running QEMU with > qemu-system-aarch64 -M none -nographic -m 256 > and executing > dump-guest-memory /dev/null 0 8192 > results in segfault > > Fix by checking if we have CPU. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > target/arm/arch_dump.c | 52 +++++++++++++++++++++++++++++++++----------------- > 1 file changed, 34 insertions(+), 18 deletions(-) > > diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c > index 1a9861f69b..1f58cff256 100644 > --- a/target/arm/arch_dump.c > +++ b/target/arm/arch_dump.c > @@ -273,8 +273,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, > int cpu_get_dump_info(ArchDumpInfo *info, > const GuestPhysBlockList *guest_phys_blocks) > { > - ARMCPU *cpu = ARM_CPU(first_cpu); > - CPUARMState *env = &cpu->env; > GuestPhysBlock *block; > hwaddr lowest_addr = ULLONG_MAX; > > @@ -290,13 +288,32 @@ int cpu_get_dump_info(ArchDumpInfo *info, > } > } > > - if (arm_feature(env, ARM_FEATURE_AARCH64)) { > - info->d_machine = EM_AARCH64; > - info->d_class = ELFCLASS64; > - info->page_size = (1 << 16); /* aarch64 max pagesize */ > - if (lowest_addr != ULLONG_MAX) { > - info->phys_base = lowest_addr; > + if (first_cpu) { > + ARMCPU *cpu = ARM_CPU(first_cpu); > + CPUARMState *env = &cpu->env; > + if (arm_feature(env, ARM_FEATURE_AARCH64)) { > + info->d_machine = EM_AARCH64; > + info->d_class = ELFCLASS64; > + info->page_size = (1 << 16); /* aarch64 max pagesize */ > + if (lowest_addr != ULLONG_MAX) { > + info->phys_base = lowest_addr; > + } > + } else { > + info->d_machine = EM_ARM; > + info->d_class = ELFCLASS32; > + info->page_size = (1 << 12); > + if (lowest_addr < UINT_MAX) { > + info->phys_base = lowest_addr; > + } > } > + > + /* We assume the relevant endianness is that of EL1; this is right > + * for kernels, but might give the wrong answer if you're trying to > + * dump a hypervisor that happens to be running an opposite-endian > + * kernel. > + */ > + info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0 > + ? ELFDATA2MSB : ELFDATA2LSB; > } else { > info->d_machine = EM_ARM; > info->d_class = ELFCLASS32; > @@ -304,25 +321,24 @@ int cpu_get_dump_info(ArchDumpInfo *info, > if (lowest_addr < UINT_MAX) { > info->phys_base = lowest_addr; > } > + info->d_endian = ELFDATA2LSB; > } > > - /* We assume the relevant endianness is that of EL1; this is right > - * for kernels, but might give the wrong answer if you're trying to > - * dump a hypervisor that happens to be running an opposite-endian > - * kernel. > - */ > - info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0 > - ? ELFDATA2MSB : ELFDATA2LSB; > - > return 0; > } > > ssize_t cpu_get_note_size(int class, int machine, int nr_cpus) > { > - ARMCPU *cpu = ARM_CPU(first_cpu); > - CPUARMState *env = &cpu->env; > + ARMCPU *cpu; > + CPUARMState *env; > size_t note_size; > > + if (first_cpu == NULL) { > + return 0; > + } > + Looking at the function's code, it seems that env is only needed if class != ELFCLASS64... I guess that all the code dealing with first_cpu should go to the else block. > + cpu = ARM_CPU(first_cpu); > + env = &cpu->env; > if (class == ELFCLASS64) { > note_size = AARCH64_PRSTATUS_NOTE_SIZE; > note_size += AARCH64_PRFPREG_NOTE_SIZE;
On 11/09/2017 17:08, Greg Kurz wrote: > On Mon, 11 Sep 2017 16:20:55 +0200 > Laurent Vivier <lvivier@redhat.com> wrote: > >> Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) >> fixes the problem for i386, do the same for arm. >> >> Running QEMU with >> qemu-system-aarch64 -M none -nographic -m 256 >> and executing >> dump-guest-memory /dev/null 0 8192 >> results in segfault >> >> Fix by checking if we have CPU. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> target/arm/arch_dump.c | 52 +++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 34 insertions(+), 18 deletions(-) >> >> diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c >> index 1a9861f69b..1f58cff256 100644 >> --- a/target/arm/arch_dump.c >> +++ b/target/arm/arch_dump.c >> @@ -273,8 +273,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, >> int cpu_get_dump_info(ArchDumpInfo *info, >> const GuestPhysBlockList *guest_phys_blocks) >> { >> - ARMCPU *cpu = ARM_CPU(first_cpu); >> - CPUARMState *env = &cpu->env; >> GuestPhysBlock *block; >> hwaddr lowest_addr = ULLONG_MAX; >> >> @@ -290,13 +288,32 @@ int cpu_get_dump_info(ArchDumpInfo *info, >> } >> } >> >> - if (arm_feature(env, ARM_FEATURE_AARCH64)) { >> - info->d_machine = EM_AARCH64; >> - info->d_class = ELFCLASS64; >> - info->page_size = (1 << 16); /* aarch64 max pagesize */ >> - if (lowest_addr != ULLONG_MAX) { >> - info->phys_base = lowest_addr; >> + if (first_cpu) { >> + ARMCPU *cpu = ARM_CPU(first_cpu); >> + CPUARMState *env = &cpu->env; >> + if (arm_feature(env, ARM_FEATURE_AARCH64)) { >> + info->d_machine = EM_AARCH64; >> + info->d_class = ELFCLASS64; >> + info->page_size = (1 << 16); /* aarch64 max pagesize */ >> + if (lowest_addr != ULLONG_MAX) { >> + info->phys_base = lowest_addr; >> + } >> + } else { >> + info->d_machine = EM_ARM; >> + info->d_class = ELFCLASS32; >> + info->page_size = (1 << 12); >> + if (lowest_addr < UINT_MAX) { >> + info->phys_base = lowest_addr; >> + } >> } >> + >> + /* We assume the relevant endianness is that of EL1; this is right >> + * for kernels, but might give the wrong answer if you're trying to >> + * dump a hypervisor that happens to be running an opposite-endian >> + * kernel. >> + */ >> + info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0 >> + ? ELFDATA2MSB : ELFDATA2LSB; >> } else { >> info->d_machine = EM_ARM; >> info->d_class = ELFCLASS32; >> @@ -304,25 +321,24 @@ int cpu_get_dump_info(ArchDumpInfo *info, >> if (lowest_addr < UINT_MAX) { >> info->phys_base = lowest_addr; >> } >> + info->d_endian = ELFDATA2LSB; >> } >> >> - /* We assume the relevant endianness is that of EL1; this is right >> - * for kernels, but might give the wrong answer if you're trying to >> - * dump a hypervisor that happens to be running an opposite-endian >> - * kernel. >> - */ >> - info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0 >> - ? ELFDATA2MSB : ELFDATA2LSB; >> - >> return 0; >> } >> >> ssize_t cpu_get_note_size(int class, int machine, int nr_cpus) >> { >> - ARMCPU *cpu = ARM_CPU(first_cpu); >> - CPUARMState *env = &cpu->env; >> + ARMCPU *cpu; >> + CPUARMState *env; >> size_t note_size; >> >> + if (first_cpu == NULL) { >> + return 0; >> + } >> + > > Looking at the function's code, it seems that env is only needed if > class != ELFCLASS64... I guess that all the code dealing with first_cpu > should go to the else block. if first_cpu is NULL, nr_cpus is 0 and the function always returns 0. Thanks, Laurent
On 11/09/2017 16:39, Peter Maydell wrote: > On 11 September 2017 at 15:20, Laurent Vivier <lvivier@redhat.com> wrote: >> Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) >> fixes the problem for i386, do the same for arm. >> >> Running QEMU with >> qemu-system-aarch64 -M none -nographic -m 256 >> and executing >> dump-guest-memory /dev/null 0 8192 >> results in segfault >> >> Fix by checking if we have CPU. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > It seems a little arbitrary to assume that if there's no > CPU what you wanted was a 32-bit little-endian dump. We need a default value. Is 64bit and/or big-endian better? > Why do we have a machine without a CPU anyway ? To hotplug it later? As we can, we should avoid the segfault. Thanks, Laurent
On Mon, 11 Sep 2017 17:25:23 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > On 11/09/2017 17:08, Greg Kurz wrote: > > On Mon, 11 Sep 2017 16:20:55 +0200 > > Laurent Vivier <lvivier@redhat.com> wrote: > > > >> Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) > >> fixes the problem for i386, do the same for arm. > >> > >> Running QEMU with > >> qemu-system-aarch64 -M none -nographic -m 256 > >> and executing > >> dump-guest-memory /dev/null 0 8192 > >> results in segfault > >> > >> Fix by checking if we have CPU. > >> > >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> > >> --- > >> target/arm/arch_dump.c | 52 +++++++++++++++++++++++++++++++++----------------- > >> 1 file changed, 34 insertions(+), 18 deletions(-) > >> > >> diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c > >> index 1a9861f69b..1f58cff256 100644 > >> --- a/target/arm/arch_dump.c > >> +++ b/target/arm/arch_dump.c > >> @@ -273,8 +273,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, > >> int cpu_get_dump_info(ArchDumpInfo *info, > >> const GuestPhysBlockList *guest_phys_blocks) > >> { > >> - ARMCPU *cpu = ARM_CPU(first_cpu); > >> - CPUARMState *env = &cpu->env; > >> GuestPhysBlock *block; > >> hwaddr lowest_addr = ULLONG_MAX; > >> > >> @@ -290,13 +288,32 @@ int cpu_get_dump_info(ArchDumpInfo *info, > >> } > >> } > >> > >> - if (arm_feature(env, ARM_FEATURE_AARCH64)) { > >> - info->d_machine = EM_AARCH64; > >> - info->d_class = ELFCLASS64; > >> - info->page_size = (1 << 16); /* aarch64 max pagesize */ > >> - if (lowest_addr != ULLONG_MAX) { > >> - info->phys_base = lowest_addr; > >> + if (first_cpu) { > >> + ARMCPU *cpu = ARM_CPU(first_cpu); > >> + CPUARMState *env = &cpu->env; > >> + if (arm_feature(env, ARM_FEATURE_AARCH64)) { > >> + info->d_machine = EM_AARCH64; > >> + info->d_class = ELFCLASS64; > >> + info->page_size = (1 << 16); /* aarch64 max pagesize */ > >> + if (lowest_addr != ULLONG_MAX) { > >> + info->phys_base = lowest_addr; > >> + } > >> + } else { > >> + info->d_machine = EM_ARM; > >> + info->d_class = ELFCLASS32; > >> + info->page_size = (1 << 12); > >> + if (lowest_addr < UINT_MAX) { > >> + info->phys_base = lowest_addr; > >> + } > >> } > >> + > >> + /* We assume the relevant endianness is that of EL1; this is right > >> + * for kernels, but might give the wrong answer if you're trying to > >> + * dump a hypervisor that happens to be running an opposite-endian > >> + * kernel. > >> + */ > >> + info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0 > >> + ? ELFDATA2MSB : ELFDATA2LSB; > >> } else { > >> info->d_machine = EM_ARM; > >> info->d_class = ELFCLASS32; > >> @@ -304,25 +321,24 @@ int cpu_get_dump_info(ArchDumpInfo *info, > >> if (lowest_addr < UINT_MAX) { > >> info->phys_base = lowest_addr; > >> } > >> + info->d_endian = ELFDATA2LSB; > >> } > >> > >> - /* We assume the relevant endianness is that of EL1; this is right > >> - * for kernels, but might give the wrong answer if you're trying to > >> - * dump a hypervisor that happens to be running an opposite-endian > >> - * kernel. > >> - */ > >> - info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0 > >> - ? ELFDATA2MSB : ELFDATA2LSB; > >> - > >> return 0; > >> } > >> > >> ssize_t cpu_get_note_size(int class, int machine, int nr_cpus) > >> { > >> - ARMCPU *cpu = ARM_CPU(first_cpu); > >> - CPUARMState *env = &cpu->env; > >> + ARMCPU *cpu; > >> + CPUARMState *env; > >> size_t note_size; > >> > >> + if (first_cpu == NULL) { > >> + return 0; > >> + } > >> + > > > > Looking at the function's code, it seems that env is only needed if > > class != ELFCLASS64... I guess that all the code dealing with first_cpu > > should go to the else block. > > if first_cpu is NULL, nr_cpus is 0 and the function always returns 0. > True. Reviewed-by: Greg Kurz <groug@kaod.org> > Thanks, > Laurent
On 11 September 2017 at 15:45, Thomas Huth <thuth@redhat.com> wrote: > On 11.09.2017 16:39, Peter Maydell wrote: >> On 11 September 2017 at 15:20, Laurent Vivier <lvivier@redhat.com> wrote: >>> Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) >>> fixes the problem for i386, do the same for arm. >>> >>> Running QEMU with >>> qemu-system-aarch64 -M none -nographic -m 256 >>> and executing >>> dump-guest-memory /dev/null 0 8192 >>> results in segfault >>> >>> Fix by checking if we have CPU. >>> >>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> >> It seems a little arbitrary to assume that if there's no >> CPU what you wanted was a 32-bit little-endian dump. >> >> Why do we have a machine without a CPU anyway ? > > The "none" machine is always started without a default CPU. If it has no CPU then how can we create a core dump for it? We don't (in theory) even know whether it's x86 or ARM. (One day we may support multiple CPU architectures in one QEMU binary...) If the theory is hotplug-later then we're a bit stuck because we need to know information now that we can't know until the CPU is actually hotplugged. thanks -- PMM
On 11.09.2017 18:40, Peter Maydell wrote: > On 11 September 2017 at 15:45, Thomas Huth <thuth@redhat.com> wrote: >> On 11.09.2017 16:39, Peter Maydell wrote: >>> On 11 September 2017 at 15:20, Laurent Vivier <lvivier@redhat.com> wrote: >>>> Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) >>>> fixes the problem for i386, do the same for arm. >>>> >>>> Running QEMU with >>>> qemu-system-aarch64 -M none -nographic -m 256 >>>> and executing >>>> dump-guest-memory /dev/null 0 8192 >>>> results in segfault >>>> >>>> Fix by checking if we have CPU. >>>> >>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>> >>> It seems a little arbitrary to assume that if there's no >>> CPU what you wanted was a 32-bit little-endian dump. >>> >>> Why do we have a machine without a CPU anyway ? >> >> The "none" machine is always started without a default CPU. > > If it has no CPU then how can we create a core dump for it? > We don't (in theory) even know whether it's x86 or ARM. > (One day we may support multiple CPU architectures in > one QEMU binary...) True. Maybe it's better to return -1 if first_cpu is NULL to signal that a dump is not possible...? Thomas
On 11/09/2017 18:40, Peter Maydell wrote: > On 11 September 2017 at 15:45, Thomas Huth <thuth@redhat.com> wrote: >> On 11.09.2017 16:39, Peter Maydell wrote: >>> On 11 September 2017 at 15:20, Laurent Vivier <lvivier@redhat.com> wrote: >>>> Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) >>>> fixes the problem for i386, do the same for arm. >>>> >>>> Running QEMU with >>>> qemu-system-aarch64 -M none -nographic -m 256 >>>> and executing >>>> dump-guest-memory /dev/null 0 8192 >>>> results in segfault >>>> >>>> Fix by checking if we have CPU. >>>> >>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >>> >>> It seems a little arbitrary to assume that if there's no >>> CPU what you wanted was a 32-bit little-endian dump. >>> >>> Why do we have a machine without a CPU anyway ? >> >> The "none" machine is always started without a default CPU. > > If it has no CPU then how can we create a core dump for it? > We don't (in theory) even know whether it's x86 or ARM. > (One day we may support multiple CPU architectures in > one QEMU binary...) > > If the theory is hotplug-later then we're a bit stuck > because we need to know information now that we can't > know until the CPU is actually hotplugged. As we have memory we should be able to dump memory, even without CPU. But I can also do as proposed by Thomas and return -1 to cancel the dump if there is no CPU. Thanks, Laurent
diff --git a/target/arm/arch_dump.c b/target/arm/arch_dump.c index 1a9861f69b..1f58cff256 100644 --- a/target/arm/arch_dump.c +++ b/target/arm/arch_dump.c @@ -273,8 +273,6 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs, int cpu_get_dump_info(ArchDumpInfo *info, const GuestPhysBlockList *guest_phys_blocks) { - ARMCPU *cpu = ARM_CPU(first_cpu); - CPUARMState *env = &cpu->env; GuestPhysBlock *block; hwaddr lowest_addr = ULLONG_MAX; @@ -290,13 +288,32 @@ int cpu_get_dump_info(ArchDumpInfo *info, } } - if (arm_feature(env, ARM_FEATURE_AARCH64)) { - info->d_machine = EM_AARCH64; - info->d_class = ELFCLASS64; - info->page_size = (1 << 16); /* aarch64 max pagesize */ - if (lowest_addr != ULLONG_MAX) { - info->phys_base = lowest_addr; + if (first_cpu) { + ARMCPU *cpu = ARM_CPU(first_cpu); + CPUARMState *env = &cpu->env; + if (arm_feature(env, ARM_FEATURE_AARCH64)) { + info->d_machine = EM_AARCH64; + info->d_class = ELFCLASS64; + info->page_size = (1 << 16); /* aarch64 max pagesize */ + if (lowest_addr != ULLONG_MAX) { + info->phys_base = lowest_addr; + } + } else { + info->d_machine = EM_ARM; + info->d_class = ELFCLASS32; + info->page_size = (1 << 12); + if (lowest_addr < UINT_MAX) { + info->phys_base = lowest_addr; + } } + + /* We assume the relevant endianness is that of EL1; this is right + * for kernels, but might give the wrong answer if you're trying to + * dump a hypervisor that happens to be running an opposite-endian + * kernel. + */ + info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0 + ? ELFDATA2MSB : ELFDATA2LSB; } else { info->d_machine = EM_ARM; info->d_class = ELFCLASS32; @@ -304,25 +321,24 @@ int cpu_get_dump_info(ArchDumpInfo *info, if (lowest_addr < UINT_MAX) { info->phys_base = lowest_addr; } + info->d_endian = ELFDATA2LSB; } - /* We assume the relevant endianness is that of EL1; this is right - * for kernels, but might give the wrong answer if you're trying to - * dump a hypervisor that happens to be running an opposite-endian - * kernel. - */ - info->d_endian = (env->cp15.sctlr_el[1] & SCTLR_EE) != 0 - ? ELFDATA2MSB : ELFDATA2LSB; - return 0; } ssize_t cpu_get_note_size(int class, int machine, int nr_cpus) { - ARMCPU *cpu = ARM_CPU(first_cpu); - CPUARMState *env = &cpu->env; + ARMCPU *cpu; + CPUARMState *env; size_t note_size; + if (first_cpu == NULL) { + return 0; + } + + cpu = ARM_CPU(first_cpu); + env = &cpu->env; if (class == ELFCLASS64) { note_size = AARCH64_PRSTATUS_NOTE_SIZE; note_size += AARCH64_PRFPREG_NOTE_SIZE;
Commit fd5d23babf (hmp: fix "dump-quest-memory" segfault) fixes the problem for i386, do the same for arm. Running QEMU with qemu-system-aarch64 -M none -nographic -m 256 and executing dump-guest-memory /dev/null 0 8192 results in segfault Fix by checking if we have CPU. Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- target/arm/arch_dump.c | 52 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 18 deletions(-)