Re: [Xen-devel] [PATCH v5 03/14] x86emul: abstract out XCRn accesses

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 15 March 2018 13:04
> To: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>
> Subject: [PATCH v5 03/14] x86emul: abstract out XCRn accesses
> 
> Use hooks, just like done for other special purpose registers.
> 
> This includes moving XCR0 checks from hvmemul_get_fpu() to the emulator
> itself as well as adding support for XGETBV emulation.
> 
> For now fuzzer reads will obtain the real values (minus the fuzzing of
> the hook pointer itself).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

hvm/emulate parts...

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> ---
> v5: Move index validation into hook functions. Introduce
>     x86emul_{read,write}_xcr().
> v4: Have hvmemul_read_xcr() raise an exception instead of returning
>     X86EMUL_UNHANDLEABLE for invalid indexes. Introduce xgetbv() and add
>     volatile to the asm() moved there. Split out _XSTATE_* movement.
> v2: Re-base.
> 
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -459,6 +459,8 @@ static int fuzz_write_cr(
>      return X86EMUL_OKAY;
>  }
> 
> +#define fuzz_read_xcr emul_test_read_xcr
> +
>  enum {
>      MSRI_IA32_SYSENTER_CS,
>      MSRI_IA32_SYSENTER_ESP,
> @@ -577,6 +579,7 @@ static const struct x86_emulate_ops all_
>      SET(write_io),
>      SET(read_cr),
>      SET(write_cr),
> +    SET(read_xcr),
>      SET(read_msr),
>      SET(write_msr),
>      SET(wbinvd),
> @@ -685,6 +688,7 @@ enum {
>      HOOK_write_cr,
>      HOOK_read_dr,
>      HOOK_write_dr,
> +    HOOK_read_xcr,
>      HOOK_read_msr,
>      HOOK_write_msr,
>      HOOK_wbinvd,
> @@ -729,6 +733,7 @@ static void disable_hooks(struct x86_emu
>      MAYBE_DISABLE_HOOK(write_io);
>      MAYBE_DISABLE_HOOK(read_cr);
>      MAYBE_DISABLE_HOOK(write_cr);
> +    MAYBE_DISABLE_HOOK(read_xcr);
>      MAYBE_DISABLE_HOOK(read_msr);
>      MAYBE_DISABLE_HOOK(write_msr);
>      MAYBE_DISABLE_HOOK(wbinvd);
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -371,6 +371,7 @@ static struct x86_emulate_ops emulops =
>      .read_segment = read_segment,
>      .cpuid      = emul_test_cpuid,
>      .read_cr    = emul_test_read_cr,
> +    .read_xcr   = emul_test_read_xcr,
>      .read_msr   = read_msr,
>      .get_fpu    = emul_test_get_fpu,
>      .put_fpu    = emul_test_put_fpu,
> --- a/tools/tests/x86_emulator/x86-emulate.c
> +++ b/tools/tests/x86_emulator/x86-emulate.c
> @@ -163,6 +163,35 @@ int emul_test_read_cr(
>      return X86EMUL_UNHANDLEABLE;
>  }
> 
> +int emul_test_read_xcr(
> +    unsigned int reg,
> +    uint64_t *val,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    uint32_t lo, hi;
> +
> +    ASSERT(cpu_has_xsave);
> +
> +    switch ( reg )
> +    {
> +    case 0:
> +        break;
> +
> +    case 1:
> +        if ( cpu_has_xgetbv1 )
> +            break;
> +        /* fall through */
> +    default:
> +        x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
> +        return X86EMUL_EXCEPTION;
> +    }
> +
> +    asm ( "xgetbv" : "=a" (lo), "=d" (hi) : "c" (reg) );
> +    *val = lo | ((uint64_t)hi << 32);
> +
> +    return X86EMUL_OKAY;
> +}
> +
>  int emul_test_get_fpu(
>      void (*exception_callback)(void *, struct cpu_user_regs *),
>      void *exception_callback_arg,
> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -186,6 +186,16 @@ static inline uint64_t xgetbv(uint32_t x
>      (res.b & (1U << 5)) != 0; \
>  })
> 
> +#define cpu_has_xgetbv1 ({ \
> +    struct cpuid_leaf res; \
> +    emul_test_cpuid(1, 0, &res, NULL); \
> +    if ( !(res.c & (1U << 27)) ) \
> +        res.a = 0; \
> +    else \
> +        emul_test_cpuid(0xd, 1, &res, NULL); \
> +    (res.a & (1U << 2)) != 0; \
> +})
> +
>  #define cpu_has_bmi1 ({ \
>      struct cpuid_leaf res; \
>      emul_test_cpuid(7, 0, &res, NULL); \
> @@ -247,6 +257,11 @@ int emul_test_read_cr(
>      unsigned long *val,
>      struct x86_emulate_ctxt *ctxt);
> 
> +int emul_test_read_xcr(
> +    unsigned int reg,
> +    uint64_t *val,
> +    struct x86_emulate_ctxt *ctxt);
> +
>  int emul_test_get_fpu(
>      void (*exception_callback)(void *, struct cpu_user_regs *),
>      void *exception_callback_arg,
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1826,6 +1826,29 @@ static int hvmemul_write_cr(
>      return rc;
>  }
> 
> +static int hvmemul_read_xcr(
> +    unsigned int reg,
> +    uint64_t *val,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    int rc = x86emul_read_xcr(reg, val, ctxt);
> +
> +    if ( rc == X86EMUL_OKAY )
> +        HVMTRACE_LONG_2D(XCR_READ, reg, TRC_PAR_LONG(*val));
> +
> +    return rc;
> +}
> +
> +static int hvmemul_write_xcr(
> +    unsigned int reg,
> +    uint64_t val,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    HVMTRACE_LONG_2D(XCR_WRITE, reg, TRC_PAR_LONG(val));
> +
> +    return x86emul_write_xcr(reg, val, ctxt);
> +}
> +
>  static int hvmemul_read_msr(
>      unsigned int reg,
>      uint64_t *val,
> @@ -1874,22 +1897,6 @@ static int hvmemul_get_fpu(
>  {
>      struct vcpu *curr = current;
> 
> -    switch ( type )
> -    {
> -    case X86EMUL_FPU_fpu:
> -    case X86EMUL_FPU_wait:
> -    case X86EMUL_FPU_mmx:
> -    case X86EMUL_FPU_xmm:
> -        break;
> -    case X86EMUL_FPU_ymm:
> -        if ( !(curr->arch.xcr0 & X86_XCR0_SSE) ||
> -             !(curr->arch.xcr0 & X86_XCR0_YMM) )
> -            return X86EMUL_UNHANDLEABLE;
> -        break;
> -    default:
> -        return X86EMUL_UNHANDLEABLE;
> -    }
> -
>      if ( !curr->fpu_dirtied )
>          hvm_funcs.fpu_dirty_intercept();
>      else if ( type == X86EMUL_FPU_fpu )
> @@ -2073,6 +2080,8 @@ static const struct x86_emulate_ops hvm_
>      .write_io      = hvmemul_write_io,
>      .read_cr       = hvmemul_read_cr,
>      .write_cr      = hvmemul_write_cr,
> +    .read_xcr      = hvmemul_read_xcr,
> +    .write_xcr     = hvmemul_write_xcr,
>      .read_msr      = hvmemul_read_msr,
>      .write_msr     = hvmemul_write_msr,
>      .wbinvd        = hvmemul_wbinvd,
> @@ -2098,6 +2107,8 @@ static const struct x86_emulate_ops hvm_
>      .write_io      = hvmemul_write_io_discard,
>      .read_cr       = hvmemul_read_cr,
>      .write_cr      = hvmemul_write_cr,
> +    .read_xcr      = hvmemul_read_xcr,
> +    .write_xcr     = hvmemul_write_xcr,
>      .read_msr      = hvmemul_read_msr,
>      .write_msr     = hvmemul_write_msr_discard,
>      .wbinvd        = hvmemul_wbinvd_discard,
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -1317,6 +1317,7 @@ static const struct x86_emulate_ops priv
>      .write_cr            = write_cr,
>      .read_dr             = read_dr,
>      .write_dr            = write_dr,
> +    .write_xcr           = x86emul_write_xcr,
>      .read_msr            = read_msr,
>      .write_msr           = write_msr,
>      .cpuid               = pv_emul_cpuid,
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1114,10 +1114,30 @@ static int _get_fpu(
>      struct x86_emulate_ctxt *ctxt,
>      const struct x86_emulate_ops *ops)
>  {
> +    uint64_t xcr0;
>      int rc;
> 
>      fail_if(!ops->get_fpu);
>      ASSERT(type != X86EMUL_FPU_none);
> +
> +    if ( type < X86EMUL_FPU_ymm || !ops->read_xcr ||
> +         ops->read_xcr(0, &xcr0, ctxt) != X86EMUL_OKAY )
> +    {
> +        ASSERT(!ctxt->event_pending);
> +        xcr0 = 0;
> +    }
> +
> +    switch ( type )
> +    {
> +    case X86EMUL_FPU_ymm:
> +        if ( !(xcr0 & X86_XCR0_SSE) || !(xcr0 & X86_XCR0_YMM) )
> +            return X86EMUL_UNHANDLEABLE;
> +        break;
> +
> +    default:
> +        break;
> +    }
> +
>      rc = ops->get_fpu(fpu_handle_exception, fic, type, ctxt);
> 
>      if ( rc == X86EMUL_OKAY )
> @@ -5006,18 +5026,31 @@ x86_emulate(
>                  _regs.eflags |= X86_EFLAGS_AC;
>              break;
> 
> -#ifdef __XEN__
> +        case 0xd0: /* xgetbv */
> +            generate_exception_if(vex.pfx, EXC_UD);
> +            if ( !ops->read_cr || !ops->read_xcr ||
> +                 ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
> +                cr4 = 0;
> +            generate_exception_if(!(cr4 & X86_CR4_OSXSAVE), EXC_UD);
> +            rc = ops->read_xcr(_regs.ecx, &msr_val, ctxt);
> +            if ( rc != X86EMUL_OKAY )
> +                goto done;
> +            _regs.r(ax) = (uint32_t)msr_val;
> +            _regs.r(dx) = msr_val >> 32;
> +            break;
> +
>          case 0xd1: /* xsetbv */
>              generate_exception_if(vex.pfx, EXC_UD);
> -            if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) != 
> X86EMUL_OKAY )
> +            if ( !ops->read_cr || !ops->write_xcr ||
> +                 ops->read_cr(4, &cr4, ctxt) != X86EMUL_OKAY )
>                  cr4 = 0;
>              generate_exception_if(!(cr4 & X86_CR4_OSXSAVE), EXC_UD);
> -            generate_exception_if(!mode_ring0() ||
> -                                  handle_xsetbv(_regs.ecx,
> -                                                _regs.eax | (_regs.rdx << 
> 32)),
> -                                  EXC_GP, 0);
> +            generate_exception_if(!mode_ring0(), EXC_GP, 0);
> +            rc = ops->write_xcr(_regs.ecx,
> +                                _regs.eax | ((uint64_t)_regs.edx << 32), 
> ctxt);
> +            if ( rc != X86EMUL_OKAY )
> +                goto done;
>              break;
> -#endif
> 
>          case 0xd4: /* vmfunc */
>              generate_exception_if(vex.pfx, EXC_UD);
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -398,6 +398,24 @@ struct x86_emulate_ops
>          struct x86_emulate_ctxt *ctxt);
> 
>      /*
> +     * read_xcr: Read from extended control register.
> +     *  @reg:   [IN ] Register to read.
> +     */
> +    int (*read_xcr)(
> +        unsigned int reg,
> +        uint64_t *val,
> +        struct x86_emulate_ctxt *ctxt);
> +
> +    /*
> +     * write_xcr: Write to extended control register.
> +     *  @reg:   [IN ] Register to write.
> +     */
> +    int (*write_xcr)(
> +        unsigned int reg,
> +        uint64_t val,
> +        struct x86_emulate_ctxt *ctxt);
> +
> +    /*
>       * read_msr: Read from model-specific register.
>       *  @reg:   [IN ] Register to read.
>       */
> @@ -683,6 +701,11 @@ static inline void x86_emulate_free_stat
>  void x86_emulate_free_state(struct x86_emulate_state *state);
>  #endif
> 
> +int x86emul_read_xcr(unsigned int reg, uint64_t *val,
> +                     struct x86_emulate_ctxt *ctxt);
> +int x86emul_write_xcr(unsigned int reg, uint64_t val,
> +                      struct x86_emulate_ctxt *ctxt);
> +
>  #endif
> 
>  static inline void x86_emul_hw_exception(
> --- a/xen/arch/x86/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate.c
> @@ -42,3 +42,50 @@
>  })
> 
>  #include "x86_emulate/x86_emulate.c"
> +
> +int x86emul_read_xcr(unsigned int reg, uint64_t *val,
> +                     struct x86_emulate_ctxt *ctxt)
> +{
> +    switch ( reg )
> +    {
> +    case 0:
> +        *val = current->arch.xcr0;
> +        return X86EMUL_OKAY;
> +
> +    case 1:
> +        if ( cpu_has_xgetbv1 && current->domain->arch.cpuid->xstate.xgetbv1
> )
> +            break;
> +        /* fall through */
> +    default:
> +        x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
> +        return X86EMUL_EXCEPTION;
> +    }
> +
> +    *val = xgetbv(reg);
> +
> +    return X86EMUL_OKAY;
> +}
> +
> +int x86emul_write_xcr(unsigned int reg, uint64_t val,
> +                      struct x86_emulate_ctxt *ctxt)
> +{
> +    switch ( reg )
> +    {
> +    case 0:
> +        break;
> +
> +    case 1:
> +        if ( cpu_has_xgetbv1 && current->domain->arch.cpuid->xstate.xgetbv1
> )
> +            break;
> +        /* fall through */
> +    default:
> +    gp_fault:
> +        x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
> +        return X86EMUL_EXCEPTION;
> +    }
> +
> +    if ( unlikely(handle_xsetbv(reg, val) != 0) )
> +        goto gp_fault;
> +
> +    return X86EMUL_OKAY;
> +}
> --- a/xen/include/asm-x86/hvm/trace.h
> +++ b/xen/include/asm-x86/hvm/trace.h
> @@ -33,6 +33,8 @@
>  #define DO_TRC_HVM_CR_WRITE64  DEFAULT_HVM_REGACCESS
>  #define DO_TRC_HVM_DR_READ     DEFAULT_HVM_REGACCESS
>  #define DO_TRC_HVM_DR_WRITE    DEFAULT_HVM_REGACCESS
> +#define DO_TRC_HVM_XCR_READ64  DEFAULT_HVM_REGACCESS
> +#define DO_TRC_HVM_XCR_WRITE64 DEFAULT_HVM_REGACCESS
>  #define DO_TRC_HVM_MSR_READ    DEFAULT_HVM_REGACCESS
>  #define DO_TRC_HVM_MSR_WRITE   DEFAULT_HVM_REGACCESS
>  #define DO_TRC_HVM_RDTSC       DEFAULT_HVM_REGACCESS
> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -109,6 +109,17 @@ int xstate_alloc_save_area(struct vcpu *
>  void xstate_init(struct cpuinfo_x86 *c);
>  unsigned int xstate_ctxt_size(u64 xcr0);
> 
> +static inline uint64_t xgetbv(unsigned int index)
> +{
> +    uint32_t lo, hi;
> +
> +    ASSERT(index); /* get_xcr0() should be used instead. */
> +    asm volatile ( ".byte 0x0f,0x01,0xd0" /* xgetbv */
> +                   : "=a" (lo), "=d" (hi) : "c" (index) );
> +
> +    return lo | ((uint64_t)hi << 32);
> +}
> +
>  static inline bool xstate_all(const struct vcpu *v)
>  {
>      /*
> --- a/xen/include/public/trace.h
> +++ b/xen/include/public/trace.h
> @@ -235,6 +235,8 @@
>  #define TRC_HVM_TRAP             (TRC_HVM_HANDLER + 0x23)
>  #define TRC_HVM_TRAP_DEBUG       (TRC_HVM_HANDLER + 0x24)
>  #define TRC_HVM_VLAPIC           (TRC_HVM_HANDLER + 0x25)
> +#define TRC_HVM_XCR_READ64      (TRC_HVM_HANDLER + TRC_64_FLAG +
> 0x26)
> +#define TRC_HVM_XCR_WRITE64     (TRC_HVM_HANDLER + TRC_64_FLAG
> + 0x27)
> 
>  #define TRC_HVM_IOPORT_WRITE    (TRC_HVM_HANDLER + 0x216)
>  #define TRC_HVM_IOMEM_WRITE     (TRC_HVM_HANDLER + 0x217)
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel