From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42226) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uq0OE-00030B-27 for qemu-devel@nongnu.org; Fri, 21 Jun 2013 08:24:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uq0OC-0001qK-GL for qemu-devel@nongnu.org; Fri, 21 Jun 2013 08:24:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31750) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uq0OC-0001q7-8q for qemu-devel@nongnu.org; Fri, 21 Jun 2013 08:24:28 -0400 Message-ID: <51C445F1.3040607@redhat.com> Date: Fri, 21 Jun 2013 14:24:17 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <51C4028E.20301@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Hu Tao , =?ISO-8859-1?Q?Andreas_F=E4rber?= , "qemu-devel@nongnu.org Developers" Il 21/06/2013 09:44, Peter Crosthwaite ha scritto: > Hi Paolo, > > On Fri, Jun 21, 2013 at 5:36 PM, Paolo Bonzini wrote: >> 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? >> > > Not really, > > 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. > > Slightly off topic, should we perhaps use the object canonical patch > for the device as the memory region name string?: > > - 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)); > > Its unambiguous and solves the problem where you have two-of-a-kind > devices in the one system and you need to differentiate. I think the memory region name should be just a descriptive text, for example "mmio". I'm going to add an owner Object to MemoryRegion, and the owner+name should be unique (except that some regions that are created by the board, typically RAM, and thus do not have an owner). (Which also means that the less you touch the memory_region_init* lines, the best. My patch has to touch all 800 of them!!!). Paolo