| Message ID | 20241108125242.60136-3-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Add support for user-creatable nested SMMUv3 | expand |
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
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); > + SMMUv3NestedClass *c = ARM_SMMUV3_NESTED_GET_CLASS(s_nested); > + Error *local_err = NULL; > + > + c->parent_realize(d, &local_err); > + 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; this may be set at the very end of the series eventually. Eric > + 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 }, > /* ...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, > }; > > 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)) { > + 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++; > + } > + > + 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 > + > 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;
Hi Eric, On Wed, Nov 13, 2024 at 06:12:15PM +0100, Eric Auger wrote: > On 11/8/24 13:52, Shameer Kolothum wrote: > > @@ -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 (?). VCMDQ makes a multi-vSMMU-instance design a hard requirement, yet the point (3) for separate invalidation paths also matters. Jason suggested VMM in base case to create multi vSMMU instances as the kernel doc mentioned here: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/userspace-api/iommufd.rst#n84 W.r.t naming, maybe something related to "hardware-accelerated"? Thanks Nicolin
Hi Eric, > -----Original Message----- > From: Eric Auger <eric.auger@redhat.com> > Sent: Wednesday, November 13, 2024 5:12 PM > To: Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org; > qemu-devel@nongnu.org > Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com; > ddutile@redhat.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B) > <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>; > Jonathan Cameron <jonathan.cameron@huawei.com>; > zhangfei.gao@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.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. 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. > 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. > > /* ...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. > > + 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 */ > > +#define MAX_SMMU_NESTED 128 > Ouch, that many?!
Hi Shameer, On 11/14/24 09:20, Shameerali Kolothum Thodi wrote: > Hi Eric, > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Sent: Wednesday, November 13, 2024 5:12 PM >> To: Shameerali Kolothum Thodi >> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org; >> qemu-devel@nongnu.org >> Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com; >> ddutile@redhat.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B) >> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>; >> Jonathan Cameron <jonathan.cameron@huawei.com>; >> zhangfei.gao@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.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. > 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?! >
Hi Eric, > -----Original Message----- > From: Eric Auger <eric.auger@redhat.com> > Sent: Thursday, November 14, 2024 8:42 AM > To: Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org; > qemu-devel@nongnu.org > Cc: peter.maydell@linaro.org; jgg@nvidia.com; nicolinc@nvidia.com; > ddutile@redhat.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B) > <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>; > Jonathan Cameron <jonathan.cameron@huawei.com>; > zhangfei.gao@linaro.org > Subject: Re: [RFC PATCH 2/5] hw/arm/smmuv3: Add initial support for > SMMUv3 Nested device > >> 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)); > } > } Ah..I see. I missed that it does that transparently. And use platform_bus_get_mmio_addr() for retrieving it for ACPI/DT similar to what TPM device is doing. So we don't need specific entries for this device in memmap/irqmap. I will give it a try. Thanks, Shameer.
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); + SMMUv3NestedClass *c = ARM_SMMUV3_NESTED_GET_CLASS(s_nested); + Error *local_err = NULL; + + c->parent_realize(d, &local_err); + 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 }, /* ...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, }; 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)) { + 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++; + } + + 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 + 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;
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(-)