From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57713) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkE5E-0004EE-7o for qemu-devel@nongnu.org; Wed, 05 Jun 2013 09:49:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UkE5A-0003ba-1p for qemu-devel@nongnu.org; Wed, 05 Jun 2013 09:49:00 -0400 Received: from cantor2.suse.de ([195.135.220.15]:35401 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkE59-0003az-O6 for qemu-devel@nongnu.org; Wed, 05 Jun 2013 09:48:55 -0400 Message-ID: <51AF41C4.8030408@suse.de> Date: Wed, 05 Jun 2013 15:48:52 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1369926481-20720-1-git-send-email-afaerber@suse.de> <1369926481-20720-10-git-send-email-afaerber@suse.de> <20130531101405.4605417e@redhat.com> In-Reply-To: <20130531101405.4605417e@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH qom-cpu v3 9/9] memory_mapping: Change qemu_get_guest_memory_mapping() semantics List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: pbonzini@redhat.com, qiaonuohan@cn.fujitsu.com, qemu-devel@nongnu.org Am 31.05.2013 16:14, schrieb Luiz Capitulino: > On Thu, 30 May 2013 17:08:01 +0200 > Andreas F=C3=A4rber wrote: >=20 >> Previously it would search for the first CPU with paging enabled and >> retrieve memory mappings from this and any following CPUs or return an >> error if that fails. >> >> Instead walk all CPUs and if paging is enabled retrieve the memory >> mappings. Fail only if retrieving memory mappings on a CPU with paging >> enabled fails. >> >> This not only allows to more easily use one qemu_for_each_cpu() instea= d >> of open-coding two CPU loops and drops find_first_paging_enabled_cpu() >> but removes the implicit assumption of heterogeneity between CPUs n..N >> in having paging enabled. >> >> Cc: Wen Congyang >> Signed-off-by: Andreas F=C3=A4rber >> --- >> memory_mapping.c | 42 +++++++++++++++++++++++------------------- >> 1 file changed, 23 insertions(+), 19 deletions(-) >> >> diff --git a/memory_mapping.c b/memory_mapping.c >> index 481530a..55ac2b8 100644 >> --- a/memory_mapping.c >> +++ b/memory_mapping.c >> @@ -165,35 +165,39 @@ void memory_mapping_list_init(MemoryMappingList = *list) >> QTAILQ_INIT(&list->head); >> } >> =20 >> -static CPUArchState *find_paging_enabled_cpu(CPUArchState *start_cpu) >> +typedef struct GetGuestMemoryMappingData { >> + MemoryMappingList *list; >> + int ret; >> +} GetGuestMemoryMappingData; >> + >> +static void qemu_get_one_guest_memory_mapping(CPUState *cpu, void *da= ta) >> { >> - CPUArchState *env; >> + GetGuestMemoryMappingData *s =3D data; >> + int ret; >> =20 >> - for (env =3D start_cpu; env !=3D NULL; env =3D env->next_cpu) { >> - if (cpu_paging_enabled(ENV_GET_CPU(env))) { >> - return env; >> - } >> + if (!cpu_paging_enabled(cpu) || s->ret =3D=3D -1) { >> + return; >> + } >> + ret =3D cpu_get_memory_mapping(cpu, s->list); >> + if (ret < 0) { >> + s->ret =3D -1; >> + } else { >> + s->ret =3D 0; >> } >=20 > Does cpu_get_memory_mapping() ever return a positive value or a negativ= e > value !=3D -1 ? >=20 > It it doesn't, I'd do: >=20 > s->ret =3D cpu_get_memory_mapping(); > assert(s->ret =3D=3D 0 || s->ret =3D=3D -1); This no longer applies after returning an Error* from cpu_get_memory_mapping() instead. :) >=20 >> - >> - return NULL; >> } >> =20 >> int qemu_get_guest_memory_mapping(MemoryMappingList *list) >> { >> - CPUArchState *env, *first_paging_enabled_cpu; >> + GetGuestMemoryMappingData s =3D { >> + .list =3D list, >> + .ret =3D -2, >> + }; >> RAMBlock *block; >> ram_addr_t offset, length; >> - int ret; >> =20 >> - first_paging_enabled_cpu =3D find_paging_enabled_cpu(first_cpu); >> - if (first_paging_enabled_cpu) { >> - for (env =3D first_paging_enabled_cpu; env !=3D NULL; env =3D= env->next_cpu) { >> - ret =3D cpu_get_memory_mapping(ENV_GET_CPU(env), list); >> - if (ret < 0) { >> - return -1; >> - } >> - } >> - return 0; >> + qemu_for_each_cpu(qemu_get_one_guest_memory_mapping, &s); >> + if (s.ret !=3D -2) { >> + return s.ret; >> } >=20 > I see we ignore error in dump_init(). Doesn't matter today because > x86_cpu_get_memory_mapping() never fails. But as you're doing this clea= nup, > shouldn't you add proper error handling? This patch is not needed for arm/s390x, so we can easily postpone it. I included it here for early feedback since my series building on this still needs to be tested and tweaked for bsd-user. I figure passing through Error** would be the logical follow-up here? Yet s.ret is being reused to check if any CPU provided any mappings at all - we could instead do two loops, one for checking if any CPU has paging enabled and then one passing out any Error* and propagating that. Thanks, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg