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

[Liu, Yi L]

> From: Peter Xu <address@hidden>
> Sent: Wednesday, March 25, 2020 11:16 PM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [PATCH v1 19/22] intel_iommu: process PASID-based iotlb 
> invalidation
> 
> On Wed, Mar 25, 2020 at 01:36:03PM +0000, Liu, Yi L wrote:
> > > From: Peter Xu <address@hidden>
> > > Sent: Wednesday, March 25, 2020 2:26 AM
> > > To: Liu, Yi L <address@hidden>
> > > 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 <address@hidden>
> > > > Cc: Jacob Pan <address@hidden>
> > > > Cc: Peter Xu <address@hidden>
> > > > Cc: Yi Sun <address@hidden>
> > > > Cc: Paolo Bonzini <address@hidden>
> > > > Cc: Richard Henderson <address@hidden>
> > > > Cc: Eduardo Habkost <address@hidden>
> > > > Signed-off-by: Liu Yi L <address@hidden>
> > > > ---
> > > >  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.
> 
> Yes that's exactly what I wanted to ask...  Let me try again.
> 
> I thought VTD_MAMV was only for 2nd level page table, not for pasid-iotlb
> invalidations.  So I think we should remove this "if"
> check (that corresponds to "we should even support larger AM"), right?

Right. I confirmed with spec owner. Will remove it. :-)

Regards,
Yi Liu