Hi Shameer,
On 11/14/24 09:20, Shameerali Kolothum Thodi wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.au...@redhat.com>
>> Sent: Wednesday, November 13, 2024 5:12 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; 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 2/5] hw/arm/smmuv3: Add initial support for
>> SMMUv3 Nested device
>>
>> Hi Shameer,
>> On 11/8/24 13:52, Shameer Kolothum wrote:
>>> Based on SMMUv3 as a parent device, add a user-creatable smmuv3-
>> nested
>>> device. Subsequent patches will add support to specify a PCI bus for
>>> this device.
>>>
>>> Currently only supported for "virt", so hook up the sybus mem & irq
>>> for that as well.
>>>
>>> No FDT support is added for now.
>>>
>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.th...@huawei.com>
>>> ---
>>> hw/arm/smmuv3.c | 34 ++++++++++++++++++++++++++++++++++
>>> hw/arm/virt.c | 31 +++++++++++++++++++++++++++++--
>>> hw/core/sysbus-fdt.c | 1 +
>>> include/hw/arm/smmuv3.h | 15 +++++++++++++++
>>> include/hw/arm/virt.h | 6 ++++++
>>> 5 files changed, 85 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index
>>> 2101031a8f..0033eb8125 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -2201,6 +2201,19 @@ static void smmu_realize(DeviceState *d, Error
>> **errp)
>>> smmu_init_irq(s, dev);
>>> }
>>>
>>> +static void smmu_nested_realize(DeviceState *d, Error **errp) {
>>> + SMMUv3NestedState *s_nested = ARM_SMMUV3_NESTED(d);
>> nit: s/s_nested/ns or just s?
>>> + SMMUv3NestedClass *c =
>> ARM_SMMUV3_NESTED_GET_CLASS(s_nested);
>>> + Error *local_err = NULL;
>>> +
>>> + c->parent_realize(d, &local_err);
>> I think it is safe to use errp directly here.
> Ok.
>
>>> + if (local_err) {
>>> + error_propagate(errp, local_err);
>>> + return;
>>> + }
>>> +}
>>> +
>>> static const VMStateDescription vmstate_smmuv3_queue = {
>>> .name = "smmuv3_queue",
>>> .version_id = 1,
>>> @@ -2299,6 +2312,18 @@ static void smmuv3_class_init(ObjectClass
>> *klass, void *data)
>>> device_class_set_props(dc, smmuv3_properties); }
>>>
>>> +static void smmuv3_nested_class_init(ObjectClass *klass, void *data)
>>> +{
>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>> + SMMUv3NestedClass *c = ARM_SMMUV3_NESTED_CLASS(klass);
>>> +
>>> + dc->vmsd = &vmstate_smmuv3;
>>> + device_class_set_parent_realize(dc, smmu_nested_realize,
>>> + &c->parent_realize);
>>> + dc->user_creatable = true;
>>> + dc->hotpluggable = false;
>>> +}
>>> +
>>> static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>>> IOMMUNotifierFlag old,
>>> IOMMUNotifierFlag new, @@
>>> -2337,6 +2362,14 @@ static void
>> smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
>>> imrc->notify_flag_changed = smmuv3_notify_flag_changed; }
>>>
>>> +static const TypeInfo smmuv3_nested_type_info = {
>>> + .name = TYPE_ARM_SMMUV3_NESTED,
>>> + .parent = TYPE_ARM_SMMUV3,
>>> + .instance_size = sizeof(SMMUv3NestedState),
>>> + .class_size = sizeof(SMMUv3NestedClass),
>>> + .class_init = smmuv3_nested_class_init,
>>> +};
>>> +
>>> static const TypeInfo smmuv3_type_info = {
>>> .name = TYPE_ARM_SMMUV3,
>>> .parent = TYPE_ARM_SMMU,
>>> @@ -2355,6 +2388,7 @@ static const TypeInfo
>>> smmuv3_iommu_memory_region_info = { static void
>>> smmuv3_register_types(void) {
>>> type_register(&smmuv3_type_info);
>>> + type_register(&smmuv3_nested_type_info);
>>> type_register(&smmuv3_iommu_memory_region_info);
>>> }
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c index
>>> 780bcff77c..38075f9ab2 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -181,6 +181,7 @@ static const MemMapEntry base_memmap[] = {
>>> [VIRT_PVTIME] = { 0x090a0000, 0x00010000 },
>>> [VIRT_SECURE_GPIO] = { 0x090b0000, 0x00001000 },
>>> [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
>>> + [VIRT_SMMU_NESTED] = { 0x0b000000, 0x01000000 },
>> I agree with Mostafa that the _NESTED terminology may not be the best
>> choice.
> Yes. Agree.
Nicolin's suggestion to use a reference to HW acceleration looks
sensible to me.
>
>> The motivation behind that multi-instance attempt, as introduced in
>> https://lore.kernel.org/all/ZEcT%2F7erkhHDaNvD@Asurada-Nvidia/
>> was:
>> - SMMUs with different feature bits
>> - support of VCMDQ HW extension for SMMU CMDQ
>> - need for separate S1 invalidation paths
>>
>> If I understand correctly this is mostly wanted for VCMDQ handling? if this
>> is correct we may indicate that somehow in the terminology.
>>
> Not just for VCMDQ, but it benefits when we have multiple physical SMMUv3
> instances as well.
>
>> If I understand correctly VCMDQ terminology is NVidia specific while ECMDQ
>> is the baseline (?).
> Yes, VCMDQ is NVIDIA specific. And ECMDQ is ARM SMMUv3, but don’t think we
> can associate ECMDQ with a virtual SMMUv3.
ok
>
>>> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that
>> size */
>>> [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
>>> [VIRT_SECURE_MEM] = { 0x0e000000, 0x01000000 },
>>> @@ -226,6 +227,7 @@ static const int a15irqmap[] = {
>>> [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>>> [VIRT_SMMU] = 74, /* ...to 74 + NUM_SMMU_IRQS - 1 */
>>> [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS
>>> -1 */
>>> + [VIRT_SMMU_NESTED] = 200,
>> What is the max IRQs expected to be consumed. Wother to comment for
>> next interrupt user.
> Depends on how many we plan to support max and each requires minimum 4. I
> will
> update with a comment here.
>
>>> };
>>>
>>> static void create_randomness(MachineState *ms, const char *node) @@
>>> -2883,10 +2885,34 @@ static void
>> virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
>>> DeviceState *dev, Error
>>> **errp) {
>>> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>>> + MachineClass *mc = MACHINE_GET_CLASS(vms);
>>>
>>> - if (vms->platform_bus_dev) {
>>> - MachineClass *mc = MACHINE_GET_CLASS(vms);
>>> + /* For smmuv3-nested devices we need to set the mem & irq */
>>> + if (device_is_dynamic_sysbus(mc, dev) &&
>>> + object_dynamic_cast(OBJECT(dev), TYPE_ARM_SMMUV3_NESTED)) {
>> why did you choose not using the PLATFORM BUS infra which does that
>> kind of binding automatically (also it provisions for dedicated MMIOs and
>> IRQs). At least you would need to justify in the commit msg I think
> Because I was not sure how to do this binding otherwise. I couldn't find
> any such precedence for a dynamic platform bus dev binding
> MMIOs/IRQs(May be I didn't look hard). I mentioned it in cover letter.
>
> Could you please give me some pointers/example for this? I will also
> take another look.
vfio platform users such automatic binding (however you must check that
vfio platform bus mmio and irq space is large enough for your needs).
the binding is transparently handled by
if (vms->platform_bus_dev) {
if (device_is_dynamic_sysbus(mc, dev)) {
platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
SYS_BUS_DEVICE(dev));
}
}
>
>>> + hwaddr base = vms->memmap[VIRT_SMMU_NESTED].base;
>>> + int irq = vms->irqmap[VIRT_SMMU_NESTED];
>>> +
>>> + if (vms->smmu_nested_count >= MAX_SMMU_NESTED) {
>>> + error_setg(errp, "smmuv3-nested max count reached!");
>>> + return;
>>> + }
>>> +
>>> + base += (vms->smmu_nested_count * SMMU_IO_LEN);
>>> + irq += (vms->smmu_nested_count * NUM_SMMU_IRQS);
>>>
>>> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>>> + for (int i = 0; i < 4; i++) {
>>> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
>>> + qdev_get_gpio_in(vms->gic, irq + i));
>>> + }
>>> + if (vms->iommu != VIRT_IOMMU_SMMUV3_NESTED) {
>>> + vms->iommu = VIRT_IOMMU_SMMUV3_NESTED;
>>> + }
>>> + vms->smmu_nested_count++;
>> this kind of check would definitively not integrated in the platform bus but
>> this could be introduced generically in the framework though or special
>> cased after the platform_bus_link_device
> Ok. So I assume there is a better way to link the MMIOs/IRQs as you mentioned
> above and we can add another helper to track this count as well.
>
>>> + }
>>> +
>>> + if (vms->platform_bus_dev) {
>>> if (device_is_dynamic_sysbus(mc, dev)) {
>>> platform_bus_link_device(PLATFORM_BUS_DEVICE(vms-
>>> platform_bus_dev),
>>> SYS_BUS_DEVICE(dev)); @@ -3067,6
>>> +3093,7 @@ static void virt_machine_class_init(ObjectClass *oc, void
>> *data)
>>> machine_class_allow_dynamic_sysbus_dev(mc,
>> TYPE_VFIO_AMD_XGBE);
>>> machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
>>> machine_class_allow_dynamic_sysbus_dev(mc,
>> TYPE_VFIO_PLATFORM);
>>> + machine_class_allow_dynamic_sysbus_dev(mc,
>>> + TYPE_ARM_SMMUV3_NESTED);
>>> #ifdef CONFIG_TPM
>>> machine_class_allow_dynamic_sysbus_dev(mc,
>> TYPE_TPM_TIS_SYSBUS);
>>> #endif diff --git a/hw/core/sysbus-fdt.c b/hw/core/sysbus-fdt.c index
>>> eebcd28f9a..0f0d0b3e58 100644
>>> --- a/hw/core/sysbus-fdt.c
>>> +++ b/hw/core/sysbus-fdt.c
>>> @@ -489,6 +489,7 @@ static const BindingEntry bindings[] = { #ifdef
>>> CONFIG_LINUX
>>> TYPE_BINDING(TYPE_VFIO_CALXEDA_XGMAC,
>> add_calxeda_midway_xgmac_fdt_node),
>>> TYPE_BINDING(TYPE_VFIO_AMD_XGBE, add_amd_xgbe_fdt_node),
>>> + TYPE_BINDING("arm-smmuv3-nested", no_fdt_node),
>>> VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a",
>>> add_amd_xgbe_fdt_node), #endif #ifdef CONFIG_TPM diff --git
>>> a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h index
>>> d183a62766..87e628be7a 100644
>>> --- a/include/hw/arm/smmuv3.h
>>> +++ b/include/hw/arm/smmuv3.h
>>> @@ -84,6 +84,21 @@ struct SMMUv3Class {
>>> #define TYPE_ARM_SMMUV3 "arm-smmuv3"
>>> OBJECT_DECLARE_TYPE(SMMUv3State, SMMUv3Class, ARM_SMMUV3)
>>>
>>> +#define TYPE_ARM_SMMUV3_NESTED "arm-smmuv3-nested"
>>> +OBJECT_DECLARE_TYPE(SMMUv3NestedState, SMMUv3NestedClass,
>>> +ARM_SMMUV3_NESTED)
>>> +
>>> +struct SMMUv3NestedState {
>>> + SMMUv3State smmuv3_state;
>>> +};
>>> +
>>> +struct SMMUv3NestedClass {
>>> + /*< private >*/
>>> + SMMUv3Class smmuv3_class;
>>> + /*< public >*/
>>> +
>>> + DeviceRealize parent_realize;
>>> +};
>>> +
>>> #define STAGE1_SUPPORTED(s) FIELD_EX32(s->idr[0], IDR0, S1P)
>>> #define STAGE2_SUPPORTED(s) FIELD_EX32(s->idr[0], IDR0, S2P)
>>>
>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index
>>> 46f48fe561..50e47a4ef3 100644
>>> --- a/include/hw/arm/virt.h
>>> +++ b/include/hw/arm/virt.h
>>> @@ -50,6 +50,9 @@
>>> /* MMIO region size for SMMUv3 */
>>> #define SMMU_IO_LEN 0x20000
>>>
>>> +/* Max supported nested SMMUv3 */ if (vms->platform_bus_dev) {
>>> if (device_is_dynamic_sysbus(mc, dev)) {
>>>
>>> platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
>>> SYS_BUS_DEVICE(dev));
>>> }
>>> }
>>>
>>> +#define MAX_SMMU_NESTED 128
>> Ouch, that many?!
> 😊. I just came up with the max we can support the allocated MMIO space.
> We do have systems at present which has 8 physical SMMUv3s at the moment.
> Probably 16/32 would be a better number I guess.
OK thanks
Eric
>
> Thanks,
> Shameer
