Hi Shameer,
On 3/13/25 9:09 AM, Shameerali Kolothum Thodi wrote:
> 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?
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
