Re: [PATCH v2] x86/vmx: save guest non-register state in hvm_hw_cpu

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.03.2022 16:28, Tamas K Lengyel wrote:
> On Mon, Mar 21, 2022 at 10:58 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 21.03.2022 15:39, Tamas K Lengyel wrote:
>>> On Mon, Mar 21, 2022 at 8:16 AM Jan Beulich <jbeulich@xxxxxxxx> 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
>