[PATCH v2 0/5] x86: Switch some bits of MSR handing over to the new infrastructure

[PATCH v2 0/5] x86: Switch some bits of MSR handing over to the new infrastructure

[Andrew Cooper]

Various changes to MSR handling which don't impact the MSR policy objects
themselves.  See individual patches for details.

Andrew Cooper (5):
  x86/hvm: Handle viridian MSRs via the new guest_{rd,wr}msr() infrastructure
  x86: Handle the Xen MSRs via the new guest_{rd,wr}msr() infrastructure
  x86: Fix APIC MSR constant names
  x86/hvm: Handle x2apic MSRs via the new guest_{rd,wr}msr() infrastructure
  x86/msr: Blacklist various MSRs which guests definitely shouldn't be using

 xen/arch/x86/apic.c                |  66 +++++++++++------------
 xen/arch/x86/genapic/x2apic.c      |   4 +-
 xen/arch/x86/hvm/hvm.c             |  14 +----
 xen/arch/x86/hvm/svm/svm.c         |  27 ++--------
 xen/arch/x86/hvm/viridian.c        |  52 ++++++++----------
 xen/arch/x86/hvm/vlapic.c          |  82 +++++++++++++++-------------
 xen/arch/x86/hvm/vmx/vmx.c         |  48 +++++------------
 xen/arch/x86/msr.c                 | 106 +++++++++++++++++++++++++++++++++++--
 xen/arch/x86/pv/emul-priv-op.c     |   6 ---
 xen/arch/x86/traps.c               |  36 ++++++-------
 xen/include/asm-x86/hvm/hvm.h      |   4 +-
 xen/include/asm-x86/hvm/viridian.h |  11 +---
 xen/include/asm-x86/hvm/vlapic.h   |   6 +--
 xen/include/asm-x86/msr-index.h    |  47 +++++++++++-----
 xen/include/asm-x86/processor.h    |   4 +-
 15 files changed, 287 insertions(+), 226 deletions(-)

-- 
2.1.4


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

[PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

[Andrew Cooper]

Dispatch from the guest_{rd,wr}msr() functions, after confirming that the
domain is configured to use viridian.  This allows for simplifiction of the
viridian helpers as they don't need to cope with the "not a viridian MSR"
case.  It also means that viridian MSRs which are unimplemented, or excluded
because of features, don't fall back into default handling path.

Rename {rd,wr}msr_viridian_regs() to guest_{rd,wr}msr_viridian() for
consistency, and because the _regs suffix isn't very appropriate.

Update them to take a vcpu pointer rather than presuming that they act on
current, which is safe for all implemented operations.  Also update them to
use X86EMUL_* return values.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>

v2:
 * Introduce some constants for the hypervisor range.  However, I do not think
   the current code is an improvement over bare numbers.
---
 xen/arch/x86/hvm/svm/svm.c         |  6 +----
 xen/arch/x86/hvm/viridian.c        | 52 +++++++++++++++++---------------------
 xen/arch/x86/hvm/vmx/vmx.c         |  6 +----
 xen/arch/x86/msr.c                 | 41 +++++++++++++++++++++++++++---
 xen/include/asm-x86/hvm/viridian.h | 11 ++------
 xen/include/asm-x86/msr-index.h    |  4 +++
 6 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c34f5b5..e8ac2d8 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1966,8 +1966,7 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         else if ( ret )
             break;
 
-        if ( rdmsr_viridian_regs(msr, msr_content) ||
-             rdmsr_hypervisor_regs(msr, msr_content) )
+        if ( rdmsr_hypervisor_regs(msr, msr_content) )
             break;
 
         if ( rdmsr_safe(msr, *msr_content) == 0 )
@@ -2122,9 +2121,6 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         else if ( ret )
             break;
 
-        if ( wrmsr_viridian_regs(msr, msr_content) )
-            break;
-
         switch ( wrmsr_hypervisor_regs(msr, msr_content) )
         {
         case -ERESTART:
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 70aab52..074d920 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -88,9 +88,6 @@
 #define HV_X64_MSR_CRASH_P4                      0x40000104
 #define HV_X64_MSR_CRASH_CTL                     0x40000105
 
-#define VIRIDIAN_MSR_MIN HV_X64_MSR_GUEST_OS_ID
-#define VIRIDIAN_MSR_MAX HV_X64_MSR_CRASH_CTL
-
 /* Viridian Hypercall Status Codes. */
 #define HV_STATUS_SUCCESS                       0x0000
 #define HV_STATUS_INVALID_HYPERCALL_CODE        0x0002
@@ -554,13 +551,11 @@ static void update_reference_tsc(struct domain *d, bool_t initialize)
     put_page_and_type(page);
 }
 
-int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
+int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
 {
-    struct vcpu *v = current;
     struct domain *d = v->domain;
 
-    if ( !is_viridian_domain(d) )
-        return 0;
+    ASSERT(is_viridian_domain(d));
 
     switch ( idx )
     {
@@ -615,7 +610,7 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
 
     case HV_X64_MSR_REFERENCE_TSC:
         if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
-            return 0;
+            goto gp_fault;
 
         perfc_incr(mshv_wrmsr_tsc_msr);
         d->arch.hvm_domain.viridian.reference_tsc.raw = val;
@@ -655,14 +650,15 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
     }
 
     default:
-        if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
-            gprintk(XENLOG_WARNING, "write to unimplemented MSR %#x\n",
-                    idx);
-
-        return 0;
+        gdprintk(XENLOG_WARNING,
+                 "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx);
+        goto gp_fault;
     }
 
-    return 1;
+    return X86EMUL_OKAY;
+
+ gp_fault:
+    return X86EMUL_EXCEPTION;
 }
 
 static int64_t raw_trc_val(struct domain *d)
@@ -698,13 +694,11 @@ void viridian_time_ref_count_thaw(struct domain *d)
         trc->off = (int64_t)trc->val - raw_trc_val(d);
 }
 
-int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
+int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val)
 {
-    struct vcpu *v = current;
     struct domain *d = v->domain;
-    
-    if ( !is_viridian_domain(d) )
-        return 0;
+
+    ASSERT(is_viridian_domain(d));
 
     switch ( idx )
     {
@@ -725,7 +719,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
 
     case HV_X64_MSR_TSC_FREQUENCY:
         if ( viridian_feature_mask(d) & HVMPV_no_freq )
-            return 0;
+            goto gp_fault;
 
         perfc_incr(mshv_rdmsr_tsc_frequency);
         *val = (uint64_t)d->arch.tsc_khz * 1000ull;
@@ -733,7 +727,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
 
     case HV_X64_MSR_APIC_FREQUENCY:
         if ( viridian_feature_mask(d) & HVMPV_no_freq )
-            return 0;
+            goto gp_fault;
 
         perfc_incr(mshv_rdmsr_apic_frequency);
         *val = 1000000000ull / APIC_BUS_CYCLE_NS;
@@ -757,7 +751,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
 
     case HV_X64_MSR_REFERENCE_TSC:
         if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
-            return 0;
+            goto gp_fault;
 
         perfc_incr(mshv_rdmsr_tsc_msr);
         *val = d->arch.hvm_domain.viridian.reference_tsc.raw;
@@ -770,7 +764,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
         trc = &d->arch.hvm_domain.viridian.time_ref_count;
 
         if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
-            return 0;
+            goto gp_fault;
 
         if ( !test_and_set_bit(_TRC_accessed, &trc->flags) )
             printk(XENLOG_G_INFO "d%d: VIRIDIAN MSR_TIME_REF_COUNT: accessed\n",
@@ -804,14 +798,14 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
     }
 
     default:
-        if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
-            gprintk(XENLOG_WARNING, "read from unimplemented MSR %#x\n",
-                    idx);
-
-        return 0;
+        gdprintk(XENLOG_WARNING, "Read from unimplemented MSR %#x\n", idx);
+        goto gp_fault;
     }
 
-    return 1;
+    return X86EMUL_OKAY;
+
+ gp_fault:
+    return X86EMUL_EXCEPTION;
 }
 
 void viridian_vcpu_deinit(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 18d8ce2..5fc21a5 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2909,8 +2909,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
             break;
         }
 
-        if ( rdmsr_viridian_regs(msr, msr_content) ||
-             rdmsr_hypervisor_regs(msr, msr_content) )
+        if ( rdmsr_hypervisor_regs(msr, msr_content) )
             break;
 
         if ( rdmsr_safe(msr, *msr_content) == 0 )
@@ -3150,9 +3149,6 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         if ( passive_domain_do_wrmsr(msr, msr_content) )
             return X86EMUL_OKAY;
 
-        if ( wrmsr_viridian_regs(msr, msr_content) ) 
-            break;
-
         if ( vmx_write_guest_msr(msr, msr_content) == 0 ||
              is_last_branch_msr(msr) )
             break;
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 8ae3b4e..2fc2021 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -141,9 +141,11 @@ int init_vcpu_msr_policy(struct vcpu *v)
 
 int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
 {
-    const struct cpuid_policy *cp = v->domain->arch.cpuid;
-    const struct msr_domain_policy *dp = v->domain->arch.msr;
+    const struct domain *d = v->domain;
+    const struct cpuid_policy *cp = d->arch.cpuid;
+    const struct msr_domain_policy *dp = d->arch.msr;
     const struct msr_vcpu_policy *vp = v->arch.msr;
+    int ret = X86EMUL_OKAY;
 
     switch ( msr )
     {
@@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
                _MSR_MISC_FEATURES_CPUID_FAULTING;
         break;
 
+    case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1:
+        if ( is_viridian_domain(d) )
+        {
+            ret = guest_rdmsr_viridian(v, msr, val);
+            goto out;
+        }
+
+        /* Fallthrough. */
     default:
         return X86EMUL_UNHANDLEABLE;
     }
 
-    return X86EMUL_OKAY;
+ out:
+    /*
+     * Check that functions we dispatch to don't end up returning
+     * X86EMUL_UNHANDLEABLE, as that interferes with the transitionary period
+     * meaning of "fall back to the legacy MSR handlers".
+     */
+    ASSERT(ret != X86EMUL_UNHANDLEABLE);
+    return ret;
 
  gp_fault:
     return X86EMUL_EXCEPTION;
@@ -192,6 +209,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     const struct cpuid_policy *cp = d->arch.cpuid;
     struct msr_domain_policy *dp = d->arch.msr;
     struct msr_vcpu_policy *vp = v->arch.msr;
+    int ret = X86EMUL_OKAY;
 
     switch ( msr )
     {
@@ -250,11 +268,26 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
     }
 
+    case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1:
+        if ( is_viridian_domain(d) )
+        {
+            ret = guest_wrmsr_viridian(v, msr, val);
+            goto out;
+        }
+
+        /* Fallthrough. */
     default:
         return X86EMUL_UNHANDLEABLE;
     }
 
