Re: [PATCH v3 13/32] hw/arm/tegra241-cmdqv: Implement CMDQV vIOMMU alloc/free

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Mar 09, 2026 at 11:25:21AM -0700, Shameer Kolothum Thodi wrote:
> 
> 
> > -----Original Message-----
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: 09 March 2026 18:10
> > To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>
> > Cc: eric.auger@redhat.com; qemu-arm@nongnu.org; qemu-
> > devel@nongnu.org; peter.maydell@linaro.org; clg@redhat.com;
> > alex@shazbot.org; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> > <mochs@nvidia.com>; Jiandi An <jan@nvidia.com>; Jason Gunthorpe
> > <jgg@nvidia.com>; jonathan.cameron@huawei.com;
> > zhangfei.gao@linaro.org; zhenzhong.duan@intel.com; Krishnakant Jaju
> > <kjaju@nvidia.com>; phrdina@redhat.com
> > 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