From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51206) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmFiP-0006u5-DL for qemu-devel@nongnu.org; Tue, 03 Jul 2012 22:53:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SmFiN-0004iO-6Q for qemu-devel@nongnu.org; Tue, 03 Jul 2012 22:53:16 -0400 Received: from [222.73.24.84] (port=26339 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmFiM-0004iJ-9C for qemu-devel@nongnu.org; Tue, 03 Jul 2012 22:53:15 -0400 Message-ID: <4FF3B120.6080206@cn.fujitsu.com> Date: Wed, 04 Jul 2012 10:57:36 +0800 From: Wen Congyang MIME-Version: 1.0 References: <1340213303-596-1-git-send-email-rabin@rab.in> <1340213303-596-4-git-send-email-rabin@rab.in> <4FEDA2C0.7040405@suse.de> In-Reply-To: <4FEDA2C0.7040405@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/4] target-arm: add minimal dump-guest-memory support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Rabin Vincent , Peter Maydell , qemu-devel@nongnu.org At 06/29/2012 08:42 PM, Andreas F=C3=A4rber Wrote: > Am 28.06.2012 18:46, schrieb Peter Maydell: >> On 20 June 2012 18:28, Rabin Vincent 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 >>> --- >>> configure | 4 +-- >>> target-arm/Makefile.objs | 2 +- >>> target-arm/arch=5Fdump.c | 59 ++++++++++++++++++++++++++++= ++++++++++ >>> target-arm/arch=5Fmemory=5Fmapping.c | 13 +++++++++ >>> 4 files changed, 75 insertions(+), 3 deletions(-) >>> create mode 100644 target-arm/arch=5Fdump.c >>> create mode 100644 target-arm/arch=5Fmemory=5Fmapping.c >>> >>> diff --git a/configure b/configure >>> index b68c0ca..a20ad19 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -3727,7 +3727,7 @@ case "$target=5Farch2" in >>> fi >>> esac >>> case "$target=5Farch2" in >>> - i386|x86=5F64) >>> + arm|i386|x86=5F64) >>> echo "CONFIG=5FHAVE=5FGET=5FMEMORY=5FMAPPING=3Dy" >> $config=5Ftarg= et=5Fmak >>> esac >>> if test "$target=5Farch2" =3D "ppc64" -a "$fdt" =3D "yes"; then >>> @@ -3746,7 +3746,7 @@ if test "$target=5Fsoftmmu" =3D "yes" ; then >>> echo "subdir-$target: subdir-libcacard" >> $config=5Fhost=5Fmak >>> fi >>> case "$target=5Farch2" in >>> - i386|x86=5F64) >>> + arm|i386|x86=5F64) >>> echo "CONFIG=5FHAVE=5FCORE=5FDUMP=3Dy" >> $config=5Ftarget=5Fmak >>> 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 +=3D arm-semi.o >>> -obj-$(CONFIG=5FSOFTMMU) +=3D machine.o >>> +obj-$(CONFIG=5FSOFTMMU) +=3D machine.o arch=5Fmemory=5Fmapping.o arch= =5Fdump.o >>> obj-y +=3D translate.o op=5Fhelper.o helper.o cpu.o >>> obj-y +=3D neon=5Fhelper.o iwmmxt=5Fhelper.o >>> >>> diff --git a/target-arm/arch=5Fdump.c b/target-arm/arch=5Fdump.c >>> new file mode 100644 >>> index 0000000..47a7e40 >>> --- /dev/null >>> +++ b/target-arm/arch=5Fdump.c >>> @@ -0,0 +1,59 @@ >>> +#include "cpu.h" >>> +#include "cpu-all.h" >>> +#include "dump.h" >>> +#include "elf.h" >>> + >>> +typedef struct { >>> + char pad1[24]; >>> + uint32=5Ft pid; >>> + char pad2[44]; >>> + uint32=5Ft regs[18]; >>> + char pad3[4]; >>> +} arm=5Felf=5Fprstatus; >> >> I'm guessing this is following some specification's structure layout; >> what specification? >> >>> + >>> +int cpu=5Fwrite=5Felf64=5Fnote(write=5Fcore=5Fdump=5Ffunction f, CPUAr= chState *env, >>> + int cpuid, void *opaque) >> >> Should these APIs really be taking a CPUArchState* rather rather than >> an ARMCPU* ? (Andreas?) >=20 > I'm missing some context here. This is a new API? Where is it being used? >=20 > 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 tod= ay. It applies to multiple architectures, and use arch-spec fileds(For example: register's value). Which is better? CPUArchState or XXXCPUState? >=20 >>> +{ >>> + return -1; >>> +} >>> + >>> +int cpu=5Fwrite=5Felf32=5Fnote(write=5Fcore=5Fdump=5Ffunction f, CPUAr= chState *env, >>> + int cpuid, void *opaque) >>> +{ >>> + arm=5Felf=5Fprstatus 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 =3D cpuid; >>> + >>> + return dump=5Fwrite=5Felf=5Fnote(ELFCLASS32, "CORE", NT=5FPRSTATUS, >>> + &prstatus, sizeof(prstatus), >>> + f, opaque); >>> +} >>> + >>> +int cpu=5Fwrite=5Felf64=5Fqemunote(write=5Fcore=5Fdump=5Ffunction f, C= PUArchState *env, >>> + void *opaque) >>> +{ >>> + return -1; >>> +} >>> + >>> +int cpu=5Fwrite=5Felf32=5Fqemunote(write=5Fcore=5Fdump=5Ffunction f, C= PUArchState *env, >>> + void *opaque) >>> +{ >>> + return 0; >>> +} >>> + >>> +int cpu=5Fget=5Fdump=5Finfo(ArchDumpInfo *info) >>> +{ >>> + info->d=5Fmachine =3D EM=5FARM; >>> + info->d=5Fendian =3D ELFDATA2LSB; >> >> ...even for big endian ARM? >> >>> + info->d=5Fclass =3D ELFCLASS32; >>> + >>> + return 0; >>> +} >>> + >>> +ssize=5Ft cpu=5Fget=5Fnote=5Fsize(int class, int machine, int nr=5Fcpu= s) >>> +{ >>> + return nr=5Fcpus * dump=5Fget=5Fnote=5Fsize(ELFCLASS32, "CORE", >>> + sizeof(arm=5Felf=5Fprstatus)); >>> +} >>> diff --git a/target-arm/arch=5Fmemory=5Fmapping.c b/target-arm/arch=5Fm= emory=5Fmapping.c >>> new file mode 100644 >>> index 0000000..eeaaf09 >>> --- /dev/null >>> +++ b/target-arm/arch=5Fmemory=5Fmapping.c >>> @@ -0,0 +1,13 @@ >>> +#include "cpu.h" >>> +#include "cpu-all.h" >>> +#include "memory=5Fmapping.h" >>> + >>> +bool cpu=5Fpaging=5Fenabled(CPUArchState *env) >>> +{ >>> + return 0; >>> +} >=20 > What is this supposed to be? I think this is calling for a CPUClass > field that gets overridden in target-arm/cpu.c:arm=5Fcpu=5Fclass=5Finit()= ... No, it is for the monitor command dump-guest-memory's option -p. If the user specifies this option, we will write guest physicall address and virtual address mapping in the core(PT=5FLOAD). If the guest runs in real mode(not use paging), the physical address is equal to virtual address. otherwise, we should walk page table to get this mapping. This API can tell us whether the guest runs in real mode or not. Thanks Wen Congyang >=20 >>> + >>> +int cpu=5Fget=5Fmemory=5Fmapping(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=5Fmapping-stub.c ? >> >> (memory=5Fmapping.h could use some documentation comments >> describing the purpose and API of these functions IMHO.) >=20 > Generally I'm not happy about an API defining new global per-arch > cpu=5F*() functions, that conflicts with the QOM CPUState efforts. >=20 > Rabin, please keep my in the loop. >=20 > Thanks, > Andreas >=20 =