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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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".

>> @@ -4177,14 +4205,31 @@ x86_emulate(
>>          }
>>          break;
>>  
>> -    case X86EMUL_OPC(0x0f, 0x00): /* Grp6 */
>> -        fail_if((modrm_reg & 6) != 2);
>> +    case X86EMUL_OPC(0x0f, 0x00): /* Grp6 */ {
> 
> Newline for {

Yeah, we're kind of inconsistent with that when they are for case
statements.

>> +        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)".

> 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.

Jan


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