Re: [Xen-devel] [PATCH for-4.11] x86/VT-x: Fix determination of EFER.LMA in vmcs_dump_vcpu()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.04.18 at 10:38, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 11/04/2018 03:15, Tian, Kevin wrote:
>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>> Sent: Tuesday, April 10, 2018 4:44 PM
>>>
>>>>>> On 09.04.18 at 19:56, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>> @@ -1788,7 +1788,10 @@ void vmcs_dump_vcpu(struct vcpu *v)
>>>>      vmentry_ctl = vmr32(VM_ENTRY_CONTROLS),
>>>>      vmexit_ctl = vmr32(VM_EXIT_CONTROLS);
>>>>      cr4 = vmr(GUEST_CR4);
>>>> -    efer = vmr(GUEST_EFER);
>>>> +
>>>> +    /* EFER.LMA is read as zero, and is loaded from vmentry_ctl on entry.
>>> */
>>>> +    BUILD_BUG_ON(VM_ENTRY_IA32E_MODE << 1 != EFER_LMA);
>>>> +    efer = vmr(GUEST_EFER) | ((vmentry_ctl & VM_ENTRY_IA32E_MODE)
>>> << 1);
>>>
>>> I have to admit that - despite the BUILD_BUG_ON() - I dislike the
>>> literal 1 here, which would better be
>>> (_EFER_LMA - _VM_ENTRY_IA32E_MODE), albeit the latter doesn't
>>> exist, so perhaps
>>>
>>>     efer = vmr(GUEST_EFER) | ((vmentry_ctl & VM_ENTRY_IA32E_MODE) *
>>> (EFER_LMA / VM_ENTRY_IA32E_MODE));
>>>
>>> or the same expressed through MASK_EXTR() / MASK_INSR()? But
>>> it's the VMX maintainers to judge anyway.
>>>
>> using 1 is fine to me, with intention well explained with BUILD_BUG_ON.
>> as long as BUILD_BUG_ON is still a valid usage, I'm OK with current one:
>>
>> Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> I tried MASK_EXTR/INSR to begin with, but the code was practically
> illegible.  The * and / trick is only easy to follow if you know your
> bit hackary off by heart, and will go silently wrong if the constants
> happen to change or the operands happen to be the wrong way around.
> 
> I'm not a massive fan of the literal 1 either, but it was the most
> obvious way I could find of expressing the transformation.
> 
> Therefore, I'd prefer to keep the patch in this form unless there are
> serious objections.

As said - with Kevin being fine with the patch as is, I clearly won't
object to it going in without further changes.

Jan


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