Re: [PATCH v3 12/25] x86emul: abstract out XCRn accesses

["Jan Beulich" <JBeulich@suse.com>]

>>> On 02.02.18 at 14:29, <andrew.cooper3@citrix.com> wrote:
> 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.

How would AFL manage to do that? It doesn't fuzz the function
alone, and there's no call path leading here that would pass an
invalid value. It is the main emulator that should never call this
with a wrong value, or if it does, we should be happy for it to
be flagged by AFL rather than going silent (via some error path).

Plus - if I wanted to add proper checking here, I'd have to re-do
exactly what the emulator code around the call site does.

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

Sure.

>  It needs to be volatile, because
> the result depends on unspecified other operations, which for xgetbv1
> includes any instruction which alters xsave state.

Well, yes, strictly speaking it should be volatile. Will add.

> 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.

In what way do you see it possibly producing the wrong value?

> 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.

Nor would it make much sense to cache a dozen or more XCRs,
once there'll be that many.

>> +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.

I can switch to that, sure.

>> @@ -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.

Any filtering that can be done here should be done here - this
is the central place to enforce architectural dependencies. I'd
rather add a similar check to xsetbv; in fact I'm not sure why I
didn't.

>> --- 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.

Well, yes, if you insist I can add another patch doing this - doing
it here would be insane, as I'll need to update all users. As to
"XCR0 middle" do you mean in place of XSTATE, or in addition to
(I'd prefer the former)?

Jan

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