[PATCH v3 0/3] x86: Cleanup of MSR handling for Xen and Viridian ranges

[PATCH v3 0/3] x86: Cleanup of MSR handling for Xen and Viridian ranges

[Andrew Cooper]

Split apart from v2, to make all the dispatching code obvious in patch 1.  See
individual patches for changes.

Andrew Cooper (3):
  x86/msr: Dispatch Xen and Viridian MSRs from guest_{wr,rd}msr()
  x86/viridan: Clean up Viridian MSR infrastructure
  x86: Clean up the Xen MSR infrastructure

 xen/arch/x86/hvm/svm/svm.c         | 27 +++-------------------
 xen/arch/x86/hvm/viridian.c        | 46 ++++++++++++++------------------------
 xen/arch/x86/hvm/vmx/vmx.c         | 28 ++++-------------------
 xen/arch/x86/msr.c                 | 35 +++++++++++++++++++++++++----
 xen/arch/x86/pv/emul-priv-op.c     |  6 -----
 xen/arch/x86/traps.c               | 29 +++++++++++-------------
 xen/include/asm-x86/hvm/viridian.h | 11 ++-------
 xen/include/asm-x86/processor.h    |  4 ++--
 8 files changed, 72 insertions(+), 114 deletions(-)

-- 
2.1.4


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

[PATCH v3 1/3] x86/msr: Dispatch Xen and Viridian MSRs from guest_{wr, rd}msr()

[Andrew Cooper]

Despite the complicated diff in {svm,vmx}_msr_write_intercept(), it is just
the 0 case losing one level of indentation, as part of removing the call to
wrmsr_hypervisor_regs().

The case blocks in guest_{wr,rd}msr() use raw numbers, partly for consistency
with the CPUID side of things, but mainly because this is clearer code to
follow.  In particular, the Xen block may overlap with the Viridian block if
Viridian is not enabled for the domain, and trying to express this with named
literals caused more confusion that it solved.

Future changes with clean up the individual APIs, including allowing these
MSRs to be usable for vcpus other than current (no callers exist with v !=
current).

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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v3:
 * Split out of previous series.  Retain appropriate R-by's
---
 xen/arch/x86/hvm/svm/svm.c     | 27 +++------------------------
 xen/arch/x86/hvm/vmx/vmx.c     | 28 ++++------------------------
 xen/arch/x86/msr.c             | 39 +++++++++++++++++++++++++++++++++++----
 xen/arch/x86/pv/emul-priv-op.c |  6 ------
 4 files changed, 42 insertions(+), 58 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 34d55b4..ef8f271 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2016,10 +2016,6 @@ 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) )
-            break;
-
         if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
 
@@ -2218,28 +2214,11 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         else if ( ret )
             break;
 
-        if ( wrmsr_viridian_regs(msr, msr_content) )
+        /* Match up with the RDMSR side; ultimately this should go away. */
+        if ( rdmsr_safe(msr, msr_content) == 0 )
             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:
-            break;
-        default:
-            goto gpf;
-        }
-        break;
+        goto gpf;
     }
 
     return result;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b2e1a28..bf90e22 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2965,10 +2965,6 @@ 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) )
-            break;
-
         if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
 
@@ -3249,31 +3245,15 @@ 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(v, msr, msr_content) == 0 ||
              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 b675f3a..7f60e85 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -114,9 +114,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_policy *mp = v->domain->arch.msr;
+    const struct domain *d = v->domain;
+    const struct cpuid_policy *cp = d->arch.cpuid;
+    const struct msr_policy *mp = d->arch.msr;
     const struct vcpu_msrs *msrs = v->arch.msrs;
+    int ret = X86EMUL_OKAY;
 
     switch ( msr )
     {
@@ -145,11 +147,25 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         *val = msrs->misc_features_enables.raw;
         break;
 
+    case 0x40000000 ... 0x400001ff:
+        if ( is_viridian_domain(d) )
+        {
+            ret = (rdmsr_viridian_regs(msr, val)
+                   ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+            break;
+        }
+
+        /* Fallthrough. */
+    case 0x40000200 ... 0x400002ff:
+        ret = (rdmsr_hypervisor_regs(msr, val)
+               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+        break;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
 
-    return X86EMUL_OKAY;
+    return ret;
 
  gp_fault:
     return X86EMUL_EXCEPTION;
@@ -162,6 +178,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     const struct cpuid_policy *cp = d->arch.cpuid;
     const struct msr_policy *mp = d->arch.msr;
     struct vcpu_msrs *msrs = v->arch.msrs;
+    int ret = X86EMUL_OKAY;
 
     switch ( msr )
     {
@@ -252,11 +269,25 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
     }
 
+    case 0x40000000 ... 0x400001ff:
+        if ( is_viridian_domain(d) )
+        {
+            ret = (wrmsr_viridian_regs(msr, val)
+                   ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+            break;
+        }
+
+        /* Fallthrough. */
+    case 0x40000200 ... 0x400002ff:
+        ret = (wrmsr_hypervisor_regs(msr, val) == 1
+               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+        break;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
 
-    return X86EMUL_OKAY;
+    return ret;
 
  gp_fault:
     return X86EMUL_EXCEPTION;
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 45941ea..6422f91 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -950,9 +950,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;
@@ -1149,9 +1146,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;
-- 
2.1.4


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

[PATCH v4 1/3] x86/msr: Dispatch Xen and Viridian MSRs from guest_{wr, rd}msr()

[Andrew Cooper]

Despite the complicated diff in {svm,vmx}_msr_write_intercept(), it is just
the 0 case losing one level of indentation, as part of removing the call to
wrmsr_hypervisor_regs().

The case blocks in guest_{wr,rd}msr() use raw numbers, partly for consistency
with the CPUID side of things, but mainly because this is clearer code to
follow.  In particular, the Xen block may overlap with the Viridian block if
Viridian is not enabled for the domain, and trying to express this with named
literals caused more confusion that it solved.

Future changes with clean up the individual APIs, including allowing these
MSRs to be usable for vcpus other than current (no callers exist with v !=
current).

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

v3:
 * Split out of previous series.  Retain appropriate R-by's
v4:
 * Retain switch() for interpreting the result of wrmsr_hypervisor_regs()
---
 xen/arch/x86/hvm/svm/svm.c     | 27 +++-----------------------
 xen/arch/x86/hvm/vmx/vmx.c     | 28 ++++-----------------------
 xen/arch/x86/msr.c             | 43 ++++++++++++++++++++++++++++++++++++++----
 xen/arch/x86/pv/emul-priv-op.c |  6 ------
 4 files changed, 46 insertions(+), 58 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 34d55b4..ef8f271 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2016,10 +2016,6 @@ 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) )
-            break;
-
         if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
 
@@ -2218,28 +2214,11 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         else if ( ret )
             break;
 
-        if ( wrmsr_viridian_regs(msr, msr_content) )
+        /* Match up with the RDMSR side; ultimately this should go away. */
+        if ( rdmsr_safe(msr, msr_content) == 0 )
             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:
-            break;
-        default:
-            goto gpf;
-        }
-        break;
+        goto gpf;
     }
 
     return result;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b2e1a28..bf90e22 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2965,10 +2965,6 @@ 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) )
-            break;
-
         if ( rdmsr_safe(msr, *msr_content) == 0 )
             break;
 
@@ -3249,31 +3245,15 @@ 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(v, msr, msr_content) == 0 ||
              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 b675f3a..d7c38f1 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -114,9 +114,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_policy *mp = v->domain->arch.msr;
+    const struct domain *d = v->domain;
+    const struct cpuid_policy *cp = d->arch.cpuid;
+    const struct msr_policy *mp = d->arch.msr;
     const struct vcpu_msrs *msrs = v->arch.msrs;
+    int ret = X86EMUL_OKAY;
 
     switch ( msr )
     {
@@ -145,11 +147,25 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         *val = msrs->misc_features_enables.raw;
         break;
 
+    case 0x40000000 ... 0x400001ff:
+        if ( is_viridian_domain(d) )
+        {
+            ret = (rdmsr_viridian_regs(msr, val)
+                   ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+            break;
+        }
+
+        /* Fallthrough. */
+    case 0x40000200 ... 0x400002ff:
+        ret = (rdmsr_hypervisor_regs(msr, val)
+               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+        break;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
 
-    return X86EMUL_OKAY;
+    return ret;
 
  gp_fault:
     return X86EMUL_EXCEPTION;
@@ -162,6 +178,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     const struct cpuid_policy *cp = d->arch.cpuid;
     const struct msr_policy *mp = d->arch.msr;
     struct vcpu_msrs *msrs = v->arch.msrs;
+    int ret = X86EMUL_OKAY;
 
     switch ( msr )
     {
@@ -252,11 +269,29 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
     }
 
+    case 0x40000000 ... 0x400001ff:
+        if ( is_viridian_domain(d) )
+        {
+            ret = (wrmsr_viridian_regs(msr, val)
+                   ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+            break;
+        }
+
+        /* Fallthrough. */
+    case 0x40000200 ... 0x400002ff:
+        switch ( wrmsr_hypervisor_regs(msr, val) )
+        {
+        case -ERESTART: ret = X86EMUL_RETRY;     break;
+        case 1:         ret = X86EMUL_OKAY;      break;
+        default:        ret = X86EMUL_EXCEPTION; break;
+        }
+        break;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
 
-    return X86EMUL_OKAY;
+    return ret;
 
  gp_fault:
     return X86EMUL_EXCEPTION;
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 45941ea..6422f91 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -950,9 +950,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;
@@ -1149,9 +1146,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;
-- 
2.1.4


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

Re: [PATCH v4 1/3] x86/msr: Dispatch Xen and Viridian MSRs from guest_{wr, rd}msr()

[Sergey Dyasli]

On Wed, 2018-09-12 at 13:00 +0100, Andrew Cooper wrote:
> Despite the complicated diff in {svm,vmx}_msr_write_intercept(), it is just
> the 0 case losing one level of indentation, as part of removing the call to
> wrmsr_hypervisor_regs().
> 
> The case blocks in guest_{wr,rd}msr() use raw numbers, partly for consistency
> with the CPUID side of things, but mainly because this is clearer code to
> follow.  In particular, the Xen block may overlap with the Viridian block if
> Viridian is not enabled for the domain, and trying to express this with named
> literals caused more confusion that it solved.
> 
> Future changes with clean up the individual APIs, including allowing these
> MSRs to be usable for vcpus other than current (no callers exist with v !=
> current).
> 
> 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>
> ---
> 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>
> 
> v3:
>  * Split out of previous series.  Retain appropriate R-by's
> v4:
>  * Retain switch() for interpreting the result of wrmsr_hypervisor_regs()
> ---
>  xen/arch/x86/hvm/svm/svm.c     | 27 +++-----------------------
>  xen/arch/x86/hvm/vmx/vmx.c     | 28 ++++-----------------------
>  xen/arch/x86/msr.c             | 43 ++++++++++++++++++++++++++++++++++++++----
>  xen/arch/x86/pv/emul-priv-op.c |  6 ------
>  4 files changed, 46 insertions(+), 58 deletions(-)

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

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

Re: [PATCH v4 1/3] x86/msr: Dispatch Xen and Viridian MSRs from guest_{wr, rd}msr()

[Jan Beulich]

>>> On 12.09.18 at 14:00, <andrew.cooper3@citrix.com> wrote:
> Despite the complicated diff in {svm,vmx}_msr_write_intercept(), it is just
> the 0 case losing one level of indentation, as part of removing the call to
> wrmsr_hypervisor_regs().
> 
> The case blocks in guest_{wr,rd}msr() use raw numbers, partly for consistency
> with the CPUID side of things, but mainly because this is clearer code to
> follow.  In particular, the Xen block may overlap with the Viridian block if
> Viridian is not enabled for the domain, and trying to express this with named
> literals caused more confusion that it solved.

Well, imo it's not much better this way, but also not much worse.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -114,9 +114,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_policy *mp = v->domain->arch.msr;
> +    const struct domain *d = v->domain;
> +    const struct cpuid_policy *cp = d->arch.cpuid;
> +    const struct msr_policy *mp = d->arch.msr;
>      const struct vcpu_msrs *msrs = v->arch.msrs;
> +    int ret = X86EMUL_OKAY;
>  
>      switch ( msr )
>      {
> @@ -145,11 +147,25 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          *val = msrs->misc_features_enables.raw;
>          break;
>  
> +    case 0x40000000 ... 0x400001ff:
> +        if ( is_viridian_domain(d) )
> +        {
> +            ret = (rdmsr_viridian_regs(msr, val)
> +                   ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> +            break;
> +        }
> +
> +        /* Fallthrough. */
> +    case 0x40000200 ... 0x400002ff:
> +        ret = (rdmsr_hypervisor_regs(msr, val)
> +               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> +        break;
> +
>      default:
>          return X86EMUL_UNHANDLEABLE;

Considering the "return" here, could I talk you into using "return"
for your additions as well, rather than introducing a local variable?
Same for the wrmsr side then. In any event
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

[PATCH v3 2/3] x86/viridan: Clean up Viridian MSR infrastructure

[Andrew Cooper]

Rename the functions 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, and switch their return
ABI to use X86EMUL_*.

The default cases no longer need to deal with MSRs out of the Viridian range,
but drop the printks to debug builds only and identify the value attempting to
be written.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@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>

v3:
 * Clean up after splitting the series.  Retain appropriate R-by's
---
 xen/arch/x86/hvm/viridian.c        | 46 ++++++++++++++------------------------
 xen/arch/x86/msr.c                 |  6 ++---
 xen/include/asm-x86/hvm/viridian.h | 11 ++-------
 3 files changed, 21 insertions(+), 42 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index a23d087..48f7a31 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;
+            return X86EMUL_EXCEPTION;
 
         perfc_incr(mshv_wrmsr_tsc_msr);
         d->arch.hvm.viridian.reference_tsc.raw = val;
@@ -659,14 +654,12 @@ 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_INFO,
+                 "Write %016"PRIx64" to unimplemented MSR %#x\n", val, idx);
+        return X86EMUL_EXCEPTION;
     }
 
-    return 1;
+    return X86EMUL_OKAY;
 }
 
 static int64_t raw_trc_val(struct domain *d)
@@ -702,13 +695,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 )
     {
@@ -729,7 +720,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;
+            return X86EMUL_EXCEPTION;
 
         perfc_incr(mshv_rdmsr_tsc_frequency);
         *val = (uint64_t)d->arch.tsc_khz * 1000ull;
@@ -737,7 +728,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;
+            return X86EMUL_EXCEPTION;
 
         perfc_incr(mshv_rdmsr_apic_frequency);
         *val = 1000000000ull / APIC_BUS_CYCLE_NS;
@@ -761,7 +752,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;
+            return X86EMUL_EXCEPTION;
 
         perfc_incr(mshv_rdmsr_tsc_msr);
         *val = d->arch.hvm.viridian.reference_tsc.raw;
@@ -774,7 +765,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
         trc = &d->arch.hvm.viridian.time_ref_count;
 
         if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
-            return 0;
+            return X86EMUL_EXCEPTION;
 
         if ( !test_and_set_bit(_TRC_accessed, &trc->flags) )
             printk(XENLOG_G_INFO "d%d: VIRIDIAN MSR_TIME_REF_COUNT: accessed\n",
@@ -808,14 +799,11 @@ 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_INFO, "Read from unimplemented MSR %#x\n", idx);
+        return X86EMUL_EXCEPTION;
     }
 
-    return 1;
+    return X86EMUL_OKAY;
 }
 
 void viridian_vcpu_deinit(struct vcpu *v)
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 7f60e85..cf0dc27 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -150,8 +150,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
-            ret = (rdmsr_viridian_regs(msr, val)
-                   ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+            ret = guest_rdmsr_viridian(v, msr, val);
             break;
         }
 
@@ -272,8 +271,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
-            ret = (wrmsr_viridian_regs(msr, val)
-                   ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+            ret = guest_wrmsr_viridian(v, msr, val);
             break;
         }
 
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);
-- 
2.1.4


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

Re: [PATCH v3 2/3] x86/viridan: Clean up Viridian MSR infrastructure

[Sergey Dyasli]

On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
> Rename the functions 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, and switch their return
> ABI to use X86EMUL_*.
> 
> The default cases no longer need to deal with MSRs out of the Viridian range,
> but drop the printks to debug builds only and identify the value attempting to
> be written.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Paul Durrant <paul.durrant@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>
> 
> v3:
>  * Clean up after splitting the series.  Retain appropriate R-by's
> ---
>  xen/arch/x86/hvm/viridian.c        | 46 ++++++++++++++------------------------
>  xen/arch/x86/msr.c                 |  6 ++---
>  xen/include/asm-x86/hvm/viridian.h | 11 ++-------
>  3 files changed, 21 insertions(+), 42 deletions(-)
> 

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

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

Re: [PATCH v3 2/3] x86/viridan: Clean up Viridian MSR infrastructure

[Jan Beulich]

>>> On 11.09.18 at 20:56, <andrew.cooper3@citrix.com> wrote:
> Rename the functions 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, and switch their return
> ABI to use X86EMUL_*.
> 
> The default cases no longer need to deal with MSRs out of the Viridian range,
> but drop the printks to debug builds only and identify the value attempting to
> be written.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

[PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Andrew Cooper]

Rename them 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>
---
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>

v3:
 * Clean up after splitting the series.
---
 xen/arch/x86/msr.c              |  6 ++----
 xen/arch/x86/traps.c            | 29 +++++++++++++----------------
 xen/include/asm-x86/processor.h |  4 ++--
 3 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index cf0dc27..8f02a89 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -156,8 +156,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
 
         /* Fallthrough. */
     case 0x40000200 ... 0x400002ff:
-        ret = (rdmsr_hypervisor_regs(msr, val)
-               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+        ret = guest_rdmsr_xen(v, msr, val);
         break;
 
     default:
@@ -277,8 +276,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
 
         /* Fallthrough. */
     case 0x40000200 ... 0x400002ff:
-        ret = (wrmsr_hypervisor_regs(msr, val) == 1
-               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
+        ret = guest_wrmsr_xen(v, msr, val);
         break;
 
     default:
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 7c17806..3988753 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -768,29 +768,25 @@ 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;
 
     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;
 
@@ -809,7 +805,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;
+            return X86EMUL_EXCEPTION;
         }
 
         page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
@@ -822,13 +818,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 %#"PRI_mfn") to MSR %08x\n",
                      gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN), base);
-            return 0;
+            return X86EMUL_EXCEPTION;
         }
 
         hypercall_page = __map_domain_page(page);
@@ -836,11 +832,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;
+    default:
+        return X86EMUL_EXCEPTION;
+    }
 }
 
 void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index a166802..03555e1 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -554,8 +554,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 v3 3/3] x86: Clean up the Xen MSR infrastructure

[Sergey Dyasli]

On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
> Rename them 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>
> ---
> 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>
> 
> v3:
>  * Clean up after splitting the series.
> ---
>  xen/arch/x86/msr.c              |  6 ++----
>  xen/arch/x86/traps.c            | 29 +++++++++++++----------------
>  xen/include/asm-x86/processor.h |  4 ++--
>  3 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index cf0dc27..8f02a89 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -156,8 +156,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>  
>          /* Fallthrough. */
>      case 0x40000200 ... 0x400002ff:
> -        ret = (rdmsr_hypervisor_regs(msr, val)
> -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> +        ret = guest_rdmsr_xen(v, msr, val);
>          break;
>  
>      default:
> @@ -277,8 +276,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>  
>          /* Fallthrough. */
>      case 0x40000200 ... 0x400002ff:
> -        ret = (wrmsr_hypervisor_regs(msr, val) == 1
> -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> +        ret = guest_wrmsr_xen(v, msr, val);
>          break;
>  
>      default:
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 7c17806..3988753 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -768,29 +768,25 @@ 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;
>  
>      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;
>  
> @@ -809,7 +805,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;
> +            return X86EMUL_EXCEPTION;
>          }
>  
>          page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
> @@ -822,13 +818,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;

Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
with this patch, X86EMUL_RETRY will actually be returned. I don't think
that callers can handle this situation.

E.g. the code from vmx_vmexit_handler():

    case EXIT_REASON_MSR_WRITE:
        switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
        {
        case X86EMUL_OKAY:
            update_guest_eip(); /* Safe: WRMSR */
            break;

        case X86EMUL_EXCEPTION:
            hvm_inject_hw_exception(TRAP_gp_fault, 0);
            break;
        }
        break;

>              }
>  
>              gdprintk(XENLOG_WARNING,
>                       "Bad GMFN %lx (MFN %#"PRI_mfn") to MSR %08x\n",
>                       gmfn, mfn_x(page ? page_to_mfn(page) : INVALID_MFN), base);
> -            return 0;
> +            return X86EMUL_EXCEPTION;
>          }
>  
>          hypercall_page = __map_domain_page(page);
> @@ -836,11 +832,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;
> +    default:
> +        return X86EMUL_EXCEPTION;
> +    }
>  }
>  
>  void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
> index a166802..03555e1 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -554,8 +554,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);
-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Andrew Cooper]

On 12/09/18 09:29, Sergey Dyasli wrote:
> On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
>> Rename them 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>
>> ---
>> 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>
>>
>> v3:
>>  * Clean up after splitting the series.
>> ---
>>  xen/arch/x86/msr.c              |  6 ++----
>>  xen/arch/x86/traps.c            | 29 +++++++++++++----------------
>>  xen/include/asm-x86/processor.h |  4 ++--
>>  3 files changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
>> index cf0dc27..8f02a89 100644
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -156,8 +156,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>  
>>          /* Fallthrough. */
>>      case 0x40000200 ... 0x400002ff:
>> -        ret = (rdmsr_hypervisor_regs(msr, val)
>> -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
>> +        ret = guest_rdmsr_xen(v, msr, val);
>>          break;
>>  
>>      default:
>> @@ -277,8 +276,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>>  
>>          /* Fallthrough. */
>>      case 0x40000200 ... 0x400002ff:
>> -        ret = (wrmsr_hypervisor_regs(msr, val) == 1
>> -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
>> +        ret = guest_wrmsr_xen(v, msr, val);
>>          break;
>>  
>>      default:
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 7c17806..3988753 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -768,29 +768,25 @@ 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;
>>  
>>      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;
>>  
>> @@ -809,7 +805,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;
>> +            return X86EMUL_EXCEPTION;
>>          }
>>  
>>          page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
>> @@ -822,13 +818,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;
> Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
> with this patch, X86EMUL_RETRY will actually be returned. I don't think
> that callers can handle this situation.
>
> E.g. the code from vmx_vmexit_handler():
>
>     case EXIT_REASON_MSR_WRITE:
>         switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
>         {
>         case X86EMUL_OKAY:
>             update_guest_eip(); /* Safe: WRMSR */
>             break;
>
>         case X86EMUL_EXCEPTION:
>             hvm_inject_hw_exception(TRAP_gp_fault, 0);
>             break;
>         }
>         break;

Hmm lovely, so it was broken before, but should be correct now.

RETRY has caused an entry to go onto the paging ring, which will pause
the vcpu until a reply occurs, after which we will re-enter the guest
without having moved RIP forwards, re-execute the wrmsr instruction, and
this time succeed because the frame has been paged in.

~Andrew

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

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Jan Beulich]

>>> On 12.09.18 at 11:12, <andrew.cooper3@citrix.com> wrote:
> On 12/09/18 09:29, Sergey Dyasli wrote:
>> On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
>>> @@ -822,13 +818,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;
>> Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
>> with this patch, X86EMUL_RETRY will actually be returned. I don't think
>> that callers can handle this situation.
>>
>> E.g. the code from vmx_vmexit_handler():
>>
>>     case EXIT_REASON_MSR_WRITE:
>>         switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
>>         {
>>         case X86EMUL_OKAY:
>>             update_guest_eip(); /* Safe: WRMSR */
>>             break;
>>
>>         case X86EMUL_EXCEPTION:
>>             hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>             break;
>>         }
>>         break;
> 
> Hmm lovely, so it was broken before, but should be correct now.
> 
> RETRY has caused an entry to go onto the paging ring, which will pause
> the vcpu until a reply occurs, after which we will re-enter the guest
> without having moved RIP forwards, re-execute the wrmsr instruction, and
> this time succeed because the frame has been paged in.

But then perhaps split out the actual bugfix into a prereq patch,
especially as that one may want backporting?

Jan



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

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Andrew Cooper]

On 12/09/18 10:17, Jan Beulich wrote:
>>>> On 12.09.18 at 11:12, <andrew.cooper3@citrix.com> wrote:
>> On 12/09/18 09:29, Sergey Dyasli wrote:
>>> On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
>>>> @@ -822,13 +818,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;
>>> Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
>>> with this patch, X86EMUL_RETRY will actually be returned. I don't think
>>> that callers can handle this situation.
>>>
>>> E.g. the code from vmx_vmexit_handler():
>>>
>>>     case EXIT_REASON_MSR_WRITE:
>>>         switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
>>>         {
>>>         case X86EMUL_OKAY:
>>>             update_guest_eip(); /* Safe: WRMSR */
>>>             break;
>>>
>>>         case X86EMUL_EXCEPTION:
>>>             hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>>             break;
>>>         }
>>>         break;
>> Hmm lovely, so it was broken before, but should be correct now.
>>
>> RETRY has caused an entry to go onto the paging ring, which will pause
>> the vcpu until a reply occurs, after which we will re-enter the guest
>> without having moved RIP forwards, re-execute the wrmsr instruction, and
>> this time succeed because the frame has been paged in.
> But then perhaps split out the actual bugfix into a prereq patch,
> especially as that one may want backporting?

