On 21.03.2022 16:28, Tamas K Lengyel wrote:
> On Mon, Mar 21, 2022 at 10:58 AM Jan Beulich <jbeul...@suse.com> wrote:
>> On 21.03.2022 15:39, Tamas K Lengyel wrote:
>>> On Mon, Mar 21, 2022 at 8:16 AM Jan Beulich <jbeul...@suse.com> wrote:
>>>> On 17.03.2022 16:57, Tamas K Lengyel wrote:
>>>>> @@ -166,6 +167,11 @@ struct hvm_hw_cpu {
>>>>> #define XEN_X86_FPU_INITIALISED (1U<<_XEN_X86_FPU_INITIALISED)
>>>>> uint32_t flags;
>>>>> uint32_t pad0;
>>>>> +
>>>>> + /* non-register state */
>>>>> + uint32_t activity_state;
>>>>> + uint32_t interruptibility_state;
>>>>> + uint64_t pending_dbg;
>>>>> };
>>>>
>>>> ... these fields now represent vendor state in a supposedly vendor
>>>> independent structure. Besides my wish to see this represented in
>>>> field naming (thus at least making provisions for SVM to gain
>>>> similar support; perhaps easiest would be to include these in a
>>>> sub-structure with a field name of "vmx"), I wonder in how far cross-
>>>> vendor migration was taken into consideration. As long as the fields
>>>> are zero / ignored, things wouldn't be worse than before your
>>>> change, but I think it wants spelling out that the SVM counterpart(s)
>>>> may not be added by way of making a VMX/SVM union.
>>>
>>> I wasn't aware cross-vendor migration is even a thing.
>>
>> It used to be a thing long ago; it may not work right now for no-one
>> testing it.
>>
>>> But adding a
>>> vmx sub-structure seems to me a workable route, we would perhaps just
>>> need an extra field that specifies where the fields were taken
>>> (vmx/svm) and ignore them if the place where the restore is taking
>>> place doesn't match where the save happened. That would be equivalent
>>> to how migration works today. Thoughts?
>>
>> I don't think such a field is needed, at least not right away, as
>> long as the respectively other vendor's fields are left zero when
>> storing the data. These fields being zero matches current behavior
>> of not communicating the values at all. A separate flag might be
>> needed if the receiving side would want to "emulate" settings from
>> incoming values from the respectively other vendor. Yet even then
>> only one of the two sets of fields could potentially be non-zero
>> (both being non-zero is an error imo); both fields being zero
>> would be compatible both ways. Hence it would be possible to
>> determine the source vendor without an extra field even then, I
>> would think.
>>
>> A separate flag would of course be needed if we meant to overlay
>> the vendors' data in a union. But as per my earlier reply I think
>> we're better off not using a union in this case.
>
> Right, both structs being non-zero at the same time wouldn't make
> sense and would indicate corruption of the hvm save file. But I think
> the same would easily be achieved by defining a bit on the flags and
> then a union. If two vendor bits are set that would indicate an error
> without taking up the same with two separate structs. This is what I
> have right now and IMHO it looks good
> (https://xenbits.xen.org/gitweb/?p=people/tklengyel/xen.git;a=commitdiff;h=84f15b2e1bef6c901bbdf29a07c7904cb365c0b2):
Yeah, why not. With the separate flag all should be fine.
Jan
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -52,6 +52,7 @@ DECLARE_HVM_SAVE_TYPE(HEADER, 1, struct hvm_save_header);
> * Compat:
> * - Pre-3.4 didn't have msr_tsc_aux
> * - Pre-4.7 didn't have fpu_initialised
> + * - Pre-4.17 didn't have non-register state
> */
>
> struct hvm_hw_cpu {
> @@ -163,9 +164,21 @@ struct hvm_hw_cpu {
> uint32_t error_code;
>
> #define _XEN_X86_FPU_INITIALISED 0
> +#define _XEN_X86_VMX 1
> #define XEN_X86_FPU_INITIALISED (1U<<_XEN_X86_FPU_INITIALISED)
> +#define XEN_X86_VMX (1U<<_XEN_X86_VMX)
> uint32_t flags;
> uint32_t pad0;
> +
> + /* non-register state */
> + union {
> + /* if flags & XEN_X86_VMX */
> + struct {
> + uint32_t activity_state;
> + uint32_t interruptibility_info;
> + uint64_t pending_dbg;
> + } vmx;
> + };
> };
>
> Tamas
>
