Re: [PATCH v4 22/23] iommu/tegra241-cmdqv: Add user-space use support

[Nicolin Chen <nicolinc@nvidia.com>]

On Thu, May 15, 2025 at 08:27:17AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Friday, May 9, 2025 11:03 AM
> > 
> >  /**
> >   * struct iommu_hw_info_arm_smmuv3 - ARM SMMUv3 hardware
> > information
> >   *                                   (IOMMU_HW_INFO_TYPE_ARM_SMMUV3)
> >   *
> > - * @flags: Must be set to 0
> > - * @impl: Must be 0
> > + * @flags: Combination of enum iommu_hw_info_arm_smmuv3_flags
> > + * @impl: Implementation-defined bits when the following flags are set:
> > + *        - IOMMU_HW_INFO_ARM_SMMUV3_HAS_TEGRA241_CMDQV
> > + *          Bits[15:12] - Log2 of the total number of SID replacements
> > + *          Bits[11:08] - Log2 of the total number of VINTFs per vIOMMU
> > + *          Bits[07:04] - Log2 of the total number of VCMDQs per vIOMMU
> > + *          Bits[03:00] - Version number for the CMDQ-V HW
> 
> hmm throughout this series I drew an equation between VINTF
> and vIOMMU. Not sure how multiple VINTFs can be represented
> w/o introducing more objects. Do we want to keep such info here?

You are right that VINTF=vIOMMU. This is a per SMMU instance ioctl.
So, each VM should only have one VTINF/vIOMMU per SMMU instance.

For multi-VINTF (multi-vIOMMU) case, there needs to be more SMMUs
backing passthrough devices being assigned to the VM.

What exactly the concern of keeping this info here?

> > +	 * - suggest to back the queue memory with contiguous physical
> > pages or
> > +	 *   a single huge page with alignment of the queue size, limit
> > vSMMU's
> > +	 *   IDR1.CMDQS to the huge page size divided by 16 bytes
> > +	 */
> > +	IOMMU_HW_QUEUE_TYPE_TEGRA241_CMDQV = 1,
> 
> Not sure about the last sentence. 'limit' refers to a certain action
> which the user should perform?

Yes, set vSMMU's IDR1.CMDQS field up to the huge page size divided by
16 bytes, e.g. if using one 2MB huge page backing the queue memory,
VMM should set IDR1.CMDQS no higher than 17:
	2MB = (1 << 17) * 16B

Certainly, it can set to lower than 17. So it's an upper "limit".

Or any better word in your mind that can be less confusing?

> > +
> > +	ret = tegra241_vintf_init_lvcmdq(vintf, lidx, vcmdq);
> > +	if (ret)
> > +		goto undepend_vcmdq;
> > +
> > +	dev_dbg(cmdqv->dev, "%sallocated\n",
> > +		lvcmdq_error_header(vcmdq, header, 64));
> > +
> > +	tegra241_vcmdq_map_lvcmdq(vcmdq);
> > +
> > +	vcmdq->cmdq.q.q_base = q_base & VCMDQ_ADDR;
> > +	vcmdq->cmdq.q.q_base |= log2size;
> > +
> > +	ret = tegra241_vcmdq_hw_init_user(vcmdq);
> > +	if (ret)
> > +		goto unmap_lvcmdq;
> > +	vintf->lvcmdqs[lidx] = vcmdq;
> 
> this is already done in tegra241_vintf_init_lvcmdq().

Oh, will drop that.

Thanks
Nicolin