Re: [PATCH v9 1/2] x86emul: New return code for unimplemented instruction

[Petre Ovidiu PIRCALABU <ppircalabu@bitdefender.com>]

On Vi, 2017-09-01 at 04:33 -0600, Jan Beulich wrote:
> >
> > >
> > > >
> > > > On 30.08.17 at 20:57, <ppircalabu@bitdefender.com> wrote:
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -2044,6 +2044,7 @@ int hvm_emulate_one_mmio(unsigned long mfn,
> > unsigned long gla)
> Way earlier in this file there are consumers of X86EMUL_UNHANDLEABLE
> in hvmemul_do_io() and hvmemul_do_io_buffer(). I'm sure I did
> point this out before, and I cannot see why you don't adjust those as
> well, and you also don't say anything in this regard in the
> description.
> Similarly there's a consumer in hvm_process_io_intercept() (in a file
> you don't touch at all). The use in hvm_broadcast_ioreq() is likely
> fine, but I'd still like you to reason about that in the description.

My mistake. I have added my comments in the cover letter as I thought
they will be easier to read. I will add them the the patch description
for the next iteration.

In my opinion, hvm_process_io_intercept cannot return
X86EMUL_UNIMPLEMENTED. The return value of this function depends on
either the return code of one of the hvm_io_ops handlers (read/write)
or the value returned by hvm_copy_guest_from_phys/
hvm_copy_to_guest_phys.
In case of the latter, the function crashes the domain and
returns X86EMUL_UNHANDLEABLE if the result is neither HVMCOPY_okay
nor HVMCOPY_bad_gfn_to_mfn.
hvm_op_ops should not return X86EMUL_UNIMPLEMENTED as this return
code's usage should strictly be limited to the core emulator and only
when an instruction opcode is invalid.

hvmemul_do_io - X86EMUL_UNHANDLEABLE is tested against the return code
of hvm_io_intercept which can return X86EMUL_UNHANDLEABLE only if the
IO handler was not found or it was returned by hvm_process_io_intercept
(cannot return X86EMUL_UNIMPLEMENTED).
In this case I think adding the X86EMUL_UNIMPLEMENTED check would mask
a possible misuse of this return code instead of just triggering BUG()
in the early stages of development.

hvm_send_buffered_ioreq - currently returns X86EMUL_UNHANDLEABLE if:
- the buffered iopage is NULL.
- the ioreq size if invalid
- the queue is full.
In none of these cases the function cannot return X86EMUL_UNIMPLEMENTED
because they are not related to the instruction opcode decoding, so
this function cannot return X86EMUL_UNIMPLEMENTED.

hvm_send_ioreq - can return X86EMUL_UNHANDLEABLE only if the current
vcpu in not the hvm_iorec_server's list or the value if returned from
hvm_send_buffered_ioreq, hence this function also cannot return
X86EMUL_UNIMPLEMENTED.

hvm_broadcast_ioreq - calls hvm_send_ioreq and chvemul_do_iohecks the
return code against X86EMUL_UNHANDLEABLE. There is no need to extend
the check to handle X86EMUL_UNIMPLEMENTED because this value cannot be
returned by hvm_send_ioreq.

hvmemul_do_io_buffer - calls hvmemul_do_io and, if the return code is
X86EMUL_UNHANDLEABLE and dir is IOREQ_READ, sets the return buffer to
0xFF.
The return value of hvemul_do_io can be:
- return value of hvm_io_intercept (cannot be X86EMUL_UNIMPLEMENTED),
- return value of hvm_process_io_intercept (cannot be
X86EMUL_UNIMPLEMENTED)
- return value of hvm_send_ioreq(cannot be X86EMUL_UNIMPLEMENTED)
- X86EMUL_RETRY / X86EMUL_OKAY (depending on state)
The condition also should not be extended to take into account
X86EMUL_UNIMPLEMENTED because this value cannot be returned by
hvemul_do_io.

> >
> > @@ -5177,7 +5177,7 @@ x86_emulate(
> >                  goto done;
> >              break;
> >          default:
> > -            goto cannot_emulate;
> > +            goto unimplemented_insn;
> While I can see why you do this change, for many/all of the ones
> I'll leave in context below I think you rather want to switch to
> generate_exception(EXC_UD).
Some of the opcodes are valid but not supported by the emulator. In
this case X86EMUL_UNIMPLEMENTED should be returned to allow the monitor
app to handle this case. Also, in the worst case scenario, when the
opcode doesn't correspond to a valid x86(-64) instruction, if the
monitor app for example tries to single-step it on the real hardware an
UD exception will also be reported.
>
> >
> > @@ -6176,7 +6176,7 @@ x86_emulate(
> >                  /* vpsll{w,d} $imm8,{x,y}mm,{x,y}mm */
> >              break;
> >          default:
> > -            goto cannot_emulate;
> > +            goto unimplemented_insn;
> >          }
> >
> > > >      simd_0f_shift_imm:
> >          generate_exception_if(ea.type != OP_REG, EXC_UD);
> > @@ -6224,7 +6224,7 @@ x86_emulate(
> >          case 6: /* psllq $imm8,mm */
> >              goto simd_0f_shift_imm;
> >          }
> > -        goto cannot_emulate;
> > +        goto unimplemented_insn;
> >
> >      case X86EMUL_OPC_66(0x0f, 0x73):
> >      case X86EMUL_OPC_VEX_66(0x0f, 0x73):
> > @@ -6240,7 +6240,7 @@ x86_emulate(
> >                  /* vpslldq $imm8,{x,y}mm,{x,y}mm */
> >              goto simd_0f_shift_imm;
> >          }
> > -        goto cannot_emulate;
> > +        goto unimplemented_insn;
> >
> >      case X86EMUL_OPC(0x0f, 0x77):        /* emms */
> >      case X86EMUL_OPC_VEX(0x0f, 0x77):    /* vzero{all,upper} */
> > @@ -6304,7 +6304,7 @@ x86_emulate(
> >          case 0: /* extrq $imm8,$imm8,xmm */
> >              break;
> >          default:
> > -            goto cannot_emulate;
> > +            goto unimplemented_insn;
> >          }
> >          /* fall through */
> >      case X86EMUL_OPC_F2(0x0f, 0x78):     /* insertq
> > $imm8,$imm8,xmm,xmm */
> > @@ -6515,7 +6515,7 @@ x86_emulate(
> A few lines up from here is another instance you appear to have
> missed.
>
> >
> > @@ -7341,7 +7341,7 @@ x86_emulate(
> >              host_and_vcpu_must_have(bmi1);
> >              break;
> >          default:
> > -            goto cannot_emulate;
> > +            goto unimplemented_insn;
> >          }
> >
> >          generate_exception_if(vex.l, EXC_UD);
> > @@ -7650,7 +7650,7 @@ x86_emulate(
> >              host_and_vcpu_must_have(tbm);
> >              break;
> >          default:
> > -            goto cannot_emulate;
> > +            goto unimplemented_insn;
> >          }
> >
> >      xop_09_rm_rv:
> > @@ -7684,7 +7684,7 @@ x86_emulate(
> >              host_and_vcpu_must_have(tbm);
> >              goto xop_09_rm_rv;
> >          }
> > -        goto cannot_emulate;
> > +        goto unimplemented_insn;
> >
> >      case X86EMUL_OPC_XOP(0a, 0x10): /* bextr imm,r/m,r */
> >      {
> > @@ -7716,6 +7716,9 @@ x86_emulate(
> >      }
> >
Thanks for noticing it. I will change it back to cannot_emulate as
there are no other valid instructions for this opcodes.

> >      default:
> > +    unimplemented_insn:
> > +        rc = X86EMUL_UNIMPLEMENTED;
> > +        goto done;
> >      cannot_emulate:
> >          rc = X86EMUL_UNHANDLEABLE;
> >          goto done;
> I guess the cannot_emulate label would then better be moved
> elsewhere (either out of the switch or to a place where one of
> the goto-s to it currently sits).
I will change it in the new iteration of the patch.

> Jan
>
>
> ________________________
> This email was scanned by Bitdefender

Many thanks for your patience,
//Petre

________________________
This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel