On Sat, Apr 04, 2020 at 12:00:12PM +0000, Liu, Yi L wrote:
> Hi Peter,
>
> > From: Peter Xu <pet...@redhat.com>
> > Sent: Saturday, April 4, 2020 12:11 AM
> > To: Liu, Yi L <yi.l....@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 <pet...@redhat.com>
> > > > Sent: Friday, April 3, 2020 10:46 PM
> > > > To: Liu, Yi L <yi.l....@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.t...@intel.com>
> > > > > Cc: Jacob Pan <jacob.jun....@linux.intel.com>
> > > > > Cc: Peter Xu <pet...@redhat.com>
> > > > > Cc: Yi Sun <yi.y....@linux.intel.com>
> > > > > Cc: Paolo Bonzini <pbonz...@redhat.com>
> > > > > Cc: Richard Henderson <r...@twiddle.net>
> > > > > Cc: Eduardo Habkost <ehabk...@redhat.com>
> > > > > Signed-off-by: Liu Yi L <yi.l....@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,
--
Peter Xu
