Re: [Xen-devel] [PATCH 15/15] x86/hvm: Use system-segment relative memory accesses

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/11/16 16:01, Jan Beulich wrote:
>>>> On 23.11.16 at 16:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -1181,20 +1181,38 @@ static int ioport_access_check(
>>          return rc;
>>  
>>      /* Ensure the TSS has an io-bitmap-offset field. */
>> -    generate_exception_if(tr.attr.fields.type != 0xb ||
>> -                          tr.limit < 0x67, EXC_GP, 0);
>> +    generate_exception_if(tr.attr.fields.type != 0xb, EXC_GP, 0);
>>  
>> -    if ( (rc = read_ulong(x86_seg_none, tr.base + 0x66,
>> -                          &iobmp, 2, ctxt, ops)) )
>> +    switch ( rc = read_ulong(x86_seg_tr, 0x66, &iobmp, 2, ctxt, ops) )
>> +    {
>> +    case X86EMUL_OKAY:
>> +        break;
>> +
>> +    case X86EMUL_EXCEPTION:
>> +        if ( !ctxt->event_pending )
>> +            generate_exception_if(true, EXC_GP, 0);
> generate_exception_if(!ctxt->event_pending, EXC_GP, 0) ?

Already noticed and fixed in v2.

>
>> @@ -1471,9 +1490,12 @@ protmode_load_seg(
>>      {
>>          uint32_t new_desc_b = desc.b | a_flag;
>>  
>> -        if ( (rc = ops->cmpxchg(x86_seg_none, desctab.base + (sel & 0xfff8) 
>> + 4,
>> -                                &desc.b, &new_desc_b, 4, ctxt)) != 0 )
>> +        if ( (rc = ops->cmpxchg(sel_seg, (sel & 0xfff8) + 4, &desc.b,
>> +                                &new_desc_b, 4, ctxt) != X86EMUL_OKAY) )
>> +        {
>> +            ASSERT(rc != X86EMUL_EXCEPTION);
> Hmm, now that I look at this again I don't think it's right: Why did
> we think there can't be any exception here?

Hmm.  I cant remember either.

> What if the descriptor table page is write protected?

Architecturally, a #PF is raised.

> Or page tables have been changed behind our back after the earlier read?

Currently nothing because ops->cmpxchg() doesn't have atomic or xchg
propertied.

I will drop the assertion, because it is definitely wrong for the
pagefault case.

~Andrew

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