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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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