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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/06/18 10:58, Jan Beulich wrote:
>>>> 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.

Oops yes - you're right.  I'll fix that.

~Andrew

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