> From: Peter Xu [mailto:peterx@xxxxxxxxxx] > Sent: Sunday, November 3, 2019 12:06 AM > To: Liu, Yi L <yi.l.liu@xxxxxxxxx> > Subject: Re: [RFC v2 11/22] intel_iommu: process pasid cache invalidation > > On Thu, Oct 24, 2019 at 08:34:32AM -0400, Liu Yi L wrote: > > This patch adds PASID cache invalidation handling. When guest enabled > > PASID usages (e.g. SVA), guest software should issue a proper PASID > > cache invalidation when caching-mode is exposed. This patch only adds > > the draft handling of pasid cache invalidation. Detailed handling will > > be added in subsequent patches. > > > > Cc: Kevin Tian <kevin.tian@xxxxxxxxx> > > Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx> > > Cc: Peter Xu <peterx@xxxxxxxxxx> > > Cc: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> > > Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx> > > --- > > hw/i386/intel_iommu.c | 66 ++++++++++++++++++++++++++++++++++++++-- > -- > > hw/i386/intel_iommu_internal.h | 12 ++++++++ > > hw/i386/trace-events | 3 ++ > > 3 files changed, 76 insertions(+), 5 deletions(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 88b843f..84ff6f0 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -2335,6 +2335,63 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState > *s, VTDInvDesc *inv_desc) > > return true; > > } > > > > +static int vtd_pasid_cache_dsi(IntelIOMMUState *s, uint16_t domain_id) > > +{ > > + return 0; > > +} > > + > > +static int vtd_pasid_cache_psi(IntelIOMMUState *s, > > + uint16_t domain_id, uint32_t pasid) > > +{ > > + return 0; > > +} > > + > > +static int vtd_pasid_cache_gsi(IntelIOMMUState *s) > > +{ > > + return 0; > > +} > > + > > +static bool vtd_process_pasid_desc(IntelIOMMUState *s, > > + VTDInvDesc *inv_desc) > > +{ > > + uint16_t domain_id; > > + uint32_t pasid; > > + int ret = 0; > > + > > + if ((inv_desc->val[0] & VTD_INV_DESC_PASIDC_RSVD_VAL0) || > > + (inv_desc->val[1] & VTD_INV_DESC_PASIDC_RSVD_VAL1) || > > + (inv_desc->val[2] & VTD_INV_DESC_PASIDC_RSVD_VAL2) || > > + (inv_desc->val[3] & VTD_INV_DESC_PASIDC_RSVD_VAL3)) { > > + error_report_once("non-zero-field-in-pc_inv_desc hi: 0x%" PRIx64 > > + " lo: 0x%" PRIx64, inv_desc->val[1], inv_desc->val[0]); > > + return false; > > + } > > + > > + domain_id = VTD_INV_DESC_PASIDC_DID(inv_desc->val[0]); > > + pasid = VTD_INV_DESC_PASIDC_PASID(inv_desc->val[0]); > > + > > + switch (inv_desc->val[0] & VTD_INV_DESC_PASIDC_G) { > > + case VTD_INV_DESC_PASIDC_DSI: > > + ret = vtd_pasid_cache_dsi(s, domain_id); > > + break; > > + > > + case VTD_INV_DESC_PASIDC_PASID_SI: > > + ret = vtd_pasid_cache_psi(s, domain_id, pasid); > > + break; > > + > > + case VTD_INV_DESC_PASIDC_GLOBAL: > > + ret = vtd_pasid_cache_gsi(s); > > + break; > > + > > + default: > > + error_report_once("invalid-inv-granu-in-pc_inv_desc hi: 0x%" PRIx64 > > + " lo: 0x%" PRIx64, inv_desc->val[1], inv_desc->val[0]); > > + return false; > > + } > > + > > + return (ret == 0) ? true : false; > > +} > > + > > static bool vtd_process_inv_iec_desc(IntelIOMMUState *s, > > VTDInvDesc *inv_desc) > > { > > @@ -2441,12 +2498,11 @@ static bool vtd_process_inv_desc(IntelIOMMUState > *s) > > } > > break; > > > > - /* > > - * TODO: the entity of below two cases will be implemented in future series. > > - * To make guest (which integrates scalable mode support patch set in > > - * iommu driver) work, just return true is enough so far. > > - */ > > case VTD_INV_DESC_PC: > > + trace_vtd_inv_desc("pasid-cache", inv_desc.val[1], inv_desc.val[0]); > > Could be helpful if you dump [2|3] together here... sure. Let me add it in next version. > > + if (!vtd_process_pasid_desc(s, &inv_desc)) { > > + return false; > > + } > > break; > > > > case VTD_INV_DESC_PIOTLB: > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > > index 8668771..c6cb28b 100644 > > --- a/hw/i386/intel_iommu_internal.h > > +++ b/hw/i386/intel_iommu_internal.h > > @@ -445,6 +445,18 @@ typedef union VTDInvDesc VTDInvDesc; > > #define VTD_SPTE_LPAGE_L4_RSVD_MASK(aw) \ > > (0x880ULL | ~(VTD_HAW_MASK(aw) | VTD_SL_IGN_COM)) > > > > +#define VTD_INV_DESC_PASIDC_G (3ULL << 4) > > +#define VTD_INV_DESC_PASIDC_PASID(val) (((val) >> 32) & 0xfffffULL) > > +#define VTD_INV_DESC_PASIDC_DID(val) (((val) >> 16) & > VTD_DOMAIN_ID_MASK) > > +#define VTD_INV_DESC_PASIDC_RSVD_VAL0 0xfff000000000ffc0ULL > > Nit: Mind to comment here that bit 9-11 is marked as zero rather than > reserved? This seems to work but if bit 9-11 can be non-zero in some > other descriptors then it would be clearer to define it as > 0xfff000000000f1c0ULL then explicitly check bits 9-11. > > Otherwise looks good to me. You are right. This is not reserved. It's parts of the descriptor type now. Will fix it in next version. Regards, Yi Liu
