Re: [PATCH] x86/svm: Use flush-by-asid when available

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

On Tue, May 05, 2020 at 07:25:39PM +0100, Andrew Cooper wrote:
> AMD Fam15h processors introduced the flush-by-asid feature, for more fine
> grain flushing purposes.
> 
> Flushing everything including ASID 0 (i.e. Xen context) is an an unnecesserily
> large hammer, and never necessary in the context of guest TLBs needing
> invalidating.
> 
> When available, use TLB_CTRL_FLUSH_ASID in preference to TLB_CTRL_FLUSH_ALL.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I should also look into restricting the usage of FLUSH_HVM_ASID_CORE
and instead perform more fine grained per-vCPU flushes when possible,
since FLUSH_HVM_ASID_CORE resets the pCPU ASID generation forcing a
new ASID to be allocated for all vCPUs running on that pCPU.

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> The APM currently describes tlb_control encoding 1 as "Flush entire
> TLB (Should be used only by legacy hypervisors.)".  AMD have agreed that this
> is misleading and should say "legacy hardware" instead.

AFAICT using TLB_CTRL_FLUSH_ASID on hardware not supporting the
feature has not been found to be safe? Ie: TLB_CTRL_FLUSH_ALL is a
subset of TLB_CTRL_FLUSH_ASID from a bitmap PoV, so if those bits
where ignored on older hardware it should be safe to unconditionally
use it.

> This change does come with a minor observed perf improvement on Fam17h
> hardware, of ~0.6s over ~22s for my XTF pagewalk test.  This test will spot
> TLB flushing issues, but isn't optimal for spotting the perf increase from
> better flushing.  There were no observed differences for Fam15h, but this
> could simply mean that the measured code footprint was larger than the TLB on
> this CPU.
> ---
>  xen/arch/x86/hvm/svm/asid.c       | 9 ++++++---
>  xen/include/asm-x86/hvm/svm/svm.h | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/asid.c b/xen/arch/x86/hvm/svm/asid.c
> index 9be90058c7..ab06dd3f3a 100644
> --- a/xen/arch/x86/hvm/svm/asid.c
> +++ b/xen/arch/x86/hvm/svm/asid.c
> @@ -18,6 +18,7 @@
>  #include <asm/amd.h>
>  #include <asm/hvm/nestedhvm.h>
>  #include <asm/hvm/svm/asid.h>
> +#include <asm/hvm/svm/svm.h>
>  
>  void svm_asid_init(const struct cpuinfo_x86 *c)
>  {
> @@ -47,15 +48,17 @@ void svm_asid_handle_vmrun(void)
>      if ( p_asid->asid == 0 )
>      {
>          vmcb_set_guest_asid(vmcb, 1);
> -        /* TODO: investigate using TLB_CTRL_FLUSH_ASID here instead. */
> -        vmcb->tlb_control = TLB_CTRL_FLUSH_ALL;
> +        vmcb->tlb_control =
> +            cpu_has_svm_flushbyasid ? TLB_CTRL_FLUSH_ASID : 
> TLB_CTRL_FLUSH_ALL;
>          return;
>      }
>  
>      if ( vmcb_get_guest_asid(vmcb) != p_asid->asid )
>          vmcb_set_guest_asid(vmcb, p_asid->asid);
>  
> -    vmcb->tlb_control = need_flush ? TLB_CTRL_FLUSH_ALL : TLB_CTRL_NO_FLUSH;
> +    vmcb->tlb_control =
> +        !need_flush ? TLB_CTRL_NO_FLUSH :
> +        cpu_has_svm_flushbyasid ? TLB_CTRL_FLUSH_ASID : TLB_CTRL_FLUSH_ALL;

Since this code structure is used in two places I would consider
locally introducing something like:

#define TLB_CTRL_FLUSH_CMD (cpu_has_svm_flushbyasid ? TLB_CTRL_FLUSH_ASID \
                                                    : TLB_CTRL_FLUSH_ALL)

To abstract it away.

Thanks, Roger.