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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> @@ -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.

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

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

Jan