Re: [PATCH v5] x86/vmx: add hvm functions to get/set non-register state

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Apr 27, 2022 at 3:07 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 27.04.2022 05:46, Tian, Kevin wrote:
> >> From: Lengyel, Tamas <tamas.lengyel@xxxxxxxxx>
> >> Sent: Friday, March 25, 2022 9:33 PM
> >>
> >> 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 adding a new pair of hvm functions to get/set the guest
> >> non-register state so that the overall vCPU state remains in sync.
> >>
> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> >
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> with ...
>
> >> @@ -863,6 +892,17 @@ static inline void hvm_set_reg(struct vcpu *v,
> >> unsigned int reg, uint64_t val)
> >>      ASSERT_UNREACHABLE();
> >>  }
> >>
> >> +static inline void hvm_get_nonreg_state(struct vcpu *v,
> >> +                                        struct hvm_vcpu_nonreg_state *nrs)
> >> +{
> >> +    ASSERT_UNREACHABLE();
> >> +}
> >> +static inline void hvm_set_nonreg_state(struct vcpu *v,
> >> +                                        struct hvm_vcpu_nonreg_state *nrs)
> >> +{
> >> +    ASSERT_UNREACHABLE();
> >> +}
>
> ... these unnecessary stubs dropped (they should be introduced only
> once actually needed, i.e. when a caller appears in a file which is
> also built when !CONFIG_HVM), and ...
>
> >> --- a/xen/arch/x86/mm/mem_sharing.c
> >> +++ b/xen/arch/x86/mm/mem_sharing.c
> >> @@ -1643,6 +1643,13 @@ static int bring_up_vcpus(struct domain *cd,
> >> struct domain *d)
> >>      return 0;
> >>  }
> >>
> >> +static void copy_vcpu_nonreg_state(struct vcpu *d_vcpu, struct vcpu
> >> *cd_vcpu)
> >> +{
> >> +    struct hvm_vcpu_nonreg_state nrs = {};
> >> +    hvm_get_nonreg_state(d_vcpu, &nrs);
>
> ... this missing blank line inserted between these two lines. I'll
> make both adjustments while committing.

Thanks, both changes are fine from my side.

Tamas