Sorry, but I'm not sure you can't have that.  It will depend on the
internals of the hooks, but I suspect you'd need the entire rest of my
MSR cleanup work if you wanted to cleanly backport this bugfix.

~Andrew

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

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Jan Beulich]

>>> On 12.09.18 at 11:24, <andrew.cooper3@citrix.com> wrote:
> On 12/09/18 10:17, Jan Beulich wrote:
>>>>> On 12.09.18 at 11:12, <andrew.cooper3@citrix.com> wrote:
>>> On 12/09/18 09:29, Sergey Dyasli wrote:
>>>> On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
>>>>> @@ -822,13 +818,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;
>>>> Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
>>>> with this patch, X86EMUL_RETRY will actually be returned. I don't think
>>>> that callers can handle this situation.
>>>>
>>>> E.g. the code from vmx_vmexit_handler():
>>>>
>>>>     case EXIT_REASON_MSR_WRITE:
>>>>         switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
>>>>         {
>>>>         case X86EMUL_OKAY:
>>>>             update_guest_eip(); /* Safe: WRMSR */
>>>>             break;
>>>>
>>>>         case X86EMUL_EXCEPTION:
>>>>             hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>>>             break;
>>>>         }
>>>>         break;
>>> Hmm lovely, so it was broken before, but should be correct now.
>>>
>>> RETRY has caused an entry to go onto the paging ring, which will pause
>>> the vcpu until a reply occurs, after which we will re-enter the guest
>>> without having moved RIP forwards, re-execute the wrmsr instruction, and
>>> this time succeed because the frame has been paged in.
>> But then perhaps split out the actual bugfix into a prereq patch,
>> especially as that one may want backporting?
> 
> Sorry, but I'm not sure you can't have that.  It will depend on the
> internals of the hooks, but I suspect you'd need the entire rest of my
> MSR cleanup work if you wanted to cleanly backport this bugfix.

Ouch, well - then: No.

Jan



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

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Andrew Cooper]

On 12/09/18 11:05, Jan Beulich wrote:
>>>> On 12.09.18 at 11:24, <andrew.cooper3@citrix.com> wrote:
>> On 12/09/18 10:17, Jan Beulich wrote:
>>>>>> On 12.09.18 at 11:12, <andrew.cooper3@citrix.com> wrote:
>>>> On 12/09/18 09:29, Sergey Dyasli wrote:
>>>>> On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
>>>>>> @@ -822,13 +818,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;
>>>>> Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
>>>>> with this patch, X86EMUL_RETRY will actually be returned. I don't think
>>>>> that callers can handle this situation.
>>>>>
>>>>> E.g. the code from vmx_vmexit_handler():
>>>>>
>>>>>     case EXIT_REASON_MSR_WRITE:
>>>>>         switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
>>>>>         {
>>>>>         case X86EMUL_OKAY:
>>>>>             update_guest_eip(); /* Safe: WRMSR */
>>>>>             break;
>>>>>
>>>>>         case X86EMUL_EXCEPTION:
>>>>>             hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>>>>             break;
>>>>>         }
>>>>>         break;
>>>> Hmm lovely, so it was broken before, but should be correct now.
>>>>
>>>> RETRY has caused an entry to go onto the paging ring, which will pause
>>>> the vcpu until a reply occurs, after which we will re-enter the guest
>>>> without having moved RIP forwards, re-execute the wrmsr instruction, and
>>>> this time succeed because the frame has been paged in.
>>> But then perhaps split out the actual bugfix into a prereq patch,
>>> especially as that one may want backporting?
>> Sorry, but I'm not sure you can't have that.  It will depend on the
>> internals of the hooks, but I suspect you'd need the entire rest of my
>> MSR cleanup work if you wanted to cleanly backport this bugfix.
> Ouch, well - then: No.

Turns out it was a transitory bug introduced by patch 1, so no cause for
alarm.

It is latently buggy on the PV side, but there is no paging support for
PV guests yet, so no need to backport any fixes.

~Andrew

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

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Sergey Dyasli]

On Wed, 2018-09-12 at 10:12 +0100, Andrew Cooper wrote:
> On 12/09/18 09:29, Sergey Dyasli wrote:
> > On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
> > > Rename them 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>
> > > ---
> > > 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>
> > > 
> > > v3:
> > >  * Clean up after splitting the series.
> > > ---
> > >  xen/arch/x86/msr.c              |  6 ++----
> > >  xen/arch/x86/traps.c            | 29 +++++++++++++----------------
> > >  xen/include/asm-x86/processor.h |  4 ++--
> > >  3 files changed, 17 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> > > index cf0dc27..8f02a89 100644
> > > --- a/xen/arch/x86/msr.c
> > > +++ b/xen/arch/x86/msr.c
> > > @@ -156,8 +156,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
> > >  
> > >          /* Fallthrough. */
> > >      case 0x40000200 ... 0x400002ff:
> > > -        ret = (rdmsr_hypervisor_regs(msr, val)
> > > -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> > > +        ret = guest_rdmsr_xen(v, msr, val);
> > >          break;
> > >  
> > >      default:
> > > @@ -277,8 +276,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
> > >  
> > >          /* Fallthrough. */
> > >      case 0x40000200 ... 0x400002ff:
> > > -        ret = (wrmsr_hypervisor_regs(msr, val) == 1
> > > -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> > > +        ret = guest_wrmsr_xen(v, msr, val);
> > >          break;
> > >  
> > >      default:
> > > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > > index 7c17806..3988753 100644
> > > --- a/xen/arch/x86/traps.c
> > > +++ b/xen/arch/x86/traps.c
> > > @@ -768,29 +768,25 @@ 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;
> > >  
> > >      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;
> > >  
> > > @@ -809,7 +805,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;
> > > +            return X86EMUL_EXCEPTION;
> > >          }
> > >  
> > >          page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
> > > @@ -822,13 +818,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;
> > 
> > Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
> > with this patch, X86EMUL_RETRY will actually be returned. I don't think
> > that callers can handle this situation.
> > 
> > E.g. the code from vmx_vmexit_handler():
> > 
> >     case EXIT_REASON_MSR_WRITE:
> >         switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
> >         {
> >         case X86EMUL_OKAY:
> >             update_guest_eip(); /* Safe: WRMSR */
> >             break;
> > 
> >         case X86EMUL_EXCEPTION:
> >             hvm_inject_hw_exception(TRAP_gp_fault, 0);
> >             break;
> >         }
> >         break;
> 
> Hmm lovely, so it was broken before, but should be correct now.
> 
> RETRY has caused an entry to go onto the paging ring, which will pause
> the vcpu until a reply occurs, after which we will re-enter the guest
> without having moved RIP forwards, re-execute the wrmsr instruction, and
> this time succeed because the frame has been paged in.

Actually, the current VMX/SVM (but not PV) code does:

        switch ( wrmsr_hypervisor_regs(msr, msr_content) )
        {
        case -ERESTART:
            return X86EMUL_RETRY;

This code is removed in 1/3 patch but I wasn't CCed.

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

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Andrew Cooper]

On 12/09/18 10:46, Sergey Dyasli wrote:
> On Wed, 2018-09-12 at 10:12 +0100, Andrew Cooper wrote:
>> On 12/09/18 09:29, Sergey Dyasli wrote:
>>> On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
>>>> Rename them 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>
>>>> ---
>>>> 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>
>>>>
>>>> v3:
>>>>  * Clean up after splitting the series.
>>>> ---
>>>>  xen/arch/x86/msr.c              |  6 ++----
>>>>  xen/arch/x86/traps.c            | 29 +++++++++++++----------------
>>>>  xen/include/asm-x86/processor.h |  4 ++--
>>>>  3 files changed, 17 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
>>>> index cf0dc27..8f02a89 100644
>>>> --- a/xen/arch/x86/msr.c
>>>> +++ b/xen/arch/x86/msr.c
>>>> @@ -156,8 +156,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>>>  
>>>>          /* Fallthrough. */
>>>>      case 0x40000200 ... 0x400002ff:
>>>> -        ret = (rdmsr_hypervisor_regs(msr, val)
>>>> -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
>>>> +        ret = guest_rdmsr_xen(v, msr, val);
>>>>          break;
>>>>  
>>>>      default:
>>>> @@ -277,8 +276,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>>>>  
>>>>          /* Fallthrough. */
>>>>      case 0x40000200 ... 0x400002ff:
>>>> -        ret = (wrmsr_hypervisor_regs(msr, val) == 1
>>>> -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
>>>> +        ret = guest_wrmsr_xen(v, msr, val);
>>>>          break;
>>>>  
>>>>      default:
>>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>>>> index 7c17806..3988753 100644
>>>> --- a/xen/arch/x86/traps.c
>>>> +++ b/xen/arch/x86/traps.c
>>>> @@ -768,29 +768,25 @@ 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;
>>>>  
>>>>      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;
>>>>  
>>>> @@ -809,7 +805,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;
>>>> +            return X86EMUL_EXCEPTION;
>>>>          }
>>>>  
>>>>          page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
>>>> @@ -822,13 +818,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;
>>> Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
>>> with this patch, X86EMUL_RETRY will actually be returned. I don't think
>>> that callers can handle this situation.
>>>
>>> E.g. the code from vmx_vmexit_handler():
>>>
>>>     case EXIT_REASON_MSR_WRITE:
>>>         switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
>>>         {
>>>         case X86EMUL_OKAY:
>>>             update_guest_eip(); /* Safe: WRMSR */
>>>             break;
>>>
>>>         case X86EMUL_EXCEPTION:
>>>             hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>>             break;
>>>         }
>>>         break;
>> Hmm lovely, so it was broken before, but should be correct now.
>>
>> RETRY has caused an entry to go onto the paging ring, which will pause
>> the vcpu until a reply occurs, after which we will re-enter the guest
>> without having moved RIP forwards, re-execute the wrmsr instruction, and
>> this time succeed because the frame has been paged in.
> Actually, the current VMX/SVM (but not PV) code does:
>
>         switch ( wrmsr_hypervisor_regs(msr, msr_content) )
>         {
>         case -ERESTART:
>             return X86EMUL_RETRY;
>
> This code is removed in 1/3 patch but I wasn't CCed.

Ah right, in which case I need to temporarily transplant this switch
into patch 1.  Given its only the PV side which is then broken, I can
probably see about doing a bugfix for that.

~Andrew

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

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Sergey Dyasli]

On Wed, 2018-09-12 at 11:23 +0100, Andrew Cooper wrote:
> On 12/09/18 10:46, Sergey Dyasli wrote:
> > On Wed, 2018-09-12 at 10:12 +0100, Andrew Cooper wrote:
> > > On 12/09/18 09:29, Sergey Dyasli wrote:
> > > > On Tue, 2018-09-11 at 19:56 +0100, Andrew Cooper wrote:
> > > > > Rename them 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>
> > > > > ---
> > > > > 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>
> > > > > 
> > > > > v3:
> > > > >  * Clean up after splitting the series.
> > > > > ---
> > > > >  xen/arch/x86/msr.c              |  6 ++----
> > > > >  xen/arch/x86/traps.c            | 29 +++++++++++++----------------
> > > > >  xen/include/asm-x86/processor.h |  4 ++--
> > > > >  3 files changed, 17 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> > > > > index cf0dc27..8f02a89 100644
> > > > > --- a/xen/arch/x86/msr.c
> > > > > +++ b/xen/arch/x86/msr.c
> > > > > @@ -156,8 +156,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
> > > > >  
> > > > >          /* Fallthrough. */
> > > > >      case 0x40000200 ... 0x400002ff:
> > > > > -        ret = (rdmsr_hypervisor_regs(msr, val)
> > > > > -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> > > > > +        ret = guest_rdmsr_xen(v, msr, val);
> > > > >          break;
> > > > >  
> > > > >      default:
> > > > > @@ -277,8 +276,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
> > > > >  
> > > > >          /* Fallthrough. */
> > > > >      case 0x40000200 ... 0x400002ff:
> > > > > -        ret = (wrmsr_hypervisor_regs(msr, val) == 1
> > > > > -               ? X86EMUL_OKAY : X86EMUL_EXCEPTION);
> > > > > +        ret = guest_wrmsr_xen(v, msr, val);
> > > > >          break;
> > > > >  
> > > > >      default:
> > > > > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > > > > index 7c17806..3988753 100644
> > > > > --- a/xen/arch/x86/traps.c
> > > > > +++ b/xen/arch/x86/traps.c
> > > > > @@ -768,29 +768,25 @@ 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;
> > > > >  
> > > > >      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;
> > > > >  
> > > > > @@ -809,7 +805,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;
> > > > > +            return X86EMUL_EXCEPTION;
> > > > >          }
> > > > >  
> > > > >          page = get_page_from_gfn(d, gmfn, &t, P2M_ALLOC);
> > > > > @@ -822,13 +818,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;
> > > > 
> > > > Previously -ERESTART would've been converted to X86EMUL_EXCEPTION. But
> > > > with this patch, X86EMUL_RETRY will actually be returned. I don't think
> > > > that callers can handle this situation.
> > > > 
> > > > E.g. the code from vmx_vmexit_handler():
> > > > 
> > > >     case EXIT_REASON_MSR_WRITE:
> > > >         switch ( hvm_msr_write_intercept(regs->ecx, msr_fold(regs), 1) )
> > > >         {
> > > >         case X86EMUL_OKAY:
> > > >             update_guest_eip(); /* Safe: WRMSR */
> > > >             break;
> > > > 
> > > >         case X86EMUL_EXCEPTION:
> > > >             hvm_inject_hw_exception(TRAP_gp_fault, 0);
> > > >             break;
> > > >         }
> > > >         break;
> > > 
> > > Hmm lovely, so it was broken before, but should be correct now.
> > > 
> > > RETRY has caused an entry to go onto the paging ring, which will pause
> > > the vcpu until a reply occurs, after which we will re-enter the guest
> > > without having moved RIP forwards, re-execute the wrmsr instruction, and
> > > this time succeed because the frame has been paged in.
> > 
> > Actually, the current VMX/SVM (but not PV) code does:
> > 
> >         switch ( wrmsr_hypervisor_regs(msr, msr_content) )
> >         {
> >         case -ERESTART:
> >             return X86EMUL_RETRY;
> > 
> > This code is removed in 1/3 patch but I wasn't CCed.
> 
> Ah right, in which case I need to temporarily transplant this switch
> into patch 1.  Given its only the PV side which is then broken, I can
> probably see about doing a bugfix for that.

With this being rebased on top of v4 1/3:

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

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

Re: [PATCH v3 3/3] x86: Clean up the Xen MSR infrastructure

[Jan Beulich]

>>> On 13.09.18 at 09:57, <sergey.dyasli@citrix.com> wrote:
> With this being rebased on top of v4 1/3:
> 
> Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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