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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.12.16 at 14:36, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/12/16 08:32, Jan Beulich wrote:
>> 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.

But not in this patch, no. The more that such a patch then should
also remove all the respective NULL checks. Plus at least
x86_decode_insn() invokes x86_decode() without cpuid hook, so
further care would be needed. Overall I'm not convinced it is a
good idea to close the road for x86_emulate() uses without that
hook - see the "as long as respective instructions may get used in
that case" in the description of the commit you point out.

>>>> @@ -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" ?

I can add a comment, but the wording you suggest doesn't seem
appropriate, due to my further earlier argumentation (it being a
bad idea to update other guest visible machine state). As I now
realize that wouldn't in fact be a problem here, as the instructions
writing to memory we emulate so far don't update other machine
state, but it would still set a bad precedent, as there are others
which do. And I really hope to get at least a good part of SSE and
AVX done by the time 4.9 freezes.

Jan


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