| Message ID | 1585542301-84087-17-git-send-email-yi.l.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | intel_iommu: expose Shared Virtual Addressing to VMs | expand |
On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote: > This patch replays guest pasid bindings after context cache > invalidation. This is a behavior to ensure safety. Actually, > programmer should issue pasid cache invalidation with proper > granularity after issuing a context cache invalidation. > > 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 | 51 ++++++++++++++++++++++++++++++++++++++++++ > hw/i386/intel_iommu_internal.h | 6 ++++- > hw/i386/trace-events | 1 + > 3 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index d87f608..883aeac 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -68,6 +68,10 @@ static void vtd_address_space_refresh_all(IntelIOMMUState *s); > static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n); > > static void vtd_pasid_cache_reset(IntelIOMMUState *s); > +static void vtd_pasid_cache_sync(IntelIOMMUState *s, > + VTDPASIDCacheInfo *pc_info); > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, > + VTDBus *vtd_bus, uint16_t devfn); > > static void vtd_panic_require_caching_mode(void) > { > @@ -1853,7 +1857,10 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s) > > static void vtd_context_global_invalidate(IntelIOMMUState *s) > { > + VTDPASIDCacheInfo pc_info; > + > trace_vtd_inv_desc_cc_global(); > + > /* Protects context cache */ > vtd_iommu_lock(s); > s->context_cache_gen++; > @@ -1870,6 +1877,9 @@ static void vtd_context_global_invalidate(IntelIOMMUState *s) > * VT-d emulation codes. > */ > vtd_iommu_replay_all(s); > + > + pc_info.flags = VTD_PASID_CACHE_GLOBAL; > + vtd_pasid_cache_sync(s, &pc_info); > } > > /** > @@ -2005,6 +2015,22 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s, > * happened. > */ > vtd_sync_shadow_page_table(vtd_as); > + /* > + * Per spec, context flush should also followed with PASID > + * cache and iotlb flush. Regards to a device selective > + * context cache invalidation: If context entry flush should also follow another pasid cache flush, then this is still needed? Shouldn't the pasid flush do the same thing again? > + * if (emaulted_device) > + * modify the pasid cache gen and pasid-based iotlb gen > + * value (will be added in following patches) Let's avoid using "following patches" because it'll be helpless after merged. Also, the pasid cache gen is gone. > + * else if (assigned_device) > + * check if the device has been bound to any pasid > + * invoke pasid_unbind regards to each bound pasid > + * Here, we have vtd_pasid_cache_devsi() to invalidate pasid > + * caches, while for piotlb in QEMU, we don't have it yet, so > + * no handling. For assigned device, host iommu driver would > + * flush piotlb when a pasid unbind is pass down to it. > + */ > + vtd_pasid_cache_devsi(s, vtd_bus, devfn_it); > } > } > } > @@ -2619,6 +2645,12 @@ static gboolean vtd_flush_pasid(gpointer key, gpointer value, > /* Fall through */ > case VTD_PASID_CACHE_GLOBAL: > break; > + case VTD_PASID_CACHE_DEVSI: > + if (pc_info->vtd_bus != vtd_bus || > + pc_info->devfn == devfn) { Do you mean "!="? > + return false; > + } > + break; > default: > error_report("invalid pc_info->flags"); > abort(); > @@ -2827,6 +2859,11 @@ static void vtd_replay_guest_pasid_bindings(IntelIOMMUState *s, > walk_info.flags |= VTD_PASID_TABLE_DID_SEL_WALK; > /* loop all assigned devices */ > break; > + case VTD_PASID_CACHE_DEVSI: > + walk_info.vtd_bus = pc_info->vtd_bus; > + walk_info.devfn = pc_info->devfn; > + vtd_replay_pasid_bind_for_dev(s, start, end, &walk_info); > + return; > case VTD_PASID_CACHE_FORCE_RESET: > /* For force reset, no need to go further replay */ > return; > @@ -2912,6 +2949,20 @@ static void vtd_pasid_cache_sync(IntelIOMMUState *s, > vtd_iommu_unlock(s); > } > > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, > + VTDBus *vtd_bus, uint16_t devfn) > +{ > + VTDPASIDCacheInfo pc_info; > + > + trace_vtd_pasid_cache_devsi(devfn); > + > + pc_info.flags = VTD_PASID_CACHE_DEVSI; > + pc_info.vtd_bus = vtd_bus; > + pc_info.devfn = devfn; > + > + vtd_pasid_cache_sync(s, &pc_info); > +} > + > /** > * Caller of this function should hold iommu_lock > */ > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > index b9e48ab..9122601 100644 > --- a/hw/i386/intel_iommu_internal.h > +++ b/hw/i386/intel_iommu_internal.h > @@ -529,14 +529,18 @@ struct VTDPASIDCacheInfo { > #define VTD_PASID_CACHE_GLOBAL (1ULL << 1) > #define VTD_PASID_CACHE_DOMSI (1ULL << 2) > #define VTD_PASID_CACHE_PASIDSI (1ULL << 3) > +#define VTD_PASID_CACHE_DEVSI (1ULL << 4) > uint32_t flags; > uint16_t domain_id; > uint32_t pasid; > + VTDBus *vtd_bus; > + uint16_t devfn; > }; > #define VTD_PASID_CACHE_INFO_MASK (VTD_PASID_CACHE_FORCE_RESET | \ > VTD_PASID_CACHE_GLOBAL | \ > VTD_PASID_CACHE_DOMSI | \ > - VTD_PASID_CACHE_PASIDSI) > + VTD_PASID_CACHE_PASIDSI | \ > + VTD_PASID_CACHE_DEVSI) > typedef struct VTDPASIDCacheInfo VTDPASIDCacheInfo; > > /* PASID Table Related Definitions */ > diff --git a/hw/i386/trace-events b/hw/i386/trace-events > index 60d20c1..3853fa8 100644 > --- a/hw/i386/trace-events > +++ b/hw/i386/trace-events > @@ -26,6 +26,7 @@ vtd_pasid_cache_gsi(void) "" > vtd_pasid_cache_reset(void) "" > vtd_pasid_cache_dsi(uint16_t domain) "Domian slective PC invalidation domain 0x%"PRIx16 > vtd_pasid_cache_psi(uint16_t domain, uint32_t pasid) "PASID slective PC invalidation domain 0x%"PRIx16" pasid 0x%"PRIx32 > +vtd_pasid_cache_devsi(uint16_t devfn) "Dev selective PC invalidation dev: 0x%"PRIx16 > vtd_re_not_present(uint8_t bus) "Root entry bus %"PRIu8" not present" > vtd_ce_not_present(uint8_t bus, uint8_t devfn) "Context entry bus %"PRIu8" devfn %"PRIu8" not present" > vtd_iotlb_page_hit(uint16_t sid, uint64_t addr, uint64_t slpte, uint16_t domain) "IOTLB page hit sid 0x%"PRIx16" iova 0x%"PRIx64" slpte 0x%"PRIx64" domain 0x%"PRIx16 > -- > 2.7.4 >
> From: Peter Xu <peterx@redhat.com> > Sent: Friday, April 3, 2020 10:46 PM > To: Liu, Yi L <yi.l.liu@intel.com> > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache > invalidation > > On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote: > > This patch replays guest pasid bindings after context cache > > invalidation. This is a behavior to ensure safety. Actually, > > programmer should issue pasid cache invalidation with proper > > granularity after issuing a context cache invalidation. > > > > 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 | 51 > ++++++++++++++++++++++++++++++++++++++++++ > > hw/i386/intel_iommu_internal.h | 6 ++++- > > hw/i386/trace-events | 1 + > > 3 files changed, 57 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index d87f608..883aeac 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -68,6 +68,10 @@ static void > vtd_address_space_refresh_all(IntelIOMMUState *s); > > static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n); > > > > static void vtd_pasid_cache_reset(IntelIOMMUState *s); > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s, > > + VTDPASIDCacheInfo *pc_info); > > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, > > + VTDBus *vtd_bus, uint16_t devfn); > > > > static void vtd_panic_require_caching_mode(void) > > { > > @@ -1853,7 +1857,10 @@ static void vtd_iommu_replay_all(IntelIOMMUState > *s) > > > > static void vtd_context_global_invalidate(IntelIOMMUState *s) > > { > > + VTDPASIDCacheInfo pc_info; > > + > > trace_vtd_inv_desc_cc_global(); > > + > > /* Protects context cache */ > > vtd_iommu_lock(s); > > s->context_cache_gen++; > > @@ -1870,6 +1877,9 @@ static void > vtd_context_global_invalidate(IntelIOMMUState *s) > > * VT-d emulation codes. > > */ > > vtd_iommu_replay_all(s); > > + > > + pc_info.flags = VTD_PASID_CACHE_GLOBAL; > > + vtd_pasid_cache_sync(s, &pc_info); > > } > > > > /** > > @@ -2005,6 +2015,22 @@ static void > vtd_context_device_invalidate(IntelIOMMUState *s, > > * happened. > > */ > > vtd_sync_shadow_page_table(vtd_as); > > + /* > > + * Per spec, context flush should also followed with PASID > > + * cache and iotlb flush. Regards to a device selective > > + * context cache invalidation: > > If context entry flush should also follow another pasid cache flush, > then this is still needed? Shouldn't the pasid flush do the same > thing again? yes, but how about guest software failed to follow it? It will do the same thing when pasid cache flush comes. But this only happens for the rid2pasid case (the IOVA page table). > > + * if (emaulted_device) > > + * modify the pasid cache gen and pasid-based iotlb gen > > + * value (will be added in following patches) > > Let's avoid using "following patches" because it'll be helpless after > merged. Also, the pasid cache gen is gone. got it. will modify the description here. > > + * else if (assigned_device) > > + * check if the device has been bound to any pasid > > + * invoke pasid_unbind regards to each bound pasid > > + * Here, we have vtd_pasid_cache_devsi() to invalidate pasid > > + * caches, while for piotlb in QEMU, we don't have it yet, so > > + * no handling. For assigned device, host iommu driver would > > + * flush piotlb when a pasid unbind is pass down to it. > > + */ > > + vtd_pasid_cache_devsi(s, vtd_bus, devfn_it); > > } > > } > > } > > @@ -2619,6 +2645,12 @@ static gboolean vtd_flush_pasid(gpointer key, gpointer > value, > > /* Fall through */ > > case VTD_PASID_CACHE_GLOBAL: > > break; > > + case VTD_PASID_CACHE_DEVSI: > > + if (pc_info->vtd_bus != vtd_bus || > > + pc_info->devfn == devfn) { > > Do you mean "!="? exactly. thanks for catching it. Regards, Yi Liu > > + return false; > > + } > > + break; > > default: > > error_report("invalid pc_info->flags"); > > abort(); > > @@ -2827,6 +2859,11 @@ static void > vtd_replay_guest_pasid_bindings(IntelIOMMUState *s, > > walk_info.flags |= VTD_PASID_TABLE_DID_SEL_WALK; > > /* loop all assigned devices */ > > break; > > + case VTD_PASID_CACHE_DEVSI: > > + walk_info.vtd_bus = pc_info->vtd_bus; > > + walk_info.devfn = pc_info->devfn; > > + vtd_replay_pasid_bind_for_dev(s, start, end, &walk_info); > > + return; > > case VTD_PASID_CACHE_FORCE_RESET: > > /* For force reset, no need to go further replay */ > > return; > > @@ -2912,6 +2949,20 @@ static void vtd_pasid_cache_sync(IntelIOMMUState *s, > > vtd_iommu_unlock(s); > > } > > > > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, > > + VTDBus *vtd_bus, uint16_t devfn) > > +{ > > + VTDPASIDCacheInfo pc_info; > > + > > + trace_vtd_pasid_cache_devsi(devfn); > > + > > + pc_info.flags = VTD_PASID_CACHE_DEVSI; > > + pc_info.vtd_bus = vtd_bus; > > + pc_info.devfn = devfn; > > + > > + vtd_pasid_cache_sync(s, &pc_info); > > +} > > + > > /** > > * Caller of this function should hold iommu_lock > > */ > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > > index b9e48ab..9122601 100644 > > --- a/hw/i386/intel_iommu_internal.h > > +++ b/hw/i386/intel_iommu_internal.h > > @@ -529,14 +529,18 @@ struct VTDPASIDCacheInfo { > > #define VTD_PASID_CACHE_GLOBAL (1ULL << 1) > > #define VTD_PASID_CACHE_DOMSI (1ULL << 2) > > #define VTD_PASID_CACHE_PASIDSI (1ULL << 3) > > +#define VTD_PASID_CACHE_DEVSI (1ULL << 4) > > uint32_t flags; > > uint16_t domain_id; > > uint32_t pasid; > > + VTDBus *vtd_bus; > > + uint16_t devfn; > > }; > > #define VTD_PASID_CACHE_INFO_MASK (VTD_PASID_CACHE_FORCE_RESET | > \ > > VTD_PASID_CACHE_GLOBAL | \ > > VTD_PASID_CACHE_DOMSI | \ > > - VTD_PASID_CACHE_PASIDSI) > > + VTD_PASID_CACHE_PASIDSI | \ > > + VTD_PASID_CACHE_DEVSI) > > typedef struct VTDPASIDCacheInfo VTDPASIDCacheInfo; > > > > /* PASID Table Related Definitions */ > > diff --git a/hw/i386/trace-events b/hw/i386/trace-events > > index 60d20c1..3853fa8 100644 > > --- a/hw/i386/trace-events > > +++ b/hw/i386/trace-events > > @@ -26,6 +26,7 @@ vtd_pasid_cache_gsi(void) "" > > vtd_pasid_cache_reset(void) "" > > vtd_pasid_cache_dsi(uint16_t domain) "Domian slective PC invalidation domain > 0x%"PRIx16 > > vtd_pasid_cache_psi(uint16_t domain, uint32_t pasid) "PASID slective PC > invalidation domain 0x%"PRIx16" pasid 0x%"PRIx32 > > +vtd_pasid_cache_devsi(uint16_t devfn) "Dev selective PC invalidation dev: > 0x%"PRIx16 > > vtd_re_not_present(uint8_t bus) "Root entry bus %"PRIu8" not present" > > vtd_ce_not_present(uint8_t bus, uint8_t devfn) "Context entry bus %"PRIu8" > devfn %"PRIu8" not present" > > vtd_iotlb_page_hit(uint16_t sid, uint64_t addr, uint64_t slpte, uint16_t domain) > "IOTLB page hit sid 0x%"PRIx16" iova 0x%"PRIx64" slpte 0x%"PRIx64" domain > 0x%"PRIx16 > > -- > > 2.7.4 > > > > -- > Peter Xu
On Fri, Apr 03, 2020 at 03:21:10PM +0000, Liu, Yi L wrote: > > From: Peter Xu <peterx@redhat.com> > > Sent: Friday, April 3, 2020 10:46 PM > > To: Liu, Yi L <yi.l.liu@intel.com> > > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache > > invalidation > > > > On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote: > > > This patch replays guest pasid bindings after context cache > > > invalidation. This is a behavior to ensure safety. Actually, > > > programmer should issue pasid cache invalidation with proper > > > granularity after issuing a context cache invalidation. > > > > > > 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 | 51 > > ++++++++++++++++++++++++++++++++++++++++++ > > > hw/i386/intel_iommu_internal.h | 6 ++++- > > > hw/i386/trace-events | 1 + > > > 3 files changed, 57 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > index d87f608..883aeac 100644 > > > --- a/hw/i386/intel_iommu.c > > > +++ b/hw/i386/intel_iommu.c > > > @@ -68,6 +68,10 @@ static void > > vtd_address_space_refresh_all(IntelIOMMUState *s); > > > static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n); > > > > > > static void vtd_pasid_cache_reset(IntelIOMMUState *s); > > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s, > > > + VTDPASIDCacheInfo *pc_info); > > > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, > > > + VTDBus *vtd_bus, uint16_t devfn); > > > > > > static void vtd_panic_require_caching_mode(void) > > > { > > > @@ -1853,7 +1857,10 @@ static void vtd_iommu_replay_all(IntelIOMMUState > > *s) > > > > > > static void vtd_context_global_invalidate(IntelIOMMUState *s) > > > { > > > + VTDPASIDCacheInfo pc_info; > > > + > > > trace_vtd_inv_desc_cc_global(); > > > + > > > /* Protects context cache */ > > > vtd_iommu_lock(s); > > > s->context_cache_gen++; > > > @@ -1870,6 +1877,9 @@ static void > > vtd_context_global_invalidate(IntelIOMMUState *s) > > > * VT-d emulation codes. > > > */ > > > vtd_iommu_replay_all(s); > > > + > > > + pc_info.flags = VTD_PASID_CACHE_GLOBAL; > > > + vtd_pasid_cache_sync(s, &pc_info); > > > } > > > > > > /** > > > @@ -2005,6 +2015,22 @@ static void > > vtd_context_device_invalidate(IntelIOMMUState *s, > > > * happened. > > > */ > > > vtd_sync_shadow_page_table(vtd_as); > > > + /* > > > + * Per spec, context flush should also followed with PASID > > > + * cache and iotlb flush. Regards to a device selective > > > + * context cache invalidation: > > > > If context entry flush should also follow another pasid cache flush, > > then this is still needed? Shouldn't the pasid flush do the same > > thing again? > > yes, but how about guest software failed to follow it? It will do > the same thing when pasid cache flush comes. But this only happens > for the rid2pasid case (the IOVA page table). Do you mean it will not happen when nested page table is used (so it's required for nested tables)? Yeah we can keep them for safe no matter what; at least I'm fine with it (I believe most of the code we're discussing is not fast path). Just want to be sure of it since if it's definitely duplicated then we can instead drop it. Thanks,
Hi Peter, > From: Peter Xu <peterx@redhat.com> > Sent: Saturday, April 4, 2020 12:11 AM > To: Liu, Yi L <yi.l.liu@intel.com> > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache > invalidation > > On Fri, Apr 03, 2020 at 03:21:10PM +0000, Liu, Yi L wrote: > > > From: Peter Xu <peterx@redhat.com> > > > Sent: Friday, April 3, 2020 10:46 PM > > > To: Liu, Yi L <yi.l.liu@intel.com> > > > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context > cache > > > invalidation > > > > > > On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote: > > > > This patch replays guest pasid bindings after context cache > > > > invalidation. This is a behavior to ensure safety. Actually, > > > > programmer should issue pasid cache invalidation with proper > > > > granularity after issuing a context cache invalidation. > > > > > > > > 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 | 51 > > > ++++++++++++++++++++++++++++++++++++++++++ > > > > hw/i386/intel_iommu_internal.h | 6 ++++- > > > > hw/i386/trace-events | 1 + > > > > 3 files changed, 57 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > index d87f608..883aeac 100644 > > > > --- a/hw/i386/intel_iommu.c > > > > +++ b/hw/i386/intel_iommu.c > > > > @@ -68,6 +68,10 @@ static void > > > vtd_address_space_refresh_all(IntelIOMMUState *s); > > > > static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier > *n); > > > > > > > > static void vtd_pasid_cache_reset(IntelIOMMUState *s); > > > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s, > > > > + VTDPASIDCacheInfo *pc_info); > > > > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, > > > > + VTDBus *vtd_bus, uint16_t devfn); > > > > > > > > static void vtd_panic_require_caching_mode(void) > > > > { > > > > @@ -1853,7 +1857,10 @@ static void > vtd_iommu_replay_all(IntelIOMMUState > > > *s) > > > > > > > > static void vtd_context_global_invalidate(IntelIOMMUState *s) > > > > { > > > > + VTDPASIDCacheInfo pc_info; > > > > + > > > > trace_vtd_inv_desc_cc_global(); > > > > + > > > > /* Protects context cache */ > > > > vtd_iommu_lock(s); > > > > s->context_cache_gen++; > > > > @@ -1870,6 +1877,9 @@ static void > > > vtd_context_global_invalidate(IntelIOMMUState *s) > > > > * VT-d emulation codes. > > > > */ > > > > vtd_iommu_replay_all(s); > > > > + > > > > + pc_info.flags = VTD_PASID_CACHE_GLOBAL; > > > > + vtd_pasid_cache_sync(s, &pc_info); > > > > } > > > > > > > > /** > > > > @@ -2005,6 +2015,22 @@ static void > > > vtd_context_device_invalidate(IntelIOMMUState *s, > > > > * happened. > > > > */ > > > > vtd_sync_shadow_page_table(vtd_as); > > > > + /* > > > > + * Per spec, context flush should also > > > > followed with PASID > > > > + * cache and iotlb flush. Regards to > > > > a device selective > > > > + * context cache invalidation: > > > > > > If context entry flush should also follow another pasid cache flush, > > > then this is still needed? Shouldn't the pasid flush do the same > > > thing again? > > > > yes, but how about guest software failed to follow it? It will do > > the same thing when pasid cache flush comes. But this only happens > > for the rid2pasid case (the IOVA page table). > > Do you mean it will not happen when nested page table is used (so it's > required for nested tables)? no, by the IOVA page table case, I just want to confirm the duplicate replay is true. But it is not "only" case. :-) my bad. any scalable mode context entry modification will result in duplicate replay as this patch enforces a pasid replay after context cache invalidation. But for normal guest SVM usage, it won't have such duplicate work as it only modifies pasid entry. > Yeah we can keep them for safe no matter what; at least I'm fine with > it (I believe most of the code we're discussing is not fast path). > Just want to be sure of it since if it's definitely duplicated then we > can instead drop it. yes, it is not fast path. BTW. I guess the iova shadow sync applies the same notion. right? Regards, Yi Liu
On Sat, Apr 04, 2020 at 12:00:12PM +0000, Liu, Yi L wrote: > Hi Peter, > > > From: Peter Xu <peterx@redhat.com> > > Sent: Saturday, April 4, 2020 12:11 AM > > To: Liu, Yi L <yi.l.liu@intel.com> > > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache > > invalidation > > > > On Fri, Apr 03, 2020 at 03:21:10PM +0000, Liu, Yi L wrote: > > > > From: Peter Xu <peterx@redhat.com> > > > > Sent: Friday, April 3, 2020 10:46 PM > > > > To: Liu, Yi L <yi.l.liu@intel.com> > > > > Subject: Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context > > cache > > > > invalidation > > > > > > > > On Sun, Mar 29, 2020 at 09:24:55PM -0700, Liu Yi L wrote: > > > > > This patch replays guest pasid bindings after context cache > > > > > invalidation. This is a behavior to ensure safety. Actually, > > > > > programmer should issue pasid cache invalidation with proper > > > > > granularity after issuing a context cache invalidation. > > > > > > > > > > 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 | 51 > > > > ++++++++++++++++++++++++++++++++++++++++++ > > > > > hw/i386/intel_iommu_internal.h | 6 ++++- > > > > > hw/i386/trace-events | 1 + > > > > > 3 files changed, 57 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > > index d87f608..883aeac 100644 > > > > > --- a/hw/i386/intel_iommu.c > > > > > +++ b/hw/i386/intel_iommu.c > > > > > @@ -68,6 +68,10 @@ static void > > > > vtd_address_space_refresh_all(IntelIOMMUState *s); > > > > > static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier > > *n); > > > > > > > > > > static void vtd_pasid_cache_reset(IntelIOMMUState *s); > > > > > +static void vtd_pasid_cache_sync(IntelIOMMUState *s, > > > > > + VTDPASIDCacheInfo *pc_info); > > > > > +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, > > > > > + VTDBus *vtd_bus, uint16_t devfn); > > > > > > > > > > static void vtd_panic_require_caching_mode(void) > > > > > { > > > > > @@ -1853,7 +1857,10 @@ static void > > vtd_iommu_replay_all(IntelIOMMUState > > > > *s) > > > > > > > > > > static void vtd_context_global_invalidate(IntelIOMMUState *s) > > > > > { > > > > > + VTDPASIDCacheInfo pc_info; > > > > > + > > > > > trace_vtd_inv_desc_cc_global(); > > > > > + > > > > > /* Protects context cache */ > > > > > vtd_iommu_lock(s); > > > > > s->context_cache_gen++; > > > > > @@ -1870,6 +1877,9 @@ static void > > > > vtd_context_global_invalidate(IntelIOMMUState *s) > > > > > * VT-d emulation codes. > > > > > */ > > > > > vtd_iommu_replay_all(s); > > > > > + > > > > > + pc_info.flags = VTD_PASID_CACHE_GLOBAL; > > > > > + vtd_pasid_cache_sync(s, &pc_info); > > > > > } > > > > > > > > > > /** > > > > > @@ -2005,6 +2015,22 @@ static void > > > > vtd_context_device_invalidate(IntelIOMMUState *s, > > > > > * happened. > > > > > */ > > > > > vtd_sync_shadow_page_table(vtd_as); > > > > > + /* > > > > > + * Per spec, context flush should also > > > > > followed with PASID > > > > > + * cache and iotlb flush. Regards to > > > > > a device selective > > > > > + * context cache invalidation: > > > > > > > > If context entry flush should also follow another pasid cache flush, > > > > then this is still needed? Shouldn't the pasid flush do the same > > > > thing again? > > > > > > yes, but how about guest software failed to follow it? It will do > > > the same thing when pasid cache flush comes. But this only happens > > > for the rid2pasid case (the IOVA page table). > > > > Do you mean it will not happen when nested page table is used (so it's > > required for nested tables)? > > no, by the IOVA page table case, I just want to confirm the duplicate > replay is true. But it is not "only" case. :-) my bad. any scalable mode > context entry modification will result in duplicate replay as this patch > enforces a pasid replay after context cache invalidation. But for normal > guest SVM usage, it won't have such duplicate work as it only modifies > pasid entry. > > > Yeah we can keep them for safe no matter what; at least I'm fine with > > it (I believe most of the code we're discussing is not fast path). > > Just want to be sure of it since if it's definitely duplicated then we > > can instead drop it. > > yes, it is not fast path. BTW. I guess the iova shadow sync applies > the same notion. right? Yes I rem we have similar things, but the same to that - if we can confirm that it'll be duplicated then I think we should remove that too. But feel free to ignore this question for now and keep it. The comment explaining that would be helpful, as you already did. Thanks,
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index d87f608..883aeac 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -68,6 +68,10 @@ static void vtd_address_space_refresh_all(IntelIOMMUState *s); static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n); static void vtd_pasid_cache_reset(IntelIOMMUState *s); +static void vtd_pasid_cache_sync(IntelIOMMUState *s, + VTDPASIDCacheInfo *pc_info); +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, + VTDBus *vtd_bus, uint16_t devfn); static void vtd_panic_require_caching_mode(void) { @@ -1853,7 +1857,10 @@ static void vtd_iommu_replay_all(IntelIOMMUState *s) static void vtd_context_global_invalidate(IntelIOMMUState *s) { + VTDPASIDCacheInfo pc_info; + trace_vtd_inv_desc_cc_global(); + /* Protects context cache */ vtd_iommu_lock(s); s->context_cache_gen++; @@ -1870,6 +1877,9 @@ static void vtd_context_global_invalidate(IntelIOMMUState *s) * VT-d emulation codes. */ vtd_iommu_replay_all(s); + + pc_info.flags = VTD_PASID_CACHE_GLOBAL; + vtd_pasid_cache_sync(s, &pc_info); } /** @@ -2005,6 +2015,22 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s, * happened. */ vtd_sync_shadow_page_table(vtd_as); + /* + * Per spec, context flush should also followed with PASID + * cache and iotlb flush. Regards to a device selective + * context cache invalidation: + * if (emaulted_device) + * modify the pasid cache gen and pasid-based iotlb gen + * value (will be added in following patches) + * else if (assigned_device) + * check if the device has been bound to any pasid + * invoke pasid_unbind regards to each bound pasid + * Here, we have vtd_pasid_cache_devsi() to invalidate pasid + * caches, while for piotlb in QEMU, we don't have it yet, so + * no handling. For assigned device, host iommu driver would + * flush piotlb when a pasid unbind is pass down to it. + */ + vtd_pasid_cache_devsi(s, vtd_bus, devfn_it); } } } @@ -2619,6 +2645,12 @@ static gboolean vtd_flush_pasid(gpointer key, gpointer value, /* Fall through */ case VTD_PASID_CACHE_GLOBAL: break; + case VTD_PASID_CACHE_DEVSI: + if (pc_info->vtd_bus != vtd_bus || + pc_info->devfn == devfn) { + return false; + } + break; default: error_report("invalid pc_info->flags"); abort(); @@ -2827,6 +2859,11 @@ static void vtd_replay_guest_pasid_bindings(IntelIOMMUState *s, walk_info.flags |= VTD_PASID_TABLE_DID_SEL_WALK; /* loop all assigned devices */ break; + case VTD_PASID_CACHE_DEVSI: + walk_info.vtd_bus = pc_info->vtd_bus; + walk_info.devfn = pc_info->devfn; + vtd_replay_pasid_bind_for_dev(s, start, end, &walk_info); + return; case VTD_PASID_CACHE_FORCE_RESET: /* For force reset, no need to go further replay */ return; @@ -2912,6 +2949,20 @@ static void vtd_pasid_cache_sync(IntelIOMMUState *s, vtd_iommu_unlock(s); } +static void vtd_pasid_cache_devsi(IntelIOMMUState *s, + VTDBus *vtd_bus, uint16_t devfn) +{ + VTDPASIDCacheInfo pc_info; + + trace_vtd_pasid_cache_devsi(devfn); + + pc_info.flags = VTD_PASID_CACHE_DEVSI; + pc_info.vtd_bus = vtd_bus; + pc_info.devfn = devfn; + + vtd_pasid_cache_sync(s, &pc_info); +} + /** * Caller of this function should hold iommu_lock */ diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h index b9e48ab..9122601 100644 --- a/hw/i386/intel_iommu_internal.h +++ b/hw/i386/intel_iommu_internal.h @@ -529,14 +529,18 @@ struct VTDPASIDCacheInfo { #define VTD_PASID_CACHE_GLOBAL (1ULL << 1) #define VTD_PASID_CACHE_DOMSI (1ULL << 2) #define VTD_PASID_CACHE_PASIDSI (1ULL << 3) +#define VTD_PASID_CACHE_DEVSI (1ULL << 4) uint32_t flags; uint16_t domain_id; uint32_t pasid; + VTDBus *vtd_bus; + uint16_t devfn; }; #define VTD_PASID_CACHE_INFO_MASK (VTD_PASID_CACHE_FORCE_RESET | \ VTD_PASID_CACHE_GLOBAL | \ VTD_PASID_CACHE_DOMSI | \ - VTD_PASID_CACHE_PASIDSI) + VTD_PASID_CACHE_PASIDSI | \ + VTD_PASID_CACHE_DEVSI) typedef struct VTDPASIDCacheInfo VTDPASIDCacheInfo; /* PASID Table Related Definitions */ diff --git a/hw/i386/trace-events b/hw/i386/trace-events index 60d20c1..3853fa8 100644 --- a/hw/i386/trace-events +++ b/hw/i386/trace-events @@ -26,6 +26,7 @@ vtd_pasid_cache_gsi(void) "" vtd_pasid_cache_reset(void) "" vtd_pasid_cache_dsi(uint16_t domain) "Domian slective PC invalidation domain 0x%"PRIx16 vtd_pasid_cache_psi(uint16_t domain, uint32_t pasid) "PASID slective PC invalidation domain 0x%"PRIx16" pasid 0x%"PRIx32 +vtd_pasid_cache_devsi(uint16_t devfn) "Dev selective PC invalidation dev: 0x%"PRIx16 vtd_re_not_present(uint8_t bus) "Root entry bus %"PRIu8" not present" vtd_ce_not_present(uint8_t bus, uint8_t devfn) "Context entry bus %"PRIu8" devfn %"PRIu8" not present" vtd_iotlb_page_hit(uint16_t sid, uint64_t addr, uint64_t slpte, uint16_t domain) "IOTLB page hit sid 0x%"PRIx16" iova 0x%"PRIx64" slpte 0x%"PRIx64" domain 0x%"PRIx16
This patch replays guest pasid bindings after context cache invalidation. This is a behavior to ensure safety. Actually, programmer should issue pasid cache invalidation with proper granularity after issuing a context cache invalidation. 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 | 51 ++++++++++++++++++++++++++++++++++++++++++ hw/i386/intel_iommu_internal.h | 6 ++++- hw/i386/trace-events | 1 + 3 files changed, 57 insertions(+), 1 deletion(-)