Re: [PATCH v2 2/3] xen/hvm: introduce a fpu_uninitialised field to the CPU save record

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/11/15 10:51, Roger Pau Monné wrote:
> El 17/11/15 a les 19.02, Andrew Cooper ha escrit:
>> On 17/11/15 18:44, Roger Pau Monne wrote:
>>> Introduce a new filed to signal if the FPU has been initialised or not. Xen
>> field
>>
>>> needs this new filed in order to know whether to set the FPU as initialised
>>> or not during restore of CPU context. Previously Xen always wrongly assumed
>>> the FPU was initialised on restore.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> Changes since v1:
>>>  - Don't add yet another compat structure, new fields should always be added
>>>    to the end of the existing structure and offsetof should be used to
>>>    compare sizes.
>>>  - Leave the previous compat structure as-is, since the field was not added
>>>    to the end we cannot remove it and use offsetof in this case.
>> How can this work?
>>
>> Making it zeroextended means that any short record will be padded with
>> zeroes.  As a result, the compat checking logic is skipped.
>>
>> (This HVM_SAVE_* infrastructure is truly horrifying code which should
>> never have been accepted.  I think I have correctly followed what it is
>> doing, but I could be mistaken.)
> No, you are completely right, it was an oversight on my side. So neither
> hvm_load_entry or hvm_load_entry_zeroextend will do what I want/need.
> The options I see so far are:
>
>  - Make hvm_hw_cpu_compat hvm_hw_cpu minus the fpu_initalised field
> (this means dropping support for Xen pre-3.4).

Sorry - not an option.

>
>  - Rewrite part of the hvm_load_entry_zeroextend so that it will load
> records with "len < expected len" and still call the fixup function.
>
> TBH the mess with the hvm_hw_cpu_compat structure is very bad. Adding
> the msr_tsc_aux in the middle of the structure makes all the compat
> handling much more complicated that what it needs to be.

The inclusion of msr_tsc_aux in its current location was a very
regrettable mistake.


As for the problem at hand, I don't see what was wrong with v1.

Fundamentally, we have three different variations of the same structure;
two of which require special compat handling.  Pretending otherwise is
just silly.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel