On 3/12/25 5:34 PM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.au...@redhat.com>
>> Sent: Wednesday, March 12, 2025 4:08 PM
>> To: Shameerali Kolothum Thodi
>> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
>> qemu-devel@nongnu.org
>> Cc: peter.mayd...@linaro.org; j...@nvidia.com; nicol...@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: [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.
yes but for simpler use case you may not want the extra pain to
instantiate a pxb-pcie device. Actually libvirt does not instantiate it
by default.
>
>> 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?
Yes I do agree the IORT describes ID mappings between RC and SMMU but
the actual ID mappings allow you to be much more precise and state that
a given SMMU only translates few RIDs within that RID space. If you
force the device bus to be a root bus you can't model that anymore.
Eric
>
> To
>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.th...@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
