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

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

[-- Attachment #1.1: Type: text/plain, Size: 5476 bytes --]

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.

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.

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

Looking at the checks when setting HVM_PARAM_NESTEDHVM, neither of the
guest feature flags are actually checked before setting up the nested
infrastructure.

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.

~Andrew

> +        return 0;
> +
> +    if ( (value & EFER_LMSLE) && !cpu_has_lmsl )
> +        return 0;
> +
> +    if ( (value & EFER_FFXSE) &&
> +         !(ext1_edx & cpufeat_mask(X86_FEATURE_FFXSR)) )
> +        return 0;
> +
> +    return 1;
>  }
>  
>  /* These reserved bits in lower 32 remain 0 after any load of CR0 */
> @@ -1763,7 +1796,6 @@ static int hvm_load_cpu_ctxt(struct doma
>      struct vcpu *v;
>      struct hvm_hw_cpu ctxt;
>      struct segment_register seg;
> -    uint64_t efer_validbits;
>  
>      /* Which vcpu is this? */
>      vcpuid = hvm_load_instance(h);
> @@ -1794,9 +1826,7 @@ static int hvm_load_cpu_ctxt(struct doma
>          return -EINVAL;
>      }
>  
> -    efer_validbits = EFER_FFXSE | EFER_LMSLE | EFER_LME | EFER_LMA
> -                   | EFER_NX | EFER_SCE;
> -    if ( !hvm_efer_valid(d, ctxt.msr_efer, efer_validbits) )
> +    if ( !hvm_efer_valid(v, ctxt.msr_efer, 1) )
>      {
>          printk(XENLOG_G_ERR "HVM%d restore: bad EFER %#" PRIx64 "\n",
>                 d->domain_id, ctxt.msr_efer);
> @@ -2936,12 +2966,10 @@ err:
>  int hvm_set_efer(uint64_t value)
>  {
>      struct vcpu *v = current;
> -    uint64_t efer_validbits;
>  
>      value &= ~EFER_LMA;
>  
> -    efer_validbits = EFER_FFXSE | EFER_LMSLE | EFER_LME | EFER_NX | EFER_SCE;
> -    if ( !hvm_efer_valid(v->domain, value, efer_validbits) )
> +    if ( !hvm_efer_valid(v, value, 0) )
>      {
>          gdprintk(XENLOG_WARNING, "Trying to set reserved bit in "
>                   "EFER: %#"PRIx64"\n", value);
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 6702 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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