Re: [Xen-devel] [PATCH v4 08/20] x86emul: abstract out XCRn accesses

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.18 at 16:56, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 28/02/18 13:03, Jan Beulich wrote:
>> @@ -5178,18 +5202,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'm still opposed to this change.  It is inconsistent with all other
> handling in the emulator, because we do not do input register validation
> for any of the CR/DR/MSR hooks.
> 
> The {read,write}_xcr() hooks should be required to deal with any
> arbitrary register, just like the {read,write}_{cr,dr,msr}() hooks are
> currently expected to do.

And I continue to not follow you here: None of the %crN's
existence is controlled by any CPUID flags, hence a check like the
above one would not be possible there. If anything, I could see
the core emulator filtering out %cr1, %cr5-%cr7, and from %cr9
upwards (and similarly for %drN, at which point we could also
centralize the [non-]aliasing of %dr4/%dr5 onto %dr6/%dr7).
The fundamental idea behind the check above (and such a possible
%crN/%drN related change) being to keep in a single place all
checks which are mandated by the architecture.

MSRs (already by their spelled out name, even if that has become
a misnomer quite quickly after their introduction) are a bit different,
and hence I'm less convinced core RDMSR/WRMSR emulation
should go through such hoops when the callbacks will have to
normally have a big switch() statement anyway.

But yes, if the only way forward here is to move the checks into
the individual callbacks, I'll have no choice. Please clarify.

Jan


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