During VM forking and resetting a failed vmentry has been observed due
to the guest non-register state going out-of-sync with the guest register
state. For example, a VM fork reset right after a STI instruction can trigger
the failed entry. This is due to the guest non-register state not being saved
from the parent VM, thus the reset operation only copies the register state.

Fix this by including the guest non-register state in hvm_hw_cpu so that when
its copied from the parent VM the vCPU state remains in sync.

SVM is not currently wired-in as VM forking is VMX only and saving non-register
state during normal save/restore/migration operation hasn't been needed.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
v2: Include all CPU non-register state and fold the ops into vmx_vmcs_save &
    vmx_vmcs_restore.
Note: no sanity checking is performed on the fields to reduce the cycles during
      fuzzing.
---
 xen/arch/x86/hvm/vmx/vmx.c             | 13 ++++++++++++-
 xen/include/public/arch-x86/hvm/save.h |  6 ++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c075370f64..4d4dcc4b70 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -713,7 +713,7 @@ static void vmx_restore_dr(struct vcpu *v)
 
 static void vmx_vmcs_save(struct vcpu *v, struct hvm_hw_cpu *c)
 {
-    unsigned long ev;
+    unsigned long ev, activity_state, intr_state;
 
     vmx_vmcs_enter(v);
 
@@ -721,6 +721,10 @@ static void vmx_vmcs_save(struct vcpu *v, struct hvm_hw_cpu *c)
     __vmread(GUEST_SYSENTER_ESP, &c->sysenter_esp);
     __vmread(GUEST_SYSENTER_EIP, &c->sysenter_eip);
 
+    __vmread(GUEST_ACTIVITY_STATE, &activity_state);
+    __vmread(GUEST_INTERRUPTIBILITY_INFO, &intr_state);
+    __vmread(GUEST_PENDING_DBG_EXCEPTIONS, &c->pending_dbg);
+
     __vmread(VM_ENTRY_INTR_INFO, &ev);
     if ( (ev & INTR_INFO_VALID_MASK) &&
          hvm_event_needs_reinjection(MASK_EXTR(ev, INTR_INFO_INTR_TYPE_MASK),
@@ -732,6 +736,9 @@ static void vmx_vmcs_save(struct vcpu *v, struct hvm_hw_cpu *c)
     }
 
     vmx_vmcs_exit(v);
+
+    c->activity_state = activity_state;
+    c->interruptibility_state = intr_state;
 }
 
 static int vmx_restore_cr0_cr3(
@@ -807,6 +814,10 @@ static int vmx_vmcs_restore(struct vcpu *v, struct hvm_hw_cpu *c)
 
     __vmwrite(GUEST_DR7, c->dr7);
 
+    __vmwrite(GUEST_ACTIVITY_STATE, c->activity_state);
+    __vmwrite(GUEST_INTERRUPTIBILITY_INFO, c->interruptibility_state);
+    __vmwrite(GUEST_PENDING_DBG_EXCEPTIONS, c->pending_dbg);
+
     if ( c->pending_valid &&
          hvm_event_needs_reinjection(c->pending_type, c->pending_vector) )
     {
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 773a380bc2..eb72e44968 100644
--- 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 {
@@ -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;
 };
 
 struct hvm_hw_cpu_compat {
-- 
2.25.1

On 17.03.2022 16:57, Tamas K Lengyel wrote:
> During VM forking and resetting a failed vmentry has been observed due
> to the guest non-register state going out-of-sync with the guest register
> state. For example, a VM fork reset right after a STI instruction can trigger
> the failed entry. This is due to the guest non-register state not being saved
> from the parent VM, thus the reset operation only copies the register state.
> 
> Fix this by including the guest non-register state in hvm_hw_cpu so that when
> its copied from the parent VM the vCPU state remains in sync.
> 
> SVM is not currently wired-in as VM forking is VMX only and saving non-register
> state during normal save/restore/migration operation hasn't been needed.

Given that it was pointed out that e.g. STI- and MOV-SS-shadow aren't
VMX specific and also aren't impossible to hit with ordinary save /
restore / migrate, I'm not convinced of this argumentation. But of
course fixing VMX alone is better than nothing. However, ...

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

Jan

On Mon, Mar 21, 2022 at 8:16 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 17.03.2022 16:57, Tamas K Lengyel wrote:
> > During VM forking and resetting a failed vmentry has been observed due
> > to the guest non-register state going out-of-sync with the guest register
> > state. For example, a VM fork reset right after a STI instruction can trigger
> > the failed entry. This is due to the guest non-register state not being saved
> > from the parent VM, thus the reset operation only copies the register state.
> >
> > Fix this by including the guest non-register state in hvm_hw_cpu so that when
> > its copied from the parent VM the vCPU state remains in sync.
> >
> > SVM is not currently wired-in as VM forking is VMX only and saving non-register
> > state during normal save/restore/migration operation hasn't been needed.
>
> Given that it was pointed out that e.g. STI- and MOV-SS-shadow aren't
> VMX specific and also aren't impossible to hit with ordinary save /
> restore / migrate, I'm not convinced of this argumentation. But of
> course fixing VMX alone is better than nothing. However, ...
>
> > @@ -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. 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?

Tamas

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

Jan

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

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

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