Re: [PATCH v8 02/10] iommu/vt-d: Add nested translation helper function

[Jacob Pan]

On Fri, 10 Jan 2020 09:15:45 +0800
Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:

> Hi Jacob,
> 
> On 1/10/20 2:39 AM, Jacob Pan wrote:
> > On Wed, 18 Dec 2019 10:41:53 +0800
> > Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> wrote:
> >   
> >> Hi again,
> >>
> >> On 12/17/19 3:24 AM, Jacob Pan wrote:  
> >>> +/**
> >>> + * intel_pasid_setup_nested() - Set up PASID entry for nested
> >>> translation
> >>> + * which is used for vSVA. The first level page tables are used
> >>> for
> >>> + * GVA-GPA or GIOVA-GPA translation in the guest, second level
> >>> page tables
> >>> + *  are used for GPA-HPA translation.
> >>> + *
> >>> + * @iommu:      Iommu which the device belong to
> >>> + * @dev:        Device to be set up for translation
> >>> + * @gpgd:       FLPTPTR: First Level Page translation pointer in
> >>> GPA
> >>> + * @pasid:      PASID to be programmed in the device PASID table
> >>> + * @pasid_data: Additional PASID info from the guest bind request
> >>> + * @domain:     Domain info for setting up second level page
> >>> tables
> >>> + * @addr_width: Address width of the first level (guest)
> >>> + */
> >>> +int intel_pasid_setup_nested(struct intel_iommu *iommu,
> >>> +			struct device *dev, pgd_t *gpgd,
> >>> +			int pasid, struct
> >>> iommu_gpasid_bind_data_vtd *pasid_data,
> >>> +			struct dmar_domain *domain,
> >>> +			int addr_width)
> >>> +{
> >>> +	struct pasid_entry *pte;
> >>> +	struct dma_pte *pgd;
> >>> +	u64 pgd_val;
> >>> +	int agaw;
> >>> +	u16 did;
> >>> +
> >>> +	if (!ecap_nest(iommu->ecap)) {
> >>> +		pr_err("IOMMU: %s: No nested translation
> >>> support\n",
> >>> +		       iommu->name);
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	pte = intel_pasid_get_entry(dev, pasid);
> >>> +	if (WARN_ON(!pte))
> >>> +		return -EINVAL;
> >>> +
> >>> +	pasid_clear_entry(pte);  
> >>
> >> In some cases, e.g. nested mode for GIOVA-HPA, the PASID entry
> >> might have already been setup for second level translation. (This
> >> could be checked with the Present bit.) Hence, it's safe to flush
> >> caches here.
> >>
> >> Or, maybe intel_pasid_tear_down_entry() is more suitable?
> >>  
> > We don't allow binding the same device-PASID twice, so if the PASID
> > entry was used for GIOVA/RID2PASID, it should unbind first, and
> > teardown flush included, right?
> >   
> 
> Fair enough. Can you please add this as a comment to this function? So
> that the caller of this interface can know this. Or add a check in
> this function which returns error if the pasid entry has already been
> bond.
> 
Sounds good, i will do both comment and check as this:

	/*
	 * Caller must ensure PASID entry is not in use, i.e. not bind the
	 * same PASID to the same device twice.
	 */
	if (pasid_pte_is_present(pte))
		return -EBUSY;
We already have the check in the current caller.
Thanks,
> Best regards,
> baolu

[Jacob Pan]