Re: [PATCH] sabre: use object_initialize_child() for iommu child object

[Mark Cave-Ayland]

On 25/10/2020 11:41, Philippe Mathieu-Daudé wrote:

> On 10/25/20 12:11 PM, Mark Cave-Ayland wrote:
> > On 21/10/2020 12:43, Mark Cave-Ayland wrote:
> >
> > > Store the child object directly within the sabre object rather than using
> > > link properties.
> > >
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > ---
> > >   hw/pci-host/sabre.c         | 10 ++++------
> > >   hw/sparc64/sun4u.c          |  8 +-------
> > >   include/hw/pci-host/sabre.h |  2 +-
> > >   3 files changed, 6 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/hw/pci-host/sabre.c b/hw/pci-host/sabre.c
> > > index f41a0cc301..aaa93acd6e 100644
> > > --- a/hw/pci-host/sabre.c
> > > +++ b/hw/pci-host/sabre.c
> > > @@ -383,9 +383,10 @@ static void sabre_realize(DeviceState *dev, Error **errp)
> > >       pci_create_simple(phb->bus, 0, TYPE_SABRE_PCI_DEVICE);
> > >       /* IOMMU */
> > > +    sysbus_realize(SYS_BUS_DEVICE(&s->iommu), &error_fatal);
> > >       memory_region_add_subregion_overlap(&s->sabre_config, 0x200,
> > > -                    sysbus_mmio_get_region(SYS_BUS_DEVICE(s->iommu), 0), 1);
> > > -    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, s->iommu);
> > > +                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->iommu), 0), 1);
> > > +    pci_setup_iommu(phb->bus, sabre_pci_dma_iommu, &s->iommu);
> > >       /* APB secondary busses */
> > >       pci_dev = pci_new_multifunction(PCI_DEVFN(1, 0), true,
> > > @@ -422,10 +423,7 @@ static void sabre_init(Object *obj)
> > >       s->pci_irq_in = 0ULL;
> > >       /* IOMMU */
> > > -    object_property_add_link(obj, "iommu", TYPE_SUN4U_IOMMU,
> > > -                             (Object **) &s->iommu,
> > > -                             qdev_prop_allow_set_link_before_realize,
> > > -                             0);
> > > +    object_initialize_child(obj, "iommu", &s->iommu, TYPE_SUN4U_IOMMU);
> > >       /* sabre_config */
> > >       memory_region_init_io(&s->sabre_config, OBJECT(s), &sabre_config_ops, s,
> > > diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> > > index 2f8fc670cf..a33f1eccfd 100644
> > > --- a/hw/sparc64/sun4u.c
> > > +++ b/hw/sparc64/sun4u.c
> > > @@ -562,7 +562,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> > >       PCIBus *pci_bus, *pci_busA, *pci_busB;
> > >       PCIDevice *ebus, *pci_dev;
> > >       SysBusDevice *s;
> > > -    DeviceState *iommu, *dev;
> > > +    DeviceState *dev;
> > >       FWCfgState *fw_cfg;
> > >       NICInfo *nd;
> > >       MACAddr macaddr;
> > > @@ -571,10 +571,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> > >       /* init CPUs */
> > >       cpu = sparc64_cpu_devinit(machine->cpu_type, hwdef->prom_addr);
> > > -    /* IOMMU */
> > > -    iommu = qdev_new(TYPE_SUN4U_IOMMU);
> > > -    sysbus_realize_and_unref(SYS_BUS_DEVICE(iommu), &error_fatal);
> > > -
> > >       /* set up devices */
> > >       ram_init(0, machine->ram_size);
> > > @@ -584,8 +580,6 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
> > >       sabre = SABRE(qdev_new(TYPE_SABRE));
> > >       qdev_prop_set_uint64(DEVICE(sabre), "special-base", PBM_SPECIAL_BASE);
> > >       qdev_prop_set_uint64(DEVICE(sabre), "mem-base", PBM_MEM_BASE);
> > > -    object_property_set_link(OBJECT(sabre), "iommu", OBJECT(iommu),
> > > -                             &error_abort);
> > >       sysbus_realize_and_unref(SYS_BUS_DEVICE(sabre), &error_fatal);
> > >       /* sabre_config */
> > > diff --git a/include/hw/pci-host/sabre.h b/include/hw/pci-host/sabre.h
> > > index 01190241bb..05bf741cde 100644
> > > --- a/include/hw/pci-host/sabre.h
> > > +++ b/include/hw/pci-host/sabre.h
> > > @@ -34,7 +34,7 @@ struct SabreState {
> > >       MemoryRegion pci_mmio;
> > >       MemoryRegion pci_ioport;
> > >       uint64_t pci_irq_in;
> > > -    IOMMUState *iommu;
> > > +    IOMMUState iommu;
> > >       PCIBridge *bridgeA;
> > >       PCIBridge *bridgeB;
> > >       uint32_t pci_control[16];
> >
> > No further comments (and I'm happier that this is a better solution than having an 
> > "optional" link property) so I've applied this to my qemu-sparc branch.
>
> Sorry I had this patch tagged for review but am having trouble
> managing that folder. This is certainly better, thanks for this
> cleanup.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Looks like this patch exposes another issue related to QOM lifecycle, so I'm dropping 
it from my qemu-sparc branch for now.


ATB,

Mark.