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 02:59:10PM +0200, Jan Beulich wrote:
> On 21.04.2020 12:43, Roger Pau Monné wrote:
> > 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.
> 
> This matches my understanding.

Please bear with me, as I'm not sure if your question was because you
think the paragraph is not clear and/or should be expanded.

The point of the paragraph you mention was to have a difference
between guests running in shadow mode vs guests running in HAP mode.
Maybe I should use guest loaded page pages, to differentiate between
guest created page tables and the page tables actually loaded in cr3
in guest mode?

> >>> @@ -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.
> 
> Why would a non-shdow PV path find a need to call this function?

The memory management code in PV guests also needs to perform TLB
flushes, so I wasn't sure whether the aim was to also switch it to use
guest_flush_tlb_mask.

I guess this doesn't make a lot of sense since PV guests just need a
plain TLB flush, and there's not a lot of benefit from having a helper
around that. Maybe for PV guests running in XPTI mode, where the flush
can be avoided as the return to guest path already flushes the page
tables? Anyway, will remove the is_pv_domain check.

Thanks, Roger.