[3/8] x86/pv: handle writes to the EFER MSR

Commit Message

Roger Pau Monné Aug. 17, 2020, 3:57 p.m. UTC

Silently drop writes to the EFER MSR for PV guests if the value is not
changed from what it's being reported. Current PV Linux will attempt
to write to the MSR with the same value that's been read, and raising
a fault will result in a guest crash.

As part of this work introduce a helper to easily get the EFER value
reported to guests.

Signed-off-by: Roger Pau Monné <[e-mail]>
---
 xen/arch/x86/pv/emul-priv-op.c | 35 ++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

Comments

Andrew Cooper Aug. 18, 2020, 1:46 p.m. UTC | #1

On 17/08/2020 16:57, Roger Pau Monne wrote:
> @@ -1005,6 +1013,13 @@ static int write_msr(unsigned int reg, uint64_t val,
>          curr->arch.pv.gs_base_user = val;
>          return X86EMUL_OKAY;
>  
> +    case MSR_EFER:
> +        /* Silently drop writes that don't change the reported value. */
> +        temp = guest_efer(currd);
> +        if ( val != temp )
> +            goto invalid;

break.

The invalid label does write-discard, rather than injecting #GP.

The comment would be clearer as "Reject writes which change the value,
but tolerate no-op writes", seeing as that is the compatibility
behaviour we're adding.

~Andrew

Patch

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index efeb2a727e..fd3cbfaebc 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -837,6 +837,23 @@  static inline bool is_cpufreq_controller(const struct domain *d)
             is_hardware_domain(d));
 }
 
+static uint64_t guest_efer(const struct domain *d)
+{
+    uint64_t val;
+
+    /* 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(d) )
+        val &= ~(EFER_LME | EFER_LMA |
+                 (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL
+                  ? EFER_SCE : 0));
+    return val;
+}
+
 static int read_msr(unsigned int reg, uint64_t *val,
                     struct x86_emulate_ctxt *ctxt)
 {
@@ -880,16 +897,7 @@  static int read_msr(unsigned int reg, uint64_t *val,
         return X86EMUL_OKAY;
 
     case MSR_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 |
-                      (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL
-                       ? EFER_SCE : 0));
+        *val = guest_efer(currd);
         return X86EMUL_OKAY;
 
     case MSR_K7_FID_VID_CTL:
@@ -1005,6 +1013,13 @@  static int write_msr(unsigned int reg, uint64_t val,
         curr->arch.pv.gs_base_user = val;
         return X86EMUL_OKAY;
 
+    case MSR_EFER:
+        /* Silently drop writes that don't change the reported value. */
+        temp = guest_efer(currd);
+        if ( val != temp )
+            goto invalid;
+        return X86EMUL_OKAY;
+
     case MSR_K7_FID_VID_STATUS:
     case MSR_K7_FID_VID_CTL:
     case MSR_K8_PSTATE_LIMIT: