Re: [PATCH v2 for-4.11] x86/pv: Hide more EFER bits from PV guests

[Brian Woods <brian.woods@amd.com>]

On Mon, May 07, 2018 at 11:00:23AM +0100, Andrew Cooper wrote:
> We don't advertise SVM in CPUID so a PV guest shouldn't be under the
> impression that it can use SVM functionality, but despite this, it really
> shouldn't see SVME set when reading EFER.
> 
> On Intel processors, 32bit PV guests don't see, and can't use SYSCALL.
> 
> Introduce EFER_KNOWN_MASK to whitelist the features Xen knows about, and use
> this to clamp the guests view.
> 
> Take the opportunity to reuse the mask to simplify svm_vmcb_isvalid(), and
> change "undefined" to "unknown" in the print message, as there is at least
> EFER.TCE (Translation Cache Extension) defined but unknown to Xen.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> 
> Arguably, this wants backporting to the stable trees, so should be considered
> for 4.11 at this point.
> ---
>  xen/arch/x86/hvm/svm/svmdebug.c |  5 ++---
>  xen/arch/x86/pv/emul-priv-op.c  | 11 +++++++++--
>  xen/include/asm-x86/msr-index.h |  3 +++
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svmdebug.c b/xen/arch/x86/hvm/svm/svmdebug.c
> index 6c215d1..d35e405 100644
> --- a/xen/arch/x86/hvm/svm/svmdebug.c
> +++ b/xen/arch/x86/hvm/svm/svmdebug.c
> @@ -133,9 +133,8 @@ bool svm_vmcb_isvalid(const char *from, const struct vmcb_struct *vmcb,
>          PRINTF("DR7: bits [63:32] are not zero (%#"PRIx64")\n",
>                 vmcb_get_dr7(vmcb));
>  
> -    if ( efer & ~(EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | EFER_SVME |
> -                  EFER_LMSLE | EFER_FFXSE) )
> -        PRINTF("EFER: undefined bits are not zero (%#"PRIx64")\n", efer);
> +    if ( efer & ~EFER_KNOWN_MASK )
> +        PRINTF("EFER: unknown bits are not zero (%#"PRIx64")\n", efer);
>  
>      if ( hvm_efer_valid(v, efer, -1) )
>          PRINTF("EFER: %s (%"PRIx64")\n", hvm_efer_valid(v, efer, -1), efer);
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index 15f42b3..ce2ec76 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -867,9 +867,16 @@ static int read_msr(unsigned int reg, uint64_t *val,
>          return X86EMUL_OKAY;
>  
>      case MSR_EFER:
> -        *val = read_efer();
> +        /* Hide unknown bits, and unconditionally hide SVME from guests. */
> +        *val = read_efer() & EFER_KNOWN_MASK & ~EFER_SVME;
> +        /*
> +         * Hide the 64-bit features from 32-bit guests.  SCE has
> +         * vendor-dependent behaviour.
> +         */
>          if ( is_pv_32bit_domain(currd) )
> -            *val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE);
> +            *val &= ~(EFER_LME | EFER_LMA | EFER_LMSLE |
> +                      (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL
> +                       ? EFER_SCE : 0));
>          return X86EMUL_OKAY;
>  
>      case MSR_K7_FID_VID_CTL:
> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index c9f44eb..6d94d65 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -31,6 +31,9 @@
>  #define EFER_LMSLE		(1<<_EFER_LMSLE)
>  #define EFER_FFXSE		(1<<_EFER_FFXSE)
>  
> +#define EFER_KNOWN_MASK		(EFER_SCE | EFER_LME | EFER_LMA | EFER_NX | \
> +				 EFER_SVME | EFER_LMSLE | EFER_FFXSE)
> +
>  /* Speculation Controls. */
>  #define MSR_SPEC_CTRL			0x00000048
>  #define SPEC_CTRL_IBRS			(_AC(1, ULL) << 0)
> -- 
> 2.1.4
> 

Reviewed-by: Brian Woods <brian.woods@amd.com>

-- 
Brian Woods

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