Hi Don,
On 3/19/25 5:21 PM, Donald Dutile wrote:
>
>
> On 3/19/25 5:26 AM, Shameerali Kolothum Thodi wrote:
>> Hi Don,
>>
> Hey!
>
>>> -----Original Message-----
>>> From: Donald Dutile <ddut...@redhat.com>
>>> Sent: Tuesday, March 18, 2025 10:12 PM
>>> To: Shameerali Kolothum Thodi
>>> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
>>> qemu-devel@nongnu.org
>>> Cc: eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
>>> nicol...@nvidia.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
>>>
>>> Shameer,
>>>
>>> Hi!
>>>
>>> On 3/11/25 10:10 AM, 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.
>>>>
>>>> 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);
>>>> + }
>>>> + }
>>>> + 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;
>>>> }
>>>>
>>>> static const TypeInfo smmuv3_accel_type_info = {
>>>
>>> I am not seeing the need for a pxb-pcie bus(switch) introduced for each
>>> 'accel'.
>>> Isn't the IORT able to define different SMMUs for different RIDs?
>>> if so,
>>> itsn't that sufficient
>>> to associate (define) an SMMU<->RID association without introducing a
>>> pxb-pcie?
>>> and again, I'm not sure how that improves/enables the device<->SMMU
>>> associativity?
>>
>> Thanks for taking a look at the series. As discussed elsewhere in
>> this thread(with
>> Eric), normally in physical world (or atleast in the most common
>> cases) SMMUv3
>> is attached to PCIe Root Complex and if you take a look at the IORT
>> spec, it describes
>> association of ID mappings between a RC node and SMMUV3 node.
>>
>> And if my understanding is correct, in Qemu, only pxb-pcie allows you
>> to add
>> extra root complexes even though it is still plugged to
>> parent(pcie.0). ie, for all
>> devices downstream it acts as a root complex but still plugged into a
>> parent pcie.0.
>> This allows us to add/describe multiple "smmuv3-accel" each
>> associated with a RC.
>>
> I find the qemu statements a bit unclear here as well.
> I looked at the hot plug statement(s) in docs/pcie.txt, as I figured
> that's where dynamic
> IORT changes would be needed as well. There, it says you can hot-add
> PCIe devices to RPs,
> one has to define/add RP's to the machine model for that plug-in.
>
> Using libvirt, it could auto-add the needed RPs to do dynmaic smmuv3
> additions,
I am not sure I understand your statement here. we don't want "dynamic"
SMMUv3 instantiation. SMMUv3 is a platform device which is supposed to
be coldplugged on a pre-existing PCIe hierarchy. The SMMUv3 device is
not something that is meant to be hotplugged or hotunplugged.
To me we hijack the bus= property to provide information about the IORT
IDMAP
Thanks
Eric
> if I understand how libvirt does that today for pcie devices now (/me
> looks at danpb for feedback).
>
>> Having said that, current code only allows pxb-pcie root complexes
>> avoiding
>> the pcie.0. The idea behind this was, user can use pcie.0 with a non
>> accel SMMUv3
>> for any emulated devices avoiding the performance bottlenecks we are
>> discussing for emulated dev+smmuv3-accel cases. But based on the
>> feedback from
>> Eric and Daniel I will relax that restriction and will allow
>> association with pcie.0.
>>
> So, I think this isn't a restriction that this smmuv3 feature should
> enforce;
> lack of a proper RP or pxb-pcie will yield an invalid config
> issue/error, and
> the machine definition will be modified to meet the needs for IORT.
>
>> Thanks,
>> Shameer
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>>>> to root complexes.
>>> Feel free to enlighten me where I may have mis-read/interpreted the
>>> IORT
>>> & SMMUv3 specs.
>>>
>>> Thanks,
>>> - Don
>>>
>>
>
