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

Jacob Pan
2020-01-10T18:20:40Z

On Fri, 10 Jan 2020 09:15:45 +0800
Lu Baolu <baolu.lu@linux.intel.com> 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@linux.intel.com> 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]