Re: [RFC PATCH 2/5] hw/arm/smmuv3: Add initial support for SMMUv3 Nested device

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

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.thodi@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.
> +    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.
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.

If I understand correctly VCMDQ terminology is NVidia specific while
ECMDQ is the baseline (?).
>      /* ...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.
>  };
>  
>  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
> +        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
> +    }
> +
> +    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 */
> +#define MAX_SMMU_NESTED 128
Ouch, that many?!
> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,
> @@ -62,6 +65,7 @@ enum {
>      VIRT_GIC_ITS,
>      VIRT_GIC_REDIST,
>      VIRT_SMMU,
> +    VIRT_SMMU_NESTED,
>      VIRT_UART0,
>      VIRT_MMIO,
>      VIRT_RTC,
> @@ -92,6 +96,7 @@ enum {
>  typedef enum VirtIOMMUType {
>      VIRT_IOMMU_NONE,
>      VIRT_IOMMU_SMMUV3,
> +    VIRT_IOMMU_SMMUV3_NESTED,
>      VIRT_IOMMU_VIRTIO,
>  } VirtIOMMUType;
>  
> @@ -155,6 +160,7 @@ struct VirtMachineState {
>      bool mte;
>      bool dtb_randomness;
>      bool second_ns_uart_present;
> +    int smmu_nested_count;
>      OnOffAuto acpi;
>      VirtGICType gic_version;
>      VirtIOMMUType iommu;
Thanks

Eric