Am 26.06.2013 23:57, schrieb Jean-Christophe DUBOIS: > On 06/26/2013 11:18 PM, Paolo Bonzini wrote: >> Il 26/06/2013 23:13, Jean-Christophe DUBOIS ha scritto: >>> On 06/26/2013 08:58 PM, Jean-Christophe DUBOIS wrote: >>>> On 06/26/2013 09:21 AM, Peter Crosthwaite wrote: >>>>> Hi >>>>> >>>>> On Wed, Jun 26, 2013 at 4:56 PM, Paolo Bonzini <pbonz...@redhat.com> >>>>> wrote: >>>>>> Il 25/06/2013 22:53, Peter Maydell ha scritto: >>>>>>> On 25 June 2013 19:42, Paolo Bonzini <pbonz...@redhat.com> wrote: >>>>>>>> Il 25/06/2013 20:21, Peter Maydell ha scritto: >>>>>>>>> @@ -416,7 +513,7 @@ static int imx_timerg_init(SysBusDevice *dev) >>>>>>>>> >>>>>>>>> sysbus_init_irq(dev, &s->irq); >>>>>>>>> memory_region_init_io(&s->iomem, &imx_timerg_ops, >>>>>>>>> - s, "imxg-timer", >>>>>>>>> + s, TYPE_IMX_GPT, >>>>>>>>> 0x00001000); >>>>>>>>> sysbus_init_mmio(dev, &s->iomem); >>>>>>>>> >>>>>>>> There was some agreement that this is not a good change. >>>>>>> I agree (and more so regarding the use of the macro in the >>>>>>> vmstate name), but nobody actually posted any comment to >>>>>>> that effect against any of the versions of this patch that >>>>>>> got sent out for review... >>>>>> Yeah, the timing was bad... Can you post a revert, though? >>>>>> >>>>> The original string being replaced was a poor choice as well. IIUC the >>>>> consensus was string field of the memory regions is supposed to >>>>> indicate the purpose of the memory region for the device. Good >>>>> examples would be "Control regs" or "MMIO". Naming the memory region >>>>> after the device type is a redundancy as that info will come via >>>>> memory region owners. >>>>> >>>>> So rather than revert could you just choose something more descriptive? >>>> Peter (Maydell), >>>> >>>> How do you want to work this out? >>>> >>>> Do you revert it and we start over? >>>> >>>> Or should I provide a patch on top of the actual file to change the >>>> "wrong name"? >>>> >>>> Please advise. >>> I browsed through the various timer implementations in the hw/timer >>> directory and I was not able to spot one that would follow the >>> convention you indicated. >>> >>> Could you point me to a "good citizen" example? >> Here is one example (hw/pci-host/piix.c): >> >> memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s, >> "pci-conf-idx", 4); >> sysbus_add_io(dev, 0xcf8, &s->conf_mem); >> sysbus_init_ioports(&s->busdev, 0xcf8, 4); >> >> memory_region_init_io(&s->data_mem, &pci_host_data_le_ops, s, >> "pci-conf-data", 4); >> sysbus_add_io(dev, 0xcfc, &s->data_mem); >> sysbus_init_ioports(&s->busdev, 0xcfc, 4); >> >> Just reverting the changes to memory regions and vmstate names is enough >> for now. > > I tried to change the memory region name for the timer registers to > something not prepended wit the device name (I choose "regs") and here > is the result in the qemu console (I changes both EPIT and GPT timers). > We went from: > > (qemu) info mtree > memory > 0000000000000000-7ffffffffffffffe (prio 0, RW): system > 000000001fffc000-000000001fffffff (prio 0, RW): kzm.sram > 0000000043f90000-0000000043f90fff (prio 0, RW): imx-serial > 0000000043f94000-0000000043f94fff (prio 0, RW): imx-serial > 0000000053f80000-0000000053f80fff (prio 0, RW): imx_ccm > 0000000053f90000-0000000053f90fff (prio 0, RW): imx.gpt > 0000000053f94000-0000000053f94fff (prio 0, RW): imx.epit > 0000000053f98000-0000000053f98fff (prio 0, RW): imx.epit > 0000000068000000-0000000068000fff (prio 0, RW): imx_avic > 0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram > 0000000088000000-000000008bffffff (prio 0, RW): alias ram.alias > @kzm.ram 0000000000000000-0000000003ffffff > 00000000b6000000-00000000b60000ff (prio 0, RW): lan9118-mmio > I/O > 0000000000000000-000000000000ffff (prio 0, RW): io > aliases > kzm.ram > 0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram > (qemu) > > to: > > (qemu) info mtree > memory > 0000000000000000-7ffffffffffffffe (prio 0, RW): system > 000000001fffc000-000000001fffffff (prio 0, RW): kzm.sram > 0000000043f90000-0000000043f90fff (prio 0, RW): imx-serial > 0000000043f94000-0000000043f94fff (prio 0, RW): imx-serial > 0000000053f80000-0000000053f80fff (prio 0, RW): imx_ccm > 0000000053f90000-0000000053f90fff (prio 0, RW): regs > 0000000053f94000-0000000053f94fff (prio 0, RW): regs > 0000000053f98000-0000000053f98fff (prio 0, RW): regs > 0000000068000000-0000000068000fff (prio 0, RW): imx_avic > 0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram > 0000000088000000-000000008bffffff (prio 0, RW): alias ram.alias > @kzm.ram 0000000000000000-0000000003ffffff > 00000000b6000000-00000000b60000ff (prio 0, RW): lan9118-mmio > I/O > 0000000000000000-000000000000ffff (prio 0, RW): io > aliases > kzm.ram > 0000000080000000-0000000083ffffff (prio 0, RW): kzm.ram > (qemu) > > > Names don't feel really useful in the second case as they are > indistinguishable. Is the consensus around "generic" names (like MMIO or > "Ctrl regs") without adding reference to the device a good one for all > platforms?
Paolo's series adds the MemoryRegion owner but hasn't been merged yet. Just revert the names so that Paolo's series applies cleanly. Any name changes can then still be done as follow-ups. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
