[PATCH 0/8] x86: switch default MSR behavior

[PATCH 0/8] x86: switch default MSR behavior

[Roger Pau Monne]

Hello,

The current series attempts to change the current MSR default handling
behavior, which is to silently drop writes to writable MSRs, and allow
reading any MSR not explicitly handled.

After this series access to MSRs not explicitly handled will trigger a
#GP fault. I've tested this series with osstest and it doesn't introduce
any regression, at least on the boxes selected for testing:

http://logs.test-lab.xenproject.org/osstest/logs/152602/

Thanks, Roger.

Andrew Cooper (1):
  x86/hvm: Disallow access to unknown MSRs

Roger Pau Monne (7):
  x86/vmx: handle writes to MISC_ENABLE MSR
  x86/svm: silently drop writes to SYSCFG and related MSRs
  x86/pv: handle writes to the EFER MSR
  x86/pv: handle reads to the PAT MSR
  x86/pv: allow reading APIC_BASE MSR
  x86/pv: allow reading FEATURE_CONTROL MSR
  x86/pv: disallow access to unknown MSRs

 xen/arch/x86/hvm/svm/svm.c     | 21 +++++++++--
 xen/arch/x86/hvm/vmx/vmx.c     | 20 ++++++----
 xen/arch/x86/pv/emul-priv-op.c | 68 +++++++++++++++++++++++++---------
 3 files changed, 79 insertions(+), 30 deletions(-)

-- 
2.28.0

[PATCH 1/8] x86/vmx: handle writes to MISC_ENABLE MSR

[Roger Pau Monne]

Such handling consist in checking that no bits have been changed from
the read value, if that's the case silently drop the write, otherwise
inject a fault.

