Re: [PATCH v4 1/2] x86/vtx: add LBR_SELECT to the list of LBR MSRs

[Igor Druzhinin <igor.druzhinin@citrix.com>]

On 14/04/2021 12:41, Jan Beulich wrote:
> On 14.04.2021 06:40, Igor Druzhinin wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -2915,14 +2915,16 @@ static const struct lbr_info {
>>   }, nh_lbr[] = {
>>       { MSR_IA32_LASTINTFROMIP,       1 },
>>       { MSR_IA32_LASTINTTOIP,         1 },
>> -    { MSR_C2_LASTBRANCH_TOS,        1 },
>> +    { MSR_NHL_LBR_SELECT,           1 },
>> +    { MSR_NHL_LASTBRANCH_TOS,       1 },
>>       { MSR_P4_LASTBRANCH_0_FROM_LIP, NUM_MSR_P4_LASTBRANCH_FROM_TO },
>>       { MSR_P4_LASTBRANCH_0_TO_LIP,   NUM_MSR_P4_LASTBRANCH_FROM_TO },
>>       { 0, 0 }
>>   }, sk_lbr[] = {
>>       { MSR_IA32_LASTINTFROMIP,       1 },
>>       { MSR_IA32_LASTINTTOIP,         1 },
>> -    { MSR_SKL_LASTBRANCH_TOS,       1 },
>> +    { MSR_NHL_LBR_SELECT,           1 },
>> +    { MSR_NHL_LASTBRANCH_TOS,       1 },
>>       { MSR_SKL_LASTBRANCH_0_FROM_IP, NUM_MSR_SKL_LASTBRANCH },
>>       { MSR_SKL_LASTBRANCH_0_TO_IP,   NUM_MSR_SKL_LASTBRANCH },
>>       { MSR_SKL_LASTBRANCH_0_INFO,    NUM_MSR_SKL_LASTBRANCH },
>> @@ -2937,6 +2939,7 @@ static const struct lbr_info {
>>   }, gm_lbr[] = {
>>       { MSR_IA32_LASTINTFROMIP,       1 },
>>       { MSR_IA32_LASTINTTOIP,         1 },
>> +    { MSR_GM_LBR_SELECT,            1 },
> 
> What about Xeon Phi, Silvermont, and Airmont?

Yes, you're right - forgot about those. Will need to shuffle arrays a 
little.

>> --- a/xen/include/asm-x86/msr-index.h
>> +++ b/xen/include/asm-x86/msr-index.h
>> @@ -606,14 +606,18 @@
>>   #define NUM_MSR_C2_LASTBRANCH_FROM_TO	4
>>   #define NUM_MSR_ATOM_LASTBRANCH_FROM_TO	8
>>   
>> +/* Nehalem (and newer) last-branch recording */
>> +#define MSR_NHL_LBR_SELECT		0x000001c8
>> +#define MSR_NHL_LASTBRANCH_TOS		0x000001c9
>> +
>>   /* Skylake (and newer) last-branch recording */
>> -#define MSR_SKL_LASTBRANCH_TOS		0x000001c9
>>   #define MSR_SKL_LASTBRANCH_0_FROM_IP	0x00000680
>>   #define MSR_SKL_LASTBRANCH_0_TO_IP	0x000006c0
>>   #define MSR_SKL_LASTBRANCH_0_INFO	0x00000dc0
>>   #define NUM_MSR_SKL_LASTBRANCH		32
>>   
>>   /* Goldmont last-branch recording */
>> +#define MSR_GM_LBR_SELECT		0x000001c8
>>   #define MSR_GM_LASTBRANCH_TOS		0x000001c9
> 
> Wouldn't it make sense to also re-use the NHL constants, like you
> do for Skylake?

I didn't really see GM to be derived from NHL so decided to split those. 
Looks cleaner to me that way otherwise might be a little confusing to 
use NHL constants in GM definitions. Given the change above - I will 
have to reshuffle those anyway in v5.

Igor