From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41513) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpwtZ-00015a-MY for qemu-devel@nongnu.org; Fri, 21 Jun 2013 04:40:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UpwtY-0001mz-Ga for qemu-devel@nongnu.org; Fri, 21 Jun 2013 04:40:37 -0400 Received: from cantor2.suse.de ([195.135.220.15]:38165 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpwtY-0001ms-7J for qemu-devel@nongnu.org; Fri, 21 Jun 2013 04:40:36 -0400 Message-ID: <51C41182.9000704@suse.de> Date: Fri, 21 Jun 2013 10:40:34 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <51C4028E.20301@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] Object cast macro change-pattern automation. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Paolo Bonzini , "qemu-devel@nongnu.org Developers" , Hu Tao Hi, Am 21.06.2013 09:44, schrieb Peter Crosthwaite: > On Fri, Jun 21, 2013 at 5:36 PM, Paolo Bonzini wr= ote: >> Il 21/06/2013 06:21, Peter Crosthwaite ha scritto: >>> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c >>> index 0c39cff..ae09170 100644 >>> --- a/hw/timer/xilinx_timer.c >>> +++ b/hw/timer/xilinx_timer.c >>> @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) >>> ptimer_set_freq(xt->ptimer, t->freq_hz); >>> } >>> >>> - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", >>> + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER= , >>> R_MAX * 4 * num_timers(t)); >>> sysbus_init_mmio(dev, &t->mmio); >>> return 0; >>> @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass >> >> Isn't this a false positive? >> >=20 > Not really, >=20 > For consistency I just the TYPE_FOO for the memory region name, so I > don't see why that shouldn't be macrofiied along with the rest. It is > quite deliberately the same - please advise if this is wrong. Same for > the VMSD name. I concur with Paolo: The MemoryRegion name is free text that can be changed as desired and doesn't need to match the type name. The TYPE_* constant serves to keep consistency between usages of the string literal but does not prohibit changing its value. The VMStateDescription should thus not use the TYPE_* constant because it must not ever change for live migration. Renaming a type is a valid thing to do if there is no command line compatibility to keep (think board-level "xlnx.foo" -> "xlnx,foo"). > Slightly off topic, should we perhaps use the object canonical patch > for the device as the memory region name string?: >=20 > - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", > + memory_region_init_io(&t->mmio, &timer_ops, t, > object_get_canonical_path(OBJECT(dev)), > R_MAX * 4 * num_timers(t)); >=20 > Its unambiguous and solves the problem where you have two-of-a-kind > devices in the one system and you need to differentiate. MemoryRegion names can well be changed, but why hardcode a path there? We could just as well associate the MemoryRegion with the Object and let info mtree obtain the current path at runtime. The number of MMIO regions per device is not limited to 1, so the interesting information is what the region is for, not just whom it is from. If there's only one region and the object is associated with it, we could make a NULL argument default to the object's type or something. Anyway, since the path of most devices will look like /machine/unassigned/device[42] I don't see much added value in using it... Anthony once had patches to QOM'ify MemoryRegions, at which point they would probably be child<>s of the device; either Object::parent (controversial) or a backlink could be used to go from MemoryRegion to containing object then. Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg