Hi Eric,
> -----Original Message-----
> From: Eric Auger <eric.au...@redhat.com>
> Sent: Wednesday, March 12, 2025 4:24 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>; qemu-...@nongnu.org;
> qemu-devel@nongnu.org
> Cc: peter.mayd...@linaro.org; j...@nvidia.com; nicol...@nvidia.com;
> ddut...@redhat.com; berra...@redhat.com; nath...@nvidia.com;
> mo...@nvidia.com; smost...@google.com; Linuxarm
> <linux...@huawei.com>; Wangzhou (B) <wangzh...@hisilicon.com>;
> jiangkunkun <jiangkun...@huawei.com>; Jonathan Cameron
> <jonathan.came...@huawei.com>; zhangfei....@linaro.org
> Subject: Re: [RFC PATCH 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.th...@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