-    return X86EMUL_OKAY;
+ out:
+    /*
+     * Check that functions we dispatch to don't end up returning
+     * X86EMUL_UNHANDLEABLE, as that interferes with the transitionary period
+     * meaning of "fall back to the legacy MSR handlers".
+     */
+    ASSERT(ret != X86EMUL_UNHANDLEABLE);
+    return ret;
 
  gp_fault:
     return X86EMUL_EXCEPTION;
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 4cbd133..071fb44 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -101,15 +101,8 @@ struct viridian_domain
 void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
                            uint32_t subleaf, struct cpuid_leaf *res);
 
-int
-wrmsr_viridian_regs(
-    uint32_t idx,
-    uint64_t val);
-
-int
-rdmsr_viridian_regs(
-    uint32_t idx,
-    uint64_t *val);
+int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val);
+int guest_rdmsr_viridian(const struct vcpu *v, uint32_t idx, uint64_t *val);
 
 int
 viridian_hypercall(struct cpu_user_regs *regs);
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 23ad743..ee029a2 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -540,4 +540,8 @@
 #define MSR_PKGC9_IRTL			0x00000634
 #define MSR_PKGC10_IRTL			0x00000635
 
+/* Hypervisor leaves in the 0x4xxxxxxx range. */
+#define MSR_HYPERVISOR_START            0x40000000
+#define NR_VIRIDIAN_MSRS                0x00000200
+
 #endif /* __ASM_MSR_INDEX_H */
-- 
2.1.4


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

Re: [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

[Jan Beulich]

>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
> @@ -554,13 +551,11 @@ static void update_reference_tsc(struct domain *d, bool_t initialize)
>      put_page_and_type(page);
>  }
>  
> -int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> +int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
>  {
> -    struct vcpu *v = current;
>      struct domain *d = v->domain;
>  
> -    if ( !is_viridian_domain(d) )
> -        return 0;
> +    ASSERT(is_viridian_domain(d));
>  
>      switch ( idx )
>      {
> @@ -615,7 +610,7 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>  
>      case HV_X64_MSR_REFERENCE_TSC:
>          if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
> -            return 0;
> +            goto gp_fault;
>  
>          perfc_incr(mshv_wrmsr_tsc_msr);
>          d->arch.hvm_domain.viridian.reference_tsc.raw = val;
> @@ -655,14 +650,15 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>      }
>  
>      default:
> -        if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
> -            gprintk(XENLOG_WARNING, "write to unimplemented MSR %#x\n",
> -                    idx);
> -
> -        return 0;
> +        gdprintk(XENLOG_WARNING,
> +                 "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx);
> +        goto gp_fault;
>      }
>  
> -    return 1;
> +    return X86EMUL_OKAY;
> +
> + gp_fault:
> +    return X86EMUL_EXCEPTION;
>  }

Still these ugly goto-s to just a single return statement. But well,
Paul is happy with them ...

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -141,9 +141,11 @@ int init_vcpu_msr_policy(struct vcpu *v)
>  
>  int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  {
> -    const struct cpuid_policy *cp = v->domain->arch.cpuid;
> -    const struct msr_domain_policy *dp = v->domain->arch.msr;
> +    const struct domain *d = v->domain;
> +    const struct cpuid_policy *cp = d->arch.cpuid;
> +    const struct msr_domain_policy *dp = d->arch.msr;
>      const struct msr_vcpu_policy *vp = v->arch.msr;
> +    int ret = X86EMUL_OKAY;
>  
>      switch ( msr )
>      {
> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1:

The "HYPERVISOR" in here starts to make sense in the next patch,
but its combination with "VIRIDIAN" is still suspicious. I'll comment
on this further for the next patch.

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -540,4 +540,8 @@
>  #define MSR_PKGC9_IRTL			0x00000634
>  #define MSR_PKGC10_IRTL			0x00000635
>  
> +/* Hypervisor leaves in the 0x4xxxxxxx range. */
> +#define MSR_HYPERVISOR_START            0x40000000
> +#define NR_VIRIDIAN_MSRS                0x00000200

Is "leaves" really an appropriate term for MSRs?

Jan


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

Re: [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

[Jan Beulich]

>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 
> 1:
> +        if ( is_viridian_domain(d) )
> +        {
> +            ret = guest_rdmsr_viridian(v, msr, val);
> +            goto out;
> +        }
> +
> +        /* Fallthrough. */
>      default:
>          return X86EMUL_UNHANDLEABLE;
>      }
>  
> -    return X86EMUL_OKAY;
> + out:

I've noticed this only in the context of patch 4, but why is this label
and yet another unnecessary "goto" here? That "goto" could simply
be "break" afaics.

Jan


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

Re: [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

[Andrew Cooper]

On 13/03/18 15:20, Jan Beulich wrote:
>>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
>> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>          break;
>>  
>> +    case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 
>> 1:
>> +        if ( is_viridian_domain(d) )
>> +        {
>> +            ret = guest_rdmsr_viridian(v, msr, val);
>> +            goto out;
>> +        }
>> +
>> +        /* Fallthrough. */
>>      default:
>>          return X86EMUL_UNHANDLEABLE;
>>      }
>>  
>> -    return X86EMUL_OKAY;
>> + out:
> I've noticed this only in the context of patch 4, but why is this label
> and yet another unnecessary "goto" here? That "goto" could simply
> be "break" afaics.

Ah - that is for changes which I haven't posted yet.

When we get onto MSRs which might be in the load/save lists, or may be
stashed in the VMCB/VMCS rather than in real hardware, we need to call
back into arch specific code when an update is completed.

~Andrew

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

Re: [PATCH v2 1/5] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

[Jan Beulich]

>>> On 13.03.18 at 16:47, <andrew.cooper3@citrix.com> wrote:
> On 13/03/18 15:20, Jan Beulich wrote:
>>>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
>>> @@ -175,11 +177,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
> uint64_t *val)
>>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>>          break;
>>>  
>>> +    case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 
> 
>>> 1:
>>> +        if ( is_viridian_domain(d) )
>>> +        {
>>> +            ret = guest_rdmsr_viridian(v, msr, val);
>>> +            goto out;
>>> +        }
>>> +
>>> +        /* Fallthrough. */
>>>      default:
>>>          return X86EMUL_UNHANDLEABLE;
>>>      }
>>>  
>>> -    return X86EMUL_OKAY;
>>> + out:
>> I've noticed this only in the context of patch 4, but why is this label
>> and yet another unnecessary "goto" here? That "goto" could simply
>> be "break" afaics.
> 
> Ah - that is for changes which I haven't posted yet.
> 
> When we get onto MSRs which might be in the load/save lists, or may be
> stashed in the VMCB/VMCS rather than in real hardware, we need to call
> back into arch specific code when an update is completed.

But I don't think this requires a goto here, does it? If that future
code structure really can't get away without, so be it (then). But
please let's evaluate that at the time you have that code ready.

Jan


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

[PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

[Andrew Cooper]

Dispatch from the guest_{rd,wr}msr() functions, after falling through from the
!is_viridian_domain() case.

Rename {rd,wr}msr_hypervisor_regs() to guest_{rd,wr}msr_xen() for consistency,
and because the _regs suffix isn't very appropriate.

Update them to take a vcpu pointer rather than presuming that they act on
current, and switch to using X86EMUL_* return values.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>

v2:
 * Introduce some constants for the hypervisor range.  However, I do not think
   the current code is an improvement over bare numbers.
---
 xen/arch/x86/hvm/svm/svm.c      | 25 ++++---------------------
 xen/arch/x86/hvm/vmx/vmx.c      | 24 ++++--------------------
 xen/arch/x86/msr.c              |  8 ++++++++
 xen/arch/x86/pv/emul-priv-op.c  |  6 ------
 xen/arch/x86/traps.c            | 36 ++++++++++++++++++------------------
 xen/include/asm-x86/msr-index.h |  2 ++
 xen/include/asm-x86/processor.h |  4 ++--
 7 files changed, 38 insertions(+), 67 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index e8ac2d8..dadf92d 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1966,9 +1966,6 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         else if ( ret )
             break;
 
-        if ( rdmsr_hypervisor_regs(msr, msr_content) )
-            break;
-
         if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
 
@@ -2121,25 +2118,11 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         else if ( ret )
             break;
 
-        switch ( wrmsr_hypervisor_regs(msr, msr_content) )
-        {
-        case -ERESTART:
-            result = X86EMUL_RETRY;
-            break;
-        case 0:
-            /*
-             * Match up with the RDMSR side for now; ultimately this entire
-             * case block should go away.
-             */
-            if ( rdmsr_safe(msr, msr_content) == 0 )
-                break;
-            goto gpf;
-        case 1:
+        /* Match up with the RDMSR side; ultimately this should go away. */
+        if ( rdmsr_safe(msr, msr_content) == 0 )
             break;
-        default:
-            goto gpf;
-        }
-        break;
+
+        goto gpf;
     }
 
     if ( sync )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 5fc21a5..b9157af 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2909,9 +2909,6 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
             break;
         }
 
-        if ( rdmsr_hypervisor_regs(msr, msr_content) )
-            break;
-
         if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
 
@@ -3153,24 +3150,11 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
              is_last_branch_msr(msr) )
             break;
 
-        switch ( wrmsr_hypervisor_regs(msr, msr_content) )
-        {
-        case -ERESTART:
-            return X86EMUL_RETRY;
-        case 0:
-            /*
-             * Match up with the RDMSR side for now; ultimately this
-             * entire case block should go away.
-             */
-            if ( rdmsr_safe(msr, msr_content) == 0 )
-                break;
-            goto gp_fault;
-        case 1:
+        /* Match up with the RDMSR side; ultimately this should go away. */
+        if ( rdmsr_safe(msr, msr_content) == 0 )
             break;
-        default:
-            goto gp_fault;
-        }
-        break;
+
+        goto gp_fault;
     }
 
     return X86EMUL_OKAY;
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 2fc2021..62079bb 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         }
 
         /* Fallthrough. */
+    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
+        ret = guest_rdmsr_xen(v, msr, val);
+        goto out;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
@@ -276,6 +280,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         }
 
         /* Fallthrough. */
+    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
+        ret = guest_wrmsr_xen(v, msr, val);
+        goto out;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index ecb3b9c..9ddc5ca 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -974,9 +974,6 @@ static int read_msr(unsigned int reg, uint64_t *val,
         }
         /* fall through */
     default:
-        if ( rdmsr_hypervisor_regs(reg, val) )
-            return X86EMUL_OKAY;
-
         rc = vmce_rdmsr(reg, val);
         if ( rc < 0 )
             break;
@@ -1173,9 +1170,6 @@ static int write_msr(unsigned int reg, uint64_t val,
         }
         /* fall through */
     default:
-        if ( wrmsr_hypervisor_regs(reg, val) == 1 )
-            return X86EMUL_OKAY;
-
         rc = vmce_wrmsr(reg, val);
         if ( rc < 0 )
             break;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 19bc174..ea6b17c 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -777,31 +777,29 @@ static void do_trap(struct cpu_user_regs *regs)
           trapnr, trapstr(trapnr), regs->error_code);
 }
 
-/* Returns 0 if not handled, and non-0 for success. */
-int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
+int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val)
 {
-    struct domain *d = current->domain;
+    const struct domain *d = v->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
-    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
+    uint32_t base = (is_viridian_domain(d)
+                     ? MSR_XEN_ALT_START : MSR_HYPERVISOR_START);
 
     switch ( idx - base )
     {
     case 0: /* Write hypercall page MSR.  Read as zero. */
-    {
         *val = 0;
-        return 1;
-    }
+        return X86EMUL_OKAY;
     }
 
-    return 0;
+    return X86EMUL_EXCEPTION;
 }
 
-/* Returns 1 if handled, 0 if not and -Exx for error. */
-int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
+int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
 {
-    struct domain *d = current->domain;
+    struct domain *d = v->domain;
     /* Optionally shift out of the way of Viridian architectural MSRs. */
-    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
+    uint32_t base = (is_viridian_domain(d)
+                     ? MSR_XEN_ALT_START : MSR_HYPERVISOR_START);
 
     switch ( idx - base )
     {
@@ -818,7 +816,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
             gdprintk(XENLOG_WARNING,
                      "wrmsr hypercall page index %#x unsupported\n",
                      page_index);
-            return 0;
+            goto gp_fault;
         }
 
         page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
@@ -831,13 +829,13 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
             if ( p2m_is_paging(t) )
             {
                 p2m_mem_paging_populate(d, gmfn);
-                return -ERESTART;
+                return X86EMUL_RETRY;
             }
 
             gdprintk(XENLOG_WARNING,
                      "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
                      gmfn, page ? page_to_mfn(page) : -1UL, base);
-            return 0;
+            goto gp_fault;
         }
 
         hypercall_page = __map_domain_page(page);
@@ -845,11 +843,12 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
-        return 1;
+        return X86EMUL_OKAY;
     }
     }
 
-    return 0;
+ gp_fault:
+    return X86EMUL_EXCEPTION;
 }
 
 void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
@@ -887,7 +886,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
     case 2:
         res->a = 1;            /* Number of hypercall-transfer pages */
                                /* MSR base address */
-        res->b = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
+        res->b = (is_viridian_domain(d)
+                  ? MSR_XEN_ALT_START : MSR_HYPERVISOR_START);
         if ( is_pv_domain(d) ) /* Features */
             res->c |= XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD;
         break;
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index ee029a2..2b4014c 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -543,5 +543,7 @@
 /* Hypervisor leaves in the 0x4xxxxxxx range. */
 #define MSR_HYPERVISOR_START            0x40000000
 #define NR_VIRIDIAN_MSRS                0x00000200
+#define MSR_XEN_ALT_START               0x40000200
+#define NR_XEN_MSRS                     0x00000100
 
 #endif /* __ASM_MSR_INDEX_H */
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 01bc89f..28f2db6 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -519,8 +519,8 @@ unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn);
 
 void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
                              uint32_t subleaf, struct cpuid_leaf *res);
-int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val);
-int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val);
+int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val);
+int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val);
 
 void microcode_set_module(unsigned int);
 int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
-- 
2.1.4


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

Re: [PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

[Jan Beulich]

>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          }
>  
>          /* Fallthrough. */
> +    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:

To account for what I've said on patch 1, perhaps this better would
be

    case MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1 ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:

to produce consistent results regardless of the value of
NR_VIRIDIAN_MSRS (which I suppose is somewhere specified)?

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -777,31 +777,29 @@ static void do_trap(struct cpu_user_regs *regs)
>            trapnr, trapstr(trapnr), regs->error_code);
>  }
>  
> -/* Returns 0 if not handled, and non-0 for success. */
> -int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
> +int guest_rdmsr_xen(const struct vcpu *v, uint32_t idx, uint64_t *val)
>  {
> -    struct domain *d = current->domain;
> +    const struct domain *d = v->domain;
>      /* Optionally shift out of the way of Viridian architectural MSRs. */
> -    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
> +    uint32_t base = (is_viridian_domain(d)
> +                     ? MSR_XEN_ALT_START : MSR_HYPERVISOR_START);

Places like this is where it is really helpful to have the constants.

>      switch ( idx - base )
>      {
>      case 0: /* Write hypercall page MSR.  Read as zero. */
> -    {
>          *val = 0;
> -        return 1;
> -    }
> +        return X86EMUL_OKAY;
>      }
>  
> -    return 0;
> +    return X86EMUL_EXCEPTION;
>  }
>  
> -/* Returns 1 if handled, 0 if not and -Exx for error. */
> -int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
> +int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val)
>  {
> -    struct domain *d = current->domain;
> +    struct domain *d = v->domain;
>      /* Optionally shift out of the way of Viridian architectural MSRs. */
> -    uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
> +    uint32_t base = (is_viridian_domain(d)
> +                     ? MSR_XEN_ALT_START : MSR_HYPERVISOR_START);
>  
>      switch ( idx - base )
>      {
> @@ -818,7 +816,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>              gdprintk(XENLOG_WARNING,
>                       "wrmsr hypercall page index %#x unsupported\n",
>                       page_index);
> -            return 0;
> +            goto gp_fault;
>          }
>  
>          page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
> @@ -831,13 +829,13 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>              if ( p2m_is_paging(t) )
>              {
>                  p2m_mem_paging_populate(d, gmfn);
> -                return -ERESTART;
> +                return X86EMUL_RETRY;
>              }
>  
>              gdprintk(XENLOG_WARNING,
>                       "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
>                       gmfn, page ? page_to_mfn(page) : -1UL, base);
> -            return 0;
> +            goto gp_fault;
>          }
>  
>          hypercall_page = __map_domain_page(page);
> @@ -845,11 +843,12 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>          unmap_domain_page(hypercall_page);
>  
>          put_page_and_type(page);
> -        return 1;
> +        return X86EMUL_OKAY;
>      }
>      }
>  
> -    return 0;
> + gp_fault:
> +    return X86EMUL_EXCEPTION;
>  }

I'll make one more attempt here: Can I talk you into avoiding goto-s
in cases like this?

> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -543,5 +543,7 @@
>  /* Hypervisor leaves in the 0x4xxxxxxx range. */
>  #define MSR_HYPERVISOR_START            0x40000000
>  #define NR_VIRIDIAN_MSRS                0x00000200
> +#define MSR_XEN_ALT_START               0x40000200
> +#define NR_XEN_MSRS                     0x00000100

Where is this count coming from? I don't think it's part of the public
interface, but if there was such an upper bound I think it should be.

Jan

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

Re: [PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

[Andrew Cooper]

On 13/03/18 15:04, Jan Beulich wrote:
>>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>          }
>>  
>>          /* Fallthrough. */
>> +    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
> To account for what I've said on patch 1, perhaps this better would
> be
>
>     case MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1 ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>
> to produce consistent results regardless of the value of
> NR_VIRIDIAN_MSRS (which I suppose is somewhere specified)?

This demonstrates perfectly why using names here complicates things.  No
expression like this is going to be obvious to read.

The raw numbers are the normal way developers think about these ranges,
and its trivial to spot that the ranges are adjacent.

Please compare this suggestion to the guest_cpuid().  The CPUID code is
far far clearer to read, and the more I think about this, the more I'm
considering switching back to raw numbers.

>
>> --- a/xen/include/asm-x86/msr-index.h
>> +++ b/xen/include/asm-x86/msr-index.h
>> @@ -543,5 +543,7 @@
>>  /* Hypervisor leaves in the 0x4xxxxxxx range. */
>>  #define MSR_HYPERVISOR_START            0x40000000
>>  #define NR_VIRIDIAN_MSRS                0x00000200
>> +#define MSR_XEN_ALT_START               0x40000200
>> +#define NR_XEN_MSRS                     0x00000100
> Where is this count coming from? I don't think it's part of the public
> interface, but if there was such an upper bound I think it should be.

Its not part of the public ABI, and it should not be, because we don't
want to impose an arbitrary limit on how many blocks of 0x100 MSRs the
Xen range uses.  Its an artefact of attempting to use numbers.

~Andrew

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

Re: [PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

[Jan Beulich]

>>> On 07.09.18 at 16:30, <andrew.cooper3@citrix.com> wrote:
> On 13/03/18 15:04, Jan Beulich wrote:
>>>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:

Wow, resuming a discussion after a full half year.

>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>>          }
>>>  
>>>          /* Fallthrough. */
>>> +    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>> To account for what I've said on patch 1, perhaps this better would
>> be
>>
>>     case MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1 ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>>
>> to produce consistent results regardless of the value of
>> NR_VIRIDIAN_MSRS (which I suppose is somewhere specified)?
> 
> This demonstrates perfectly why using names here complicates things.  No
> expression like this is going to be obvious to read.
> 
> The raw numbers are the normal way developers think about these ranges,
> and its trivial to spot that the ranges are adjacent.

That's a personal thing - to me it's sometimes this way, sometimes
the other.

> Please compare this suggestion to the guest_cpuid().  The CPUID code is
> far far clearer to read, and the more I think about this, the more I'm
> considering switching back to raw numbers.

That depends very much on how easily the reader can associate back
the raw numbers. This is more likely to be the case for the rather
common CPUID leaves or groups of leaves, and perhaps less likely for
some of the less well known MSRs.

>>> --- a/xen/include/asm-x86/msr-index.h
>>> +++ b/xen/include/asm-x86/msr-index.h
>>> @@ -543,5 +543,7 @@
>>>  /* Hypervisor leaves in the 0x4xxxxxxx range. */
>>>  #define MSR_HYPERVISOR_START            0x40000000
>>>  #define NR_VIRIDIAN_MSRS                0x00000200
>>> +#define MSR_XEN_ALT_START               0x40000200
>>> +#define NR_XEN_MSRS                     0x00000100
>> Where is this count coming from? I don't think it's part of the public
>> interface, but if there was such an upper bound I think it should be.
> 
> Its not part of the public ABI, and it should not be, because we don't
> want to impose an arbitrary limit on how many blocks of 0x100 MSRs the
> Xen range uses.  Its an artefact of attempting to use numbers.

"attempting to use numbers"??? How would we get away without
using some form of number somewhere?

Jan



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

Re: [PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

[Andrew Cooper]

On 07/09/18 16:47, Jan Beulich wrote:
>>
>>>>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
> Wow, resuming a discussion after a full half year.

What can I say?  "Guess roughly when I was told about L1TF?"

As you can see, I did quite literally drop everything and start working
on speculative side-channel fixes.

>>>> --- a/xen/arch/x86/msr.c
>>>> +++ b/xen/arch/x86/msr.c
>>>> @@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>>>          }
>>>>  
>>>>          /* Fallthrough. */
>>>> +    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>>> To account for what I've said on patch 1, perhaps this better would
>>> be
>>>
>>>     case MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1 ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>>>
>>> to produce consistent results regardless of the value of
>>> NR_VIRIDIAN_MSRS (which I suppose is somewhere specified)?
>> This demonstrates perfectly why using names here complicates things.  No
>> expression like this is going to be obvious to read.
>>
>> The raw numbers are the normal way developers think about these ranges,
>> and its trivial to spot that the ranges are adjacent.
> That's a personal thing - to me it's sometimes this way, sometimes
> the other.
>
>> Please compare this suggestion to the guest_cpuid().  The CPUID code is
>> far far clearer to read, and the more I think about this, the more I'm
>> considering switching back to raw numbers.
> That depends very much on how easily the reader can associate back
> the raw numbers. This is more likely to be the case for the rather
> common CPUID leaves or groups of leaves, and perhaps less likely for
> some of the less well known MSRs.

Right, but we are only talking about the course categorisation of the
Viridian block of MSRs, and the Xen block of MSRs.  All of them are of
the form 0x40000xxx.

The end result of this bit of code will look almost identical to the
CPUID side, with the dispatch function names making the context clear.

>
>>>> --- a/xen/include/asm-x86/msr-index.h
>>>> +++ b/xen/include/asm-x86/msr-index.h
>>>> @@ -543,5 +543,7 @@
>>>>  /* Hypervisor leaves in the 0x4xxxxxxx range. */
>>>>  #define MSR_HYPERVISOR_START            0x40000000
>>>>  #define NR_VIRIDIAN_MSRS                0x00000200
>>>> +#define MSR_XEN_ALT_START               0x40000200
>>>> +#define NR_XEN_MSRS                     0x00000100
>>> Where is this count coming from? I don't think it's part of the public
>>> interface, but if there was such an upper bound I think it should be.
>> Its not part of the public ABI, and it should not be, because we don't
>> want to impose an arbitrary limit on how many blocks of 0x100 MSRs the
>> Xen range uses.  Its an artefact of attempting to use numbers.
> "attempting to use numbers"??? How would we get away without
> using some form of number somewhere?

Sorry.  I meant "of attempting to use names".

~Andrew

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

Re: [PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

[Jan Beulich]

>>> On 07.09.18 at 18:01, <andrew.cooper3@citrix.com> wrote:
> On 07/09/18 16:47, Jan Beulich wrote:
>>>>>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/msr.c
>>>>> +++ b/xen/arch/x86/msr.c
>>>>> @@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>>>>          }
>>>>>  
>>>>>          /* Fallthrough. */
>>>>> +    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>>>> To account for what I've said on patch 1, perhaps this better would
>>>> be
>>>>
>>>>     case MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1 ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>>>>
>>>> to produce consistent results regardless of the value of
>>>> NR_VIRIDIAN_MSRS (which I suppose is somewhere specified)?
>>> This demonstrates perfectly why using names here complicates things.  No
>>> expression like this is going to be obvious to read.
>>>
>>> The raw numbers are the normal way developers think about these ranges,
>>> and its trivial to spot that the ranges are adjacent.
>> That's a personal thing - to me it's sometimes this way, sometimes
>> the other.
>>
>>> Please compare this suggestion to the guest_cpuid().  The CPUID code is
>>> far far clearer to read, and the more I think about this, the more I'm
>>> considering switching back to raw numbers.
>> That depends very much on how easily the reader can associate back
>> the raw numbers. This is more likely to be the case for the rather
>> common CPUID leaves or groups of leaves, and perhaps less likely for
>> some of the less well known MSRs.
> 
> Right, but we are only talking about the course categorisation of the
> Viridian block of MSRs, and the Xen block of MSRs.  All of them are of
> the form 0x40000xxx.
> 
> The end result of this bit of code will look almost identical to the
> CPUID side, with the dispatch function names making the context clear.

Well, okay, let's see how it ends up looking.

Jan



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

[PATCH v2 3/5] x86: Fix APIC MSR constant names

[Andrew Cooper]

We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR which are
synonymous from a naming point of view, but refer to very different things.

Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants and
visually separate the register function from the generic APIC name.  For the
case ranges, introduce MSR_X2APIC_LAST, rather than relying on the knowledge
that there are 0x3ff MSRs architecturally reserved for x2APIC functionality.

For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for the MSR
itself, but drop the MSR prefix from the other constants to shorten the names.
In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious
from the context.

No functional change (the combined binary is identical).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>

v2:
 * New
---
 xen/arch/x86/apic.c              | 66 ++++++++++++++++++++--------------------
 xen/arch/x86/genapic/x2apic.c    |  4 +--
 xen/arch/x86/hvm/hvm.c           |  8 ++---
 xen/arch/x86/hvm/vlapic.c        | 19 ++++++------
 xen/arch/x86/hvm/vmx/vmx.c       | 20 ++++++------
 xen/include/asm-x86/hvm/vlapic.h |  6 ++--
 xen/include/asm-x86/msr-index.h  | 27 ++++++++--------
 7 files changed, 76 insertions(+), 74 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index ffa5a69..12e0c92 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -302,31 +302,31 @@ void disable_local_APIC(void)
 
     if (enabled_via_apicbase) {
         uint64_t msr_content;
-        rdmsrl(MSR_IA32_APICBASE, msr_content);
-        wrmsrl(MSR_IA32_APICBASE, msr_content &
-               ~(MSR_IA32_APICBASE_ENABLE|MSR_IA32_APICBASE_EXTD));
+        rdmsrl(MSR_APIC_BASE, msr_content);
+        wrmsrl(MSR_APIC_BASE, msr_content &
+               ~(APIC_BASE_ENABLE | APIC_BASE_EXTD));
     }
 
     if ( kexecing && (current_local_apic_mode() != apic_boot_mode) )
     {
         uint64_t msr_content;
-        rdmsrl(MSR_IA32_APICBASE, msr_content);
-        msr_content &= ~(MSR_IA32_APICBASE_ENABLE|MSR_IA32_APICBASE_EXTD);
-        wrmsrl(MSR_IA32_APICBASE, msr_content);
+        rdmsrl(MSR_APIC_BASE, msr_content);
+        msr_content &= ~(APIC_BASE_ENABLE | APIC_BASE_EXTD);
+        wrmsrl(MSR_APIC_BASE, msr_content);
 
         switch ( apic_boot_mode )
         {
         case APIC_MODE_DISABLED:
             break; /* Nothing to do - we did this above */
         case APIC_MODE_XAPIC:
-            msr_content |= MSR_IA32_APICBASE_ENABLE;
-            wrmsrl(MSR_IA32_APICBASE, msr_content);
+            msr_content |= APIC_BASE_ENABLE;
+            wrmsrl(MSR_APIC_BASE, msr_content);
             break;
         case APIC_MODE_X2APIC:
-            msr_content |= MSR_IA32_APICBASE_ENABLE;
-            wrmsrl(MSR_IA32_APICBASE, msr_content);
-            msr_content |= MSR_IA32_APICBASE_EXTD;
-            wrmsrl(MSR_IA32_APICBASE, msr_content);
+            msr_content |= APIC_BASE_ENABLE;
+            wrmsrl(MSR_APIC_BASE, msr_content);
+            msr_content |= APIC_BASE_EXTD;
+            wrmsrl(MSR_APIC_BASE, msr_content);
             break;
         default:
             printk("Default case when reverting #%d lapic to boot state\n",
@@ -478,12 +478,12 @@ static void __enable_x2apic(void)
 {
     uint64_t msr_content;
 
-    rdmsrl(MSR_IA32_APICBASE, msr_content);
-    if ( !(msr_content & MSR_IA32_APICBASE_EXTD) )
+    rdmsrl(MSR_APIC_BASE, msr_content);
+    if ( !(msr_content & APIC_BASE_EXTD) )
     {
-        msr_content |= MSR_IA32_APICBASE_ENABLE | MSR_IA32_APICBASE_EXTD;
+        msr_content |= APIC_BASE_ENABLE | APIC_BASE_EXTD;
         msr_content = (uint32_t)msr_content;
-        wrmsrl(MSR_IA32_APICBASE, msr_content);
+        wrmsrl(MSR_APIC_BASE, msr_content);
     }
 }
 
@@ -743,10 +743,10 @@ int lapic_resume(void)
      */
     if ( !x2apic_enabled )
     {
-        rdmsrl(MSR_IA32_APICBASE, msr_content);
-        msr_content &= ~MSR_IA32_APICBASE_BASE;
-        wrmsrl(MSR_IA32_APICBASE,
-            msr_content | MSR_IA32_APICBASE_ENABLE | mp_lapic_addr);
+        rdmsrl(MSR_APIC_BASE, msr_content);
+        msr_content &= ~APIC_BASE_BASE;
+        wrmsrl(MSR_APIC_BASE,
+               msr_content | APIC_BASE_ENABLE | mp_lapic_addr);
     }
     else
         resume_x2apic();
@@ -817,7 +817,8 @@ static int __init detect_init_APIC (void)
     if (enable_local_apic < 0)
         return -1;
 
-    if (rdmsr_safe(MSR_IA32_APICBASE, msr_content)) {
+    if ( rdmsr_safe(MSR_APIC_BASE, msr_content) )
+    {
         printk("No local APIC present\n");
         return -1;
     }
@@ -838,11 +839,12 @@ static int __init detect_init_APIC (void)
          * software for Intel P6 or later and AMD K7
          * (Model > 1) or later.
          */
-        if (!(msr_content & MSR_IA32_APICBASE_ENABLE)) {
+        if ( !(msr_content & APIC_BASE_ENABLE) )
+        {
             printk("Local APIC disabled by BIOS -- reenabling.\n");
-            msr_content &= ~MSR_IA32_APICBASE_BASE;
-            msr_content |= MSR_IA32_APICBASE_ENABLE | APIC_DEFAULT_PHYS_BASE;
-            wrmsrl(MSR_IA32_APICBASE, msr_content);
+            msr_content &= ~APIC_BASE_BASE;
+            msr_content |= APIC_BASE_ENABLE | APIC_DEFAULT_PHYS_BASE;
+            wrmsrl(MSR_APIC_BASE, msr_content);
             enabled_via_apicbase = true;
         }
     }
@@ -859,8 +861,8 @@ static int __init detect_init_APIC (void)
     mp_lapic_addr = APIC_DEFAULT_PHYS_BASE;
 
     /* The BIOS may have set up the APIC at some other address */
-    if (msr_content & MSR_IA32_APICBASE_ENABLE)
-        mp_lapic_addr = msr_content & MSR_IA32_APICBASE_BASE;
+    if ( msr_content & APIC_BASE_ENABLE )
+        mp_lapic_addr = msr_content & APIC_BASE_BASE;
 
     if (nmi_watchdog != NMI_NONE)
         nmi_watchdog = NMI_LOCAL_APIC;
@@ -1446,23 +1448,21 @@ void __init record_boot_APIC_mode(void)
                 apic_mode_to_str(apic_boot_mode));
 }
 
-/* Look at the bits in MSR_IA32_APICBASE and work out which
- * APIC mode we are in */
+/* Look at the bits in MSR_APIC_BASE and work out which APIC mode we are in */
 enum apic_mode current_local_apic_mode(void)
 {
     u64 msr_contents;
 
-    rdmsrl(MSR_IA32_APICBASE, msr_contents);
+    rdmsrl(MSR_APIC_BASE, msr_contents);
 
     /* Reading EXTD bit from the MSR is only valid if CPUID
      * says so, else reserved */
-    if ( boot_cpu_has(X86_FEATURE_X2APIC)
-         && (msr_contents & MSR_IA32_APICBASE_EXTD) )
+    if ( boot_cpu_has(X86_FEATURE_X2APIC) && (msr_contents & APIC_BASE_EXTD) )
         return APIC_MODE_X2APIC;
 
     /* EN bit should always be valid as long as we can read the MSR
      */
-    if ( msr_contents & MSR_IA32_APICBASE_ENABLE )
+    if ( msr_contents & APIC_BASE_ENABLE )
         return APIC_MODE_XAPIC;
 
     return APIC_MODE_DISABLED;
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
index 4779b0d..1cf67ea 100644
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -253,8 +253,8 @@ void __init check_x2apic_preenabled(void)
         return;
 
     /* Check whether x2apic mode was already enabled by the BIOS. */
-    rdmsr(MSR_IA32_APICBASE, lo, hi);
-    if ( lo & MSR_IA32_APICBASE_EXTD )
+    rdmsr(MSR_APIC_BASE, lo, hi);
+    if ( lo & APIC_BASE_EXTD )
     {
         printk("x2APIC mode is already enabled by BIOS.\n");
         x2apic_enabled = 1;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4618664..403ffd5 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3435,11 +3435,11 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         *msr_content = hvm_msr_tsc_aux(v);
         break;
 
-    case MSR_IA32_APICBASE:
+    case MSR_APIC_BASE:
         *msr_content = vcpu_vlapic(v)->hw.apic_base_msr;
         break;
 
-    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
+    case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
         if ( hvm_x2apic_msr_read(v, msr, msr_content) )
             goto gp_fault;
         break;
@@ -3589,7 +3589,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
             wrmsr_tsc_aux(msr_content);
         break;
 
-    case MSR_IA32_APICBASE:
+    case MSR_APIC_BASE:
         if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
             goto gp_fault;
         break;
@@ -3598,7 +3598,7 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
         vlapic_tdt_msr_set(vcpu_vlapic(v), msr_content);
         break;
 
-    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
+    case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
         if ( hvm_x2apic_msr_write(v, msr, msr_content) )
             goto gp_fault;
         break;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index e715729..fbb57f8 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -682,7 +682,7 @@ int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
 #undef REGBLOCK
         };
     const struct vlapic *vlapic = vcpu_vlapic(v);
-    uint32_t high = 0, reg = msr - MSR_IA32_APICBASE_MSR, offset = reg << 4;
+    uint32_t high = 0, reg = msr - MSR_X2APIC_BASE, offset = reg << 4;
 
     if ( !vlapic_x2apic_mode(vlapic) ||
          (reg >= sizeof(readable) * 8) || !test_bit(reg, readable) )
@@ -986,7 +986,7 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
 int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
-    uint32_t offset = (msr - MSR_IA32_APICBASE_MSR) << 4;
+    uint32_t offset = (msr - MSR_X2APIC_BASE) << 4;
 
     if ( !vlapic_x2apic_mode(vlapic) )
         return X86EMUL_UNHANDLEABLE;
@@ -1093,11 +1093,11 @@ bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
     if ( !has_vlapic(vlapic_domain(vlapic)) )
         return 0;
 
-    if ( (vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_ENABLE )
+    if ( (vlapic->hw.apic_base_msr ^ value) & APIC_BASE_ENABLE )
     {
-        if ( unlikely(value & MSR_IA32_APICBASE_EXTD) )
+        if ( unlikely(value & APIC_BASE_EXTD) )
             return 0;
-        if ( value & MSR_IA32_APICBASE_ENABLE )
+        if ( value & APIC_BASE_ENABLE )
         {
             vlapic_reset(vlapic);
             vlapic->hw.disabled &= ~VLAPIC_HW_DISABLED;
@@ -1109,7 +1109,7 @@ bool_t vlapic_msr_set(struct vlapic *vlapic, uint64_t value)
             pt_may_unmask_irq(vlapic_domain(vlapic), NULL);
         }
     }
-    else if ( ((vlapic->hw.apic_base_msr ^ value) & MSR_IA32_APICBASE_EXTD) &&
+    else if ( ((vlapic->hw.apic_base_msr ^ value) & APIC_BASE_EXTD) &&
               unlikely(!vlapic_xapic_mode(vlapic)) )
         return 0;
 
@@ -1394,10 +1394,9 @@ void vlapic_reset(struct vlapic *vlapic)
     if ( !has_vlapic(v->domain) )
         return;
 
-    vlapic->hw.apic_base_msr = (MSR_IA32_APICBASE_ENABLE |
-                                APIC_DEFAULT_PHYS_BASE);
+    vlapic->hw.apic_base_msr = APIC_BASE_ENABLE | APIC_DEFAULT_PHYS_BASE;
     if ( v->vcpu_id == 0 )
-        vlapic->hw.apic_base_msr |= MSR_IA32_APICBASE_BSP;
+        vlapic->hw.apic_base_msr |= APIC_BASE_BSP;
 
     vlapic_set_reg(vlapic, APIC_ID, (v->vcpu_id * 2) << 24);
     vlapic_do_init(vlapic);
@@ -1529,7 +1528,7 @@ static int lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
     if ( s->loaded.regs )
         lapic_load_fixup(s);
 
-    if ( !(s->hw.apic_base_msr & MSR_IA32_APICBASE_ENABLE) &&
+    if ( !(s->hw.apic_base_msr & APIC_BASE_ENABLE) &&
          unlikely(vlapic_x2apic_mode(s)) )
         return -EINVAL;
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b9157af..d1cbc28 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2999,19 +2999,19 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
                 SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
             if ( cpu_has_vmx_apic_reg_virt )
             {
-                for ( msr = MSR_IA32_APICBASE_MSR;
-                      msr <= MSR_IA32_APICBASE_MSR + 0xff; msr++ )
+                for ( msr = MSR_X2APIC_BASE;
+                      msr <= MSR_X2APIC_BASE + 0xff; msr++ )
                     vmx_clear_msr_intercept(v, msr, VMX_MSR_R);
 
-                vmx_set_msr_intercept(v, MSR_IA32_APICPPR_MSR, VMX_MSR_R);
-                vmx_set_msr_intercept(v, MSR_IA32_APICTMICT_MSR, VMX_MSR_R);
-                vmx_set_msr_intercept(v, MSR_IA32_APICTMCCT_MSR, VMX_MSR_R);
+                vmx_set_msr_intercept(v, MSR_X2APIC_PPR, VMX_MSR_R);
+                vmx_set_msr_intercept(v, MSR_X2APIC_TMICT, VMX_MSR_R);
+                vmx_set_msr_intercept(v, MSR_X2APIC_TMCCT, VMX_MSR_R);
             }
             if ( cpu_has_vmx_virtual_intr_delivery )
             {
-                vmx_clear_msr_intercept(v, MSR_IA32_APICTPR_MSR, VMX_MSR_W);
-                vmx_clear_msr_intercept(v, MSR_IA32_APICEOI_MSR, VMX_MSR_W);
-                vmx_clear_msr_intercept(v, MSR_IA32_APICSELF_MSR, VMX_MSR_W);
+                vmx_clear_msr_intercept(v, MSR_X2APIC_TPR, VMX_MSR_W);
+                vmx_clear_msr_intercept(v, MSR_X2APIC_EOI, VMX_MSR_W);
+                vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
             }
         }
         else
@@ -3020,8 +3020,8 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
     }
     if ( !(v->arch.hvm_vmx.secondary_exec_control &
            SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE) )
-        for ( msr = MSR_IA32_APICBASE_MSR;
-              msr <= MSR_IA32_APICBASE_MSR + 0xff; msr++ )
+        for ( msr = MSR_X2APIC_BASE;
+              msr <= MSR_X2APIC_BASE + 0xff; msr++ )
             vmx_set_msr_intercept(v, msr, VMX_MSR_RW);
 
     vmx_update_secondary_exec_control(v);
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index 212c36b..bea7bc6 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -50,13 +50,13 @@
 #define vlapic_enabled(vlapic)     (!vlapic_disabled(vlapic))
 
 #define vlapic_base_address(vlapic)                             \
-    ((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_BASE)
+    ((vlapic)->hw.apic_base_msr & APIC_BASE_BASE)
 /* Only check EXTD bit as EXTD can't be set if it is disabled by hardware */
 #define vlapic_x2apic_mode(vlapic)                              \
-    ((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD)
+    ((vlapic)->hw.apic_base_msr & APIC_BASE_EXTD)
 #define vlapic_xapic_mode(vlapic)                               \
     (!vlapic_hw_disabled(vlapic) && \
-     !((vlapic)->hw.apic_base_msr & MSR_IA32_APICBASE_EXTD))
+     !((vlapic)->hw.apic_base_msr & APIC_BASE_EXTD))
 
 /*
  * Generic APIC bitmap vector update & search routines.
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 2b4014c..07f2209 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -312,18 +312,21 @@
 
 #define MSR_IA32_TSC_ADJUST		0x0000003b
 
-#define MSR_IA32_APICBASE		0x0000001b
-#define MSR_IA32_APICBASE_BSP		(1<<8)
-#define MSR_IA32_APICBASE_EXTD		(1<<10)
-#define MSR_IA32_APICBASE_ENABLE	(1<<11)
-#define MSR_IA32_APICBASE_BASE		0x000ffffffffff000ul
-#define MSR_IA32_APICBASE_MSR           0x800
-#define MSR_IA32_APICTPR_MSR            0x808
-#define MSR_IA32_APICPPR_MSR            0x80a
-#define MSR_IA32_APICEOI_MSR            0x80b
-#define MSR_IA32_APICTMICT_MSR          0x838
-#define MSR_IA32_APICTMCCT_MSR          0x839
-#define MSR_IA32_APICSELF_MSR           0x83f
+#define MSR_APIC_BASE                   0x0000001b
+#define APIC_BASE_BSP                   (1<<8)
+#define APIC_BASE_EXTD                  (1<<10)
+#define APIC_BASE_ENABLE                (1<<11)
+#define APIC_BASE_BASE                  0x000ffffffffff000ul
+
+#define MSR_X2APIC_BASE                 0x800
+#define MSR_X2APIC_LAST                 0xbff
+
+#define MSR_X2APIC_TPR                  0x808
+#define MSR_X2APIC_PPR                  0x80a
+#define MSR_X2APIC_EOI                  0x80b
+#define MSR_X2APIC_TMICT                0x838
+#define MSR_X2APIC_TMCCT                0x839
+#define MSR_X2APIC_SELF                 0x83f
 
 #define MSR_IA32_UCODE_WRITE		0x00000079
 #define MSR_IA32_UCODE_REV		0x0000008b
-- 
2.1.4


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

Re: [PATCH v2 3/5] x86: Fix APIC MSR constant names

[Konrad Rzeszutek Wilk]

On Wed, Mar 07, 2018 at 06:58:34PM +0000, Andrew Cooper wrote:
> We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR which are
> synonymous from a naming point of view, but refer to very different things.
> 
> Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants and
> visually separate the register function from the generic APIC name.  For the
> case ranges, introduce MSR_X2APIC_LAST, rather than relying on the knowledge
> that there are 0x3ff MSRs architecturally reserved for x2APIC functionality.
> 
> For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for the MSR
> itself, but drop the MSR prefix from the other constants to shorten the names.
> In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious
> from the context.
> 
> No functional change (the combined binary is identical).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!

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

Re: [PATCH v2 3/5] x86: Fix APIC MSR constant names

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, March 8, 2018 2:59 AM
> 
> We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR
> which are
> synonymous from a naming point of view, but refer to very different things.
> 
> Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants
> and
> visually separate the register function from the generic APIC name.  For the
> case ranges, introduce MSR_X2APIC_LAST, rather than relying on the
> knowledge
> that there are 0x3ff MSRs architecturally reserved for x2APIC functionality.
> 
> For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for
> the MSR
> itself, but drop the MSR prefix from the other constants to shorten the
> names.
> In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious
> from the context.
> 
> No functional change (the combined binary is identical).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> 

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/5] x86: Fix APIC MSR constant names

[Jan Beulich]

>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -312,18 +312,21 @@
>  
>  #define MSR_IA32_TSC_ADJUST		0x0000003b
>  
> -#define MSR_IA32_APICBASE		0x0000001b
> -#define MSR_IA32_APICBASE_BSP		(1<<8)
> -#define MSR_IA32_APICBASE_EXTD		(1<<10)
> -#define MSR_IA32_APICBASE_ENABLE	(1<<11)
> -#define MSR_IA32_APICBASE_BASE		0x000ffffffffff000ul
> -#define MSR_IA32_APICBASE_MSR           0x800
> -#define MSR_IA32_APICTPR_MSR            0x808
> -#define MSR_IA32_APICPPR_MSR            0x80a
> -#define MSR_IA32_APICEOI_MSR            0x80b
> -#define MSR_IA32_APICTMICT_MSR          0x838
> -#define MSR_IA32_APICTMCCT_MSR          0x839
> -#define MSR_IA32_APICSELF_MSR           0x83f
> +#define MSR_APIC_BASE                   0x0000001b
> +#define APIC_BASE_BSP                   (1<<8)
> +#define APIC_BASE_EXTD                  (1<<10)
> +#define APIC_BASE_ENABLE                (1<<11)
> +#define APIC_BASE_BASE                  0x000ffffffffff000ul

This sounds a little clumsy; how about APIC_BASE_ADDR_MASK?

> +#define MSR_X2APIC_BASE                 0x800
> +#define MSR_X2APIC_LAST                 0xbff

With "LAST", perhaps also MSR_X2APIC_FIRST (even further
separating it from MSR_APIC_BASE)?

> +#define MSR_X2APIC_TPR                  0x808
> +#define MSR_X2APIC_PPR                  0x80a
> +#define MSR_X2APIC_EOI                  0x80b
> +#define MSR_X2APIC_TMICT                0x838
> +#define MSR_X2APIC_TMCCT                0x839
> +#define MSR_X2APIC_SELF                 0x83f

All surrounding MSR indexes have leading zeros spelled out; would
you mind doing so for the x2APIC ones as well?

I won't insist on any of these though, so with or without any or all
of them
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v2 3/5] x86: Fix APIC MSR constant names

[Roger Pau Monné]

On Wed, Mar 07, 2018 at 06:58:34PM +0000, Andrew Cooper wrote:
> We currently have MSR_IA32_APICBASE and MSR_IA32_APICBASE_MSR which are
> synonymous from a naming point of view, but refer to very different things.
> 
> Rename the x2APIC MSRs to MSR_X2APIC_*, which are shorter constants and
> visually separate the register function from the generic APIC name.  For the
> case ranges, introduce MSR_X2APIC_LAST, rather than relying on the knowledge
> that there are 0x3ff MSRs architecturally reserved for x2APIC functionality.
> 
> For functionality relating to the APIC_BASE MSR, use MSR_APIC_BASE for the MSR
> itself, but drop the MSR prefix from the other constants to shorten the names.
> In all cases, the fact that we are dealing with the APIC_BASE MSR is obvious
> from the context.
> 
> No functional change (the combined binary is identical).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
> index 2b4014c..07f2209 100644
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -312,18 +312,21 @@
>  
>  #define MSR_IA32_TSC_ADJUST		0x0000003b
>  
> -#define MSR_IA32_APICBASE		0x0000001b
> -#define MSR_IA32_APICBASE_BSP		(1<<8)
> -#define MSR_IA32_APICBASE_EXTD		(1<<10)
> -#define MSR_IA32_APICBASE_ENABLE	(1<<11)
> -#define MSR_IA32_APICBASE_BASE		0x000ffffffffff000ul
> -#define MSR_IA32_APICBASE_MSR           0x800
> -#define MSR_IA32_APICTPR_MSR            0x808
> -#define MSR_IA32_APICPPR_MSR            0x80a
> -#define MSR_IA32_APICEOI_MSR            0x80b
> -#define MSR_IA32_APICTMICT_MSR          0x838
> -#define MSR_IA32_APICTMCCT_MSR          0x839
> -#define MSR_IA32_APICSELF_MSR           0x83f
> +#define MSR_APIC_BASE                   0x0000001b
> +#define APIC_BASE_BSP                   (1<<8)
> +#define APIC_BASE_EXTD                  (1<<10)
> +#define APIC_BASE_ENABLE                (1<<11)
> +#define APIC_BASE_BASE                  0x000ffffffffff000ul

Maybe those could be indented like:

#define MSR_FOO
#define  FOO_BAR

Thanks, Roger.

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

[PATCH v2 4/5] x86/hvm: Handle x2apic MSRs via the new guest_{rd, wr}msr() infrastructure

[Andrew Cooper]

Dispatch from the guest_{rd,wr}msr() functions.  The read side should be safe
outside of current context, but the write side is definitely not.  As the
toolstack has no legitimate reason to access the APIC registers via this
interface (not least because whether they are accessible at all depends on
guest settings), unilaterally reject access attempts outside of current
context.

Rename to guest_{rd,wr}msr_x2apic() for consistency, and alter the functions
to use X86EMUL_EXCEPTION rather than X86EMUL_UNHANDLEABLE.  The previous
callers turned UNHANDLEABLE into EXCEPTION, but using UNHANDLEABLE will now
interfere with the fallback to legacy MSR handling.

While altering guest_rdmsr_x2apic() make a couple of minor improvements.
Reformat the initialiser for readable[] so it indents in a more natural way,
and alter high to be a 64bit integer to avoid shifting 0 by 32 in the common
path.

Observant people might notice that we now don't let PV guests read the x2apic
MSRs.  They should never have been able to in the first place.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>

v2:
 * Rebase over APIC MSR naming changes
 * Drop gp_fault label in guest_wrmsr_x2apic()
---
 xen/arch/x86/hvm/hvm.c        | 10 -------
 xen/arch/x86/hvm/vlapic.c     | 65 +++++++++++++++++++++++++------------------
 xen/arch/x86/msr.c            | 15 ++++++++++
 xen/include/asm-x86/hvm/hvm.h |  4 +--
 4 files changed, 55 insertions(+), 39 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 403ffd5..a8d7e19 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3439,11 +3439,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         *msr_content = vcpu_vlapic(v)->hw.apic_base_msr;
         break;
 
-    case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
-        if ( hvm_x2apic_msr_read(v, msr, msr_content) )
-            goto gp_fault;
-        break;
-
     case MSR_IA32_TSC_DEADLINE:
         *msr_content = vlapic_tdt_msr_get(vcpu_vlapic(v));
         break;
@@ -3598,11 +3593,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
         vlapic_tdt_msr_set(vcpu_vlapic(v), msr_content);
         break;
 
-    case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
-        if ( hvm_x2apic_msr_write(v, msr, msr_content) )
-            goto gp_fault;
-        break;
-
     case MSR_IA32_CR_PAT:
         if ( !hvm_set_guest_pat(v, msr_content) )
            goto gp_fault;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index fbb57f8..8817476 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -666,33 +666,39 @@ static int vlapic_read(
     return X86EMUL_OKAY;
 }
 
-int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
+int guest_rdmsr_x2apic(const struct vcpu *v, uint32_t msr, uint64_t *val)
 {
-    static const unsigned long readable[] =
-        {
+    static const unsigned long readable[] = {
 #define REG(x) (1UL << (APIC_ ## x >> 4))
-            REG(ID)    | REG(LVR)  | REG(TASKPRI) | REG(PROCPRI) |
-            REG(LDR)   | REG(SPIV) | REG(ESR)     | REG(ICR)     |
-            REG(CMCI)  | REG(LVTT) | REG(LVTTHMR) | REG(LVTPC)   |
-            REG(LVT0)  | REG(LVT1) | REG(LVTERR)  | REG(TMICT)   |
-            REG(TMCCT) | REG(TDCR) |
+        REG(ID)    | REG(LVR)  | REG(TASKPRI) | REG(PROCPRI) |
+        REG(LDR)   | REG(SPIV) | REG(ESR)     | REG(ICR)     |
+        REG(CMCI)  | REG(LVTT) | REG(LVTTHMR) | REG(LVTPC)   |
+        REG(LVT0)  | REG(LVT1) | REG(LVTERR)  | REG(TMICT)   |
+        REG(TMCCT) | REG(TDCR) |
 #undef REG
 #define REGBLOCK(x) (((1UL << (NR_VECTORS / 32)) - 1) << (APIC_ ## x >> 4))
-            REGBLOCK(ISR) | REGBLOCK(TMR) | REGBLOCK(IRR)
+        REGBLOCK(ISR) | REGBLOCK(TMR) | REGBLOCK(IRR)
 #undef REGBLOCK
-        };
+    };
     const struct vlapic *vlapic = vcpu_vlapic(v);
-    uint32_t high = 0, reg = msr - MSR_X2APIC_BASE, offset = reg << 4;
+    uint64_t high = 0;
+    uint32_t reg = msr - MSR_X2APIC_BASE, offset = reg << 4;
+
+    /*
+     * The read side looks as if it might be safe to use outside of current
+     * context, but the write side is most certainly not.  As we don't need
+     * any non-current access, enforce symmetry with the write side.
+     */
+    ASSERT(v == current);
 
     if ( !vlapic_x2apic_mode(vlapic) ||
          (reg >= sizeof(readable) * 8) || !test_bit(reg, readable) )
-        return X86EMUL_UNHANDLEABLE;
+        return X86EMUL_EXCEPTION;
 
     if ( offset == APIC_ICR )
-        high = vlapic_read_aligned(vlapic, APIC_ICR2);
+        high = (uint64_t)vlapic_read_aligned(vlapic, APIC_ICR2) << 32;
 
-    *msr_content = ((uint64_t)high << 32) |
-                   vlapic_read_aligned(vlapic, offset);
+    *val = high | vlapic_read_aligned(vlapic, offset);
 
     return X86EMUL_OKAY;
 }
@@ -983,49 +989,52 @@ int vlapic_apicv_write(struct vcpu *v, unsigned int offset)
     return X86EMUL_OKAY;
 }
 
-int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content)
+int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
 {
     struct vlapic *vlapic = vcpu_vlapic(v);
     uint32_t offset = (msr - MSR_X2APIC_BASE) << 4;
 
+    /* The timer handling at least is unsafe outside of current context. */
+    ASSERT(v == current);
+
     if ( !vlapic_x2apic_mode(vlapic) )
-        return X86EMUL_UNHANDLEABLE;
+        return X86EMUL_EXCEPTION;
 
     switch ( offset )
     {
     case APIC_TASKPRI:
         if ( msr_content & ~APIC_TPRI_MASK )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         break;
 
     case APIC_SPIV:
         if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
                              (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
                               ? APIC_SPIV_DIRECTED_EOI : 0)) )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         break;
 
     case APIC_LVTT:
         if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         break;
 
     case APIC_LVTTHMR:
     case APIC_LVTPC:
     case APIC_CMCI:
         if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         break;
 
     case APIC_LVT0:
     case APIC_LVT1:
         if ( msr_content & ~LINT_MASK )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         break;
 
     case APIC_LVTERR:
         if ( msr_content & ~LVT_MASK )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         break;
 
     case APIC_TMICT:
@@ -1033,20 +1042,20 @@ int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content)
 
     case APIC_TDCR:
         if ( msr_content & ~APIC_TDR_DIV_1 )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         break;
 
     case APIC_ICR:
         if ( (uint32_t)msr_content & ~(APIC_VECTOR_MASK | APIC_MODE_MASK |
                                        APIC_DEST_MASK | APIC_INT_ASSERT |
                                        APIC_INT_LEVELTRIG | APIC_SHORT_MASK) )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
         break;
 
     case APIC_SELF_IPI:
         if ( msr_content & ~APIC_VECTOR_MASK )
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
         offset = APIC_ICR;
         msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
         break;
@@ -1054,8 +1063,10 @@ int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content)
     case APIC_EOI:
     case APIC_ESR:
         if ( msr_content )
+        {
     default:
-            return X86EMUL_UNHANDLEABLE;
+            return X86EMUL_EXCEPTION;
+        }
     }
 
     vlapic_reg_write(v, offset, msr_content);
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 62079bb..fa2552a 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -141,6 +141,7 @@ int init_vcpu_msr_policy(struct vcpu *v)
 
 int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
 {
+    const struct vcpu *curr = current;
     const struct domain *d = v->domain;
     const struct cpuid_policy *cp = d->arch.cpuid;
     const struct msr_domain_policy *dp = d->arch.msr;
@@ -177,6 +178,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
                _MSR_MISC_FEATURES_CPUID_FAULTING;
         break;
 
+    case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
+        if ( !is_hvm_domain(d) || v != curr )
+            goto gp_fault;
+
+        ret = guest_rdmsr_x2apic(v, msr, val);
+        goto out;
+
     case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1:
         if ( is_viridian_domain(d) )
         {
@@ -272,6 +280,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
     }
 
+    case MSR_X2APIC_BASE ... MSR_X2APIC_LAST:
+        if ( !is_hvm_domain(d) || v != curr )
+            goto gp_fault;
+
+        ret = guest_wrmsr_x2apic(v, msr, val);
+        goto out;
+
     case MSR_HYPERVISOR_START ... MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1:
         if ( is_viridian_domain(d) )
         {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 9aa6c72..988b896 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -528,8 +528,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
         ? (u32)__d->arch.incarnation : (u32)(v)->arch.hvm_vcpu.msr_tsc_aux; \
 })
 
-int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content);
-int hvm_x2apic_msr_write(struct vcpu *v, unsigned int msr, uint64_t msr_content);
+int guest_rdmsr_x2apic(const struct vcpu *v, uint32_t msr, uint64_t *val);
+int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val);
 
 /*
  * Nested HVM
-- 
2.1.4


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

Re: [PATCH v2 4/5] x86/hvm: Handle x2apic MSRs via the new guest_{rd, wr}msr() infrastructure

[Konrad Rzeszutek Wilk]

On Wed, Mar 07, 2018 at 06:58:35PM +0000, Andrew Cooper wrote:
> Dispatch from the guest_{rd,wr}msr() functions.  The read side should be safe
> outside of current context, but the write side is definitely not.  As the
> toolstack has no legitimate reason to access the APIC registers via this
> interface (not least because whether they are accessible at all depends on
> guest settings), unilaterally reject access attempts outside of current
> context.
> 
> Rename to guest_{rd,wr}msr_x2apic() for consistency, and alter the functions
> to use X86EMUL_EXCEPTION rather than X86EMUL_UNHANDLEABLE.  The previous
> callers turned UNHANDLEABLE into EXCEPTION, but using UNHANDLEABLE will now
> interfere with the fallback to legacy MSR handling.
> 
> While altering guest_rdmsr_x2apic() make a couple of minor improvements.
> Reformat the initialiser for readable[] so it indents in a more natural way,
> and alter high to be a 64bit integer to avoid shifting 0 by 32 in the common
> path.
> 
> Observant people might notice that we now don't let PV guests read the x2apic
> MSRs.  They should never have been able to in the first place.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


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

Re: [PATCH v2 4/5] x86/hvm: Handle x2apic MSRs via the new guest_{rd, wr}msr() infrastructure

[Jan Beulich]

>>> On 07.03.18 at 21:59, <konrad.wilk@oracle.com> wrote:
> On Wed, Mar 07, 2018 at 06:58:35PM +0000, Andrew Cooper wrote:
>> Dispatch from the guest_{rd,wr}msr() functions.  The read side should be 
> safe
>> outside of current context, but the write side is definitely not.  As the
>> toolstack has no legitimate reason to access the APIC registers via this
>> interface (not least because whether they are accessible at all depends on
>> guest settings), unilaterally reject access attempts outside of current
>> context.
>> 
>> Rename to guest_{rd,wr}msr_x2apic() for consistency, and alter the functions
>> to use X86EMUL_EXCEPTION rather than X86EMUL_UNHANDLEABLE.  The previous
>> callers turned UNHANDLEABLE into EXCEPTION, but using UNHANDLEABLE will now
>> interfere with the fallback to legacy MSR handling.
>> 
>> While altering guest_rdmsr_x2apic() make a couple of minor improvements.
>> Reformat the initialiser for readable[] so it indents in a more natural way,
>> and alter high to be a 64bit integer to avoid shifting 0 by 32 in the common
>> path.
>> 
>> Observant people might notice that we now don't let PV guests read the 
> x2apic
>> MSRs.  They should never have been able to in the first place.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Subject to the possible replacement of "goto out" in
xen/arch/x86/msr.c (as per the comment on patch 1)
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v2 4/5] x86/hvm: Handle x2apic MSRs via the new guest_{rd, wr}msr() infrastructure

[Roger Pau Monné]

On Wed, Mar 07, 2018 at 06:58:35PM +0000, Andrew Cooper wrote:
> Dispatch from the guest_{rd,wr}msr() functions.  The read side should be safe
> outside of current context, but the write side is definitely not.  As the
> toolstack has no legitimate reason to access the APIC registers via this
> interface (not least because whether they are accessible at all depends on
> guest settings), unilaterally reject access attempts outside of current
> context.
> 
> Rename to guest_{rd,wr}msr_x2apic() for consistency, and alter the functions
> to use X86EMUL_EXCEPTION rather than X86EMUL_UNHANDLEABLE.  The previous
> callers turned UNHANDLEABLE into EXCEPTION, but using UNHANDLEABLE will now
> interfere with the fallback to legacy MSR handling.
> 
> While altering guest_rdmsr_x2apic() make a couple of minor improvements.
> Reformat the initialiser for readable[] so it indents in a more natural way,
> and alter high to be a 64bit integer to avoid shifting 0 by 32 in the common
> path.
> 
> Observant people might notice that we now don't let PV guests read the x2apic
> MSRs.  They should never have been able to in the first place.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Roger.

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

[PATCH v2 5/5] x86/msr: Blacklist various MSRs which guests definitely shouldn't be using

[Andrew Cooper]

The main purpose is to blacklist the Intel Resource Director Technology MSRs.
We do not yet virtualise support for guests, but Linux has been observed to
probe for these MSRs without checking CPUID first.

The architecturally inaccessable ranges don't need to fall back into the
legacy ranges, because they are not going to eventually evaluate as
accessible.

The Silicon Debug interface will probably never be virtualised for guests, but
doesn't want to leak through from real hardware.  SGX isn't yet virtualised,
but likely will be in the future.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>

v2:
 * Use names.  Blacklist another SGX MSR.
---
 xen/arch/x86/msr.c              | 42 +++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/msr-index.h | 14 ++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index fa2552a..c3314db 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -197,7 +197,28 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         ret = guest_rdmsr_xen(v, msr, val);
         goto out;
 
+        /* Specific blacklisted MSRs while the legacy handlers still exist. */
+    case MSR_SGX_PUBKEY_HASH(0) ... MSR_SGX_PUBKEY_HASH(3):
+    case MSR_SGX_SVN_STATUS:
+    case MSR_DEBUG_INTERFACE:
+    case MSR_L3_QOS_CFG:
+    case MSR_L2_QOS_CFG:
+    case MSR_QM_EVTSEL:
+    case MSR_QM_CTR:
+    case MSR_PQR_ASSOC:
+    case MSR_CAT_MASK_START ... MSR_CAT_MASK_LAST:
+        goto gp_fault;
+
     default:
