Re: [XEN][PATCH] x86/hvm: move save/restore under HVM_SAVE_RESTORE config

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2025 00:00, Grygorii Strashko wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> 
> Introduce separate HVM_SAVE_RESTORE config for HVM save/restore feature,
> which is enabled by default for HVM and depends on MGMT_HYPERCALLS config.
> 
> This allows to make MGMT_HYPERCALLS specific changes more granular and, if
> required, make HVM save/restore optional, selectable feature.
> 
> Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> ---
> I'd like to propose this patch as a replacement of Patch 19 [1]
> 
> [1] 
> https://patchwork.kernel.org/project/xen-devel/patch/20251121105801.1251262-20-Penny.Zheng@xxxxxxx/
> 
>  xen/arch/x86/cpu/mcheck/vmce.c       | 4 ++--
>  xen/arch/x86/emul-i8254.c            | 4 +++-
>  xen/arch/x86/hvm/Kconfig             | 6 ++++++
>  xen/arch/x86/hvm/Makefile            | 2 +-
>  xen/arch/x86/hvm/hpet.c              | 3 ++-
>  xen/arch/x86/hvm/hvm.c               | 4 ++++
>  xen/arch/x86/hvm/irq.c               | 2 ++
>  xen/arch/x86/hvm/mtrr.c              | 2 ++
>  xen/arch/x86/hvm/pmtimer.c           | 2 ++
>  xen/arch/x86/hvm/rtc.c               | 2 ++
>  xen/arch/x86/hvm/vioapic.c           | 2 ++
>  xen/arch/x86/hvm/viridian/viridian.c | 2 ++
>  xen/arch/x86/hvm/vlapic.c            | 3 ++-
>  xen/arch/x86/hvm/vpic.c              | 2 ++
>  xen/arch/x86/include/asm/hvm/save.h  | 5 ++++-
>  15 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
> index 1a7e92506ac8..ba27f6f8bd91 100644
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -349,7 +349,7 @@ int vmce_wrmsr(uint32_t msr, uint64_t val)
>      return ret;
>  }
>  
> -#if CONFIG_HVM
> +#if defined(CONFIG_HVM_SAVE_RESTORE)

#if wasn't really correct to use here; #ifdef was and is wanted.

>  static int cf_check vmce_save_vcpu_ctxt(struct vcpu *v, hvm_domain_context_t 
> *h)
>  {
>      struct hvm_vmce_vcpu ctxt = {
> @@ -380,10 +380,10 @@ static int cf_check vmce_load_vcpu_ctxt(struct domain 
> *d, hvm_domain_context_t *
>  
>      return err ?: vmce_restore_vcpu(v, &ctxt);
>  }
> +#endif /* CONFIG_HVM_SAVE_RESTORE */
>  
>  HVM_REGISTER_SAVE_RESTORE(VMCE_VCPU, vmce_save_vcpu_ctxt, NULL,
>                            vmce_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
> -#endif

Why would this #endif move? (It gaining a comment is fine of course.)

> --- a/xen/arch/x86/emul-i8254.c
> +++ b/xen/arch/x86/emul-i8254.c
> @@ -409,7 +409,9 @@ void pit_stop_channel0_irq(PITState *pit)
>      destroy_periodic_time(&pit->pt0);
>      spin_unlock(&pit->lock);
>  }
> +#endif
>  
> +#if defined(CONFIG_HVM_SAVE_RESTORE)

Hmm, again - please use the shorter #ifdef.

> @@ -507,9 +509,9 @@ static int cf_check pit_load(struct domain *d, 
> hvm_domain_context_t *h)
>  
>      return rc;
>  }
> +#endif /* CONFIG_HVM_SAVE_RESTORE */
>  
>  HVM_REGISTER_SAVE_RESTORE(PIT, pit_save, pit_check, pit_load, 1, 
> HVMSR_PER_DOM);
> -#endif

And again - why move it?

> --- a/xen/arch/x86/hvm/Kconfig
> +++ b/xen/arch/x86/hvm/Kconfig
> @@ -93,4 +93,10 @@ config MEM_SHARING
>       depends on INTEL_VMX
>       depends on MGMT_HYPERCALLS
>  
> +config HVM_SAVE_RESTORE
> +     depends on MGMT_HYPERCALLS
> +     def_bool y
> +     help
> +       Enables HVM save/load functionality.
> +
>  endif

This wants to move up some imo; MEM_SHARING is clearing the more niche feature.

> --- a/xen/arch/x86/include/asm/hvm/save.h
> +++ b/xen/arch/x86/include/asm/hvm/save.h
> @@ -123,6 +123,7 @@ void hvm_register_savevm(uint16_t typecode,
>  
>  /* Syntactic sugar around that function: specify the max number of
>   * saves, and this calculates the size of buffer needed */
> +#ifdef CONFIG_HVM_SAVE_RESTORE
>  #define HVM_REGISTER_SAVE_RESTORE(_x, _save, check, _load, _num, _k)      \
>  static int __init cf_check __hvm_register_##_x##_save_and_restore(void)   \
>  {                                                                         \
> @@ -137,7 +138,9 @@ static int __init cf_check 
> __hvm_register_##_x##_save_and_restore(void)   \
>      return 0;                                                             \
>  }                                                                         \
>  __initcall(__hvm_register_##_x##_save_and_restore);
> -
> +#else
> +#define HVM_REGISTER_SAVE_RESTORE(_x, _save, check, _load, _num, _k)
> +#endif

By suitably moving the #endif-s I would hope we can get away without this
dummy #define.

Jan