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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <SN6PR02MB415792702B78B9855485DCE5D4DDA@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Sat, 29 Nov 2025 06:48:36 +0000
From: Michael Kelley <mhklinux@...look.com>
To: 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

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.

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
>