* RE: [PATCH hyperv-next v3 00/15] Confidential VMBus
From: Michael Kelley @ 2025-06-18 16:13 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-1-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:43 PM
>
> The guests running on Hyper-V can be confidential where the memory and the
> register content are encrypted, provided that the hardware supports that
> (currently support AMD SEV-SNP and Intel TDX is implemented) and the guest
> is capable of using these features. The confidential guests cannot be
> introspected by the host nor the hypervisor without the guest sharing the
> memory contents upon doing which the memory is decrypted.
>
> In the confidential guests, neither the host nor the hypervisor need to be
> trusted, and the guests processing sensitive data can take advantage of that.
>
> Not trusting the host and the hypervisor (removing them from the Trusted
> Computing Base aka TCB) ncessitates that the method of communication
> between the host and the guest be changed. Below there is the breakdown of
> the options used in the both cases (in the diagrams below the server is
> marked as S, the client is marked as C):
>
> 1. Without the paravisoor the devices are connected to the host, and the
> host provides the device emulation or translation to the guest:
>
> +---- GUEST ----+ +----- DEVICE ----+ +----- HOST -----+
> | | | | | |
> | | | | | |
> | | | ========== |
> | | | | | |
> | | | | | |
> | | | | | |
> +----- C -------+ +-----------------+ +------- S ------+
> || ||
> || ||
> +------||------------------ VMBus --------------------------||------+
> | Interrupts, MMIO |
> +-------------------------------------------------------------------+
>
> 2. With the paravisor, the devices are connected to the paravisor, and
> the paravisor provides the device emulation or translation to the guest.
> The guest doesn't communicate with the host directly, and the guest
> communicates with the paravisor via the VMBus. The host is not trusted
> in this model, and the paravisor is trusted:
>
> +---- GUEST --------------- VTL0 ------+ +-- DEVICE --+
> | | | |
> | +- PARAVISOR --------- VTL2 -----+ | | |
> | | +-- VMBus Relay ------+ ====+================ |
> | | | Interrupts, MMIO | | | | |
> | | +-------- S ----------+ | | +------------+
> | | || | |
> | +---------+ || | |
> | | Linux | || OpenHCL | |
> | | kernel | || | |
> | +---- C --+-----||---------------+ |
> | || || |
> +-------++------- C -------------------+ +------------+
> || | HOST |
> || +---- S -----+
> +-------||----------------- VMBus ---------------------------||-----+
> | Interrupts, MMIO |
> +-------------------------------------------------------------------+
>
> Note that in the second case the guest doesn't need to share the memory
> with the host as it communicates only with the paravisor within their
> partition boundary. That is precisely the raison d'etre and the value
> proposition of this patch series: equip the confidential guest to use
> private (encrypted) memory and rely on the paravisor when this is
> available to be more secure.
I still have a fairly fundamental question about the value proposition.
The paravisor runs as part of the VM context and so is outside the host.
It should not trust the host any more than the guest does. For operations
that require the host, such as doing disk I/O, does the paravisor provide
any security benefit over the guest just talking directly to the host? Relying
on the paravisor is great, but doesn't that just push the problem from the
guest to the paravisor, which is no better equipped to solve it than the guest?
That's what I'm trying to get clear on.
>
> An implementation of the VMBus relay that offers the Confidential VMBus channels
> is available in the OpenVMM project as a part of the OpenHCL paravisor. Please
> refer to https://openvmm.dev/guide/ for more
> information about the OpenHCL paravisor.
>
> I'd like to thank the following people for their help with this
> patch series:
>
> * Dexuan for help with validation and the fruitful discussions,
> * Easwar for reviewing the refactoring of the page allocating and
> freeing in `hv.c`,
> * John and Sven for the design,
> * Mike for helping to avoid pitfalls when dealing with the GFP flags,
> * Sven for blazing the trail and implementing the design in few
> codebases.
>
> I made sure to validate the patch series on
>
> {TrustedLaunch(x86_64), OpenHCL} x
> {SNP(x86_64), TDX(x86_64), No hardware isolation, No paravisor} x
> {VMBus 5.0, VMBus 6.0} x
> {arm64, x86_64}.
>
> [V3]
> - The patch series is rebased on top of the latest hyperv-next branch.
> - Reworked the "wiring" diagram in the cover letter, added links to the
> OpenVMM project and the OpenHCL paravisor.
>
> - More precise wording in the comments and clearer code.
> **Thank you, Alok!**
>
> - Reworked the documentation patch.
> - Split the patchset into much more granular patches.
> - Various fixes and improvements throughout the patch series.
> **Thank you, Michael!**
I have fully reviewed v3 and am sending comments on most of the
individual patches. The split into more granular patches works really
well and was a big help in doing the review. I did not have comments
on a couple of the patches, but I have not given my Reviewed-by yet,
pending resolution of the other issues I've pointed out.
Michael
>
> [V2] https://lore.kernel.org/linux-hyperv/20250511230758.160674-1-romank@linux.microsoft.com/
> - The patch series is rebased on top of the latest hyperv-next branch.
>
> - Better wording in the commit messages and the Documentation.
> **Thank you, Alok and Wei!**
>
> - Removed the patches 5 and 6 concerning turning bounce buffering off from
> the previous version of the patch series as they were found to be
> architecturally unsound. The value proposition of the patch series is not
> diminished by this removal: these patches were an optimization and only for
> the storage (for the simplicity sake) but not for the network. These changes
> might be proposed in the future again after revolving the issues.
> ** Thanks you, Christoph, Dexuan, Dan, Michael, James, Robin! **
>
> [V1] https://lore.kernel.org/linux-hyperv/20250409000835.285105-1-romank@linux.microsoft.com/
>
> Roman Kisel (15):
> Documentation: hyperv: Confidential VMBus
> drivers: hv: VMBus protocol version 6.0
> arch: hyperv: Get/set SynIC synth.registers via paravisor
> arch/x86: mshyperv: Trap on access for some synthetic MSRs
> Drivers: hv: Rename fields for SynIC message and event pages
> Drivers: hv: Allocate the paravisor SynIC pages when required
> Drivers: hv: Post messages via the confidential VMBus if available
> Drivers: hv: remove stale comment
> Drivers: hv: Use memunmap() to check if the address is in IO map
> Drivers: hv: Rename the SynIC enable and disable routines
> Drivers: hv: Functions for setting up and tearing down the paravisor
> SynIC
> Drivers: hv: Allocate encrypted buffers when requested
> Drivers: hv: Support confidential VMBus channels
> Drivers: hv: Support establishing the confidential VMBus connection
> Drivers: hv: Set the default VMBus version to 6.0
>
> Documentation/virt/hyperv/coco.rst | 125 ++++++++-
> arch/x86/kernel/cpu/mshyperv.c | 67 ++++-
> drivers/hv/channel.c | 43 ++--
> drivers/hv/channel_mgmt.c | 27 +-
> drivers/hv/connection.c | 6 +-
> drivers/hv/hv.c | 399 ++++++++++++++++++++---------
> drivers/hv/hv_common.c | 13 +
> drivers/hv/hyperv_vmbus.h | 28 +-
> drivers/hv/mshv_root.h | 2 +-
> drivers/hv/mshv_synic.c | 6 +-
> drivers/hv/ring_buffer.c | 5 +-
> drivers/hv/vmbus_drv.c | 187 +++++++++-----
> include/asm-generic/mshyperv.h | 3 +
> include/linux/hyperv.h | 69 +++--
> 14 files changed, 740 insertions(+), 240 deletions(-)
>
>
> base-commit: 96959283a58d91ae20d025546f00e16f0a555208
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 01/15] Documentation: hyperv: Confidential VMBus
From: Michael Kelley @ 2025-06-18 16:17 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-2-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:43 PM
>
> Define what the confidential VMBus is and describe what advantages
> it offers on the capable hardware.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> Reviewed-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> ---
> Documentation/virt/hyperv/coco.rst | 125 ++++++++++++++++++++++++++++-
> 1 file changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/virt/hyperv/coco.rst b/Documentation/virt/hyperv/coco.rst
> index c15d6fe34b4e..b4904b64219d 100644
> --- a/Documentation/virt/hyperv/coco.rst
> +++ b/Documentation/virt/hyperv/coco.rst
> @@ -178,7 +178,7 @@ These Hyper-V and VMBus memory pages are marked as
> decrypted:
>
> * VMBus monitor pages
>
> -* Synthetic interrupt controller (synic) related pages (unless supplied by
> +* Synthetic interrupt controller (SynIC) related pages (unless supplied by
> the paravisor)
>
> * Per-cpu hypercall input and output pages (unless running with a paravisor)
> @@ -258,3 +258,126 @@ normal page fault is generated instead of #VC or #VE, and the page-fault-
> based handlers for load_unaligned_zeropad() fixup the reference. When the
> encrypted/decrypted transition is complete, the pages are marked as "present"
> again. See hv_vtom_clear_present() and hv_vtom_set_host_visibility().
> +
> +Confidential VMBus
> +------------------
Maybe put this section immediately after the "Guest communication with
Hyper-V" section, since it is an extension of that topic. Then leave the
load_unaligned_zeropad() section as last.
> +
> +The confidential VMBus enables the confidential guest not to interact with the
> +untrusted host partition and the untrusted hypervisor. Instead, the guest relies
> +on the trusted paravisor to communicate with the devices processing sensitive
> +data. The hardware (SNP or TDX) encrypts the guest memory and the register state
> +while measuring the paravisor image using the platform security processor to
> +ensure trusted and confidential computing.
> +
> +Confidential VMBus provides a secure communication channel between the guest and
> +the paravisor, ensuring that sensitive data is protected from hypervisor-level
> +access through memory encryption and register state isolation.
> +
> +The unencrypted data never leaves the VM
Is this statement really true? The VM contains both the guest and the paravisor,
so it's also a statement about the paravisor. I don't know what the paravisor
does for channels it is just proxying. Presumably it must communicate the
unencrypted data to the host.
> +so neither the host partition nor the
> +hypervisor can access it at all. In addition to that, the guest only needs to
> +establish a VMBus connection with the paravisor for the channels that process
> +sensitive data, and the paravisor abstracts the details of communicating with
> +the specific devices away.
> +
> +Confidential VMBus is an extension of Confidential Computing (CoCo) VMs
> +(a.k.a. "Isolated" VMs in Hyper-V terminology). Without Confidential VMBus,
> +guest VMBus device drivers (the "VSC"s in VMBus terminology) communicate
> +with VMBus servers (the VSPs) running on the Hyper-V host. The
> +communication must be through memory that has been decrypted so the
> +host can access it. With Confidential VMBus, one or more of the VSPs reside
> +in the trusted paravisor layer in the guest VM. Since the paravisor layer also
> +operates in encrypted memory, the memory used for communication with
> +such VSPs does not need to be decrypted and thereby exposed to the
> +Hyper-V host. The paravisor is responsible for communicating securely
> +with the Hyper-V host as necessary. In some cases (e.g. time synchonization,
> +key-value pairs exchange
I'm not sure about key-value pairs exchange. I thought the whole point of
KVP is to communicate information between the guest and the host.
> +) the unencrypted data doesn't need to be communicated
> +with the host at all, and a conventional VMBus connection suffices.
> +
> +Here is the data flow for a conventional VMBus connection and the Confidential
> +VMBus connection (C stands for the client or VSC, S for the server or VSP):
> +
> ++---- GUEST ----+ +----- DEVICE ----+ +----- HOST -----+
> +| | | | | |
> +| | | | | |
> +| | | ========== |
> +| | | | | |
> +| | | | | |
> +| | | | | |
> ++----- C -------+ +-----------------+ +------- S ------+
> + || ||
> + || ||
> ++------||------------------ VMBus --------------------------||------+
> +| Interrupts, MMIO |
> ++-------------------------------------------------------------------+
> +
> ++---- GUEST --------------- VTL0 ------+ +-- DEVICE --+
> +| | | |
> +| +- PARAVISOR --------- VTL2 -----+ | | |
> +| | +-- VMBus Relay ------+ ====+================ |
> +| | | Interrupts, MMIO | | | | |
> +| | +-------- S ----------+ | | +------------+
> +| | || | |
> +| +---------+ || | |
> +| | Linux | || OpenHCL | |
> +| | kernel | || | |
> +| +---- C --+-----||---------------+ |
> +| || || |
> ++-------++------- C -------------------+ +------------+
> + || | HOST |
> + || +---- S -----+
> ++-------||----------------- VMBus ---------------------------||-----+
> +| Interrupts, MMIO |
> ++-------------------------------------------------------------------+
> +
> +An implementation of the VMBus relay that offers the Confidential VMBus channels
> +is available in the OpenVMM project as a part of the OpenHCL paravisor. Please
> +refer to https://openvmm.dev/guide/ for more
> +information about the OpenHCL paravisor.
> +
> +A guest that is running with a paravisor must determine at runtime if
> +Confidential VMBus is supported by the current paravisor. It does so by first
> +trying to establish a Confidential VMBus connection with the paravisor using
> +standard mechanisms where the memory remains encrypted. If this succeeds,
> +then the guest can proceed to use Confidential VMBus. If it fails, then the
> +guest must fallback to establishing a non-Confidential VMBus connection with
> +the Hyper-V host.
> +
> +Confidential VMBus is a characteristic of the VMBus connection as a whole,
> +and of each VMBus channel that is created. When a Confidential VMBus
> +connection is established, the paravisor provides the guest the message-passing
> +path that is used for VMBus device creation and deletion, and it provides a
> +per-CPU synthetic interrupt controller (SynIC) just like the SynIC that is
> +offered by the Hyper-V host. Each VMBus device that is offered to the guest
> +indicates the degree to which it participates in Confidential VMBus. The offer
> +indicates if the device uses encrypted ring buffers, and if the device uses
> +encrypted memory for DMA that is done outside the ring buffer. These settings
> +may be different for different devices using the same Confidential VMBus
> +connection.
> +
> +Although these settings are separate, in practice it'll always be encrypted
> +ring buffer only or both encrypted ring buffer and external data. If a channel
s/ring buffer only or both/ring buffer only, or both/
> +is offered by the paravisor with confidential VMBus, the ring buffer can always
> +be encrypted since it's strictly for communication between the VTL2 paravisor
> +and the VTL0 guest. However, other memory regions are often used for e.g. DMA,
> +so they need to be accessible by the underlying hardware, and must be unencrypted
> +(unless the device supports encrypted memory). Currently, there are no any VSPs
s/no any/not any/
> +in OpenHCL that support encrypted external memory, but we will use it in the
Avoid personal pronouns like "we". Suggest this, or something similar:
"but future versions are expected to enable this capability."
> +future.
> +
> +Because some devices on a Confidential VMBus may require decrypted ring buffers
> +and DMA transfers, the guest must interact with two SynICs -- the one provided
> +by the paravisor and the one provided by the Hyper-V host when Confidential
> +VMBus is not offered. Interrupts are always signaled by the paravisor SynIC, but
> +the guest must check for messages and for channel interrupts on both SynICs.
> +
> +In the case of a confidential VM,
I think you mean "In the case of confidential VMBus,"
> +regular SynIC access by the guest is
> +intercepted by the paravisor (this includes various MSRs such as the SIMP and
> +SIEFP, as well as hypercalls like HvPostMessage and HvSignalEvent). If the guest
> +actually wants to communicate with the hypervisor, it has to use special mechanisms
> +(GHCB page on SNP, or tdcall on TDX). Messages will always be one or the other:
> +with confidential VMBus, all messages use the paravisor SynIC,
This statement seems to contradict the statement in the previous paragraph, and
the code. If all messages use the paravisor SynIC when Confidential VMBus is used,
why must the guest check for messages on both SynICs?
> +otherwise they all
> +use the hypervisor SynIC. For interrupt signaling, though, some channels may be
> +running on the host (non-confidential, using the VMBus relay) and use the hypervisor
> +SynIC, and some on the paravisor and use its SynIC. The RelIDs are coordinated by
> +the OpenHCL VMBus server and are guaranteed to be unique regardless of whether
> +the channel originated on the host or the paravisor.
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 03/15] arch: hyperv: Get/set SynIC synth.registers via paravisor
From: Michael Kelley @ 2025-06-18 16:17 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-4-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:43 PM
>
> The confidential VMBus is built on the guest talking to the
> paravisor only.
>
> Provide functions that allow manipulating the SynIC registers
> via paravisor. No functional changes.
I'd like to see a little more detailed explanation for "why" the
new functions. Something like:
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.
And I'm not sure "No functional changes" is correct. This is adding
new functionality. It's not just a cosmetic change or code
refactoring.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> Reviewed-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> ---
> arch/x86/kernel/cpu/mshyperv.c | 44 ++++++++++++++++++++++++++++++++++
> drivers/hv/hv_common.c | 13 ++++++++++
> include/asm-generic/mshyperv.h | 2 ++
> 3 files changed, 59 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 3e2533954675..83a85d94bcb3 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -89,6 +89,50 @@ 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 comminucations).
s/comminucations/communications/
> + */
> +int hv_para_get_synic_register(unsigned int reg, u64 *val)
> +{
> + u64 reg_val = ~0ULL;
> + int err = -ENODEV;
> +
> + if (hv_is_synic_msr(reg))
> + reg_val = native_read_msr_safe(reg, &err);
> + *val = reg_val;
> +
> + return err;
> +}
> +
> +/*
> + * Attempt to set the SynIC register value with the paravisor.
> + *
> + * Not all paravisors support reading SynIC registers, so this function
s/reading/setting/
> + * 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 comminucations).
s/comminucations/communications/
> + */
> +int hv_para_set_synic_register(unsigned int reg, u64 val)
> +{
> + if (!hv_is_synic_msr(reg))
> + return -ENODEV;
> + return wrmsrl_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..f239b102fc53 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -333,6 +333,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);
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 04/15] arch/x86: mshyperv: Trap on access for some synthetic MSRs
From: Michael Kelley @ 2025-06-18 16:18 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-5-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> To set up and run the confidential VMBus, the guest needs the paravisor
> to intercept access to some synthetic MSRs. In the non-confidential case,
> the guest continues using the vendor-specific guest-host communication
> protocol.
>
> Update the hv_set_non_nested_msr() function to trap access to some
> synthetic MSRs.
"trap access" is somewhat generic, and it's not clear what the intent is.
I'd suggest something like:
hv_set_non_nested_msr() has special handling for SINT MSRs
when a paravisor is present. In addition to updating the MSR on the
host, the mirror MSR in the paravisor is updated, including with the
proxy bit. But with Confidential VMBus, the proxy bit must not be
used, so add a special case to skip it.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> Reviewed-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> ---
> arch/x86/kernel/cpu/mshyperv.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 83a85d94bcb3..db6f3e3db012 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -28,6 +28,7 @@
> #include <asm/apic.h>
> #include <asm/timer.h>
> #include <asm/reboot.h>
> +#include <asm/msr.h>
> #include <asm/nmi.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/numa.h>
> @@ -77,14 +78,28 @@ EXPORT_SYMBOL_GPL(hv_get_non_nested_msr);
>
> void hv_set_non_nested_msr(unsigned int reg, u64 value)
> {
> + if (reg == HV_X64_MSR_EOM && vmbus_is_confidential()) {
> + /* Reach out to the paravisor. */
> + native_wrmsrl(reg, value);
> + return;
> + }
> +
It seems a bit inconsistent to have this particular MSR treated as
a special case in the generic code path, when the new functions
hv_para_get/set_synic_register() have been introduced to handle
the unique requirements of Confidential VMBus. This MSR is set
only in vmbus_signal_eom(), so maybe vmbus_signal_eom()
should test for confidential VM, and call hv_para_set_synic_register()
instead?
> if (hv_is_synic_msr(reg) && ms_hyperv.paravisor_present) {
> + /* The hypervisor will get the intercept. */
> hv_ivm_msr_write(reg, value);
>
> - /* Write proxy bit via wrmsl instruction */
> - if (hv_is_sint_msr(reg))
> - wrmsrl(reg, value | 1 << 20);
> + if (hv_is_sint_msr(reg)) {
> + /*
> + * Write proxy bit in the case of non-confidential VMBus.
> + * Using wrmsl instruction so the following goes to the paravisor.
> + */
> + u32 proxy = vmbus_is_confidential() ? 0 : 1;
> +
> + value |= (proxy << 20);
> + native_wrmsrl(reg, value);
> + }
> } else {
> - wrmsrl(reg, value);
> + native_wrmsrl(reg, value);
> }
> }
> EXPORT_SYMBOL_GPL(hv_set_non_nested_msr);
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 05/15] Drivers: hv: Rename fields for SynIC message and event pages
From: Michael Kelley @ 2025-06-18 16:18 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-6-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> Support for the confidential VMBus requires using SynIC message
> and event pages shared with the host and separate ones accessible
> only to the paravisor.
Slight tweak -- tie this to having two SynICs:
Confidential VMBus requires interacting with two SynICs -- one
provided by the host hypervisor, and one provided by the paravisor.
Each SynIC requires its own message and event pages.
>
> Rename the host-accessible SynIC message and event pages to be
> able to add the paravisor ones. No functional changes. The field
> name is also changed in mshv_root.* for consistency.
Rename the existing host-accessible SynIC message and event pages
with the "hyp_" prefix to clearly distinguish them from the paravisor
ones. The field name is also changed in mshv_root.* for consistency.
No functional change.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/channel_mgmt.c | 6 ++--
> drivers/hv/hv.c | 66 +++++++++++++++++++--------------------
> drivers/hv/hyperv_vmbus.h | 4 +--
> drivers/hv/mshv_root.h | 2 +-
> drivers/hv/mshv_synic.c | 6 ++--
> drivers/hv/vmbus_drv.c | 6 ++--
> 6 files changed, 45 insertions(+), 45 deletions(-)
>
^ permalink raw reply
* RE: [PATCH hyperv-next v3 06/15] Drivers: hv: Allocate the paravisor SynIC pages when required
From: Michael Kelley @ 2025-06-18 16:18 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-7-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> The paravisor needs the SynIC pages to communicate with the guest
> via the confidential VMBus.
>
> Refactor and extaned the exisitng code to account for that.
Suggest adding a bit more context to the commit message:
Confidential VMBus requires interacting with two SynICs -- one
provided by the host hypervisor, and one provided by the paravisor.
Each SynIC requires its own message and event pages.
Refactor and extend the existing code to add allocating and freeing
the message and event pages for the paravisor SynIC when it is
present.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/hv.c | 184 +++++++++++++++++++-------------------
> drivers/hv/hyperv_vmbus.h | 17 ++++
> 2 files changed, 111 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 964b9102477d..e25c91eb6af5 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -94,10 +94,70 @@ int hv_post_message(union hv_connection_id connection_id,
> return hv_result(status);
> }
>
> +static int hv_alloc_page(unsigned int cpu, void **page, bool decrypt,
> + const char *note)
Why have a "cpu" argument to this function? It's not used anywhere.
> +{
> + int ret = 0;
> +
> + /*
> + * After the page changes its encryption status, its contents might
> + * appear scrambled on some hardware. Thus `get_zeroed_page` would
> + * zero the page out in vain, we do that ourselves exactly once.
s/we do that/so do that explicitly exactly once/
Avoid personal pronouns like "we".
> + *
> + * By default, the page is allocated encrypted provided the system
> + * supports that.
More precise: "the page is allocated encrypted in a CoCo VM"
> + */
> + *page = (void *)__get_free_page(GFP_KERNEL);
> + if (!*page)
> + return -ENOMEM;
> +
> + if (decrypt)
> + ret = set_memory_decrypted((unsigned long)*page, 1);
> + if (ret)
> + goto failed;
> +
> + memset(*page, 0, PAGE_SIZE);
> + return 0;
> +
> +failed:
> +
Don't need a blank line here.
> + pr_err("allocation failed for %s page, error %d when allocating the page, decrypted %d\n",
The "when allocating the page" portion of the above message is somewhat redundant.
Compare with the similar message in hv_free_page().
> + note, ret, decrypt);
> + free_page((unsigned long)*page);
I don't think the page should be freed here. When set_memory_decrypted() fails, the
encryption state of the memory is unknown, so it should not be put back on the free list.
It's the same situation as in hv_free_page().
> + *page = NULL;
> + return ret;
> +}
> +
> +static int hv_free_page(void **page, bool encrypt, const char *note)
> +{
> + int ret = 0;
> +
> + if (!*page)
> + return 0;
> +
> + if (encrypt)
> + ret = set_memory_encrypted((unsigned long)*page, 1);
> +
> + /*
> + * In the case of the action failure, the page is leaked.
> + * Something is wrong, prefer to lose the page and stay afloat.
> + */
> + if (ret) {
> + pr_err("deallocation failed for %s page, error %d, encrypt %d\n",
> + note, ret, encrypt);
> + } else
> + free_page((unsigned long)*page);
> +
> + *page = NULL;
> +
> + return ret;
> +}
> +
> int hv_synic_alloc(void)
> {
> int cpu, ret = -ENOMEM;
> struct hv_per_cpu_context *hv_cpu;
> + const bool decrypt = !vmbus_is_confidential();
>
> /*
> * First, zero all per-cpu memory areas so hv_synic_free() can
> @@ -123,73 +183,37 @@ int hv_synic_alloc(void)
> vmbus_on_msg_dpc, (unsigned long)hv_cpu);
>
> if (ms_hyperv.paravisor_present && hv_isolation_type_tdx()) {
> - hv_cpu->post_msg_page = (void *)get_zeroed_page(GFP_ATOMIC);
> - if (!hv_cpu->post_msg_page) {
> - pr_err("Unable to allocate post msg page\n");
> - goto err;
> - }
> -
> - ret = set_memory_decrypted((unsigned long)hv_cpu->post_msg_page, 1);
> - if (ret) {
> - pr_err("Failed to decrypt post msg page: %d\n", ret);
> - /* Just leak the page, as it's unsafe to free the page. */
> - hv_cpu->post_msg_page = NULL;
> + ret = hv_alloc_page(cpu, &hv_cpu->post_msg_page,
> + decrypt, "post msg");
> + if (ret)
> goto err;
> - }
> -
> - memset(hv_cpu->post_msg_page, 0, PAGE_SIZE);
> }
>
> /*
> - * Synic message and event pages are allocated by paravisor.
> - * Skip these pages allocation here.
> + * If these SynIC pages are not allocated, SIEF and SIM pages
> + * are configured using what the root partition or the paravisor
> + * provides upon reading the SIEFP and SIMP registers.
> */
> if (!ms_hyperv.paravisor_present && !hv_root_partition()) {
> - hv_cpu->hyp_synic_message_page =
> - (void *)get_zeroed_page(GFP_ATOMIC);
> - if (!hv_cpu->hyp_synic_message_page) {
> - pr_err("Unable to allocate SYNIC message page\n");
> + ret = hv_alloc_page(cpu, &hv_cpu->hyp_synic_message_page,
> + decrypt, "hypervisor SynIC msg");
> + if (ret)
> goto err;
> - }
> -
> - hv_cpu->hyp_synic_event_page =
> - (void *)get_zeroed_page(GFP_ATOMIC);
> - if (!hv_cpu->hyp_synic_event_page) {
> - pr_err("Unable to allocate SYNIC event page\n");
> -
> - free_page((unsigned long)hv_cpu->hyp_synic_message_page);
> - hv_cpu->hyp_synic_message_page = NULL;
> + ret = hv_alloc_page(cpu, &hv_cpu->hyp_synic_event_page,
> + decrypt, "hypervisor SynIC event");
> + if (ret)
> goto err;
> }
> - }
>
> - if (!ms_hyperv.paravisor_present &&
> - (hv_isolation_type_snp() || hv_isolation_type_tdx())) {
> - ret = set_memory_decrypted((unsigned long)
> - hv_cpu->hyp_synic_message_page, 1);
> - if (ret) {
> - pr_err("Failed to decrypt SYNIC msg page: %d\n", ret);
> - hv_cpu->hyp_synic_message_page = NULL;
> -
> - /*
> - * Free the event page here so that hv_synic_free()
> - * won't later try to re-encrypt it.
> - */
> - free_page((unsigned long)hv_cpu->hyp_synic_event_page);
> - hv_cpu->hyp_synic_event_page = NULL;
> + if (vmbus_is_confidential()) {
> + ret = hv_alloc_page(cpu, &hv_cpu->para_synic_message_page,
> + decrypt, "paravisor SynIC msg");
Shouldn't the "decrypt" parameter just always be passed as "false"? That's the
fundamental tenet of Confidential VMBus -- these pages should not be decrypted.
> + if (ret)
> goto err;
> - }
> -
> - ret = set_memory_decrypted((unsigned long)
> - hv_cpu->hyp_synic_event_page, 1);
> - if (ret) {
> - pr_err("Failed to decrypt SYNIC event page: %d\n", ret);
> - hv_cpu->hyp_synic_event_page = NULL;
> + ret = hv_alloc_page(cpu, &hv_cpu->para_synic_event_page,
> + decrypt, "paravisor SynIC event");
Same here. "decrypt" is always "false".
> + if (ret)
> goto err;
> - }
> -
> - memset(hv_cpu->hyp_synic_message_page, 0, PAGE_SIZE);
> - memset(hv_cpu->hyp_synic_event_page, 0, PAGE_SIZE);
> }
> }
Refactoring this function with the hv_alloc_page() helper function works out
very nicely! The code is simpler and the error handling is much easier to get right.
>
> @@ -205,48 +229,28 @@ int hv_synic_alloc(void)
>
> void hv_synic_free(void)
> {
> - int cpu, ret;
> + int cpu;
> + const bool encrypt = !vmbus_is_confidential();
>
> for_each_present_cpu(cpu) {
> struct hv_per_cpu_context *hv_cpu =
> per_cpu_ptr(hv_context.cpu_context, cpu);
>
> - /* It's better to leak the page if the encryption fails. */
> - if (ms_hyperv.paravisor_present && hv_isolation_type_tdx()) {
> - if (hv_cpu->post_msg_page) {
> - ret = set_memory_encrypted((unsigned long)
> - hv_cpu->post_msg_page, 1);
> - if (ret) {
> - pr_err("Failed to encrypt post msg page: %d\n", ret);
> - hv_cpu->post_msg_page = NULL;
> - }
> - }
> + if (ms_hyperv.paravisor_present && hv_isolation_type_tdx())
> + hv_free_page(&hv_cpu->post_msg_page,
> + encrypt, "post msg");
> + if (!ms_hyperv.paravisor_present && !hv_root_partition()) {
> + hv_free_page(&hv_cpu->hyp_synic_event_page,
> + encrypt, "hypervisor SynIC event");
> + hv_free_page(&hv_cpu->hyp_synic_message_page,
> + encrypt, "hypervisor SynIC msg");
> }
> -
> - if (!ms_hyperv.paravisor_present &&
> - (hv_isolation_type_snp() || hv_isolation_type_tdx())) {
> - if (hv_cpu->hyp_synic_message_page) {
> - ret = set_memory_encrypted((unsigned long)
> - hv_cpu->hyp_synic_message_page, 1);
> - if (ret) {
> - pr_err("Failed to encrypt SYNIC msg page: %d\n", ret);
> - hv_cpu->hyp_synic_message_page = NULL;
> - }
> - }
> -
> - if (hv_cpu->hyp_synic_event_page) {
> - ret = set_memory_encrypted((unsigned long)
> - hv_cpu->hyp_synic_event_page, 1);
> - if (ret) {
> - pr_err("Failed to encrypt SYNIC event page: %d\n", ret);
> - hv_cpu->hyp_synic_event_page = NULL;
> - }
> - }
> + if (vmbus_is_confidential()) {
> + hv_free_page(&hv_cpu->para_synic_event_page,
> + encrypt, "paravisor SynIC event");
> + hv_free_page(&hv_cpu->para_synic_message_page,
> + encrypt, "paravisor SynIC msg");
As with hv_synic_alloc(), for these two calls, always "false" for the "encrypt"
parameter.
> }
> -
> - free_page((unsigned long)hv_cpu->post_msg_page);
> - free_page((unsigned long)hv_cpu->hyp_synic_event_page);
> - free_page((unsigned long)hv_cpu->hyp_synic_message_page);
> }
Same here on the refactoring. Very nice!
>
> kfree(hv_context.hv_numa_map);
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index fc3cdb26ff1a..9619edcf9f88 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -120,8 +120,25 @@ enum {
> * Per cpu state for channel handling
> */
> struct hv_per_cpu_context {
> + /*
> + * SynIC pages for communicating with the host.
> + *
> + * These pages are accessible to the host partition and the hypervisor,
> + * so they can only be used for exchanging data when the host partition
> + * and the hypervisor are trusted.
This comment isn't quite accurate. The hypervisor SynIC and its message/event
pages are used in today's CoCo VMs (without Confidential VMBus) where the host
partition and hypervisor are not trusted. The guest must be prepared for malicious
behavior by the SynIC, but that doesn't prevent today's CoCo VMs from providing
the intended confidentiality.
> + */
> void *hyp_synic_message_page;
> void *hyp_synic_event_page;
> + /*
> + * SynIC pages for communicating with the paravisor.
> + *
> + * These pages can be accessed only from within the guest partition.
> + * Neither the host partition nor the hypervisor can access these pages,
> + * so they can be used for exchanging data when the host partition and
> + * the hypervisor are not trusted, such as in a confidential VM.
Same here on this comment.
> + */
> + void *para_synic_message_page;
> + void *para_synic_event_page;
>
> /*
> * The page is only used in hv_post_message() for a TDX VM (with the
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 07/15] Drivers: hv: Post messages via the confidential VMBus if available
From: Michael Kelley @ 2025-06-18 16:18 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-8-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> When the confidential VMBus is available, the guest should post
> messages via the paravisor.
>
> Update hv_post_message() to request posting messages from the paravisor
"via the paravisor"? I'm not sure what "from the paravisor" means. And
you used "via" in the previous sentence and patch Subject.
> rather than through GHCB or TD calls.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/hv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index e25c91eb6af5..1f7cf1244509 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -74,7 +74,7 @@ int hv_post_message(union hv_connection_id connection_id,
> aligned_msg->payload_size = payload_size;
> memcpy((void *)aligned_msg->payload, payload, payload_size);
>
> - if (ms_hyperv.paravisor_present) {
> + if (ms_hyperv.paravisor_present && !vmbus_is_confidential()) {
Does this change make post_msg_page unnecessary when Confidential
VMBus is present? When using Confidential VMBus, the code path will be
to use a normal hypercall, which will go to the paravisor, and hence
doesn't need decrypted memory.
If my thinking is correct, the code in hv_synic_alloc() could be updated to
not allocate post_msg_page when vmbus_is_confidential().
> if (hv_isolation_type_tdx())
> status = hv_tdx_hypercall(HVCALL_POST_MESSAGE,
> virt_to_phys(aligned_msg), 0);
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 09/15] Drivers: hv: Use memunmap() to check if the address is in IO map
From: Michael Kelley @ 2025-06-18 16:18 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-10-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> It might happen that some hyp SynIC pages aren't IO mapped.
>
> Use memunmap() that checks for that and only then calls iounmap()
I'm concerned by the lack of symmetry in using io_remap_cache()
to do the mapping, and then memunmap() to remove it. The issue
is presumably that hyp_synic_[message/event]_page might be NULL?
Or is there some other case? But I'm thinking it would be better to
explicitly test for NULL and only call iounmap() if non-NULL. Then there's
no dependence on the implementation of memumap().
Not doing the explicit test for NULL actually caused the problem in
the first place. When the paravisor and root partition code was
introduced, iounmap() did a test and just returned, so everything
worked. Then commit 50c6dbdfd16e was added in the 6.12 kernel,
and iounmap() started generating a WARN if NULL is passed in.
Michael
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/hv.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 6a4857def82d..9a66656d89e0 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -358,7 +358,7 @@ void hv_synic_disable_regs(unsigned int cpu)
> */
> simp.simp_enabled = 0;
> if (ms_hyperv.paravisor_present || hv_root_partition()) {
> - iounmap(hv_cpu->hyp_synic_message_page);
> + memunmap(hv_cpu->hyp_synic_message_page);
> hv_cpu->hyp_synic_message_page = NULL;
> } else {
> simp.base_simp_gpa = 0;
> @@ -370,7 +370,7 @@ void hv_synic_disable_regs(unsigned int cpu)
> siefp.siefp_enabled = 0;
>
> if (ms_hyperv.paravisor_present || hv_root_partition()) {
> - iounmap(hv_cpu->hyp_synic_event_page);
> + memunmap(hv_cpu->hyp_synic_event_page);
> hv_cpu->hyp_synic_event_page = NULL;
> } else {
> siefp.base_siefp_gpa = 0;
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 10/15] Drivers: hv: Rename the SynIC enable and disable routines
From: Michael Kelley @ 2025-06-18 16:19 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-11-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> The confidential VMBus requires support for the both hypervisor
> facing SynIC and the paravisor one.
>
> Rename the functions that enable and disable SynIC with the
> hypervisor.
Add "No functional changes"?
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/channel_mgmt.c | 2 +-
> drivers/hv/hv.c | 11 ++++++-----
> drivers/hv/hyperv_vmbus.h | 4 ++--
> drivers/hv/vmbus_drv.c | 6 +++---
> 4 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 6f87220e2ca3..ca2fe10c110a 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -845,7 +845,7 @@ static void vmbus_wait_for_unload(void)
> /*
> * In a CoCo VM the hyp_synic_message_page is not allocated
> * in hv_synic_alloc(). Instead it is set/cleared in
> - * hv_synic_enable_regs() and hv_synic_disable_regs()
> + * hv_hyp_synic_enable_regs() and hv_hyp_synic_disable_regs()
> * such that it is set only when the CPU is online. If
> * not all present CPUs are online, the message page
> * might be NULL, so skip such CPUs.
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 9a66656d89e0..2b561825089a 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -257,9 +257,10 @@ void hv_synic_free(void)
> }
>
> /*
> - * hv_synic_enable_regs - Initialize the Synthetic Interrupt Controller.
> + * hv_hyp_synic_enable_regs - Initialize the Synthetic Interrupt Controller
> + * with the hypervisor.
> */
> -void hv_synic_enable_regs(unsigned int cpu)
> +void hv_hyp_synic_enable_regs(unsigned int cpu)
> {
> struct hv_per_cpu_context *hv_cpu =
> per_cpu_ptr(hv_context.cpu_context, cpu);
> @@ -325,14 +326,14 @@ void hv_synic_enable_regs(unsigned int cpu)
>
> int hv_synic_init(unsigned int cpu)
> {
> - hv_synic_enable_regs(cpu);
> + hv_hyp_synic_enable_regs(cpu);
>
> hv_stimer_legacy_init(cpu, VMBUS_MESSAGE_SINT);
>
> return 0;
> }
>
> -void hv_synic_disable_regs(unsigned int cpu)
> +void hv_hyp_synic_disable_regs(unsigned int cpu)
> {
> struct hv_per_cpu_context *hv_cpu =
> per_cpu_ptr(hv_context.cpu_context, cpu);
> @@ -515,7 +516,7 @@ int hv_synic_cleanup(unsigned int cpu)
> always_cleanup:
> hv_stimer_legacy_cleanup(cpu);
>
> - hv_synic_disable_regs(cpu);
> + hv_hyp_synic_disable_regs(cpu);
>
> return ret;
> }
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 9619edcf9f88..c1df611d1eb2 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -188,10 +188,10 @@ extern int hv_synic_alloc(void);
>
> extern void hv_synic_free(void);
>
> -extern void hv_synic_enable_regs(unsigned int cpu);
> +extern void hv_hyp_synic_enable_regs(unsigned int cpu);
> extern int hv_synic_init(unsigned int cpu);
>
> -extern void hv_synic_disable_regs(unsigned int cpu);
> +extern void hv_hyp_synic_disable_regs(unsigned int cpu);
> extern int hv_synic_cleanup(unsigned int cpu);
>
> /* Interface */
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 695b1ba7113c..f7e82a4fe133 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -2809,7 +2809,7 @@ static void hv_crash_handler(struct pt_regs *regs)
> */
> cpu = smp_processor_id();
> hv_stimer_cleanup(cpu);
> - hv_synic_disable_regs(cpu);
> + hv_hyp_synic_disable_regs(cpu);
> };
>
> static int hv_synic_suspend(void)
> @@ -2834,14 +2834,14 @@ static int hv_synic_suspend(void)
> * interrupts-disabled context.
> */
>
> - hv_synic_disable_regs(0);
> + hv_hyp_synic_disable_regs(0);
>
> return 0;
> }
>
> static void hv_synic_resume(void)
> {
> - hv_synic_enable_regs(0);
> + hv_hyp_synic_enable_regs(0);
>
> /*
> * Note: we don't need to call hv_stimer_init(0), because the timer
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 11/15] Drivers: hv: Functions for setting up and tearing down the paravisor SynIC
From: Michael Kelley @ 2025-06-18 16:19 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-12-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> The confidential VMBus runs with the paravisor SynIC and requires
> configuring it with the paravisor.
>
> Add the functions for configuring the paravisor SynIC
Suggest:
Add the functions for configuring the paravisor SynIC. Update
overall SynIC initialization logic to initialize the SynIC if it is present.
Finally, break out SynIC interrupt enable/disable code into separate
functions so that SynIC interrupts can be enabled/disable via the
paravisor instead of the hypervisor if the paravisor SynIC is present.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/hv.c | 180 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 169 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 2b561825089a..c9649ab3439e 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -203,7 +203,7 @@ int hv_synic_alloc(void)
> decrypt, "hypervisor SynIC event");
> if (ret)
> goto err;
> - }
> + }
This looks like a code cleanup that should be part of Patch 6 of this series.
>
> if (vmbus_is_confidential()) {
> ret = hv_alloc_page(cpu, &hv_cpu->para_synic_message_page,
> @@ -267,7 +267,6 @@ void hv_hyp_synic_enable_regs(unsigned int cpu)
> union hv_synic_simp simp;
> union hv_synic_siefp siefp;
> union hv_synic_sint shared_sint;
> - union hv_synic_scontrol sctrl;
>
> /* Setup the Synic's message page */
> simp.as_uint64 = hv_get_msr(HV_MSR_SIMP);
> @@ -288,7 +287,7 @@ void hv_hyp_synic_enable_regs(unsigned int cpu)
>
> hv_set_msr(HV_MSR_SIMP, simp.as_uint64);
>
> - /* Setup the Synic's event page */
> + /* Setup the Synic's event page with the hypervisor. */
You added "with the hypervisor" to this comment, but not the similar
comment above for the message page. Consistency .... :-)
> siefp.as_uint64 = hv_get_msr(HV_MSR_SIEFP);
> siefp.siefp_enabled = 1;
>
> @@ -316,6 +315,11 @@ void hv_hyp_synic_enable_regs(unsigned int cpu)
> shared_sint.masked = false;
> shared_sint.auto_eoi = hv_recommend_using_aeoi();
> hv_set_msr(HV_MSR_SINT0 + VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
> +}
> +
> +static void hv_hyp_synic_enable_interrupts(void)
> +{
> + union hv_synic_scontrol sctrl;
>
> /* Enable the global synic bit */
> sctrl.as_uint64 = hv_get_msr(HV_MSR_SCONTROL);
> @@ -324,13 +328,90 @@ void hv_hyp_synic_enable_regs(unsigned int cpu)
> hv_set_msr(HV_MSR_SCONTROL, sctrl.as_uint64);
> }
>
> +/*
> + * The paravisor might not support proxying SynIC, and this
> + * function may fail.
> + */
> +static int hv_para_synic_enable_regs(unsigned int cpu)
> +{
> + int err;
> + union hv_synic_simp simp;
> + union hv_synic_siefp siefp;
> + struct hv_per_cpu_context *hv_cpu
> + = per_cpu_ptr(hv_context.cpu_context, cpu);
> +
> + /* Setup the Synic's message page with the paravisor. */
> + err = hv_para_get_synic_register(HV_MSR_SIMP, &simp.as_uint64);
> + if (err)
> + return err;
> + simp.simp_enabled = 1;
> + simp.base_simp_gpa = virt_to_phys(hv_cpu->para_synic_message_page)
> + >> HV_HYP_PAGE_SHIFT;
> + err = hv_para_set_synic_register(HV_MSR_SIMP, simp.as_uint64);
> + if (err)
> + return err;
> +
> + /* Setup the Synic's event page with the paravisor. */
> + err = hv_para_get_synic_register(HV_MSR_SIEFP, &siefp.as_uint64);
> + if (err)
> + return err;
> + siefp.siefp_enabled = 1;
> + siefp.base_siefp_gpa = virt_to_phys(hv_cpu->para_synic_event_page)
> + >> HV_HYP_PAGE_SHIFT;
> + return hv_para_set_synic_register(HV_MSR_SIEFP, siefp.as_uint64);
> +}
> +
> +static int hv_para_synic_enable_interrupts(void)
> +{
> + union hv_synic_scontrol sctrl;
> + int err;
> +
> + /* Enable the global synic bit */
> + err = hv_para_get_synic_register(HV_MSR_SCONTROL, &sctrl.as_uint64);
> + if (err)
> + return err;
> + sctrl.enable = 1;
> +
> + return hv_para_set_synic_register(HV_MSR_SCONTROL, sctrl.as_uint64);
> +}
> +
> int hv_synic_init(unsigned int cpu)
> {
> + int err;
> +
> + /*
> + * The paravisor may not support the confidential VMBus,
> + * check on that first.
> + */
> + if (vmbus_is_confidential()) {
> + err = hv_para_synic_enable_regs(cpu);
> + if (err)
> + goto fail;
> + }
> +
> hv_hyp_synic_enable_regs(cpu);
> + if (vmbus_is_confidential()) {
> + err = hv_para_synic_enable_interrupts();
> + if (err)
> + goto fail;
> + } else
> + hv_hyp_synic_enable_interrupts();
It would be nice to have some detailed code comments somewhere
describing how VMBus interrupts work with Confidential VMBus. I
see that the SINT is set in hv_hyperv_synic_enable_regs() by calling
hv_set_msr(). hv_set_msr() in turn has special case code for the
SINT MSRs that write to the hypervisor version of the MSR *and*
the paravisor version of the MSR (but *without* the proxy bit when
VMBus is confidential). Then the code above enables interrupts
via the paravisor if VMBus is confidential, and otherwise via the
hypervisor.
Assuming my description is correct, maybe just writing that down
in a comment somewhere will confirm to later developers of this
code about what is supposed to be happening.
>
> hv_stimer_legacy_init(cpu, VMBUS_MESSAGE_SINT);
>
> return 0;
> +
> +fail:
> + /*
> + * The failure may only come from enabling the paravisor SynIC.
> + * That in turn means that the confidential VMBus cannot be used
> + * which is not an error: the setup will be re-tried with the
> + * non-confidential VMBus.
> + *
> + * We also don't bother attempting to reset the paravisor registers
> + * as something isn't working there anyway.
> + */
> + return err;
> }
>
> void hv_hyp_synic_disable_regs(unsigned int cpu)
> @@ -340,7 +421,6 @@ void hv_hyp_synic_disable_regs(unsigned int cpu)
> union hv_synic_sint shared_sint;
> union hv_synic_simp simp;
> union hv_synic_siefp siefp;
> - union hv_synic_scontrol sctrl;
>
> shared_sint.as_uint64 = hv_get_msr(HV_MSR_SINT0 + VMBUS_MESSAGE_SINT);
>
> @@ -378,14 +458,71 @@ void hv_hyp_synic_disable_regs(unsigned int cpu)
> }
>
> hv_set_msr(HV_MSR_SIEFP, siefp.as_uint64);
> +}
> +
> +static void hv_hyp_synic_disable_interrupts(void)
> +{
> + union hv_synic_scontrol sctrl;
>
> /* Disable the global synic bit */
> sctrl.as_uint64 = hv_get_msr(HV_MSR_SCONTROL);
> sctrl.enable = 0;
> hv_set_msr(HV_MSR_SCONTROL, sctrl.as_uint64);
> +}
>
> - if (vmbus_irq != -1)
> - disable_percpu_irq(vmbus_irq);
> +static void hv_para_synic_disable_regs(unsigned int cpu)
> +{
> + /*
> + * When a get/set register error is encountered, the function
> + * returns as the paravisor may not support these registers.
> + */
> + int err;
> + union hv_synic_simp simp;
> + union hv_synic_siefp siefp;
> +
> + struct hv_per_cpu_context *hv_cpu
> + = per_cpu_ptr(hv_context.cpu_context, cpu);
> +
> + /* Disable SynIC's message page in the paravisor. */
> + err = hv_para_get_synic_register(HV_MSR_SIMP, &simp.as_uint64);
> + if (err)
> + return;
> + simp.simp_enabled = 0;
> +
> + if (hv_cpu->para_synic_message_page) {
> + free_page((u64)(hv_cpu->para_synic_message_page));
> + hv_cpu->para_synic_message_page = NULL;
> + }
Freeing the para_synic_message_page memory here causes problems if a
CPU is taken offline, then back online. This function gets called when the CPU
goes offline, so the para_synic_message_page and para_synic_event_page
memory is freed. Then if the CPU comes back online,
hv_para_synic_enable_regs() runs without the memory being allocated
again.
Be sure to test the CPU offlining/onlining scenario to make sure it
works throughout the stack, including the paravisor.
> +
> + err = hv_para_set_synic_register(HV_MSR_SIMP, simp.as_uint64);
> + if (err)
> + return;
> +
> + /* Disable SynIC's event page in the paravisor. */
> + err = hv_para_get_synic_register(HV_MSR_SIEFP, &siefp.as_uint64);
> + if (err)
> + return;
> + siefp.siefp_enabled = 0;
> +
> + if (hv_cpu->para_synic_event_page) {
> + free_page((u64)(hv_cpu->para_synic_event_page));
> + hv_cpu->para_synic_event_page = NULL;
As above, don't free the memory here.
> + }
> +
> + hv_para_set_synic_register(HV_MSR_SIEFP, siefp.as_uint64);
> +}
> +
> +static void hv_para_synic_disable_interrupts(void)
> +{
> + union hv_synic_scontrol sctrl;
> + int err;
> +
> + /* Disable the global synic bit */
> + err = hv_para_get_synic_register(HV_MSR_SCONTROL, &sctrl.as_uint64);
> + if (err)
> + return;
> + sctrl.enable = 0;
> + hv_para_set_synic_register(HV_MSR_SCONTROL, sctrl.as_uint64);
> }
>
> #define HV_MAX_TRIES 3
> @@ -398,16 +535,18 @@ void hv_hyp_synic_disable_regs(unsigned int cpu)
> * that the normal interrupt handling mechanism will find and process the channel interrupt
> * "very soon", and in the process clear the bit.
> */
> -static bool hv_synic_event_pending(void)
> +static bool __hv_synic_event_pending(union hv_synic_event_flags *event, int sint)
> {
> - struct hv_per_cpu_context *hv_cpu = this_cpu_ptr(hv_context.cpu_context);
> - union hv_synic_event_flags *event =
> - (union hv_synic_event_flags *)hv_cpu->hyp_synic_event_page + VMBUS_MESSAGE_SINT;
> - unsigned long *recv_int_page = event->flags; /* assumes VMBus version >= VERSION_WIN8 */
> + unsigned long *recv_int_page;
> bool pending;
> u32 relid;
> int tries = 0;
>
> + if (!event)
> + return false;
> +
> + event += sint;
> + recv_int_page = event->flags; /* assumes VMBus version >= VERSION_WIN8 */
> retry:
> pending = false;
> for_each_set_bit(relid, recv_int_page, HV_EVENT_FLAGS_COUNT) {
> @@ -424,6 +563,17 @@ static bool hv_synic_event_pending(void)
> return pending;
> }
>
> +static bool hv_synic_event_pending(void)
> +{
> + struct hv_per_cpu_context *hv_cpu = this_cpu_ptr(hv_context.cpu_context);
> + union hv_synic_event_flags *hyp_synic_event_page = hv_cpu->hyp_synic_event_page;
> + union hv_synic_event_flags *para_synic_event_page = hv_cpu->para_synic_event_page;
> +
> + return
> + __hv_synic_event_pending(hyp_synic_event_page, VMBUS_MESSAGE_SINT) ||
> + __hv_synic_event_pending(para_synic_event_page, VMBUS_MESSAGE_SINT);
> +}
> +
> static int hv_pick_new_cpu(struct vmbus_channel *channel)
> {
> int ret = -EBUSY;
> @@ -517,6 +667,14 @@ int hv_synic_cleanup(unsigned int cpu)
> hv_stimer_legacy_cleanup(cpu);
>
> hv_hyp_synic_disable_regs(cpu);
> + if (vmbus_is_confidential()) {
> + hv_para_synic_disable_regs(cpu);
> + hv_para_synic_disable_interrupts();
The ordering of the interrupt handling here is a bit scary to me, but maybe
it's right. hv_hyp_synic_disable_regs() first masks the SINT, and because it's
a SINT MSR, it does so first in the hypervisor and then in the paravisor.
Then hv_para_synic_disable_regs() resets the SIMP and SIEFP. Finally
hv_para_synic_disable_interrupts() does a global interrupt disable via
the paravisor. I worry about there being gaps between steps where bad
things can happen. Might want to confirm with the hypervisor and
paravisor folks.
When VMBus is not confidential, the sequencing is the same as it is
now, which is good. There's presumably no reason to change that.
This is another aspect of my earlier comment about adding comments
that clearly describe how interrupts are to be setup/disabled when both
SynICs are present.
> + } else {
> + hv_hyp_synic_disable_interrupts();
> + }
> + if (vmbus_irq != -1)
> + disable_percpu_irq(vmbus_irq);
>
> return ret;
> }
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 12/15] Drivers: hv: Allocate encrypted buffers when requested
From: Michael Kelley @ 2025-06-18 16:19 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-13-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> Confidential VMBus is built around using buffers not shared with
> the host.
>
> Support allocating encrypted buffers when requested.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/channel.c | 43 +++++++++++++++++++++++----------------
> drivers/hv/hyperv_vmbus.h | 3 ++-
> drivers/hv/ring_buffer.c | 5 +++--
> 3 files changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index fb8cd8469328..3e2891c4b800 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -443,20 +443,23 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> return ret;
> }
>
> - /*
> - * Set the "decrypted" flag to true for the set_memory_decrypted()
> - * success case. In the failure case, the encryption state of the
> - * memory is unknown. Leave "decrypted" as true to ensure the
> - * memory will be leaked instead of going back on the free list.
> - */
> - gpadl->decrypted = true;
> - ret = set_memory_decrypted((unsigned long)kbuffer,
> - PFN_UP(size));
> - if (ret) {
> - dev_warn(&channel->device_obj->device,
> - "Failed to set host visibility for new GPADL %d.\n",
> - ret);
> - return ret;
> + gpadl->decrypted = !((channel->co_external_memory && type == HV_GPADL_BUFFER) ||
> + (channel->co_ring_buffer && type == HV_GPADL_RING));
> + if (gpadl->decrypted) {
> + /*
> + * Set the "decrypted" flag to true for the set_memory_decrypted()
> + * success case. In the failure case, the encryption state of the
> + * memory is unknown. Leave "decrypted" as true to ensure the
> + * memory will be leaked instead of going back on the free list.
> + */
The wording in the comment seems a little weird since the code below isn't setting
the "decrypted" flag. Perhaps:
The "decrypted" flag being true assumes that set_memory_decrypted() succeeds.
But if it fails, the encryption state of the memory is unknown. In that case, leave
"decrypted" as true to ensure the memory is leaked instead of going back on the
free list.
> + ret = set_memory_decrypted((unsigned long)kbuffer,
> + PFN_UP(size));
> + if (ret) {
> + dev_warn(&channel->device_obj->device,
> + "Failed to set host visibility for new GPADL %d.\n",
> + ret);
> + return ret;
This return is leaking the "msginfo" memory and any "submsginfo" memory that
was allocated by create_gpadl_header(). It looks like that leak is present in the
existing code as well.
> + }
There's still a problem here. If VMBus is Confidential, then the buffer memory remains
encrypted. Then later in __vmbus_establish_gpadl() if we get to the "cleanup:" label
with ret != 0 due to some error along the way, set_memory_encrypted() is called on
memory that is already encrypted. That should not be done as
set_memory_encrypted/decrypted() are not idempotent.
> }
>
> init_completion(&msginfo->waitevent);
> @@ -676,12 +679,13 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> goto error_clean_ring;
>
> err = hv_ringbuffer_init(&newchannel->outbound,
> - page, send_pages, 0);
> + page, send_pages, 0, newchannel->co_ring_buffer);
> if (err)
> goto error_free_gpadl;
>
> err = hv_ringbuffer_init(&newchannel->inbound, &page[send_pages],
> - recv_pages, newchannel->max_pkt_size);
> + recv_pages, newchannel->max_pkt_size,
> + newchannel->co_ring_buffer);
> if (err)
> goto error_free_gpadl;
>
> @@ -862,8 +866,11 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad
>
> kfree(info);
>
> - ret = set_memory_encrypted((unsigned long)gpadl->buffer,
> - PFN_UP(gpadl->size));
> + if (gpadl->decrypted)
> + ret = set_memory_encrypted((unsigned long)gpadl->buffer,
> + PFN_UP(gpadl->size));
> + else
> + ret = 0;
> if (ret)
> pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
>
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index c1df611d1eb2..0f02e163b0a0 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -199,7 +199,8 @@ extern int hv_synic_cleanup(unsigned int cpu);
> void hv_ringbuffer_pre_init(struct vmbus_channel *channel);
>
> int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
> - struct page *pages, u32 pagecnt, u32 max_pkt_size);
> + struct page *pages, u32 pagecnt, u32 max_pkt_size,
> + bool confidential);
>
> void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info);
>
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 3c9b02471760..05c2cd42fc75 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -183,7 +183,8 @@ void hv_ringbuffer_pre_init(struct vmbus_channel *channel)
>
> /* Initialize the ring buffer. */
> int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
> - struct page *pages, u32 page_cnt, u32 max_pkt_size)
> + struct page *pages, u32 page_cnt, u32 max_pkt_size,
> + bool confidential)
> {
> struct page **pages_wraparound;
> int i;
> @@ -207,7 +208,7 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
>
> ring_info->ring_buffer = (struct hv_ring_buffer *)
> vmap(pages_wraparound, page_cnt * 2 - 1, VM_MAP,
> - pgprot_decrypted(PAGE_KERNEL));
> + confidential ? PAGE_KERNEL : pgprot_decrypted(PAGE_KERNEL));
>
> kfree(pages_wraparound);
> if (!ring_info->ring_buffer)
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 13/15] Drivers: hv: Support confidential VMBus channels
From: Michael Kelley @ 2025-06-18 16:19 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-14-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> To run a confidential VMBus channels, one has to initialize the
How about:
To make use of Confidential VMBus channels, initialize the
> co_ring_buffers and co_external_memory fields of the channel
> structure.
>
> Advertise support upon negoatiating the version and compute
s/negoatiating/negotiating/
> values for those fields and initialize them.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/channel_mgmt.c | 19 +++++++++++++++++++
> drivers/hv/connection.c | 3 +++
> 2 files changed, 22 insertions(+)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index ca2fe10c110a..33bc29e826bd 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -1021,6 +1021,7 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
> struct vmbus_channel_offer_channel *offer;
> struct vmbus_channel *oldchannel, *newchannel;
> size_t offer_sz;
> + bool co_ring_buffer, co_external_memory;
>
> offer = (struct vmbus_channel_offer_channel *)hdr;
>
> @@ -1033,6 +1034,22 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
> return;
> }
>
> + co_ring_buffer = is_co_ring_buffer(offer);
> + if (co_ring_buffer) {
> + if (vmbus_proto_version < VERSION_WIN10_V6_0 || !vmbus_is_confidential()) {
> + atomic_dec(&vmbus_connection.offer_in_progress);
> + return;
> + }
> + }
> +
> + co_external_memory = is_co_external_memory(offer);
> + if (is_co_external_memory(offer)) {
Use the local variable co_external_memory instead of the function, like with co_ring_buffer?
Consistency .... :-)
> + if (vmbus_proto_version < VERSION_WIN10_V6_0 || !vmbus_is_confidential()) {
> + atomic_dec(&vmbus_connection.offer_in_progress);
> + return;
> + }
The test for valid vmbus_proto_version and VMBus being confidential is duplicated. You
could do:
if (co_ring_buffer || co_external_memory)
and have just one copy of the tests. Also I'd suggest adding an error message like
with the "vmbus_is_valid_offer()" test at the start of vmbus_onoffer(). That way
incoming offers aren't silently ignored. Something is wrong on the paravisor or
host side if the tests fail.
Also, since the combination where co_external_memory = true and
co_ring_buffer = false is not allowed, perhaps a check for that invalid
combination should be made here as well.
> + }
> +
> oldchannel = find_primary_channel_by_offer(offer);
>
> if (oldchannel != NULL) {
> @@ -1111,6 +1128,8 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
> pr_err("Unable to allocate channel object\n");
> return;
> }
> + newchannel->co_ring_buffer = co_ring_buffer;
> + newchannel->co_external_memory = co_external_memory;
>
> vmbus_setup_channel_state(newchannel, offer);
>
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index be490c598785..eeb472019d69 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -105,6 +105,9 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
> vmbus_connection.msg_conn_id = VMBUS_MESSAGE_CONNECTION_ID;
> }
>
> + if (vmbus_is_confidential() && version >= VERSION_WIN10_V6_0)
> + msg->feature_flags = VMBUS_FEATURE_FLAG_CONFIDENTIAL_CHANNELS;
> +
> /*
> * shared_gpa_boundary is zero in non-SNP VMs, so it's safe to always
> * bitwise OR it
> --
> 2.43.0
^ permalink raw reply
* RE: [PATCH hyperv-next v3 14/15] Drivers: hv: Support establishing the confidential VMBus connection
From: Michael Kelley @ 2025-06-18 16:19 UTC (permalink / raw)
To: Roman Kisel, alok.a.tiwari@oracle.com, arnd@arndb.de,
bp@alien8.de, corbet@lwn.net, dave.hansen@linux.intel.com,
decui@microsoft.com, haiyangz@microsoft.com, hpa@zytor.com,
kys@microsoft.com, mingo@redhat.com, tglx@linutronix.de,
wei.liu@kernel.org, linux-arch@vger.kernel.org,
linux-doc@vger.kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, x86@kernel.org
Cc: apais@microsoft.com, benhill@microsoft.com,
bperkins@microsoft.com, sunilmut@microsoft.com
In-Reply-To: <20250604004341.7194-15-romank@linux.microsoft.com>
From: Roman Kisel <romank@linux.microsoft.com> Sent: Tuesday, June 3, 2025 5:44 PM
>
> To establish the confidential VMBus connection the CoCo VM guest
> first attempts to connect to the VMBus server run by the paravisor.
> If that fails, the guest falls back to the non-confidential VMBus.
>
> Implement that in the VMBus driver initialization.
>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> ---
> drivers/hv/vmbus_drv.c | 169 +++++++++++++++++++++++++++--------------
> 1 file changed, 110 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index f7e82a4fe133..88701c3ad999 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1057,12 +1057,9 @@ static void vmbus_onmessage_work(struct work_struct *work)
> kfree(ctx);
> }
>
> -void vmbus_on_msg_dpc(unsigned long data)
> +static void __vmbus_on_msg_dpc(void *message_page_addr)
> {
> - struct hv_per_cpu_context *hv_cpu = (void *)data;
> - void *page_addr = hv_cpu->hyp_synic_message_page;
> - struct hv_message msg_copy, *msg = (struct hv_message *)page_addr +
> - VMBUS_MESSAGE_SINT;
> + struct hv_message msg_copy, *msg;
> struct vmbus_channel_message_header *hdr;
> enum vmbus_channel_message_type msgtype;
> const struct vmbus_channel_message_table_entry *entry;
> @@ -1070,6 +1067,10 @@ void vmbus_on_msg_dpc(unsigned long data)
> __u8 payload_size;
> u32 message_type;
>
> + if (!message_page_addr)
> + return;
> + msg = (struct hv_message *)message_page_addr + VMBUS_MESSAGE_SINT;
> +
> /*
> * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
> * it is being used in 'struct vmbus_channel_message_header' definition
> @@ -1195,6 +1196,14 @@ void vmbus_on_msg_dpc(unsigned long data)
> vmbus_signal_eom(msg, message_type);
> }
>
> +void vmbus_on_msg_dpc(unsigned long data)
> +{
> + struct hv_per_cpu_context *hv_cpu = (void *)data;
> +
> + __vmbus_on_msg_dpc(hv_cpu->hyp_synic_message_page);
> + __vmbus_on_msg_dpc(hv_cpu->para_synic_message_page);
> +}
> +
> #ifdef CONFIG_PM_SLEEP
> /*
> * Fake RESCIND_CHANNEL messages to clean up hv_sock channels by force for
> @@ -1233,21 +1242,19 @@ static void vmbus_force_channel_rescinded(struct
> vmbus_channel *channel)
> #endif /* CONFIG_PM_SLEEP */
>
> /*
> - * Schedule all channels with events pending
> + * Schedule all channels with events pending.
> + * The event page can be directly checked to get the id of
> + * the channel that has the interrupt pending.
> */
> -static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
> +static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu, void *event_page_addr)
The hv_cpu parameter to this function isn't used. Couldn't it be removed? Perhaps there's a
case for making the function API more parallel to vmbus_message_sched(), but in my judgment
that's not enough value to warrant passing an unneeded parameter.
> {
> unsigned long *recv_int_page;
> u32 maxbits, relid;
> + union hv_synic_event_flags *event;
>
> - /*
> - * The event page can be directly checked to get the id of
> - * the channel that has the interrupt pending.
> - */
> - void *page_addr = hv_cpu->hyp_synic_event_page;
> - union hv_synic_event_flags *event
> - = (union hv_synic_event_flags *)page_addr +
> - VMBUS_MESSAGE_SINT;
> + if (!event_page_addr)
> + return;
> + event = (union hv_synic_event_flags *)event_page_addr + VMBUS_MESSAGE_SINT;
>
> maxbits = HV_EVENT_FLAGS_COUNT;
> recv_int_page = event->flags;
> @@ -1318,26 +1325,40 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
> }
> }
>
> -static void vmbus_isr(void)
> +static void vmbus_message_sched(struct hv_per_cpu_context *hv_cpu, void *message_page_addr)
> {
> - struct hv_per_cpu_context *hv_cpu
> - = this_cpu_ptr(hv_context.cpu_context);
> - void *page_addr;
> struct hv_message *msg;
>
> - vmbus_chan_sched(hv_cpu);
> -
> - page_addr = hv_cpu->hyp_synic_message_page;
> - msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT;
> + if (!message_page_addr)
> + return;
> + msg = (struct hv_message *)message_page_addr + VMBUS_MESSAGE_SINT;
>
> /* Check if there are actual msgs to be processed */
> if (msg->header.message_type != HVMSG_NONE) {
> if (msg->header.message_type == HVMSG_TIMER_EXPIRED) {
> hv_stimer0_isr();
> vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
> - } else
> + } else {
> tasklet_schedule(&hv_cpu->msg_dpc);
> + }
> }
> +}
> +
> +static void vmbus_isr(void)
> +{
> + struct hv_per_cpu_context *hv_cpu
> + = this_cpu_ptr(hv_context.cpu_context);
> +
> + /*
> + * Suggested-by: Michael Kelley <mhklinux@outlook.com>
> + * One possible optimization would be to keep track of the largest relID that's in use,
> + * and only scan up to that relID.
> + */
I'd put this comment in vmbus_chan_sched() just before the "for_each_set_bit()"
loop. That's the loop that is doing the scanning.
> + vmbus_chan_sched(hv_cpu, hv_cpu->hyp_synic_event_page);
> + vmbus_chan_sched(hv_cpu, hv_cpu->para_synic_event_page);
> +
> + vmbus_message_sched(hv_cpu, hv_cpu->hyp_synic_message_page);
> + vmbus_message_sched(hv_cpu, hv_cpu->para_synic_message_page);
>
> add_interrupt_randomness(vmbus_interrupt);
> }
> @@ -1355,6 +1376,60 @@ static void vmbus_percpu_work(struct work_struct *work)
> hv_synic_init(cpu);
> }
>
> +static int vmbus_alloc_synic_and_connect(void)
> +{
> + int ret, cpu;
> + struct work_struct __percpu *works;
> + int hyperv_cpuhp_online;
> +
> + ret = hv_synic_alloc();
> + if (ret < 0)
> + goto err_alloc;
> +
Extra blank line here.
> +
> + works = alloc_percpu(struct work_struct);
> + if (!works) {
> + ret = -ENOMEM;
> + goto err_alloc;
> + }
> +
> + /*
> + * Initialize the per-cpu interrupt state and stimer state.
> + * Then connect to the host.
> + */
> + cpus_read_lock();
> + for_each_online_cpu(cpu) {
> + struct work_struct *work = per_cpu_ptr(works, cpu);
> +
> + INIT_WORK(work, vmbus_percpu_work);
> + schedule_work_on(cpu, work);
> + }
> +
> + for_each_online_cpu(cpu)
> + flush_work(per_cpu_ptr(works, cpu));
> +
> + /* Register the callbacks for possible CPU online/offline'ing */
> + ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
> + hv_synic_init, hv_synic_cleanup);
> + cpus_read_unlock();
There's a subtle problem here. As soon as the lock is released, there could be
vCPUs that start coming online (started by the udev daemon), and running
hv_synic_init(). My assumption is that hv_synic_init() might fail if this invocation
of vmbus_alloc_synic_and_connect() is trying to determine if Confidential VMBus
is supported. A failure will abort the new CPU coming online. That suggests that
hv_synic_init() should not return an error in the case where initializing the paravisor
SynIC doesn't work.
> + free_percpu(works);
> + if (ret < 0)
> + goto err_alloc;
> + hyperv_cpuhp_online = ret;
> +
> + ret = vmbus_connect();
When doing vmbus_alloc_synic_and_connect() the first time with "is_confidential"
set to "true", where exactly does the failure occur if the paravisor doesn't support
Confidential VMBus? Since your patches add machinery to detect MSR accesses
against the paravisor that fail, I'm presuming hv_synic_init() will fail for all vCPUs.
But the error return from hv_synic_init() isn't checked (except as I mentioned above
for cpuhp_setup_state). So evidently the failure is detected in vmbus_connect() when
vmbus_negotiate_version() sends the INITIATE_CONTACT message. And presumably
sending the message fails because there's no way to wait for a failure response since
the SynICs didn't initialized. So is it the HV_POST_MESSAGE hypercall that is the
exact point of failure, and if so, what error status is returned? It looks like
vmbus_post_msg() would output an error message. Or is there some other failure
point that I'm missing?
I ask because there's a lot of work done before the failure is detected. Each
vCPU must try to initialize their paravisor SynIC and fail. Then each CPU does
hv_synic_cleanup() as part of cpuhp_remove_state(). Workqueues are created
in vmbus_connect(), and memory is allocated, all of which must be cleaned
up. Then everything is retried with "is_confidential" set to "false". Do you have
any sense of how much elapsed time it takes to get the initial failure and then
cleanup? Consider the case with a large number of vCPUs, all of which must
run hv_synic_init() and then hv_synic_cleanup(). See commit 87c9741a38c4
where this elapsed time was a concern in large VMs.
Can the failure be detected earlier by doing a simple MSR read against
the paravisor for an MSR that's only available if Confidential VMBus is
implemented? Then "is _confidential" could be set correctly before
all the work is done, and the work would only be done once even
if Confidential VMBus isn't supported.
> + if (ret)
> + goto err_connect;
> + return 0;
> +
> +err_connect:
> + cpuhp_remove_state(hyperv_cpuhp_online);
> + return -ENODEV;
> +err_alloc:
> + hv_synic_free();
> + return -ENOMEM;
> +}
> +
> /*
> * vmbus_bus_init -Main vmbus driver initialization routine.
> *
> @@ -1365,8 +1440,7 @@ static void vmbus_percpu_work(struct work_struct *work)
> */
> static int vmbus_bus_init(void)
> {
> - int ret, cpu;
> - struct work_struct __percpu *works;
> + int ret;
>
> ret = hv_init();
> if (ret != 0) {
> @@ -1401,41 +1475,21 @@ static int vmbus_bus_init(void)
> }
> }
>
> - ret = hv_synic_alloc();
> - if (ret)
> - goto err_alloc;
> -
> - works = alloc_percpu(struct work_struct);
> - if (!works) {
> - ret = -ENOMEM;
> - goto err_alloc;
> - }
> -
> /*
> - * Initialize the per-cpu interrupt state and stimer state.
> - * Then connect to the host.
> + * Attempt to establish the confidential VMBus connection first if this VM is
> + * a hardware confidential VM, and the paravisor is present.
> */
> - cpus_read_lock();
> - for_each_online_cpu(cpu) {
> - struct work_struct *work = per_cpu_ptr(works, cpu);
> + ret = -ENODEV;
> + if (ms_hyperv.paravisor_present && (hv_isolation_type_tdx() || hv_isolation_type_snp())) {
> + is_confidential = true;
> + ret = vmbus_alloc_synic_and_connect();
> + is_confidential = !ret;
>
> - INIT_WORK(work, vmbus_percpu_work);
> - schedule_work_on(cpu, work);
> + pr_info("VMBus is confidential: %d\n", is_confidential);
> }
>
> - for_each_online_cpu(cpu)
> - flush_work(per_cpu_ptr(works, cpu));
> -
> - /* Register the callbacks for possible CPU online/offline'ing */
> - ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
> - hv_synic_init, hv_synic_cleanup);
> - cpus_read_unlock();
> - free_percpu(works);
> - if (ret < 0)
> - goto err_alloc;
> - hyperv_cpuhp_online = ret;
> -
> - ret = vmbus_connect();
> + if (!is_confidential)
> + ret = vmbus_alloc_synic_and_connect();
> if (ret)
> goto err_connect;
>
> @@ -1451,9 +1505,6 @@ static int vmbus_bus_init(void)
> return 0;
>
> err_connect:
> - cpuhp_remove_state(hyperv_cpuhp_online);
> -err_alloc:
> - hv_synic_free();
> if (vmbus_irq == -1) {
> hv_remove_vmbus_handler();
> } else {
> --
> 2.43.0
^ permalink raw reply
* Re: [PATCH] firmware: smccc: support both conduits for getting hyp UUID
From: Roman Kisel @ 2025-06-18 17:57 UTC (permalink / raw)
To: Anirudh Rayabharam
Cc: sudeep.holla, linux-arm-kernel, linux-hyperv, linux-kernel,
lpieralisi, mark.rutland
In-Reply-To: <aFKu9TFA6oj1N2cR@anirudh-surface.localdomain>
On 6/18/2025 5:20 AM, Anirudh Rayabharam wrote:
> On Tue, Jun 10, 2025 at 09:06:48AM -0700, Roman Kisel wrote:
>>> (sorry for the delay, found the patch in the spam 🙁)
>>
>> "b4" shows the the mail server used for the patch submission
>> doesn't pass the DKIM check, so finding the patch in the spam seems
>
> How do you check this? I mean, what b4 command do you run?
>
> I think it should be fix now. Let's see...
"b4 am" was showing the failed DKIM check, and all good now!
| $ b4 am 20250521094049.960056-1-anirudh@anirudhrb.com
|
| Grabbing thread from
lore.kernel.org/all/20250521094049.960056-1-anirudh@anirudhrb.com/t.mbox.gz
| Analyzing 7 messages in the thread
| Looking for additional code-review trailers on lore.kernel.org
| Checking attestation on all messages, may take a moment...
| ---
| ✓ [PATCH] firmware: smccc: support both conduits for getting hyp UUID
| + Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
| + Tested-by: Roman Kisel <romank@linux.microsoft.com> (✓
DKIM/linux.microsoft.com)
| + Reviewed-by: Roman Kisel <romank@linux.microsoft.com> (✓
DKIM/linux.microsoft.com)
| ---
| ✓ Signed: DKIM/anirudhrb.com
| ---
| Total patches: 1
| ---
| Link:
https://lore.kernel.org/r/20250521094049.960056-1-anirudh@anirudhrb.com
| Base: applies clean to current tree
| git checkout -b 20250521_anirudh_anirudhrb_com HEAD
| git am
./20250521_anirudh_firmware_smccc_support_both_conduits_for_getting_hyp_uuid.mbx
|
[...]
>> Anirudh had been OOF for some time and would be for another
>> week iiuc so I thought I'd reply.
>
> Thanks Roman!
My pleasure :)
>
> Regards,
> Anirudh
>
>>
>> The patch this depends on is 13423063c7cb
>> ("arm64: kvm, smccc: Introduce and use API for getting hypervisor UUID"),
>> and this patch has already been pulled into the Linus'es tree.
>>
>> As for routing, (arm-)soc should be good it appears as the change
>> is contained within the firmware drivers path. Although I'd trust more to your,
>> Arnd's or Wei's opinion than mine!
>>
>>>
>>> --
>>> Regards,
>>> Sudeep
--
Thank you,
Roman
^ permalink raw reply
* Re: [PATCH 3/4] x86: hyperv: Expose hv_map_msi_interrupt function
From: Nuno Das Neves @ 2025-06-18 21:08 UTC (permalink / raw)
To: Michael Kelley, linux-hyperv@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Cc: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
decui@microsoft.com, catalin.marinas@arm.com, will@kernel.org,
tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
dave.hansen@linux.intel.com, hpa@zytor.com, lpieralisi@kernel.org,
kw@linux.com, robh@kernel.org, bhelgaas@google.com,
jinankjain@linux.microsoft.com, skinsburskii@linux.microsoft.com,
mrathor@linux.microsoft.com, x86@kernel.org
In-Reply-To: <SN6PR02MB4157639630F8AD2D8FD8F52FD475A@SN6PR02MB4157.namprd02.prod.outlook.com>
On 6/11/2025 4:07 PM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Tuesday, June 10, 2025 4:52 PM
>>
>> From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>
> The preferred patch Subject prefix is "x86/hyperv:"
>
Thank you for clarifying - I thought I saw some precedent for x86: hyperv:
but must have been mistaken.
>>
>> This patch moves a part of currently internal logic into the
>> hv_map_msi_interrupt function and makes it globally available helper
>> function, which will be used to map PCI interrupts in case of root
>> partition.
>
> Avoid "this patch" in commit messages. Suggest:
>
> Create a helper function hv_map_msi_interrupt() that contains some
> logic that is currently internal to irqdomain.c. Make the helper function
> globally available so it can be used to map PCI interrupts when running
> in the root partition.
>
Thanks, I'll rephrase.
>>
>> Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> ---
>> arch/x86/hyperv/irqdomain.c | 47 ++++++++++++++++++++++++---------
>> arch/x86/include/asm/mshyperv.h | 2 ++
>> 2 files changed, 36 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/x86/hyperv/irqdomain.c b/arch/x86/hyperv/irqdomain.c
>> index 31f0d29cbc5e..82f3bafb93d6 100644
>> --- a/arch/x86/hyperv/irqdomain.c
>> +++ b/arch/x86/hyperv/irqdomain.c
>> @@ -169,13 +169,40 @@ static union hv_device_id hv_build_pci_dev_id(struct pci_dev *dev)
>> return dev_id;
>> }
>>
>> -static int hv_map_msi_interrupt(struct pci_dev *dev, int cpu, int vector,
>> - struct hv_interrupt_entry *entry)
>> +/**
>> + * hv_map_msi_interrupt() - "Map" the MSI IRQ in the hypervisor.
>> + * @data: Describes the IRQ
>> + * @out_entry: Hypervisor (MSI) interrupt entry (can be NULL)
>> + *
>> + * Map the IRQ in the hypervisor by issuing a MAP_DEVICE_INTERRUPT hypercall.
>> + */
>> +int hv_map_msi_interrupt(struct irq_data *data,
>> + struct hv_interrupt_entry *out_entry)
>> {
>> - union hv_device_id device_id = hv_build_pci_dev_id(dev);
>> + struct msi_desc *msidesc;
>> + struct pci_dev *dev;
>> + union hv_device_id device_id;
>> + struct hv_interrupt_entry dummy;
>> + struct irq_cfg *cfg = irqd_cfg(data);
>> + const cpumask_t *affinity;
>> + int cpu;
>> + u64 res;
>>
>> - return hv_map_interrupt(device_id, false, cpu, vector, entry);
>> + msidesc = irq_data_get_msi_desc(data);
>> + dev = msi_desc_to_pci_dev(msidesc);
>> + device_id = hv_build_pci_dev_id(dev);
>> + affinity = irq_data_get_effective_affinity_mask(data);
>> + cpu = cpumask_first_and(affinity, cpu_online_mask);
>
> Is the cpus_read_lock held at this point? I'm not sure what the
> overall calling sequence looks like. If it is not held, the CPU that
> is selected could go offline before hv_map_interrupt() is called.
> This computation of the target CPU is the same as in the code
> before this patch, but that existing code looks like it has the
> same problem.
>
Thanks for pointing it out - It *looks* like the read lock is not held
everywhere this could be called, so it could indeed be a problem.
I've been thinking about different ways around this but I lack the
knowledge to have an informed opinion about it:
- We could take the cpu read lock in this function, would that work?
- I'm not actually sure why the code is getting the first cpu off the effective
affinity mask in the first place. It is possible to get the apic id (and hence
the cpu) already associated with the irq, as per e.g. x86_vector_msi_compose_msg()
Maybe we could get the cpu that way, assuming that doesn't have a similar issue.
- We could just let this race happen, maybe the outcome isn't too catastrophic?
What do you think?
>> +
>> + res = hv_map_interrupt(device_id, false, cpu, cfg->vector,
>> + out_entry ? out_entry : &dummy);
>> + if (!hv_result_success(res))
>> + pr_err("%s: failed to map interrupt: %s",
>> + __func__, hv_result_to_string(res));
>
> hv_map_interrupt() already outputs a message if the hypercall
> fails. Is another message needed here?
>
It does print the function name, which gives additional context.
Probably it can just be removed however.
>> +
>> + return hv_result_to_errno(res);
>
> The error handling is rather messed up. First hv_map_interrupt()
> sometimes returns a Linux errno (not negated), and sometimes a
> hypercall result. The errno is EINVAL, which has value "22", which is
> the same as hypercall result HV_STATUS_ACKNOWLEDGED. And
> the hypercall result returned from hv_map_interrupt() is just
> the result code, not the full 64-bit status, as hv_map_interrupt()
> has already done hv_result(status). Hence the "res" input arg to
> hv_result_to_errno() isn't really the correct input. For example,
> if the hypercall returns U64_MAX, that won't be caught by
> hv_result_to_errno() since the value has been truncated to
> 32 bits. Fixing all this will require some unscrambling.
>
Good point, it's pretty messed up! I think in v2 I'll add a patch to
clean this up first.
>> }
>> +EXPORT_SYMBOL_GPL(hv_map_msi_interrupt);
>>
>> static inline void entry_to_msi_msg(struct hv_interrupt_entry *entry, struct msi_msg *msg)
>> {
>> @@ -190,10 +217,8 @@ static void hv_irq_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>> {
>> struct msi_desc *msidesc;
>> struct pci_dev *dev;
>> - struct hv_interrupt_entry out_entry, *stored_entry;
>> + struct hv_interrupt_entry *stored_entry;
>> struct irq_cfg *cfg = irqd_cfg(data);
>> - const cpumask_t *affinity;
>> - int cpu;
>> u64 status;
>>
>> msidesc = irq_data_get_msi_desc(data);
>> @@ -204,9 +229,6 @@ static void hv_irq_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>> return;
>> }
>>
>> - affinity = irq_data_get_effective_affinity_mask(data);
>> - cpu = cpumask_first_and(affinity, cpu_online_mask);
>> -
>> if (data->chip_data) {
>> /*
>> * This interrupt is already mapped. Let's unmap first.
>> @@ -235,15 +257,14 @@ static void hv_irq_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>> return;
>> }
>>
>> - status = hv_map_msi_interrupt(dev, cpu, cfg->vector, &out_entry);
>> + status = hv_map_msi_interrupt(data, stored_entry);
>> if (status != HV_STATUS_SUCCESS) {
>
> hv_map_msi_interrupt() returns an errno, so testing for HV_STATUS_SUCCESS
> is bogus.
>
Thanks, noted.
>> kfree(stored_entry);
>> return;
>> }
>>
>> - *stored_entry = out_entry;
>> data->chip_data = stored_entry;
>> - entry_to_msi_msg(&out_entry, msg);
>> + entry_to_msi_msg(data->chip_data, msg);
>>
>> return;
>> }
>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
>> index 5ec92e3e2e37..843121465ddd 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -261,6 +261,8 @@ static inline void hv_apic_init(void) {}
>>
>> struct irq_domain *hv_create_pci_msi_domain(void);
>>
>> +int hv_map_msi_interrupt(struct irq_data *data,
>> + struct hv_interrupt_entry *out_entry);
>> int hv_map_ioapic_interrupt(int ioapic_id, bool level, int vcpu, int vector,
>> struct hv_interrupt_entry *entry);
>> int hv_unmap_ioapic_interrupt(int ioapic_id, struct hv_interrupt_entry *entry);
>> --
>> 2.34.1
^ permalink raw reply
* Re: [PATCH 4/4] PCI: hv: Use the correct hypercall for unmasking interrupts on nested
From: Nuno Das Neves @ 2025-06-18 21:24 UTC (permalink / raw)
To: Michael Kelley, linux-hyperv@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Cc: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
decui@microsoft.com, catalin.marinas@arm.com, will@kernel.org,
tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
dave.hansen@linux.intel.com, hpa@zytor.com, lpieralisi@kernel.org,
kw@linux.com, robh@kernel.org, bhelgaas@google.com,
jinankjain@linux.microsoft.com, skinsburskii@linux.microsoft.com,
mrathor@linux.microsoft.com, x86@kernel.org
In-Reply-To: <SN6PR02MB4157BAE54D3BBD4CD8065722D475A@SN6PR02MB4157.namprd02.prod.outlook.com>
On 6/11/2025 4:07 PM, Michael Kelley wrote:
> From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Tuesday, June 10, 2025 4:52 PM
>>
>> From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>>
>> Running as nested root on MSHV imposes a different requirement
>> for the pci-hyperv controller.
>>
>> In this setup, the interrupt will first come to the L1 (nested) hypervisor,
>> which will deliver it to the appropriate root CPU. Instead of issuing the
>> RETARGET hypercall, we should issue the MAP_DEVICE_INTERRUPT
>> hypercall to L1 to complete the setup.
>>
>> Rename hv_arch_irq_unmask() to hv_irq_retarget_interrupt().
>>
>> Co-developed-by: Jinank Jain <jinankjain@linux.microsoft.com>
>> Signed-off-by: Jinank Jain <jinankjain@linux.microsoft.com>
>> Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> ---
>> drivers/pci/controller/pci-hyperv.c | 18 ++++++++++++++++--
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
>> index 4d25754dfe2f..0f491c802fb9 100644
>> --- a/drivers/pci/controller/pci-hyperv.c
>> +++ b/drivers/pci/controller/pci-hyperv.c
>> @@ -600,7 +600,7 @@ static unsigned int hv_msi_get_int_vector(struct irq_data *data)
>> #define hv_msi_prepare pci_msi_prepare
>>
>> /**
>> - * hv_arch_irq_unmask() - "Unmask" the IRQ by setting its current
>> + * hv_irq_retarget_interrupt() - "Unmask" the IRQ by setting its current
>> * affinity.
>> * @data: Describes the IRQ
>> *
>> @@ -609,7 +609,7 @@ static unsigned int hv_msi_get_int_vector(struct irq_data *data)
>> * is built out of this PCI bus's instance GUID and the function
>> * number of the device.
>> */
>> -static void hv_arch_irq_unmask(struct irq_data *data)
>> +static void hv_irq_retarget_interrupt(struct irq_data *data)
>> {
>> struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
>> struct hv_retarget_device_interrupt *params;
>> @@ -714,6 +714,20 @@ static void hv_arch_irq_unmask(struct irq_data *data)
>> dev_err(&hbus->hdev->device,
>> "%s() failed: %#llx", __func__, res);
>> }
>> +
>> +static void hv_arch_irq_unmask(struct irq_data *data)
>> +{
>> + if (hv_nested && hv_root_partition())
>
> Based on Patch 1 of this series, this driver is not loaded for the root
> partition in the non-nested case. So testing hv_nested is redundant.
>
Good point, I'll change it to just check for hv_root_partition() here.
>> + /*
>> + * In case of the nested root partition, the nested hypervisor
>> + * is taking care of interrupt remapping and thus the
>> + * MAP_DEVICE_INTERRUPT hypercall is required instead of
>> + * RETARGET_INTERRUPT.
>> + */
>> + (void)hv_map_msi_interrupt(data, NULL);
>> + else
>> + hv_irq_retarget_interrupt(data);
>> +}
>> #elif defined(CONFIG_ARM64)
>> /*
>> * SPI vectors to use for vPCI; arch SPIs range is [32, 1019], but leaving a bit
>> --
>> 2.34.1
^ permalink raw reply
* Re: [PATCH 4/4] PCI: hv: Use the correct hypercall for unmasking interrupts on nested
From: Nuno Das Neves @ 2025-06-18 22:45 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-hyperv, linux-arm-kernel, linux-kernel, linux-pci, kys,
haiyangz, wei.liu, mhklinux, decui, catalin.marinas, will, tglx,
mingo, bp, dave.hansen, hpa, lpieralisi, kw,
manivannan.sadhasivam, robh, bhelgaas, jinankjain, skinsburskii,
mrathor, x86
In-Reply-To: <20250613193616.GA971782@bhelgaas>
On 6/13/2025 12:36 PM, Bjorn Helgaas wrote:
> On Tue, Jun 10, 2025 at 04:52:06PM -0700, Nuno Das Neves wrote:
>> From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
>>
>> Running as nested root on MSHV imposes a different requirement
>> for the pci-hyperv controller.
>>
>> In this setup, the interrupt will first come to the L1 (nested) hypervisor,
>> which will deliver it to the appropriate root CPU. Instead of issuing the
>> RETARGET hypercall, we should issue the MAP_DEVICE_INTERRUPT
>> hypercall to L1 to complete the setup.
>
> Maybe strengthen this to say that this issues MAP_DEVICE_INTERRUPT
> instead of RETARGET in this case? (Not just that we "should".)
>
Good suggestion, thanks.
>> Rename hv_arch_irq_unmask() to hv_irq_retarget_interrupt().
^ permalink raw reply
* [PATCH] hv/hv_kvp_daemon: Prevent similar logs in kvp_key_add_or_modify()
From: Shradha Gupta @ 2025-06-19 5:26 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Naman Jain,
linux-hyperv, linux-kernel
Cc: Shradha Gupta, Shradha Gupta
When hv_kvp_daemon is started in debug mode, in function
kvp_key_add_or_modify() too many similar logs are logged for
key/value being too long.
Restructured the logs to prevent this extra logging
Suggested-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
---
tools/hv/hv_kvp_daemon.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 1f64c680be13..91a708282358 100644
--- a/tools/hv/hv_kvp_daemon.c
+++ b/tools/hv/hv_kvp_daemon.c
@@ -352,21 +352,17 @@ static int kvp_key_add_or_modify(int pool, const __u8 *key, int key_size,
int num_blocks;
int i;
- if (debug)
- syslog(LOG_DEBUG, "%s: got a KVP: pool=%d key=%s val=%s",
- __func__, pool, key, value);
-
if ((key_size > HV_KVP_EXCHANGE_MAX_KEY_SIZE) ||
(value_size > HV_KVP_EXCHANGE_MAX_VALUE_SIZE)) {
- syslog(LOG_ERR, "%s: Too long key or value: key=%s, val=%s",
- __func__, key, value);
-
- if (debug)
- syslog(LOG_DEBUG, "%s: Too long key or value: pool=%d, key=%s, val=%s",
- __func__, pool, key, value);
+ syslog(LOG_ERR, "%s: Too long key or value: pool=%d key=%s, val=%s",
+ __func__, pool, key, value);
return 1;
}
+ if (debug)
+ syslog(LOG_DEBUG, "%s: got a KVP: pool=%d key=%s val=%s",
+ __func__, pool, key, value);
+
/*
* First update the in-memory state.
*/
base-commit: 4325743c7e209ae7845293679a4de94b969f2bef
--
2.34.1
^ permalink raw reply related
* Re: [PATCH net-next v3 0/4] Support bandwidth clamping in mana using net shapers
From: patchwork-bot+netdevbpf @ 2025-06-19 13:30 UTC (permalink / raw)
To: Erni Sri Satya Vennela
Cc: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
kuba, pabeni, longli, kotaranov, horms, shirazsaleem, leon,
shradhagupta, schakrabarti, gerhard, rosenp, sdf, linux-hyperv,
netdev, linux-kernel, linux-rdma
In-Reply-To: <1750144656-2021-1-git-send-email-ernis@linux.microsoft.com>
Hello:
This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:
On Tue, 17 Jun 2025 00:17:32 -0700 you wrote:
> This patchset introduces hardware-backed bandwidth rate limiting
> for MANA NICs via the net_shaper_ops interface, enabling efficient and
> fine-grained traffic shaping directly on the device.
>
> Previously, MANA lacked a mechanism for user-configurable bandwidth
> control. With this addition, users can now configure shaping parameters,
> allowing better traffic management and performance isolation.
>
> [...]
Here is the summary with links:
- [net-next,v3,1/4] net: mana: Fix potential deadlocks in mana napi ops
https://git.kernel.org/netdev/net-next/c/d5c8f0e4e0cb
- [net-next,v3,2/4] net: mana: Add support for net_shaper_ops
https://git.kernel.org/netdev/net-next/c/75cabb46935b
- [net-next,v3,3/4] net: mana: Add speed support in mana_get_link_ksettings
https://git.kernel.org/netdev/net-next/c/a6d5edf11e0c
- [net-next,v3,4/4] net: mana: Handle unsupported HWC commands
https://git.kernel.org/netdev/net-next/c/ca8ac489ca33
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* Re: [Patch net-next v3] net: mana: Record doorbell physical address in PF mode
From: Simon Horman @ 2025-06-19 13:36 UTC (permalink / raw)
To: longli
Cc: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Shradha Gupta, Konstantin Taranov, Souradeep Chakrabarti,
Erick Archer, linux-hyperv, netdev, linux-kernel, linux-rdma
In-Reply-To: <1750210606-12167-1-git-send-email-longli@linuxonhyperv.com>
On Tue, Jun 17, 2025 at 06:36:46PM -0700, longli@linuxonhyperv.com wrote:
> From: Long Li <longli@microsoft.com>
>
> MANA supports RDMA in PF mode. The driver should record the doorbell
> physical address when in PF mode.
>
> The doorbell physical address is used by the RDMA driver to map
> doorbell pages of the device to user-mode applications through RDMA
> verbs interface. In the past, they have been mapped to user-mode while
> the device is in VF mode. With the support for PF mode implemented,
> also expose those pages in PF mode.
>
> Support for PF mode is implemented in
> 290e5d3c49f6 ("net: mana: Add support for Multi Vports on Bare metal")
>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
> Changes
> v2: add more details in commit message on how the doorbell physical address is used
> v3: add the early commit detail where the support for PF mode is implemented
Thanks for the updates.
I agree that this revision address Jakub's feedback on v2.
Which in turn addressed his feedback on v1.
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply
* RE: [PATCH 3/4] x86: hyperv: Expose hv_map_msi_interrupt function
From: Michael Kelley @ 2025-06-19 22:02 UTC (permalink / raw)
To: Nuno Das Neves, linux-hyperv@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Cc: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
decui@microsoft.com, catalin.marinas@arm.com, will@kernel.org,
tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
dave.hansen@linux.intel.com, hpa@zytor.com, lpieralisi@kernel.org,
kw@linux.com, robh@kernel.org, bhelgaas@google.com,
jinankjain@linux.microsoft.com, skinsburskii@linux.microsoft.com,
mrathor@linux.microsoft.com, x86@kernel.org
In-Reply-To: <8f96db3f-fc3b-44b6-ab28-26bca6e2615b@linux.microsoft.com>
From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Wednesday, June 18, 2025 2:08 PM
>
> On 6/11/2025 4:07 PM, Michael Kelley wrote:
> > From: Nuno Das Neves <nunodasneves@linux.microsoft.com> Sent: Tuesday, June
> 10, 2025 4:52 PM
> >>
> >> From: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
> >
> > The preferred patch Subject prefix is "x86/hyperv:"
> >
>
> Thank you for clarifying - I thought I saw some precedent for x86: hyperv:
> but must have been mistaken.
>
> >>
> >> This patch moves a part of currently internal logic into the
> >> hv_map_msi_interrupt function and makes it globally available helper
> >> function, which will be used to map PCI interrupts in case of root
> >> partition.
> >
> > Avoid "this patch" in commit messages. Suggest:
> >
> > Create a helper function hv_map_msi_interrupt() that contains some
> > logic that is currently internal to irqdomain.c. Make the helper function
> > globally available so it can be used to map PCI interrupts when running
> > in the root partition.
> >
>
> Thanks, I'll rephrase.
>
> >>
> >> Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
> >> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
> >> ---
> >> arch/x86/hyperv/irqdomain.c | 47 ++++++++++++++++++++++++---------
> >> arch/x86/include/asm/mshyperv.h | 2 ++
> >> 2 files changed, 36 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/arch/x86/hyperv/irqdomain.c b/arch/x86/hyperv/irqdomain.c
> >> index 31f0d29cbc5e..82f3bafb93d6 100644
> >> --- a/arch/x86/hyperv/irqdomain.c
> >> +++ b/arch/x86/hyperv/irqdomain.c
> >> @@ -169,13 +169,40 @@ static union hv_device_id hv_build_pci_dev_id(struct pci_dev *dev)
> >> return dev_id;
> >> }
> >>
> >> -static int hv_map_msi_interrupt(struct pci_dev *dev, int cpu, int vector,
> >> - struct hv_interrupt_entry *entry)
> >> +/**
> >> + * hv_map_msi_interrupt() - "Map" the MSI IRQ in the hypervisor.
> >> + * @data: Describes the IRQ
> >> + * @out_entry: Hypervisor (MSI) interrupt entry (can be NULL)
> >> + *
> >> + * Map the IRQ in the hypervisor by issuing a MAP_DEVICE_INTERRUPT hypercall.
> >> + */
> >> +int hv_map_msi_interrupt(struct irq_data *data,
> >> + struct hv_interrupt_entry *out_entry)
> >> {
> >> - union hv_device_id device_id = hv_build_pci_dev_id(dev);
> >> + struct msi_desc *msidesc;
> >> + struct pci_dev *dev;
> >> + union hv_device_id device_id;
> >> + struct hv_interrupt_entry dummy;
> >> + struct irq_cfg *cfg = irqd_cfg(data);
> >> + const cpumask_t *affinity;
> >> + int cpu;
> >> + u64 res;
> >>
> >> - return hv_map_interrupt(device_id, false, cpu, vector, entry);
> >> + msidesc = irq_data_get_msi_desc(data);
> >> + dev = msi_desc_to_pci_dev(msidesc);
> >> + device_id = hv_build_pci_dev_id(dev);
> >> + affinity = irq_data_get_effective_affinity_mask(data);
> >> + cpu = cpumask_first_and(affinity, cpu_online_mask);
> >
> > Is the cpus_read_lock held at this point? I'm not sure what the
> > overall calling sequence looks like. If it is not held, the CPU that
> > is selected could go offline before hv_map_interrupt() is called.
> > This computation of the target CPU is the same as in the code
> > before this patch, but that existing code looks like it has the
> > same problem.
> >
>
> Thanks for pointing it out - It *looks* like the read lock is not held
> everywhere this could be called, so it could indeed be a problem.
>
> I've been thinking about different ways around this but I lack the
> knowledge to have an informed opinion about it:
>
> - We could take the cpu read lock in this function, would that work?
>
> - I'm not actually sure why the code is getting the first cpu off the effective
> affinity mask in the first place. It is possible to get the apic id (and hence
> the cpu) already associated with the irq, as per e.g. x86_vector_msi_compose_msg()
> Maybe we could get the cpu that way, assuming that doesn't have a similar issue.
>
> - We could just let this race happen, maybe the outcome isn't too catastrophic?
>
> What do you think?
I would have to study further to provide good answers to your questions as
I don't have deep knowledge of this area off the top of my head. The code
looked suspicious because AND'ing the affinity with the cpu_online_mask in
the first place is presumably to prevent assigning the interrupt to a CPU
that is offline. That's a valid intent, since such assigning would indeed be
problematic.
But as written the code is inherently racy unless the cpus_read_lock() is
held. I'm on vacation all next week, and probably won't be able to look at
this again until early July. So the best I can do for now is flag the issue.
Michael
>
> >> +
> >> + res = hv_map_interrupt(device_id, false, cpu, cfg->vector,
> >> + out_entry ? out_entry : &dummy);
> >> + if (!hv_result_success(res))
> >> + pr_err("%s: failed to map interrupt: %s",
> >> + __func__, hv_result_to_string(res));
> >
> > hv_map_interrupt() already outputs a message if the hypercall
> > fails. Is another message needed here?
> >
>
> It does print the function name, which gives additional context.
> Probably it can just be removed however.
>
> >> +
> >> + return hv_result_to_errno(res);
> >
> > The error handling is rather messed up. First hv_map_interrupt()
> > sometimes returns a Linux errno (not negated), and sometimes a
> > hypercall result. The errno is EINVAL, which has value "22", which is
> > the same as hypercall result HV_STATUS_ACKNOWLEDGED. And
> > the hypercall result returned from hv_map_interrupt() is just
> > the result code, not the full 64-bit status, as hv_map_interrupt()
> > has already done hv_result(status). Hence the "res" input arg to
> > hv_result_to_errno() isn't really the correct input. For example,
> > if the hypercall returns U64_MAX, that won't be caught by
> > hv_result_to_errno() since the value has been truncated to
> > 32 bits. Fixing all this will require some unscrambling.
> >
>
> Good point, it's pretty messed up! I think in v2 I'll add a patch to
> clean this up first.
>
> >> }
> >> +EXPORT_SYMBOL_GPL(hv_map_msi_interrupt);
> >>
> >> static inline void entry_to_msi_msg(struct hv_interrupt_entry *entry, struct
> msi_msg *msg)
> >> {
> >> @@ -190,10 +217,8 @@ static void hv_irq_compose_msi_msg(struct irq_data
> *data, struct msi_msg *msg)
> >> {
> >> struct msi_desc *msidesc;
> >> struct pci_dev *dev;
> >> - struct hv_interrupt_entry out_entry, *stored_entry;
> >> + struct hv_interrupt_entry *stored_entry;
> >> struct irq_cfg *cfg = irqd_cfg(data);
> >> - const cpumask_t *affinity;
> >> - int cpu;
> >> u64 status;
> >>
> >> msidesc = irq_data_get_msi_desc(data);
> >> @@ -204,9 +229,6 @@ static void hv_irq_compose_msi_msg(struct irq_data *data,
> struct msi_msg *msg)
> >> return;
> >> }
> >>
> >> - affinity = irq_data_get_effective_affinity_mask(data);
> >> - cpu = cpumask_first_and(affinity, cpu_online_mask);
> >> -
> >> if (data->chip_data) {
> >> /*
> >> * This interrupt is already mapped. Let's unmap first.
> >> @@ -235,15 +257,14 @@ static void hv_irq_compose_msi_msg(struct irq_data
> *data, struct msi_msg *msg)
> >> return;
> >> }
> >>
> >> - status = hv_map_msi_interrupt(dev, cpu, cfg->vector, &out_entry);
> >> + status = hv_map_msi_interrupt(data, stored_entry);
> >> if (status != HV_STATUS_SUCCESS) {
> >
> > hv_map_msi_interrupt() returns an errno, so testing for HV_STATUS_SUCCESS
> > is bogus.
> >
>
> Thanks, noted.
>
> >> kfree(stored_entry);
> >> return;
> >> }
> >>
> >> - *stored_entry = out_entry;
> >> data->chip_data = stored_entry;
> >> - entry_to_msi_msg(&out_entry, msg);
> >> + entry_to_msi_msg(data->chip_data, msg);
> >>
> >> return;
> >> }
> >> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> >> index 5ec92e3e2e37..843121465ddd 100644
> >> --- a/arch/x86/include/asm/mshyperv.h
> >> +++ b/arch/x86/include/asm/mshyperv.h
> >> @@ -261,6 +261,8 @@ static inline void hv_apic_init(void) {}
> >>
> >> struct irq_domain *hv_create_pci_msi_domain(void);
> >>
> >> +int hv_map_msi_interrupt(struct irq_data *data,
> >> + struct hv_interrupt_entry *out_entry);
> >> int hv_map_ioapic_interrupt(int ioapic_id, bool level, int vcpu, int vector,
> >> struct hv_interrupt_entry *entry);
> >> int hv_unmap_ioapic_interrupt(int ioapic_id, struct hv_interrupt_entry *entry);
> >> --
> >> 2.34.1
^ permalink raw reply
* Re: [PATCH net-next] net: mana: Set tx_packets to post gso processing packet count
From: patchwork-bot+netdevbpf @ 2025-06-19 23:00 UTC (permalink / raw)
To: Shradha Gupta
Cc: decui, wei.liu, haiyangz, kys, andrew+netdev, davem, edumazet,
kuba, pabeni, kotaranov, schakrabarti, ernis, longli, dipayanroy,
shirazsaleem, netdev, linux-hyperv, linux-kernel, paulros,
shradhagupta
In-Reply-To: <1750160021-24589-1-git-send-email-shradhagupta@linux.microsoft.com>
Hello:
This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:
On Tue, 17 Jun 2025 04:33:41 -0700 you wrote:
> Allow tx_packets and tx_bytes counter in the driver to represent
> the packets transmitted post GSO processing.
>
> Currently they are populated as bigger pre-GSO packets and bytes
>
> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
>
> [...]
Here is the summary with links:
- [net-next] net: mana: Set tx_packets to post gso processing packet count
https://git.kernel.org/netdev/net-next/c/7399ef984022
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply
* [PATCH v4 0/2] vmbus: Add DeviceTree support for message connection-id
From: Hardik Garg @ 2025-06-19 23:06 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, robh, krzk+dt, conor+dt
Cc: devicetree, linux-hyperv, linux-kernel, ssengar, hargar, apais,
Hardik Garg
This patch series adds support for configuring the VMBus message connection-id
through DeviceTree. The connection-id determines which hypervisor communication
channel the guest should use to talk to the VMBus host.
Changes in v4:
- Split the patch into two separate patches:
* DeviceTree bindings documentation
* Implementation changes
- Fix warnings reported by checkpatch
Changes in v3:
- https://lore.kernel.org/all/6a92ca86-ad6b-4d49-af6e-1ed7651b8ab8@linux.microsoft.com/
Changes in v2:
- https://lore.kernel.org/all/096edaf7-cc90-42b6-aff4-c5f088574e1e@linux.microsoft.com/
Changes in v1:
- https://lore.kernel.org/all/6acee4bf-cb04-43b9-9476-e8d811d26dfd@linux.microsoft.com/
The series consists of two patches:
1. dt-bindings: microsoft: Add vmbus message-connection-id property
2. vmbus: retrieve connection-id from DeviceTree
Testing:
- Tested on Microsoft Hyper-V
- Verified with and without the DeviceTree property
- Confirmed proper fallback to default values
- Validated binding documentation with dt_binding_check
Hardik Garg (2):
dt-bindings: microsoft: Add vmbus message-connection-id property
vmbus: retrieve connection-id from DeviceTree
Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml | 9 +++++++++
drivers/hv/connection.c | 6 ++++--
drivers/hv/vmbus_drv.c | 13 +++++++++++++
3 files changed, 26 insertions(+), 2 deletions(-)
--
2.40.4
^ permalink raw reply
* [PATCH v4 1/2] dt-bindings: microsoft: Add vmbus message-connection-id property
From: Hardik Garg @ 2025-06-19 23:06 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, robh, krzk+dt, conor+dt
Cc: devicetree, linux-hyperv, linux-kernel, ssengar, hargar, apais,
Hardik Garg
In-Reply-To: <1750374395-14615-1-git-send-email-hargar@linux.microsoft.com>
Document the microsoft,message-connection-id property for VMBus DeviceTree
node. This property allows specifying the connection ID used for
communication between host and guest.
Signed-off-by: Hardik Garg <hargar@linux.microsoft.com>
---
v3: https://lore.kernel.org/all/6a92ca86-ad6b-4d49-af6e-1ed7651b8ab8@linux.microsoft.com
v2: https://lore.kernel.org/all/096edaf7-cc90-42b6-aff4-c5f088574e1e@linux.microsoft.com
v1: https://lore.kernel.org/all/6acee4bf-cb04-43b9-9476-e8d811d26dfd@linux.microsoft.com
---
Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
index 0bea4f5287ce..615b48bd6a8b 100644
--- a/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
+++ b/Documentation/devicetree/bindings/bus/microsoft,vmbus.yaml
@@ -17,6 +17,14 @@ properties:
compatible:
const: microsoft,vmbus
+ microsoft,message-connection-id:
+ description: |
+ VMBus message connection ID to be used for communication between host and
+ guest. If not specified, defaults to VMBUS_MESSAGE_CONNECTION_ID_4 (4) for
+ protocol version VERSION_WIN10_V5 and above, or VMBUS_MESSAGE_CONNECTION_ID
+ (1) for older versions.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
ranges: true
'#address-cells':
@@ -55,6 +63,7 @@ examples:
vmbus@ff0000000 {
compatible = "microsoft,vmbus";
+ microsoft,message-connection-id = <4>;
#address-cells = <2>;
#size-cells = <1>;
ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
--
2.40.4
^ permalink raw reply related
* [PATCH v4 2/2] vmbus: retrieve connection-id from DeviceTree
From: Hardik Garg @ 2025-06-19 23:06 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, robh, krzk+dt, conor+dt
Cc: devicetree, linux-hyperv, linux-kernel, ssengar, hargar, apais,
Hardik Garg
In-Reply-To: <1750374395-14615-1-git-send-email-hargar@linux.microsoft.com>
The connection-id determines which hypervisor communication channel the
guest should use to talk to the VMBus host. This patch adds support to
read this value from the DeviceTree where it exists as a property under
the vmbus node with the compatible ID "microsoft,message-connection-id".
The property name follows the format <vendor>,<field> where
"vendor": "microsoft" and "field": "message-connection-id"
Reading from DeviceTree allows platforms to specify their preferred
communication channel, making it more flexible. If the property is
not found in the DeviceTree, use the default connection ID
(VMBUS_MESSAGE_CONNECTION_ID or VMBUS_MESSAGE_CONNECTION_ID_4
based on protocol version).
Signed-off-by: Hardik Garg <hargar@linux.microsoft.com>
---
v3: https://lore.kernel.org/all/6a92ca86-ad6b-4d49-af6e-1ed7651b8ab8@linux.microsoft.com
v2: https://lore.kernel.org/all/096edaf7-cc90-42b6-aff4-c5f088574e1e@linux.microsoft.com
v1: https://lore.kernel.org/all/6acee4bf-cb04-43b9-9476-e8d811d26dfd@linux.microsoft.com
---
drivers/hv/connection.c | 6 ++++--
drivers/hv/vmbus_drv.c | 13 +++++++++++++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index be490c598785..15d2b652783d 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -99,11 +99,13 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
if (version >= VERSION_WIN10_V5) {
msg->msg_sint = VMBUS_MESSAGE_SINT;
msg->msg_vtl = ms_hyperv.vtl;
- vmbus_connection.msg_conn_id = VMBUS_MESSAGE_CONNECTION_ID_4;
} else {
msg->interrupt_page = virt_to_phys(vmbus_connection.int_page);
- vmbus_connection.msg_conn_id = VMBUS_MESSAGE_CONNECTION_ID;
}
+ /* Set default connection ID if not provided via DeviceTree */
+ if (!vmbus_connection.msg_conn_id)
+ vmbus_connection.msg_conn_id = (version >= VERSION_WIN10_V5) ?
+ VMBUS_MESSAGE_CONNECTION_ID_4 : VMBUS_MESSAGE_CONNECTION_ID;
/*
* shared_gpa_boundary is zero in non-SNP VMs, so it's safe to always
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index c236081d0a87..b78d5499e4bc 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2541,10 +2541,23 @@ static int vmbus_device_add(struct platform_device *pdev)
struct of_range range;
struct of_range_parser parser;
struct device_node *np = pdev->dev.of_node;
+ unsigned int conn_id;
int ret;
vmbus_root_device = &pdev->dev;
+ /*
+ * Read connection ID from DeviceTree. The property name follows the
+ * format <vendor>,<field> where:
+ * - vendor: "microsoft"
+ * - field: "message-connection-id"
+ */
+ ret = of_property_read_u32(np, "microsoft,message-connection-id", &conn_id);
+ if (!ret) {
+ pr_info("VMBus message connection ID: %u\n", conn_id);
+ vmbus_connection.msg_conn_id = conn_id;
+ }
+
ret = of_range_parser_init(&parser, np);
if (ret)
return ret;
--
2.40.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox