On Mon, Mar 09, 2026 at 11:25:21AM -0700, Shameer Kolothum Thodi wrote: > > > > -----Original Message----- > > From: Nicolin Chen <[email protected]> > > Sent: 09 March 2026 18:10 > > To: Shameer Kolothum Thodi <[email protected]> > > Cc: [email protected]; [email protected]; qemu- > > [email protected]; [email protected]; [email protected]; > > [email protected]; Nathan Chen <[email protected]>; Matt Ochs > > <[email protected]>; Jiandi An <[email protected]>; Jason Gunthorpe > > <[email protected]>; [email protected]; > > [email protected]; [email protected]; Krishnakant Jaju > > <[email protected]>; [email protected] > > Subject: Re: [PATCH v3 13/32] hw/arm/tegra241-cmdqv: Implement CMDQV > > vIOMMU alloc/free > > > > On Mon, Mar 09, 2026 at 04:31:22AM -0700, Shameer Kolothum Thodi > > wrote: > > > > > - error_setg(errp, "NVIDIA Tegra241 CMDQV is unsupported"); > > > > > - return false; > > > > > + Tegra241CMDQV *cmdqv = s->s_accel->cmdqv; > > > > > + > > > > > + if (!iommufd_backend_alloc_viommu(idev->iommufd, idev->devid, > > > > > + > > > > > + IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV, > > > > if the only think that differs compared to no cmdq is the type, ie. > > > > > > > > IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV vs > > > > IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV > > > > > > > > + passing true args, maybe you can just record the type of the cmdqv > > > > + that is being used in the smmu_accel device. Then you can get rid of > > > > + alloc and free > > > > > > It is not just the type. Based on the type we also need to pass, > > > > > > * @data_len: Length of the type specific data > > > * @__reserved: Must be 0 > > > * @data_uptr: User pointer to a driver-specific virtual IOMMU data > > > * > > > > > > And the above is implementation specific. > > > > > > If our idea of the "ops" is to allow easier support for different > > > implementations in future, it probably makes sense to keep this. > > > > Any future "cmdqv" will likely have a different viommu type. So, > > essentially this is just a viommu type override, as Eric pointed > > out above. > > > > What we actually need to expose per viommu is type/data_len. The > > data_uptr can be a union viommu_data in the accel structure? > > So the suggestion is to get rid of alloc_viommu() and retrieve type/len > during probe() and store the data_uptr in struct accel . Doable, I guess. > > But then we need to do something similar for veventq as well, right? > > Isn't that actually bloating struct accel then? > IMHO, the ops way looks better.
The data_uptr could be dynamically allocated corresponding to the data_len, to avoid bloating. But you are right we do need a place to alloc/free a 2nd veventq. Maybe we can have a pair of alloc/free_viommu for now. Thanks Nicolin
