Re: [Xen-devel] [PATCH 1/5] x86emul: support UMIP

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/09/16 13:23, Jan Beulich wrote:
>>>> On 30.09.16 at 12:32, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 08/09/16 14:42, Jan Beulich wrote:
>>> @@ -1484,6 +1485,17 @@ static bool is_aligned(enum x86_segment
>>>      return !((reg.base + offs) & (size - 1));
>>>  }
>>>  
>>> +static bool is_umip(struct x86_emulate_ctxt *ctxt,
>>> +                    const struct x86_emulate_ops *ops)
>> is_umip is an odd way of phrasing this.  umip_enabled() or
>> is_umip_enabled() would be better.
> That would have been my choice if there wasn't the CPL check in here.
> I prefer to read the 'p' here as "prevented" rather then "prevention".

In which case, umip_active()?

>>> +        enum x86_segment seg = (modrm_reg & 1) ? x86_seg_tr : x86_seg_ldtr;
>>> +
>>> +        fail_if(modrm_reg & 4);
>>>          generate_exception_if(!in_protmode(ctxt, ops), EXC_UD, -1);
>>> -        generate_exception_if(!mode_ring0(), EXC_GP, 0);
>>> -        if ( (rc = load_seg((modrm_reg & 1) ? x86_seg_tr : x86_seg_ldtr,
>>> -                            src.val, 0, NULL, ctxt, ops)) != 0 )
>>> -            goto done;
>>> +        if ( modrm_reg & 2 )
>> This needs to be (modrm_reg & 6) == 2.  Otherwise, the /6 and /7
>> encodings will also raise #GP when they should raise #UD
> Note the earlier "fail_if(modrm_reg & 4)".

Ah - I hadn't spotted that, which does catch that case, as well as ver{r,w}.

>
>> Actually thinking about it, could we just have a full switch here like
>> other Grp $N decodes?
> I can certainly pull this ahead from a later (not yet submitted)
> patch.

I think this would be best, especially if you already have the code to hand.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel