Re: [Xen-devel] [PATCH v5 4/7] x86/tlb: introduce a flush guests TLB flag

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 28, 2020 at 05:14:05PM +0100, Jan Beulich wrote:
> On 19.02.2020 18:43, Roger Pau Monne wrote:
> > Introduce a specific flag to request a HVM guest TLB flush, which is
> > an ASID/VPID tickle that forces a linear TLB flush for all HVM guests.
> 
> Here and below, what do you mean by "linear"? I guess you mean
> TLBs holding translations from guest linear to guest physical,
> but I think this could do with then also saying so, even if it's
> more words.

Yes, will change.

> > This was previously unconditionally done in each pre_flush call, but
> > that's not required: HVM guests not using shadow don't require linear
> > TLB flushes as Xen doesn't modify the guest page tables in that case
> > (ie: when using HAP).
> 
> This explains the correctness in one direction. What about the
> removal of this from the switch_cr3_cr4() path?

AFAICT that's never used by shadow code to modify cr3 or cr4, and
hence doesn't require a guest linear TLB flush.

> And what about
> our safety assumptions from the ticking of tlbflush_clock,
> where we then imply that pages e.g. about to be freed can't
> have any translations left in any TLBs anymore?

I'm slightly confused. That flush only affects HVM guests linear TLB,
and hence is not under Xen control unless shadow mode is used. Pages
to be freed in the HAP case need to be flushed from the EPT/NPT, but
whether there are references left in the guest TLB to point to that
gfn really doesn't matter to Xen at all, since the guest is in full
control of it's MMU and TLB in that case.

For shadow any change in the guest page-tables should already be
accompanied by a guest TLB flush, or else the guest could access no
longer present entries, regardless of whether the affected pages are
freed or not.

> > --- a/xen/include/asm-x86/flushtlb.h
> > +++ b/xen/include/asm-x86/flushtlb.h
> > @@ -105,6 +105,8 @@ void switch_cr3_cr4(unsigned long cr3, unsigned long 
> > cr4);
> >  #define FLUSH_VCPU_STATE 0x1000
> >   /* Flush the per-cpu root page table */
> >  #define FLUSH_ROOT_PGTBL 0x2000
> > + /* Flush all HVM guests linear TLB (using ASID/VPID) */
> > +#define FLUSH_GUESTS_TLB 0x4000
> 
> For one, the "all" is pretty misleading. A single such request
> doesn't do this for all vCPU-s of all HVM guests, does it?

It kind of does because it tickles the pCPU ASID/VPID generation ID,
so any vCPU scheduled on the selected pCPUs will get a new ASID/VPID
allocated and thus a clean TLB.

> I'm
> also struggling with the 'S' in "GUESTS" - why is it not just
> "GUEST"? 

Any guest vCPUs running on the selected pCPUs will get a new ASID/VPID
ID and thus a clean TLB.

> I admit the names of the involved functions
> (hvm_flush_guest_tlbs(), hvm_asid_flush_core()) are somewhat
> misleading, as they don't actually do any flushing, they merely
> arrange for what is in the TLB to no longer be able to be used,
> so giving this a suitable name is "historically" complicated.
> What if we did away with the hvm_flush_guest_tlbs() wrapper,
> naming the constant here then after hvm_asid_flush_core(), e.g.
> FLUSH_ASID_CORE?

I'm not opposed to renaming. The comment before the definition was
also meant to clarify it's usage, and hence the explicit mention of
ASID/VPID.

> I also think this constant might better be zero when !HVM.

Yes, that's a good idea.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel