Message-ID:
 <SN6PR02MB4157C9A80037D1D37FCFB56DD45CA@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Tue, 22 Jul 2025 17:43:43 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Roman Kisel <romank@...ux.microsoft.com>, "alok.a.tiwari@...cle.com"
	<alok.a.tiwari@...cle.com>, "arnd@...db.de" <arnd@...db.de>, "bp@...en8.de"
	<bp@...en8.de>, "corbet@....net" <corbet@....net>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"decui@...rosoft.com" <decui@...rosoft.com>, "haiyangz@...rosoft.com"
	<haiyangz@...rosoft.com>, "hpa@...or.com" <hpa@...or.com>,
	"kys@...rosoft.com" <kys@...rosoft.com>, "mingo@...hat.com"
	<mingo@...hat.com>, "rdunlap@...radead.org" <rdunlap@...radead.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "Tianyu.Lan@...rosoft.com"
	<Tianyu.Lan@...rosoft.com>, "wei.liu@...nel.org" <wei.liu@...nel.org>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	"linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>
CC: "apais@...rosoft.com" <apais@...rosoft.com>, "benhill@...rosoft.com"
	<benhill@...rosoft.com>, "bperkins@...rosoft.com" <bperkins@...rosoft.com>,
	"sunilmut@...rosoft.com" <sunilmut@...rosoft.com>
Subject: RE: [PATCH hyperv-next v4 03/16] arch: hyperv: Get/set SynIC
 synth.registers via paravisor

From: Roman Kisel <romank@...ux.microsoft.com> Sent: Monday, July 14, 2025 3:16 PM
> 
> The existing Hyper-V wrappers for getting and setting MSRs are
> hv_get/set_msr(). Via hv_get/set_non_nested_msr(), they detect
> when running in a CoCo VM with a paravisor, and use the TDX or
> SNP guest-host communication protocol to bypass the paravisor
> and go directly to the host hypervisor for SynIC MSRs. The "set"
> function also implements the required special handling for the
> SINT MSRs.
> 
> But in some Confidential VMBus cases, the guest wants to talk only
> with the paravisor. To accomplish this, provide new functions for
> accessing SynICs that always do direct accesses (i.e., not via
> TDX or SNP GHCB), which will go to the paravisor. The mirroring
> of the existing "set" function is also not needed. These functions
> should be used only in the specific Confidential VMBus cases that
> require them.
> 
> Provide functions that allow manipulating the SynIC registers
> through the paravisor.

This last paragraph seems to be a leftover from a previous
version of the commit message. I think it can be deleted since
the second paragraph now covers the topic.

