* [PATCH] x86: miscellaneous emulator adjustments
@ 2009-08-18 13:18 Jan Beulich
2009-08-18 14:12 ` Christoph Egger
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2009-08-18 13:18 UTC (permalink / raw)
To: xen-devel
Defer fail_if()-s as much as possible (in favor of possibly generating
exceptions), and avoid generating exceptions when not strictly
necessary.
Avoid fail_if()-s for simple return code checks (making the code that
used them consistent with other, longer existing code).
Eliminate redundant generate_exception_if()-s checking lock_prefix
(which is already covered by the general check prior to decoding
operands).
Also fix the testing code to add PROT_EXEC for the mapping that is
intended to have instruction executed from.
Signed-off-by: Jan Beulich <jbeulich@novell.com>
--- 2009-08-18.orig/tools/tests/test_x86_emulator.c 2008-07-18 16:19:34.000000000 +0200
+++ 2009-08-18/tools/tests/test_x86_emulator.c 2009-08-18 14:18:20.000000000 +0200
@@ -76,7 +76,7 @@ int main(int argc, char **argv)
ctxt.addr_size = 32;
ctxt.sp_size = 32;
- res = mmap((void *)0x100000, MMAP_SZ, PROT_READ|PROT_WRITE,
+ res = mmap((void *)0x100000, MMAP_SZ, PROT_READ|PROT_WRITE|PROT_EXEC,
MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
if ( res == MAP_FAILED )
{
--- 2009-08-18.orig/xen/arch/x86/x86_emulate/x86_emulate.c 2009-08-18 14:18:15.000000000 +0200
+++ 2009-08-18/xen/arch/x86/x86_emulate/x86_emulate.c 2009-08-18 14:18:20.000000000 +0200
@@ -3583,21 +3583,17 @@ x86_emulate(
struct segment_register cs = { 0 }, ss = { 0 };
int rc;
- fail_if(ops->read_msr == NULL);
- fail_if(ops->read_segment == NULL);
- fail_if(ops->write_segment == NULL);
-
generate_exception_if(in_realmode(ctxt, ops), EXC_UD, 0);
generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, 0);
- generate_exception_if(lock_prefix, EXC_UD, 0);
/* Inject #UD if syscall/sysret are disabled. */
- rc = ops->read_msr(MSR_EFER, &msr_content, ctxt);
- fail_if(rc != 0);
+ fail_if(ops->read_msr == NULL);
+ if ( (rc = ops->read_msr(MSR_EFER, &msr_content, ctxt)) != 0 )
+ goto done;
generate_exception_if((msr_content & EFER_SCE) == 0, EXC_UD, 0);
- rc = ops->read_msr(MSR_STAR, &msr_content, ctxt);
- fail_if(rc != 0);
+ if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
+ goto done;
msr_content >>= 32;
cs.sel = (uint16_t)(msr_content & 0xfffc);
@@ -3617,27 +3613,27 @@ x86_emulate(
_regs.rcx = _regs.rip;
_regs.r11 = _regs.eflags & ~EFLG_RF;
- rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
- &msr_content, ctxt);
- fail_if(rc != 0);
-
+ if ( (rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
+ &msr_content, ctxt)) != 0 )
+ goto done;
_regs.rip = msr_content;
- rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt);
- fail_if(rc != 0);
+ if ( (rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt)) != 0 )
+ goto done;
_regs.eflags &= ~(msr_content | EFLG_RF);
}
else
#endif
{
- rc = ops->read_msr(MSR_STAR, &msr_content, ctxt);
- fail_if(rc != 0);
+ if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
+ goto done;
_regs.ecx = _regs.eip;
_regs.eip = (uint32_t)msr_content;
_regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
}
+ fail_if(ops->write_segment == NULL);
if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) ||
(rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) )
goto done;
@@ -3717,10 +3713,13 @@ x86_emulate(
case 0x31: /* rdtsc */ {
unsigned long cr4;
uint64_t val;
- fail_if(ops->read_cr == NULL);
- if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
- goto done;
- generate_exception_if((cr4 & CR4_TSD) && !mode_ring0(), EXC_GP, 0);
+ if ( !mode_ring0() )
+ {
+ fail_if(ops->read_cr == NULL);
+ if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
+ goto done;
+ generate_exception_if(cr4 & CR4_TSD, EXC_GP, 0);
+ }
fail_if(ops->read_msr == NULL);
if ( (rc = ops->read_msr(MSR_TSC, &val, ctxt)) != 0 )
goto done;
@@ -3751,17 +3750,13 @@ x86_emulate(
struct segment_register cs, ss;
int rc;
- fail_if(ops->read_msr == NULL);
- fail_if(ops->read_segment == NULL);
- fail_if(ops->write_segment == NULL);
-
generate_exception_if(mode_ring0(), EXC_GP, 0);
generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
- generate_exception_if(lock_prefix, EXC_UD, 0);
- rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt);
- fail_if(rc != 0);
+ fail_if(ops->read_msr == NULL);
+ if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) != 0 )
+ goto done;
if ( mode_64bit() )
generate_exception_if(msr_content == 0, EXC_GP, 0);
@@ -3770,6 +3765,7 @@ x86_emulate(
_regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
+ fail_if(ops->read_segment == NULL);
ops->read_segment(x86_seg_cs, &cs, ctxt);
cs.sel = (uint16_t)msr_content & ~3; /* SELECTOR_RPL_MASK */
cs.base = 0; /* flat segment */
@@ -3787,17 +3783,17 @@ x86_emulate(
cs.attr.fields.l = 1;
}
- rc = ops->write_segment(x86_seg_cs, &cs, ctxt);
- fail_if(rc != 0);
- rc = ops->write_segment(x86_seg_ss, &ss, ctxt);
- fail_if(rc != 0);
+ fail_if(ops->write_segment == NULL);
+ if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
+ (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
+ goto done;
- rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt);
- fail_if(rc != 0);
+ if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt)) != 0 )
+ goto done;
_regs.eip = msr_content;
- rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt);
- fail_if(rc != 0);
+ if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt)) != 0 )
+ goto done;
_regs.esp = msr_content;
break;
@@ -3809,19 +3805,13 @@ x86_emulate(
int user64 = !!(rex_prefix & 8); /* REX.W */
int rc;
- fail_if(ops->read_msr == NULL);
- fail_if(ops->read_segment == NULL);
- fail_if(ops->write_segment == NULL);
-
generate_exception_if(!mode_ring0(), EXC_GP, 0);
generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
- generate_exception_if(lock_prefix, EXC_UD, 0);
- rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt);
- fail_if(rc != 0);
- rc = ops->read_segment(x86_seg_cs, &cs, ctxt);
- fail_if(rc != 0);
+ fail_if(ops->read_msr == NULL);
+ if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) != 0 )
+ goto done;
if ( user64 )
{
@@ -3852,10 +3842,10 @@ x86_emulate(
cs.attr.fields.l = 1;
}
- rc = ops->write_segment(x86_seg_cs, &cs, ctxt);
- fail_if(rc != 0);
- rc = ops->write_segment(x86_seg_ss, &ss, ctxt);
- fail_if(rc != 0);
+ fail_if(ops->write_segment == NULL);
+ if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
+ (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
+ goto done;
_regs.eip = _regs.edx;
_regs.esp = _regs.ecx;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86: miscellaneous emulator adjustments
2009-08-18 13:18 [PATCH] x86: miscellaneous emulator adjustments Jan Beulich
@ 2009-08-18 14:12 ` Christoph Egger
2009-08-18 14:55 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Egger @ 2009-08-18 14:12 UTC (permalink / raw)
To: xen-devel; +Cc: Jan Beulich
How did you test this patch ?
Christoph
On Tuesday 18 August 2009 15:18:49 Jan Beulich wrote:
> Defer fail_if()-s as much as possible (in favor of possibly generating
> exceptions), and avoid generating exceptions when not strictly
> necessary.
>
> Avoid fail_if()-s for simple return code checks (making the code that
> used them consistent with other, longer existing code).
>
> Eliminate redundant generate_exception_if()-s checking lock_prefix
> (which is already covered by the general check prior to decoding
> operands).
>
> Also fix the testing code to add PROT_EXEC for the mapping that is
> intended to have instruction executed from.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>
> --- 2009-08-18.orig/tools/tests/test_x86_emulator.c 2008-07-18
> 16:19:34.000000000 +0200 +++
> 2009-08-18/tools/tests/test_x86_emulator.c 2009-08-18 14:18:20.000000000
> +0200 @@ -76,7 +76,7 @@ int main(int argc, char **argv)
> ctxt.addr_size = 32;
> ctxt.sp_size = 32;
>
> - res = mmap((void *)0x100000, MMAP_SZ, PROT_READ|PROT_WRITE,
> + res = mmap((void *)0x100000, MMAP_SZ, PROT_READ|PROT_WRITE|PROT_EXEC,
> MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
> if ( res == MAP_FAILED )
> {
> --- 2009-08-18.orig/xen/arch/x86/x86_emulate/x86_emulate.c 2009-08-18
> 14:18:15.000000000 +0200 +++
> 2009-08-18/xen/arch/x86/x86_emulate/x86_emulate.c 2009-08-18
> 14:18:20.000000000 +0200 @@ -3583,21 +3583,17 @@ x86_emulate(
> struct segment_register cs = { 0 }, ss = { 0 };
> int rc;
>
> - fail_if(ops->read_msr == NULL);
> - fail_if(ops->read_segment == NULL);
> - fail_if(ops->write_segment == NULL);
> -
> generate_exception_if(in_realmode(ctxt, ops), EXC_UD, 0);
> generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, 0);
> - generate_exception_if(lock_prefix, EXC_UD, 0);
>
> /* Inject #UD if syscall/sysret are disabled. */
> - rc = ops->read_msr(MSR_EFER, &msr_content, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->read_msr == NULL);
> + if ( (rc = ops->read_msr(MSR_EFER, &msr_content, ctxt)) != 0 )
> + goto done;
> generate_exception_if((msr_content & EFER_SCE) == 0, EXC_UD, 0);
>
> - rc = ops->read_msr(MSR_STAR, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
> + goto done;
>
> msr_content >>= 32;
> cs.sel = (uint16_t)(msr_content & 0xfffc);
> @@ -3617,27 +3613,27 @@ x86_emulate(
> _regs.rcx = _regs.rip;
> _regs.r11 = _regs.eflags & ~EFLG_RF;
>
> - rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
> - &msr_content, ctxt);
> - fail_if(rc != 0);
> -
> + if ( (rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
> + &msr_content, ctxt)) != 0 )
> + goto done;
> _regs.rip = msr_content;
>
> - rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt)) != 0
> ) + goto done;
> _regs.eflags &= ~(msr_content | EFLG_RF);
> }
> else
> #endif
> {
> - rc = ops->read_msr(MSR_STAR, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
> + goto done;
>
> _regs.ecx = _regs.eip;
> _regs.eip = (uint32_t)msr_content;
> _regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
> }
>
> + fail_if(ops->write_segment == NULL);
> if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) ||
> (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) )
> goto done;
> @@ -3717,10 +3713,13 @@ x86_emulate(
> case 0x31: /* rdtsc */ {
> unsigned long cr4;
> uint64_t val;
> - fail_if(ops->read_cr == NULL);
> - if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
> - goto done;
> - generate_exception_if((cr4 & CR4_TSD) && !mode_ring0(), EXC_GP,
> 0); + if ( !mode_ring0() )
> + {
> + fail_if(ops->read_cr == NULL);
> + if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
> + goto done;
> + generate_exception_if(cr4 & CR4_TSD, EXC_GP, 0);
> + }
> fail_if(ops->read_msr == NULL);
> if ( (rc = ops->read_msr(MSR_TSC, &val, ctxt)) != 0 )
> goto done;
> @@ -3751,17 +3750,13 @@ x86_emulate(
> struct segment_register cs, ss;
> int rc;
>
> - fail_if(ops->read_msr == NULL);
> - fail_if(ops->read_segment == NULL);
> - fail_if(ops->write_segment == NULL);
> -
> generate_exception_if(mode_ring0(), EXC_GP, 0);
> generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
> generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
> - generate_exception_if(lock_prefix, EXC_UD, 0);
>
> - rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->read_msr == NULL);
> + if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) !=
> 0 ) + goto done;
>
> if ( mode_64bit() )
> generate_exception_if(msr_content == 0, EXC_GP, 0);
> @@ -3770,6 +3765,7 @@ x86_emulate(
>
> _regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
>
> + fail_if(ops->read_segment == NULL);
> ops->read_segment(x86_seg_cs, &cs, ctxt);
> cs.sel = (uint16_t)msr_content & ~3; /* SELECTOR_RPL_MASK */
> cs.base = 0; /* flat segment */
> @@ -3787,17 +3783,17 @@ x86_emulate(
> cs.attr.fields.l = 1;
> }
>
> - rc = ops->write_segment(x86_seg_cs, &cs, ctxt);
> - fail_if(rc != 0);
> - rc = ops->write_segment(x86_seg_ss, &ss, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->write_segment == NULL);
> + if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
> + (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
> + goto done;
>
> - rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt)) !=
> 0 ) + goto done;
> _regs.eip = msr_content;
>
> - rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt)) !=
> 0 ) + goto done;
> _regs.esp = msr_content;
>
> break;
> @@ -3809,19 +3805,13 @@ x86_emulate(
> int user64 = !!(rex_prefix & 8); /* REX.W */
> int rc;
>
> - fail_if(ops->read_msr == NULL);
> - fail_if(ops->read_segment == NULL);
> - fail_if(ops->write_segment == NULL);
> -
> generate_exception_if(!mode_ring0(), EXC_GP, 0);
> generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
> generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
> - generate_exception_if(lock_prefix, EXC_UD, 0);
>
> - rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt);
> - fail_if(rc != 0);
> - rc = ops->read_segment(x86_seg_cs, &cs, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->read_msr == NULL);
> + if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) !=
> 0 ) + goto done;
>
> if ( user64 )
> {
> @@ -3852,10 +3842,10 @@ x86_emulate(
> cs.attr.fields.l = 1;
> }
>
> - rc = ops->write_segment(x86_seg_cs, &cs, ctxt);
> - fail_if(rc != 0);
> - rc = ops->write_segment(x86_seg_ss, &ss, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->write_segment == NULL);
> + if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
> + (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
> + goto done;
>
> _regs.eip = _regs.edx;
> _regs.esp = _regs.ecx;
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86: miscellaneous emulator adjustments
2009-08-18 14:12 ` Christoph Egger
@ 2009-08-18 14:55 ` Jan Beulich
2009-08-18 15:00 ` Christoph Egger
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2009-08-18 14:55 UTC (permalink / raw)
To: Christoph Egger; +Cc: xen-devel
I didn't do significantly more testing than what the test utility does plus
bring up a couple of HVM guests.
Do you see anything obviously wrong with it (after all, it's mainly code
re-ordering)?
Jan
>>> Christoph Egger <Christoph.Egger@amd.com> 18.08.09 16:12 >>>
How did you test this patch ?
Christoph
On Tuesday 18 August 2009 15:18:49 Jan Beulich wrote:
> Defer fail_if()-s as much as possible (in favor of possibly generating
> exceptions), and avoid generating exceptions when not strictly
> necessary.
>
> Avoid fail_if()-s for simple return code checks (making the code that
> used them consistent with other, longer existing code).
>
> Eliminate redundant generate_exception_if()-s checking lock_prefix
> (which is already covered by the general check prior to decoding
> operands).
>
> Also fix the testing code to add PROT_EXEC for the mapping that is
> intended to have instruction executed from.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>
> --- 2009-08-18.orig/tools/tests/test_x86_emulator.c 2008-07-18
> 16:19:34.000000000 +0200 +++
> 2009-08-18/tools/tests/test_x86_emulator.c 2009-08-18 14:18:20.000000000
> +0200 @@ -76,7 +76,7 @@ int main(int argc, char **argv)
> ctxt.addr_size = 32;
> ctxt.sp_size = 32;
>
> - res = mmap((void *)0x100000, MMAP_SZ, PROT_READ|PROT_WRITE,
> + res = mmap((void *)0x100000, MMAP_SZ, PROT_READ|PROT_WRITE|PROT_EXEC,
> MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, 0, 0);
> if ( res == MAP_FAILED )
> {
> --- 2009-08-18.orig/xen/arch/x86/x86_emulate/x86_emulate.c 2009-08-18
> 14:18:15.000000000 +0200 +++
> 2009-08-18/xen/arch/x86/x86_emulate/x86_emulate.c 2009-08-18
> 14:18:20.000000000 +0200 @@ -3583,21 +3583,17 @@ x86_emulate(
> struct segment_register cs = { 0 }, ss = { 0 };
> int rc;
>
> - fail_if(ops->read_msr == NULL);
> - fail_if(ops->read_segment == NULL);
> - fail_if(ops->write_segment == NULL);
> -
> generate_exception_if(in_realmode(ctxt, ops), EXC_UD, 0);
> generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, 0);
> - generate_exception_if(lock_prefix, EXC_UD, 0);
>
> /* Inject #UD if syscall/sysret are disabled. */
> - rc = ops->read_msr(MSR_EFER, &msr_content, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->read_msr == NULL);
> + if ( (rc = ops->read_msr(MSR_EFER, &msr_content, ctxt)) != 0 )
> + goto done;
> generate_exception_if((msr_content & EFER_SCE) == 0, EXC_UD, 0);
>
> - rc = ops->read_msr(MSR_STAR, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
> + goto done;
>
> msr_content >>= 32;
> cs.sel = (uint16_t)(msr_content & 0xfffc);
> @@ -3617,27 +3613,27 @@ x86_emulate(
> _regs.rcx = _regs.rip;
> _regs.r11 = _regs.eflags & ~EFLG_RF;
>
> - rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
> - &msr_content, ctxt);
> - fail_if(rc != 0);
> -
> + if ( (rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
> + &msr_content, ctxt)) != 0 )
> + goto done;
> _regs.rip = msr_content;
>
> - rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt)) != 0
> ) + goto done;
> _regs.eflags &= ~(msr_content | EFLG_RF);
> }
> else
> #endif
> {
> - rc = ops->read_msr(MSR_STAR, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
> + goto done;
>
> _regs.ecx = _regs.eip;
> _regs.eip = (uint32_t)msr_content;
> _regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
> }
>
> + fail_if(ops->write_segment == NULL);
> if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) ||
> (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) )
> goto done;
> @@ -3717,10 +3713,13 @@ x86_emulate(
> case 0x31: /* rdtsc */ {
> unsigned long cr4;
> uint64_t val;
> - fail_if(ops->read_cr == NULL);
> - if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
> - goto done;
> - generate_exception_if((cr4 & CR4_TSD) && !mode_ring0(), EXC_GP,
> 0); + if ( !mode_ring0() )
> + {
> + fail_if(ops->read_cr == NULL);
> + if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
> + goto done;
> + generate_exception_if(cr4 & CR4_TSD, EXC_GP, 0);
> + }
> fail_if(ops->read_msr == NULL);
> if ( (rc = ops->read_msr(MSR_TSC, &val, ctxt)) != 0 )
> goto done;
> @@ -3751,17 +3750,13 @@ x86_emulate(
> struct segment_register cs, ss;
> int rc;
>
> - fail_if(ops->read_msr == NULL);
> - fail_if(ops->read_segment == NULL);
> - fail_if(ops->write_segment == NULL);
> -
> generate_exception_if(mode_ring0(), EXC_GP, 0);
> generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
> generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
> - generate_exception_if(lock_prefix, EXC_UD, 0);
>
> - rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->read_msr == NULL);
> + if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) !=
> 0 ) + goto done;
>
> if ( mode_64bit() )
> generate_exception_if(msr_content == 0, EXC_GP, 0);
> @@ -3770,6 +3765,7 @@ x86_emulate(
>
> _regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
>
> + fail_if(ops->read_segment == NULL);
> ops->read_segment(x86_seg_cs, &cs, ctxt);
> cs.sel = (uint16_t)msr_content & ~3; /* SELECTOR_RPL_MASK */
> cs.base = 0; /* flat segment */
> @@ -3787,17 +3783,17 @@ x86_emulate(
> cs.attr.fields.l = 1;
> }
>
> - rc = ops->write_segment(x86_seg_cs, &cs, ctxt);
> - fail_if(rc != 0);
> - rc = ops->write_segment(x86_seg_ss, &ss, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->write_segment == NULL);
> + if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
> + (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
> + goto done;
>
> - rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt)) !=
> 0 ) + goto done;
> _regs.eip = msr_content;
>
> - rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt);
> - fail_if(rc != 0);
> + if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt)) !=
> 0 ) + goto done;
> _regs.esp = msr_content;
>
> break;
> @@ -3809,19 +3805,13 @@ x86_emulate(
> int user64 = !!(rex_prefix & 8); /* REX.W */
> int rc;
>
> - fail_if(ops->read_msr == NULL);
> - fail_if(ops->read_segment == NULL);
> - fail_if(ops->write_segment == NULL);
> -
> generate_exception_if(!mode_ring0(), EXC_GP, 0);
> generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
> generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
> - generate_exception_if(lock_prefix, EXC_UD, 0);
>
> - rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt);
> - fail_if(rc != 0);
> - rc = ops->read_segment(x86_seg_cs, &cs, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->read_msr == NULL);
> + if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt)) !=
> 0 ) + goto done;
>
> if ( user64 )
> {
> @@ -3852,10 +3842,10 @@ x86_emulate(
> cs.attr.fields.l = 1;
> }
>
> - rc = ops->write_segment(x86_seg_cs, &cs, ctxt);
> - fail_if(rc != 0);
> - rc = ops->write_segment(x86_seg_ss, &ss, ctxt);
> - fail_if(rc != 0);
> + fail_if(ops->write_segment == NULL);
> + if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
> + (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
> + goto done;
>
> _regs.eip = _regs.edx;
> _regs.esp = _regs.ecx;
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86: miscellaneous emulator adjustments
2009-08-18 14:55 ` Jan Beulich
@ 2009-08-18 15:00 ` Christoph Egger
0 siblings, 0 replies; 4+ messages in thread
From: Christoph Egger @ 2009-08-18 15:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On Tuesday 18 August 2009 16:55:04 Jan Beulich wrote:
> I didn't do significantly more testing than what the test utility does plus
> bring up a couple of HVM guests.
>
> Do you see anything obviously wrong with it (after all, it's mainly code
> re-ordering)?
No, I don't. I just ask, because the syscall/sysret emulation path is not that
easy to test like you did.
Christoph
>
> Jan
>
> >>> Christoph Egger <Christoph.Egger@amd.com> 18.08.09 16:12 >>>
>
> How did you test this patch ?
>
> Christoph
>
> On Tuesday 18 August 2009 15:18:49 Jan Beulich wrote:
> > Defer fail_if()-s as much as possible (in favor of possibly generating
> > exceptions), and avoid generating exceptions when not strictly
> > necessary.
> >
> > Avoid fail_if()-s for simple return code checks (making the code that
> > used them consistent with other, longer existing code).
> >
> > Eliminate redundant generate_exception_if()-s checking lock_prefix
> > (which is already covered by the general check prior to decoding
> > operands).
> >
> > Also fix the testing code to add PROT_EXEC for the mapping that is
> > intended to have instruction executed from.
> >
> > Signed-off-by: Jan Beulich <jbeulich@novell.com>
> >
> > --- 2009-08-18.orig/tools/tests/test_x86_emulator.c 2008-07-18
> > 16:19:34.000000000 +0200 +++
> > 2009-08-18/tools/tests/test_x86_emulator.c 2009-08-18 14:18:20.000000000
> > +0200 @@ -76,7 +76,7 @@ int main(int argc, char **argv)
> > ctxt.addr_size = 32;
> > ctxt.sp_size = 32;
> >
> > - res = mmap((void *)0x100000, MMAP_SZ, PROT_READ|PROT_WRITE,
> > + res = mmap((void *)0x100000, MMAP_SZ,
> > PROT_READ|PROT_WRITE|PROT_EXEC, MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, 0,
> > 0);
> > if ( res == MAP_FAILED )
> > {
> > --- 2009-08-18.orig/xen/arch/x86/x86_emulate/x86_emulate.c 2009-08-18
> > 14:18:15.000000000 +0200 +++
> > 2009-08-18/xen/arch/x86/x86_emulate/x86_emulate.c 2009-08-18
> > 14:18:20.000000000 +0200 @@ -3583,21 +3583,17 @@ x86_emulate(
> > struct segment_register cs = { 0 }, ss = { 0 };
> > int rc;
> >
> > - fail_if(ops->read_msr == NULL);
> > - fail_if(ops->read_segment == NULL);
> > - fail_if(ops->write_segment == NULL);
> > -
> > generate_exception_if(in_realmode(ctxt, ops), EXC_UD, 0);
> > generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, 0);
> > - generate_exception_if(lock_prefix, EXC_UD, 0);
> >
> > /* Inject #UD if syscall/sysret are disabled. */
> > - rc = ops->read_msr(MSR_EFER, &msr_content, ctxt);
> > - fail_if(rc != 0);
> > + fail_if(ops->read_msr == NULL);
> > + if ( (rc = ops->read_msr(MSR_EFER, &msr_content, ctxt)) != 0 )
> > + goto done;
> > generate_exception_if((msr_content & EFER_SCE) == 0, EXC_UD, 0);
> >
> > - rc = ops->read_msr(MSR_STAR, &msr_content, ctxt);
> > - fail_if(rc != 0);
> > + if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0 )
> > + goto done;
> >
> > msr_content >>= 32;
> > cs.sel = (uint16_t)(msr_content & 0xfffc);
> > @@ -3617,27 +3613,27 @@ x86_emulate(
> > _regs.rcx = _regs.rip;
> > _regs.r11 = _regs.eflags & ~EFLG_RF;
> >
> > - rc = ops->read_msr(mode_64bit() ? MSR_LSTAR : MSR_CSTAR,
> > - &msr_content, ctxt);
> > - fail_if(rc != 0);
> > -
> > + if ( (rc = ops->read_msr(mode_64bit() ? MSR_LSTAR :
> > MSR_CSTAR, + &msr_content, ctxt)) !=
> > 0 ) + goto done;
> > _regs.rip = msr_content;
> >
> > - rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt);
> > - fail_if(rc != 0);
> > + if ( (rc = ops->read_msr(MSR_FMASK, &msr_content, ctxt)) !=
> > 0 ) + goto done;
> > _regs.eflags &= ~(msr_content | EFLG_RF);
> > }
> > else
> > #endif
> > {
> > - rc = ops->read_msr(MSR_STAR, &msr_content, ctxt);
> > - fail_if(rc != 0);
> > + if ( (rc = ops->read_msr(MSR_STAR, &msr_content, ctxt)) != 0
> > ) + goto done;
> >
> > _regs.ecx = _regs.eip;
> > _regs.eip = (uint32_t)msr_content;
> > _regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
> > }
> >
> > + fail_if(ops->write_segment == NULL);
> > if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) ||
> > (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) )
> > goto done;
> > @@ -3717,10 +3713,13 @@ x86_emulate(
> > case 0x31: /* rdtsc */ {
> > unsigned long cr4;
> > uint64_t val;
> > - fail_if(ops->read_cr == NULL);
> > - if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
> > - goto done;
> > - generate_exception_if((cr4 & CR4_TSD) && !mode_ring0(), EXC_GP,
> > 0); + if ( !mode_ring0() )
> > + {
> > + fail_if(ops->read_cr == NULL);
> > + if ( (rc = ops->read_cr(4, &cr4, ctxt)) )
> > + goto done;
> > + generate_exception_if(cr4 & CR4_TSD, EXC_GP, 0);
> > + }
> > fail_if(ops->read_msr == NULL);
> > if ( (rc = ops->read_msr(MSR_TSC, &val, ctxt)) != 0 )
> > goto done;
> > @@ -3751,17 +3750,13 @@ x86_emulate(
> > struct segment_register cs, ss;
> > int rc;
> >
> > - fail_if(ops->read_msr == NULL);
> > - fail_if(ops->read_segment == NULL);
> > - fail_if(ops->write_segment == NULL);
> > -
> > generate_exception_if(mode_ring0(), EXC_GP, 0);
> > generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
> > generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
> > - generate_exception_if(lock_prefix, EXC_UD, 0);
> >
> > - rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt);
> > - fail_if(rc != 0);
> > + fail_if(ops->read_msr == NULL);
> > + if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt))
> > != 0 ) + goto done;
> >
> > if ( mode_64bit() )
> > generate_exception_if(msr_content == 0, EXC_GP, 0);
> > @@ -3770,6 +3765,7 @@ x86_emulate(
> >
> > _regs.eflags &= ~(EFLG_VM | EFLG_IF | EFLG_RF);
> >
> > + fail_if(ops->read_segment == NULL);
> > ops->read_segment(x86_seg_cs, &cs, ctxt);
> > cs.sel = (uint16_t)msr_content & ~3; /* SELECTOR_RPL_MASK */
> > cs.base = 0; /* flat segment */
> > @@ -3787,17 +3783,17 @@ x86_emulate(
> > cs.attr.fields.l = 1;
> > }
> >
> > - rc = ops->write_segment(x86_seg_cs, &cs, ctxt);
> > - fail_if(rc != 0);
> > - rc = ops->write_segment(x86_seg_ss, &ss, ctxt);
> > - fail_if(rc != 0);
> > + fail_if(ops->write_segment == NULL);
> > + if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
> > + (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
> > + goto done;
> >
> > - rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt);
> > - fail_if(rc != 0);
> > + if ( (rc = ops->read_msr(MSR_SYSENTER_EIP, &msr_content, ctxt))
> > != 0 ) + goto done;
> > _regs.eip = msr_content;
> >
> > - rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt);
> > - fail_if(rc != 0);
> > + if ( (rc = ops->read_msr(MSR_SYSENTER_ESP, &msr_content, ctxt))
> > != 0 ) + goto done;
> > _regs.esp = msr_content;
> >
> > break;
> > @@ -3809,19 +3805,13 @@ x86_emulate(
> > int user64 = !!(rex_prefix & 8); /* REX.W */
> > int rc;
> >
> > - fail_if(ops->read_msr == NULL);
> > - fail_if(ops->read_segment == NULL);
> > - fail_if(ops->write_segment == NULL);
> > -
> > generate_exception_if(!mode_ring0(), EXC_GP, 0);
> > generate_exception_if(in_realmode(ctxt, ops), EXC_GP, 0);
> > generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0);
> > - generate_exception_if(lock_prefix, EXC_UD, 0);
> >
> > - rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt);
> > - fail_if(rc != 0);
> > - rc = ops->read_segment(x86_seg_cs, &cs, ctxt);
> > - fail_if(rc != 0);
> > + fail_if(ops->read_msr == NULL);
> > + if ( (rc = ops->read_msr(MSR_SYSENTER_CS, &msr_content, ctxt))
> > != 0 ) + goto done;
> >
> > if ( user64 )
> > {
> > @@ -3852,10 +3842,10 @@ x86_emulate(
> > cs.attr.fields.l = 1;
> > }
> >
> > - rc = ops->write_segment(x86_seg_cs, &cs, ctxt);
> > - fail_if(rc != 0);
> > - rc = ops->write_segment(x86_seg_ss, &ss, ctxt);
> > - fail_if(rc != 0);
> > + fail_if(ops->write_segment == NULL);
> > + if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != 0 ||
> > + (rc = ops->write_segment(x86_seg_ss, &ss, ctxt)) != 0 )
> > + goto done;
> >
> > _regs.eip = _regs.edx;
> > _regs.esp = _regs.ecx;
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
--
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-08-18 15:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-08-18 13:18 [PATCH] x86: miscellaneous emulator adjustments Jan Beulich
2009-08-18 14:12 ` Christoph Egger
2009-08-18 14:55 ` Jan Beulich
2009-08-18 15:00 ` Christoph Egger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.