Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
SMMUv3 will have different handling for those ops callbacks
in subsequent patches.
The "accel" property is not yet added, so users cannot set it at this
point. It will be introduced in a subsequent patch once the necessary
support is in place.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
hw/arm/meson.build | 3 +-
hw/arm/smmu-common.c | 6 +++-
hw/arm/smmuv3-accel.c | 66 ++++++++++++++++++++++++++++++++++++
hw/arm/smmuv3-accel.h | 19 +++++++++++
include/hw/arm/smmu-common.h | 3 ++
5 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644 hw/arm/smmuv3-accel.c
create mode 100644 hw/arm/smmuv3-accel.h
diff --git a/hw/arm/meson.build b/hw/arm/meson.build
index dc68391305..6126eb1b64 100644
--- a/hw/arm/meson.build
+++ b/hw/arm/meson.build
@@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE', if_true: files('armsse.c'))
arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true: files('fsl-imx7.c', 'mcimx7d-sabre.c'))
arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true: files('fsl-imx8mp.c'))
arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true: files('imx8mp-evk.c'))
-arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
+arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
+arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'], if_true: files('smmuv3-accel.c'))
arm_common_ss.add(when: 'CONFIG_FSL_IMX6UL', if_true: files('fsl-imx6ul.c', 'mcimx6ul-evk.c'))
arm_common_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_soc.c'))
arm_ss.add(when: 'CONFIG_XEN', if_true: files(
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 3a1080773a..6a58f574d3 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -938,7 +938,11 @@ static const PCIIOMMUOps *smmu_iommu_ops_by_type(SMMUState *s)
{
SMMUBaseClass *sbc;
- sbc = ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
+ if (s->accel) {
+ sbc = ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMUV3_ACCEL));
+ } else {
+ sbc = ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
+ }
assert(sbc->iommu_ops);
return sbc->iommu_ops;
diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
new file mode 100644
index 0000000000..2eac9c6ff4
--- /dev/null
+++ b/hw/arm/smmuv3-accel.c
@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2025 Huawei Technologies R & D (UK) Ltd
+ * Copyright (C) 2025 NVIDIA
+ * Written by Nicolin Chen, Shameer Kolothum
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+
+#include "hw/arm/smmuv3.h"
+#include "smmuv3-accel.h"
+
+static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs, SMMUPciBus *sbus,
+ PCIBus *bus, int devfn)
+{
+ SMMUDevice *sdev = sbus->pbdev[devfn];
+ SMMUv3AccelDevice *accel_dev;
+
+ if (sdev) {
+ accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
+ } else {
+ accel_dev = g_new0(SMMUv3AccelDevice, 1);
+ sdev = &accel_dev->sdev;
+
+ sbus->pbdev[devfn] = sdev;
+ smmu_init_sdev(bs, sdev, bus, devfn);
+ }
+
+ return accel_dev;
+}
+
+static AddressSpace *smmuv3_accel_find_add_as(PCIBus *bus, void *opaque,
+ int devfn)
+{
+ SMMUState *bs = opaque;
+ SMMUPciBus *sbus;
+ SMMUv3AccelDevice *accel_dev;
+ SMMUDevice *sdev;
+
+ sbus = smmu_get_sbus(bs, bus);
+ accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
+ sdev = &accel_dev->sdev;
+
+ return &sdev->as;
+}
+
+static const PCIIOMMUOps smmuv3_accel_ops = {
+ .get_address_space = smmuv3_accel_find_add_as,
+};
+
+static void smmuv3_accel_class_init(ObjectClass *oc, const void *data)
+{
+ SMMUBaseClass *sbc = ARM_SMMU_CLASS(oc);
+
+ sbc->iommu_ops = &smmuv3_accel_ops;
+}
+
+static const TypeInfo types[] = {
+ {
+ .name = TYPE_ARM_SMMUV3_ACCEL,
+ .parent = TYPE_ARM_SMMUV3,
+ .class_init = smmuv3_accel_class_init,
+ }
+};
+DEFINE_TYPES(types)
diff --git a/hw/arm/smmuv3-accel.h b/hw/arm/smmuv3-accel.h
new file mode 100644
index 0000000000..4cf30b1291
--- /dev/null
+++ b/hw/arm/smmuv3-accel.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) 2025 Huawei Technologies R & D (UK) Ltd
+ * Copyright (C) 2025 NVIDIA
+ * Written by Nicolin Chen, Shameer Kolothum
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_ARM_SMMUV3_ACCEL_H
+#define HW_ARM_SMMUV3_ACCEL_H
+
+#include "hw/arm/smmu-common.h"
+#include CONFIG_DEVICES
+
+typedef struct SMMUv3AccelDevice {
+ SMMUDevice sdev;
+} SMMUv3AccelDevice;
+
+#endif /* HW_ARM_SMMUV3_ACCEL_H */
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index eb94623555..c459d24427 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -162,6 +162,7 @@ struct SMMUState {
uint8_t bus_num;
PCIBus *primary_bus;
bool smmu_per_bus; /* SMMU is specific to the primary_bus */
+ bool accel; /* SMMU has accelerator support */
};
struct SMMUBaseClass {
@@ -178,6 +179,8 @@ struct SMMUBaseClass {
#define TYPE_ARM_SMMU "arm-smmu"
OBJECT_DECLARE_TYPE(SMMUState, SMMUBaseClass, ARM_SMMU)
+#define TYPE_ARM_SMMUV3_ACCEL "arm-smmuv3-accel"
+
/* Return the SMMUPciBus handle associated to a PCI bus number */
SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num);
--
2.34.1
>-----Original Message-----
>From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>Subject: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce smmuv3
>accel device
>
>Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
>SMMUv3 will have different handling for those ops callbacks
>in subsequent patches.
>
>The "accel" property is not yet added, so users cannot set it at this
>point. It will be introduced in a subsequent patch once the necessary
>support is in place.
>
>Signed-off-by: Shameer Kolothum
><shameerali.kolothum.thodi@huawei.com>
>---
> hw/arm/meson.build | 3 +-
> hw/arm/smmu-common.c | 6 +++-
> hw/arm/smmuv3-accel.c | 66
>++++++++++++++++++++++++++++++++++++
> hw/arm/smmuv3-accel.h | 19 +++++++++++
> include/hw/arm/smmu-common.h | 3 ++
> 5 files changed, 95 insertions(+), 2 deletions(-)
> create mode 100644 hw/arm/smmuv3-accel.c
> create mode 100644 hw/arm/smmuv3-accel.h
>
>diff --git a/hw/arm/meson.build b/hw/arm/meson.build
>index dc68391305..6126eb1b64 100644
>--- a/hw/arm/meson.build
>+++ b/hw/arm/meson.build
>@@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE', if_true:
>files('armsse.c'))
> arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true: files('fsl-imx7.c',
>'mcimx7d-sabre.c'))
> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true:
>files('fsl-imx8mp.c'))
> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true:
>files('imx8mp-evk.c'))
>-arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true:
>files('smmuv3.c'))
>+arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
>+arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'], if_true:
>files('smmuv3-accel.c'))
> arm_common_ss.add(when: 'CONFIG_FSL_IMX6UL', if_true:
>files('fsl-imx6ul.c', 'mcimx6ul-evk.c'))
> arm_common_ss.add(when: 'CONFIG_NRF51_SOC', if_true:
>files('nrf51_soc.c'))
> arm_ss.add(when: 'CONFIG_XEN', if_true: files(
>diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
>index 3a1080773a..6a58f574d3 100644
>--- a/hw/arm/smmu-common.c
>+++ b/hw/arm/smmu-common.c
>@@ -938,7 +938,11 @@ static const PCIIOMMUOps
>*smmu_iommu_ops_by_type(SMMUState *s)
> {
> SMMUBaseClass *sbc;
>
>- sbc = ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
>+ if (s->accel) {
>+ sbc =
>ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMUV3_ACCEL));
>+ } else {
>+ sbc =
>ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
>+ }
> assert(sbc->iommu_ops);
>
> return sbc->iommu_ops;
>diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
>new file mode 100644
>index 0000000000..2eac9c6ff4
>--- /dev/null
>+++ b/hw/arm/smmuv3-accel.c
>@@ -0,0 +1,66 @@
>+/*
>+ * Copyright (c) 2025 Huawei Technologies R & D (UK) Ltd
>+ * Copyright (C) 2025 NVIDIA
>+ * Written by Nicolin Chen, Shameer Kolothum
>+ *
>+ * SPDX-License-Identifier: GPL-2.0-or-later
>+ */
>+
>+#include "qemu/osdep.h"
>+
>+#include "hw/arm/smmuv3.h"
>+#include "smmuv3-accel.h"
>+
>+static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs,
>SMMUPciBus *sbus,
>+ PCIBus *bus, int
>devfn)
>+{
>+ SMMUDevice *sdev = sbus->pbdev[devfn];
>+ SMMUv3AccelDevice *accel_dev;
>+
>+ if (sdev) {
>+ accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
>+ } else {
>+ accel_dev = g_new0(SMMUv3AccelDevice, 1);
>+ sdev = &accel_dev->sdev;
>+
>+ sbus->pbdev[devfn] = sdev;
>+ smmu_init_sdev(bs, sdev, bus, devfn);
>+ }
>+
>+ return accel_dev;
>+}
>+
>+static AddressSpace *smmuv3_accel_find_add_as(PCIBus *bus, void
>*opaque,
>+ int devfn)
>+{
>+ SMMUState *bs = opaque;
>+ SMMUPciBus *sbus;
>+ SMMUv3AccelDevice *accel_dev;
>+ SMMUDevice *sdev;
>+
>+ sbus = smmu_get_sbus(bs, bus);
>+ accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
>+ sdev = &accel_dev->sdev;
>+
>+ return &sdev->as;
>+}
>+
>+static const PCIIOMMUOps smmuv3_accel_ops = {
>+ .get_address_space = smmuv3_accel_find_add_as,
>+};
>+
>+static void smmuv3_accel_class_init(ObjectClass *oc, const void *data)
>+{
>+ SMMUBaseClass *sbc = ARM_SMMU_CLASS(oc);
>+
>+ sbc->iommu_ops = &smmuv3_accel_ops;
>+}
>+
>+static const TypeInfo types[] = {
>+ {
>+ .name = TYPE_ARM_SMMUV3_ACCEL,
>+ .parent = TYPE_ARM_SMMUV3,
>+ .class_init = smmuv3_accel_class_init,
>+ }
In cover-letter, I see "-device arm-smmuv3", so where is above accel device
created so we could use smmuv3_accel_ops?
On Tue, Jul 15, 2025 at 10:48:31AM +0000, Duan, Zhenzhong wrote:
> >+static const TypeInfo types[] = {
> >+ {
> >+ .name = TYPE_ARM_SMMUV3_ACCEL,
> >+ .parent = TYPE_ARM_SMMUV3,
> >+ .class_init = smmuv3_accel_class_init,
> >+ }
>
> In cover-letter, I see "-device arm-smmuv3", so where is above accel device
> created so we could use smmuv3_accel_ops?
The smmu-common.c is the shared file between accel and non-accel
instances. It has a module property:
DEFINE_PROP_BOOL("accel", SMMUState, accel, false),
where it directs to different iommu_ops:
937 static const PCIIOMMUOps *smmu_iommu_ops_by_type(SMMUState *s)
938 {
939 SMMUBaseClass *sbc;
940
941 if (s->accel) {
942 sbc = ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMUV3_ACCEL));
943 } else {
944 sbc = ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
945 }
946 assert(sbc->iommu_ops);
947
948 return sbc->iommu_ops;
949 }
Nicolin
>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
>smmuv3 accel device
>
>On Tue, Jul 15, 2025 at 10:48:31AM +0000, Duan, Zhenzhong wrote:
>> >+static const TypeInfo types[] = {
>> >+ {
>> >+ .name = TYPE_ARM_SMMUV3_ACCEL,
>> >+ .parent = TYPE_ARM_SMMUV3,
>> >+ .class_init = smmuv3_accel_class_init,
>> >+ }
>>
>> In cover-letter, I see "-device arm-smmuv3", so where is above accel device
>> created so we could use smmuv3_accel_ops?
>
>The smmu-common.c is the shared file between accel and non-accel
>instances. It has a module property:
> DEFINE_PROP_BOOL("accel", SMMUState, accel, false),
It looks we expose a new TYPE_ARM_SMMUV3_ACCEL type device just for exporting accel iommu_ops?
What about returning accel iommu_ops directly in smmu_iommu_ops_by_type() and drop the new type?
>
>where it directs to different iommu_ops:
>937 static const PCIIOMMUOps *smmu_iommu_ops_by_type(SMMUState *s)
>938 {
>939 SMMUBaseClass *sbc;
>940
>941 if (s->accel) {
>942 sbc =
>ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMUV3_ACCEL));
>943 } else {
>944 sbc =
>ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
>945 }
>946 assert(sbc->iommu_ops);
>947
>948 return sbc->iommu_ops;
>949 }
>
>Nicolin
> -----Original Message-----
> From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
> Sent: Wednesday, July 16, 2025 4:39 AM
> To: Nicolin Chen <nicolinc@nvidia.com>
> Cc: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> qemu-devel@nongnu.org; eric.auger@redhat.com;
> peter.maydell@linaro.org; jgg@nvidia.com; ddutile@redhat.com;
> berrange@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>;
> zhangfei.gao@linaro.org; shameerkolothum@gmail.com
> Subject: RE: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
> smmuv3 accel device
>
>
>
> >-----Original Message-----
> >From: Nicolin Chen <nicolinc@nvidia.com>
> >Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
> >smmuv3 accel device
> >
> >On Tue, Jul 15, 2025 at 10:48:31AM +0000, Duan, Zhenzhong wrote:
> >> >+static const TypeInfo types[] = {
> >> >+ {
> >> >+ .name = TYPE_ARM_SMMUV3_ACCEL,
> >> >+ .parent = TYPE_ARM_SMMUV3,
> >> >+ .class_init = smmuv3_accel_class_init,
> >> >+ }
> >>
> >> In cover-letter, I see "-device arm-smmuv3", so where is above accel
> >> device created so we could use smmuv3_accel_ops?
> >
> >The smmu-common.c is the shared file between accel and non-accel
> >instances. It has a module property:
> > DEFINE_PROP_BOOL("accel", SMMUState, accel, false),
>
> It looks we expose a new TYPE_ARM_SMMUV3_ACCEL type device just for
> exporting accel iommu_ops?
> What about returning accel iommu_ops directly in
> smmu_iommu_ops_by_type() and drop the new type?
We are not creating any new device here. Its just a Class object of different type.
I had a different approach previously and Eric suggested to try this as there
are examples in VFIO/IOMMUFD for something like this.
https://lore.kernel.org/qemu-devel/1105d100-dd1e-4aca-b518-50f903967416@redhat.com/
Thanks,
Shameer
> >
> >where it directs to different iommu_ops:
> >937 static const PCIIOMMUOps *smmu_iommu_ops_by_type(SMMUState
> *s)
> >938 {
> >939 SMMUBaseClass *sbc;
> >940
> >941 if (s->accel) {
> >942 sbc =
> >ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMUV3_ACCEL));
> >943 } else {
> >944 sbc =
> >ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
> >945 }
> >946 assert(sbc->iommu_ops);
> >947
> >948 return sbc->iommu_ops;
> >949 }
> >
> >Nicolin
Hi Shameer,
On 7/16/25 11:27 AM, Shameerali Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>> Sent: Wednesday, July 16, 2025 4:39 AM
>> To: Nicolin Chen <nicolinc@nvidia.com>
>> Cc: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
>> qemu-devel@nongnu.org; eric.auger@redhat.com;
>> peter.maydell@linaro.org; jgg@nvidia.com; ddutile@redhat.com;
>> berrange@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
>> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B)
>> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
>> Jonathan Cameron <jonathan.cameron@huawei.com>;
>> zhangfei.gao@linaro.org; shameerkolothum@gmail.com
>> Subject: RE: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
>> smmuv3 accel device
>>
>>
>>
>>> -----Original Message-----
>>> From: Nicolin Chen <nicolinc@nvidia.com>
>>> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
>>> smmuv3 accel device
>>>
>>> On Tue, Jul 15, 2025 at 10:48:31AM +0000, Duan, Zhenzhong wrote:
>>>>> +static const TypeInfo types[] = {
>>>>> + {
>>>>> + .name = TYPE_ARM_SMMUV3_ACCEL,
>>>>> + .parent = TYPE_ARM_SMMUV3,
>>>>> + .class_init = smmuv3_accel_class_init,
>>>>> + }
>>>> In cover-letter, I see "-device arm-smmuv3", so where is above accel
>>>> device created so we could use smmuv3_accel_ops?
>>> The smmu-common.c is the shared file between accel and non-accel
>>> instances. It has a module property:
>>> DEFINE_PROP_BOOL("accel", SMMUState, accel, false),
>> It looks we expose a new TYPE_ARM_SMMUV3_ACCEL type device just for
>> exporting accel iommu_ops?
>> What about returning accel iommu_ops directly in
>> smmu_iommu_ops_by_type() and drop the new type?
> We are not creating any new device here. Its just a Class object of different type.
> I had a different approach previously and Eric suggested to try this as there
> are examples in VFIO/IOMMUFD for something like this.
>
> https://lore.kernel.org/qemu-devel/1105d100-dd1e-4aca-b518-50f903967416@redhat.com/
Actually I pointed out that usually we don't add methods in states as it
was done in v2 (in SMMUState) but rather in classes hence my suggestion
to use a class instead. Now what looks strange is your class does not
implement any method ;-)
docs/devel/qom.rst says "The #ObjectClass typically holds a table of function pointers
for the virtual methods implemented by this type."
Sorry if I was unclear.
Now if you don't need any "accel" specific methods besides the PCIIOMMUOps which have a specific struct, I am not sure we need a class anymore. or then you direct embed the PCIIOMMUOps Struct in the method?
Also it's true that's weird that the actual object is never instantiated and may also appear in qom object list. This is not the case for the example I gave, ie. the VFIOContainerBase.
I don't know if anyone has a better/more elegant idea?
Thanks
Eric
>
> Thanks,
> Shameer
>
>>> where it directs to different iommu_ops:
>>> 937 static const PCIIOMMUOps *smmu_iommu_ops_by_type(SMMUState
>> *s)
>>> 938 {
>>> 939 SMMUBaseClass *sbc;
>>> 940
>>> 941 if (s->accel) {
>>> 942 sbc =
>>> ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMUV3_ACCEL));
>>> 943 } else {
>>> 944 sbc =
>>> ARM_SMMU_CLASS(object_class_by_name(TYPE_ARM_SMMU));
>>> 945 }
>>> 946 assert(sbc->iommu_ops);
>>> 947
>>> 948 return sbc->iommu_ops;
>>> 949 }
>>>
>>> Nicolin
Hi Eric,
> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 04 September 2025 15:32
> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; Nicolin Chen
> <nicolinc@nvidia.com>; Shameer Kolothum <skolothumtho@nvidia.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>;
> ddutile@redhat.com; berrange@redhat.com; Nathan Chen
> <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>;
> zhangfei.gao@linaro.org; shameerkolothum@gmail.com
> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce smmuv3
> accel device
>
> External email: Use caution opening links or attachments
>
>
> Hi Shameer,
>
> On 7/16/25 11:27 AM, Shameerali Kolothum Thodi wrote:
> >
> >> -----Original Message-----
> >> From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
> >> Sent: Wednesday, July 16, 2025 4:39 AM
> >> To: Nicolin Chen <nicolinc@nvidia.com>
> >> Cc: Shameerali Kolothum Thodi
> >> <shameerali.kolothum.thodi@huawei.com>; qemu-arm@nongnu.org;
> >> qemu-devel@nongnu.org; eric.auger@redhat.com;
> >> peter.maydell@linaro.org; jgg@nvidia.com; ddutile@redhat.com;
> >> berrange@redhat.com; nathanc@nvidia.com; mochs@nvidia.com;
> >> smostafa@google.com; Linuxarm <linuxarm@huawei.com>; Wangzhou (B)
> >> <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> >> Jonathan Cameron <jonathan.cameron@huawei.com>;
> >> zhangfei.gao@linaro.org; shameerkolothum@gmail.com
> >> Subject: RE: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
> >> smmuv3 accel device
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Nicolin Chen <nicolinc@nvidia.com>
> >>> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
> >>> smmuv3 accel device
> >>>
> >>> On Tue, Jul 15, 2025 at 10:48:31AM +0000, Duan, Zhenzhong wrote:
> >>>>> +static const TypeInfo types[] = {
> >>>>> + {
> >>>>> + .name = TYPE_ARM_SMMUV3_ACCEL,
> >>>>> + .parent = TYPE_ARM_SMMUV3,
> >>>>> + .class_init = smmuv3_accel_class_init,
> >>>>> + }
> >>>> In cover-letter, I see "-device arm-smmuv3", so where is above accel
> >>>> device created so we could use smmuv3_accel_ops?
> >>> The smmu-common.c is the shared file between accel and non-accel
> >>> instances. It has a module property:
> >>> DEFINE_PROP_BOOL("accel", SMMUState, accel, false),
> >> It looks we expose a new TYPE_ARM_SMMUV3_ACCEL type device just for
> >> exporting accel iommu_ops?
> >> What about returning accel iommu_ops directly in
> >> smmu_iommu_ops_by_type() and drop the new type?
> > We are not creating any new device here. Its just a Class object of different
> type.
> > I had a different approach previously and Eric suggested to try this as there
> > are examples in VFIO/IOMMUFD for something like this.
> >
> > https://lore.kernel.org/qemu-devel/1105d100-dd1e-4aca-b518-
> 50f903967416@redhat.com/
> Actually I pointed out that usually we don't add methods in states as it
> was done in v2 (in SMMUState) but rather in classes hence my suggestion
> to use a class instead. Now what looks strange is your class does not
> implement any method ;-)
>
> docs/devel/qom.rst says "The #ObjectClass typically holds a table of function
> pointers
> for the virtual methods implemented by this type."
>
> Sorry if I was unclear.
>
> Now if you don't need any "accel" specific methods besides the
> PCIIOMMUOps which have a specific struct, I am not sure we need a class
> anymore. or then you direct embed the PCIIOMMUOps Struct in the method?
Looks like I misinterpreted your suggestion. I don't see a need for any specific
"accel" methods at the moment. And ObjectClass as per the definition above
Is indeed not a candidate for this now.
> Also it's true that's weird that the actual object is never instantiated and may
> also appear in qom object list. This is not the case for the example I gave, ie.
> the VFIOContainerBase.
True. My bad.
>
> I don't know if anyone has a better/more elegant idea?
How about something like this instead,
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 0f1a06cec2..0412ad26f0 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -956,6 +956,9 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
return;
}
+ if (!s->iommu_ops) {
+ s->iommu_ops = &smmu_ops;
+ }
/*
* We only allow default PCIe Root Complex(pcie.0) or pxb-pcie based extra
* root complexes to be associated with SMMU.
@@ -975,9 +978,9 @@ static void smmu_base_realize(DeviceState *dev, Error **errp)
}
if (s->smmu_per_bus) {
- pci_setup_iommu_per_bus(pci_bus, &smmu_ops, s);
+ pci_setup_iommu_per_bus(pci_bus, s->iommu_ops, s);
} else {
- pci_setup_iommu(pci_bus, &smmu_ops, s);
+ pci_setup_iommu(pci_bus, s->iommu_ops, s);
}
return;
}
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index bcf8af8dc7..f0c352515b 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -32,6 +32,7 @@
#include "qapi/error.h"
#include "hw/arm/smmuv3.h"
+#include "smmuv3-accel.h"
#include "smmuv3-internal.h"
#include "smmu-internal.h"
@@ -1882,6 +1883,10 @@ static void smmu_realize(DeviceState *d, Error **errp)
SysBusDevice *dev = SYS_BUS_DEVICE(d);
Error *local_err = NULL;
+ if (sys->accel) {
+ smmuv3_accel_set_iommu_ops(sys);
+ }
+
c->parent_realize(d, &local_err);
if (local_err) {
error_propagate(errp, local_err);
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index c6f899e403..7f06d95a52 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -162,6 +162,8 @@ struct SMMUState {
uint8_t bus_num;
PCIBus *primary_bus;
bool smmu_per_bus; /* SMMU is specific to the primary_bus */
+ const PCIIOMMUOps *iommu_ops;
+ bool accel;
};
struct SMMUBaseClass {
diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
new file mode 100644
index 0000000000..7b4b635044
--- /dev/null
+++ b/hw/arm/smmuv3-accel.c
[...]
+
+static const PCIIOMMUOps smmuv3_accel_ops = {
+ .get_address_space = smmuv3_accel_find_add_as,
+};
+
+void smmuv3_accel_set_iommu_ops(SMMUState *s)
+{
+ s->iommu_ops = &smmuv3_accel_ops;
+}
Please let me know.
Thanks,
Shameer
On Mon, 14 Jul 2025 16:59:31 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
> SMMUv3 will have different handling for those ops callbacks
> in subsequent patches.
>
> The "accel" property is not yet added, so users cannot set it at this
> point. It will be introduced in a subsequent patch once the necessary
> support is in place.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> new file mode 100644
> index 0000000000..2eac9c6ff4
> --- /dev/null
> +++ b/hw/arm/smmuv3-accel.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (c) 2025 Huawei Technologies R & D (UK) Ltd
> + * Copyright (C) 2025 NVIDIA
> + * Written by Nicolin Chen, Shameer Kolothum
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/arm/smmuv3.h"
> +#include "smmuv3-accel.h"
> +
> +static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs, SMMUPciBus *sbus,
> + PCIBus *bus, int devfn)
> +{
> + SMMUDevice *sdev = sbus->pbdev[devfn];
> + SMMUv3AccelDevice *accel_dev;
> +
> + if (sdev) {
> + accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
Nicolin made good point on early return being nicer here.
> + } else {
> + accel_dev = g_new0(SMMUv3AccelDevice, 1);
> + sdev = &accel_dev->sdev;
> +
> + sbus->pbdev[devfn] = sdev;
> + smmu_init_sdev(bs, sdev, bus, devfn);
> + }
> +
> + return accel_dev;
> +}
> +
> +static AddressSpace *smmuv3_accel_find_add_as(PCIBus *bus, void *opaque,
> + int devfn)
> +{
> + SMMUState *bs = opaque;
Why bs? (other than for giggles) If that is standard naming already then
fair enough.
> + SMMUPciBus *sbus;
> + SMMUv3AccelDevice *accel_dev;
> + SMMUDevice *sdev;
Maybe tidy up the ordering to some scheme.
> +
> + sbus = smmu_get_sbus(bs, bus);
> + accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
> + sdev = &accel_dev->sdev;
> +
> + return &sdev->as;
Not a lot of point in having local sdev unless you add more
stuff here that uses it later.
> +}
> diff --git a/hw/arm/smmuv3-accel.h b/hw/arm/smmuv3-accel.h
> new file mode 100644
> index 0000000000..4cf30b1291
> --- /dev/null
> +++ b/hw/arm/smmuv3-accel.h
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (c) 2025 Huawei Technologies R & D (UK) Ltd
> + * Copyright (C) 2025 NVIDIA
> + * Written by Nicolin Chen, Shameer Kolothum
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_ARM_SMMUV3_ACCEL_H
> +#define HW_ARM_SMMUV3_ACCEL_H
> +
> +#include "hw/arm/smmu-common.h"
> +#include CONFIG_DEVICES
> +
> +typedef struct SMMUv3AccelDevice {
> + SMMUDevice sdev;
Bonus space.
> +} SMMUv3AccelDevice;
> +
> +#endif /* HW_ARM_SMMUV3_ACCEL_H */
On Mon, Jul 14, 2025 at 04:59:31PM +0100, Shameer Kolothum wrote:
> Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
> SMMUv3 will have different handling for those ops callbacks
> in subsequent patches.
>
> The "accel" property is not yet added, so users cannot set it at this
> point. It will be introduced in a subsequent patch once the necessary
> support is in place.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Overall the patch looks good to me,
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
with some nits:
> @@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE', if_true: files('armsse.c'))
> arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true: files('fsl-imx7.c', 'mcimx7d-sabre.c'))
> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true: files('fsl-imx8mp.c'))
> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true: files('imx8mp-evk.c'))
> -arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
> +arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
> +arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'], if_true: files('smmuv3-accel.c'))
Wondering why "arm_common_ss" is changed to "arm_ss"?
> +static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs, SMMUPciBus *sbus,
> + PCIBus *bus, int devfn)
There seems to be an extra space in the 2nd line.
> +{
> + SMMUDevice *sdev = sbus->pbdev[devfn];
> + SMMUv3AccelDevice *accel_dev;
> +
> + if (sdev) {
> + accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
> + } else {
> + accel_dev = g_new0(SMMUv3AccelDevice, 1);
> + sdev = &accel_dev->sdev;
> +
> + sbus->pbdev[devfn] = sdev;
> + smmu_init_sdev(bs, sdev, bus, devfn);
> + }
Could just:
if (sdev) {
return container_of(sdev, SMMUv3AccelDevice, sdev);
}
Then, no extra indentations for the rest of the code.
> +
> + return accel_dev;
> +}
> +
> +static AddressSpace *smmuv3_accel_find_add_as(PCIBus *bus, void *opaque,
> + int devfn)
> +{
> + SMMUState *bs = opaque;
> + SMMUPciBus *sbus;
> + SMMUv3AccelDevice *accel_dev;
> + SMMUDevice *sdev;
> +
> + sbus = smmu_get_sbus(bs, bus);
> + accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
> + sdev = &accel_dev->sdev;
Maybe just:
+ SMMUPciBus *sbus = smmu_get_sbus(bs, bus);
+ SMMUv3AccelDevice *accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
+ SMMUDevice *sdev = &accel_dev->sdev;
?
> +typedef struct SMMUv3AccelDevice {
> + SMMUDevice sdev;
Let's drop the extra space in between.
> +} SMMUv3AccelDevice;
> +
> +#endif /* HW_ARM_SMMUV3_ACCEL_H */
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index eb94623555..c459d24427 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -162,6 +162,7 @@ struct SMMUState {
> uint8_t bus_num;
> PCIBus *primary_bus;
> bool smmu_per_bus; /* SMMU is specific to the primary_bus */
> + bool accel; /* SMMU has accelerator support */
How about:
"SMMU is in the HW-accelerated mode for stage-1 translation"
?
Thanks
Nicolin
On 7/14/25 7:23 PM, Nicolin Chen wrote:
> On Mon, Jul 14, 2025 at 04:59:31PM +0100, Shameer Kolothum wrote:
>> Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
>> SMMUv3 will have different handling for those ops callbacks
>> in subsequent patches.
>>
>> The "accel" property is not yet added, so users cannot set it at this
>> point. It will be introduced in a subsequent patch once the necessary
>> support is in place.
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Overall the patch looks good to me,
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>
> with some nits:
>
>> @@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE', if_true: files('armsse.c'))
>> arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true: files('fsl-imx7.c', 'mcimx7d-sabre.c'))
>> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true: files('fsl-imx8mp.c'))
>> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true: files('imx8mp-evk.c'))
>> -arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
>> +arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
>> +arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'], if_true: files('smmuv3-accel.c'))
> Wondering why "arm_common_ss" is changed to "arm_ss"?
Indeed why did you need to change that?
Thanks
Eric
>
>> +static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs, SMMUPciBus *sbus,
>> + PCIBus *bus, int devfn)
> There seems to be an extra space in the 2nd line.
>
>> +{
>> + SMMUDevice *sdev = sbus->pbdev[devfn];
>> + SMMUv3AccelDevice *accel_dev;
>> +
>> + if (sdev) {
>> + accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
>> + } else {
>> + accel_dev = g_new0(SMMUv3AccelDevice, 1);
>> + sdev = &accel_dev->sdev;
>> +
>> + sbus->pbdev[devfn] = sdev;
>> + smmu_init_sdev(bs, sdev, bus, devfn);
>> + }
> Could just:
> if (sdev) {
> return container_of(sdev, SMMUv3AccelDevice, sdev);
> }
>
> Then, no extra indentations for the rest of the code.
>
>> +
>> + return accel_dev;
>> +}
>> +
>> +static AddressSpace *smmuv3_accel_find_add_as(PCIBus *bus, void *opaque,
>> + int devfn)
>> +{
>> + SMMUState *bs = opaque;
>> + SMMUPciBus *sbus;
>> + SMMUv3AccelDevice *accel_dev;
>> + SMMUDevice *sdev;
>> +
>> + sbus = smmu_get_sbus(bs, bus);
>> + accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
>> + sdev = &accel_dev->sdev;
> Maybe just:
>
> + SMMUPciBus *sbus = smmu_get_sbus(bs, bus);
> + SMMUv3AccelDevice *accel_dev = smmuv3_accel_get_dev(bs, sbus, bus, devfn);
> + SMMUDevice *sdev = &accel_dev->sdev;
>
> ?
>
>> +typedef struct SMMUv3AccelDevice {
>> + SMMUDevice sdev;
> Let's drop the extra space in between.
>
>> +} SMMUv3AccelDevice;
>> +
>> +#endif /* HW_ARM_SMMUV3_ACCEL_H */
>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>> index eb94623555..c459d24427 100644
>> --- a/include/hw/arm/smmu-common.h
>> +++ b/include/hw/arm/smmu-common.h
>> @@ -162,6 +162,7 @@ struct SMMUState {
>> uint8_t bus_num;
>> PCIBus *primary_bus;
>> bool smmu_per_bus; /* SMMU is specific to the primary_bus */
>> + bool accel; /* SMMU has accelerator support */
> How about:
> "SMMU is in the HW-accelerated mode for stage-1 translation"
> ?
>
> Thanks
> Nicolin
>
> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 04 September 2025 15:33
> To: Nicolin Chen <nicolinc@nvidia.com>; Shameer Kolothum
> <skolothumtho@nvidia.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>;
> ddutile@redhat.com; berrange@redhat.com; Nathan Chen
> <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
> smostafa@google.com; linuxarm@huawei.com; wangzhou1@hisilicon.com;
> jiangkunkun@huawei.com; jonathan.cameron@huawei.com;
> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com;
> shameerkolothum@gmail.com
> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce smmuv3
> accel device
>
> External email: Use caution opening links or attachments
>
>
> On 7/14/25 7:23 PM, Nicolin Chen wrote:
> > On Mon, Jul 14, 2025 at 04:59:31PM +0100, Shameer Kolothum wrote:
> >> Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
> >> SMMUv3 will have different handling for those ops callbacks in
> >> subsequent patches.
> >>
> >> The "accel" property is not yet added, so users cannot set it at this
> >> point. It will be introduced in a subsequent patch once the necessary
> >> support is in place.
> >>
> >> Signed-off-by: Shameer Kolothum
> >> <shameerali.kolothum.thodi@huawei.com>
> > Overall the patch looks good to me,
> > Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> >
> > with some nits:
> >
> >> @@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE',
> if_true:
> >> files('armsse.c'))
> >> arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true:
> >> files('fsl-imx7.c', 'mcimx7d-sabre.c'))
> >> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true:
> >> files('fsl-imx8mp.c'))
> >> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true:
> >> files('imx8mp-evk.c'))
> >> -arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true:
> >> files('smmuv3.c'))
> >> +arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
> >> +arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'],
> if_true:
> >> +files('smmuv3-accel.c'))
> > Wondering why "arm_common_ss" is changed to "arm_ss"?
> Indeed why did you need to change that?
Thanks for going through the series. I will go through all the comments and
rework the series soon, but a quick one on the above question.
I was getting this compile error as now we use #include CONFIG_DEVICES to check
CONFIG_IOMMUFD
.d -o libsystem_arm.a.p/hw_arm_smmuv3.c.o -c ../hw/arm/smmuv3.c
In file included from ../hw/arm/smmuv3.c:35:
../hw/arm/smmuv3-accel.h:17:10: error: #include expects "FILENAME" or <FILENAME>
17 | #include CONFIG_DEVICES
| ^~~~~~~~~~~~~~
../hw/arm/smmuv3-accel.h:55:13: error: attempt to use poisoned "CONFIG_ARM_SMMUV3"
55 | #if defined(CONFIG_ARM_SMMUV3) && defined(CONFIG_IOMMUFD)
| ^
../hw/arm/smmuv3-accel.h:55:43: error: attempt to use poisoned "CONFIG_IOMMUFD"
55 | #if defined(CONFIG_ARM_SMMUV3) && defined(CONFIG_IOMMUFD)
| ^
ninja: build stopped: subcommand failed.
make[1]: *** [Makefile:168: run-ninja] Error 1
make[1]: Leaving directory '/root/qemu-hisi/build'
And with arm_common_ss.add, it looks like the compiler is missing
'-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"'.
Not sure how to fix this with arm_common_ss.add. Please let me know
If there is a way to address this.
Thanks,
Shameer
On 9/5/25 10:22 AM, Shameer Kolothum wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: 04 September 2025 15:33
>> To: Nicolin Chen <nicolinc@nvidia.com>; Shameer Kolothum
>> <skolothumtho@nvidia.com>
>> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
>> peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>;
>> ddutile@redhat.com; berrange@redhat.com; Nathan Chen
>> <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
>> smostafa@google.com; linuxarm@huawei.com; wangzhou1@hisilicon.com;
>> jiangkunkun@huawei.com; jonathan.cameron@huawei.com;
>> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com;
>> shameerkolothum@gmail.com
>> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce smmuv3
>> accel device
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 7/14/25 7:23 PM, Nicolin Chen wrote:
>>> On Mon, Jul 14, 2025 at 04:59:31PM +0100, Shameer Kolothum wrote:
>>>> Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
>>>> SMMUv3 will have different handling for those ops callbacks in
>>>> subsequent patches.
>>>>
>>>> The "accel" property is not yet added, so users cannot set it at this
>>>> point. It will be introduced in a subsequent patch once the necessary
>>>> support is in place.
>>>>
>>>> Signed-off-by: Shameer Kolothum
>>>> <shameerali.kolothum.thodi@huawei.com>
>>> Overall the patch looks good to me,
>>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>>>
>>> with some nits:
>>>
>>>> @@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE',
>> if_true:
>>>> files('armsse.c'))
>>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true:
>>>> files('fsl-imx7.c', 'mcimx7d-sabre.c'))
>>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true:
>>>> files('fsl-imx8mp.c'))
>>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true:
>>>> files('imx8mp-evk.c'))
>>>> -arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true:
>>>> files('smmuv3.c'))
>>>> +arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
>>>> +arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'],
>> if_true:
>>>> +files('smmuv3-accel.c'))
>>> Wondering why "arm_common_ss" is changed to "arm_ss"?
>> Indeed why did you need to change that?
> Thanks for going through the series. I will go through all the comments and
> rework the series soon, but a quick one on the above question.
>
> I was getting this compile error as now we use #include CONFIG_DEVICES to check
> CONFIG_IOMMUFD
>
> .d -o libsystem_arm.a.p/hw_arm_smmuv3.c.o -c ../hw/arm/smmuv3.c
> In file included from ../hw/arm/smmuv3.c:35:
> ../hw/arm/smmuv3-accel.h:17:10: error: #include expects "FILENAME" or <FILENAME>
> 17 | #include CONFIG_DEVICES
> | ^~~~~~~~~~~~~~
> ../hw/arm/smmuv3-accel.h:55:13: error: attempt to use poisoned "CONFIG_ARM_SMMUV3"
> 55 | #if defined(CONFIG_ARM_SMMUV3) && defined(CONFIG_IOMMUFD)
> | ^
> ../hw/arm/smmuv3-accel.h:55:43: error: attempt to use poisoned "CONFIG_IOMMUFD"
> 55 | #if defined(CONFIG_ARM_SMMUV3) && defined(CONFIG_IOMMUFD)
> | ^
> ninja: build stopped: subcommand failed.
> make[1]: *** [Makefile:168: run-ninja] Error 1
> make[1]: Leaving directory '/root/qemu-hisi/build'
>
> And with arm_common_ss.add, it looks like the compiler is missing
> '-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"'.
I guess this is because devices are arch specific so it cannot be
"common" anymore. Looks sensible to me. Adding Philippe in the loop.
Unrelated to that, do you have a way to opt-out SMMUv3 HW acceleration.
Some people might not be interested in that feature.
Eric
>
> Not sure how to fix this with arm_common_ss.add. Please let me know
> If there is a way to address this.
>
> Thanks,
> Shameer
>
>
>
> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 05 September 2025 11:18
> To: Shameer Kolothum <skolothumtho@nvidia.com>; Nicolin Chen
> <nicolinc@nvidia.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>;
> ddutile@redhat.com; berrange@redhat.com; Nathan Chen
> <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
> smostafa@google.com; linuxarm@huawei.com; wangzhou1@hisilicon.com;
> jiangkunkun@huawei.com; jonathan.cameron@huawei.com;
> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com;
> shameerkolothum@gmail.com; Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
> smmuv3 accel device
>
> External email: Use caution opening links or attachments
>
>
> On 9/5/25 10:22 AM, Shameer Kolothum wrote:
> >
> >> -----Original Message-----
> >> From: Eric Auger <eric.auger@redhat.com>
> >> Sent: 04 September 2025 15:33
> >> To: Nicolin Chen <nicolinc@nvidia.com>; Shameer Kolothum
> >> <skolothumtho@nvidia.com>
> >> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> >> peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>;
> >> ddutile@redhat.com; berrange@redhat.com; Nathan Chen
> >> <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
> >> smostafa@google.com; linuxarm@huawei.com;
> wangzhou1@hisilicon.com;
> >> jiangkunkun@huawei.com; jonathan.cameron@huawei.com;
> >> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com;
> >> shameerkolothum@gmail.com
> >> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
> >> smmuv3 accel device
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> On 7/14/25 7:23 PM, Nicolin Chen wrote:
> >>> On Mon, Jul 14, 2025 at 04:59:31PM +0100, Shameer Kolothum wrote:
> >>>> Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
> >>>> SMMUv3 will have different handling for those ops callbacks in
> >>>> subsequent patches.
> >>>>
> >>>> The "accel" property is not yet added, so users cannot set it at
> >>>> this point. It will be introduced in a subsequent patch once the
> >>>> necessary support is in place.
> >>>>
> >>>> Signed-off-by: Shameer Kolothum
> >>>> <shameerali.kolothum.thodi@huawei.com>
> >>> Overall the patch looks good to me,
> >>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> >>>
> >>> with some nits:
> >>>
> >>>> @@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE',
> >> if_true:
> >>>> files('armsse.c'))
> >>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true:
> >>>> files('fsl-imx7.c', 'mcimx7d-sabre.c'))
> >>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true:
> >>>> files('fsl-imx8mp.c'))
> >>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true:
> >>>> files('imx8mp-evk.c'))
> >>>> -arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true:
> >>>> files('smmuv3.c'))
> >>>> +arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true:
> files('smmuv3.c'))
> >>>> +arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'],
> >> if_true:
> >>>> +files('smmuv3-accel.c'))
> >>> Wondering why "arm_common_ss" is changed to "arm_ss"?
> >> Indeed why did you need to change that?
> > Thanks for going through the series. I will go through all the
> > comments and rework the series soon, but a quick one on the above
> question.
> >
> > I was getting this compile error as now we use #include CONFIG_DEVICES
> > to check CONFIG_IOMMUFD
> >
> > .d -o libsystem_arm.a.p/hw_arm_smmuv3.c.o -c ../hw/arm/smmuv3.c In
> > file included from ../hw/arm/smmuv3.c:35:
> > ../hw/arm/smmuv3-accel.h:17:10: error: #include expects "FILENAME" or
> <FILENAME>
> > 17 | #include CONFIG_DEVICES
> > | ^~~~~~~~~~~~~~
> > ../hw/arm/smmuv3-accel.h:55:13: error: attempt to use poisoned
> "CONFIG_ARM_SMMUV3"
> > 55 | #if defined(CONFIG_ARM_SMMUV3) &&
> defined(CONFIG_IOMMUFD)
> > | ^
> > ../hw/arm/smmuv3-accel.h:55:43: error: attempt to use poisoned
> "CONFIG_IOMMUFD"
> > 55 | #if defined(CONFIG_ARM_SMMUV3) &&
> defined(CONFIG_IOMMUFD)
> > | ^
> > ninja: build stopped: subcommand failed.
> > make[1]: *** [Makefile:168: run-ninja] Error 1
> > make[1]: Leaving directory '/root/qemu-hisi/build'
> >
> > And with arm_common_ss.add, it looks like the compiler is missing
> > '-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"'.
> I guess this is because devices are arch specific so it cannot be "common"
> anymore. Looks sensible to me. Adding Philippe in the loop.
>
> Unrelated to that, do you have a way to opt-out SMMUv3 HW acceleration.
> Some people might not be interested in that feature.
You mean a config option like ARM_SMMUV3_ACCEL to opt out during compile?
Can do.
Thanks,
Shameer
On 9/8/25 11:15 AM, Shameer Kolothum wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: 05 September 2025 11:18
>> To: Shameer Kolothum <skolothumtho@nvidia.com>; Nicolin Chen
>> <nicolinc@nvidia.com>
>> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
>> peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>;
>> ddutile@redhat.com; berrange@redhat.com; Nathan Chen
>> <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
>> smostafa@google.com; linuxarm@huawei.com; wangzhou1@hisilicon.com;
>> jiangkunkun@huawei.com; jonathan.cameron@huawei.com;
>> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com;
>> shameerkolothum@gmail.com; Philippe Mathieu-Daudé <philmd@linaro.org>
>> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
>> smmuv3 accel device
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 9/5/25 10:22 AM, Shameer Kolothum wrote:
>>>> -----Original Message-----
>>>> From: Eric Auger <eric.auger@redhat.com>
>>>> Sent: 04 September 2025 15:33
>>>> To: Nicolin Chen <nicolinc@nvidia.com>; Shameer Kolothum
>>>> <skolothumtho@nvidia.com>
>>>> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
>>>> peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>;
>>>> ddutile@redhat.com; berrange@redhat.com; Nathan Chen
>>>> <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
>>>> smostafa@google.com; linuxarm@huawei.com;
>> wangzhou1@hisilicon.com;
>>>> jiangkunkun@huawei.com; jonathan.cameron@huawei.com;
>>>> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com;
>>>> shameerkolothum@gmail.com
>>>> Subject: Re: [RFC PATCH v3 05/15] hw/arm/smmuv3-accel: Introduce
>>>> smmuv3 accel device
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 7/14/25 7:23 PM, Nicolin Chen wrote:
>>>>> On Mon, Jul 14, 2025 at 04:59:31PM +0100, Shameer Kolothum wrote:
>>>>>> Also setup specific PCIIOMMUOps for accel SMMUv3 as accel
>>>>>> SMMUv3 will have different handling for those ops callbacks in
>>>>>> subsequent patches.
>>>>>>
>>>>>> The "accel" property is not yet added, so users cannot set it at
>>>>>> this point. It will be introduced in a subsequent patch once the
>>>>>> necessary support is in place.
>>>>>>
>>>>>> Signed-off-by: Shameer Kolothum
>>>>>> <shameerali.kolothum.thodi@huawei.com>
>>>>> Overall the patch looks good to me,
>>>>> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>>>>>
>>>>> with some nits:
>>>>>
>>>>>> @@ -61,7 +61,8 @@ arm_common_ss.add(when: 'CONFIG_ARMSSE',
>>>> if_true:
>>>>>> files('armsse.c'))
>>>>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX7', if_true:
>>>>>> files('fsl-imx7.c', 'mcimx7d-sabre.c'))
>>>>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP', if_true:
>>>>>> files('fsl-imx8mp.c'))
>>>>>> arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true:
>>>>>> files('imx8mp-evk.c'))
>>>>>> -arm_common_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true:
>>>>>> files('smmuv3.c'))
>>>>>> +arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true:
>> files('smmuv3.c'))
>>>>>> +arm_ss.add(when: ['CONFIG_ARM_SMMUV3', 'CONFIG_IOMMUFD'],
>>>> if_true:
>>>>>> +files('smmuv3-accel.c'))
>>>>> Wondering why "arm_common_ss" is changed to "arm_ss"?
>>>> Indeed why did you need to change that?
>>> Thanks for going through the series. I will go through all the
>>> comments and rework the series soon, but a quick one on the above
>> question.
>>> I was getting this compile error as now we use #include CONFIG_DEVICES
>>> to check CONFIG_IOMMUFD
>>>
>>> .d -o libsystem_arm.a.p/hw_arm_smmuv3.c.o -c ../hw/arm/smmuv3.c In
>>> file included from ../hw/arm/smmuv3.c:35:
>>> ../hw/arm/smmuv3-accel.h:17:10: error: #include expects "FILENAME" or
>> <FILENAME>
>>> 17 | #include CONFIG_DEVICES
>>> | ^~~~~~~~~~~~~~
>>> ../hw/arm/smmuv3-accel.h:55:13: error: attempt to use poisoned
>> "CONFIG_ARM_SMMUV3"
>>> 55 | #if defined(CONFIG_ARM_SMMUV3) &&
>> defined(CONFIG_IOMMUFD)
>>> | ^
>>> ../hw/arm/smmuv3-accel.h:55:43: error: attempt to use poisoned
>> "CONFIG_IOMMUFD"
>>> 55 | #if defined(CONFIG_ARM_SMMUV3) &&
>> defined(CONFIG_IOMMUFD)
>>> | ^
>>> ninja: build stopped: subcommand failed.
>>> make[1]: *** [Makefile:168: run-ninja] Error 1
>>> make[1]: Leaving directory '/root/qemu-hisi/build'
>>>
>>> And with arm_common_ss.add, it looks like the compiler is missing
>>> '-DCONFIG_DEVICES="aarch64-softmmu-config-devices.h"'.
>> I guess this is because devices are arch specific so it cannot be "common"
>> anymore. Looks sensible to me. Adding Philippe in the loop.
>>
>> Unrelated to that, do you have a way to opt-out SMMUv3 HW acceleration.
>> Some people might not be interested in that feature.
> You mean a config option like ARM_SMMUV3_ACCEL to opt out during compile?
Yes that's what I meant
Eric
> Can do.
>
> Thanks,
> Shameer
>
>
© 2016 - 2025 Red Hat, Inc.