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

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

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):

--- 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