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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Jul 04, 2022 at 08:15:15AM +0200, Jan Beulich wrote:
> On 01.07.2022 17:39, Roger Pau Monné wrote:
> > On Mon, May 30, 2022 at 05:31:18PM +0200, Jan Beulich wrote:
> >> 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.
> > 
> > Yes, I've only added the bits used.  I could add all if that's better.
> 
> Personally I'd slightly prefer if you added all. But if you don't, which
> is also okay, please make this explicit 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?
> > 
> > I didn't want to change behavior for existing platforms that have
> > both architectural and model specific LBRs.
> 
> Are there such platforms? While it may not be said explicitly, so far
> I took it that the LBR format indicator being 0x3f was connected to
> arch LBR being available.

IIRC Ice Lake has both model-specific and architectural LBRs.

While format being 0x3f could indicate the likely presence of arch
LBRs, the CPUID bit need to be checked.

> >> 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.
> > 
> > LBR_CTL will only be set by Xen when the CPU only supports
> > architectural LBRs (no model specific LBR support at all), and in that
> > case LBR support won't be exposed to guests, as the ARCH_LBR CPUID is
> > not exposed, neither are guests allowed access to ARCH_LBR_CTL.
> > 
> > Note that in such scenario also setting DebugCtl.LBR has not effect, as
> > there's no model specific LBR support, and the hardware will just
> > ignore the bit.  Also none of the LBR MSRs are exposed to guests
> > either.
> 
> My question wasn't about guest support, but about us (perhaps mistakenly)
> running guest code with the Xen setting in place. It cannot be excluded
> that running with LBR enabled has a performance impact, after all.

It's possible.  'ler' option already states that it should be used for
debugging purposes only, so I think it's fine if this results in
slower guest performance, as it's not a general purpose option after
all.

Thanks, Roger.