Re: [Xen-devel] [PATCH 2/2] x86/vtx: Fixes to Haswell/Broadwell LBR TSX errata

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/10/2019 15:32, Jan Beulich wrote:
> On 28.10.2019 16:01, Andrew Cooper wrote:
>> Cross reference and list each errata, now that they are published.
> Shouldn't this be "all errata" or "each erratum"?

Probably.

>
>> @@ -2727,40 +2719,50 @@ enum
>>  
>>  static bool __read_mostly lbr_tsx_fixup_needed;
>>  static bool __read_mostly bdf93_fixup_needed;
>> -static uint32_t __read_mostly lbr_from_start;
>> -static uint32_t __read_mostly lbr_from_end;
>> -static uint32_t __read_mostly lbr_lastint_from;
>>  
>>  static void __init lbr_tsx_fixup_check(void)
>>  {
>> -    bool tsx_support = cpu_has_hle || cpu_has_rtm;
>>      uint64_t caps;
>>      uint32_t lbr_format;
>>  
>> -    /* Fixup is needed only when TSX support is disabled ... */
>> -    if ( tsx_support )
>> +    /*
>> +     * HSM182, HSD172, HSE117, BDM127, BDD117, BDF85, BDE105:
>> +     *
>> +     * On processors that do not support Intel Transactional Synchronization
>> +     * Extensions (Intel TSX) (CPUID.07H.EBX bits 4 and 11 are both zero),
>> +     * writes to MSR_LASTBRANCH_x_FROM_IP (MSR 680H to 68FH) may #GP unless
>> +     * bits[62:61] are equal to bit[47].
>> +     *
>> +     * Software should sign the MSRs.
> Missing "extend"?

Yes.

>
>> +     * Experimentally, MSR_LER_FROM_LIP (1DDH) is similarly impacted, so is
>> +     * fixed up as well.
>> +     */
>> +    if ( cpu_has_hle || cpu_has_rtm ||
>> +         boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
>> +         boot_cpu_data.x86 != 6 ||
>> +         (boot_cpu_data.x86_model != 0x3c && /* HSM182, HSD172 - 4th gen 
>> Core */
>> +          boot_cpu_data.x86_model != 0x3f && /* HSE117 - Xeon E5 v3 */
>> +          boot_cpu_data.x86_model != 0x45 && /* HSM182 - 4th gen Core */
>> +          boot_cpu_data.x86_model != 0x46 && /* HSM182, HSD172 - 4th gen 
>> Core (GT3) */
>> +          boot_cpu_data.x86_model != 0x3d && /* BDM127 - 5th gen Core */
>> +          boot_cpu_data.x86_model != 0x47 && /* BDD117 - 5th gen Core (GT3) 
>> */
>> +          boot_cpu_data.x86_model != 0x4f && /* BDF85  - Xeon E5-2600 v4 */
>> +          boot_cpu_data.x86_model != 0x56) ) /* BDE105 - Xeon D-1500 */
> Perhaps easier as switch(), as we do elsewhere?

So, I can, but it makes the logic a little awkward.  (Although less so
in this version where I moved the HLE/RTM check to the beginning).

I was pleased however to find that GCC did compile this to "subtract 3c,
upper bounds check against 1a, bit test against an immediate".

I'll see what I can do about rearranging this into a switch statement,
because we will need the horizontal space when switching to use
intel-family.h

>
>> @@ -4133,8 +4135,12 @@ static void lbr_tsx_fixup(void)
>>      struct vmx_msr_entry *msr_area = curr->arch.hvm.vmx.msr_area;
>>      struct vmx_msr_entry *msr;
>>  
>> -    if ( (msr = vmx_find_msr(curr, lbr_from_start, VMX_MSR_GUEST)) != NULL )
>> +    if ( (msr = vmx_find_msr(curr, MSR_P4_LASTBRANCH_0_FROM_LIP,
>> +                             VMX_MSR_GUEST)) != NULL )
>>      {
>> +        unsigned int lbr_from_end =
>> +            MSR_P4_LASTBRANCH_0_FROM_LIP + NUM_MSR_P4_LASTBRANCH_FROM_TO;
> const?
>
> With these largely cosmetic remarks taken care of as you see fit,
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks,

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel