thr3ads.net - Xen devel - [Xen-devel] A simple clean up of __get_instruction_length & __update_guest

If this information is useful, please help other people find it:
Share via:

Dong, Eddie

2010-Sep-15 14:34 UTC

[Xen-devel] A simple clean up of __get_instruction_length & __update_guest_eip

Replace so many sparsed __get_instruction_length & __update_guest_eip with
one function call.

Signed-off-by: Eddie Dong <eddie.dong@intel.com>

diff -r 221cf46cbf2c xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Wed Sep 15 17:31:53 2010 +0800
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed Sep 15 17:46:24 2010 +0800
@@ -1494,6 +1494,14 @@
         vmx_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
 }
 
+static void update_guest_eip(void)
+{
+    unsigned long inst_len;
+
+    inst_len = __get_instruction_length();
+    __update_guest_eip(inst_len);
+}
+
 static void vmx_fpu_dirty_intercept(void)
 {
     struct vcpu *curr = current;
@@ -2244,8 +2252,7 @@
     if ( (((exit_qualification >> 12) & 0xf) == 1) &&
          ((exit_qualification & 0xfff) == APIC_EOI) )
     {
-        int inst_len = __get_instruction_length(); /* Safe: APIC data write */
-        __update_guest_eip(inst_len);
+        update_guest_eip();
         vlapic_EOI_set(vcpu_vlapic(current));
         return 1;
     }
@@ -2424,8 +2431,7 @@
         case TRAP_int3:
             if ( !v->domain->debugger_attached )
                 goto exit_and_crash;
-            inst_len = __get_instruction_length(); /* Safe: INT3 */
-            __update_guest_eip(inst_len);
+            update_guest_eip();
             current->arch.gdbsx_vcpu_event = TRAP_int3;
             domain_pause_for_debugger();
             break;
@@ -2516,18 +2522,15 @@
         break;
     }
     case EXIT_REASON_CPUID:
-        inst_len = __get_instruction_length(); /* Safe: CPUID */
-        __update_guest_eip(inst_len);
+        update_guest_eip();
         vmx_do_cpuid(regs);
         break;
     case EXIT_REASON_HLT:
-        inst_len = __get_instruction_length(); /* Safe: HLT */
-        __update_guest_eip(inst_len);
+        update_guest_eip();
         hvm_hlt(regs->eflags);
         break;
     case EXIT_REASON_INVLPG:
-        inst_len = __get_instruction_length(); /* Safe: INVLPG */
-        __update_guest_eip(inst_len);
+        update_guest_eip();
         exit_qualification = __vmread(EXIT_QUALIFICATION);
         vmx_invlpg_intercept(exit_qualification);
         break;
@@ -2535,19 +2538,17 @@
         regs->ecx = hvm_msr_tsc_aux(v);
         /* fall through */
     case EXIT_REASON_RDTSC:
-        inst_len = __get_instruction_length();
-        __update_guest_eip(inst_len);
+        update_guest_eip();
         hvm_rdtsc_intercept(regs);
         break;
     case EXIT_REASON_VMCALL:
     {
         int rc;
         HVMTRACE_1D(VMMCALL, regs->eax);
-        inst_len = __get_instruction_length(); /* Safe: VMCALL */
         rc = hvm_do_hypercall(regs);
         if ( rc != HVM_HCALL_preempted )
         {
-            __update_guest_eip(inst_len);
+            update_guest_eip();
             if ( rc == HVM_HCALL_invalidate )
                 send_invalidate_req();
         }
@@ -2556,9 +2557,8 @@
     case EXIT_REASON_CR_ACCESS:
     {
         exit_qualification = __vmread(EXIT_QUALIFICATION);
-        inst_len = __get_instruction_length(); /* Safe: MOV Cn, LMSW, CLTS */
         if ( vmx_cr_access(exit_qualification, regs) )
-            __update_guest_eip(inst_len);
+            update_guest_eip();
         break;
     }
     case EXIT_REASON_DR_ACCESS:
@@ -2568,22 +2568,20 @@
     case EXIT_REASON_MSR_READ:
     {
         uint64_t msr_content;
-        inst_len = __get_instruction_length(); /* Safe: RDMSR */
         if ( hvm_msr_read_intercept(regs->ecx, &msr_content) ==
X86EMUL_OKAY )
         {
             regs->eax = (uint32_t)msr_content;
             regs->edx = (uint32_t)(msr_content >> 32);
-            __update_guest_eip(inst_len);
+            update_guest_eip();
         }
         break;
     }
     case EXIT_REASON_MSR_WRITE:
     {
         uint64_t msr_content;
-        inst_len = __get_instruction_length(); /* Safe: WRMSR */
         msr_content = ((uint64_t)regs->edx << 32) |
(uint32_t)regs->eax;
         if ( hvm_msr_write_intercept(regs->ecx, msr_content) == X86EMUL_OKAY
)
-            __update_guest_eip(inst_len);
+            update_guest_eip();
         break;
     }
 
@@ -2652,8 +2650,7 @@
     case EXIT_REASON_INVD:
     case EXIT_REASON_WBINVD:
     {
-        inst_len = __get_instruction_length(); /* Safe: INVD, WBINVD */
-        __update_guest_eip(inst_len);
+        update_guest_eip();
         vmx_wbinvd_intercept();
         break;
     }
@@ -2686,8 +2683,7 @@
         u64 new_bv  =  (((u64)regs->edx) << 32) | regs->eax;
         if ( vmx_handle_xsetbv(new_bv) == 0 )
         {
-            inst_len = __get_instruction_length();
-            __update_guest_eip(inst_len);
+            update_guest_eip();
         }
         break;
     }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2010-Sep-15 15:25 UTC

head link

Re: [Xen-devel] A simple clean up of __get_instruction_length & __update_guest_eip

>>> On 15.09.10 at 16:34, "Dong, Eddie"
<eddie.dong@intel.com> wrote:
> Replace so many sparsed __get_instruction_length & __update_guest_eip
with
> one function call.
> 
> Signed-off-by: Eddie Dong <eddie.dong@intel.com>
> 
> diff -r 221cf46cbf2c xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c	Wed Sep 15 17:31:53 2010 +0800
> +++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed Sep 15 17:46:24 2010 +0800
> @@ -1494,6 +1494,14 @@
>          vmx_inject_hw_exception(TRAP_debug, HVM_DELIVER_NO_ERROR_CODE);
>  }
>  
> +static void update_guest_eip(void)
> +{
> +    unsigned long inst_len;
> +
> +    inst_len = __get_instruction_length();
> +    __update_guest_eip(inst_len);
> +}
> +
>  static void vmx_fpu_dirty_intercept(void)
>  {
>      struct vcpu *curr = current;
> @@ -2244,8 +2252,7 @@
>      if ( (((exit_qualification >> 12) & 0xf) == 1) &&
>           ((exit_qualification & 0xfff) == APIC_EOI) )
>      {
> -        int inst_len = __get_instruction_length(); /* Safe: APIC data
write
> */
> -        __update_guest_eip(inst_len);
> +        update_guest_eip();
Is it really a good idea to remove all these "Safe: ..." comments?
Without them, I think it''ll be much easier to not remember that
this cannot be done everywhere and hence to add an inappropriate
call to this function.

Jan
>          vlapic_EOI_set(vcpu_vlapic(current));
>          return 1;
>      }
> @@ -2424,8 +2431,7 @@
>          case TRAP_int3:
>              if ( !v->domain->debugger_attached )
>                  goto exit_and_crash;
> -            inst_len = __get_instruction_length(); /* Safe: INT3 */
> -            __update_guest_eip(inst_len);
> +            update_guest_eip();
>              current->arch.gdbsx_vcpu_event = TRAP_int3;
>              domain_pause_for_debugger();
>              break;
> @@ -2516,18 +2522,15 @@
>          break;
>      }
>      case EXIT_REASON_CPUID:
> -        inst_len = __get_instruction_length(); /* Safe: CPUID */
> -        __update_guest_eip(inst_len);
> +        update_guest_eip();
>          vmx_do_cpuid(regs);
>          break;
>      case EXIT_REASON_HLT:
> -        inst_len = __get_instruction_length(); /* Safe: HLT */
> -        __update_guest_eip(inst_len);
> +        update_guest_eip();
>          hvm_hlt(regs->eflags);
>          break;
>      case EXIT_REASON_INVLPG:
> -        inst_len = __get_instruction_length(); /* Safe: INVLPG */
> -        __update_guest_eip(inst_len);
> +        update_guest_eip();
>          exit_qualification = __vmread(EXIT_QUALIFICATION);
>          vmx_invlpg_intercept(exit_qualification);
>          break;
> @@ -2535,19 +2538,17 @@
>          regs->ecx = hvm_msr_tsc_aux(v);
>          /* fall through */
>      case EXIT_REASON_RDTSC:
> -        inst_len = __get_instruction_length();
> -        __update_guest_eip(inst_len);
> +        update_guest_eip();
>          hvm_rdtsc_intercept(regs);
>          break;
>      case EXIT_REASON_VMCALL:
>      {
>          int rc;
>          HVMTRACE_1D(VMMCALL, regs->eax);
> -        inst_len = __get_instruction_length(); /* Safe: VMCALL */
>          rc = hvm_do_hypercall(regs);
>          if ( rc != HVM_HCALL_preempted )
>          {
> -            __update_guest_eip(inst_len);
> +            update_guest_eip();
>              if ( rc == HVM_HCALL_invalidate )
>                  send_invalidate_req();
>          }
> @@ -2556,9 +2557,8 @@
>      case EXIT_REASON_CR_ACCESS:
>      {
>          exit_qualification = __vmread(EXIT_QUALIFICATION);
> -        inst_len = __get_instruction_length(); /* Safe: MOV Cn, LMSW, CLTS
> */
>          if ( vmx_cr_access(exit_qualification, regs) )
> -            __update_guest_eip(inst_len);
> +            update_guest_eip();
>          break;
>      }
>      case EXIT_REASON_DR_ACCESS:
> @@ -2568,22 +2568,20 @@
>      case EXIT_REASON_MSR_READ:
>      {
>          uint64_t msr_content;
> -        inst_len = __get_instruction_length(); /* Safe: RDMSR */
>          if ( hvm_msr_read_intercept(regs->ecx, &msr_content) ==
X86EMUL_OKAY )
>          {
>              regs->eax = (uint32_t)msr_content;
>              regs->edx = (uint32_t)(msr_content >> 32);
> -            __update_guest_eip(inst_len);
> +            update_guest_eip();
>          }
>          break;
>      }
>      case EXIT_REASON_MSR_WRITE:
>      {
>          uint64_t msr_content;
> -        inst_len = __get_instruction_length(); /* Safe: WRMSR */
>          msr_content = ((uint64_t)regs->edx << 32) |
(uint32_t)regs->eax;
>          if ( hvm_msr_write_intercept(regs->ecx, msr_content) ==
X86EMUL_OKAY
> )
> -            __update_guest_eip(inst_len);
> +            update_guest_eip();
>          break;
>      }
>  
> @@ -2652,8 +2650,7 @@
>      case EXIT_REASON_INVD:
>      case EXIT_REASON_WBINVD:
>      {
> -        inst_len = __get_instruction_length(); /* Safe: INVD, WBINVD */
> -        __update_guest_eip(inst_len);
> +        update_guest_eip();
>          vmx_wbinvd_intercept();
>          break;
>      }
> @@ -2686,8 +2683,7 @@
>          u64 new_bv  =  (((u64)regs->edx) << 32) | regs->eax;
>          if ( vmx_handle_xsetbv(new_bv) == 0 )
>          {
> -            inst_len = __get_instruction_length();
> -            __update_guest_eip(inst_len);
> +            update_guest_eip();
>          }
>          break;
>      }


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2010-Sep-15 15:38 UTC

head link

Re: [Xen-devel] A simple clean up of __get_instruction_length & __update_guest_eip

On 15/09/2010 16:25, "Jan Beulich" <JBeulich@novell.com> wrote:
> Is it really a good idea to remove all these "Safe: ..."
comments?
> Without them, I think it''ll be much easier to not remember that
> this cannot be done everywhere and hence to add an inappropriate
> call to this function.
I meant to check they were kept, and then forgot and applied it anyway.
I''ll
add them back in.

 --Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Sep 2010 - A simple clean up of __get_instruction_length & __update_guest_eip

[Xen-devel] A simple clean up of __get_instruction_length & __update_guest_eip

Re: [Xen-devel] A simple clean up of __get_instruction_length & __update_guest_eip

Re: [Xen-devel] A simple clean up of __get_instruction_length & __update_guest_eip