> -----Original Message-----
> From: Nicolin Chen <[email protected]>
> Sent: 31 October 2025 22:02
> To: Shameer Kolothum <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Jason Gunthorpe
> <[email protected]>; [email protected]; [email protected]; Nathan
> Chen <[email protected]>; Matt Ochs <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Krishnakant Jaju <[email protected]>
> Subject: Re: [PATCH v5 12/32] hw/arm/smmuv3-accel: Add
> set/unset_iommu_device callback
>
> 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.
Looks sensible to me. I will take a look during next revision.
Thanks,
Shameer
