Hi, Am 21.06.2013 09:44, schrieb Peter Crosthwaite: > On Fri, Jun 21, 2013 at 5:36 PM, Paolo Bonzini <pbonz...@redhat.com> 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.
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?: > > - 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. 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
