Hi Shameer,
On 5/6/25 11:07 AM, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.au...@redhat.com>
>> Sent: Monday, May 5, 2025 9:19 AM
>> To: Donald Dutile <ddut...@redhat.com>; Shameerali Kolothum Thodi
>> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
>> qemu-devel@nongnu.org; Markus Armbruster <arm...@redhat.com>;
>> Peter Maydell <peter.mayd...@linaro.org>; Daniel P. Berrange
>> <berra...@redhat.com>; Alex Bennée <alex.ben...@linaro.org>
>> Cc: 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: [PATCH v2 1/6] hw/arm/smmuv3: Add support to associate a
>> PCIe RC
>>
>> Hi,
>> On 5/2/25 8:16 PM, Donald Dutile wrote:
>>>
>>> On 5/2/25 6:27 AM, 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>
>>>> ---
>>>> 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"
>>>> #include "cpu.h"
>>>> #include "exec/target_page.h"
>>>> #include "trace.h"
>>>> @@ -1874,6 +1875,25 @@ static void smmu_reset_exit(Object *obj,
>>>> ResetType type)
>>>> smmuv3_init_regs(s);
>>>> }
>>>> +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.*/
>>>> + return 1;
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> static void smmu_realize(DeviceState *d, Error **errp)
>>>> {
>>>> SMMUState *sys = ARM_SMMU(d);
>>>> @@ -1882,6 +1902,10 @@ static void smmu_realize(DeviceState *d,
>> Error
>>>> **errp)
>>>> SysBusDevice *dev = SYS_BUS_DEVICE(d);
>>>> Error *local_err = NULL;
>>>> + if (!object_property_get_link(OBJECT(d), "primary-bus",
>>>> &error_abort)) {
>>>> + object_child_foreach_recursive(object_get_root(),
>>>> smmuv3_pcie_bus, d);
>>>> + }
>>>> +
>>>> c->parent_realize(d, &local_err);
>>>> if (local_err) {
>>>> error_propagate(errp, local_err);
>>>> @@ -1996,6 +2020,8 @@ static void smmuv3_class_init(ObjectClass
>>>> *klass, const void *data)
>>>> device_class_set_parent_realize(dc, smmu_realize,
>>>> &c->parent_realize);
>>>> device_class_set_props(dc, smmuv3_properties);
>>>> + dc->hotpluggable = false;
>>>> + dc->bus_type = TYPE_PCIE_BUS;
>>> Does this force legacy SMMUv3 to be tied to a PCIe bus now?
>>> if so, will that break some existing legacy smmuv3 configs?, i.e.,
>>> virtio-scsi attached to a legacy smmuv3.
>> Previously the SMMU was already always attached to a PCI primary-bus
>> (vms->bus ie. pci0). virtio-scsi-pci is the device being protected. The
>> SMMU is not able to protect platforms devices atm.
>>
>> My only concern is we are highjacking the "bus" prop to record the bus
>> hierarchy the SMMU is protecting. While the SMMU is a platform device
>> and does not inherit the PCI device base class its bus type becomes
>> "TYPE_PCIE_BUS". So in terms of qom hierachy is is seen as a PCI device
>> now? I don't know if it is a problem. An alternative could be to keep
>> the bus pointer and type as it was before and introduce a primary-bus
>> property. Adding Markus, Peter, Daniel and Alex in to.
> Yes. The SMMUV3 is a platform device and we are making the bus type
> here as PCIE which is a bit odd. As replied elsewhere in this thread,
> in the initial RFC we had a specific "pci-bus" property associated with
> SMMUv3 device,
> Eg:
> -device arm-smmuv3,pci-bus=pcie.0,...
>
> But then there were feedback that, it is more intuitive and easy for libvirt
> if we can just use the default "bus" property associated with a Qemu device.
> Hence the change.
I also think that using the bus property was natural. It is only when
you look at the actual implementation and potential implications on
hierarchy introspections that it may be frown upon. That's why I call
for specialists feedback ;-)
>
>> At some point it was envisionned to support protected platform devices
>> (I think this was need for CCA). My fear is that if we turn the bus type
>> to PCIE it may be difficult to extend the support to non PCIe protected
>> devices. The SMMU shall remain a platform device being able to protect
>> either PCI devices and, in the future, platform devices.
> Ok. So in the case of platform device how do we envisage to specify the "bus"?
>
> -device arm-smmv3, primary-bus=sysbus
The problem is on a SOC not all platform devices are protected by an
SMMU. One needs to be more precise I think
>
> Or something like having a SMMUv3 dev without any bus and each platform
> device has to specify the SMMUv3?
> Eg:
>
> -device arm-smmv3,id=smmuv3.3
> -device xxxx,smmuv3= smmuv3.3
Yeah I think this would look that way instead.
The last attempt was
https://lore.kernel.org/all/20210902081429.140293-1-chunming_li1...@163.com/
if I remember correctly but it did not go further. It defined a
peri-sid-map[%d] prop array to define SIDs but I don't really get how it
defines which device is protected.
> If later, I think we can stick to current one.
That's also my pov.
Cheers
Eric
>
> Thanks,
> Shameer
