|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 12/25] x86emul: abstract out XCRn accesses
On 07/12/17 14:07, Jan Beulich wrote:
> --- a/tools/tests/x86_emulator/x86-emulate.c
> +++ b/tools/tests/x86_emulator/x86-emulate.c
> @@ -120,6 +120,19 @@ 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;
> +
> + asm ( "xgetbv" : "=a" (lo), "=d" (hi) : "c" (reg) );
> + *val = lo | ((uint64_t)hi << 32);
This will want a reg filter, or AFL will find that trying to read reg 2
will explode.
> +
> + return X86EMUL_OKAY;
> +}
> +
> 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
> @@ -1825,6 +1825,49 @@ static int hvmemul_write_cr(
> return rc;
> }
>
> +static int hvmemul_read_xcr(
> + unsigned int reg,
> + uint64_t *val,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + uint32_t lo, hi;
> +
> + switch ( reg )
> + {
> + case 0:
> + *val = current->arch.xcr0;
> + return X86EMUL_OKAY;
> +
> + case 1:
> + if ( !cpu_has_xgetbv1 )
> + return X86EMUL_UNHANDLEABLE;
> + break;
> +
> + default:
> + return X86EMUL_UNHANDLEABLE;
> + }
> +
> + asm ( ".byte 0x0f,0x01,0xd0" /* xgetbv */
> + : "=a" (lo), "=d" (hi) : "c" (reg) );
Please can we have a static inline? It needs to be volatile, because
the result depends on unspecified other operations, which for xgetbv1
includes any instruction which alters xsave state.
Furthermore, does this actually return the correct result? I'd prefer
if we didn't have to read from hardware here, but I can't see an
alternative.
From the guests point of view, we should at least have the guests xcr0
in context, but we have xcr0_accum loaded, meaning that the guest is
liable to see returned set bits which are higher than its idea of xcr0.
> + *val = lo | ((uint64_t)hi << 32);
> + HVMTRACE_LONG_2D(XCR_READ, reg, TRC_PAR_LONG(*val));
> +
> + return X86EMUL_OKAY;
> +}
> +
> +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));
> + if ( likely(handle_xsetbv(reg, val) == 0) )
> + return X86EMUL_OKAY;
> +
> + x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
> + return X86EMUL_EXCEPTION;
This exception is inconsistent with unhandleable above. FTR, I'd expect
all of them to be exception rather than unhandleable.
> +}
> +
> static int hvmemul_read_msr(
> unsigned int reg,
> uint64_t *val,
> @@ -5161,18 +5182,33 @@ x86_emulate(
> _regs.eflags |= X86_EFLAGS_AC;
> break;
>
> -#ifdef __XEN__
> - case 0xd1: /* xsetbv */
> + case 0xd0: /* xgetbv */
> generate_exception_if(vex.pfx, EXC_UD);
> - if ( !ops->read_cr || ops->read_cr(4, &cr4, ctxt) !=
> X86EMUL_OKAY )
> + 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);
> - generate_exception_if(!mode_ring0() ||
> - handle_xsetbv(_regs.ecx,
> - _regs.eax | (_regs.rdx <<
> 32)),
> + generate_exception_if(_regs.ecx > (vcpu_has_xgetbv1() ? 1 : 0),
> EXC_GP, 0);
I don't think this filtering is correct. We don't filter on the xsetbv
side, or for the plain cr/dr index. It should be up to the hook to
decide whether a specific index is appropriate.
> + 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->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() || _regs.ecx, 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/include/asm-x86/x86-defns.h
> +++ b/xen/include/asm-x86/x86-defns.h
> @@ -66,4 +66,28 @@
> #define X86_CR4_SMAP 0x00200000 /* enable SMAP */
> #define X86_CR4_PKE 0x00400000 /* enable PKE */
>
> +/*
> + * XSTATE component flags in XCR0
> + */
> +#define _XSTATE_FP 0
> +#define XSTATE_FP (1ULL << _XSTATE_FP)
> +#define _XSTATE_SSE 1
> +#define XSTATE_SSE (1ULL << _XSTATE_SSE)
> +#define _XSTATE_YMM 2
> +#define XSTATE_YMM (1ULL << _XSTATE_YMM)
> +#define _XSTATE_BNDREGS 3
> +#define XSTATE_BNDREGS (1ULL << _XSTATE_BNDREGS)
> +#define _XSTATE_BNDCSR 4
> +#define XSTATE_BNDCSR (1ULL << _XSTATE_BNDCSR)
> +#define _XSTATE_OPMASK 5
> +#define XSTATE_OPMASK (1ULL << _XSTATE_OPMASK)
> +#define _XSTATE_ZMM 6
> +#define XSTATE_ZMM (1ULL << _XSTATE_ZMM)
> +#define _XSTATE_HI_ZMM 7
> +#define XSTATE_HI_ZMM (1ULL << _XSTATE_HI_ZMM)
> +#define _XSTATE_PKRU 9
> +#define XSTATE_PKRU (1ULL << _XSTATE_PKRU)
> +#define _XSTATE_LWP 62
> +#define XSTATE_LWP (1ULL << _XSTATE_LWP)
Can we name these consistently as part of moving into this file? At the
very least an X86_ prefix, and possibly an XCR0 middle.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |