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

[Eric Auger <eric.auger@redhat.com>]

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?

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?
>
> 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.

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?
> +        }
> +    }
> +    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? Thanks Eric
>  }
>  
>  static const TypeInfo smmuv3_accel_type_info = {