+        /*
+         * Blacklist the architecturally inaccessable MSRs. No point wandering
+         * the legacy handlers.
+         */
+        if ( msr > 0x1fff &&
+             (msr < 0xc0000000 || msr > 0xc0001fff) &&
+             (msr < 0xc0010000 || msr > 0xc0011fff) )
+            goto gp_fault;
+
         return X86EMUL_UNHANDLEABLE;
     }
 
@@ -299,7 +320,28 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         ret = guest_wrmsr_xen(v, msr, val);
         goto out;
 
+        /* Specific blacklisted MSRs while the legacy handlers still exist. */
+    case MSR_SGX_PUBKEY_HASH(0) ... MSR_SGX_PUBKEY_HASH(3):
+    case MSR_SGX_SVN_STATUS:
+    case MSR_DEBUG_INTERFACE:
+    case MSR_L3_QOS_CFG:
+    case MSR_L2_QOS_CFG:
+    case MSR_QM_EVTSEL:
+    case MSR_QM_CTR:
+    case MSR_PQR_ASSOC:
+    case MSR_CAT_MASK_START ... MSR_CAT_MASK_LAST:
+        goto gp_fault;
+
     default:
+        /*
+         * Blacklist the architecturally inaccessable MSRs. No point wandering
+         * the legacy handlers.
+         */
+        if ( msr > 0x1fff &&
+             (msr < 0xc0000000 || msr > 0xc0001fff) &&
+             (msr < 0xc0010000 || msr > 0xc0011fff) )
+            goto gp_fault;
+
         return X86EMUL_UNHANDLEABLE;
     }
 
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 07f2209..b3986ad 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -39,6 +39,8 @@
 #define MSR_PRED_CMD			0x00000049
 #define PRED_CMD_IBPB			(_AC(1, ULL) << 0)
 
+#define MSR_SGX_PUBKEY_HASH(x)		(0x0000008c + (x)) /* 0 ... 3 */
+
 #define MSR_ARCH_CAPABILITIES		0x0000010a
 
 /* Intel MSRs. Some also available on other CPUs */
@@ -69,6 +71,18 @@
 /* Lower 6 bits define the format of the address in the LBR stack */
 #define MSR_IA32_PERF_CAP_LBR_FORMAT	0x3f
 
+#define MSR_SGX_SVN_STATUS		0x00000500
+
+#define MSR_DEBUG_INTERFACE		0x00000c80
+
+#define MSR_L3_QOS_CFG			0x00000c81
+#define MSR_L2_QOS_CFG			0x00000c82
+#define MSR_QM_EVTSEL			0x00000c8d
+#define MSR_QM_CTR			0x00000c8e
+#define MSR_PQR_ASSOC			0x00000c8f
+#define MSR_CAT_MASK_START		0x00000c90
+#define MSR_CAT_MASK_LAST		0x00000d8f
+
 #define MSR_IA32_BNDCFGS		0x00000d90
 #define IA32_BNDCFGS_ENABLE		0x00000001
 #define IA32_BNDCFGS_PRESERVE		0x00000002
-- 
2.1.4


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

Re: [PATCH v2 5/5] x86/msr: Blacklist various MSRs which guests definitely shouldn't be using

[Konrad Rzeszutek Wilk]

On Wed, Mar 07, 2018 at 06:58:36PM +0000, Andrew Cooper wrote:
> The main purpose is to blacklist the Intel Resource Director Technology MSRs.
> We do not yet virtualise support for guests, but Linux has been observed to
> probe for these MSRs without checking CPUID first.

Bad bad guest..
> 
> The architecturally inaccessable ranges don't need to fall back into the
> legacy ranges, because they are not going to eventually evaluate as
> accessible.
> 
> The Silicon Debug interface will probably never be virtualised for guests, but
> doesn't want to leak through from real hardware.  SGX isn't yet virtualised,
> but likely will be in the future.


> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!

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

Re: [PATCH v2 5/5] x86/msr: Blacklist various MSRs which guests definitely shouldn't be using

[Jan Beulich]

>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -197,7 +197,28 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          ret = guest_rdmsr_xen(v, msr, val);
>          goto out;
>  
> +        /* Specific blacklisted MSRs while the legacy handlers still exist. */
> +    case MSR_SGX_PUBKEY_HASH(0) ... MSR_SGX_PUBKEY_HASH(3):

Did you intentionally misalign the comment wrt the case labels?

> +    case MSR_SGX_SVN_STATUS:
> +    case MSR_DEBUG_INTERFACE:
> +    case MSR_L3_QOS_CFG:
> +    case MSR_L2_QOS_CFG:
> +    case MSR_QM_EVTSEL:
> +    case MSR_QM_CTR:
> +    case MSR_PQR_ASSOC:
> +    case MSR_CAT_MASK_START ... MSR_CAT_MASK_LAST:
> +        goto gp_fault;
> +
>      default:
> +        /*
> +         * Blacklist the architecturally inaccessable MSRs. No point wandering
> +         * the legacy handlers.
> +         */
> +        if ( msr > 0x1fff &&
> +             (msr < 0xc0000000 || msr > 0xc0001fff) &&
> +             (msr < 0xc0010000 || msr > 0xc0011fff) )
> +            goto gp_fault;

I wonder what you derive "architecturally inaccessable" from: On
one hand nothing exists there at present. Otoh it looks to me as if
you derive that state from MSRs outside these ranges always
being intercepted (which is not the same as inaccessable, just take
the hypervisor MSR as example). And then the last of these ranges
would appear to be AMD specific.

> @@ -69,6 +71,18 @@
>  /* Lower 6 bits define the format of the address in the LBR stack */
>  #define MSR_IA32_PERF_CAP_LBR_FORMAT	0x3f
>  
> +#define MSR_SGX_SVN_STATUS		0x00000500
> +
> +#define MSR_DEBUG_INTERFACE		0x00000c80
> +
> +#define MSR_L3_QOS_CFG			0x00000c81
> +#define MSR_L2_QOS_CFG			0x00000c82
> +#define MSR_QM_EVTSEL			0x00000c8d
> +#define MSR_QM_CTR			0x00000c8e
> +#define MSR_PQR_ASSOC			0x00000c8f
> +#define MSR_CAT_MASK_START		0x00000c90
> +#define MSR_CAT_MASK_LAST		0x00000d8f

For one, MSR_CAT_MASK_FIRST to better pair with ..._LAST
again? And then - why all these duplicates? We already have

/* Platform Shared Resource MSRs */
#define MSR_IA32_CMT_EVTSEL		0x00000c8d
#define MSR_IA32_CMT_EVTSEL_UE_MASK	0x0000ffff
#define MSR_IA32_CMT_CTR		0x00000c8e
#define MSR_IA32_PSR_ASSOC		0x00000c8f
#define MSR_IA32_PSR_L3_QOS_CFG	0x00000c81
#define MSR_IA32_PSR_L3_MASK(n)	(0x00000c90 + (n))
#define MSR_IA32_PSR_L3_MASK_CODE(n)	(0x00000c90 + (n) * 2 + 1)
#define MSR_IA32_PSR_L3_MASK_DATA(n)	(0x00000c90 + (n) * 2)
#define MSR_IA32_PSR_L2_MASK(n)		(0x00000d10 + (n))
#define MSR_IA32_PSR_MBA_MASK(n)	(0x00000d50 + (n))

and considering that we supposedly have L2 support I'm a little
puzzled that there's no #define of use of 0xc82 so far.

Jan


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