Re: [PATCH v4 11/23] iommufd/viommu: Add IOMMUFD_CMD_HW_QUEUE_ALLOC ioctl

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, May 15, 2025 at 06:30:27AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Friday, May 9, 2025 11:03 AM
> > 
> > +
> > +/**
> > + * struct iommu_hw_queue_alloc - ioctl(IOMMU_HW_QUEUE_ALLOC)
> > + * @size: sizeof(struct iommu_hw_queue_alloc)
> > + * @flags: Must be 0
> > + * @viommu_id: Virtual IOMMU ID to associate the HW queue with
> > + * @type: One of enum iommu_hw_queue_type
> > + * @index: The logical index to the HW queue per virtual IOMMU for a
> > multi-queue
> > + *         model
> 
> I'm thinking of an alternative way w/o having the user to assign index
> and allowing the driver to poke object dependency (next patch).
> 
> Let's say the index is internally assigned by the driver. so this cmd is
> just for allowing a hw queue and it's the driver to decide the allocation
> policy, e.g. in ascending order.
> 
> Introduce a new flag in viommu_ops to indicate to core that the
> new hw queue should hold a reference to the previous hw queue.
> 
> core maintains a last_queue field in viommu. Upon success return
> from @hw_queue_alloc() the core increments the users refcnt of
> last_queue, records the dependency in iommufd_hw_queue struct,
> and update viommu->last_queue.
> 
> Then the destroy order is naturally guaranteed.

I have thought about that too. It's nice that the core can easily
maintain the dependency for the driver. 

But there would still need an out_index to mark each dynamically
allocated queue. So VMM would know where it should map the queue.

For example, if VMM wants to allocate a queue at its own index=1
without allocating index=0 first, kernel cannot fail that as VMM
doesn't provide the index. The only way left for kernel would be
to output the allocated queue with index=0 and then wish VMM can
validate it, which doesn't sound safe..

> > + * @out_hw_queue_id: The ID of the new HW queue
> > + * @base_addr: Base address of the queue memory in guest physical
> > address space
> > + * @length: Length of the queue memory in the guest physical address
> > space
> 
> length is agnostic to address space.

Ack.

* @length: Length of the queue memory

> > +int iommufd_hw_queue_alloc_ioctl(struct iommufd_ucmd *ucmd)
> > +{
> > +	struct iommu_hw_queue_alloc *cmd = ucmd->cmd;
> > +	struct iommufd_hw_queue *hw_queue;
> > +	struct iommufd_hwpt_paging *hwpt;
> > +	struct iommufd_viommu *viommu;
> > +	struct page **pages;
> > +	int max_npages, i;
> > +	u64 end;
> > +	int rc;
> > +
> > +	if (cmd->flags || cmd->type == IOMMU_HW_QUEUE_TYPE_DEFAULT)
> > +		return -EOPNOTSUPP;
> > +	if (!cmd->base_addr || !cmd->length)
> > +		return -EINVAL;
> > +	if (check_add_overflow(cmd->base_addr, cmd->length - 1, &end))
> > +		return -EOVERFLOW;
> > +
> > +	max_npages = DIV_ROUND_UP(cmd->length, PAGE_SIZE);
> 
> what about [base_addr, base_addr+length) spanning two pages but
> 'length' is smaller than the size of one page? 

Ah, right! Probably should be something like:

	offset = cmd->base_addr - PAGE_ALIGN(cmd->base_addr);
	max_npages = DIV_ROUND_UP(offset + cmd->length, PAGE_SIZE);

> > +	pages = kcalloc(max_npages, sizeof(*pages), GFP_KERNEL);
> > +	if (!pages)
> > +		return -ENOMEM;
> 
> this could be moved to right before iopt_pin_pages().

Ack.

> > +
> > +	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
> > +	if (IS_ERR(viommu)) {
> > +		rc = PTR_ERR(viommu);
> > +		goto out_free;
> > +	}
> > +	hwpt = viommu->hwpt;
> > +
> > +	if (!viommu->ops || !viommu->ops->hw_queue_alloc) {
> > +		rc = -EOPNOTSUPP;
> > +		goto out_put_viommu;
> > +	}
> > +
> > +	/* Quick test on the base address */
> > +	if (!iommu_iova_to_phys(hwpt->common.domain, cmd->base_addr))
> > {
> > +		rc = -ENXIO;
> > +		goto out_put_viommu;
> > +	}
> 
> this check is redundant. Actually it's not future proof, assuming that
> S2 must be pinned before the user attempts to call this cmd. But what
> if one day iommufd supports unpinned S2 (if a device is 100% PRI faultable)
> then this path will be broken.
 
OK. Let's drop it.

> > +	hw_queue = viommu->ops->hw_queue_alloc(viommu, cmd->type,
> > cmd->index,
> > +					       cmd->base_addr, cmd->length);
> > +	if (IS_ERR(hw_queue)) {
> > +		rc = PTR_ERR(hw_queue);
> > +		goto out_unpin;
> > +	}
> > +
> > +	hw_queue->viommu = viommu;
> > +	refcount_inc(&viommu->obj.users);
> > +	hw_queue->ictx = ucmd->ictx;
> 
> viommu/ictx are already set by iommufd_hw_queue_alloc().

OK. We'd need to be careful if someday there is a core-allocated
hw_queue that doesn't call iommufd_hw_queue_alloc(). Maybe I can
put a note here.

Thanks
Nicolin