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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.11.18 at 19:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 02/11/18 16:46, 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've got most of a series doing this for cpuid, which drops ~4k of .text
> volume from x86_emulate() alone.
> 
> My plan was to get all the architectural state in a directly readable
> form, to reduce the complexity and boilerplate.

I'm not convinced, the more that I don't think you really mean "all"
when you say "all": Surely not the whole set of MSRs or any of
XSTATE?

>  On that subject...
> 
>> @@ -3247,6 +3245,8 @@ x86_emulate(
>>  
>>      ASSERT(ops->read);
>>  
>> +    cr4_rc = ops->read_cr ? ops->read_cr(4, &cr4, ctxt) : X86EMUL_OKAY;
> 
> ... why not make read_cr() mandatory, or put cr4 into ctxt?  Plumbing
> cr4_rc around still feels like a lot of boilerplate.

read_cr() is not currently populated by all paths; see
hvm_emulate_one_mmio() for one example. I don't want to make
the hook mandatory, at least not yet.

Jan



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