Re: [Xen-devel] [PATCH v2 3/3] x86emul: consolidate CR4 handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.12.18 at 20:37, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 06/11/2018 13:45, Jan Beulich wrote:
>> Now that there's an almost unconditional CR4 read right at the beginning
>> of x86_emulate(), centralize its reading there and use result and value
>> everywhere else without further invoking the hook.
>>
>> Subsequently we may want to consider having the callers provide
>> whichever value they deem appropriate in their contexts, to avoid
>> invoking the hook altogether for this purpose.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I'm afraid that I am still unconvinced that cr4_rc is a clever move.
> 
> It would be far more simple to require callers to provide it in
> x86_emulate_ctxt.

I'll look into this another time, but I'm afraid this will result in a
behavioral change in particular for the callers which currently
don't supply a read_cr hook. Furthermore I'd then question
the purpose of that hook altogether: To be consistent, we then
should pass in all CRs. At which point the question arises
whether DRs and MSRs and XCRn shouldn't also be passed
in, instead of getting obtained via callbacks.

>> @@ -4000,13 +4000,10 @@ x86_emulate(
>>          if ( (_regs.eflags & X86_EFLAGS_VM) &&
>>               MASK_EXTR(_regs.eflags, X86_EFLAGS_IOPL) != 3 )
>>          {
>> -            cr4 = 0;
>> -            if ( op_bytes == 2 && ops->read_cr )
>> -            {
>> -                rc = ops->read_cr(4, &cr4, ctxt);
>> -                if ( rc != X86EMUL_OKAY )
>> -                    goto done;
>> -            }
>> +            if ( op_bytes == 2 )
>> +                check_cr4();
>> +            else
>> +                cr4 = 0;
> 
> This clobbers cr4, which is a latent bug if any of the retire logic
> wants to start using the value.

Yes, I did realize this when writing this code, but at this point
I can't see any such case (at least for the particular instructions
where such clobbering does happen), even if taking a purely
abstract perspective. Hence I'm considering this acceptable.

> This code is only like this because you've overloaded the value in CR4
> to include an implicit "opsize == 16", and results in exceedingly
> complicated logic to follow.

"Exceedingly complicated" is a matter of perception - adding
several more op_bytes checks is what looked to me to
complicate / obscure things more than would be desirable.

Jan



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