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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 2/5] x86/lbr: enable hypervisor LER with arch LBR
Date: Fri, 1 Jul 2022 17:39:11 +0200	[thread overview]
Message-ID: <Yr8VH8EUahBV0p7B@MacBook-Air-de-Roger.local> (raw)
In-Reply-To: <4a193fd4-deb3-12c3-2faf-eed33e7b52af@suse.com>

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.

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

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

So I've now added extra checks in calc_ler_msr() so that it only
returns != 0 when there's LBR support (either model specific or
architectural).

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

opt_ler will be set to false if calc_ler_msr() return 0, which is the
case for non-{Intel,AMD,Hygon} or unrecognized models.

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

I can try to clarify all the above in the commit message.

Thanks, Roger.