RE: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a PCIe RC

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicol...@nvidia.com>
> Sent: Friday, May 2, 2025 6:23 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com>
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org;
> eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
> ddut...@redhat.com; berra...@redhat.com; nath...@nvidia.com;
> mo...@nvidia.com; smost...@google.com; Linuxarm
> <linux...@huawei.com>; Wangzhou (B) <wangzh...@hisilicon.com>;
> jiangkunkun <jiangkun...@huawei.com>; Jonathan Cameron
> <jonathan.came...@huawei.com>; zhangfei....@linaro.org
> Subject: Re: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
> PCIe RC
> 
> On Fri, May 02, 2025 at 11:27:02AM +0100, Shameer Kolothum wrote:
> > Although this change does not affect functionality at present, it lays
> > the groundwork for enabling user-created SMMUv3 devices in future
> > patches
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.th...@huawei.com>
> 
> Reviewed-by: Nicolin Chen <nicol...@nvidia.com>
> 
> With some nits:
> 
> > ---
> >  hw/arm/smmuv3.c | 26 ++++++++++++++++++++++++++
> >  hw/arm/virt.c   |  3 ++-
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index
> > ab67972353..605de9b721 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -24,6 +24,7 @@
> >  #include "hw/qdev-properties.h"
> >  #include "hw/qdev-core.h"
> >  #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bridge.h"
> 
> Could probably replace the pci.h since pci_bridge.h includes it.

I think it is best not to as it is indirect inclusion.

> 
> > +static int smmuv3_pcie_bus(Object *obj, void *opaque) {
> > +    DeviceState *d = opaque;
> > +    PCIBus *bus;
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_PCI_HOST_BRIDGE)) {
> > +        return 0;
> > +    }
> > +
> > +    bus = PCI_HOST_BRIDGE(obj)->bus;
> > +    if (d->parent_bus && !strcmp(bus->qbus.name, d->parent_bus-
> >name)) {
> > +        object_property_set_link(OBJECT(d), "primary-bus", OBJECT(bus),
> > +                                 &error_abort);
> > +        /* Return non-zero as we got the bus and don't need further
> > + iteration.*/
> 
> Missing a space behind the '.'

Sure.

> 
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> 
> > @@ -1442,7 +1443,7 @@ static void create_smmu(const
> VirtMachineState *vms,
> >      }
> >      object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
> >                               &error_abort);
> > -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > +    qdev_realize_and_unref(dev, &bus->qbus, &error_fatal);
> 
> Could add a line of note in the commit message for this change?

Will do. 

Thanks,
Shameer