Linux-HyperV List
 help / color / mirror / Atom feed
* [PATCH 1/1] mshv: Add conditional VMBus dependency
@ 2026-05-21 16:49 Michael Kelley
  2026-05-21 20:15 ` Arnd Bergmann
  2026-05-21 21:19 ` Jork Loeser
  0 siblings, 2 replies; 5+ messages in thread
From: Michael Kelley @ 2026-05-21 16:49 UTC (permalink / raw)
  To: kys, haiyangz, wei.liu, decui, longli, jloeser, linux-hyperv
  Cc: linux-kernel, arnd, hamzamahfooz

From: Michael Kelley <mhklinux@outlook.com>

When the VMBus driver is not part of the kernel (CONFIG_HYPERV_VMBUS=n),
the MSHV root driver fails to link:

ERROR: modpost: "hv_vmbus_exists" [drivers/hv/mshv_root.ko] undefined!

Fix this while meeting these requirements:
* It must be possible to include the MSHV root driver without the
  VMBus driver. In such case, the MSHV root driver can be built-in
  to the kernel image, or it can be built as a separate module.
* If both the MSHV root driver and the VMBus driver are present, the
  MSHV root driver and VMBus driver can both be built-in, or they can
  both be separate modules. Or the MSHV root driver can be a module
  while the VMBus driver can be built-in, but the reverse is
  disallowed. Regardless of the build choices, the VMBus driver must
  be loaded before the MSHV driver in order for the SynIC to be
  managed properly (see comments in the MSHV SynIC code).

The fix has two parts:
* Add a Kconfig entry for MSHV_ROOT to depend on HYPERV_VMBUS if
  HYPERV_VMBUS is present. The entry disallows MSHV_ROOT being
  built-in when HYPERV_VMBUS is a module, but without requiring that
  HYPERV_VMBUS be built.
* Add #ifdefs around MSHV SynIC calls to hv_vmbus_exists(). When
  the VMBus driver is present, these calls establish a module
  dependency to ensure that the VMBus driver loads first when both
  are built as modules. But if the VMBus driver is not present,
  the behavior is as if hv_vmbus_exists() returned "false", and
  there is no module dependency.

Existing code ensures that the VMBus driver loads first if it is
built-in. The VMBus driver uses subsys_initcall(), which is
initcall level 4. The MSHV root driver uses module_init(), which
becomes device_init() when built-in, and device_init() is
initcall level 6.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Closes: https://lore.kernel.org/all/20260520074044.923728-1-arnd@kernel.org/
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/hv/Kconfig      |  1 +
 drivers/hv/mshv_synic.c | 12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index 2d0b3fcb0ff8..aa11bcefddf2 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -74,6 +74,7 @@ config MSHV_ROOT
 	# e.g. When withdrawing memory, the hypervisor gives back 4k pages in
 	# no particular order, making it impossible to reassemble larger pages
 	depends on PAGE_SIZE_4KB
+	depends on HYPERV_VMBUS if HYPERV_VMBUS
 	select EVENTFD
 	select VIRT_XFER_TO_GUEST_WORK
 	select HMM_MIRROR
diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
index 88170ce6b83f..3f72a3dd232d 100644
--- a/drivers/hv/mshv_synic.c
+++ b/drivers/hv/mshv_synic.c
@@ -463,11 +463,15 @@ static int mshv_synic_cpu_init(unsigned int cpu)
 			&spages->synic_event_flags_page;
 	struct hv_synic_event_ring_page **event_ring_page =
 			&spages->synic_event_ring_page;
+	bool vmbus_active = false;
+
 	/*
 	 * VMBus owns SIMP/SIEFP/SCONTROL when it is active.
 	 * See hv_hyp_synic_enable_regs() for that initialization.
 	 */
-	bool vmbus_active = hv_vmbus_exists();
+#if IS_ENABLED(CONFIG_HYPERV_VMBUS)
+	vmbus_active = hv_vmbus_exists();
+#endif
 
 	/*
 	 * Map the SYNIC message page. When VMBus is not active the
@@ -587,8 +591,12 @@ static int mshv_synic_cpu_exit(unsigned int cpu)
 		&spages->synic_event_flags_page;
 	struct hv_synic_event_ring_page **event_ring_page =
 		&spages->synic_event_ring_page;
+	bool vmbus_active = false;
+
 	/* VMBus owns SIMP/SIEFP/SCONTROL when it is active */
