Re: [PATCH v2 16/22] intel_iommu: replay pasid binds after context cache invalidation

[Peter Xu]

On Sat, Apr 04, 2020 at 12:00:12PM +0000, Liu, Yi L wrote:
> Hi Peter,
> 
> > From: Peter Xu <address@hidden>
> > Sent: Saturday, April 4, 2020 12:11 AM
> > To: Liu, Yi L <address@hidden>
> > 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 <address@hidden>
> > > > Sent: Friday, April 3, 2020 10:46 PM
> > > > To: Liu, Yi L <address@hidden>
> > > > 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 <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          | 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