|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/2] x86emul: support LAR/LSL/VERR/VERW
On 08/12/16 11:52, Jan Beulich wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -182,7 +182,7 @@ static const opcode_desc_t opcode_table[
>
> static const opcode_desc_t twobyte_table[256] = {
> /* 0x00 - 0x07 */
> - ModRM, ImplicitOps|ModRM, ModRM, ModRM,
> + ModRM, ImplicitOps|ModRM, DstReg|SrcMem16|ModRM, DstReg|SrcMem16|ModRM,
> 0, ImplicitOps, ImplicitOps, ImplicitOps,
> /* 0x08 - 0x0F */
> ImplicitOps, ImplicitOps, 0, ImplicitOps,
> @@ -1384,7 +1384,7 @@ protmode_load_seg(
> }
A number of the following changes were very confusing to follow until I
realised that you are introducing a meaning, where
protmode_load_seg(x86_seg_none, ...) means "please do all the checks,
but don't commit any state or raise any exceptions".
It would be helpful to point this out in the commit message and in a
comment at the head of protmode_load_seg().
>
> /* System segment descriptors must reside in the GDT. */
> - if ( !is_x86_user_segment(seg) && (sel & 4) )
> + if ( is_x86_system_segment(seg) && (sel & 4) )
> goto raise_exn;
>
> switch ( rc = ops->read(sel_seg, sel & 0xfff8, &desc, sizeof(desc),
> ctxt) )
> @@ -1401,14 +1401,11 @@ protmode_load_seg(
> return rc;
> }
>
> - if ( !is_x86_user_segment(seg) )
> - {
> - /* System segments must have S flag == 0. */
> - if ( desc.b & (1u << 12) )
> - goto raise_exn;
> - }
> + /* System segments must have S flag == 0. */
> + if ( is_x86_system_segment(seg) && (desc.b & (1u << 12)) )
> + goto raise_exn;
> /* User segments must have S flag == 1. */
> - else if ( !(desc.b & (1u << 12)) )
> + if ( is_x86_user_segment(seg) && !(desc.b & (1u << 12)) )
> goto raise_exn;
This might be clearer as (and is definitely shorter as)
/* Check .S is correct for the type of segment. */
if ( seg != x86_seg_none &&
is_x86_user_segment(seg) != (desc.b & (1u << 12)) )
goto raise_exn;
>
> dpl = (desc.b >> 13) & 3;
> @@ -1470,10 +1467,17 @@ protmode_load_seg(
> ((dpl < cpl) || (dpl < rpl)) )
> goto raise_exn;
> break;
> + case x86_seg_none:
> + /* Non-conforming segment: check DPL against RPL and CPL. */
> + if ( ((desc.b & (0x1c << 8)) != (0x1c << 8)) &&
> + ((dpl < cpl) || (dpl < rpl)) )
> + return X86EMUL_EXCEPTION;
I realise it is no functional change, but it would be cleaner to have
this as a goto raise_exn, to avoid having one spurious early-exit in a
fucntion which otherwise always goes to raise_exn or done.
> + a_flag = 0;
> + break;
> }
>
> /* Segment present in memory? */
> - if ( !(desc.b & (1 << 15)) )
> + if ( !(desc.b & (1 << 15)) && seg != x86_seg_none )
What is the purpose of this change, given that the raise_exn case is
always conditional on seg != x86_seg_none ?
> {
> fault_type = seg != x86_seg_ss ? EXC_NP : EXC_SS;
> goto raise_exn;
> @@ -1481,7 +1485,7 @@ protmode_load_seg(
>
> if ( !is_x86_user_segment(seg) )
> {
> - int lm = in_longmode(ctxt, ops);
> + int lm = (desc.b & (1u << 12)) ? 0 : in_longmode(ctxt, ops);
>
> if ( lm < 0 )
> return X86EMUL_UNHANDLEABLE;
> @@ -1501,7 +1505,8 @@ protmode_load_seg(
> return rc;
> }
> if ( (desc_hi.b & 0x00001f00) ||
> - !is_canonical_address((uint64_t)desc_hi.a << 32) )
> + (seg != x86_seg_none &&
> + !is_canonical_address((uint64_t)desc_hi.a << 32)) )
> goto raise_exn;
> }
> }
> @@ -1544,7 +1549,8 @@ protmode_load_seg(
> return X86EMUL_OKAY;
>
> raise_exn:
> - generate_exception(fault_type, sel & 0xfffc);
> + generate_exception_if(seg != x86_seg_none, fault_type, sel & 0xfffc);
> + rc = X86EMUL_EXCEPTION;
> done:
> return rc;
> }
> @@ -4424,6 +4430,28 @@ x86_emulate(
> if ( (rc = load_seg(seg, src.val, 0, NULL, ctxt, ops)) != 0 )
> goto done;
> break;
> + case 4: /* verr / verw */
This looks wrong, but I see it isn't actually. Whether this patch or a
subsequent one, it would be clearer to alter the switch statement not to
mask off the bottom bit, and have individual case labels for the
instructions.
> + _regs.eflags &= ~EFLG_ZF;
> + switch ( rc = protmode_load_seg(x86_seg_none, src.val, false,
> + &sreg, ctxt, ops) )
> + {
> + case X86EMUL_OKAY:
> + if ( sreg.attr.fields.s &&
> + ((modrm_reg & 1) ? ((sreg.attr.fields.type & 0xa) ==
> 0x2)
> + : ((sreg.attr.fields.type & 0xa) !=
> 0x8)) )
> + _regs.eflags |= EFLG_ZF;
> + break;
> + case X86EMUL_EXCEPTION:
Could we please annotate the areas where we selectively passing
exceptions? I can see this pattern getting confusing if we don't.
Something like:
/* #PF needs to escape. #GP should have been squashed already. */
> + if ( ctxt->event_pending )
> + {
> + default:
> + goto done;
> + }
> + /* Instead of the exception, ZF remains cleared. */
> + rc = X86EMUL_OKAY;
> + break;
> + }
> + break;
> default:
> generate_exception_if(true, EXC_UD);
> break;
> @@ -4621,6 +4649,96 @@ x86_emulate(
> break;
> }
>
> + case X86EMUL_OPC(0x0f, 0x02): /* lar */
> + generate_exception_if(!in_protmode(ctxt, ops), EXC_UD);
As a tangential point, the distinction between the various in_*()
predicates is sufficiently subtle that I keep on having to look it up to
check.
What do you think about replacing the current predicates with a single
mode_is() predicate which takes an or-able set of REAL|VM86|PROT|LONG
flags ?
> + _regs.eflags &= ~EFLG_ZF;
> + switch ( rc = protmode_load_seg(x86_seg_none, src.val, false, &sreg,
> + ctxt, ops) )
> + {
> + case X86EMUL_OKAY:
> + if ( !sreg.attr.fields.s )
> + {
> + switch ( sreg.attr.fields.type )
> + {
> + case 0x01: /* available 16-bit TSS */
> + case 0x03: /* busy 16-bit TSS */
> + case 0x04: /* 16-bit call gate */
> + case 0x05: /* 16/32-bit task gate */
> + if ( in_longmode(ctxt, ops) )
> + break;
> + /* fall through */
> + case 0x02: /* LDT */
According to the Intel manual, LDT is valid in protected mode, but not
valid in long mode.
This appears to be a functional difference from AMD, who permit querying
LDT in long mode.
I haven't checked what real hardware behaviour is. Given that Intel
documents LDT as valid for LSL, I wonder whether this is a documentation
error.
> + case 0x09: /* available 32/64-bit TSS */
> + case 0x0b: /* busy 32/64-bit TSS */
> + case 0x0c: /* 32/64-bit call gate */
> + _regs.eflags |= EFLG_ZF;
> + break;
> + }
> + }
> + else
> + _regs.eflags |= EFLG_ZF;
> + break;
> + case X86EMUL_EXCEPTION:
> + if ( ctxt->event_pending )
> + {
> + default:
> + goto done;
> + }
> + /* Instead of the exception, ZF remains cleared. */
> + rc = X86EMUL_OKAY;
> + break;
> + }
> + if ( _regs.eflags & EFLG_ZF )
> + dst.val = ((sreg.attr.bytes & 0xff) << 8) |
> + ((sreg.limit >> (sreg.attr.fields.g ? 12 : 0)) &
> + 0xf0000) |
> + ((sreg.attr.bytes & 0xf00) << 12);
Is this correct? The AMD manuals says for 16bit, attr & 0xff00, and for
32 or 64bit, attr & 0xffff00.
The Intel manual describes this in a far more complicated way, but still
looks compatible.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |