Re: [Xen-devel] [PATCH v2 5/9] x86/vmx: Improvements to LBR MSR handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.06.18 at 18:33, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/06/18 10:00, Jan Beulich wrote:
>>>>> On 12.06.18 at 10:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 12/06/2018 09:15, Jan Beulich wrote:
>>>>>>> On 08.06.18 at 20:48, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> @@ -3106,14 +3104,13 @@ static int vmx_msr_write_intercept(unsigned int 
>>>>> msr, 
> 
>>> uint64_t msr_content)
>>>>>              for ( ; (rc == 0) && lbr->count; lbr++ )
>>>>>                  for ( i = 0; (rc == 0) && (i < lbr->count); i++ )
>>>>>                      if ( (rc = vmx_add_guest_msr(v, lbr->base + i)) == 0 
>>>>> )
>>>>> -                    {
>>>>>                          vmx_clear_msr_intercept(v, lbr->base + i, 
>>> VMX_MSR_RW);
>>>>> -                        if ( lbr_tsx_fixup_needed )
>>>>> -                            v->arch.hvm_vmx.lbr_fixup_enabled |= 
>>> FIXUP_LBR_TSX;
>>>>> -                        if ( bdw_erratum_bdf14_fixup_needed )
>>>>> -                            v->arch.hvm_vmx.lbr_fixup_enabled |=
>>>>> -                                FIXUP_BDW_ERRATUM_BDF14;
>>>>> -                    }
>>>>> +
>>>>> +            v->arch.hvm_vmx.lbr_flags |= LBR_MSRS_INSERTED;
>>>>> +            if ( lbr_tsx_fixup_needed )
>>>>> +                v->arch.hvm_vmx.lbr_flags |= LBR_FIXUP_TSX;
>>>>> +            if ( bdw_erratum_bdf14_fixup_needed )
>>>>> +                v->arch.hvm_vmx.lbr_flags |= LBR_FIXUP_BDF14;
>>>> Note how the setting of the flags previously depended on
>>>> vmx_add_guest_msr() having returned success at least once.
>>> And?
>>>
>>> Unless this sequence returns fully successfully, we throw #MC into the
>>> guest without setting any kind of vMCE state.  It might be the least bad
>>> option we have available, but its also not reasonable to expect the
>>> guest to survive.
>>>
>>> The two ways to fail are ENOMEM which E2BIG.  The former is going to be
>>> causing other forms of chaos, and the latter isn't going to occur in
>>> practice because current codepaths in Xen use a maximum of ~40 or the
>>> 256 available slots.  If in the unlikely case that we fail with ENOMEM
>>> on the first entry, all the fixup logic gets short circuited due to the
>>> missing memory allocation (so practically 0 extra overhead), and the
>>> guest will still malfunction.
>>>
>>> The error handling here is sufficiently poor that I'm not worried about
>>> changing one minor corner case.  I'm actually debating whether it would
>>> be better to make the allocation at vmcs construction time, to avoid
>>> runtime out-of-memory issues.
>> With further improved MSR handling down the road, I assume we'll
>> have some entries in the list in almost all cases, so yes, I think that
>> would be desirable.
> 
> For performance reasons, we'll want to keep the size of the lists to an
> absolute minimum.
> 
> On a closer inspection, the only uses we currently have for the
> load/save lists are this new EFER case (on Gen1 hardware), the Global
> Perf Ctl (for vPMU, and we really should be using the load/save support
> like EFER), and the LBR MSRs.
> 
> Therefore, for on non-ancient hardware, a guest which doesn't touch
> MSR_DEBUGCTL is not going to need the memory allocation, so perhaps an
> up-front allocation isn't the wisest of options.  I'll keep this in mind
> during the MSR work.

Hmm, okay. In which case, if we anyway don't expect the guest to
survive here in case of some failure, why don't we crash it right away
instead of injecting #MC? Would make for a much easier to recognize
cause of the guest crash.

Jan



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