RE: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-pcie bus

[Shameerali Kolothum Thodi via <qemu-arm@nongnu.org>]

Hi Eric,

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Wednesday, March 12, 2025 4:08 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [RFC PATCH v2 05/20] hw/arm/smmuv3-accel: Associate a pxb-
> pcie bus
> 
> Hi Shameer,
> 
> 
> On 3/11/25 3:10 PM, Shameer Kolothum wrote:
> > User must associate a pxb-pcie root bus to smmuv3-accel
> > and that is set as the primary-bus for the smmu dev.
> why do we require a pxb-pcie root bus? why can't pci.0 root bus be used
> for simpler use cases (ie. I just want to passthough a NIC in
> accelerated mode). Or may pci.0 is also called a pax-pcie root bus?

The idea was since pcie.0 is the default RC with virt, leave that to cases where
we want to attach any emulated devices and use pxb-pcie based RCs for vfio-pci.

> 
> Besides, why do we put the constraint to plug on a root bus. I know that
> at this point we always plug to pci.0 but with the new -device option it
> would be possible to plug it anywhere in the pcie hierarchy. At SOC
> level can't an SMMU be plugged anywhere protecting just a few RIDs?

In my understanding normally(or atleast in the most common cases) it is attached 
to root complexes. Also IORT mappings are at the root complex level, right?

To 
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/smmuv3-accel.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> > index c327661636..1471b65374 100644
> > --- a/hw/arm/smmuv3-accel.c
> > +++ b/hw/arm/smmuv3-accel.c
> > @@ -9,6 +9,21 @@
> >  #include "qemu/osdep.h"
> >
> >  #include "hw/arm/smmuv3-accel.h"
> > +#include "hw/pci/pci_bridge.h"
> > +
> > +static int smmuv3_accel_pxb_pcie_bus(Object *obj, void *opaque)
> > +{
> > +    DeviceState *d = opaque;
> > +
> > +    if (object_dynamic_cast(obj, "pxb-pcie-bus")) {
> > +        PCIBus *bus = PCI_HOST_BRIDGE(obj->parent)->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);
> if you want to stop the recursive search I think you need to return
> something != 0 here.

Ok.
 
> I don't really understand why we don't simply set the primary-bus to
> <bus> where -device arm-smmuv3-accel, bus=<bus>? or maybe enforce that
> this bus is an actual root bus if we really need that?

The primary-bus here is actually the property of the parent SMMU device.
This one now has,

-device arm-smmuv3-accel, bus format.


> > +        }
> > +    }
> > +    return 0;
> > +}
> >
> >  static void smmu_accel_realize(DeviceState *d, Error **errp)
> >  {
> > @@ -17,6 +32,9 @@ static void smmu_accel_realize(DeviceState *d, Error
> **errp)
> >      SysBusDevice *dev = SYS_BUS_DEVICE(d);
> >      Error *local_err = NULL;
> >
> > +    object_child_foreach_recursive(object_get_root(),
> > +                                   smmuv3_accel_pxb_pcie_bus, d);
> > +
> >      object_property_set_bool(OBJECT(dev), "accel", true, &error_abort);
> >      c->parent_realize(d, &local_err);
> >      if (local_err) {
> > @@ -33,6 +51,7 @@ static void smmuv3_accel_class_init(ObjectClass
> *klass, void *data)
> >      device_class_set_parent_realize(dc, smmu_accel_realize,
> >                                      &c->parent_realize);
> >      dc->hotpluggable = false;
> > +    dc->bus_type = TYPE_PCIE_BUS;
> shouldn't it below to 3/20? It is not really related to primary_bus
> setting?

This is to set the bus property of this smmuv3-accel device. As mentioned 
above primary-bus is the property of parent TYPE_ARM_SMMU device.

Thanks,
Shameer