Hi Peter,
> From: Peter Xu <peterx@xxxxxxxxxx>
> Sent: Saturday, April 4, 2020 12:11 AM
> To: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> 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@xxxxxxxxxx>
> > > Sent: Friday, April 3, 2020 10:46 PM
> > > To: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > > 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@xxxxxxxxx>
> > > > Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> > > > Cc: Peter Xu <peterx@xxxxxxxxxx>
> > > > Cc: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> > > > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > > Cc: Richard Henderson <rth@xxxxxxxxxxx>
> > > > Cc: Eduardo Habkost <ehabkost@xxxxxxxxxx>
> > > > Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> > > > ---
> > > > 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
