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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.06.18 at 11:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 27/06/18 10:12, Jan Beulich wrote:
>>>>> On 27.06.18 at 10:43, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> The main purpose of this patch is to only ever insert the LBR MSRs into the
>>> guest load/save list once, as a future patch wants to change the behaviour 
> of
>>> vmx_add_guest_msr().
>>>
>>> The repeated processing of lbr_info and the guests MSR load/save list is
>>> redundant, and a guest using LBR itself will have to re-enable
>>> MSR_DEBUGCTL.LBR in its #DB handler, meaning that Xen will repeat this
>>> redundant processing every time the guest gets a debug exception.
>>>
>>> Rename lbr_fixup_enabled to lbr_flags to be a little more generic, and use 
>>> one
>>> bit to indicate that the MSRs have been inserted into the load/save list.
>>> Shorten the existing FIXUP* identifiers to reduce code volume.
>>>
>>> Furthermore, handing the guest #MC on an error isn't a legitimate action.  
>>> Two
>>> of the three failure cases are definitely hypervisor bugs, and the third is 
>>> a
>>> boundary case which shouldn't occur in practice.  The guest also won't 
>>> execute
>>> correctly, so handle errors by cleanly crashing the guest.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> with one minor remark:
>>
>>> @@ -3097,30 +3098,64 @@ static int vmx_msr_write_intercept(unsigned int 
>>> msr, uint64_t msr_content)
>>>              if ( vpmu_do_wrmsr(msr, msr_content, supported) )
>>>                  break;
>>>          }
>>> -        if ( msr_content & IA32_DEBUGCTLMSR_LBR )
>>> +
>>> +        /*
>>> +         * When a guest first enables LBR, arrange to save and restore the 
>>> LBR
>>> +         * MSRs and allow the guest direct access.
>>> +         *
>>> +         * MSR_DEBUGCTL and LBR has existed almost long as MSRs have 
>>> existed,
>> ... as long as ...
> 
> Not quite.  MSRs have existed since the P5, whereas LBR was introduced
> in the P6.  The MSR wasn't architectural at the time, hence no CPUID
> bit, but the newer architectural declaration is compatible with the P6
> (as far as LBR is concerned).

Perhaps a misunderstanding resulting from an ambiguity in my reply? I
should have said "... almost as long as ..."; I didn't mean for the "almost"
to be dropped.

Jan



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