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

[Peter Xu]

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)?

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,

-- 
Peter Xu