From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56679) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UiQ5w-0002EV-Sl for qemu-devel@nongnu.org; Fri, 31 May 2013 10:14:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UiQ5v-0003m8-DE for qemu-devel@nongnu.org; Fri, 31 May 2013 10:14:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41264) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UiQ5v-0003m3-5i for qemu-devel@nongnu.org; Fri, 31 May 2013 10:14:15 -0400 Date: Fri, 31 May 2013 10:14:05 -0400 From: Luiz Capitulino Message-ID: <20130531101405.4605417e@redhat.com> In-Reply-To: <1369926481-20720-10-git-send-email-afaerber@suse.de> References: <1369926481-20720-1-git-send-email-afaerber@suse.de> <1369926481-20720-10-git-send-email-afaerber@suse.de> Mime-Version: 1.0 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: Andreas =?UTF-8?B?RsOkcmJlcg==?= Cc: pbonzini@redhat.com, qiaonuohan@cn.fujitsu.com, qemu-devel@nongnu.org On Thu, 30 May 2013 17:08:01 +0200 Andreas F=C3=A4rber wrote: > 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. >=20 > 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. >=20 > This not only allows to more easily use one qemu_for_each_cpu() instead > 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. >=20 > Cc: Wen Congyang > Signed-off-by: Andreas F=C3=A4rber > --- > memory_mapping.c | 42 +++++++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 19 deletions(-) >=20 > 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 *li= st) > 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 *data) > { > - 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; > } Does cpu_get_memory_mapping() ever return a positive value or a negative value !=3D -1 ? It it doesn't, I'd do: s->ret =3D cpu_get_memory_mapping(); assert(s->ret =3D=3D 0 || s->ret =3D=3D -1); > - > - 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 en= v->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; > } 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 cleanup, shouldn't you add proper error handling? > =20 > /*