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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/12/16 08:32, Jan Beulich wrote:
>>>> 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().

Ok.

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

You made it convincing argument in c/s 043ad80 "x86: always supply
.cpuid() handler to x86_emulate()", as to why the ->cpuid() hook should
be expected.

Especially if we are going to be adding more CPUID checks going forward,
I would take this opportunity to make it mandatory.

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

Ok.  How about some comment indicating "bail early if we won't be able
to complete the operation" ?

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

Fair point.

> Could you live with it staying here and an ASSERT() being added right ahead 
> of the use?

Yes, or indeed just a comment indicating why the check is early.  After
all, its still in the same emulation block and we can reasonably expect
people to consider the block as a whole.

~Andrew

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