Re: [PATCH v10 1/3] x86/tlb: introduce a flush HVM ASIDs flag

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

On Tue, Apr 21, 2020 at 12:21:12PM +0200, Jan Beulich wrote:
> On 16.04.2020 15:59, Roger Pau Monne wrote:
> > Introduce a specific flag to request a HVM guest linear TLB flush,
> > which is an ASID/VPID tickle that forces a guest linear to guest
> > physical TLB flush for all HVM guests.
> > 
> > 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).
> 
> I'm afraid I'm being confused by this: Even in shadow mode Xen
> doesn't modify guest page tables, does it?

I'm also confused now. It's my understand that when running in shadow
mode guest page tables are not actually used, and the guest uses Xen's
crafted shadow tables instead, which are based on the original guest
page tables suitably adjusted by Xen in order to do the p2m
translation in the HVM case, or the needed PTE adjustments in the PV
case.

So guest page tables are not modified, but are also not used as they
are never loaded into cr3.

> > @@ -254,3 +257,14 @@ unsigned int flush_area_local(const void *va, unsigned 
> > int flags)
> >  
> >      return flags;
> >  }
> > +
> > +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
> > +{
> > +    unsigned int flags = (is_pv_domain(d) || paging_mode_shadow(d) ? 
> > FLUSH_TLB
> > +                                                                   : 0) |
> > +                         (is_hvm_domain(d) && cpu_has_svm ? 
> > FLUSH_HVM_ASID_CORE
> > +                                                          : 0);
> 
> Why the is_pv_domain() part of the condition? Afaict for PV
> domains you can get here only if they have shadow mode enabled.

Right now yes, the only way to get here for PV domains is when using
shadow, but if this helper gets used in other non-shadow PV paths then
Xen's needs to do a TLB flush.

> > --- a/xen/arch/x86/mm/shadow/private.h
> > +++ b/xen/arch/x86/mm/shadow/private.h
> > @@ -818,6 +818,12 @@ static inline int sh_check_page_has_no_refs(struct 
> > page_info *page)
> >  bool shadow_flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
> >                        void *ctxt);
> >  
> > +static inline void sh_flush_local(const struct domain *d)
> > +{
> > +    flush_local(FLUSH_TLB |
> > +                (is_hvm_domain(d) && cpu_has_svm ? FLUSH_HVM_ASID_CORE : 
> > 0));
> > +}
> 
> I think the right side of | wants folding with its counterpart in
> guest_flush_tlb_mask(). Doing so would avoid guest_flush_tlb_mask()
> getting updated but this one forgotten. Perhaps split out
> guest_flush_tlb_flags() from guest_flush_tlb_mask()?

Can do.

> I also think this function should move into multi.c as long as it's
> needed only there.

Ack.

Thanks, Roger.