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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.01.15 at 19:49, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 08/01/15 15:23, Jan Beulich wrote:
>> Following the earlier similar change validating CR4 modifications.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- 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 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).

>> +    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.

> 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.

> 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)?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel