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

[Liu, Yi L]

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?

Regards,
Yi Liu