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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

Thanks.

Jan