> 
> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
> Reviewed-by: Alok Tiwari <alok.a.tiwari@...cle.com>
> ---
>  arch/x86/kernel/cpu/mshyperv.c | 39 ++++++++++++++++++
>  drivers/hv/hv_common.c         | 13 ++++++
>  include/asm-generic/mshyperv.h | 75 ++++++++++++++++++----------------
>  3 files changed, 92 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index c78f860419d6..07c60231d0d8 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -90,6 +90,45 @@ void hv_set_non_nested_msr(unsigned int reg, u64 value)
>  }
>  EXPORT_SYMBOL_GPL(hv_set_non_nested_msr);
> 
> +/*
> + * Attempt to get the SynIC register value from the paravisor.
> + *
> + * Not all paravisors support reading SynIC registers, so this function
> + * may fail. The register for the SynIC of the running CPU is accessed.
> + *
> + * Writes the SynIC register value into the memory pointed by val,
> + * and ~0ULL is on failure.
> + *
> + * Returns -ENODEV if the MSR is not a SynIC register, or another error
> + * code if getting the MSR fails (meaning the paravisor doesn't support
> + * relaying VMBus communucations).
> + */
> +int hv_para_get_synic_register(unsigned int reg, u64 *val)
> +{
> +	if (!hv_is_synic_msr(reg))
> +		return -ENODEV;
> +	return native_read_msr_safe(reg, val);
> +}
> +
> +/*
> + * Attempt to set the SynIC register value with the paravisor.
> + *
> + * Not all paravisors support setting SynIC registers, so this function
> + * may fail. The register for the SynIC of the running CPU is accessed.
> + *
> + * Sets the register to the value supplied.
> + *
> + * Returns: -ENODEV if the MSR is not a SynIC register, or another error
> + * code if writing to the MSR fails (meaning the paravisor doesn't support
> + * relaying VMBus communucations).
> + */
> +int hv_para_set_synic_register(unsigned int reg, u64 val)
> +{
> +	if (!hv_is_synic_msr(reg))
> +		return -ENODEV;
> +	return native_write_msr_safe(reg, val);
> +}
> +
>  u64 hv_get_msr(unsigned int reg)
>  {
>  	if (hv_nested)
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 49898d10faff..a179ea482cb1 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -716,6 +716,19 @@ u64 __weak hv_tdx_hypercall(u64 control, u64 param1, u64 param2)
>  }
>  EXPORT_SYMBOL_GPL(hv_tdx_hypercall);
> 
> +int __weak hv_para_get_synic_register(unsigned int reg, u64 *val)
> +{
> +	*val = ~0ULL;
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(hv_para_get_synic_register);
> +
> +int __weak hv_para_set_synic_register(unsigned int reg, u64 val)
> +{
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(hv_para_set_synic_register);
> +
>  void hv_identify_partition_type(void)
>  {
>  	/* Assume guest role */
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 9722934a8332..9447558f425b 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -162,41 +162,6 @@ static inline u64 hv_generate_guest_id(u64 kernel_version)
>  	return guest_id;
>  }
> 
> -/* Free the message slot and signal end-of-message if required */
> -static inline void vmbus_signal_eom(struct hv_message *msg, u32 old_msg_type)
> -{
> -	/*
> -	 * On crash we're reading some other CPU's message page and we need
> -	 * to be careful: this other CPU may already had cleared the header
> -	 * and the host may already had delivered some other message there.
> -	 * In case we blindly write msg->header.message_type we're going
> -	 * to lose it. We can still lose a message of the same type but
> -	 * we count on the fact that there can only be one
> -	 * CHANNELMSG_UNLOAD_RESPONSE and we don't care about other messages
> -	 * on crash.
> -	 */
> -	if (cmpxchg(&msg->header.message_type, old_msg_type,
> -		    HVMSG_NONE) != old_msg_type)
> -		return;
> -
> -	/*
> -	 * The cmxchg() above does an implicit memory barrier to
> -	 * ensure the write to MessageType (ie set to
> -	 * HVMSG_NONE) happens before we read the
> -	 * MessagePending and EOMing. Otherwise, the EOMing
> -	 * will not deliver any more messages since there is
> -	 * no empty slot
> -	 */
> -	if (msg->header.message_flags.msg_pending) {
> -		/*
> -		 * This will cause message queue rescan to
> -		 * possibly deliver another msg from the
> -		 * hypervisor
> -		 */
> -		hv_set_msr(HV_MSR_EOM, 0);
> -	}
> -}
> -
>  int hv_get_hypervisor_version(union hv_hypervisor_version_info *info);
> 
>  void hv_setup_vmbus_handler(void (*handler)(void));
> @@ -333,6 +298,8 @@ bool hv_is_isolation_supported(void);
>  bool hv_isolation_type_snp(void);
>  u64 hv_ghcb_hypercall(u64 control, void *input, void *output, u32 input_size);
>  u64 hv_tdx_hypercall(u64 control, u64 param1, u64 param2);
> +int hv_para_get_synic_register(unsigned int reg, u64 *val);
> +int hv_para_set_synic_register(unsigned int reg, u64 val);
>  void hyperv_cleanup(void);
>  bool hv_query_ext_cap(u64 cap_query);
>  void hv_setup_dma_ops(struct device *dev, bool coherent);
> @@ -375,6 +342,44 @@ static inline int hv_call_create_vp(int node, u64 partition_id,
> u32 vp_index, u3
>  #endif /* CONFIG_MSHV_ROOT */
>  bool vmbus_is_confidential(void);
> 
> +/* Free the message slot and signal end-of-message if required */
> +static inline void vmbus_signal_eom(struct hv_message *msg, u32 old_msg_type)
> +{
> +	/*
> +	 * On crash we're reading some other CPU's message page and we need
> +	 * to be careful: this other CPU may already had cleared the header
> +	 * and the host may already had delivered some other message there.
> +	 * In case we blindly write msg->header.message_type we're going
> +	 * to lose it. We can still lose a message of the same type but
> +	 * we count on the fact that there can only be one
> +	 * CHANNELMSG_UNLOAD_RESPONSE and we don't care about other messages
> +	 * on crash.
> +	 */
> +	if (cmpxchg(&msg->header.message_type, old_msg_type,
> +		    HVMSG_NONE) != old_msg_type)
> +		return;
> +
> +	/*
> +	 * The cmxchg() above does an implicit memory barrier to
> +	 * ensure the write to MessageType (ie set to
> +	 * HVMSG_NONE) happens before we read the
> +	 * MessagePending and EOMing. Otherwise, the EOMing
> +	 * will not deliver any more messages since there is
> +	 * no empty slot
> +	 */
> +	if (msg->header.message_flags.msg_pending) {
> +		/*
> +		 * This will cause message queue rescan to
> +		 * possibly deliver another msg from the
> +		 * hypervisor
> +		 */
> +		if (vmbus_is_confidential())
> +			hv_para_set_synic_register(HV_MSR_EOM, 0);

As reported by the kernel test robot, this function call is generating an
error because there's no declaration for hv_para_set_synic_register()
when built with !CONFIG_HYPERV.

One solution is to add a stub higher up in this source code file in
the existing !CONFIG_HYPERV section.

But a better solution might be move vmbus_signal_eom() out of this
file entirely and put it in drivers/hv/hyperv_vmbus.h. Then no stub
for !CONFIG_HYPERV is needed. vmbus_signal_eom() is very much
a VMBus function, and it's only called from vmbus_wait_for_unload(),
__vmbus_on_msg_dpc(), and vmbus_message_sched().

The file asm-generic/mshyperv.h should be focused on the Hyper-V
interface, not on VMBus, so it's not clear to me why vmbus_signal_eom()
is in asm-generic/mshyperv.h in the first place. There's no reason to expose
vmbus_signal_eom() to code that needs definitions related to Hyper-V,
such as the x86 MTRR code or the Hyper-V emulation code in KVM
(which is where the kernel test robot detected the error). Note that
drivers/hv/hyperv_vmbus.h will need to add #include <asm/mshyperv.h>
in order for the move to work.

I've tested build with CONFIG_HYPERV and !CONFIG_HYPERV on
x86 and on arm64, and the proposed move seems to work OK.

Sorry my suggestion in v3 of your patch set to handle HV_MSR_EOM
here in vmbus_signal_eom() turned out to be more complicated than
I originally contemplated. :-(

> +		else
> +			hv_set_msr(HV_MSR_EOM, 0);
> +	}
> +}
> +
>  #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
>  u8 __init get_vtl(void);
>  #else
> --
> 2.43.0

Powered by blists - more mailing lists