Re: [Xen-devel] [PATCH v3 4/5] x86emul: make write and cmpxchg hooks optional

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.12.16 at 18:34, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 06/12/16 11:15, Jan Beulich wrote:
>> While the read and fetch hooks are basically unavoidable, write and
>> cmpxchg aren't really needed by that many insns.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> As a corollary, please can we gain
> 
> ASSERT(ops->read && ops->fetch && ops->cpuid)
> 
> at the start of x86_emulate/decode to make it rather more obvious that
> these are required.  This bit me while developing the AFL harness.

Well, not exactly: The ->read hok formally isn't required by the
decoder, so I'd prefer to assert its presence on x86_emulate(). And
I don't see why the ->cpuid() hook would be required all of the
sudden - all its uses are guarded by a NULL check.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -1492,6 +1492,8 @@ protmode_load_seg(
>>      {
>>          uint32_t new_desc_b = desc.b | a_flag;
>>  
>> +        if ( !ops->cmpxchg )
>> +            return X86EMUL_UNHANDLEABLE;
> 
> Any reason this isn't a fail_if() ?

Right, it certainly can be made one.

>> @@ -2624,13 +2626,18 @@ x86_emulate(
>>          }
>>          else if ( !(d & Mov) ) /* optimisation - avoid slow emulated read */
>>          {
>> +            fail_if(lock_prefix ? !ops->cmpxchg : !ops->write);
>>              if ( (rc = read_ulong(dst.mem.seg, dst.mem.off,
>>                                    &dst.val, dst.bytes, ctxt, ops)) )
>>                  goto done;
>>              dst.orig_val = dst.val;
>>          }
>> -        else /* Lock prefix is allowed only on RMW instructions. */
>> +        else
>> +        {
>> +            /* Lock prefix is allowed only on RMW instructions. */
>>              generate_exception_if(lock_prefix, EXC_UD);
>> +            fail_if(!ops->write);
> 
> I am not sure that these two new fail_if()'s are sensibly placed here,
> remote from the use of the hooks they are protecting against.

Well - I don't see a point continuing the emulation attempt in that
case. They're being duplicated in the writeback code already
anyway, for safety reasons.

>> @@ -3334,6 +3343,7 @@ x86_emulate(
>>          uint8_t depth = imm2 & 31;
>>          int i;
>>  
>> +        fail_if(!ops->write);
> 
> This would be slighly better moved down by 3 lines to be adjacent to the
> first ->write call.

I can certainly do this, but ...

>> @@ -4707,6 +4724,8 @@ x86_emulate(
>>              if ( !(b & 1) )
>>                  rc = ops->read(ea.mem.seg, ea.mem.off+0, mmvalp,
>>                                 ea.bytes, ctxt);
>> +            else
>> +                fail_if(!ops->write);
> 
> Again, this wants moving closer to the ->write call.
> 
> I don't think we need to worry about partially-emulated instructions
> which fail due to a lack of write.  Anything we get wrong like that will
> be obvious.

... I'm rather hesitant to do what you ask for here: I'm of the
opinion that we shouldn't alter machine state (MMX/XMM/YMM
registers) in that case. Could you live with it staying here and
an ASSERT() being added right ahead of the use?

Jan


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