| Message ID | 584826390200007800126459@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/12/16 14:09, Jan Beulich wrote: > Now that segment registers are numbered naturally this can be easily > done to achieve some code size reduction. > > Also consistently use X86EMUL_OKAY in the code being touched. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Nice improvement in principle, but... > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -2670,51 +2670,40 @@ x86_emulate( > break; > > case 0x06: /* push %%es */ > - src.val = x86_seg_es; > - push_seg: > - generate_exception_if(mode_64bit() && !ext, EXC_UD); > + case 0x0e: /* push %%cs */ > + case 0x16: /* push %%ss */ > + case 0x1e: /* push %%ds */ > + generate_exception_if(mode_64bit(), EXC_UD); > + /* fall through */ > + case X86EMUL_OPC(0x0f, 0xa0): /* push %%fs */ > + case X86EMUL_OPC(0x0f, 0xa8): /* push %%gs */ > fail_if(ops->read_segment == NULL); > - if ( (rc = ops->read_segment(src.val, &sreg, ctxt)) != 0 ) > + if ( (rc = ops->read_segment((b >> 3) & 7, &sreg, > + ctxt)) != X86EMUL_OKAY ) > goto done; > src.val = sreg.sel; > goto push; > > case 0x07: /* pop %%es */ > - src.val = x86_seg_es; > - pop_seg: > - generate_exception_if(mode_64bit() && !ext, EXC_UD); > + case 0x17: /* pop %%ss */ > + case 0x1f: /* pop %%ds */ > + generate_exception_if(mode_64bit(), EXC_UD); > + /* fall through */ > + case X86EMUL_OPC(0x0f, 0xa1): /* pop %%fs */ > + case X86EMUL_OPC(0x0f, 0xa9): /* pop %%gs */ > fail_if(ops->write_segment == NULL); > /* 64-bit mode: POP defaults to a 64-bit operand. */ > if ( mode_64bit() && (op_bytes == 4) ) > op_bytes = 8; > - if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), > - &dst.val, op_bytes, ctxt, ops)) != 0 || > - (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) > + if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), &dst.val, > + op_bytes, ctxt, ops)) != X86EMUL_OKAY || > + (rc = load_seg((b >> 3) & 7, dst.val, 0, NULL, ctxt, > + ops)) != X86EMUL_OKAY ) > goto done; > - if ( src.val == x86_seg_ss ) > + if ( b == 0x07 ) This would be less error prone by setting seg = (b >> 3) & 7; similarly to the mov %sreg blocks. Doing so wouldn't accidentally break this by setting mov_ss for a pop %es. ~Andrew > ctxt->retire.mov_ss = true; > break;
>>> On 07.12.16 at 16:54, <andrew.cooper3@citrix.com> wrote: > On 07/12/16 14:09, Jan Beulich wrote: >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -2670,51 +2670,40 @@ x86_emulate( >> break; >> >> case 0x06: /* push %%es */ >> - src.val = x86_seg_es; >> - push_seg: >> - generate_exception_if(mode_64bit() && !ext, EXC_UD); >> + case 0x0e: /* push %%cs */ >> + case 0x16: /* push %%ss */ >> + case 0x1e: /* push %%ds */ >> + generate_exception_if(mode_64bit(), EXC_UD); >> + /* fall through */ >> + case X86EMUL_OPC(0x0f, 0xa0): /* push %%fs */ >> + case X86EMUL_OPC(0x0f, 0xa8): /* push %%gs */ >> fail_if(ops->read_segment == NULL); >> - if ( (rc = ops->read_segment(src.val, &sreg, ctxt)) != 0 ) >> + if ( (rc = ops->read_segment((b >> 3) & 7, &sreg, >> + ctxt)) != X86EMUL_OKAY ) >> goto done; >> src.val = sreg.sel; >> goto push; >> >> case 0x07: /* pop %%es */ >> - src.val = x86_seg_es; >> - pop_seg: >> - generate_exception_if(mode_64bit() && !ext, EXC_UD); >> + case 0x17: /* pop %%ss */ >> + case 0x1f: /* pop %%ds */ >> + generate_exception_if(mode_64bit(), EXC_UD); >> + /* fall through */ >> + case X86EMUL_OPC(0x0f, 0xa1): /* pop %%fs */ >> + case X86EMUL_OPC(0x0f, 0xa9): /* pop %%gs */ >> fail_if(ops->write_segment == NULL); >> /* 64-bit mode: POP defaults to a 64-bit operand. */ >> if ( mode_64bit() && (op_bytes == 4) ) >> op_bytes = 8; >> - if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), >> - &dst.val, op_bytes, ctxt, ops)) != 0 || >> - (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) >> + if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), &dst.val, >> + op_bytes, ctxt, ops)) != X86EMUL_OKAY || >> + (rc = load_seg((b >> 3) & 7, dst.val, 0, NULL, ctxt, >> + ops)) != X86EMUL_OKAY ) >> goto done; >> - if ( src.val == x86_seg_ss ) >> + if ( b == 0x07 ) > > This would be less error prone by setting > > seg = (b >> 3) & 7; > > similarly to the mov %sreg blocks. > > Doing so wouldn't accidentally break this by setting mov_ss for a pop %es. Oops - that's a pretty kind way of saying that I've introduced a bug. Jan
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2670,51 +2670,40 @@ x86_emulate( break; case 0x06: /* push %%es */ - src.val = x86_seg_es; - push_seg: - generate_exception_if(mode_64bit() && !ext, EXC_UD); + case 0x0e: /* push %%cs */ + case 0x16: /* push %%ss */ + case 0x1e: /* push %%ds */ + generate_exception_if(mode_64bit(), EXC_UD); + /* fall through */ + case X86EMUL_OPC(0x0f, 0xa0): /* push %%fs */ + case X86EMUL_OPC(0x0f, 0xa8): /* push %%gs */ fail_if(ops->read_segment == NULL); - if ( (rc = ops->read_segment(src.val, &sreg, ctxt)) != 0 ) + if ( (rc = ops->read_segment((b >> 3) & 7, &sreg, + ctxt)) != X86EMUL_OKAY ) goto done; src.val = sreg.sel; goto push; case 0x07: /* pop %%es */ - src.val = x86_seg_es; - pop_seg: - generate_exception_if(mode_64bit() && !ext, EXC_UD); + case 0x17: /* pop %%ss */ + case 0x1f: /* pop %%ds */ + generate_exception_if(mode_64bit(), EXC_UD); + /* fall through */ + case X86EMUL_OPC(0x0f, 0xa1): /* pop %%fs */ + case X86EMUL_OPC(0x0f, 0xa9): /* pop %%gs */ fail_if(ops->write_segment == NULL); /* 64-bit mode: POP defaults to a 64-bit operand. */ if ( mode_64bit() && (op_bytes == 4) ) op_bytes = 8; - if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), - &dst.val, op_bytes, ctxt, ops)) != 0 || - (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) + if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), &dst.val, + op_bytes, ctxt, ops)) != X86EMUL_OKAY || + (rc = load_seg((b >> 3) & 7, dst.val, 0, NULL, ctxt, + ops)) != X86EMUL_OKAY ) goto done; - if ( src.val == x86_seg_ss ) + if ( b == 0x07 ) ctxt->retire.mov_ss = true; break; - case 0x0e: /* push %%cs */ - src.val = x86_seg_cs; - goto push_seg; - - case 0x16: /* push %%ss */ - src.val = x86_seg_ss; - goto push_seg; - - case 0x17: /* pop %%ss */ - src.val = x86_seg_ss; - goto pop_seg; - - case 0x1e: /* push %%ds */ - src.val = x86_seg_ds; - goto push_seg; - - case 0x1f: /* pop %%ds */ - src.val = x86_seg_ds; - goto pop_seg; - case 0x27: /* daa */ case 0x2f: /* das */ { uint8_t al = _regs.eax; @@ -5042,14 +5031,6 @@ x86_emulate( dst.val = test_cc(b, _regs.eflags); break; - case X86EMUL_OPC(0x0f, 0xa0): /* push %%fs */ - src.val = x86_seg_fs; - goto push_seg; - - case X86EMUL_OPC(0x0f, 0xa1): /* pop %%fs */ - src.val = x86_seg_fs; - goto pop_seg; - case X86EMUL_OPC(0x0f, 0xa2): /* cpuid */ { unsigned int eax = _regs.eax, ebx = _regs.ebx; unsigned int ecx = _regs.ecx, edx = _regs.edx; @@ -5107,14 +5088,6 @@ x86_emulate( break; } - case X86EMUL_OPC(0x0f, 0xa8): /* push %%gs */ - src.val = x86_seg_gs; - goto push_seg; - - case X86EMUL_OPC(0x0f, 0xa9): /* pop %%gs */ - src.val = x86_seg_gs; - goto pop_seg; - case X86EMUL_OPC(0x0f, 0xab): bts: /* bts */ emulate_2op_SrcV_nobyte("bts", src, dst, _regs.eflags); break;