Re: [PATCH v5 12/32] hw/arm/smmuv3-accel: Add set/unset_iommu_device callback

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Oct 31, 2025 at 10:49:45AM +0000, Shameer Kolothum wrote:
> +static bool
> +smmuv3_accel_dev_alloc_viommu(SMMUv3AccelDevice *accel_dev,
> +                              HostIOMMUDeviceIOMMUFD *idev, Error **errp)

Let's make it simply do alloc() on s_accel:

static bool smmuv3_accel_alloc_viommu(SMMUv3AccelState *s_accel,
                                      HostIOMMUDeviceIOMMUFD *idev,
                                      Error **errp)

Then..

> +    SMMUDevice *sdev = &accel_dev->sdev;
> +    SMMUState *bs = sdev->smmu;
> +    SMMUv3State *s = ARM_SMMUV3(bs);
> +    SMMUv3AccelState *s_accel = s->s_accel;

Drop these.

> +    if (s_accel->vsmmu) {
> +        accel_dev->vsmmu = s_accel->vsmmu;
> +        return true;
> +    }

And this.

> +static bool smmuv3_accel_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
> +                                          HostIOMMUDevice *hiod, Error **errp)
[...]
> +    if (!smmuv3_accel_dev_alloc_viommu(accel_dev, idev, errp)) {
> +        error_append_hint(errp, "Device 0x%x: Unable to alloc viommu", sid);
> +        return false;
> +    }

And here:

    if (!s_accel->vsmmu && !smmuv3_accel_alloc_viommu(s_accel, idev, errp)) {
        error_append_hint(errp, "Device 0x%x: Unable to alloc viommu", sid);
        return false;
    }

    accel_dev->idev = idev;
    accel_dev->vsmmu = s_accel->vsmmu;

Feels slightly cleaner.

> +/*
> + * Represents a virtual SMMU instance backed by an iommufd vIOMMU object.
> + * Holds references to the core iommufd vIOMMU object and to proxy HWPTs

I read "reference" as a pointer, yet...

> + * (bypass and abort) used for device attachment.
> + */
> +typedef struct SMMUViommu {
> +    IOMMUFDBackend *iommufd;
> +    IOMMUFDViommu viommu;
> +    uint32_t bypass_hwpt_id;
> +    uint32_t abort_hwpt_id;

...viommu is a containment and two HWPTs are IDs.

So, it'd sound more accurate, being:

/*
 * Represents a virtual SMMU instance backed by an iommufd vIOMMU object.
 * Holds bypass and abort proxy HWPT ids used for device attachment.
 */

> +typedef struct SMMUv3AccelState {
> +    SMMUViommu *vsmmu;
> +} SMMUv3AccelState;

Hmm, maybe we don't need another layer of structure. Every access
or validation to s_accel is for s_accel->vsmmu.

e.g.
    if (!s_accel || !s_accel->vsmmu) {
could be
    if (!s_accel) {

So, let's try merging them into one. And feel free to leave one
of your favorite.

Nic