From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: A simple clean up of __get_instruction_length & __update_guest_eip Date: Wed, 15 Sep 2010 16:25:32 +0100 Message-ID: <4C91018C02000078000165DD@vpn.id2.novell.com> References: <1A42CE6F5F474C41B63392A5F80372B22A8C23BA@shsmsx501.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <1A42CE6F5F474C41B63392A5F80372B22A8C23BA@shsmsx501.ccr.corp.intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Eddie Dong Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org >>> On 15.09.10 at 16:34, "Dong, Eddie" wrote: > Replace so many sparsed __get_instruction_length & __update_guest_eip = with=20 > one function call. >=20 > Signed-off-by: Eddie Dong >=20 > 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); > } > =20 > +static void update_guest_eip(void) > +{ > + unsigned long inst_len; > + > + inst_len =3D __get_instruction_length(); > + __update_guest_eip(inst_len); > +} > + > static void vmx_fpu_dirty_intercept(void) > { > struct vcpu *curr =3D current; > @@ -2244,8 +2252,7 @@ > if ( (((exit_qualification >> 12) & 0xf) =3D=3D 1) && > ((exit_qualification & 0xfff) =3D=3D APIC_EOI) ) > { > - int inst_len =3D __get_instruction_length(); /* Safe: APIC data = write=20 > */ > - __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 =3D __get_instruction_length(); /* Safe: INT3 */ > - __update_guest_eip(inst_len); > + update_guest_eip(); > current->arch.gdbsx_vcpu_event =3D TRAP_int3; > domain_pause_for_debugger(); > break; > @@ -2516,18 +2522,15 @@ > break; > } > case EXIT_REASON_CPUID: > - inst_len =3D __get_instruction_length(); /* Safe: CPUID */ > - __update_guest_eip(inst_len); > + update_guest_eip(); > vmx_do_cpuid(regs); > break; > case EXIT_REASON_HLT: > - inst_len =3D __get_instruction_length(); /* Safe: HLT */ > - __update_guest_eip(inst_len); > + update_guest_eip(); > hvm_hlt(regs->eflags); > break; > case EXIT_REASON_INVLPG: > - inst_len =3D __get_instruction_length(); /* Safe: INVLPG */ > - __update_guest_eip(inst_len); > + update_guest_eip(); > exit_qualification =3D __vmread(EXIT_QUALIFICATION); > vmx_invlpg_intercept(exit_qualification); > break; > @@ -2535,19 +2538,17 @@ > regs->ecx =3D hvm_msr_tsc_aux(v); > /* fall through */ > case EXIT_REASON_RDTSC: > - inst_len =3D __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 =3D __get_instruction_length(); /* Safe: VMCALL */ > rc =3D hvm_do_hypercall(regs); > if ( rc !=3D HVM_HCALL_preempted ) > { > - __update_guest_eip(inst_len); > + update_guest_eip(); > if ( rc =3D=3D HVM_HCALL_invalidate ) > send_invalidate_req(); > } > @@ -2556,9 +2557,8 @@ > case EXIT_REASON_CR_ACCESS: > { > exit_qualification =3D __vmread(EXIT_QUALIFICATION); > - inst_len =3D __get_instruction_length(); /* Safe: MOV Cn, LMSW, = CLTS=20 > */ > 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 =3D __get_instruction_length(); /* Safe: RDMSR */ > if ( hvm_msr_read_intercept(regs->ecx, &msr_content) =3D=3D = X86EMUL_OKAY ) > { > regs->eax =3D (uint32_t)msr_content; > regs->edx =3D (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 =3D __get_instruction_length(); /* Safe: WRMSR */ > msr_content =3D ((uint64_t)regs->edx << 32) | (uint32_t)regs->ea= x; > if ( hvm_msr_write_intercept(regs->ecx, msr_content) =3D=3D = X86EMUL_OKAY=20 > ) > - __update_guest_eip(inst_len); > + update_guest_eip(); > break; > } > =20 > @@ -2652,8 +2650,7 @@ > case EXIT_REASON_INVD: > case EXIT_REASON_WBINVD: > { > - inst_len =3D __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 =3D (((u64)regs->edx) << 32) | regs->eax; > if ( vmx_handle_xsetbv(new_bv) =3D=3D 0 ) > { > - inst_len =3D __get_instruction_length(); > - __update_guest_eip(inst_len); > + update_guest_eip(); > } > break; > }