[PATCH 1/2] x86/vmx: Don't use hvm_inject_hw_exception() in long_mode_do_msr_write()

[PATCH 1/2] x86/vmx: Don't use hvm_inject_hw_exception() in long_mode_do_msr_write()

[Andrew Cooper]

Since c/s 49de10f3c1718 "x86/hvm: Don't raise #GP behind the emulators back
for MSR accesses", returnning X86EMUL_EXCEPTION has pushed the exception
generation to the top of the call tree.

Using hvm_inject_hw_exception() and returning X86EMUL_EXCEPTION causes a
double #GP injection, which combines to #DF.

While fixing this up, rename uncanonical_address to the more common gp_fault,
and drop the HVM_DBG_LOG() line which is redundant given the two adjacent
lines.

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>

This wants backporting to 4.9
---
 xen/arch/x86/hvm/vmx/vmx.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b18ccea..426902b 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -542,7 +542,7 @@ long_mode_do_msr_write(unsigned int msr, uint64_t msr_content)
     case MSR_GS_BASE:
     case MSR_SHADOW_GS_BASE:
         if ( !is_canonical_address(msr_content) )
-            goto uncanonical_address;
+            goto gp_fault;
 
         if ( msr == MSR_FS_BASE )
             __vmwrite(GUEST_FS_BASE, msr_content);
@@ -560,14 +560,14 @@ long_mode_do_msr_write(unsigned int msr, uint64_t msr_content)
 
     case MSR_LSTAR:
         if ( !is_canonical_address(msr_content) )
-            goto uncanonical_address;
+            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 uncanonical_address;
+            goto gp_fault;
         v->arch.hvm_vmx.cstar = msr_content;
         break;
 
@@ -582,9 +582,7 @@ long_mode_do_msr_write(unsigned int msr, uint64_t msr_content)
 
     return HNDL_done;
 
- uncanonical_address:
-    HVM_DBG_LOG(DBG_LEVEL_MSR, "Not cano address of msr write %x", msr);
-    hvm_inject_hw_exception(TRAP_gp_fault, 0);
+ gp_fault:
     return HNDL_exception_raised;
 }
 
-- 
2.1.4


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

[PATCH 2/2] x86/vmx: Drop enum handler_return

[Andrew Cooper]

They are straight aliases of the more common X86EMUL_* constants.  While
adjusting these, fix the case indentation where appropriate.

No functional change, confirmed by diff'ing the compiled binary.

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>
---
 xen/arch/x86/hvm/vmx/vmx.c | 66 ++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 35 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 426902b..ea98a4e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -62,8 +62,6 @@
 static bool_t __initdata opt_force_ept;
 boolean_param("force-ept", opt_force_ept);
 
-enum handler_return { HNDL_done, HNDL_unhandled, HNDL_exception_raised };
-
 static void vmx_ctxt_switch_from(struct vcpu *v);
 static void vmx_ctxt_switch_to(struct vcpu *v);
 
@@ -485,8 +483,7 @@ static void vmx_vcpu_destroy(struct vcpu *v)
     passive_domain_destroy(v);
 }
 
-static enum handler_return
-long_mode_do_msr_read(unsigned int msr, uint64_t *msr_content)
+static int long_mode_do_msr_read(unsigned int msr, uint64_t *msr_content)
 {
     struct vcpu *v = current;
 
@@ -521,16 +518,15 @@ long_mode_do_msr_read(unsigned int msr, uint64_t *msr_content)
         break;
 
     default:
-        return HNDL_unhandled;
+        return X86EMUL_UNHANDLEABLE;
     }
 
     HVM_DBG_LOG(DBG_LEVEL_MSR, "msr %#x content %#"PRIx64, msr, *msr_content);
 
-    return HNDL_done;
+    return X86EMUL_OKAY;
 }
 
-static enum handler_return
-long_mode_do_msr_write(unsigned int msr, uint64_t msr_content)
+static int long_mode_do_msr_write(unsigned int msr, uint64_t msr_content)
 {
     struct vcpu *v = current;
 
@@ -577,13 +573,13 @@ long_mode_do_msr_write(unsigned int msr, uint64_t msr_content)
         break;
 
     default:
-        return HNDL_unhandled;
+        return X86EMUL_UNHANDLEABLE;
     }
 
-    return HNDL_done;
+    return X86EMUL_OKAY;
 
  gp_fault:
-    return HNDL_exception_raised;
+    return X86EMUL_EXCEPTION;
 }
 
 /*
@@ -2937,12 +2933,12 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
             goto done;
         switch ( long_mode_do_msr_read(msr, msr_content) )
         {
-            case HNDL_unhandled:
-                break;
-            case HNDL_exception_raised:
-                return X86EMUL_EXCEPTION;
-            case HNDL_done:
-                goto done;
+        case X86EMUL_UNHANDLEABLE:
+            break;
+        case X86EMUL_EXCEPTION:
+            return X86EMUL_EXCEPTION;
+        case X86EMUL_OKAY:
+            goto done;
         }
 
         if ( vmx_read_guest_msr(msr, msr_content) == 0 )
@@ -3161,24 +3157,24 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
 
         switch ( long_mode_do_msr_write(msr, msr_content) )
         {
-            case HNDL_unhandled:
-                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:
-                    case 1:
-                        break;
-                    default:
-                        goto gp_fault;
-                    }
-                break;
-            case HNDL_exception_raised:
-                return X86EMUL_EXCEPTION;
-            case HNDL_done:
-                break;
+        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:
+                case 1:
+                    break;
+                default:
+                    goto gp_fault;
+                }
+            break;
+        case X86EMUL_EXCEPTION:
+            return X86EMUL_EXCEPTION;
+        case X86EMUL_OKAY:
+            break;
         }
         break;
     }
-- 
2.1.4


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

Re: [PATCH 1/2] x86/vmx: Don't use hvm_inject_hw_exception() in long_mode_do_msr_write()

[Jan Beulich]

>>> On 06.12.17 at 21:06, <andrew.cooper3@citrix.com> wrote:
> Since c/s 49de10f3c1718 "x86/hvm: Don't raise #GP behind the emulators back
> for MSR accesses", returnning X86EMUL_EXCEPTION has pushed the exception
> generation to the top of the call tree.
> 
> Using hvm_inject_hw_exception() and returning X86EMUL_EXCEPTION causes a
> double #GP injection, which combines to #DF.
> 
> While fixing this up, rename uncanonical_address to the more common gp_fault,

It would seem even better to drop that label altogether, not that
this is being followed by a single "return".

> and drop the HVM_DBG_LOG() line which is redundant given the two adjacent
> lines.

Two adjacent lines? Not in source code, and in terms of resulting log
lines I can see coming from the start of long_mode_do_msr_write(),
but I can#t seem to be able to spot a second (presumably later) one.

Jan


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

Re: [PATCH 2/2] x86/vmx: Drop enum handler_return

[Jan Beulich]

>>> On 06.12.17 at 21:06, <andrew.cooper3@citrix.com> wrote:
> @@ -2937,12 +2933,12 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>              goto done;
>          switch ( long_mode_do_msr_read(msr, msr_content) )
>          {
> -            case HNDL_unhandled:
> -                break;
> -            case HNDL_exception_raised:
> -                return X86EMUL_EXCEPTION;
> -            case HNDL_done:
> -                goto done;
> +        case X86EMUL_UNHANDLEABLE:
> +            break;
> +        case X86EMUL_EXCEPTION:
> +            return X86EMUL_EXCEPTION;
> +        case X86EMUL_OKAY:
> +            goto done;
>          }

This not being an exhaustive enumeration of all X86EMUL_* values
I don't see the point in the X86EMUL_UNHANDLEABLE case label.

> @@ -3161,24 +3157,24 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>  
>          switch ( long_mode_do_msr_write(msr, msr_content) )
>          {
> -            case HNDL_unhandled:
> -                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:
> -                    case 1:
> -                        break;
> -                    default:
> -                        goto gp_fault;
> -                    }
> -                break;
> -            case HNDL_exception_raised:
> -                return X86EMUL_EXCEPTION;
> -            case HNDL_done:
> -                break;
> +        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:
> +                case 1:
> +                    break;
> +                default:
> +                    goto gp_fault;
> +                }
> +            break;
> +        case X86EMUL_EXCEPTION:
> +            return X86EMUL_EXCEPTION;
> +        case X86EMUL_OKAY:
> +            break;
>          }

Same here for the X86EMUL_OKAY one, except the question is
whether there wouldn't better be a default invoking
ASSERT_UNREACHABLE().

Jan


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

Re: [PATCH 1/2] x86/vmx: Don't use hvm_inject_hw_exception() in long_mode_do_msr_write()

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, December 7, 2017 4:06 AM
> 
> Since c/s 49de10f3c1718 "x86/hvm: Don't raise #GP behind the emulators
> back
> for MSR accesses", returnning X86EMUL_EXCEPTION has pushed the
> exception
> generation to the top of the call tree.
> 
> Using hvm_inject_hw_exception() and returning X86EMUL_EXCEPTION
> causes a
> double #GP injection, which combines to #DF.
> 
> While fixing this up, rename uncanonical_address to the more common
> gp_fault,
> and drop the HVM_DBG_LOG() line which is redundant given the two
> adjacent
> lines.
> 
> 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 2/2] x86/vmx: Drop enum handler_return

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Thursday, December 7, 2017 4:06 AM
> 
> They are straight aliases of the more common X86EMUL_* constants.
> While
> adjusting these, fix the case indentation where appropriate.
> 
> No functional change, confirmed by diff'ing the compiled binary.
> 
> 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