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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e5ebd6f3-0422-485c-87b7-fd9c6e271c18@gmail.com>
Date: Fri, 5 Dec 2025 13:58:15 -0600
From: Praveen K Paladugu <praveenkpaladugu@...il.com>
To: Michael Kelley <mhklinux@...look.com>,
 Praveen K Paladugu <prapal@...ux.microsoft.com>,
 "kys@...rosoft.com" <kys@...rosoft.com>,
 "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>,
 "wei.liu@...nel.org" <wei.liu@...nel.org>,
 "decui@...rosoft.com" <decui@...rosoft.com>,
 "tglx@...utronix.de" <tglx@...utronix.de>,
 "mingo@...hat.com" <mingo@...hat.com>,
 "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "bp@...en8.de" <bp@...en8.de>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
 "arnd@...db.de" <arnd@...db.de>
Cc: "anbelski@...ux.microsoft.com" <anbelski@...ux.microsoft.com>,
 "easwar.hariharan@...ux.microsoft.com"
 <easwar.hariharan@...ux.microsoft.com>,
 "nunodasneves@...ux.microsoft.com" <nunodasneves@...ux.microsoft.com>,
 "skinsburskii@...ux.microsoft.com" <skinsburskii@...ux.microsoft.com>
Subject: Re: [PATCH v6 0/3] Add support for clean shutdown with MSHV



On 11/29/2025 12:48 AM, Michael Kelley wrote:
> From: Praveen K Paladugu <prapal@...ux.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