Re: [PATCH v6 0/3] Add support for clean shutdown with MSHV

[Praveen K Paladugu <praveenkpaladugu@gmail.com>]

On 11/29/2025 12:48 AM, Michael Kelley wrote:
> From: Praveen K Paladugu <prapal@linux.microsoft.com> Sent: Wednesday, November 26, 2025 1:50 PM
>>
>> Add support for clean shutdown of the root partition when running on
>> MSHV Hypervisor.
>>
>> v6:
>>   - Fixed build errors, by adding CONFIG_X86_64 guard
> 
> Adding the CONFIG_X86_64 guard seems like the right solution, and it does
> make the build errors go away. However note that as coded in drivers/hv/Makefile,
>   the code under the new guard won't be built at all unless CONFIG_MSHV_ROOT
> is set (ignoring the VTL case for now), which can only happen in the X86_64 or
> ARM64 cases. So it was nagging at me as to why the guard is needed for an
> x86 32-bit build failure reported by the kernel test robot.
> 
> It turns out there's an underlying bug in drivers/hv/Makefile causing
> mshv_common.o to be built in cases when it shouldn't be, such as the x86
> 32-bit case. The build failures reported by the kernel test robot were on these
> cases when it shouldn't be built in the first place. The bug is in this Makefile line:
> 
> ifneq ($(CONFIG_MSHV_ROOT) $(CONFIG_MSHV_VTL),)
> 
> which should be
> 
> ifneq ($(CONFIG_MSHV_ROOT)$(CONFIG_MSHV_VTL),)
> 
> The buggy version has a spurious "space" character before the start of
> $(CONFIG_MSHV_VTL) such that the result is always "not equal" and
> mshv_common.o is always built.
> 
> If the Makefile is fixed, then the X86_64 guards you added in
> mshv_common.c are not needed. Furthermore, the stubs for
> hv_sleep_notifiers_register() and hv_machine_power_off() in
> arch/x86/include/asm/mshyperv.h for the !CONFIG_X86_64 case aren't
> needed. And the declarations for hv_sleep_notifiers_register() and
> hv_machine_power_off() can be moved out from under the #ifdef
> CONFIG_X86_64. The bottom line is that nothing in this patch set needs
> to be guarded by CONFIG_X86_64.
> 
Thanks for this investigation Michael. With these changes, I am not able
to reproduce any build failures anymore. I will push these changes in v7.

Praveen

> Here's a quick diff of what I changed on top of your v6 patch set
> (including the fix to drivers/hv/Makefile). I tested the build process
> on both x86/64 and arm64, with and without CONFIG_MSHV_ROOT
> selected.
> 
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 4c22f3257368..01d192e70211 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -178,16 +178,15 @@ int hyperv_fill_flush_guest_mapping_list(
>                  struct hv_guest_mapping_flush_list *flush,
>                  u64 start_gfn, u64 end_gfn);
> 
> +void hv_sleep_notifiers_register(void);
> +void hv_machine_power_off(void);
> +
>   #ifdef CONFIG_X86_64
>   void hv_apic_init(void);
>   void __init hv_init_spinlocks(void);
>   bool hv_vcpu_is_preempted(int vcpu);
> -void hv_sleep_notifiers_register(void);
> -void hv_machine_power_off(void);
>   #else
>   static inline void hv_apic_init(void) {}
> -static inline void hv_sleep_notifiers_register(void) {};
> -static inline void hv_machine_power_off(void) {};
>   #endif
> 
>   struct irq_domain *hv_create_pci_msi_domain(void);
> diff --git a/drivers/hv/Makefile b/drivers/hv/Makefile
> index 58b8d07639f3..6d929fb0e13d 100644
> --- a/drivers/hv/Makefile
> +++ b/drivers/hv/Makefile
> @@ -20,6 +20,6 @@ mshv_vtl-y := mshv_vtl_main.o
>   # Code that must be built-in
>   obj-$(CONFIG_HYPERV) += hv_common.o
>   obj-$(subst m,y,$(CONFIG_MSHV_ROOT)) += hv_proc.o
> -ifneq ($(CONFIG_MSHV_ROOT) $(CONFIG_MSHV_VTL),)
> +ifneq ($(CONFIG_MSHV_ROOT)$(CONFIG_MSHV_VTL),)
>          obj-y += mshv_common.o
>   endif
> diff --git a/drivers/hv/mshv_common.c b/drivers/hv/mshv_common.c
> index 28905e3ed9c0..73505cbdc324 100644
> --- a/drivers/hv/mshv_common.c
> +++ b/drivers/hv/mshv_common.c
> @@ -142,7 +142,6 @@ int hv_call_get_partition_property(u64 partition_id,
>   }
>   EXPORT_SYMBOL_GPL(hv_call_get_partition_property);
> 
> -#ifdef CONFIG_X86_64
>   /*
>    * Corresponding sleep states have to be initialized in order for a subsequent
>    * HVCALL_ENTER_SLEEP_STATE call to succeed. Currently only S5 state as per
> @@ -235,4 +234,3 @@ void hv_machine_power_off(void)
>          local_irq_restore(flags);
> 
>   }
> -#endif
> 
> The Makefile fix needs to be a separate patch.
> 
> I think I got all this correct, but please double-check my work! :-)
> 
> Michael
> 
>>   - Moved machine_ops hook definition to ms_hyperv_init_platform
>>   - Addressed review comments in v5
>>
>> v5:
>>   - Fixed build errors
>>   - Padded struct hv_input_set_system_property for alignment
>>   - Dropped CONFIG_ACPI stub
>>
>> v4:
>>   - Adopted machine_ops to order invoking HV_ENTER_SLEEP_STATE as the
>>     last step in shutdown sequence.
>>   - This ensures rest of the cleanups are done before powering off
>>
>> v3:
>>   - Dropped acpi_sleep handlers as they are not used on mshv
>>   - Applied ordering for hv_reboot_notifier
>>   - Fixed build issues on i386, arm64 architectures
>>
>> v2:
>>    - Addressed review comments from v1.
>>    - Moved all sleep state handling methods under CONFIG_ACPI stub
>>    - - This fixes build issues on non-x86 architectures.
>>
>>
>> Praveen K Paladugu (3):
>>    hyperv: Add definitions for MSHV sleep state configuration
>>    hyperv: Use reboot notifier to configure sleep state
>>    hyperv: Cleanly shutdown root partition with MSHV
>>
>>   arch/x86/hyperv/hv_init.c       |  1 +
>>   arch/x86/include/asm/mshyperv.h |  4 ++
>>   arch/x86/kernel/cpu/mshyperv.c  |  2 +
>>   drivers/hv/mshv_common.c        | 98 +++++++++++++++++++++++++++++++++
>>   include/hyperv/hvgdk_mini.h     |  4 +-
>>   include/hyperv/hvhdk_mini.h     | 40 ++++++++++++++
>>   6 files changed, 148 insertions(+), 1 deletion(-)
>>
>> --
>> 2.51.0
>>
> 
> 

-- 
Regards,
Praveen K Paladugu