-	bool vmbus_active = hv_vmbus_exists();
+#if IS_ENABLED(CONFIG_HYPERV_VMBUS)
+	vmbus_active = hv_vmbus_exists();
+#endif
 
 	/* Disable the interrupt */
 	sint.as_uint64 = hv_get_non_nested_msr(HV_MSR_SINT0 + HV_SYNIC_INTERCEPTION_SINT_INDEX);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] mshv: Add conditional VMBus dependency
  2026-05-21 16:49 [PATCH 1/1] mshv: Add conditional VMBus dependency Michael Kelley
@ 2026-05-21 20:15 ` Arnd Bergmann
  2026-05-21 21:17   ` Michael Kelley
  2026-05-21 21:19 ` Jork Loeser
  1 sibling, 1 reply; 5+ messages in thread
From: Arnd Bergmann @ 2026-05-21 20:15 UTC (permalink / raw)
  To: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
	Dexuan Cui, longli, Jork Loeser, linux-hyperv
  Cc: linux-kernel, hamzamahfooz

On Thu, May 21, 2026, at 18:49, Michael Kelley wrote:
>
> Existing code ensures that the VMBus driver loads first if it is
> built-in. The VMBus driver uses subsys_initcall(), which is
> initcall level 4. The MSHV root driver uses module_init(), which
> becomes device_init() when built-in, and device_init() is
> initcall level 6.
>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Closes: https://lore.kernel.org/all/20260520074044.923728-1-arnd@kernel.org/
> Signed-off-by: Michael Kelley <mhklinux@outlook.com>

Looks good to me, thanks for fixing it!

Acked-by: Arnd Bergmann <arnd@arndb.de>

>  	/*
>  	 * VMBus owns SIMP/SIEFP/SCONTROL when it is active.
>  	 * See hv_hyp_synic_enable_regs() for that initialization.
>  	 */
> -	bool vmbus_active = hv_vmbus_exists();
> +#if IS_ENABLED(CONFIG_HYPERV_VMBUS)
> +	vmbus_active = hv_vmbus_exists();
> +#endif

I would usually write this as 

        if (IS_ENABLED(CONFIG_HYPERV_VMBUS))
                  vmbus_active = hv_vmbus_exists();

for readability, since the hv_vmbus_exists() declarations is still
visible and the IS_ENABLED() check avoids the link failure.

      ARnd

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 1/1] mshv: Add conditional VMBus dependency
  2026-05-21 20:15 ` Arnd Bergmann
@ 2026-05-21 21:17   ` Michael Kelley
  0 siblings, 0 replies; 5+ messages in thread
From: Michael Kelley @ 2026-05-21 21:17 UTC (permalink / raw)
  To: Arnd Bergmann, Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
	Wei Liu, Dexuan Cui, longli@microsoft.com, Jork Loeser,
	linux-hyperv@vger.kernel.org
  Cc: linux-kernel@vger.kernel.org, hamzamahfooz@linux.microsoft.com

From: Arnd Bergmann <arnd@arndb.de> Sent: Thursday, May 21, 2026 1:16 PM
> 
> On Thu, May 21, 2026, at 18:49, Michael Kelley wrote:
> >
> > Existing code ensures that the VMBus driver loads first if it is
> > built-in. The VMBus driver uses subsys_initcall(), which is
> > initcall level 4. The MSHV root driver uses module_init(), which
> > becomes device_init() when built-in, and device_init() is
> > initcall level 6.
> >
> > Reported-by: Arnd Bergmann <arnd@arndb.de>
> > Closes: https://lore.kernel.org/all/20260520074044.923728-1-arnd@kernel.org/
> > Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> 
> Looks good to me, thanks for fixing it!
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> >  	/*
> >  	 * VMBus owns SIMP/SIEFP/SCONTROL when it is active.
> >  	 * See hv_hyp_synic_enable_regs() for that initialization.
> >  	 */
> > -	bool vmbus_active = hv_vmbus_exists();
> > +#if IS_ENABLED(CONFIG_HYPERV_VMBUS)
> > +	vmbus_active = hv_vmbus_exists();
> > +#endif
> 
> I would usually write this as
> 
>         if (IS_ENABLED(CONFIG_HYPERV_VMBUS))
>                   vmbus_active = hv_vmbus_exists();
> 
> for readability, since the hv_vmbus_exists() declarations is still
> visible and the IS_ENABLED() check avoids the link failure.
> 

I thought about doing that, but wasn't sure it would work. There
are nuances of #ifdef vs. #if IS_ENABLED() vs. if (IS_ENABLED())
that I haven't learned. :-(

I'll wait a few days to see if any comments come in from Jork
Jork or other MSFT folks, and then spin a v2 with your change
so the cleaner version is what goes upstream.

Thanks!

Michael

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] mshv: Add conditional VMBus dependency
  2026-05-21 16:49 [PATCH 1/1] mshv: Add conditional VMBus dependency Michael Kelley
  2026-05-21 20:15 ` Arnd Bergmann
@ 2026-05-21 21:19 ` Jork Loeser
  2026-05-21 21:41   ` Michael Kelley
  1 sibling, 1 reply; 5+ messages in thread
From: Jork Loeser @ 2026-05-21 21:19 UTC (permalink / raw)
  To: mhklinux
  Cc: kys, haiyangz, wei.liu, decui, longli, linux-hyperv, linux-kernel,
	arnd, hamzamahfooz

On Thu, 21 May 2026, Michael Kelley wrote:

> From: Michael Kelley <mhklinux@outlook.com>

> * Add #ifdefs around MSHV SynIC calls to hv_vmbus_exists(). When

Could as well do an empty definition of hv_vmbus_exists() if VMBUS is not 
configured, no?

> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index 2d0b3fcb0ff8..aa11bcefddf2 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -74,6 +74,7 @@ config MSHV_ROOT
> 	# e.g. When withdrawing memory, the hypervisor gives back 4k pages in
> 	# no particular order, making it impossible to reassemble larger pages
> 	depends on PAGE_SIZE_4KB
> +	depends on HYPERV_VMBUS if HYPERV_VMBUS

Nice, thanks!

Reviewed-by: Jork Loeser <jloeser@linux.microsoft.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 1/1] mshv: Add conditional VMBus dependency
  2026-05-21 21:19 ` Jork Loeser
@ 2026-05-21 21:41   ` Michael Kelley
  0 siblings, 0 replies; 5+ messages in thread
From: Michael Kelley @ 2026-05-21 21:41 UTC (permalink / raw)
  To: Jork Loeser
  Cc: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
	decui@microsoft.com, longli@microsoft.com,
	linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
	arnd@arndb.de, hamzamahfooz@linux.microsoft.com

From: Jork Loeser <jloeser@linux.microsoft.com> Sent: Thursday, May 21, 2026 2:20 PM
> 
> On Thu, 21 May 2026, Michael Kelley wrote:
> 
> > From: Michael Kelley <mhklinux@outlook.com>
> 
> > * Add #ifdefs around MSHV SynIC calls to hv_vmbus_exists(). When
> 
> Could as well do an empty definition of hv_vmbus_exists() if VMBUS is not
> configured, no?

Yes, indeed. I would have done that if there were more than 2 places
where hv_vmbus_exists() is called. For me, having exactly 2 places was
on the tipping point of testing CONFIG_HYPERV_VMBUS inline vs.
adding the test in a .h file.

Thinking about it more, I'll try the .h file route in a v2. That way
mshv_synic.c doesn't have to be touched at all.

Michael

> 
> > diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> > index 2d0b3fcb0ff8..aa11bcefddf2 100644
> > --- a/drivers/hv/Kconfig
> > +++ b/drivers/hv/Kconfig
> > @@ -74,6 +74,7 @@ config MSHV_ROOT
> > 	# e.g. When withdrawing memory, the hypervisor gives back 4k pages in
> > 	# no particular order, making it impossible to reassemble larger pages
> > 	depends on PAGE_SIZE_4KB
> > +	depends on HYPERV_VMBUS if HYPERV_VMBUS
> 
> Nice, thanks!
> 
> Reviewed-by: Jork Loeser <jloeser@linux.microsoft.com>


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-05-21 21:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-21 16:49 [PATCH 1/1] mshv: Add conditional VMBus dependency Michael Kelley
2026-05-21 20:15 ` Arnd Bergmann
2026-05-21 21:17   ` Michael Kelley
2026-05-21 21:19 ` Jork Loeser
2026-05-21 21:41   ` Michael Kelley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox