|
[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 at 17:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 08/12/16 11:52, Jan Beulich wrote:
>> @@ -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;
I had it that way first, and then thought the opposite (the explicit
two if()-s being more clear). I'd prefer to keep it as is, but if you
feel strongly about the other variant being better, I can change it.
>> @@ -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.
That's a matter of taste I think - to me it seems better to make
immediately obvious that no exception will be raised in this case
(which "goto raise_exn" would suggest).
>> /* 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 ?
Well - we don't want to reach raise_exn in this case, i.e. we
want to take the implicit "else" path. After all a clear P bit does
not result in the instructions to fail.
>> @@ -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.
Again a case where I think the masking off of the low bit is the
better approach. I wouldn't object to a patch altering it, but I'm
not convinced enough to put one together myself. And no, I
don't think this should be done in this patch.
>> + _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. */
Hmm, we're not selectively passing exceptions here; we pass any
which have got raised, and #GP/#SS/#NP aren't among them. So
I think the comment may rather want to be "Only #PF can come
here", which then we could as well ASSERT() ...
>> + if ( ctxt->event_pending )
>> + {
... here (and that would then serve as a de-facto comment).
What do you think?
>> @@ -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 ?
And you mean x86_decode() to set them once at its beginning?
That would make e.g. read_msr() a required hook, which I don't
think is a good idea (the more that x86_decode(), which in
particular gets passed almost no hooks at all from
x86_decode_insn(), doesn't need to know whether we're
emulating long mode).
If anything this would therefore need to be another input coming
from the original callers (complementing addr_size and sp_size).
>> + _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.
I did check all this on hardware, so yes, looks to be a doc 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.
Well - as for all operations truncation to operand size will occur
during writeback of dst.val to the destination register.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |