Re: [Xen-devel] [PATCH 2/4] x86emul: centralize put_fpu() invocations

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/03/17 11:03, Jan Beulich wrote:
> ..., splitting parts of it into check_*() macros. This is in
> preparation of making ->put_fpu() do further adjustments to register
> state. (Some of the check_xmm() invocations could be avoided, as in
> some of the cases no insns handled there can actually raise #XM, but I
> think we're better off keeping them to avoid later additions of further
> insn patterns rendering the lack of the check a bug.)
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -937,6 +937,7 @@ do {
>  
>  struct fpu_insn_ctxt {
>      uint8_t insn_bytes;
> +    uint8_t type;
>      int8_t exn_raised;
>  };
>  
> @@ -956,8 +957,6 @@ static int _get_fpu(
>  {
>      int rc;
>  
> -    fic->exn_raised = -1;
> -
>      fail_if(!ops->get_fpu);
>      rc = ops->get_fpu(fpu_handle_exception, fic, type, ctxt);
>  
> @@ -965,6 +964,8 @@ static int _get_fpu(
>      {
>          unsigned long cr0;
>  
> +        fic->type = type;
> +
>          fail_if(!ops->read_cr);
>          if ( type >= X86EMUL_FPU_xmm )
>          {
> @@ -1006,22 +1007,31 @@ do {
>      rc = _get_fpu(_type, _fic, ctxt, ops);                      \
>      if ( rc ) goto done;                                        \
>  } while (0)
> -#define _put_fpu()                                              \
> +
> +#define check_fpu(_fic)                                         \
>  do {                                                            \
> -    if ( ops->put_fpu != NULL )                                 \
> -        (ops->put_fpu)(ctxt);                                   \
> +    generate_exception_if((_fic)->exn_raised >= 0,              \
> +                          (_fic)->exn_raised);                  \
>  } while (0)
> -#define put_fpu(_fic)                                           \
> +
> +#define check_xmm(_fic)                                         \
>  do {                                                            \
> -    _put_fpu();                                                 \
>      if ( (_fic)->exn_raised == EXC_XM && ops->read_cr &&        \
>           ops->read_cr(4, &cr4, ctxt) == X86EMUL_OKAY &&         \
>           !(cr4 & X86_CR4_OSXMMEXCPT) )                               \
>          (_fic)->exn_raised = EXC_UD;                            \
> -    generate_exception_if((_fic)->exn_raised >= 0,              \
> -                          (_fic)->exn_raised);                  \
> +    check_fpu(_fic);                                            \
>  } while (0)

These two check macros seem like they should have an _exn() suffix to
make it clearer what they are actually doing.

>  
> +static void put_fpu(
> +    struct fpu_insn_ctxt *fic,
> +    struct x86_emulate_ctxt *ctxt,
> +    const struct x86_emulate_ops *ops)
> +{
> +    if ( fic->type != X86EMUL_FPU_none && ops->put_fpu )
> +        ops->put_fpu(ctxt);
> +}
> +
>  static inline bool fpu_check_write(void)
>  {
>      uint16_t fsw;
> @@ -7905,7 +7912,8 @@ x86_emulate(
>      ctxt->regs->eflags &= ~X86_EFLAGS_RF;
>  
>   done:
> -    _put_fpu();
> +    if ( rc != X86EMUL_OKAY )
> +        put_fpu(&fic, ctxt, ops);

Unless you assert at complete_insn that rc == X86EMUL_OKAY, you still
might call put_fpu() twice.

You can avoid the conditional here by setting fic->type to
X86EMUL_FPU_none at the end of put_fpu(), which looks like it is
compatible with your later modifications.

>      put_stub(stub);
>      return rc;
>  #undef state
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -161,7 +161,9 @@ enum x86_emulate_fpu_type {
>      X86EMUL_FPU_wait, /* WAIT/FWAIT instruction */
>      X86EMUL_FPU_mmx, /* MMX instruction set (%mm0-%mm7) */
>      X86EMUL_FPU_xmm, /* SSE instruction set (%xmm0-%xmm7/15) */
> -    X86EMUL_FPU_ymm  /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
> +    X86EMUL_FPU_ymm, /* AVX/XOP instruction set (%ymm0-%ymm7/15) */
> +    /* This sentinel will never be passed to ->get_fpu(). */

Don't you mean ->put_fpu() here?  Either way, can we assert this fact?

~Andrew

> +    X86EMUL_FPU_none
>  };
>  
>  struct cpuid_leaf
>
>


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