Re: [PATCH 2/5] x86/lbr: enable hypervisor LER with arch LBR

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2022 15:37, Roger Pau Monne wrote:
> --- a/xen/arch/x86/include/asm/msr-index.h
> +++ b/xen/arch/x86/include/asm/msr-index.h
> @@ -139,6 +139,24 @@
>  #define  PASID_PASID_MASK                   0x000fffff
>  #define  PASID_VALID                        (_AC(1, ULL) << 31)
>  
> +#define MSR_ARCH_LBR_CTL                    0x000014ce
> +#define  ARCH_LBR_CTL_LBREN                 (_AC(1, ULL) <<  0)
> +#define  ARCH_LBR_CTL_OS                    (_AC(1, ULL) <<  1)

Bits 2 and 3 also have meaning (USR and CALL_STACK) according to
ISE version 44. If it was intentional that you omitted those
(perhaps you intended to add only the bits you actually use right
away), it would have been nice if you said so in the description.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1963,6 +1963,29 @@ void do_device_not_available(struct cpu_user_regs 
> *regs)
>  #endif
>  }
>  
> +static bool enable_lbr(void)
> +{
> +    uint64_t debugctl;
> +
> +    wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
> +    rdmsrl(MSR_IA32_DEBUGCTLMSR, debugctl);
> +    if ( !(debugctl & IA32_DEBUGCTLMSR_LBR) )
> +    {
> +        /*
> +         * CPUs with no model-specific LBRs always return DEBUGCTLMSR.LBR
> +         * == 0, attempt to set arch LBR if available.
> +         */
> +        if ( !boot_cpu_has(X86_FEATURE_ARCH_LBR) )
> +            return false;
> +
> +        /* Note that LASTINT{FROMIP,TOIP} matches LER_{FROM_IP,TO_IP} */
> +        wrmsrl(MSR_ARCH_LBR_CTL, ARCH_LBR_CTL_LBREN | ARCH_LBR_CTL_OS |
> +                                 ARCH_LBR_CTL_RECORD_ALL);
> +    }
> +
> +    return true;
> +}

Would it make sense to try architectural LBRs first?

> @@ -1997,7 +2020,7 @@ void do_debug(struct cpu_user_regs *regs)
>  
>      /* #DB automatically disabled LBR.  Reinstate it if debugging Xen. */
>      if ( cpu_has_xen_lbr )
> -        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
> +        enable_lbr();
>  
>      if ( !guest_mode(regs) )
>      {
> @@ -2179,8 +2202,8 @@ void percpu_traps_init(void)
>      if ( !ler_msr && (ler_msr = calc_ler_msr()) )
>          setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
>  
> -    if ( cpu_has_xen_lbr )
> -        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
> +    if ( cpu_has_xen_lbr && !enable_lbr() )
> +        printk(XENLOG_ERR "CPU#%u: failed to enable LBR\n", 
> smp_processor_id());
>  }

Isn't enable_lbr() failing a strong indication that we shouldn't have
set XEN_LBR just before this? IOW doesn't this want re-arranging such
that the feature bit and maybe also ler_msr (albeit some care would
be required there; in fact I think this is broken for the case of
running on non-{Intel,AMD,Hygon} CPUs [or unrecognized models] but
opt_ler being true) remain unset in that case?

As there's no good place to ask the VMX-related question, it needs to
go here: Aiui with this patch in place VMX guests will be run with
Xen's choice of LBR_CTL. That's different from DebugCtl, which - being
part of the VMCS - would be loaded by the CPU. Such a difference, if
intended, would imo again want pointing out in the description.

Jan