On 26.11.2025 00:00, Grygorii Strashko wrote:
> From: Grygorii Strashko <[email protected]>
>
> 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 <[email protected]>
> ---
> I'd like to propose this patch as a replacement of Patch 19 [1]
>
> [1]
> https://patchwork.kernel.org/project/xen-devel/patch/[email protected]/
>
> 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
