Re: [PATCH v2 10/22] iommufd/viommmu: Add IOMMUFD_CMD_VCMDQ_ALLOC ioctl

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Apr 28, 2025 at 09:34:05PM +0000, Pranjal Shrivastava wrote:
> On Fri, Apr 25, 2025 at 10:58:05PM -0700, Nicolin Chen wrote:
> > @@ -501,6 +504,9 @@ static const struct iommufd_object_ops iommufd_object_ops[] = {
> >  	[IOMMUFD_OBJ_IOAS] = {
> >  		.destroy = iommufd_ioas_destroy,
> >  	},
> > +	[IOMMUFD_OBJ_VCMDQ] = {
> > +		.destroy = iommufd_vcmdq_destroy,
> > +	},
> >  	[IOMMUFD_OBJ_VDEVICE] = {
> >  		.destroy = iommufd_vdevice_destroy,
> >  	},
> 
> When do we expect the VMM to use this ioctl? While it's spawning a new
> VM?

When guest OS clears the VCMDQ's base address register, or when
guest OS reboots or shuts down.

> IIUC, one vintf can have multiple lvcmdqs and looking at the series
> it looks like the vcmdq_alloc allocates a single lvcmdq. Is the plan to
> dedicate one lvcmdq to per VM? Which means VMs can share a vintf?

VINTF is a vSMMU instance per SMMU. Each VINTF can have multiple
LVCMDQs. Each vCMDQ is allocated per IOMMUFD_CMD_VCMDQ_ALLOC. In
other word, VM can issue multiple IOMMUFD_CMD_VCMDQ_ALLOC calls
for each VTINF/vSMMU.

> Or do we plan to trap access to trap the access everytime the VM
> accesses an lvcmdq base register?

Yes. That's the only place the VMM can trap. All other register
accesses are going to the HW directly without trappings.

> > diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
> > index a65153458a26..02a111710ffe 100644
> > --- a/drivers/iommu/iommufd/viommu.c
> > +++ b/drivers/iommu/iommufd/viommu.c
> > @@ -170,3 +170,97 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
> >  	iommufd_put_object(ucmd->ictx, &viommu->obj);
> >  	return rc;
> >  }
> > +
> > +void iommufd_vcmdq_destroy(struct iommufd_object *obj)
> > +{
> > +	struct iommufd_vcmdq *vcmdq =
> > +		container_of(obj, struct iommufd_vcmdq, obj);
> > +	struct iommufd_viommu *viommu = vcmdq->viommu;
> > +
> > +	if (viommu->ops->vcmdq_destroy)
> > +		viommu->ops->vcmdq_destroy(vcmdq);
> > +	iopt_unpin_pages(&viommu->hwpt->ioas->iopt, vcmdq->addr, vcmdq->length);
> > +	refcount_dec(&viommu->obj.users);
> > +}
> > +
> > +int iommufd_vcmdq_alloc_ioctl(struct iommufd_ucmd *ucmd)
> > +{
> > +	struct iommu_vcmdq_alloc *cmd = ucmd->cmd;
> > +	struct iommufd_viommu *viommu;
> > +	struct iommufd_vcmdq *vcmdq;
> > +	struct page **pages;
> > +	int max_npages, i;
> > +	dma_addr_t end;
> > +	int rc;
> > +
> > +	if (cmd->flags || cmd->type == IOMMU_VCMDQ_TYPE_DEFAULT)
> > +		return -EOPNOTSUPP;
> 
> The cmd->type check is a little confusing here, I think we could
> re-order the series and add this check when we have the CMDQV type.

This is the patch that introduces the IOMMU_VCMDQ_TYPE_DEFAULT.
So, it's natural we check it here. The thing is that we have to
introduce something to fill the enum iommu_vcmdq_type, so that
it wouldn't be empty.

An unsupported DEFAULT type is what we have for vIOMMU/vEVENTQ
also.

A driver patch should define its own type along with the driver
patch. And it's what this series does. I think it's pretty clear?

> Alternatively, we could keep this in place and
[..]
> add the driver-specific
> vcmdq_alloc op calls when it's added/available for Tegra CMDQV while
> stubbing out the rest of this function accordingly.

Why?

The vcmdq_alloc op is already introduced in the prior patch. It
is cleaner to keep all core code in one patch. And then another
tegra patch to add driver type and its support.

Thanks
Nicolin