Subsequently smmuv3-accel will provide these callbacks
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
hw/arm/smmu-common.c | 27 +++++++++++++++++++++++++++
include/hw/arm/smmu-common.h | 5 +++++
2 files changed, 32 insertions(+)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 83c0693f5a..9fd455baa0 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -865,6 +865,10 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
SMMUState *s = opaque;
SMMUPciBus *sbus = smmu_get_sbus(s, bus);
+ if (s->accel && s->get_address_space) {
+ return s->get_address_space(bus, opaque, devfn);
+ }
+
sdev = sbus->pbdev[devfn];
if (!sdev) {
sdev = sbus->pbdev[devfn] = g_new0(SMMUDevice, 1);
@@ -874,8 +878,31 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
return &sdev->as;
}
+static bool smmu_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
+ HostIOMMUDevice *hiod, Error **errp)
+{
+ SMMUState *s = opaque;
+
+ if (s->accel && s->set_iommu_device) {
+ return s->set_iommu_device(bus, opaque, devfn, hiod, errp);
+ }
+
+ return false;
+}
+
+static void smmu_dev_unset_iommu_device(PCIBus *bus, void *opaque, int devfn)
+{
+ SMMUState *s = opaque;
+
+ if (s->accel && s->unset_iommu_device) {
+ s->unset_iommu_device(bus, opaque, devfn);
+ }
+}
+
static const PCIIOMMUOps smmu_ops = {
.get_address_space = smmu_find_add_as,
+ .set_iommu_device = smmu_dev_set_iommu_device,
+ .unset_iommu_device = smmu_dev_unset_iommu_device,
};
SMMUDevice *smmu_find_sdev(SMMUState *s, uint32_t sid)
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 80ff2ef6aa..7b05640167 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -160,6 +160,11 @@ struct SMMUState {
/* For smmuv3-accel */
bool accel;
+
+ AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
+ bool (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
+ HostIOMMUDevice *dev, Error **errp);
+ void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
};
struct SMMUBaseClass {
--
2.34.1
On 3/11/25 3:10 PM, Shameer Kolothum wrote:
> Subsequently smmuv3-accel will provide these callbacks
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/smmu-common.c | 27 +++++++++++++++++++++++++++
> include/hw/arm/smmu-common.h | 5 +++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 83c0693f5a..9fd455baa0 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -865,6 +865,10 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
> SMMUState *s = opaque;
> SMMUPciBus *sbus = smmu_get_sbus(s, bus);
>
> + if (s->accel && s->get_address_space) {
> + return s->get_address_space(bus, opaque, devfn);
> + }
after reading subsequent patch the code below should be another
implementation of this specific cb for non accelerate SMMU. So in that
patch you could introduce the implementation for the non accelerated SMMU.
Eric
> +
> sdev = sbus->pbdev[devfn];
> if (!sdev) {
> sdev = sbus->pbdev[devfn] = g_new0(SMMUDevice, 1);
> @@ -874,8 +878,31 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
> return &sdev->as;
> }
>
> +static bool smmu_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
> + HostIOMMUDevice *hiod, Error **errp)
> +{
> + SMMUState *s = opaque;
> +
> + if (s->accel && s->set_iommu_device) {
> + return s->set_iommu_device(bus, opaque, devfn, hiod, errp);
> + }
> +
> + return false;
> +}
> +
> +static void smmu_dev_unset_iommu_device(PCIBus *bus, void *opaque, int devfn)
> +{
> + SMMUState *s = opaque;
> +
> + if (s->accel && s->unset_iommu_device) {
> + s->unset_iommu_device(bus, opaque, devfn);
> + }
> +}
> +
> static const PCIIOMMUOps smmu_ops = {
> .get_address_space = smmu_find_add_as,
> + .set_iommu_device = smmu_dev_set_iommu_device,
> + .unset_iommu_device = smmu_dev_unset_iommu_device,
> };
>
> SMMUDevice *smmu_find_sdev(SMMUState *s, uint32_t sid)
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 80ff2ef6aa..7b05640167 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -160,6 +160,11 @@ struct SMMUState {
>
> /* For smmuv3-accel */
> bool accel;
> +
> + AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
> + bool (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
> + HostIOMMUDevice *dev, Error **errp);
> + void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
> };
>
> struct SMMUBaseClass {
On 3/11/25 3:10 PM, Shameer Kolothum wrote:
> Subsequently smmuv3-accel will provide these callbacks
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/smmu-common.c | 27 +++++++++++++++++++++++++++
> include/hw/arm/smmu-common.h | 5 +++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 83c0693f5a..9fd455baa0 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -865,6 +865,10 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
> SMMUState *s = opaque;
> SMMUPciBus *sbus = smmu_get_sbus(s, bus);
>
> + if (s->accel && s->get_address_space) {
> + return s->get_address_space(bus, opaque, devfn);
> + }
> +
why do we require that new call site? This needs to be documented in the
commit msg esp. because we don't know what this cb will do.
> sdev = sbus->pbdev[devfn];
> if (!sdev) {
> sdev = sbus->pbdev[devfn] = g_new0(SMMUDevice, 1);
> @@ -874,8 +878,31 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void *opaque, int devfn)
> return &sdev->as;
> }
>
> +static bool smmu_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
> + HostIOMMUDevice *hiod, Error **errp)
> +{
> + SMMUState *s = opaque;
> +
> + if (s->accel && s->set_iommu_device) {
> + return s->set_iommu_device(bus, opaque, devfn, hiod, errp);
> + }
> +
> + return false;
> +}
> +
> +static void smmu_dev_unset_iommu_device(PCIBus *bus, void *opaque, int devfn)
> +{
> + SMMUState *s = opaque;
> +
> + if (s->accel && s->unset_iommu_device) {
> + s->unset_iommu_device(bus, opaque, devfn);
> + }
> +}
> +
> static const PCIIOMMUOps smmu_ops = {
> .get_address_space = smmu_find_add_as,
> + .set_iommu_device = smmu_dev_set_iommu_device,
> + .unset_iommu_device = smmu_dev_unset_iommu_device,
> };
>
> SMMUDevice *smmu_find_sdev(SMMUState *s, uint32_t sid)
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 80ff2ef6aa..7b05640167 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -160,6 +160,11 @@ struct SMMUState {
>
> /* For smmuv3-accel */
> bool accel;
> +
> + AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
> + bool (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
> + HostIOMMUDevice *dev, Error **errp);
> + void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
I think this should be exposed by a class and only implemented in the
smmuv3 accel device. Adding those cbs directly in the State looks not
the std way.
Eric
> };
>
> struct SMMUBaseClass {
Hi Eric,
> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Wednesday, March 12, 2025 4:24 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; 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
> Subject: Re: [RFC PATCH v2 07/20] hw/arm/smmu-common: Introduce
> callbacks for PCIIOMMUOps
>
>
>
>
> On 3/11/25 3:10 PM, Shameer Kolothum wrote:
> > Subsequently smmuv3-accel will provide these callbacks
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> > hw/arm/smmu-common.c | 27 +++++++++++++++++++++++++++
> > include/hw/arm/smmu-common.h | 5 +++++
> > 2 files changed, 32 insertions(+)
> >
> > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index
> > 83c0693f5a..9fd455baa0 100644
> > --- a/hw/arm/smmu-common.c
> > +++ b/hw/arm/smmu-common.c
> > @@ -865,6 +865,10 @@ static AddressSpace *smmu_find_add_as(PCIBus
> *bus, void *opaque, int devfn)
> > SMMUState *s = opaque;
> > SMMUPciBus *sbus = smmu_get_sbus(s, bus);
> >
> > + if (s->accel && s->get_address_space) {
> > + return s->get_address_space(bus, opaque, devfn);
> > + }
> > +
> why do we require that new call site? This needs to be documented in the
> commit msg esp. because we don't know what this cb will do.
Currently, this is where the first time SMMUDevice sdev is allocated. And for smmuv3-accel
cases we are introducing a new SMMUv3AccelDevice accel_dev for holding additional
accel specific information. In order to do that the above cb is used. Same for other callbacks
as well.
Another way of avoiding the callbacks would be to move the pci_setup_iommu(bus, ops)
call from the smmu-common.c to smmuv3/smmuv3-accel and handle it directly there.
Or is there a better idea?
> > sdev = sbus->pbdev[devfn];
> > if (!sdev) {
> > sdev = sbus->pbdev[devfn] = g_new0(SMMUDevice, 1); @@ -874,8
> > +878,31 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void
> *opaque, int devfn)
> > return &sdev->as;
> > }
> >
> > +static bool smmu_dev_set_iommu_device(PCIBus *bus, void *opaque,
> int devfn,
> > + HostIOMMUDevice *hiod, Error
> > +**errp) {
> > + SMMUState *s = opaque;
> > +
> > + if (s->accel && s->set_iommu_device) {
> > + return s->set_iommu_device(bus, opaque, devfn, hiod, errp);
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static void smmu_dev_unset_iommu_device(PCIBus *bus, void *opaque,
> > +int devfn) {
> > + SMMUState *s = opaque;
> > +
> > + if (s->accel && s->unset_iommu_device) {
> > + s->unset_iommu_device(bus, opaque, devfn);
> > + }
> > +}
> > +
> > static const PCIIOMMUOps smmu_ops = {
> > .get_address_space = smmu_find_add_as,
> > + .set_iommu_device = smmu_dev_set_iommu_device,
> > + .unset_iommu_device = smmu_dev_unset_iommu_device,
> > };
> >
> > SMMUDevice *smmu_find_sdev(SMMUState *s, uint32_t sid) diff --git
> > a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index
> > 80ff2ef6aa..7b05640167 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -160,6 +160,11 @@ struct SMMUState {
> >
> > /* For smmuv3-accel */
> > bool accel;
> > +
> > + AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int
> devfn);
> > + bool (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
> > + HostIOMMUDevice *dev, Error **errp);
> > + void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
> I think this should be exposed by a class and only implemented in the
> smmuv3 accel device. Adding those cbs directly in the State looks not the
> std way.
Ok. You mean we can directly place PCIIOMMUOps *ops here then?
Thanks,
Shameer
Hi Shameer,
On 3/13/25 9:09 AM, Shameerali Kolothum Thodi wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: Wednesday, March 12, 2025 4:24 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; 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
>> Subject: Re: [RFC PATCH v2 07/20] hw/arm/smmu-common: Introduce
>> callbacks for PCIIOMMUOps
>>
>>
>>
>>
>> On 3/11/25 3:10 PM, Shameer Kolothum wrote:
>>> Subsequently smmuv3-accel will provide these callbacks
>>>
>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>> hw/arm/smmu-common.c | 27 +++++++++++++++++++++++++++
>>> include/hw/arm/smmu-common.h | 5 +++++
>>> 2 files changed, 32 insertions(+)
>>>
>>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index
>>> 83c0693f5a..9fd455baa0 100644
>>> --- a/hw/arm/smmu-common.c
>>> +++ b/hw/arm/smmu-common.c
>>> @@ -865,6 +865,10 @@ static AddressSpace *smmu_find_add_as(PCIBus
>> *bus, void *opaque, int devfn)
>>> SMMUState *s = opaque;
>>> SMMUPciBus *sbus = smmu_get_sbus(s, bus);
>>>
>>> + if (s->accel && s->get_address_space) {
>>> + return s->get_address_space(bus, opaque, devfn);
>>> + }
>>> +
>> why do we require that new call site? This needs to be documented in the
>> commit msg esp. because we don't know what this cb will do.
> Currently, this is where the first time SMMUDevice sdev is allocated. And for smmuv3-accel
> cases we are introducing a new SMMUv3AccelDevice accel_dev for holding additional
> accel specific information. In order to do that the above cb is used. Same for other callbacks
> as well.
>
> Another way of avoiding the callbacks would be to move the pci_setup_iommu(bus, ops)
> call from the smmu-common.c to smmuv3/smmuv3-accel and handle it directly there.
>
> Or is there a better idea?
>
>>> sdev = sbus->pbdev[devfn];
>>> if (!sdev) {
>>> sdev = sbus->pbdev[devfn] = g_new0(SMMUDevice, 1); @@ -874,8
>>> +878,31 @@ static AddressSpace *smmu_find_add_as(PCIBus *bus, void
>> *opaque, int devfn)
>>> return &sdev->as;
>>> }
>>>
>>> +static bool smmu_dev_set_iommu_device(PCIBus *bus, void *opaque,
>> int devfn,
>>> + HostIOMMUDevice *hiod, Error
>>> +**errp) {
>>> + SMMUState *s = opaque;
>>> +
>>> + if (s->accel && s->set_iommu_device) {
>>> + return s->set_iommu_device(bus, opaque, devfn, hiod, errp);
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> +static void smmu_dev_unset_iommu_device(PCIBus *bus, void *opaque,
>>> +int devfn) {
>>> + SMMUState *s = opaque;
>>> +
>>> + if (s->accel && s->unset_iommu_device) {
>>> + s->unset_iommu_device(bus, opaque, devfn);
>>> + }
>>> +}
>>> +
>>> static const PCIIOMMUOps smmu_ops = {
>>> .get_address_space = smmu_find_add_as,
>>> + .set_iommu_device = smmu_dev_set_iommu_device,
>>> + .unset_iommu_device = smmu_dev_unset_iommu_device,
>>> };
>>>
>>> SMMUDevice *smmu_find_sdev(SMMUState *s, uint32_t sid) diff --git
>>> a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>> index
>>> 80ff2ef6aa..7b05640167 100644
>>> --- a/include/hw/arm/smmu-common.h
>>> +++ b/include/hw/arm/smmu-common.h
>>> @@ -160,6 +160,11 @@ struct SMMUState {
>>>
>>> /* For smmuv3-accel */
>>> bool accel;
>>> +
>>> + AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int
>> devfn);
>>> + bool (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
>>> + HostIOMMUDevice *dev, Error **errp);
>>> + void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
>> I think this should be exposed by a class and only implemented in the
>> smmuv3 accel device. Adding those cbs directly in the State looks not the
>> std way.
> Ok. You mean we can directly place PCIIOMMUOps *ops here then?
When I first skimmed through the series I assumed you would use 2
seperate devices, in which case that would use 2 different
implementations of the same class. You may have a look at
docs/devel/qom.rst and Methods and class there.
Now as I commented earlier I think the end user shall instantiate the
same device for non accel and accel. I would advocate for passing an
option telling whether we want accel modality. Then it rather looks like
what was done for vfio device with either legacy or iommufd backend.
depending on whether the iommufd option is passed you select the right
class implementation:
see hw/vfio/common.c and vfio_attach_device
const VFIOIOMMUClass *ops =
VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
if (vbasedev->iommufd) {
ops =
VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
}
I would doing something similar for selecting the right ops depending on
the passed option.
I hope this helps
Eric
>
> Thanks,
> Shameer
> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: Monday, March 17, 2025 4:52 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; 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
> Subject: Re: [RFC PATCH v2 07/20] hw/arm/smmu-common: Introduce
> callbacks for PCIIOMMUOps
> Hi Shameer,
>
> On 3/13/25 9:09 AM, Shameerali Kolothum Thodi wrote:
> > Hi Eric,
> >
> >>> bool accel;
> >>> +
> >>> + AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque,
> int
> >> devfn);
> >>> + bool (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
> >>> + HostIOMMUDevice *dev, Error **errp);
> >>> + void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
> >> I think this should be exposed by a class and only implemented in the
> >> smmuv3 accel device. Adding those cbs directly in the State looks not the
> >> std way.
> > Ok. You mean we can directly place PCIIOMMUOps *ops here then?
> When I first skimmed through the series I assumed you would use 2
> seperate devices, in which case that would use 2 different
> implementations of the same class. You may have a look at
> docs/devel/qom.rst and Methods and class there.
>
> Now as I commented earlier I think the end user shall instantiate the
> same device for non accel and accel. I would advocate for passing an
> option telling whether we want accel modality. Then it rather looks like
> what was done for vfio device with either legacy or iommufd backend.
>
> depending on whether the iommufd option is passed you select the right
> class implementation:
> see hw/vfio/common.c and vfio_attach_device
>
>
> const VFIOIOMMUClass *ops =
> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGA
> CY));
>
> if (vbasedev->iommufd) {
> ops =
> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD)
> );
> }
>
> I would doing something similar for selecting the right ops depending on
> the passed option.
>
> I hope this helps
Thanks Eric. I will take a look.
Shameer
© 2016 - 2025 Red Hat, Inc.