At least Windows guests will expect to write to the MISC_ENABLE MSR
with the same value that's been read from it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index eb54aadfba..fbfb31af05 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3166,7 +3166,7 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 
     switch ( msr )
     {
-        uint64_t rsvd;
+        uint64_t rsvd, tmp;
 
     case MSR_IA32_SYSENTER_CS:
         __vmwrite(GUEST_SYSENTER_CS, msr_content);
@@ -3304,6 +3304,13 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         /* None of these MSRs are writeable. */
         goto gp_fault;
 
+    case MSR_IA32_MISC_ENABLE:
+        /* Silently drop writes that don't change the reported value. */
+        if ( vmx_msr_read_intercept(msr, &tmp) != X86EMUL_OKAY ||
+             tmp != msr_content )
+            goto gp_fault;
+        break;
+
     case MSR_P6_PERFCTR(0)...MSR_P6_PERFCTR(7):
     case MSR_P6_EVNTSEL(0)...MSR_P6_EVNTSEL(7):
     case MSR_CORE_PERF_FIXED_CTR0...MSR_CORE_PERF_FIXED_CTR2:
-- 
2.28.0

Re: [PATCH 1/8] x86/vmx: handle writes to MISC_ENABLE MSR

[Andrew Cooper]

On 17/08/2020 16:57, Roger Pau Monne wrote:
> Such handling consist in checking that no bits have been changed from
> the read value, if that's the case silently drop the write, otherwise
> inject a fault.
>
> At least Windows guests will expect to write to the MISC_ENABLE MSR
> with the same value that's been read from it.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 2/8] x86/svm: silently drop writes to SYSCFG and related MSRs

[Roger Pau Monne]

The SYSCFG, TOP_MEM1 and TOP_MEM2 MSRs are currently exposed to guests
and writes are silently discarded. Make this explicit in the SVM code
now, and just return 0 when attempting to read any of the MSRs, while
continuing to silently drop writes.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/svm/svm.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index ca3bbfcbb3..671cdcb724 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1917,6 +1917,13 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
             goto gpf;
         break;
 
+    case MSR_K8_TOP_MEM1:
+    case MSR_K8_TOP_MEM2:
+    case MSR_K8_SYSCFG:
+        /* Return all 0s. */
+        *msr_content = 0;
+        break;
+
     case MSR_K8_VM_CR:
         *msr_content = 0;
         break;
@@ -2094,6 +2101,12 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
             goto gpf;
         break;
 
+    case MSR_K8_TOP_MEM1:
+    case MSR_K8_TOP_MEM2:
+    case MSR_K8_SYSCFG:
+        /* Drop writes. */
+        break;
+
     case MSR_K8_VM_CR:
         /* ignore write. handle all bits as read-only. */
         break;
-- 
2.28.0

Re: [PATCH 2/8] x86/svm: silently drop writes to SYSCFG and related MSRs

[Andrew Cooper]

On 17/08/2020 16:57, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index ca3bbfcbb3..671cdcb724 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1917,6 +1917,13 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>              goto gpf;
>          break;
>  
> +    case MSR_K8_TOP_MEM1:
> +    case MSR_K8_TOP_MEM2:
> +    case MSR_K8_SYSCFG:
> +        /* Return all 0s. */
> +        *msr_content = 0;

On a Rome box, these are the normal values:

0xc0010010 (SYSCFG) 0x740000
0xc001001a (MEM1) 0xb0000000
0xc001001d (MEM2) 0x3c50000000

The SYSCFG bits are MtrrFixDramEn[18], MtrrVarDramEn[20], MtrrTom2En[21]
and Tom2ForceMemTypeWB[22].  In particular, bits 18 and 20 are expected
to be set by the system firmware.

Clearly we shouldn't be leaking TOP_MEM{1,2} into guests, but Xen also
doesn't have enough information to set these correctly yet either.

At a minimum, I think SYSCFG wants to report 0x40000 which means "the
fixed MTRRs behave as expected", and the other bits being clear should
mean that TOP_MEM{1,2} aren't enabled.

~Andrew

Re: [PATCH 2/8] x86/svm: silently drop writes to SYSCFG and related MSRs

[Roger Pau Monné]

On Tue, Aug 18, 2020 at 03:53:16PM +0100, Andrew Cooper wrote:
> On 17/08/2020 16:57, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> > index ca3bbfcbb3..671cdcb724 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -1917,6 +1917,13 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
> >              goto gpf;
> >          break;
> >  
> > +    case MSR_K8_TOP_MEM1:
> > +    case MSR_K8_TOP_MEM2:
> > +    case MSR_K8_SYSCFG:
> > +        /* Return all 0s. */
> > +        *msr_content = 0;
> 
> On a Rome box, these are the normal values:
> 
> 0xc0010010 (SYSCFG) 0x740000
> 0xc001001a (MEM1) 0xb0000000
> 0xc001001d (MEM2) 0x3c50000000
> 
> The SYSCFG bits are MtrrFixDramEn[18], MtrrVarDramEn[20], MtrrTom2En[21]
> and Tom2ForceMemTypeWB[22].  In particular, bits 18 and 20 are expected
> to be set by the system firmware.
> 
> Clearly we shouldn't be leaking TOP_MEM{1,2} into guests, but Xen also
> doesn't have enough information to set these correctly yet either.
> 
> At a minimum, I think SYSCFG wants to report 0x40000 which means "the
> fixed MTRRs behave as expected", and the other bits being clear should
> mean that TOP_MEM{1,2} aren't enabled.

I didn't enable MtrrFixDramEn because AFAICT the emulated MTRR
implementation doesn't support the usage of the Extended type-field
format, and hence those bits will be 0. I'm fine with returning having
it set, as long as we don't allow setting MtrrFixDramModEn[19].

Thanks, Roger.

Re: [PATCH 2/8] x86/svm: silently drop writes to SYSCFG and related MSRs

[Jan Beulich]

On 18.08.2020 16:53, Andrew Cooper wrote:
> On 17/08/2020 16:57, Roger Pau Monne wrote:
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index ca3bbfcbb3..671cdcb724 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1917,6 +1917,13 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>              goto gpf;
>>          break;
>>  
>> +    case MSR_K8_TOP_MEM1:
>> +    case MSR_K8_TOP_MEM2:
>> +    case MSR_K8_SYSCFG:
>> +        /* Return all 0s. */
>> +        *msr_content = 0;
> 
> On a Rome box, these are the normal values:
> 
> 0xc0010010 (SYSCFG) 0x740000
> 0xc001001a (MEM1) 0xb0000000
> 0xc001001d (MEM2) 0x3c50000000
> 
> The SYSCFG bits are MtrrFixDramEn[18], MtrrVarDramEn[20], MtrrTom2En[21]
> and Tom2ForceMemTypeWB[22].  In particular, bits 18 and 20 are expected
> to be set by the system firmware.
> 
> Clearly we shouldn't be leaking TOP_MEM{1,2} into guests, but Xen also
> doesn't have enough information to set these correctly yet either.
> 
> At a minimum, I think SYSCFG wants to report 0x40000 which means "the
> fixed MTRRs behave as expected",

How do you derive that meaning? The bit tells us whether in the
individual fixed range MTRR bytes bits 3 and 4 have an AMD-
specific meaning. Guests reading these will get back zero for
the bits in all cases right now, and hence the meaning would be
"MMIO" for all of them, despite there being some RAM ranges
covered as well. Imo the bit needs to be zero to be compatible
with the rest of our code.

Jan

> and the other bits being clear should mean that TOP_MEM{1,2} aren't enabled.
> 
> ~Andrew
> 

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

[Roger Pau Monne]

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é <roger.pau@citrix.com>
---
 xen/arch/x86/pv/emul-priv-op.c | 35 ++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

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:
-- 
2.28.0

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

[Andrew Cooper]

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 4/8] x86/pv: handle reads to the PAT MSR

[Roger Pau Monne]

The value in the PAT MSR is part of the ABI between Xen and PV guests,
and there's no reason to not allow a PV guest to read it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/pv/emul-priv-op.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index fd3cbfaebc..ff87c7d769 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -900,6 +900,10 @@ static int read_msr(unsigned int reg, uint64_t *val,
         *val = guest_efer(currd);
         return X86EMUL_OKAY;
 
+    case MSR_IA32_CR_PAT:
+        *val = XEN_MSR_PAT;
+        return X86EMUL_OKAY;
+
     case MSR_K7_FID_VID_CTL:
     case MSR_K7_FID_VID_STATUS:
     case MSR_K8_PSTATE_LIMIT:
-- 
2.28.0

Re: [PATCH 4/8] x86/pv: handle reads to the PAT MSR

[Andrew Cooper]

On 17/08/2020 16:57, Roger Pau Monne wrote:
> The value in the PAT MSR is part of the ABI between Xen and PV guests,
> and there's no reason to not allow a PV guest to read it.

This is faster than using RDMSR to find the constant.

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

(Can fix up on commit)

Re: [PATCH 4/8] x86/pv: handle reads to the PAT MSR

[Roger Pau Monné]

On Tue, Aug 18, 2020 at 02:50:24PM +0100, Andrew Cooper wrote:
> On 17/08/2020 16:57, Roger Pau Monne wrote:
> > The value in the PAT MSR is part of the ABI between Xen and PV guests,
> > and there's no reason to not allow a PV guest to read it.
> 
> This is faster than using RDMSR to find the constant.
> 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> (Can fix up on commit)

Sure, I didn't write it down because I assumed it would be obvious,
but worth writing down.

Thanks, Roger.

[PATCH 5/8] x86/pv: allow reading APIC_BASE MSR

[Roger Pau Monne]

Linux PV guests will attempt to read the APIC_BASE MSR, so just report
a default value to make Linux happy.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/pv/emul-priv-op.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index ff87c7d769..554a95ae8d 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -872,6 +872,13 @@ static int read_msr(unsigned int reg, uint64_t *val,
 
     switch ( reg )
     {
+    case MSR_APIC_BASE:
+        /* Linux PV guests will attempt to read APIC_BASE. */
+        *val = APIC_BASE_ENABLE | APIC_DEFAULT_PHYS_BASE;
+        if ( !curr->vcpu_id )
+            *val |= APIC_BASE_BSP;
+        return X86EMUL_OKAY;
+
     case MSR_FS_BASE:
         if ( is_pv_32bit_domain(currd) )
             break;
-- 
2.28.0

Re: [PATCH 5/8] x86/pv: allow reading APIC_BASE MSR

[Andrew Cooper]

On 17/08/2020 16:57, Roger Pau Monne wrote:
> Linux PV guests will attempt to read the APIC_BASE MSR, so just report
> a default value to make Linux happy.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

This is a massive layering violation, but we're a decade too late to fix
it :(

[PATCH 6/8] x86/pv: allow reading FEATURE_CONTROL MSR

[Roger Pau Monne]

Linux PV guests will attempt to read the FEATURE_CONTROL MSR, report
no features enabled or available, and that the MSR is already locked.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/pv/emul-priv-op.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 554a95ae8d..76c878b677 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -879,6 +879,10 @@ static int read_msr(unsigned int reg, uint64_t *val,
             *val |= APIC_BASE_BSP;
         return X86EMUL_OKAY;
 
+    case MSR_IA32_FEATURE_CONTROL:
+        *val = IA32_FEATURE_CONTROL_LOCK;
+        return X86EMUL_OKAY;
+
     case MSR_FS_BASE:
         if ( is_pv_32bit_domain(currd) )
             break;
-- 
2.28.0

Re: [PATCH 6/8] x86/pv: allow reading FEATURE_CONTROL MSR

[Andrew Cooper]

On 17/08/2020 16:57, Roger Pau Monne wrote:
> Linux PV guests will attempt to read the FEATURE_CONTROL MSR, report
> no features enabled or available, and that the MSR is already locked.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/pv/emul-priv-op.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
> index 554a95ae8d..76c878b677 100644
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -879,6 +879,10 @@ static int read_msr(unsigned int reg, uint64_t *val,
>              *val |= APIC_BASE_BSP;
>          return X86EMUL_OKAY;
>  
> +    case MSR_IA32_FEATURE_CONTROL:
> +        *val = IA32_FEATURE_CONTROL_LOCK;
> +        return X86EMUL_OKAY;

This isn't quite right.  This is an Intel-like MSR only, and should #GP
for AMD/Hygon.

It would be better to move it to the common guest_rdmsr() function, as
the two helpers (vmce_lmce, and nested_virt) used to construct it in the
vmx code are already generic.

This also helps progress the work to drop all the legacy MSR handling.

~Andrew

[PATCH 7/8] x86/pv: disallow access to unknown MSRs

[Roger Pau Monne]

Change the catch-all behavior for MSR not explicitly handled. Instead
of allow full read-access to the MSR space and silently dropping
writes return an exception when the MSR is not explicitly handled.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/pv/emul-priv-op.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 76c878b677..fcbcf5a6c2 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -976,9 +976,10 @@ static int read_msr(unsigned int reg, uint64_t *val,
         }
         /* fall through */
     default:
+        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", reg);
+        break;
+
     normal:
-        /* Everyone can read the MSR space. */
-        /* gdprintk(XENLOG_WARNING, "Domain attempted RDMSR %08x\n", reg); */
         if ( rdmsr_safe(reg, *val) )
             break;
         return X86EMUL_OKAY;
@@ -1143,14 +1144,15 @@ static int write_msr(unsigned int reg, uint64_t val,
         }
         /* fall through */
     default:
-        if ( rdmsr_safe(reg, temp) )
-            break;
+        gdprintk(XENLOG_WARNING,
+                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
+                 reg, val);
+        break;
 
-        if ( val != temp )
     invalid:
-            gdprintk(XENLOG_WARNING,
-                     "Domain attempted WRMSR %08x from 0x%016"PRIx64" to 0x%016"PRIx64"\n",
-                     reg, temp, val);
+        gdprintk(XENLOG_WARNING,
+                 "Domain attempted WRMSR %08x from 0x%016"PRIx64" to 0x%016"PRIx64"\n",
+                 reg, temp, val);
         return X86EMUL_OKAY;
     }
 
-- 
2.28.0

Re: [PATCH 7/8] x86/pv: disallow access to unknown MSRs

[Andrew Cooper]

On 17/08/2020 16:57, Roger Pau Monne wrote:
> Change the catch-all behavior for MSR not explicitly handled. Instead
> of allow full read-access to the MSR space and silently dropping
> writes return an exception when the MSR is not explicitly handled.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 8/8] x86/hvm: Disallow access to unknown MSRs

[Roger Pau Monne]

From: Andrew Cooper <andrew.cooper3@citrix.com>

Change the catch-all behavior for MSR not explicitly handled. Instead
of allow full read-access to the MSR space and silently dropping
writes return an exception when the MSR is not explicitly handled.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/svm/svm.c |  8 ++++----
 xen/arch/x86/hvm/vmx/vmx.c | 11 ++++-------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 671cdcb724..076fa67138 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1959,6 +1959,7 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
             break;
         }
 
+        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
         goto gpf;
     }
 
@@ -2140,10 +2141,9 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         break;
 
     default:
-        /* Match up with the RDMSR side; ultimately this should go away. */
-        if ( rdmsr_safe(msr, msr_content) == 0 )
-            break;
-
+        gdprintk(XENLOG_WARNING,
+                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
+                 msr, msr_content);
         goto gpf;
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index fbfb31af05..800066da7d 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3024,9 +3024,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
             break;
         }
 
-        if ( rdmsr_safe(msr, *msr_content) == 0 )
-            break;
-
+        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
         goto gp_fault;
     }
 
@@ -3329,10 +3327,9 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
              is_last_branch_msr(msr) )
             break;
 
-        /* Match up with the RDMSR side; ultimately this should go away. */
-        if ( rdmsr_safe(msr, msr_content) == 0 )
-            break;
-
+        gdprintk(XENLOG_WARNING,
+                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
+                 msr, msr_content);
         goto gp_fault;
     }
 
-- 
2.28.0

Re: [PATCH 8/8] x86/hvm: Disallow access to unknown MSRs

[Andrew Cooper]

On 17/08/2020 16:57, Roger Pau Monne wrote:
> From: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Change the catch-all behavior for MSR not explicitly handled. Instead
> of allow full read-access to the MSR space and silently dropping
> writes return an exception when the MSR is not explicitly handled.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

It occurs to me that this hunk should be folded.

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 7b056ccc05..fdfce4f665 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3294,11 +3294,6 @@ static int vmx_msr_write_intercept(unsigned int
msr, uint64_t msr_content)
         __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
         break;
 
-    case MSR_IA32_FEATURE_CONTROL:
-    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
-        /* None of these MSRs are writeable. */
-        goto gp_fault;
-
     case MSR_IA32_MISC_ENABLE:
         /* Silently drop writes that don't change the reported value. */
         if ( vmx_msr_read_intercept(msr, &tmp) != X86EMUL_OKAY ||

now that the default: case does the right thing.

~Andrew

[PATCH 9/8] x86/msr: Drop compatibility #GP handling in guest_{rd, wr}msr()

[Andrew Cooper]

Now that the main PV/HVM MSR handlers raise #GP for all unknown MSRs, there is
no need to special case these MSRs any more.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msr.c | 46 ----------------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index ca4307e19f..a79c6ae718 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -158,29 +158,6 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
 
     switch ( msr )
     {
-    case MSR_AMD_PATCHLOADER:
-    case MSR_IA32_UCODE_WRITE:
-    case MSR_PRED_CMD:
-    case MSR_FLUSH_CMD:
-        /* Write-only */
-    case MSR_TEST_CTRL:
-    case MSR_CORE_CAPABILITIES:
-    case MSR_TSX_FORCE_ABORT:
-    case MSR_TSX_CTRL:
-    case MSR_MCU_OPT_CTRL:
-    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
-    case MSR_U_CET:
-    case MSR_S_CET:
-    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
-    case MSR_AMD64_LWP_CFG:
-    case MSR_AMD64_LWP_CBADDR:
-    case MSR_PPIN_CTL:
-    case MSR_PPIN:
-    case MSR_AMD_PPIN_CTL:
-    case MSR_AMD_PPIN:
-        /* Not offered to guests. */
-        goto gp_fault;
-
     case MSR_IA32_PLATFORM_ID:
         if ( !(cp->x86_vendor & X86_VENDOR_INTEL) ||
              !(boot_cpu_data.x86_vendor & X86_VENDOR_INTEL) )
@@ -328,29 +305,6 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     {
         uint64_t rsvd;
 
-    case MSR_IA32_PLATFORM_ID:
-    case MSR_CORE_CAPABILITIES:
-    case MSR_INTEL_CORE_THREAD_COUNT:
-    case MSR_INTEL_PLATFORM_INFO:
-    case MSR_ARCH_CAPABILITIES:
-        /* Read-only */
-    case MSR_TEST_CTRL:
-    case MSR_TSX_FORCE_ABORT:
-    case MSR_TSX_CTRL:
-    case MSR_MCU_OPT_CTRL:
-    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
-    case MSR_U_CET:
-    case MSR_S_CET:
-    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
-    case MSR_AMD64_LWP_CFG:
-    case MSR_AMD64_LWP_CBADDR:
-    case MSR_PPIN_CTL:
-    case MSR_PPIN:
-    case MSR_AMD_PPIN_CTL:
-    case MSR_AMD_PPIN:
-        /* Not offered to guests. */
-        goto gp_fault;
-
     case MSR_AMD_PATCHLEVEL:
         BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
         /*
-- 
2.11.0