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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
> --- 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() ?

>          switch ( (rc = ops->cmpxchg(sel_seg, (sel & 0xfff8) + 4, &desc.b,
>                                      &new_desc_b, sizeof(desc.b), ctxt)) )
>          {
> @@ -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.

> +        }
>          break;
>      }
>  
> @@ -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.

>          dst.type = OP_REG;
>          dst.bytes = (mode_64bit() && (op_bytes == 4)) ? 8 : op_bytes;
>          dst.reg = (unsigned long *)&_regs.ebp;
> @@ -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.

~Andrew

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