RE: [PATCH v1 19/22] intel_iommu: process PASID-based iotlb invalidation

Liu, Yi L
2020-03-25T13:36:14Z

> From: Peter Xu <peterx@redhat.com>
> Sent: Wednesday, March 25, 2020 2:26 AM
> To: Liu, Yi L <yi.l.liu@intel.com>
> Subject: Re: [PATCH v1 19/22] intel_iommu: process PASID-based iotlb invalidation
>
> On Sun, Mar 22, 2020 at 05:36:16AM -0700, Liu Yi L wrote:
> > This patch adds the basic PASID-based iotlb (piotlb) invalidation
> > support. piotlb is used during walking Intel VT-d 1st level page
> > table. This patch only adds the basic processing. Detailed handling
> > will be added in next patch.
> >
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Cc: Yi Sun <yi.y.sun@linux.intel.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> > ---
> > hw/i386/intel_iommu.c | 57
> ++++++++++++++++++++++++++++++++++++++++++
> > hw/i386/intel_iommu_internal.h | 13 ++++++++++
> > 2 files changed, 70 insertions(+)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > b007715..b9ac07d 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3134,6 +3134,59 @@ static bool vtd_process_pasid_desc(IntelIOMMUState
> *s,
> > return (ret == 0) ? true : false; }
> >
> > +static void vtd_piotlb_pasid_invalidate(IntelIOMMUState *s,
> > + uint16_t domain_id,
> > + uint32_t pasid) { }
> > +
> > +static void vtd_piotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
> > + uint32_t pasid, hwaddr addr, uint8_t am,
> > +bool ih) { }
> > +
> > +static bool vtd_process_piotlb_desc(IntelIOMMUState *s,
> > + VTDInvDesc *inv_desc) {
> > + uint16_t domain_id;
> > + uint32_t pasid;
> > + uint8_t am;
> > + hwaddr addr;
> > +
> > + if ((inv_desc->val[0] & VTD_INV_DESC_PIOTLB_RSVD_VAL0) ||
> > + (inv_desc->val[1] & VTD_INV_DESC_PIOTLB_RSVD_VAL1)) {
> > + error_report_once("non-zero-field-in-piotlb_inv_desc hi: 0x%" PRIx64
> > + " lo: 0x%" PRIx64, inv_desc->val[1], inv_desc->val[0]);
> > + return false;
> > + }
> > +
> > + domain_id = VTD_INV_DESC_PIOTLB_DID(inv_desc->val[0]);
> > + pasid = VTD_INV_DESC_PIOTLB_PASID(inv_desc->val[0]);
> > + switch (inv_desc->val[0] & VTD_INV_DESC_IOTLB_G) {
> > + case VTD_INV_DESC_PIOTLB_ALL_IN_PASID:
> > + vtd_piotlb_pasid_invalidate(s, domain_id, pasid);
> > + break;
> > +
> > + case VTD_INV_DESC_PIOTLB_PSI_IN_PASID:
> > + am = VTD_INV_DESC_PIOTLB_AM(inv_desc->val[1]);
> > + addr = (hwaddr) VTD_INV_DESC_PIOTLB_ADDR(inv_desc->val[1]);
> > + if (am > VTD_MAMV) {
>
> I saw this of spec 10.4.2, MAMV:
>
> Independent of value reported in this field, implementations
> supporting SMTS must support address-selective PASID-based
> IOTLB invalidations (p_iotlb_inv_dsc) with any defined address
> mask.
>
> Does it mean we should even support larger AM?
>
> Besides that, the patch looks good to me.

I don't think so. This field is for second-level table in scalable mode
and the translation table in legacy mode. For first-level table, it always
supports page selective invalidation and all the supported masks
regardless of the PSI support bit and the MAMV field in the CAP_REG.

Regards,
Yi Liu