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.
