Re: [PATCH 3/3] x86/HVM: make hvm_efer_valid() honor guest features

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 09/01/15 11:20, Jan Beulich wrote:
>>>> On 08.01.15 at 19:49, <andrew.cooper3@citrix.com> wrote:
>> On 08/01/15 15:23, Jan Beulich wrote:
>>> Following the earlier similar change validating CR4 modifications.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -1672,20 +1672,53 @@ static int hvm_save_cpu_ctxt(struct doma
>>>      return 0;
>>>  }
>>>  
>>> -static bool_t hvm_efer_valid(struct domain *d,
>>> -                             uint64_t value, uint64_t efer_validbits)
>>> +static bool_t hvm_efer_valid(const struct vcpu *v, uint64_t value,
>>> +                             bool_t restore)
>>>  {
>>> -    if ( nestedhvm_enabled(d) && cpu_has_svm )
>>> -        efer_validbits |= EFER_SVME;
>>> +    unsigned int ext1_ecx = 0, ext1_edx = 0;
>>>  
>>> -    return !((value & ~efer_validbits) ||
>>> -             ((sizeof(long) != 8) && (value & EFER_LME)) ||
>>> -             (!cpu_has_svm && (value & EFER_SVME)) ||
>>> -             (!cpu_has_nx && (value & EFER_NX)) ||
>>> -             (!cpu_has_syscall && (value & EFER_SCE)) ||
>>> -             (!cpu_has_lmsl && (value & EFER_LMSLE)) ||
>>> -             (!cpu_has_ffxsr && (value & EFER_FFXSE)) ||
>>> -             ((value & (EFER_LME|EFER_LMA)) == EFER_LMA));
>>> +    if ( !restore && !is_hardware_domain(v->domain) )
>>> +    {
>>> +        unsigned int level;
>>> +
>>> +        ASSERT(v == current);
>>> +        hvm_cpuid(0x80000000, &level, NULL, NULL, NULL);
>>> +        if ( level >= 0x80000001 )
>>> +            hvm_cpuid(0x80000001, NULL, NULL, &ext1_ecx, &ext1_edx);
>>> +    }
>>> +    else
>>> +    {
>>> +        ext1_edx = boot_cpu_data.x86_capability[X86_FEATURE_LM / 32];
>>> +        ext1_ecx = boot_cpu_data.x86_capability[X86_FEATURE_SVM / 32];
>>> +    }
>>> +
>>> +    if ( (value & EFER_SCE) &&
>>> +         !(ext1_edx & cpufeat_mask(X86_FEATURE_SYSCALL)) )
>>> +        return 0;
>>> +
>>> +    if ( (value & (EFER_LME | EFER_LMA)) &&
>>> +         !(ext1_edx & cpufeat_mask(X86_FEATURE_LM)) )
>>> +        return 0;
>>> +
>>> +    if ( (value & EFER_LMA) && !(value & EFER_LME) )
>>> +        return 0;
>> The LME/LMA handling more complicated than this.
>>
>> LMA is read-only on Intel, but specified as read-write on AMD, with the
>> requirement that if it doesn't match the value generated by hardware, a
>> #GP fault will occur.  I believe this actually means it is read-only on
>> AMD as well.
>>
>> LMA only gets set by hardware after paging is enabled and the processor
>> switches properly into long mode, which means that there is a window
>> between setting LME and setting CR0.PG where LMA should read as 0.
> Did you note that hvm_set_efer() clears EFER_LMA before calling
> hvm_efer_valid(), thus avoiding all those issues?

I failed to appreciate its intent.

>
>> I think hvm_efer_valid() also needs the current EFER and CR0 to work out
>> what the current LMA should be, and reject any attempt to change it.
> I.e. this would be needed only for the restore path (and it not being
> done hasn't caused problems for me because guests don't normally
> get migrated before they fully enabled long mode).

Urgh.  In the restore case we don't have a current EFER to use.  In this
case I think it is acceptable to check that new_lma is (new_lme &&
!cr4.pg), which allows us to declare that the restore state is
consistent (presuming we have already validated cr4).

For the non-restore case, the caller takes care of removing LMA from
consideration.

>
>>> +    if ( (value & EFER_NX) && !(ext1_edx & cpufeat_mask(X86_FEATURE_NX)) )
>>> +        return 0;
>>> +
>>> +    if ( (value & EFER_SVME) &&
>>> +         (!(ext1_ecx & cpufeat_mask(X86_FEATURE_SVM)) ||
>>> +          !nestedhvm_enabled(v->domain)) )
>> This is going to cause an issue for the restore case, as the HVM PARAMs
>> follow the architectural state.
>>
>> I don't believe it is reasonable for nestedhvm_enabled() to disagree
>> with cpufeat_mask(X86_FEATURE_{SVM,VMX}), or for the toolstack to be
>> able to yank nestedhvm while the VM is active.
> I effectively only translated the pre-existing (and kind of suspicious
> to me) nestedhvm_enabled() use here.

So you did.  (My review focused a bit too much along the lines of "does
this new function match what the manuals state")

>
>> Looking at the checks when setting HVM_PARAM_NESTEDHVM, neither of the
>> guest feature flags are actually checked before setting up the nested
>> infrastructure.
> Which would be a separate issue.

It is indeed.  I apologize for rambling slightly - it was intended
purely as an observation.

>
>> Having said all of this, don't appear to be any migration records
>> associated with nested state (hardware-loaded VMCS/VMCB pointers,
>> currently nested?) which leads me to suspect that a domain actually
>> using nested virt is not going to survive a migration, so it might be
>> acceptable to fudge the checking of SVME for now.
> So am I getting you right that while these are all valid observations,
> you don't really ask for a change to the patch in this regard (i.e.
> taking care of above EFER_LMA issue is all that's needed, and even
> that only for the unusual case of a guest getting migrated in the
> middle of it enabling long mode on one of its CPUs)?

As the nested checks are the same as before, I am happy with them
staying as they are, and leaving work to whomever sees about fixing the
NESTEDHVM semantics.

~Andrew