Am 28.06.2012 18:46, schrieb Peter Maydell:
> On 20 June 2012 18:28, Rabin Vincent <ra...@rab.in> wrote:
>> Add a minimal dump-guest-memory support for ARM. The -p option is not
>> supported and we don't add any QEMU-specific notes.
>
> So what does this patch give us? This commit message is pretty
> short and I couldn't find a cover message for the patchset...
>
>> Signed-off-by: Rabin Vincent <ra...@rab.in>
>> ---
>> configure | 4 +--
>> target-arm/Makefile.objs | 2 +-
>> target-arm/arch_dump.c | 59
>> ++++++++++++++++++++++++++++++++++++++
>> target-arm/arch_memory_mapping.c | 13 +++++++++
>> 4 files changed, 75 insertions(+), 3 deletions(-)
>> create mode 100644 target-arm/arch_dump.c
>> create mode 100644 target-arm/arch_memory_mapping.c
>>
>> diff --git a/configure b/configure
>> index b68c0ca..a20ad19 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3727,7 +3727,7 @@ case "$target_arch2" in
>> fi
>> esac
>> case "$target_arch2" in
>> - i386|x86_64)
>> + arm|i386|x86_64)
>> echo "CONFIG_HAVE_GET_MEMORY_MAPPING=y" >> $config_target_mak
>> esac
>> if test "$target_arch2" = "ppc64" -a "$fdt" = "yes"; then
>> @@ -3746,7 +3746,7 @@ if test "$target_softmmu" = "yes" ; then
>> echo "subdir-$target: subdir-libcacard" >> $config_host_mak
>> fi
>> case "$target_arch2" in
>> - i386|x86_64)
>> + arm|i386|x86_64)
>> echo "CONFIG_HAVE_CORE_DUMP=y" >> $config_target_mak
>> esac
>> fi
>> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
>> index f447c4f..837b374 100644
>> --- a/target-arm/Makefile.objs
>> +++ b/target-arm/Makefile.objs
>> @@ -1,5 +1,5 @@
>> obj-y += arm-semi.o
>> -obj-$(CONFIG_SOFTMMU) += machine.o
>> +obj-$(CONFIG_SOFTMMU) += machine.o arch_memory_mapping.o arch_dump.o
>> obj-y += translate.o op_helper.o helper.o cpu.o
>> obj-y += neon_helper.o iwmmxt_helper.o
>>
>> diff --git a/target-arm/arch_dump.c b/target-arm/arch_dump.c
>> new file mode 100644
>> index 0000000..47a7e40
>> --- /dev/null
>> +++ b/target-arm/arch_dump.c
>> @@ -0,0 +1,59 @@
>> +#include "cpu.h"
>> +#include "cpu-all.h"
>> +#include "dump.h"
>> +#include "elf.h"
>> +
>> +typedef struct {
>> + char pad1[24];
>> + uint32_t pid;
>> + char pad2[44];
>> + uint32_t regs[18];
>> + char pad3[4];
>> +} arm_elf_prstatus;
>
> I'm guessing this is following some specification's structure layout;
> what specification?
>
>> +
>> +int cpu_write_elf64_note(write_core_dump_function f, CPUArchState *env,
>> + int cpuid, void *opaque)
>
> Should these APIs really be taking a CPUArchState* rather rather than
> an ARMCPU* ? (Andreas?)
I'm missing some context here. This is a new API? Where is it being used?
If it applies to multiple architectures and has separate
architecture-specific implementations then CPUState argument please. If
it's an ARM-internal API then, yes, ARMCPU please. If it's using common
CPU fields in common code then CPUArchState is still the most fitting today.
>> +{
>> + return -1;
>> +}
>> +
>> +int cpu_write_elf32_note(write_core_dump_function f, CPUArchState *env,
>> + int cpuid, void *opaque)
>> +{
>> + arm_elf_prstatus prstatus;
>> +
>> + memset(&prstatus, 0, sizeof(prstatus));
>> + memcpy(&(prstatus.regs), env->regs, sizeof(env->regs));
>
> This looks a bit odd -- env->regs[] is a 16 word array but
> prstatus.regs is 18 words. What are the last two words for?
>
>> + prstatus.pid = cpuid;
>> +
>> + return dump_write_elf_note(ELFCLASS32, "CORE", NT_PRSTATUS,
>> + &prstatus, sizeof(prstatus),
>> + f, opaque);
>> +}
>> +
>> +int cpu_write_elf64_qemunote(write_core_dump_function f, CPUArchState *env,
>> + void *opaque)
>> +{
>> + return -1;
>> +}
>> +
>> +int cpu_write_elf32_qemunote(write_core_dump_function f, CPUArchState *env,
>> + void *opaque)
>> +{
>> + return 0;
>> +}
>> +
>> +int cpu_get_dump_info(ArchDumpInfo *info)
>> +{
>> + info->d_machine = EM_ARM;
>> + info->d_endian = ELFDATA2LSB;
>
> ...even for big endian ARM?
>
>> + info->d_class = ELFCLASS32;
>> +
>> + return 0;
>> +}
>> +
>> +ssize_t cpu_get_note_size(int class, int machine, int nr_cpus)
>> +{
>> + return nr_cpus * dump_get_note_size(ELFCLASS32, "CORE",
>> + sizeof(arm_elf_prstatus));
>> +}
>> diff --git a/target-arm/arch_memory_mapping.c
>> b/target-arm/arch_memory_mapping.c
>> new file mode 100644
>> index 0000000..eeaaf09
>> --- /dev/null
>> +++ b/target-arm/arch_memory_mapping.c
>> @@ -0,0 +1,13 @@
>> +#include "cpu.h"
>> +#include "cpu-all.h"
>> +#include "memory_mapping.h"
>> +
>> +bool cpu_paging_enabled(CPUArchState *env)
>> +{
>> + return 0;
>> +}
What is this supposed to be? I think this is calling for a CPUClass
field that gets overridden in target-arm/cpu.c:arm_cpu_class_init()...
>> +
>> +int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
>> +{
>> + return -1;
>> +}
>
> Why do we need these null implementations and why do they
> work better than the default ones in memory_mapping-stub.c ?
>
> (memory_mapping.h could use some documentation comments
> describing the purpose and API of these functions IMHO.)
Generally I'm not happy about an API defining new global per-arch
cpu_*() functions, that conflicts with the QOM CPUState efforts.
Rabin, please keep my in the loop.
Thanks,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
