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

[PATCH 0/6] 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 (6):
  x86/vmx: Simplfy the default cases in vmx_msr_{read,write}_intercept()
  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/hvm: Constify the read side of vlapic handling
  x86/hvm: Handle x2apic MSRs the new guest_{rd,wr}msr() infrastructure
  x86/msr: Blacklist various MSRs which guests definitely shouldn't be using

 xen/arch/x86/hvm/hvm.c             |  10 --
 xen/arch/x86/hvm/svm/svm.c         |  27 +----
 xen/arch/x86/hvm/viridian.c        |  49 ++++-----
 xen/arch/x86/hvm/vlapic.c          |  74 +++++++------
 xen/arch/x86/hvm/vmx/vmx.c         | 208 +++++++++++++------------------------
 xen/arch/x86/hvm/vpt.c             |   2 +-
 xen/arch/x86/msr.c                 | 108 ++++++++++++++++++-
 xen/arch/x86/pv/emul-priv-op.c     |   6 --
 xen/arch/x86/traps.c               |  33 +++---
 xen/include/asm-x86/hvm/hvm.h      |   6 +-
 xen/include/asm-x86/hvm/viridian.h |  11 +-
 xen/include/asm-x86/processor.h    |   4 +-
 12 files changed, 268 insertions(+), 270 deletions(-)

-- 
2.1.4


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

[PATCH 1/6] x86/vmx: Simplfy the default cases in vmx_msr_{read, write}_intercept()

[Andrew Cooper]

The default case of vmx_msr_write_intercept() in particular is very tangled.

First of all, fold long_mode_do_msr_{read,write}() into their callers.  These
functions were split out in the past because of the 32bit build of Xen, but it
is unclear why the cases weren't simply #ifdef'd in place.

Next, invert the vmx_write_guest_msr()/is_last_branch_msr() logic to break if
the condition is satisfied, rather than nesting if it wasn't.  This allows the
wrmsr_hypervisor_regs() call to be un-nested with respect to the other default
logic.

No practical difference from a guests point of view.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 214 ++++++++++++++++++---------------------------
 1 file changed, 86 insertions(+), 128 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 8f6d87b..e1e4f17 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -482,102 +482,6 @@ static void vmx_vcpu_destroy(struct vcpu *v)
     passive_domain_destroy(v);
 }
 
-static int long_mode_do_msr_read(unsigned int msr, uint64_t *msr_content)
-{
-    struct vcpu *v = current;
-
-    switch ( msr )
-    {
-    case MSR_FS_BASE:
-        __vmread(GUEST_FS_BASE, msr_content);
-        break;
-
-    case MSR_GS_BASE:
-        __vmread(GUEST_GS_BASE, msr_content);
-        break;
-
-    case MSR_SHADOW_GS_BASE:
-        rdmsrl(MSR_SHADOW_GS_BASE, *msr_content);
-        break;
-
-    case MSR_STAR:
-        *msr_content = v->arch.hvm_vmx.star;
-        break;
-
-    case MSR_LSTAR:
-        *msr_content = v->arch.hvm_vmx.lstar;
-        break;
-
-    case MSR_CSTAR:
-        *msr_content = v->arch.hvm_vmx.cstar;
-        break;
-
-    case MSR_SYSCALL_MASK:
-        *msr_content = v->arch.hvm_vmx.sfmask;
-        break;
-
-    default:
-        return X86EMUL_UNHANDLEABLE;
-    }
-
-    HVM_DBG_LOG(DBG_LEVEL_MSR, "msr %#x content %#"PRIx64, msr, *msr_content);
-
-    return X86EMUL_OKAY;
-}
-
-static int long_mode_do_msr_write(unsigned int msr, uint64_t msr_content)
-{
-    struct vcpu *v = current;
-
-    HVM_DBG_LOG(DBG_LEVEL_MSR, "msr %#x content %#"PRIx64, msr, msr_content);
-
-    switch ( msr )
-    {
-    case MSR_FS_BASE:
-    case MSR_GS_BASE:
-    case MSR_SHADOW_GS_BASE:
-        if ( !is_canonical_address(msr_content) )
-            return X86EMUL_EXCEPTION;
-
-        if ( msr == MSR_FS_BASE )
-            __vmwrite(GUEST_FS_BASE, msr_content);
-        else if ( msr == MSR_GS_BASE )
-            __vmwrite(GUEST_GS_BASE, msr_content);
-        else
-            wrmsrl(MSR_SHADOW_GS_BASE, msr_content);
-
-        break;
-
-    case MSR_STAR:
-        v->arch.hvm_vmx.star = msr_content;
-        wrmsrl(MSR_STAR, msr_content);
-        break;
-
-    case MSR_LSTAR:
-        if ( !is_canonical_address(msr_content) )
-            return X86EMUL_EXCEPTION;
-        v->arch.hvm_vmx.lstar = msr_content;
-        wrmsrl(MSR_LSTAR, msr_content);
-        break;
-
-    case MSR_CSTAR:
-        if ( !is_canonical_address(msr_content) )
-            return X86EMUL_EXCEPTION;
-        v->arch.hvm_vmx.cstar = msr_content;
-        break;
-
-    case MSR_SYSCALL_MASK:
-        v->arch.hvm_vmx.sfmask = msr_content;
-        wrmsrl(MSR_SYSCALL_MASK, msr_content);
-        break;
-
-    default:
-        return X86EMUL_UNHANDLEABLE;
-    }
-
-    return X86EMUL_OKAY;
-}
-
 /*
  * To avoid MSR save/restore at every VM exit/entry time, we restore
  * the x86_64 specific MSRs at domain switch time. Since these MSRs
@@ -2894,6 +2798,35 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     case MSR_IA32_SYSENTER_EIP:
         __vmread(GUEST_SYSENTER_EIP, msr_content);
         break;
+
+    case MSR_FS_BASE:
+        __vmread(GUEST_FS_BASE, msr_content);
+        break;
+
+    case MSR_GS_BASE:
+        __vmread(GUEST_GS_BASE, msr_content);
+        break;
+
+    case MSR_SHADOW_GS_BASE:
+        rdmsrl(MSR_SHADOW_GS_BASE, *msr_content);
+        break;
+
+    case MSR_STAR:
+        *msr_content = curr->arch.hvm_vmx.star;
+        break;
+
+    case MSR_LSTAR:
+        *msr_content = curr->arch.hvm_vmx.lstar;
+        break;
+
+    case MSR_CSTAR:
+        *msr_content = curr->arch.hvm_vmx.cstar;
+        break;
+
+    case MSR_SYSCALL_MASK:
+        *msr_content = curr->arch.hvm_vmx.sfmask;
+        break;
+
     case MSR_IA32_DEBUGCTLMSR:
         __vmread(GUEST_IA32_DEBUGCTL, msr_content);
         break;
@@ -2928,14 +2861,6 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
     default:
         if ( passive_domain_do_rdmsr(msr, msr_content) )
             goto done;
-        switch ( long_mode_do_msr_read(msr, msr_content) )
-        {
-        case X86EMUL_EXCEPTION:
-            return X86EMUL_EXCEPTION;
-
-        case X86EMUL_OKAY:
-            goto done;
-        }
 
         if ( vmx_read_guest_msr(msr, msr_content) == 0 )
             break;
@@ -3090,6 +3015,45 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
             goto gp_fault;
         __vmwrite(GUEST_SYSENTER_EIP, msr_content);
         break;
+
+    case MSR_FS_BASE:
+    case MSR_GS_BASE:
+    case MSR_SHADOW_GS_BASE:
+        if ( !is_canonical_address(msr_content) )
+            goto gp_fault;
+
+        if ( msr == MSR_FS_BASE )
+            __vmwrite(GUEST_FS_BASE, msr_content);
+        else if ( msr == MSR_GS_BASE )
+            __vmwrite(GUEST_GS_BASE, msr_content);
+        else
+            wrmsrl(MSR_SHADOW_GS_BASE, msr_content);
+
+        break;
+
+    case MSR_STAR:
+        v->arch.hvm_vmx.star = msr_content;
+        wrmsrl(MSR_STAR, msr_content);
+        break;
+
+    case MSR_LSTAR:
+        if ( !is_canonical_address(msr_content) )
+            goto gp_fault;
+        v->arch.hvm_vmx.lstar = msr_content;
+        wrmsrl(MSR_LSTAR, msr_content);
+        break;
+
+    case MSR_CSTAR:
+        if ( !is_canonical_address(msr_content) )
+            goto gp_fault;
+        v->arch.hvm_vmx.cstar = msr_content;
+        break;
+
+    case MSR_SYSCALL_MASK:
+        v->arch.hvm_vmx.sfmask = msr_content;
+        wrmsrl(MSR_SYSCALL_MASK, msr_content);
+        break;
+
     case MSR_IA32_DEBUGCTLMSR: {
         int i, rc = 0;
         uint64_t supported = IA32_DEBUGCTLMSR_LBR | IA32_DEBUGCTLMSR_BTF;
@@ -3151,32 +3115,26 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         if ( wrmsr_viridian_regs(msr, msr_content) ) 
             break;
 
-        switch ( long_mode_do_msr_write(msr, msr_content) )
-        {
-        case X86EMUL_UNHANDLEABLE:
-            if ( (vmx_write_guest_msr(msr, msr_content) != 0) &&
-                 !is_last_branch_msr(msr) )
-                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:
-                    break;
-                default:
-                    goto gp_fault;
-                }
+        if ( vmx_write_guest_msr(msr, msr_content) == 0 ||
+             is_last_branch_msr(msr) )
             break;
 
-        case X86EMUL_EXCEPTION:
-            return X86EMUL_EXCEPTION;
+        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:
+            break;
+        default:
+            goto gp_fault;
         }
         break;
     }
-- 
2.1.4


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

Re: [PATCH 1/6] x86/vmx: Simplfy the default cases in vmx_msr_{read, write}_intercept()

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, February 27, 2018 1:35 AM
> 
> The default case of vmx_msr_write_intercept() in particular is very tangled.
> 
> First of all, fold long_mode_do_msr_{read,write}() into their callers.  These
> functions were split out in the past because of the 32bit build of Xen, but it
> is unclear why the cases weren't simply #ifdef'd in place.
> 
> Next, invert the vmx_write_guest_msr()/is_last_branch_msr() logic to break
> if
> the condition is satisfied, rather than nesting if it wasn't.  This allows the
> wrmsr_hypervisor_regs() call to be un-nested with respect to the other
> default
> logic.
> 
> No practical difference from a guests point of view.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-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 1/6] x86/vmx: Simplfy the default cases in vmx_msr_{read, write}_intercept()

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 05:35:14PM +0000, Andrew Cooper wrote:
> The default case of vmx_msr_write_intercept() in particular is very tangled.
> 
> First of all, fold long_mode_do_msr_{read,write}() into their callers.  These
> functions were split out in the past because of the 32bit build of Xen, but it
> is unclear why the cases weren't simply #ifdef'd in place.
> 
> Next, invert the vmx_write_guest_msr()/is_last_branch_msr() logic to break if
> the condition is satisfied, rather than nesting if it wasn't.  This allows the
> wrmsr_hypervisor_regs() call to be un-nested with respect to the other default
> logic.
> 
> No practical difference from a guests point of view.

I think there's a difference from guest PoV now, guest wrmsr of
GS/FS/LSTAR/CSTAR with non-canonical addresses will get a #GP.

> @@ -3090,6 +3015,45 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>              goto gp_fault;
>          __vmwrite(GUEST_SYSENTER_EIP, msr_content);
>          break;
> +
> +    case MSR_FS_BASE:
> +    case MSR_GS_BASE:
> +    case MSR_SHADOW_GS_BASE:
> +        if ( !is_canonical_address(msr_content) )
> +            goto gp_fault;

This is AFAICT different from previous behaviour, previous code would
just return X86EMUL_EXCEPTION without injecting a fault. From the SDM
I see injecting a #GP is the correct behaviour.

> +
> +        if ( msr == MSR_FS_BASE )
> +            __vmwrite(GUEST_FS_BASE, msr_content);
> +        else if ( msr == MSR_GS_BASE )
> +            __vmwrite(GUEST_GS_BASE, msr_content);
> +        else
> +            wrmsrl(MSR_SHADOW_GS_BASE, msr_content);
> +
> +        break;
> +
> +    case MSR_STAR:
> +        v->arch.hvm_vmx.star = msr_content;
> +        wrmsrl(MSR_STAR, msr_content);
> +        break;
> +
> +    case MSR_LSTAR:
> +        if ( !is_canonical_address(msr_content) )
> +            goto gp_fault;
> +        v->arch.hvm_vmx.lstar = msr_content;
> +        wrmsrl(MSR_LSTAR, msr_content);
> +        break;
> +
> +    case MSR_CSTAR:
> +        if ( !is_canonical_address(msr_content) )
> +            goto gp_fault;
> +        v->arch.hvm_vmx.cstar = msr_content;

Likely a stupid question, but why is CSTAR not written here? (like it's
done for LSTAR)

Thanks, Roger.

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

Re: [PATCH 1/6] x86/vmx: Simplfy the default cases in vmx_msr_{read, write}_intercept()

[Andrew Cooper]

On 27/02/18 12:43, Roger Pau Monné wrote:
> On Mon, Feb 26, 2018 at 05:35:14PM +0000, Andrew Cooper wrote:
>> The default case of vmx_msr_write_intercept() in particular is very tangled.
>>
>> First of all, fold long_mode_do_msr_{read,write}() into their callers.  These
>> functions were split out in the past because of the 32bit build of Xen, but it
>> is unclear why the cases weren't simply #ifdef'd in place.
>>
>> Next, invert the vmx_write_guest_msr()/is_last_branch_msr() logic to break if
>> the condition is satisfied, rather than nesting if it wasn't.  This allows the
>> wrmsr_hypervisor_regs() call to be un-nested with respect to the other default
>> logic.
>>
>> No practical difference from a guests point of view.
> I think there's a difference from guest PoV now, guest wrmsr of
> GS/FS/LSTAR/CSTAR with non-canonical addresses will get a #GP.
>
>> @@ -3090,6 +3015,45 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>              goto gp_fault;
>>          __vmwrite(GUEST_SYSENTER_EIP, msr_content);
>>          break;
>> +
>> +    case MSR_FS_BASE:
>> +    case MSR_GS_BASE:
>> +    case MSR_SHADOW_GS_BASE:
>> +        if ( !is_canonical_address(msr_content) )
>> +            goto gp_fault;
> This is AFAICT different from previous behaviour, previous code would
> just return X86EMUL_EXCEPTION without injecting a fault. From the SDM
> I see injecting a #GP is the correct behaviour.

The gp_fault label simply returns X86EMUL_EXCEPTION, but is more
descriptive when named like this.

It is up to the top level to convert EXCEPTION into an
hvm_inject_hw_exception() if it wants, because there is at least one
case in the emulator where we need to squash the exception rather than
propagating it.

Previously, non-canonical addresses on the wrmsr side did have their
EXCEPTION propagated up from long_mode_do_msr_write(), so after careful
re-review, I still think the behaviour is the same.

>
>> +
>> +        if ( msr == MSR_FS_BASE )
>> +            __vmwrite(GUEST_FS_BASE, msr_content);
>> +        else if ( msr == MSR_GS_BASE )
>> +            __vmwrite(GUEST_GS_BASE, msr_content);
>> +        else
>> +            wrmsrl(MSR_SHADOW_GS_BASE, msr_content);
>> +
>> +        break;
>> +
>> +    case MSR_STAR:
>> +        v->arch.hvm_vmx.star = msr_content;
>> +        wrmsrl(MSR_STAR, msr_content);
>> +        break;
>> +
>> +    case MSR_LSTAR:
>> +        if ( !is_canonical_address(msr_content) )
>> +            goto gp_fault;
>> +        v->arch.hvm_vmx.lstar = msr_content;
>> +        wrmsrl(MSR_LSTAR, msr_content);
>> +        break;
>> +
>> +    case MSR_CSTAR:
>> +        if ( !is_canonical_address(msr_content) )
>> +            goto gp_fault;
>> +        v->arch.hvm_vmx.cstar = msr_content;
> Likely a stupid question, but why is CSTAR not written here? (like it's
> done for LSTAR)

CSTAR on Intel is a weird corner case.  The MSR exists and can be
interacted with via RDMSR/WRMSR, but SYSCALL fails with #UD outside of
64bit mode, there is nothing in the pipeline which will make use of the
value.

~Andrew

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

Re: [PATCH 1/6] x86/vmx: Simplfy the default cases in vmx_msr_{read, write}_intercept()

[Roger Pau Monné]

On Tue, Feb 27, 2018 at 12:43:21PM +0000, Roger Pau Monné wrote:
> On Mon, Feb 26, 2018 at 05:35:14PM +0000, Andrew Cooper wrote:
> > The default case of vmx_msr_write_intercept() in particular is very tangled.
> > 
> > First of all, fold long_mode_do_msr_{read,write}() into their callers.  These
> > functions were split out in the past because of the 32bit build of Xen, but it
> > is unclear why the cases weren't simply #ifdef'd in place.
> > 
> > Next, invert the vmx_write_guest_msr()/is_last_branch_msr() logic to break if
> > the condition is satisfied, rather than nesting if it wasn't.  This allows the
> > wrmsr_hypervisor_regs() call to be un-nested with respect to the other default
> > logic.
> > 
> > No practical difference from a guests point of view.
> 
> I think there's a difference from guest PoV now, guest wrmsr of
> GS/FS/LSTAR/CSTAR with non-canonical addresses will get a #GP.

Forget about this, I've just realized this is completely wrong.

And I've also figured out my question to CSTAR, so:

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

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

[PATCH 2/6] 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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/hvm/svm/svm.c         |  6 +----
 xen/arch/x86/hvm/viridian.c        | 49 ++++++++++++++++++--------------------
 xen/arch/x86/hvm/vmx/vmx.c         |  6 +----
 xen/arch/x86/msr.c                 | 41 +++++++++++++++++++++++++++----
 xen/include/asm-x86/hvm/viridian.h | 11 ++-------
 5 files changed, 64 insertions(+), 49 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 8b4cefd..6d8ed5c 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1967,8 +1967,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 )
@@ -2123,9 +2122,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..23de433 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -554,13 +554,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 +613,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 +653,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 +697,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 +722,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 +730,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 +754,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 +767,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 +801,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 e1e4f17..5bf6f62 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2871,8 +2871,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 )
@@ -3112,9 +3111,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 7aaa2b0..2ff9361 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -139,9 +139,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 )
     {
@@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
                _MSR_MISC_FEATURES_CPUID_FAULTING;
         break;
 
+    case 0x40000000 ... 0x400001ff:
+        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;
@@ -190,6 +207,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 )
     {
@@ -248,11 +266,26 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
     }
 
+    case 0x40000000 ... 0x400001ff:
+        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);
-- 
2.1.4


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

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

[Boris Ostrovsky]

On 02/26/2018 12:35 PM, Andrew Cooper wrote:
> 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>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



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

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

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, February 27, 2018 1:35 AM
> 
> 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: 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 2/6] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 05:35:15PM +0000, Andrew Cooper wrote:
> 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: Roger Pau Monné <roger.pau@citrix.com>

Just two nits.

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c         |  6 +----
>  xen/arch/x86/hvm/viridian.c        | 49 ++++++++++++++++++--------------------
>  xen/arch/x86/hvm/vmx/vmx.c         |  6 +----
>  xen/arch/x86/msr.c                 | 41 +++++++++++++++++++++++++++----
>  xen/include/asm-x86/hvm/viridian.h | 11 ++-------
>  5 files changed, 64 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 8b4cefd..6d8ed5c 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1967,8 +1967,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 )
> @@ -2123,9 +2122,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..23de433 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -554,13 +554,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)

Since this now returns X86EMUL_* which doesn't have negative value,
should this be unsigned int?

> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 7aaa2b0..2ff9361 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -139,9 +139,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 )
>      {
> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case 0x40000000 ... 0x400001ff:

I think it would be clearer to use VIRIDIAN_MSR_MIN ...
VIRIDIAN_MSR_MAX.

Or else the defines should be removed because you are removing all of
it's users.

Thanks, Roger.

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

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

[Andrew Cooper]

On 27/02/18 14:13, Roger Pau Monné wrote:
> On Mon, Feb 26, 2018 at 05:35:15PM +0000, Andrew Cooper wrote:
>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>          break;
>>  
>> +    case 0x40000000 ... 0x400001ff:
> I think it would be clearer to use VIRIDIAN_MSR_MIN ...
> VIRIDIAN_MSR_MAX.
>
> Or else the defines should be removed because you are removing all of
> it's users.

Both of these would be wrong.  I'll remove them instead.

The problem here, and moreso with the following patch, is that the range
of acceptable MSRs depends on which hypervisor we are emulating.  Using
named labels here actually confuses rather than helps matters.

~Andrew

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

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

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 26 February 2018 17:35
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Jun Nakajima <jun.nakajima@intel.com>; Paul
> Durrant <Paul.Durrant@citrix.com>; Kevin Tian <kevin.tian@intel.com>; Boris
> Ostrovsky <boris.ostrovsky@oracle.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Wei Liu <wei.liu2@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Sergey Dyasli
> <sergey.dyasli@citrix.com>
> Subject: [PATCH 2/6] x86/hvm: Handle viridian MSRs via the new
> guest_{rd,wr}msr() infrastructure
> 
> 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>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c         |  6 +----
>  xen/arch/x86/hvm/viridian.c        | 49 ++++++++++++++++++-------------------
> -
>  xen/arch/x86/hvm/vmx/vmx.c         |  6 +----
>  xen/arch/x86/msr.c                 | 41 +++++++++++++++++++++++++++----
>  xen/include/asm-x86/hvm/viridian.h | 11 ++-------
>  5 files changed, 64 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 8b4cefd..6d8ed5c 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1967,8 +1967,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 )
> @@ -2123,9 +2122,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..23de433 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -554,13 +554,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 +613,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 +653,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 +697,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 +722,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 +730,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 +754,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;

I have a recollection that for at least one version of Windows, when debug mode is enabled, it reads the reference TSC MSR regardless of whether the feature is enabled or not so this change may well cause guest boot failures.
In general I would be wary of #GP faulting where the current code does not. I think the current code is almost certainly too liberal even in the face of buggy versions of Windows but the new code might be too conservative. It will need some testing.

In principle though...

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> +            goto gp_fault;
> 
>          perfc_incr(mshv_rdmsr_tsc_msr);
>          *val = d->arch.hvm_domain.viridian.reference_tsc.raw;
> @@ -770,7 +767,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 +801,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 e1e4f17..5bf6f62 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2871,8 +2871,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 )
> @@ -3112,9 +3111,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 7aaa2b0..2ff9361 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -139,9 +139,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 )
>      {
> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,
> uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
> 
> +    case 0x40000000 ... 0x400001ff:
> +        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;
> @@ -190,6 +207,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 )
>      {
> @@ -248,11 +266,26 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr,
> uint64_t val)
>          break;
>      }
> 
> +    case 0x40000000 ... 0x400001ff:
> +        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);
> --
> 2.1.4

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

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

[Andrew Cooper]

On 27/02/18 14:38, Paul Durrant wrote:
>> @@ -698,13 +697,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 +722,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 +730,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 +754,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;
> I have a recollection that for at least one version of Windows, when debug mode is enabled, it reads the reference TSC MSR regardless of whether the feature is enabled or not so this change may well cause guest boot failures.
> In general I would be wary of #GP faulting where the current code does not. I think the current code is almost certainly too liberal even in the face of buggy versions of Windows but the new code might be too conservative. It will need some testing.
>
> In principle though...
>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

The current code is absolutely wrong, because it falls back into the
default path and continues looking for a result.  On the read side, that
ends up leaking in L$N-1 hypervisor leaves if they are present, while
the write side ends up discarding the result.

ISTR it was only one single pre-release build of windows which failed to
check for the TSC feature, so I'm not sure we need to worry.

If we do find that it is a problem in practice, then the correct course
of action is to explicitly fill with 0 and return X86EMUL_OKAY, which at
least means that we've dealt with the request.

I've booted Win7 and Win10 with the code in this state.  Are you happy
for us to go with this provisionally, and revert back to an explicit
discard if we encounter problems?

~Andrew

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

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

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 28 February 2018 18:22
> To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen-
> devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Jun Nakajima
> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Boris
> Ostrovsky <boris.ostrovsky@oracle.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Wei Liu <wei.liu2@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Sergey Dyasli
> <sergey.dyasli@citrix.com>
> Subject: Re: [PATCH 2/6] x86/hvm: Handle viridian MSRs via the new
> guest_{rd,wr}msr() infrastructure
> 
> On 27/02/18 14:38, Paul Durrant wrote:
> >> @@ -698,13 +697,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 +722,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 +730,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 +754,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;
> > I have a recollection that for at least one version of Windows, when debug
> mode is enabled, it reads the reference TSC MSR regardless of whether the
> feature is enabled or not so this change may well cause guest boot failures.
> > In general I would be wary of #GP faulting where the current code does
> not. I think the current code is almost certainly too liberal even in the face of
> buggy versions of Windows but the new code might be too conservative. It
> will need some testing.
> >
> > In principle though...
> >
> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> 
> The current code is absolutely wrong, because it falls back into the
> default path and continues looking for a result.  On the read side, that
> ends up leaking in L$N-1 hypervisor leaves if they are present, while
> the write side ends up discarding the result.
> 
> ISTR it was only one single pre-release build of windows which failed to
> check for the TSC feature, so I'm not sure we need to worry.
> 

No, ISTR that turning debug mode on in recent Windows kernels makes them assume they have the timer MSR. (The bad breakage in pre-release Windows 8 was that it assumed it had a synIC just because it saw the basic viridian CPUID leaves).

> If we do find that it is a problem in practice, then the correct course
> of action is to explicitly fill with 0 and return X86EMUL_OKAY, which at
> least means that we've dealt with the request.
> 
> I've booted Win7 and Win10 with the code in this state.  Are you happy
> for us to go with this provisionally, and revert back to an explicit
> discard if we encounter problems?
> 

Yes, that's fine. If 7 and 10 are happy (in debug mode) then I'm happy. As you say, we'll just deal with any fallout on a case-by-case basis.

Cheers,

  Paul

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

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

[Jan Beulich]

>>> On 26.02.18 at 18:35, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -554,13 +554,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)

unsigned int would do instead of uint32_t (same on the read side).

> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case 0x40000000 ... 0x400001ff:

As was already suggested, these want to gain #define-s.

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with at least the latter taken care of.

Jan


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

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

[Andrew Cooper]

On 28/02/18 16:40, Jan Beulich wrote:
>>>> On 26.02.18 at 18:35, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/hvm/viridian.c
>> +++ b/xen/arch/x86/hvm/viridian.c
>> @@ -554,13 +554,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)
> unsigned int would do instead of uint32_t (same on the read side).

MSRs are architecturally uint32_t, and your proposed coding style
suggestions would have uint32_t here.  At the moment, uint32_t is used
consistently throughout the new MSR infrastructure.

>
>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  uint64_t *val)
>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>          break;
>>  
>> +    case 0x40000000 ... 0x400001ff:
> As was already suggested, these want to gain #define-s.
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with at least the latter taken care of.

Just like on the CPUID side, the range of valid MSRs depend on the
fallthrough pattern, and which hypervisor(s) we are emulating for.

This is clearer by the end of the subsequent patch, but the logic is far
easier to follow without these numbers being hidden.

~Andrew

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

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

[Jan Beulich]

>>> On 28.02.18 at 19:32, <andrew.cooper3@citrix.com> wrote:
> On 28/02/18 16:40, Jan Beulich wrote:
>>>>> On 26.02.18 at 18:35, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/hvm/viridian.c
>>> +++ b/xen/arch/x86/hvm/viridian.c
>>> @@ -554,13 +554,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)
>> unsigned int would do instead of uint32_t (same on the read side).
> 
> MSRs are architecturally uint32_t, and your proposed coding style
> suggestions would have uint32_t here.  At the moment, uint32_t is used
> consistently throughout the new MSR infrastructure.

I don't really agree: Passing around MSR numbers can be done
using any type at least 32 bits wide. Hence - with our general
assumptions in mind - unsigned int would be suitable (see e.g.
cpuid_count()), as would be uint_least32_t or uint_fast32_t.
IOW as long as we prefer built in types over stdint.h-like ones,
there should really be only extremely few uses of uintNN_t
outside of the public headers and other interface definitions
where "exact width" is what we want.

But as indicated - I'm not going to insist here, as it's clear there
can be quite different views.

>>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  uint64_t *val)
>>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>>          break;
>>>  
>>> +    case 0x40000000 ... 0x400001ff:
>> As was already suggested, these want to gain #define-s.
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> with at least the latter taken care of.
> 
> Just like on the CPUID side, the range of valid MSRs depend on the
> fallthrough pattern, and which hypervisor(s) we are emulating for.
> 
> This is clearer by the end of the subsequent patch, but the logic is far
> easier to follow without these numbers being hidden.

I disagree (it's simply impossible to make the connection between
the read side and the right side this way, because the numbers
could also just happen to be the same, nor is it possible to
reasonably find all uses of those numbers via e.g. grep), but well,
I don't want to block the patch over this.

Jan


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

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

[Andrew Cooper]

On 01/03/18 09:58, Jan Beulich wrote:
>
>>>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  uint64_t *val)
>>>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>>>          break;
>>>>  
>>>> +    case 0x40000000 ... 0x400001ff:
>>> As was already suggested, these want to gain #define-s.
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> with at least the latter taken care of.
>> Just like on the CPUID side, the range of valid MSRs depend on the
>> fallthrough pattern, and which hypervisor(s) we are emulating for.
>>
>> This is clearer by the end of the subsequent patch, but the logic is far
>> easier to follow without these numbers being hidden.
> I disagree (it's simply impossible to make the connection between
> the read side and the right side this way, because the numbers
> could also just happen to be the same, nor is it possible to
> reasonably find all uses of those numbers via e.g. grep), but well,
> I don't want to block the patch over this.

I'm not looking to try and railroad this through.

If you disagree, then what naming would you suggest instead?  I'd be
happy to use any naming which doesn't impede the clarity of the logic,
but I have spent a long time trying to find something suitable, and
failed to do so.  Raw numbers really are clearer to follow than any
naming scheme I tried.  The root of the problem is with the fact that
the MSR spaces overlap, but this information disappears when you try to
put named constants in.  Also notice that the number of CPUID leaves and
the number of MSR leaves have a different stride, which adds more
complexity.

~Andrew

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

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

[Jan Beulich]

>>> On 01.03.18 at 13:29, <andrew.cooper3@citrix.com> wrote:
> On 01/03/18 09:58, Jan Beulich wrote:
>>
>>>>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr,  
> uint64_t *val)
>>>>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>>>>          break;
>>>>>  
>>>>> +    case 0x40000000 ... 0x400001ff:
>>>> As was already suggested, these want to gain #define-s.
>>>>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> with at least the latter taken care of.
>>> Just like on the CPUID side, the range of valid MSRs depend on the
>>> fallthrough pattern, and which hypervisor(s) we are emulating for.
>>>
>>> This is clearer by the end of the subsequent patch, but the logic is far
>>> easier to follow without these numbers being hidden.
>> I disagree (it's simply impossible to make the connection between
>> the read side and the right side this way, because the numbers
>> could also just happen to be the same, nor is it possible to
>> reasonably find all uses of those numbers via e.g. grep), but well,
>> I don't want to block the patch over this.
> 
> I'm not looking to try and railroad this through.
> 
> If you disagree, then what naming would you suggest instead?  I'd be
> happy to use any naming which doesn't impede the clarity of the logic,
> but I have spent a long time trying to find something suitable, and
> failed to do so.  Raw numbers really are clearer to follow than any
> naming scheme I tried.  The root of the problem is with the fact that
> the MSR spaces overlap, but this information disappears when you try to
> put named constants in.  Also notice that the number of CPUID leaves and
> the number of MSR leaves have a different stride, which adds more
> complexity.

The Viridian MSR range is fixed aiui, so any simple naming scheme
could be used in this patch (as e.g. suggested by Roger, to be lifted
from viridian.c). For the conflicting hypervisor range, how about

#define XEN_MSR_BASE(stride) (0x40000000 + 0x200 * (stride))

? We'll anyway want to declare some upper bound on this range,
which would then be used to describe the upper end in case
labels.

Jan


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

[PATCH 3/6] 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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 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            | 33 ++++++++++++++++-----------------
 xen/include/asm-x86/processor.h |  4 ++--
 6 files changed, 34 insertions(+), 66 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 6d8ed5c..f90a7b4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1967,9 +1967,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;
 
@@ -2122,25 +2119,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 5bf6f62..6caaabc 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2871,9 +2871,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;
 
@@ -3115,24 +3112,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 2ff9361..9f20fd8 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -183,6 +183,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         }
 
         /* Fallthrough. */
+    case 0x40000200 ... 0x400002ff:
+        ret = guest_rdmsr_xen(v, msr, val);
+        goto out;
+
     default:
         return X86EMUL_UNHANDLEABLE;
     }
@@ -274,6 +278,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         }
 
         /* Fallthrough. */
+    case 0x40000200 ... 0x400002ff:
+        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 17aaf97..97fcac0 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 27190e0..cd5b223 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -776,29 +776,26 @@ 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;
+    default:
+        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;
 
@@ -817,7 +814,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);
@@ -830,13 +827,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);
@@ -844,11 +841,13 @@ 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:
+    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 9c70a98..14df6bd 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -521,8 +521,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 3/6] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

[Boris Ostrovsky]

On 02/26/2018 12:35 PM, Andrew Cooper wrote:
> 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>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  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            | 33 ++++++++++++++++-----------------
>  xen/include/asm-x86/processor.h |  4 ++--
>  6 files changed, 34 insertions(+), 66 deletions(-)


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

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

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

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Tuesday, February 27, 2018 1:35 AM
> 
> 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: 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 3/6] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 05:35:16PM +0000, Andrew Cooper wrote:
> 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: Roger Pau Monné <roger.pau@citrix.com>

> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 2ff9361..9f20fd8 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -183,6 +183,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          }
>  
>          /* Fallthrough. */
> +    case 0x40000200 ... 0x400002ff:
> +        ret = guest_rdmsr_xen(v, msr, val);
> +        goto out;
> +
>      default:
>          return X86EMUL_UNHANDLEABLE;
>      }
> @@ -274,6 +278,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>          }
>  
>          /* Fallthrough. */
> +    case 0x40000200 ... 0x400002ff:

A define would be better in order to prevent this two values from
getting out of sync with the ones in rdmsr maybe.

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 26.02.18 at 18:35, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -183,6 +183,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          }
>  
>          /* Fallthrough. */
> +    case 0x40000200 ... 0x400002ff:

These again want to have #define-s added.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -776,29 +776,26 @@ 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;
> +    default:
> +        return X86EMUL_EXCEPTION;
> +    }
>  }

Could I talk you into adjusting the code to have a "return" at the
end of the function, e.g. by dropping the default case? Also on
the write path then?

With at least the #define-s added,
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 4/6] x86/hvm: Constify the read side of vlapic handling

[Andrew Cooper]

This is in preparation to make hvm_x2apic_msr_read() take a const vcpu
pointer.  One modification is to alter vlapic_get_tmcct() to not use current.

This in turn needs an alteration to hvm_get_guest_time_fixed(), which is safe
because the only mutable action it makes is to take the domain plt lock.

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>
---
 xen/arch/x86/hvm/vlapic.c     | 13 +++++++------
 xen/arch/x86/hvm/vpt.c        |  2 +-
 xen/include/asm-x86/hvm/hvm.h |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 7387f91..e715729 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -171,12 +171,12 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
         vcpu_kick(target);
 }
 
-static int vlapic_find_highest_isr(struct vlapic *vlapic)
+static int vlapic_find_highest_isr(const struct vlapic *vlapic)
 {
     return vlapic_find_highest_vector(&vlapic->regs->data[APIC_ISR]);
 }
 
-static uint32_t vlapic_get_ppr(struct vlapic *vlapic)
+static uint32_t vlapic_get_ppr(const struct vlapic *vlapic)
 {
     uint32_t tpr, isrv, ppr;
     int isr;
@@ -550,9 +550,9 @@ void vlapic_ipi(
     }
 }
 
-static uint32_t vlapic_get_tmcct(struct vlapic *vlapic)
+static uint32_t vlapic_get_tmcct(const struct vlapic *vlapic)
 {
-    struct vcpu *v = current;
+    const struct vcpu *v = const_vlapic_vcpu(vlapic);
     uint32_t tmcct = 0, tmict = vlapic_get_reg(vlapic, APIC_TMICT);
     uint64_t counter_passed;
 
@@ -590,7 +590,8 @@ static void vlapic_set_tdcr(struct vlapic *vlapic, unsigned int val)
                 "timer_divisor: %d", vlapic->hw.timer_divisor);
 }
 
-static uint32_t vlapic_read_aligned(struct vlapic *vlapic, unsigned int offset)
+static uint32_t vlapic_read_aligned(const struct vlapic *vlapic,
+                                    unsigned int offset)
 {
     switch ( offset )
     {
@@ -680,7 +681,7 @@ int hvm_x2apic_msr_read(struct vcpu *v, unsigned int msr, uint64_t *msr_content)
             REGBLOCK(ISR) | REGBLOCK(TMR) | REGBLOCK(IRR)
 #undef REGBLOCK
         };
-    struct vlapic *vlapic = vcpu_vlapic(v);
+    const struct vlapic *vlapic = vcpu_vlapic(v);
     uint32_t high = 0, reg = msr - MSR_IA32_APICBASE_MSR, offset = reg << 4;
 
     if ( !vlapic_x2apic_mode(vlapic) ||
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c
index 181f4cb..862c715 100644
--- a/xen/arch/x86/hvm/vpt.c
+++ b/xen/arch/x86/hvm/vpt.c
@@ -35,7 +35,7 @@ void hvm_init_guest_time(struct domain *d)
     pl->last_guest_time = 0;
 }
 
-u64 hvm_get_guest_time_fixed(struct vcpu *v, u64 at_tsc)
+u64 hvm_get_guest_time_fixed(const struct vcpu *v, u64 at_tsc)
 {
     struct pl_time *pl = v->domain->arch.hvm_domain.pl_time;
     u64 now;
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index dd3dd5f..031af12 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -269,7 +269,7 @@ u64 hvm_get_tsc_scaling_ratio(u32 gtsc_khz);
 
 void hvm_init_guest_time(struct domain *d);
 void hvm_set_guest_time(struct vcpu *v, u64 guest_time);
-u64 hvm_get_guest_time_fixed(struct vcpu *v, u64 at_tsc);
+u64 hvm_get_guest_time_fixed(const struct vcpu *v, u64 at_tsc);
 #define hvm_get_guest_time(v) hvm_get_guest_time_fixed(v, 0)
 
 int vmsi_deliver(
-- 
2.1.4


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

Re: [PATCH 4/6] x86/hvm: Constify the read side of vlapic handling

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 05:35:17PM +0000, Andrew Cooper wrote:
> This is in preparation to make hvm_x2apic_msr_read() take a const vcpu
> pointer.  One modification is to alter vlapic_get_tmcct() to not use current.
> 
> This in turn needs an alteration to hvm_get_guest_time_fixed(), which is safe
> because the only mutable action it makes is to take the domain plt lock.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> index 181f4cb..862c715 100644
> --- a/xen/arch/x86/hvm/vpt.c
> +++ b/xen/arch/x86/hvm/vpt.c
> @@ -35,7 +35,7 @@ void hvm_init_guest_time(struct domain *d)
>      pl->last_guest_time = 0;
>  }
>  
> -u64 hvm_get_guest_time_fixed(struct vcpu *v, u64 at_tsc)
> +u64 hvm_get_guest_time_fixed(const struct vcpu *v, u64 at_tsc)

While there you could s/u64/uint64_t/.

Thanks, Roger.

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

Re: [PATCH 4/6] x86/hvm: Constify the read side of vlapic handling

[Jan Beulich]

>>> On 26.02.18 at 18:35, <andrew.cooper3@citrix.com> wrote:
> This is in preparation to make hvm_x2apic_msr_read() take a const vcpu
> pointer.  One modification is to alter vlapic_get_tmcct() to not use 
> current.
> 
> This in turn needs an alteration to hvm_get_guest_time_fixed(), which is safe
> because the only mutable action it makes is to take the domain plt lock.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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



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

[PATCH 5/6] x86/hvm: Handle x2apic MSRs 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 plausible 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>
---
 xen/arch/x86/hvm/hvm.c        | 10 -------
 xen/arch/x86/hvm/vlapic.c     | 61 ++++++++++++++++++++++++-------------------
 xen/arch/x86/msr.c            | 15 +++++++++++
 xen/include/asm-x86/hvm/hvm.h |  4 +--
 4 files changed, 51 insertions(+), 39 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 91bc3e8..b10e05f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3432,11 +3432,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_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
-        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;
@@ -3591,11 +3586,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_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
-        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 e715729..74bbfd5 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -666,33 +666,32 @@ 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_IA32_APICBASE_MSR, offset = reg << 4;
+    uint64_t high = 0;
+    uint32_t reg = msr - MSR_IA32_APICBASE_MSR, offset = reg << 4;
 
     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 +982,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_IA32_APICBASE_MSR) << 4;
 
+    /* The timer handling at least is unsafe outside of current context. */
+    ASSERT(v == current);
+
     if ( !vlapic_x2apic_mode(vlapic) )
-        return X86EMUL_UNHANDLEABLE;
+        goto gp_fault;
 
     switch ( offset )
     {
     case APIC_TASKPRI:
         if ( msr_content & ~APIC_TPRI_MASK )
-            return X86EMUL_UNHANDLEABLE;
+            goto gp_fault;
         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;
+            goto gp_fault;
         break;
 
     case APIC_LVTT:
         if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
-            return X86EMUL_UNHANDLEABLE;
+            goto gp_fault;
         break;
 
     case APIC_LVTTHMR:
     case APIC_LVTPC:
     case APIC_CMCI:
         if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
-            return X86EMUL_UNHANDLEABLE;
+            goto gp_fault;
         break;
 
     case APIC_LVT0:
     case APIC_LVT1:
         if ( msr_content & ~LINT_MASK )
-            return X86EMUL_UNHANDLEABLE;
+            goto gp_fault;
         break;
 
     case APIC_LVTERR:
         if ( msr_content & ~LVT_MASK )
-            return X86EMUL_UNHANDLEABLE;
+            goto gp_fault;
         break;
 
     case APIC_TMICT:
@@ -1033,20 +1035,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;
+            goto gp_fault;
         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;
+            goto gp_fault;
         vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
         break;
 
     case APIC_SELF_IPI:
         if ( msr_content & ~APIC_VECTOR_MASK )
-            return X86EMUL_UNHANDLEABLE;
+            goto gp_fault;
         offset = APIC_ICR;
         msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
         break;
@@ -1054,13 +1056,18 @@ 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;
+            goto gp_fault;
+        }
     }
 
     vlapic_reg_write(v, offset, msr_content);
 
     return X86EMUL_OKAY;
+
+ gp_fault:
+    return X86EMUL_EXCEPTION;
 }
 
 static int vlapic_range(struct vcpu *v, unsigned long addr)
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9f20fd8..3cb4158 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -139,6 +139,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;
@@ -175,6 +176,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
                _MSR_MISC_FEATURES_CPUID_FAULTING;
         break;
 
+    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
+        if ( !is_hvm_domain(d) || v != curr )
+            goto gp_fault;
+
+        ret = guest_rdmsr_x2apic(v, msr, val);
+        goto out;
+
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
@@ -270,6 +278,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         break;
     }
 
+    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
+        if ( !is_hvm_domain(d) || v != curr )
+            goto gp_fault;
+
+        ret = guest_wrmsr_x2apic(v, msr, val);
+        goto out;
+
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 031af12..a19b6ee 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -517,8 +517,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 5/6] x86/hvm: Handle x2apic MSRs the new guest_{rd, wr}msr() infrastructure

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 05:35:18PM +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 plausible 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>

> @@ -1054,13 +1056,18 @@ 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;

Why not add a gp_fault label here and just return X86EMUL_EXCEPTION?
That seems better than adding a gp_fault label below.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -139,6 +139,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;
> @@ -175,6 +176,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
> +        if ( !is_hvm_domain(d) || v != curr )
> +            goto gp_fault;
> +
> +        ret = guest_rdmsr_x2apic(v, msr, val);
> +        goto out;
> +
>      case 0x40000000 ... 0x400001ff:
>          if ( is_viridian_domain(d) )
>          {
> @@ -270,6 +278,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>          break;
>      }
>  
> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
> +        if ( !is_hvm_domain(d) || v != curr )
> +            goto gp_fault;
> +
> +        ret = guest_wrmsr_x2apic(v, msr, val);
> +        goto out;

AFAICT you could just use a break here?

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 26.02.18 at 18:35, <andrew.cooper3@citrix.com> wrote:

"in" missing from subject?

> 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 plausible reason to access the APIC registers via this interface

... has no plausible reason ... ?

> @@ -983,49 +982,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_IA32_APICBASE_MSR) << 4;
>  
> +    /* The timer handling at least is unsafe outside of current context. */
> +    ASSERT(v == current);
> +
>      if ( !vlapic_x2apic_mode(vlapic) )
> -        return X86EMUL_UNHANDLEABLE;
> +        goto gp_fault;
>  
>      switch ( offset )
>      {
>      case APIC_TASKPRI:
>          if ( msr_content & ~APIC_TPRI_MASK )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          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;
> +            goto gp_fault;
>          break;
>  
>      case APIC_LVTT:
>          if ( msr_content & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_LVTTHMR:
>      case APIC_LVTPC:
>      case APIC_CMCI:
>          if ( msr_content & ~(LVT_MASK | APIC_MODE_MASK) )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_LVT0:
>      case APIC_LVT1:
>          if ( msr_content & ~LINT_MASK )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_LVTERR:
>          if ( msr_content & ~LVT_MASK )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          break;
>  
>      case APIC_TMICT:
> @@ -1033,20 +1035,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;
> +            goto gp_fault;
>          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;
> +            goto gp_fault;
>          vlapic_set_reg(vlapic, APIC_ICR2, msr_content >> 32);
>          break;
>  
>      case APIC_SELF_IPI:
>          if ( msr_content & ~APIC_VECTOR_MASK )
> -            return X86EMUL_UNHANDLEABLE;
> +            goto gp_fault;
>          offset = APIC_ICR;
>          msr_content = APIC_DEST_SELF | (msr_content & APIC_VECTOR_MASK);
>          break;
> @@ -1054,13 +1056,18 @@ 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;
> +            goto gp_fault;
> +        }
>      }
>  
>      vlapic_reg_write(v, offset, msr_content);
>  
>      return X86EMUL_OKAY;
> +
> + gp_fault:
> +    return X86EMUL_EXCEPTION;
>  }

There's really no good reason to use goto all over the place here
when all that's at the label is a single return statement. I was
actually hoping to do conversions the other way around in some
other code.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -139,6 +139,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;
> @@ -175,6 +176,13 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>          break;
>  
> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3ff:
> +        if ( !is_hvm_domain(d) || v != curr )
> +            goto gp_fault;

"v != curr" is quite certainly not a reason to raise #GP.

Jan

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

[PATCH 6/6] 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>
---
 xen/arch/x86/msr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 3cb4158..0237637 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -158,6 +158,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         *val = vp->spec_ctrl.raw;
         break;
 
+    case 0x8c ... 0x8f: /* SGX Launch Enclave Public Key Hash. */
+        /* Not implemented yet. */
+        goto gp_fault;
+
     case MSR_INTEL_PLATFORM_INFO:
         if ( !dp->plaform_info.available )
             goto gp_fault;
@@ -183,6 +187,15 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         ret = guest_rdmsr_x2apic(v, msr, val);
         goto out;
 
+    case 0xc80:
+        /* Silicon Debug Inferface not advertised to guests. */
+        goto gp_fault;
+
+    case 0xc81 ... 0xc8f: /* Misc RDT MSRs. */
+    case 0xc90 ... 0xd8f: /* CAT Mask registers. */
+        /* Not implemented yet. */
+        goto gp_fault;
+
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
@@ -196,6 +209,15 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         goto out;
 
     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;
     }
 
@@ -255,6 +277,10 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
             wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
         break;
 
+    case 0x8c ... 0x8f: /* SGX Launch Enclave Public Key Hash. */
+        /* Not implemented yet. */
+        goto gp_fault;
+
     case MSR_INTEL_MISC_FEATURES_ENABLES:
     {
         uint64_t rsvd = ~0ull;
@@ -285,6 +311,15 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         ret = guest_wrmsr_x2apic(v, msr, val);
         goto out;
 
+    case 0xc80:
+        /* Silicon Debug Inferface not advertised to guests. */
+        goto gp_fault;
+
+    case 0xc81 ... 0xc8f: /* Misc RDT MSRs. */
+    case 0xc90 ... 0xd8f: /* CAT Mask registers. */
+        /* Not implemented yet. */
+        goto gp_fault;
+
     case 0x40000000 ... 0x400001ff:
         if ( is_viridian_domain(d) )
         {
@@ -298,6 +333,15 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         goto out;
 
     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;
     }
 
-- 
2.1.4


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

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

[Roger Pau Monné]

On Mon, Feb 26, 2018 at 05:35:19PM +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.
> 
> 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>
> ---
>  xen/arch/x86/msr.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 3cb4158..0237637 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -158,6 +158,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          *val = vp->spec_ctrl.raw;
>          break;
>  
> +    case 0x8c ... 0x8f: /* SGX Launch Enclave Public Key Hash. */
> +        /* Not implemented yet. */
> +        goto gp_fault;
> +
>      case MSR_INTEL_PLATFORM_INFO:
>          if ( !dp->plaform_info.available )
>              goto gp_fault;
> @@ -183,6 +187,15 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          ret = guest_rdmsr_x2apic(v, msr, val);
>          goto out;
>  
> +    case 0xc80:
> +        /* Silicon Debug Inferface not advertised to guests. */
> +        goto gp_fault;
> +
> +    case 0xc81 ... 0xc8f: /* Misc RDT MSRs. */
> +    case 0xc90 ... 0xd8f: /* CAT Mask registers. */
> +        /* Not implemented yet. */
> +        goto gp_fault;

You have sorted them by index, I would maybe group them together in
order to use a single goto.

I would however prefer to have defines for those values.

> +
>      case 0x40000000 ... 0x400001ff:
>          if ( is_viridian_domain(d) )
>          {
> @@ -196,6 +209,15 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          goto out;
>  
>      default:
> +        /*
> +         * Blacklist the architecturally inaccessable MSRs. No point wandering
> +         * the legacy handlers.
> +         */
> +        if ( msr > 0x1fff &&
> +             (msr < 0xc0000000 || msr > 0xc0001fff) &&
> +             (msr < 0xc0010000 || msr > 0xc0011fff) )

Maybe this could be a macro in order to avoid duplication with the
chunk in wrmsr? (MSR_INACCESSIBLE or some such).

Thanks, Roger.

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

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

[Jan Beulich]

>>> On 26.02.18 at 18:35, <andrew.cooper3@citrix.com> wrote:
> @@ -183,6 +187,15 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>          ret = guest_rdmsr_x2apic(v, msr, val);
>          goto out;
>  
> +    case 0xc80:
> +        /* Silicon Debug Inferface not advertised to guests. */
> +        goto gp_fault;
> +
> +    case 0xc81 ... 0xc8f: /* Misc RDT MSRs. */
> +    case 0xc90 ... 0xd8f: /* CAT Mask registers. */

At the very least where we already have #define-s you should use
those. Even better would be if you also introduced #define-s for
ones we don't handle anywhere yet, as Roger has also suggested.

Furthermore, what makes you imply the entire c81..c8f range is
RDT?

And then - even calling them RDT / CAT is questionable - the same
numbers have another use on the Xeon 7500 series as per the SDM.

Jan


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