* Re: [PATCH v2 1/2] drm/hyperv: validate resolution_count and harden VSP request paths
From: sashiko-bot @ 2026-05-17 14:47 UTC (permalink / raw)
To: Berkant Koc; +Cc: linux-hyperv
In-Reply-To: <20260517-drm-hyperv-patch1-v2@berkoc.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The fix for delayed VSP responses is incomplete, leaving a critical data race on the shared `hv->init_buf` that allows type confusion and out-of-bounds memory reads.
--
commit 3d049f39a946f7898e25e92a422ae95b066e9989
Author: Berkant Koc <me@berkoc.com>
drm/hyperv: validate resolution_count and harden VSP request paths
This commit adds bounds checking for resolution_count when parsing
SYNTHVID_RESOLUTION_RESPONSE. It degrades gracefully to default
resolutions on failure, and adds reinit_completion() before VSP requests
to prevent consuming stale completions.
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
[ ... ]
> @@ -383,6 +385,7 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
> sizeof(struct synthvid_supported_resolution_req);
> msg->resolution_req.maximum_resolution_count =
> SYNTHVID_MAX_RESOLUTION_COUNT;
> + reinit_completion(&hv->wait);
> hyperv_sendpacket(hdev, msg);
Does this reinit_completion() prevent a data race on the shared hv->init_buf?
If a timed-out request's delayed response arrives while a new request is
being constructed, could hyperv_receive_sub() concurrently overwrite
hv->init_buf without holding any locks?
Since the buffer is used for both sending and receiving, if a delayed
response arrives exactly between the memset(msg, 0) and
hyperv_sendpacket(hdev, msg), could msg->vid_hdr.size be overwritten by
the host's response?
If a host provides a crafted size, could this cause an integer overflow in
totalbytes_towrite within hv_ringbuffer_write(), bypassing ring buffer
free-space checks and causing an out-of-bounds read of guest memory into
the VMBus ring buffer?
Additionally, if the delayed response triggers complete(&hv->wait), is
there a chance the driver parses a mismatched response because it does not
validate that msg->vid_hdr.type matches the expected response?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-drm-hyperv-cover-v2@berkoc.com?part=1
^ permalink raw reply
* Re: [PATCH 2/2] drm/hyperv: validate VMBus packet size in receive callback
From: sashiko-bot @ 2026-05-17 14:17 UTC (permalink / raw)
To: Berkant Koc; +Cc: linux-hyperv
In-Reply-To: <20260517-drm-hyperv-patch2@berkoc.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete payload size validation causes wait-completion handlers to read stale request data, allowing response spoofing and invalid state.
--
commit ffe1d9057aa46275f644c2e152d0e7a9e8837c3b
Author: Berkant Koc <me@berkoc.com>
drm/hyperv: validate VMBus packet size in receive callback
This commit introduces bytes_recvd into hyperv_receive_sub() to validate
packet sizes. It restricts the memcpy length to bytes_recvd and checks
payload size for feature change messages.
Cc: stable@vger.kernel.org # 5.14+
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
[ ... ]
> @@ -420,26 +420,35 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
> static void hyperv_receive_sub(struct hv_device *hdev, u32 bytes_recvd)
> {
> struct hyperv_drm_device *hv = hv_get_drvdata(hdev);
> struct synthvid_msg *msg;
> + size_t hdr_size;
>
> if (!hv)
> return;
>
> + hdr_size = sizeof(struct pipe_msg_hdr) +
> + sizeof(struct synthvid_msg_hdr);
> + if (bytes_recvd < hdr_size || bytes_recvd > VMBUS_MAX_PACKET_SIZE)
> + return;
> +
> msg = (struct synthvid_msg *)hv->recv_buf;
>
> /* Complete the wait event */
> if (msg->vid_hdr.type == SYNTHVID_VERSION_RESPONSE ||
> msg->vid_hdr.type == SYNTHVID_RESOLUTION_RESPONSE ||
> msg->vid_hdr.type == SYNTHVID_VRAM_LOCATION_ACK) {
> - memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE);
> + memcpy(hv->init_buf, msg, bytes_recvd);
Could this change introduce a regression where a short packet leaves stale
request data in hv->init_buf?
The patch bounds the memcpy to bytes_recvd, but it doesn't appear to validate
that bytes_recvd is large enough to contain the expected response payloads
before calling complete(&hv->wait).
Because hv->init_buf is used for both sending requests and receiving responses,
a short response might only partially overwrite hv->init_buf. The payload
portion would retain the outgoing request's data or zeroes from the pre-send
initialization.
For example, when sending a VRAM location update, the driver sets
msg->vram.user_ctx = vram_pp. If a short response arrives, the payload is not
overwritten. The waiter checks if (msg->vram_ack.user_ctx != vram_pp). Since
vram and vram_ack share a union in struct synthvid_msg, this check might pass
against the driver's own request data, allowing a truncated packet to spoof a
successful ACK.
Similarly, in hyperv_get_supported_resolution(), if a truncated response
includes the resolution_count but omits the supported_resolution array, the
uncopied array memory remains zero. The driver could parse zeroed dimensions,
setting hv->preferred_width and height to 0, which could propagate to the DRM
core.
Would it be safer to have the receive handler validate the specific payload
sizes for these wait-completion response types, similar to how it validates
SYNTHVID_FEATURE_CHANGE below?
> complete(&hv->wait);
> return;
> }
>
> if (msg->vid_hdr.type == SYNTHVID_FEATURE_CHANGE) {
> + if (bytes_recvd < hdr_size +
> + sizeof(struct synthvid_feature_change))
> + return;
> hv->dirt_needed = msg->feature_chg.is_dirt_needed;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-drm-hyperv-cover@berkoc.com?part=2
^ permalink raw reply
* [PATCH v2 2/2] drm/hyperv: validate VMBus packet size in receive callback
From: Berkant Koc @ 2026-05-17 14:25 UTC (permalink / raw)
To: Saurabh Sengar, Dexuan Cui, Long Li
Cc: linux-hyperv, dri-devel, linux-kernel, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Michael Kelley, Thomas Zimmermann,
Maarten Lankhorst, Maxime Ripard, Deepak Rawat, stable
In-Reply-To: <20260517-drm-hyperv-cover-v2@berkoc.com>
hyperv_receive() reads bytes_recvd from vmbus_recvpacket() but does not
forward that value to hyperv_receive_sub(). The sub-handler reads
msg->vid_hdr.type and msg->feature_chg.is_dirt_needed without knowing
how many bytes the host actually wrote, so a short packet leaves the
parser reading bytes that the host did not write in this packet. The
unconditional memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE) on the
wait-completion path also copies the full 16 KiB recv_buf regardless
of bytes_recvd, including any residue from the prior message.
Pass bytes_recvd into hyperv_receive_sub() and reject any packet shorter
than the pipe + synthvid header. The dirt-feature branch additionally
requires the feature_change payload size before reading is_dirt_needed.
The init_buf copy now uses bytes_recvd as the length argument, which
keeps it bounded by VMBUS_MAX_PACKET_SIZE through the new upper check.
Unchanged from v1.
Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
Cc: stable@vger.kernel.org # 5.14+
Signed-off-by: Berkant Koc <me@berkoc.com>
---
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
index 3b5065fe06e4..cdab4895dd40 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
@@ -423,26 +423,35 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
return 0;
}
-static void hyperv_receive_sub(struct hv_device *hdev)
+static void hyperv_receive_sub(struct hv_device *hdev, u32 bytes_recvd)
{
struct hyperv_drm_device *hv = hv_get_drvdata(hdev);
struct synthvid_msg *msg;
+ size_t hdr_size;
if (!hv)
return;
+ hdr_size = sizeof(struct pipe_msg_hdr) +
+ sizeof(struct synthvid_msg_hdr);
+ if (bytes_recvd < hdr_size || bytes_recvd > VMBUS_MAX_PACKET_SIZE)
+ return;
+
msg = (struct synthvid_msg *)hv->recv_buf;
/* Complete the wait event */
if (msg->vid_hdr.type == SYNTHVID_VERSION_RESPONSE ||
msg->vid_hdr.type == SYNTHVID_RESOLUTION_RESPONSE ||
msg->vid_hdr.type == SYNTHVID_VRAM_LOCATION_ACK) {
- memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE);
+ memcpy(hv->init_buf, msg, bytes_recvd);
complete(&hv->wait);
return;
}
if (msg->vid_hdr.type == SYNTHVID_FEATURE_CHANGE) {
+ if (bytes_recvd < hdr_size +
+ sizeof(struct synthvid_feature_change))
+ return;
hv->dirt_needed = msg->feature_chg.is_dirt_needed;
if (hv->dirt_needed)
hyperv_hide_hw_ptr(hv->hdev);
@@ -469,7 +478,7 @@ static void hyperv_receive(void *ctx)
&bytes_recvd, &req_id);
if (bytes_recvd > 0 &&
recv_buf->pipe_hdr.type == PIPE_MSG_DATA)
- hyperv_receive_sub(hdev);
+ hyperv_receive_sub(hdev, bytes_recvd);
} while (bytes_recvd > 0 && ret == 0);
}
--
2.47.3
^ permalink raw reply related
* [PATCH v2 1/2] drm/hyperv: validate resolution_count and harden VSP request paths
From: Berkant Koc @ 2026-05-17 14:25 UTC (permalink / raw)
To: Saurabh Sengar, Dexuan Cui, Long Li
Cc: linux-hyperv, dri-devel, linux-kernel, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Michael Kelley, Thomas Zimmermann,
Maarten Lankhorst, Maxime Ripard, Deepak Rawat, stable
In-Reply-To: <20260517-drm-hyperv-cover-v2@berkoc.com>
The synthetic video device parses a SYNTHVID_RESOLUTION_RESPONSE that
contains a u8 resolution_count and a u8 default_resolution_index. The
existing check rejects resolution_count == 0 and an index greater or
equal to resolution_count, but does not bound resolution_count itself
against the fixed supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT]
array. A host that returns resolution_count > 64 together with an
in-range default_resolution_index causes the subsequent loop to read
past the array. Reject any resolution_count that exceeds the array
bound, folded into the existing zero-check so a single log entry
remains per failure.
When that bounds check (or any later failure in
hyperv_get_supported_resolution()) returns an error, the caller in
hyperv_connect_vsp() previously logged a warning and continued without
populating hv->screen_width_max / hv->screen_height_max / preferred_*.
hyperv_mode_config_init() then set dev->mode_config.max_width and
max_height to 0, which makes drm_internal_framebuffer_create() reject
every userspace framebuffer with -EINVAL. Populate the fields with the
WIN8 defaults that the pre-WIN10 branch already uses so a failed
resolution probe degrades to a usable display instead of disabling it.
The driver also issues three sequential VSP requests (negotiate
version, update VRAM location, get supported resolution) that share a
single hv->wait completion. None of the call sites call
reinit_completion() between requests. If wait_for_completion_timeout()
returns 0 but a delayed response later triggers complete(&hv->wait) in
the receive callback, the next request's wait can consume that stale
completion, return immediately, and parse stale data out of
hv->init_buf. Call reinit_completion() before each send so every
request waits for its own response.
Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
Cc: stable@vger.kernel.org # 5.14+
Signed-off-by: Berkant Koc <me@berkoc.com>
---
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
index 051ecc526832..3b5065fe06e4 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
@@ -223,6 +223,7 @@ static int hyperv_negotiate_version(struct hv_device *hdev, u32 ver)
msg->vid_hdr.size = sizeof(struct synthvid_msg_hdr) +
sizeof(struct synthvid_version_req);
msg->ver_req.version = ver;
+ reinit_completion(&hv->wait);
hyperv_sendpacket(hdev, msg);
t = wait_for_completion_timeout(&hv->wait, VMBUS_VSP_TIMEOUT);
@@ -257,6 +258,7 @@ int hyperv_update_vram_location(struct hv_device *hdev, phys_addr_t vram_pp)
msg->vram.user_ctx = vram_pp;
msg->vram.vram_gpa = vram_pp;
msg->vram.is_vram_gpa_specified = 1;
+ reinit_completion(&hv->wait);
hyperv_sendpacket(hdev, msg);
t = wait_for_completion_timeout(&hv->wait, VMBUS_VSP_TIMEOUT);
@@ -383,6 +385,7 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
sizeof(struct synthvid_supported_resolution_req);
msg->resolution_req.maximum_resolution_count =
SYNTHVID_MAX_RESOLUTION_COUNT;
+ reinit_completion(&hv->wait);
hyperv_sendpacket(hdev, msg);
t = wait_for_completion_timeout(&hv->wait, VMBUS_VSP_TIMEOUT);
@@ -391,8 +394,11 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
return -ETIMEDOUT;
}
- if (msg->resolution_resp.resolution_count == 0) {
- drm_err(dev, "No supported resolutions\n");
+ if (msg->resolution_resp.resolution_count == 0 ||
+ msg->resolution_resp.resolution_count >
+ SYNTHVID_MAX_RESOLUTION_COUNT) {
+ drm_err(dev, "Invalid resolution count: %d\n",
+ msg->resolution_resp.resolution_count);
return -ENODEV;
}
@@ -506,8 +512,13 @@ int hyperv_connect_vsp(struct hv_device *hdev)
if (hyperv_version_ge(hv->synthvid_version, SYNTHVID_VERSION_WIN10)) {
ret = hyperv_get_supported_resolution(hdev);
- if (ret)
+ if (ret) {
drm_err(dev, "Failed to get supported resolution from host, use default\n");
+ hv->screen_width_max = SYNTHVID_WIDTH_WIN8;
+ hv->screen_height_max = SYNTHVID_HEIGHT_WIN8;
+ hv->preferred_width = SYNTHVID_WIDTH_WIN8;
+ hv->preferred_height = SYNTHVID_HEIGHT_WIN8;
+ }
} else {
hv->screen_width_max = SYNTHVID_WIDTH_WIN8;
hv->screen_height_max = SYNTHVID_HEIGHT_WIN8;
--
2.47.3
^ permalink raw reply related
* [PATCH v2 0/2] drm/hyperv: harden VMBus message parser input validation
From: Berkant Koc @ 2026-05-17 14:25 UTC (permalink / raw)
To: Saurabh Sengar, Dexuan Cui, Long Li
Cc: linux-hyperv, dri-devel, linux-kernel, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Michael Kelley, Thomas Zimmermann,
Maarten Lankhorst, Maxime Ripard, Deepak Rawat
In-Reply-To: <20260517-drm-hyperv-cover@berkoc.com>
v2 folds two further issues into patch 1 that the sashiko-bot review
pointed out on v1:
1. The resolution_count bounds check in v1 returned -ENODEV, but
hyperv_connect_vsp() only logged a warning and continued without
setting hv->screen_width_max / height_max / preferred_*. That
left dev->mode_config.max_width and max_height at 0, which made
drm_internal_framebuffer_create() reject every userspace
framebuffer with -EINVAL. v2 falls back to the WIN8 defaults on
that error path, matching the pre-WIN10 branch.
2. The three sequential VSP requests in hyperv_connect_vsp()
(negotiate version, update VRAM location, get supported
resolution) all wait on the same hv->wait completion without
calling reinit_completion() between requests. A delayed
complete() after a wait_for_completion_timeout() can leak into
the next request and let it parse stale data out of
hv->init_buf. v2 calls reinit_completion() before each send.
Patch 2 is unchanged from v1.
v1: https://lore.kernel.org/r/20260517-drm-hyperv-cover@berkoc.com
Berkant Koc (2):
drm/hyperv: validate resolution_count and harden VSP request paths
drm/hyperv: validate VMBus packet size in receive callback
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 32 ++++++++++++++++++-----
1 file changed, 26 insertions(+), 6 deletions(-)
base-commit: 6916d5703ddf9a38f1f6c2cc793381a24ee914c6
--
2.47.3
^ permalink raw reply
* Re: [PATCH 1/2] drm/hyperv: validate resolution_count from host VMBus message
From: sashiko-bot @ 2026-05-17 13:49 UTC (permalink / raw)
To: Berkant Koc; +Cc: linux-hyperv
In-Reply-To: <20260517-drm-hyperv-patch1@berkoc.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Uninitialized State on Error Path Leaves Display Unusable
- [High] Missing Completion Re-Initialization Breaks Driver Synchronization
--
commit 94c5684ae582179bbf9c456d94f6a0bd0f92785f
Author: Berkant Koc <me@berkoc.com>
Subject: drm/hyperv: validate resolution_count from host VMBus message
This patch adds validation to ensure the host-provided resolution_count does
not exceed SYNTHVID_MAX_RESOLUTION_COUNT. It folds this check into the
existing zero-count check to prevent reading past the end of the supported
resolution array.
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> index 051ecc5268321..003bb118d64c9 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
> @@ -391,8 +391,11 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
> return -ETIMEDOUT;
> }
This isn't a bug in your patch, but looking at the wait_for_completion_timeout
call just before this hunk, does the driver reuse the hv->wait completion
structure for multiple sequential VMBus requests without calling
reinit_completion()?
If a previous request times out but the host sends a delayed response, the
receive callback might execute complete(&hv->wait). Could a subsequent
request's wait_for_completion_timeout incorrectly consume this stray
completion, return immediately, and parse stale data from hv->init_buf?
>
> - if (msg->resolution_resp.resolution_count == 0) {
> - drm_err(dev, "No supported resolutions\n");
> + if (msg->resolution_resp.resolution_count == 0 ||
> + msg->resolution_resp.resolution_count >
> + SYNTHVID_MAX_RESOLUTION_COUNT) {
> + drm_err(dev, "Invalid resolution count: %d\n",
> + msg->resolution_resp.resolution_count);
> return -ENODEV;
This is also a pre-existing issue, but I noticed that returning an error
here can leave the display unusable.
When hyperv_get_supported_resolution() fails due to this new bounds check,
the caller hyperv_connect_vsp() logs a warning to use defaults, but it
skips setting up the resolution limits. As a result, hv->screen_width_max
and hv->screen_height_max remain 0.
Later, hyperv_mode_config_init() sets dev->mode_config.max_width and
max_height to 0. Since the DRM core's drm_internal_framebuffer_create()
strictly bounds framebuffer dimensions to dev->mode_config.max_width,
would this cause all userspace attempts to create a framebuffer to fail
with -EINVAL?
> }
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-drm-hyperv-cover@berkoc.com?part=1
^ permalink raw reply
* [PATCH 2/2] drm/hyperv: validate VMBus packet size in receive callback
From: Berkant Koc @ 2026-05-17 12:55 UTC (permalink / raw)
To: Saurabh Sengar, Dexuan Cui, Long Li
Cc: linux-hyperv, dri-devel, linux-kernel, Wei Liu, Michael Kelley,
Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, Deepak Rawat
In-Reply-To: <20260517-drm-hyperv-cover@berkoc.com>
hyperv_receive() reads bytes_recvd from vmbus_recvpacket() but does not
forward that value to hyperv_receive_sub(). The sub-handler reads
msg->vid_hdr.type and msg->feature_chg.is_dirt_needed without knowing
how many bytes the host actually wrote, so a short packet leaves the
parser reading bytes that the host did not write in this packet. The
unconditional memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE) on the
wait-completion path also copies the full 16 KiB recv_buf regardless
of bytes_recvd, including any residue from the prior message.
Pass bytes_recvd into hyperv_receive_sub() and reject any packet shorter
than the pipe + synthvid header. The dirt-feature branch additionally
requires the feature_change payload size before reading is_dirt_needed.
The init_buf copy now uses bytes_recvd as the length argument, which
keeps it bounded by VMBUS_MAX_PACKET_SIZE through the new upper check.
Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
Cc: stable@vger.kernel.org # 5.14+
Signed-off-by: Berkant Koc <me@berkoc.com>
---
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
index 003bb118d64c..0d32d9944c43 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
@@ -420,26 +420,35 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
return 0;
}
-static void hyperv_receive_sub(struct hv_device *hdev)
+static void hyperv_receive_sub(struct hv_device *hdev, u32 bytes_recvd)
{
struct hyperv_drm_device *hv = hv_get_drvdata(hdev);
struct synthvid_msg *msg;
+ size_t hdr_size;
if (!hv)
return;
+ hdr_size = sizeof(struct pipe_msg_hdr) +
+ sizeof(struct synthvid_msg_hdr);
+ if (bytes_recvd < hdr_size || bytes_recvd > VMBUS_MAX_PACKET_SIZE)
+ return;
+
msg = (struct synthvid_msg *)hv->recv_buf;
/* Complete the wait event */
if (msg->vid_hdr.type == SYNTHVID_VERSION_RESPONSE ||
msg->vid_hdr.type == SYNTHVID_RESOLUTION_RESPONSE ||
msg->vid_hdr.type == SYNTHVID_VRAM_LOCATION_ACK) {
- memcpy(hv->init_buf, msg, VMBUS_MAX_PACKET_SIZE);
+ memcpy(hv->init_buf, msg, bytes_recvd);
complete(&hv->wait);
return;
}
if (msg->vid_hdr.type == SYNTHVID_FEATURE_CHANGE) {
+ if (bytes_recvd < hdr_size +
+ sizeof(struct synthvid_feature_change))
+ return;
hv->dirt_needed = msg->feature_chg.is_dirt_needed;
if (hv->dirt_needed)
hyperv_hide_hw_ptr(hv->hdev);
@@ -466,7 +475,7 @@ static void hyperv_receive(void *ctx)
&bytes_recvd, &req_id);
if (bytes_recvd > 0 &&
recv_buf->pipe_hdr.type == PIPE_MSG_DATA)
- hyperv_receive_sub(hdev);
+ hyperv_receive_sub(hdev, bytes_recvd);
} while (bytes_recvd > 0 && ret == 0);
}
--
2.47.3
^ permalink raw reply related
* [PATCH 1/2] drm/hyperv: validate resolution_count from host VMBus message
From: Berkant Koc @ 2026-05-17 12:55 UTC (permalink / raw)
To: Saurabh Sengar, Dexuan Cui, Long Li
Cc: linux-hyperv, dri-devel, linux-kernel, Wei Liu, Michael Kelley,
Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, Deepak Rawat
In-Reply-To: <20260517-drm-hyperv-cover@berkoc.com>
The synthetic video device receives a SYNTHVID_RESOLUTION_RESPONSE
containing a u8 resolution_count and a u8 default_resolution_index
from the host. The existing check rejects resolution_count == 0 and
rejects an index that is greater or equal to resolution_count, but
does not bound resolution_count itself against the fixed
supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT] array. A host
that returns resolution_count > 64 together with an in-range
default_resolution_index causes the subsequent loop to read past
the array.
Reject any resolution_count that exceeds SYNTHVID_MAX_RESOLUTION_COUNT,
folded into the existing zero-check for one log entry per failure.
This matches the input-validation pattern used by other VMBus parsers
under drivers/hv/ and trims one host-controlled value from the trusted
path.
Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video device")
Cc: stable@vger.kernel.org # 5.14+
Signed-off-by: Berkant Koc <me@berkoc.com>
---
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
index 051ecc526832..003bb118d64c 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_proto.c
@@ -391,8 +391,11 @@ static int hyperv_get_supported_resolution(struct hv_device *hdev)
return -ETIMEDOUT;
}
- if (msg->resolution_resp.resolution_count == 0) {
- drm_err(dev, "No supported resolutions\n");
+ if (msg->resolution_resp.resolution_count == 0 ||
+ msg->resolution_resp.resolution_count >
+ SYNTHVID_MAX_RESOLUTION_COUNT) {
+ drm_err(dev, "Invalid resolution count: %d\n",
+ msg->resolution_resp.resolution_count);
return -ENODEV;
}
--
2.47.3
^ permalink raw reply related
* [PATCH 0/2] drm/hyperv: harden VMBus message parser input validation
From: Berkant Koc @ 2026-05-17 12:55 UTC (permalink / raw)
To: Saurabh Sengar, Dexuan Cui, Long Li
Cc: linux-hyperv, dri-devel, linux-kernel, Wei Liu, Michael Kelley,
Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, Deepak Rawat
The hyperv synthetic video driver parses VMBus messages from the host
without bounding two host-controlled values that feed into fixed-size
buffers. Both items are input validation, not security bugs: the
Hyper-V host sits inside the trusted compute base under the default
Hyper-V threat-model. The patches still trim the inputs the driver
accepts at face value, matching the trajectory drivers/hv/ has
followed for Confidential-VMBus work where the host is no longer
fully trusted.
Patch 1 bounds resolution_count against
supported_resolution[SYNTHVID_MAX_RESOLUTION_COUNT]; the existing
default_resolution_index check is bypassable when both values
exceed 64.
Patch 2 forwards bytes_recvd from vmbus_recvpacket() into the
sub-handler so that vid_hdr.type and feature_chg.is_dirt_needed
are only read once the host actually delivered enough bytes, and
so that the init_buf memcpy uses the received length.
Sending as a plain patch series, not a security disclosure.
Compile-tested against drm-fixes (6916d5703ddf), static-only.
Berkant Koc (2):
drm/hyperv: validate resolution_count from host VMBus message
drm/hyperv: validate VMBus packet size in receive callback
drivers/gpu/drm/hyperv/hyperv_drm_proto.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
base-commit: 6916d5703ddf9a38f1f6c2cc793381a24ee914c6
--
2.47.3
^ permalink raw reply
* Re: [PATCH v3 40/41] x86/tsc: Add standalone helper for getting CPU frequency from CPUID
From: Paolo Bonzini @ 2026-05-16 7:42 UTC (permalink / raw)
To: Sean Christopherson, Kiryl Shutsemau, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Long Li, Ajay Kaher,
Alexey Makhalov, Jan Kiszka, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, Juergen Gross, Daniel Lezcano, Thomas Gleixner,
John Stultz
Cc: Rick Edgecombe, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, x86, linux-coco, kvm, linux-hyperv, virtualization,
linux-kernel, xen-devel, Michael Kelley, Tom Lendacky,
Nikunj A Dadhania, Thomas Gleixner, David Woodhouse
In-Reply-To: <20260515191942.1892718-41-seanjc@google.com>
On 5/15/26 21:19, Sean Christopherson wrote:
> Extract the guts of cpu_khz_from_cpuid() to a standalone helper that
> doesn't restrict the usage to Intel CPUs. This will allow sharing the
> core logic with kvmclock, as (a) CPUID.0x16 may be enumerated alongside
> kvmclock, and (b) KVM generally doesn't restrict CPUID based on vendor.
Even for native there's no real reason to restrict to Intel, I think.
native_calibrate_tsc() only limits itself because historically (prior to
commit 604dc9170f24, "x86/tsc: Use CPUID.0x16 to calculate missing
crystal frequency", 2019-05-09) it used a hardcoded table of crystal
frequencies.
Of course paranoia applies, but for virtualization, if the leaf exists
there is no reason not to trust it.
Thanks,
Paolo
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/include/asm/tsc.h | 1 +
> arch/x86/kernel/tsc.c | 37 +++++++++++++++++++++++--------------
> 2 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
> index f458be688512..c145f5707b52 100644
> --- a/arch/x86/include/asm/tsc.h
> +++ b/arch/x86/include/asm/tsc.h
> @@ -91,6 +91,7 @@ struct cpuid_tsc_info {
> };
> extern int cpuid_get_tsc_info(struct cpuid_tsc_info *info);
> extern int cpuid_get_tsc_freq(struct cpuid_tsc_info *info);
> +extern int cpuid_get_cpu_freq(unsigned int *cpu_khz);
>
> extern void tsc_early_init(void);
> extern void tsc_init(void);
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 1b569954ae5e..745fa2052c74 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -719,6 +719,24 @@ int cpuid_get_tsc_freq(struct cpuid_tsc_info *info)
> return 0;
> }
>
> +int cpuid_get_cpu_freq(unsigned int *cpu_khz)
> +{
> + unsigned int eax_base_mhz, ebx, ecx, edx;
> +
> + *cpu_khz = 0;
> +
> + if (boot_cpu_data.cpuid_level < CPUID_LEAF_FREQ)
> + return -ENOENT;
> +
> + cpuid(CPUID_LEAF_FREQ, &eax_base_mhz, &ebx, &ecx, &edx);
> +
> + if (!eax_base_mhz)
> + return -ENOENT;
> +
> + *cpu_khz = eax_base_mhz * 1000;
> + return 0;
> +}
> +
> /**
> * native_calibrate_tsc - determine TSC frequency
> * Determine TSC frequency via CPUID, else return 0.
> @@ -754,13 +772,8 @@ unsigned long native_calibrate_tsc(void)
> * clock, but we can easily calculate it to a high degree of accuracy
> * by considering the crystal ratio and the CPU speed.
> */
> - if (!info.crystal_khz && boot_cpu_data.cpuid_level >= CPUID_LEAF_FREQ) {
> - unsigned int eax_base_mhz, ebx, ecx, edx;
> -
> - cpuid(CPUID_LEAF_FREQ, &eax_base_mhz, &ebx, &ecx, &edx);
> - info.crystal_khz = eax_base_mhz * 1000 *
> - info.denominator / info.numerator;
> - }
> + if (!info.crystal_khz && !cpuid_get_cpu_freq(&cpu_khz))
> + info.crystal_khz = cpu_khz * info.denominator / info.numerator;
>
> if (!info.crystal_khz)
> return 0;
> @@ -787,19 +800,15 @@ unsigned long native_calibrate_tsc(void)
>
> static unsigned long cpu_khz_from_cpuid(void)
> {
> - unsigned int eax_base_mhz, ebx_max_mhz, ecx_bus_mhz, edx;
> + unsigned int cpu_khz;
>
> if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> return 0;
>
> - if (boot_cpu_data.cpuid_level < CPUID_LEAF_FREQ)
> + if (cpuid_get_cpu_freq(&cpu_khz))
> return 0;
>
> - eax_base_mhz = ebx_max_mhz = ecx_bus_mhz = edx = 0;
> -
> - cpuid(CPUID_LEAF_FREQ, &eax_base_mhz, &ebx_max_mhz, &ecx_bus_mhz, &edx);
> -
> - return eax_base_mhz * 1000;
> + return cpu_khz;
> }
>
> /*
^ permalink raw reply
* Re: [PATCH net-next v10 5/6] net: mana: Allocate interrupt context for each EQ when creating vPort
From: sashiko-bot @ 2026-05-16 4:05 UTC (permalink / raw)
To: Long Li; +Cc: linux-hyperv
In-Reply-To: <20260515040508.491748-6-longli@microsoft.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The RDMA driver (mana_ib) fails to initialize because it does not allocate GIC references for its EQs.
--
commit 9f615e81497dbea890c8df0a9aa9efd643e9a6a7
Author: Long Li <longli@microsoft.com>
net: mana: Allocate interrupt context for each EQ when creating vPort
This commit updates the driver to use GIC functions to create a dedicated
interrupt context or acquire a shared interrupt context for each EQ when
setting up a vPort. The caller is now responsible for acquiring and
releasing the GIC reference across the EQ create/destroy lifecycle.
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -1684,12 +1690,22 @@ int mana_create_eq(struct mana_port_context *apc)
> debugfs_create_dir("EQs", apc->mana_port_debugfs);
>
> for (i = 0; i < apc->num_queues; i++) {
> - spec.eq.msix_index = (i + 1) % gc->num_msix_usable;
> + msi = (i + 1) % gc->num_msix_usable;
> +
> + gic = mana_gd_get_gic(gc, !gc->msi_sharing, &msi);
> + if (!gic) {
> + err = -ENOMEM;
> + goto out;
> + }
> + spec.eq.msix_index = msi;
Will this change to the initialization logic break the RDMA driver (mana_ib)?
Since the caller is now required to manually acquire a GIC reference via
mana_gd_get_gic() before creating EQs, and mana_ib hasn't been updated to
do this, won't it trigger a WARN_ON(!gic) inside mana_gd_register_irq() and
fail device initialization?
I noticed this was fixed later in the patch series by commit 'RDMA/mana_ib:
Allocate interrupt contexts on EQs', but could this break bisectability here?
> +
> err = mana_gd_create_mana_eq(gd, &spec, &apc->eqs[i].eq);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515040508.491748-1-longli@microsoft.com?part=5
^ permalink raw reply
* Re: [PATCH net-next v10 2/6] net: mana: Query device capabilities and configure MSI-X sharing for EQs
From: sashiko-bot @ 2026-05-16 4:05 UTC (permalink / raw)
To: Long Li; +Cc: linux-hyperv
In-Reply-To: <20260515040508.491748-3-longli@microsoft.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Per-vPort queue counts are not clamped across device resume, leading to MSI-X vector exhaustion and resume failure if available interrupts shrink.
--
commit 5d1fd25db612c4024c6a414d322389f154cfe998
Author: Long Li <longli@microsoft.com>
Date: Fri May 15 04:05:04 2026 +0000
net: mana: Query device capabilities and configure MSI-X sharing for EQs
This commit adjusts the maximum number of queues to allow dedicated MSI-X
vectors for each vPort, calculating per-vPort limits based on hardware
capabilities. It enables MSI-X sharing dynamically when there are not
enough vectors for dedicated allocation and ensures a minimum of one port
during probe.
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 18f8f653da3d4..d23b856f48f6f 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... ]
> @@ -3426,9 +3431,9 @@ static int mana_probe_port(struct mana_context *ac, int port_idx,
> apc = netdev_priv(ndev);
> apc->ac = ac;
> apc->ndev = ndev;
> - apc->max_queues = gc->max_num_queues;
> + apc->max_queues = gc->max_num_queues_vport;
> /* Use MANA_DEF_NUM_QUEUES as default, still honoring the HW limit */
> - apc->num_queues = min(gc->max_num_queues, MANA_DEF_NUM_QUEUES);
> + apc->num_queues = min(gc->max_num_queues_vport, MANA_DEF_NUM_QUEUES);
> apc->tx_queue_size = DEF_TX_BUFFERS_PER_QUEUE;
Does this code handle clamping apc->num_queues if the available MSI-X
vectors decrease during a suspend/resume cycle?
During a device resume, mana_gd_query_max_resources() might recalculate
a smaller gc->max_num_queues_vport if fewer MSI-X vectors are provided
by the system.
Since apc->num_queues is set here during the initial probe, it appears
to retain its previously configured size on resume. When mana_attach()
is called in the resume path for existing ports, could it try to allocate
the old, larger number of vectors from the newly undersized msi_bitmap,
exhausting the bitmap and failing the resume with -ENOMEM?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515040508.491748-1-longli@microsoft.com?part=2
^ permalink raw reply
* Re: [PATCH net-next v10 1/6] net: mana: Create separate EQs for each vPort
From: sashiko-bot @ 2026-05-16 4:05 UTC (permalink / raw)
To: Long Li; +Cc: linux-hyperv
In-Reply-To: <20260515040508.491748-2-longli@microsoft.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing cross-port validation in `mana_ib_create_qp_rss()` allows userspace to trigger NULL pointer dereferences and corrupt other ports' queues.
- [High] Use of boolean flag `channel_changing` as an ad-hoc lock in `mana_set_channels()` violates networking subsystem guidelines.
--
commit ed2a523d0427d1b949add95925cad26c8b468ff8
Author: Long Li <longli@microsoft.com>
Date: Fri May 15 04:05:03 2026 +0000
net: mana: Create separate EQs for each vPort
This commit separates EQs for each vPort, removing the shared EQ mechanism
from the MANA driver to prepare for dedicated MSI-X vector assignment.
RSS QPs are updated to take a vport reference, ensuring EQs outlive QP
consumers.
> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> index 0fbcf449c134b..108ec4c5ce51c 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
[ ... ]
> @@ -155,6 +156,18 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, struct ib_pd *pd,
>
> qp->port = port;
>
> + /* Take a reference on the vport to ensure EQs outlive this QP.
> + * The vport must already be configured by a raw QP.
> + */
> + mutex_lock(&mana_pd->vport_mutex);
> + if (!mana_pd->vport_use_count) {
> + mutex_unlock(&mana_pd->vport_mutex);
> + ret = -EINVAL;
> + goto fail;
> + }
> + mana_pd->vport_use_count++;
> + mutex_unlock(&mana_pd->vport_mutex);
> +
Does this code need to verify that the requested port matches
mana_pd->vport_port?
While mana_ib_cfg_vport() correctly rejects cross-port PD sharing for
raw QPs, an RSS QP creation could specify a different port (e.g., Port B)
using the same PD bound to Port A.
If Port B is unconfigured, could fetching its mpc result in a NULL
pointer dereference when accessing mpc->eqs below?
If Port B is active, could this lead to erroneously destroying Port B's EQs
or underflowing its vport_use_count when the PD drops its references during
teardown?
> for (i = 0; i < ind_tbl_size; i++) {
> struct mana_obj_spec wq_spec = {};
[ ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 04350973e19e3..4633acc976f0a 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
[ ... ]
> @@ -462,10 +467,22 @@ static int mana_set_channels(struct net_device *ndev,
> unsigned int old_count = apc->num_queues;
> int err;
>
> + /* Set channel_changing to block RDMA from grabbing the vport
> + * during the detach/attach window. mana_cfg_vport() checks
> + * this flag under vport_mutex and returns -EBUSY if set.
> + */
> + mutex_lock(&apc->vport_mutex);
> + if (!apc->port_is_up && apc->vport_use_count) {
> + mutex_unlock(&apc->vport_mutex);
> + return -EBUSY;
> + }
> + apc->channel_changing = true;
> + mutex_unlock(&apc->vport_mutex);
> +
Is channel_changing acting as an ad-hoc lock here?
The networking subsystem guidelines discourage using boolean flags set on
entry and cleared on exit to guard code sections against concurrent access.
Since mana_detach() and mana_attach() are executed while this flag is held
to prevent mana_cfg_vport() from proceeding, could this be replaced with a
standard locking mechanism to ensure lockdep visibility?
> err = mana_pre_alloc_rxbufs(apc, ndev->mtu, new_count);
> if (err) {
> netdev_err(ndev, "Insufficient memory for new allocations");
> - return err;
> + goto clear_flag;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515040508.491748-1-longli@microsoft.com?part=1
^ permalink raw reply
* Re: [PATCH v1 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest
From: Mukesh R @ 2026-05-16 0:11 UTC (permalink / raw)
To: Yu Zhang, Michael Kelley
Cc: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
iommu@lists.linux.dev, linux-pci@vger.kernel.org,
linux-arch@vger.kernel.org, wei.liu@kernel.org, kys@microsoft.com,
haiyangz@microsoft.com, decui@microsoft.com, longli@microsoft.com,
joro@8bytes.org, will@kernel.org, robin.murphy@arm.com,
bhelgaas@google.com, kwilczynski@kernel.org,
lpieralisi@kernel.org, mani@kernel.org, robh@kernel.org,
arnd@arndb.de, jgg@ziepe.ca, jacob.pan@linux.microsoft.com,
tgopinath@linux.microsoft.com,
easwar.hariharan@linux.microsoft.com
In-Reply-To: <fw2pruvjgo7yigtcxssf3xv27soibsj6hmw2ls5wj4rylfhdha@e63f32cwu2x5>
On 5/15/26 09:53, Yu Zhang wrote:
> On Fri, May 15, 2026 at 02:51:38PM +0000, Michael Kelley wrote:
>> From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Friday, May 15, 2026 7:00 AM
>>>
>>> On Thu, May 14, 2026 at 06:13:24PM +0000, Michael Kelley wrote:
>>>> From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Monday, May 11, 2026 9:24 AM
>>>>>
>>>>> Add a para-virtualized IOMMU driver for Linux guests running on Hyper-V.
>>>>> This driver implements stage-1 IO translation within the guest OS.
>>>>> It integrates with the Linux IOMMU core, utilizing Hyper-V hypercalls
>>>>> for:
>>>>> - Capability discovery
>>>>> - Domain allocation, configuration, and deallocation
>>>>> - Device attachment and detachment
>>>>> - IOTLB invalidation
>>>>>
>>>>> The driver constructs x86-compatible stage-1 IO page tables in the
>>>>> guest memory using consolidated IO page table helpers. This allows
>>>>> the guest to manage stage-1 translations independently of vendor-
>>>>> specific drivers (like Intel VT-d or AMD IOMMU).
>>>>>
>>>>> Hyper-V consumes this stage-1 IO page table when a device domain is
>>>>> created and configured, and nests it with the host's stage-2 IO page
>>>>> tables, therefore eliminating the VM exits for guest IOMMU mapping
>>>>> operations. For unmapping operations, VM exits to perform the IOTLB
>>>>> flush are still unavoidable.
>>>>>
>>>>> Hyper-V identifies each PCI pass-thru device by a logical device ID
>>>>> in its hypercall interface. The vPCI driver (pci-hyperv) registers the
>>>>> per-bus portion of this ID with the pvIOMMU driver during bus probe.
>>>>> The pvIOMMU driver stores this mapping and combines it with the function
>>>>> number of the endpoint PCI device to form the complete ID for hypercalls.
>>>>
>>>> As you are probably aware, Mukesh's patch series to support PCI
>>>> pass-thru devices also needs to get the logical device ID. Maybe the
>>>> registration mechanism needs to move somewhere that can be shared
>>>> with his code.
>>>>
>>>
>>> Thank you so much for the review, Michael!
>>>
>>> Yes, I looked at Mukesh's series and noticed his hv_pci_vmbus_device_id()
>>> in pci-hyperv.c has the same dev_instance byte manipulation. We do need
>>> a common registration mechanism.
>>>
>>> Any suggestion on where to put it? drivers/hv/hv_common.c seems like a
>>> natural place, but the register/lookup functions are currently only
>>> meaningful when CONFIG_HYPERV_PVIOMMU is set. If Mukesh's pass-thru
>>> code also needs them, we might need a new shared Kconfig option that
>>> both can select. Open to better ideas.
>>
>> Unfortunately, I have not looked at Mukesh's series in detail yet, so
>> I don't have enough knowledge of the full situation to offer a good
>> recommendation.
>>
>
> Sorry I forgot to Cc Mukesh in the previous reply. :(
> @Mukesh, any thoughts on sharing the logical device ID registration mechanism?
Yeah, I went round and round trying to find the best place. I almost
created virt/hyperv/hv_utils.c file. Maybe that is the best place?
Thanks,
-Mukesh
>>>
>>> [...]
>>>
>>>>> +static void hv_flush_device_domain(struct hv_iommu_domain *hv_domain)
>>>>> +{
>>>>> + u64 status;
>>>>> + unsigned long flags;
>>>>> + struct hv_input_flush_device_domain *input;
>>>>> +
>>>>> + local_irq_save(flags);
>>>>> +
>>>>> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>>>>> + memset(input, 0, sizeof(*input));
>>>>> + input->device_domain = hv_domain->device_domain;
>>>>
>>>> The previous version of this patch had code to set several other fields in
>>>> the input. I wanted to confirm that not setting them in this version is
>>>> intentional. Were they not needed?
>>>>
>>>
>>> Oh. The RFC v1 set partition_id, owner_vtl, domain_id.type, and domain_id.id
>>> individually. In this version, I just simplified it to a struct assignment.
>>> No functional change.
>>
>> Of course! I should have looked more closely at the details before making
>> this comment. :-(
>>
>> [...]
>>
>>>>
>>>> Previous versions of this function did hv_iommu_detach_dev(). With that call
>>>> removed from here, hv_iommu_detach_dev() is only called when attaching a
>>>> domain to a device that already has a domain attached. Is it the case that
>>>> Hyper-V doesn't require the detach as a cleanup step?
>>>>
>>>
>>> The IOMMU core attaches the device to release_domain (our blocking domain)
>>> before calling release_device(), so I believe the explicit detach in the RFC
>>> was redundant. I simply didn't realize that at the time.
>>>
>>
>> Got it. But after the IOMMU core attaches the device to the blocking
>> domain, there's the possibility that the vPCI device is rescinded by
>> Hyper-V and it goes away entirely. Or the device might be subjected
>> to an "unbind/bind" cycle in Linux. Does the detach need to be done
>> on the blocking domain in such cases? In this version of the patches, the
>> Hyper-V "attach" and "detach" hypercalls still end up unbalanced. That
>> seems a bit untidy at best, and I wonder if there are scenarios where
>> Hyper-V will complain about the lack of balance.
>>
>
> Thank you, Michael. May I ask what "the vPCI device is rescinded by
> Hyper-V and it goes away entirely" mean?
>
> I realized it's a bit untidy. But I want to understand this issue more
> clearly first. :)
>
> B.R.
> Yu
^ permalink raw reply
* RE: [PATCH v1 4/4] iommu/hyperv: Add page-selective IOTLB flush support
From: Michael Kelley @ 2026-05-15 23:33 UTC (permalink / raw)
To: Michael Kelley, Yu Zhang
Cc: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
iommu@lists.linux.dev, linux-pci@vger.kernel.org,
linux-arch@vger.kernel.org, wei.liu@kernel.org, kys@microsoft.com,
haiyangz@microsoft.com, decui@microsoft.com, longli@microsoft.com,
joro@8bytes.org, will@kernel.org, robin.murphy@arm.com,
bhelgaas@google.com, kwilczynski@kernel.org,
lpieralisi@kernel.org, mani@kernel.org, robh@kernel.org,
arnd@arndb.de, jgg@ziepe.ca, jacob.pan@linux.microsoft.com,
tgopinath@linux.microsoft.com,
easwar.hariharan@linux.microsoft.com
In-Reply-To: <SN6PR02MB4157F7758A127AA1E8096B6CD4042@SN6PR02MB4157.namprd02.prod.outlook.com>
From: Michael Kelley <mhklinux@outlook.com> Sent: Friday, May 15, 2026 11:00 AM
>
> From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Friday, May 15, 2026 9:24 AM
> >
> > On Thu, May 14, 2026 at 06:14:22PM +0000, Michael Kelley wrote:
> > > From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Monday, May 11, 2026 9:24 AM
> > > >
>
> [....]
>
> > > > + unsigned long nr_pages = end_pfn - start_pfn;
> > > > + u16 count = 0;
> > > > +
> > > > + while (nr_pages > 0) {
> > > > + unsigned long flush_pages;
> > > > + int order;
> > > > + unsigned long pfn_align;
> > > > + unsigned long size_align;
> > > > +
> > > > + if (count >= HV_IOMMU_MAX_FLUSH_VA_COUNT) {
> > > > + count = HV_IOMMU_FLUSH_VA_OVERFLOW;
> > > > + break;
> > > > + }
> > > > +
> > > > + if (start_pfn)
> > > > + pfn_align = __ffs(start_pfn);
> > >
> > > I don't understand why __ffs() is correct here. I would expect
> > > __fls() so it is consistent with the calculation of size_align. But I
> > > can only surmise how the hypercall works since there's no
> > > documentation, so maybe my understanding of the hypercall is
> > > wrong. If __ffs really is correct, a comment explaining why
> > > would help. :-)
> > >
> >
> > The use of __ffs() is intentional. Each flush entry invalidates a
> > naturally aligned 2^N page block, and the hypervisor requires the
> > page_number to be aligned to 2^page_mask_shift.
> >
> > Here __ffs() and __fls() serve different purposes:
> > - __ffs(start_pfn) is about the alignment constraint, e.g., how
> > large a block can this address support?
> > - __fls(nr_pages) is about the size constraint, e.g., how large
> > a block can the remaining range hold?
> >
> > Taking min() of both ensures each entry is both properly aligned
> > and within bounds.
> >
> > Thanks for raising this - it definitely deserves a comment. I had to
> > stare at it for a while myself to remember why. :)
>
> Hmmm. Something about this still nags at me. I'll run some
> experiments to either convince myself that you are right, or to
> come up with a counterexample.
I have resolved what was nagging at me. My understanding of how
_ffs() and __fls() work was wrong. :-( Your code is correct.
>
> A related thought occurred to me. If each flush entry that is passed
> to Hyper-V describes a naturally aligned 2^N page block, I don't
> think the HV_IOMMU_MAX_FLUSH_VA_COUNT can ever
> be reached. The number of entries is limited by the number of
> bits in a PFN and the pages count, both of which are 64. And with
> 52 bit physical addressing and 4KiB pages, the actual size of
> a PFN and pages count is even smaller than 64.
> HV_IOMMU_MAX_FLUSH_VA_COUNT is the number of 8 byte
> union hv_iommu_flush_va entries that fit in a 4KiB page, so
> it's ~500.
>
> My statement applies to a single flush range. If multiple flush
> ranges were strung together in a single hypercall, a larger count
> could be reached, but hv_flush_device_domain_list() only does
> a single range. So I don't think the overflow case in
> hv_flush_device_domain_list() can ever happen. But let me
> do my experiments, and I will also look at this aspect to confirm
> if it's right.
My experiments also convince me that the overflow case can't
happen as long as the hypercall is being populated with entries
derived from a single range.
Michael
>
> >
> > > > + else
> > > > + pfn_align = BITS_PER_LONG - 1;
> > > > +
> > > > + size_align = __fls(nr_pages);
> > > > + order = min(pfn_align, size_align);
> > > > + iova_list[count].page_mask_shift = order;
> > > > + iova_list[count].page_number = start_pfn;
> > > > +
> > > > + flush_pages = 1UL << order;
> > > > + start_pfn += flush_pages;
> > > > + nr_pages -= flush_pages;
> > > > + count++;
> > > > + }
> > > > +
> > > > + return count;
> > > > +}
> > > > +
> > > > +static void hv_flush_device_domain_list(struct hv_iommu_domain *hv_domain,
> > > > + struct iommu_iotlb_gather *iotlb_gather)
> > > > +{
> > > > + u64 status;
> > > > + u16 count;
> > > > + unsigned long flags;
> > > > + struct hv_input_flush_device_domain_list *input;
> > > > +
> > > > + local_irq_save(flags);
> > > > +
> > > > + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> > > > + memset(input, 0, sizeof(*input));
> > > > +
> > > > + input->device_domain = hv_domain->device_domain;
> > > > + input->flags |= HV_FLUSH_DEVICE_DOMAIN_LIST_IOMMU_FORMAT;
> > >
> > > I would suggest moving the memset() and setting the input fields down
> > > under the "else" below so that they are parallel with the flush all case.
> > >
> >
> > I agree the structure should be more symmetric. Yet I guess the memset and
> > hv_iommu_fill_iova_list() need to stay before the branch since the fill
> > writes directly into input->iova_list[]. :)
>
> Agreed.
>
> >
> > > > + count = hv_iommu_fill_iova_list(input->iova_list,
> > > > + iotlb_gather->start,
> > > > + iotlb_gather->end);
> > > > + if (count == HV_IOMMU_FLUSH_VA_OVERFLOW) {
> > > > + /*
> > > > + * Range exceeds hypercall page capacity. Fall back to a full
> > > > + * domain flush.
> > > > + */
> > > > + struct hv_input_flush_device_domain *flush_all = (void *)input;
> > > > +
> > > > + memset(flush_all, 0, sizeof(*flush_all));
> > > > + flush_all->device_domain = hv_domain->device_domain;
> > > > + status = hv_do_hypercall(HVCALL_FLUSH_DEVICE_DOMAIN,
> > > > + flush_all, NULL);
> > > > + } else {
> > > > + status = hv_do_rep_hypercall(
> > > > + HVCALL_FLUSH_DEVICE_DOMAIN_LIST,
> > > > + count, 0, input, NULL);
> > > > + }
> > > > +
> > > > + local_irq_restore(flags);
> > > > +
> > > > + if (!hv_result_success(status))
> > > > + pr_err("HVCALL_FLUSH_DEVICE_DOMAIN_LIST failed, status %lld\n", status);
> > >
> > > As Sashiko pointed out, a failure here can lead to all kinds of trouble because
> > > of leaving unflushed entries. Maybe a WARN() is more appropriate? Also, maybe
> > > a failure in the list flush should try a flush all as a fallback, with the WARN()
> > > only if the flush all fails.
> > >
> >
> > Good idea. How about we restructure this routine to sth. like this:
> >
> >
> > memset(input, 0, sizeof(*input));
> > count = hv_iommu_fill_iova_list(...);
> >
> > if (count != HV_IOMMU_FLUSH_VA_OVERFLOW) {
> > input->device_domain = ...;
> > ...
> > status = hv_do_rep_hypercall(FLUSH_DEVICE_DOMAIN_LIST, ...);
> > if (hv_result_success(status))
> > goto out;
> > }
> >
> > /* overflow or list flush failed: fallback to full domain flush */
> > flush_all = (void *)input;
> > memset(flush_all, 0, sizeof(*flush_all));
> > flush_all->device_domain = ...;
> > status = hv_do_hypercall(FLUSH_DEVICE_DOMAIN, ...);
> > WARN(!hv_result_success(status), "IOTLB flush failed, status %lld\n", status);
> >
> > out:
> > local_irq_restore(flags);
> >
>
> Yes, I think this works. But per my earlier comment, if I'm right that
> the overflow case never occurs, it could be simplified further to just
> do the list flush with the full flush as the error fallback. Then WARN
> if the full flush fails.
>
> Michael
^ permalink raw reply
* Re: [PATCH v3 40/41] x86/tsc: Add standalone helper for getting CPU frequency from CPUID
From: Sean Christopherson @ 2026-05-15 23:04 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515195116.E0AFBC2BCB3@smtp.kernel.org>
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Unintended clobbering of the global `cpu_khz` variable in `native_calibrate_tsc()` due to a missing local variable declaration.
> --
>
> commit 36e45379d4c9f40ae24c5c323b4f0471c94c6bef
> Author: Sean Christopherson <seanjc@google.com>
>
> x86/tsc: Add standalone helper for getting CPU frequency from CPUID
>
> This commit extracts the logic for getting the CPU frequency via CPUID into
> a new standalone helper function. It also updates the TSC calibration code
> to use this new helper.
>
> > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> > --- a/arch/x86/kernel/tsc.c
> > +++ b/arch/x86/kernel/tsc.c
>
> [ ... ]
>
> > @@ -754,13 +772,8 @@ unsigned long native_calibrate_tsc(void)
> > * clock, but we can easily calculate it to a high degree of accuracy
> > * by considering the crystal ratio and the CPU speed.
> > */
> > - if (!info.crystal_khz && boot_cpu_data.cpuid_level >= CPUID_LEAF_FREQ) {
> > - unsigned int eax_base_mhz, ebx, ecx, edx;
> > -
> > - cpuid(CPUID_LEAF_FREQ, &eax_base_mhz, &ebx, &ecx, &edx);
> > - info.crystal_khz = eax_base_mhz * 1000 *
> > - info.denominator / info.numerator;
> > - }
> > + if (!info.crystal_khz && !cpuid_get_cpu_freq(&cpu_khz))
> > + info.crystal_khz = cpu_khz * info.denominator / info.numerator;
>
> Does this unintentionally clobber the global cpu_khz variable?
Yep. I don't think I even realized there was such a global variable. Probably
makes sense to use a different name for the local variables.
^ permalink raw reply
* Re: [PATCH v3 15/41] x86/xen/time: Nullify x86_platform's sched_clock save/restore hooks
From: Sean Christopherson @ 2026-05-15 22:43 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515194803.1D898C2BCB0@smtp.kernel.org>
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> > index 3d3165eef8214..21d366d01985e 100644
> > --- a/arch/x86/xen/time.c
> > +++ b/arch/x86/xen/time.c
> > @@ -568,6 +568,12 @@ static void __init xen_init_time_common(void)
> > xen_sched_clock_offset = xen_clocksource_read();
> > static_call_update(pv_steal_clock, xen_steal_clock);
> > paravirt_set_sched_clock(xen_sched_clock);
> > + /*
> > + * Xen has paravirtualized suspend/resume and so doesn't use the common
> > + * x86 sched_clock save/restore hooks.
> > + */
> > + x86_platform.save_sched_clock_state = NULL;
> > + x86_platform.restore_sched_clock_state = NULL;
>
> Does setting these hooks to NULL lead to an unconditional pointer dereference
> regression during suspend, hibernation, or kexec operations in Xen guests?
>
> Looking at save_processor_state() and __restore_processor_state() in
> arch/x86/power/cpu.c, the hooks are called directly without being checked
> for NULL first:
>
> arch/x86/power/cpu.c:save_processor_state() {
> __save_processor_state(&saved_context);
> x86_platform.save_sched_clock_state();
> }
>
> arch/x86/power/cpu.c:__restore_processor_state() {
> ...
> do_fpu_end();
> tsc_verify_tsc_adjust(true);
> x86_platform.restore_sched_clock_state();
> ...
> }
>
> Will operations that trigger a generic CPU state save or restore on a Xen
> guest execute a call on these NULL pointers, causing a kernel panic
> regression? Should these instead be assigned to an empty stub function to
> safely bypass them?
/facepalm
Yes. I suspect past me carried over the Xen changes before I understood why
it's safe on Xen (Xen uses a unique PV suspend/resume flow).
Playing nice with NULL x86_platform.{save,restore}_sched_clock_state pointers
is the obvious fix. The other option would be to wire up nop callbacks, but I
don't see any value in doing so. I really don't want to leave the callbacks
wired up to tsc_{save,restore}_sched_clock_state() (unless it turns out VMware
actually needs them).
Regardless, this definitely highlights that VMware guests need to be tested. :-/
^ permalink raw reply
* Re: [PATCH v1 4/4] iommu/hyperv: Add page-selective IOTLB flush support
From: Jason Gunthorpe @ 2026-05-15 22:35 UTC (permalink / raw)
To: Yu Zhang
Cc: linux-kernel, linux-hyperv, iommu, linux-pci, linux-arch, wei.liu,
kys, haiyangz, decui, longli, joro, will, robin.murphy, bhelgaas,
kwilczynski, lpieralisi, mani, robh, arnd, mhklinux, jacob.pan,
tgopinath, easwar.hariharan
In-Reply-To: <20260511162408.1180069-5-zhangyu1@linux.microsoft.com>
On Tue, May 12, 2026 at 12:24:08AM +0800, Yu Zhang wrote:
> +static inline u16 hv_iommu_fill_iova_list(union hv_iommu_flush_va *iova_list,
> + unsigned long start,
> + unsigned long end)
> +{
> + unsigned long start_pfn = start >> PAGE_SHIFT;
> + unsigned long end_pfn = PAGE_ALIGN(end) >> PAGE_SHIFT;
> + unsigned long nr_pages = end_pfn - start_pfn;
> + u16 count = 0;
> +
> + while (nr_pages > 0) {
> + unsigned long flush_pages;
> + int order;
> + unsigned long pfn_align;
> + unsigned long size_align;
> +
> + if (count >= HV_IOMMU_MAX_FLUSH_VA_COUNT) {
> + count = HV_IOMMU_FLUSH_VA_OVERFLOW;
> + break;
> + }
> +
> + if (start_pfn)
> + pfn_align = __ffs(start_pfn);
> + else
> + pfn_align = BITS_PER_LONG - 1;
> +
> + size_align = __fls(nr_pages);
> + order = min(pfn_align, size_align);
> + iova_list[count].page_mask_shift = order;
> + iova_list[count].page_number = start_pfn;
> +
> + flush_pages = 1UL << order;
> + start_pfn += flush_pages;
> + nr_pages -= flush_pages;
> + count++;
> + }
This seems like a really silly hypervisor interface. Why doesn't it
just accept a normal range? Splitting it into power of two aligned
ranges is very inefficient.
Jason
^ permalink raw reply
* Re: [PATCH v1 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest
From: Jason Gunthorpe @ 2026-05-15 22:31 UTC (permalink / raw)
To: Yu Zhang
Cc: linux-kernel, linux-hyperv, iommu, linux-pci, linux-arch, wei.liu,
kys, haiyangz, decui, longli, joro, will, robin.murphy, bhelgaas,
kwilczynski, lpieralisi, mani, robh, arnd, mhklinux, jacob.pan,
tgopinath, easwar.hariharan
In-Reply-To: <20260511162408.1180069-4-zhangyu1@linux.microsoft.com>
On Tue, May 12, 2026 at 12:24:07AM +0800, Yu Zhang wrote:
> +/*
> + * Identity and blocking domains are static singletons: identity is a 1:1
> + * passthrough with no page table, blocking rejects all DMA. Neither holds
> + * per-IOMMU state, so one instance suffices even with multiple vIOMMUs.
> + */
> +static struct hv_iommu_domain hv_identity_domain;
> +static struct hv_iommu_domain hv_blocking_domain;
> +static const struct iommu_domain_ops hv_iommu_identity_domain_ops;
> +static const struct iommu_domain_ops hv_iommu_blocking_domain_ops;
Please follow the format of other drivers and statically initialize
these here instead of in C code.
> +static struct iommu_ops hv_iommu_ops;
> +static LIST_HEAD(hv_iommu_pci_bus_list);
> +static DEFINE_SPINLOCK(hv_iommu_pci_bus_lock);
> +
> +#define hv_iommu_present(iommu_cap) (iommu_cap & HV_IOMMU_CAP_PRESENT)
> +#define hv_iommu_s1_domain_supported(iommu_cap) (iommu_cap & HV_IOMMU_CAP_S1)
> +#define hv_iommu_5lvl_supported(iommu_cap) (iommu_cap & HV_IOMMU_CAP_S1_5LVL)
> +#define hv_iommu_ats_supported(iommu_cap) (iommu_cap & HV_IOMMU_CAP_ATS)
prefer to see static inlines
> +static void hv_iommu_detach_dev(struct iommu_domain *domain, struct device *dev)
> +{
> + u64 status;
> + unsigned long flags;
> + struct hv_input_detach_device_domain *input;
> + struct pci_dev *pdev;
> + struct hv_iommu_domain *hv_domain = to_hv_iommu_domain(domain);
> + struct hv_iommu_endpoint *vdev = dev_iommu_priv_get(dev);
> +
> + /* See the attach function, only PCI devices for now */
> + if (!dev_is_pci(dev) || vdev->hv_domain != hv_domain)
> + return;
> +
> + pdev = to_pci_dev(dev);
> +
> + dev_dbg(dev, "detaching from domain %d\n", hv_domain->device_domain.domain_id.id);
> +
> + local_irq_save(flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + memset(input, 0, sizeof(*input));
> + input->partition_id = HV_PARTITION_ID_SELF;
> + if (hv_iommu_lookup_logical_dev_id(pdev, &input->device_id.as_uint64)) {
> + local_irq_restore(flags);
> + dev_warn(&pdev->dev, "no IOMMU registration for vPCI bus on detach\n");
> + return;
> + }
> + status = hv_do_hypercall(HVCALL_DETACH_DEVICE_DOMAIN, input, NULL);
FWIW the hypervisor cannot implement the linux attach semantics if it
has detach/attach. It must support simply 'attach' which atomically
changes the translation. With detach you have a confusing problem if
errors happen in the middle of the sequence the device is left in an
unclear state. You should at least document what state the hypervisor
is supposed to leaave the translation iin during these failures..
> +static void hv_iommu_release_device(struct device *dev)
> +{
> + struct hv_iommu_endpoint *vdev = dev_iommu_priv_get(dev);
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + if (pdev->ats_enabled)
> + pci_disable_ats(pdev);
> +
> + dev_iommu_priv_set(dev, NULL);
> + set_dma_ops(dev, NULL);
Does the driver really need to mess with set_dma_ops ? I'd rather not
see that in new iommu drivers if at all possible :|
> +static int __init hv_initialize_static_domains(void)
> +{
> + int ret;
> + struct hv_iommu_domain *hv_domain;
> +
> + /* Default stage-1 identity domain */
> + hv_domain = &hv_identity_domain;
> +
> + ret = hv_create_device_domain(hv_domain, HV_DEVICE_DOMAIN_TYPE_S1);
> + if (ret)
> + return ret;
> +
> + ret = hv_configure_device_domain(hv_domain, IOMMU_DOMAIN_IDENTITY);
> + if (ret)
> + goto delete_identity_domain;
> +
> + hv_domain->domain.type = IOMMU_DOMAIN_IDENTITY;
> + hv_domain->domain.ops = &hv_iommu_identity_domain_ops;
> + hv_domain->domain.owner = &hv_iommu_ops;
> + hv_domain->domain.geometry = hv_iommu_device->geometry;
> + hv_domain->domain.pgsize_bitmap = hv_iommu_device->pgsize_bitmap;
identity doesn't use geometry or pgsize_bitmap
> + /* Default stage-1 blocked domain */
> + hv_domain = &hv_blocking_domain;
> +
> + ret = hv_create_device_domain(hv_domain, HV_DEVICE_DOMAIN_TYPE_S1);
> + if (ret)
> + goto delete_identity_domain;
> +
> + ret = hv_configure_device_domain(hv_domain, IOMMU_DOMAIN_BLOCKED);
> + if (ret)
> + goto delete_blocked_domain;
> +
> + hv_domain->domain.type = IOMMU_DOMAIN_BLOCKED;
> + hv_domain->domain.ops = &hv_iommu_blocking_domain_ops;
> + hv_domain->domain.owner = &hv_iommu_ops;
> + hv_domain->domain.geometry = hv_iommu_device->geometry;
> + hv_domain->domain.pgsize_bitmap = hv_iommu_device->pgsize_bitmap;
Nor does blocked
> +#define INTERRUPT_RANGE_START (0xfee00000)
> +#define INTERRUPT_RANGE_END (0xfeefffff)
> +static void hv_iommu_get_resv_regions(struct device *dev,
> + struct list_head *head)
> +{
> + struct iommu_resv_region *region;
> +
> + region = iommu_alloc_resv_region(INTERRUPT_RANGE_START,
> + INTERRUPT_RANGE_END - INTERRUPT_RANGE_START + 1,
> + 0, IOMMU_RESV_MSI, GFP_KERNEL);
Surprised these constants are not discovered from the hv?
> +static void hv_iommu_flush_iotlb_all(struct iommu_domain *domain)
> +{
> + hv_flush_device_domain(to_hv_iommu_domain(domain));
> +}
> +
> +static void hv_iommu_iotlb_sync(struct iommu_domain *domain,
> + struct iommu_iotlb_gather *iotlb_gather)
> +{
> + hv_flush_device_domain(to_hv_iommu_domain(domain));
> +
> + iommu_put_pages_list(&iotlb_gather->freelist);
> +}
Full invalidation only huh?
> +static const struct iommu_domain_ops hv_iommu_identity_domain_ops = {
> + .attach_dev = hv_iommu_attach_dev,
> +};
> +
> +static const struct iommu_domain_ops hv_iommu_blocking_domain_ops = {
> + .attach_dev = hv_iommu_attach_dev,
> +};
Usually I would expect these to have their own attach
functions. blocking in particular must have an attach op that cannot
fail. It is used to recover the device back to a known translation in
case of cascading other errors.
> +static const struct iommu_domain_ops hv_iommu_paging_domain_ops = {
> + .attach_dev = hv_iommu_attach_dev,
> + IOMMU_PT_DOMAIN_OPS(x86_64),
> + .flush_iotlb_all = hv_iommu_flush_iotlb_all,
> + .iotlb_sync = hv_iommu_iotlb_sync,
> + .free = hv_iommu_paging_domain_free,
> +};
> +
> +static struct iommu_domain *hv_iommu_domain_alloc_paging(struct device *dev)
> +{
> + int ret;
> + struct hv_iommu_domain *hv_domain;
> + struct pt_iommu_x86_64_cfg cfg = {};
> +
> + hv_domain = kzalloc_obj(*hv_domain, GFP_KERNEL);
> + if (!hv_domain)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = hv_create_device_domain(hv_domain, HV_DEVICE_DOMAIN_TYPE_S1);
> + if (ret) {
> + kfree(hv_domain);
> + return ERR_PTR(ret);
> + }
> +
> + hv_domain->domain.geometry = hv_iommu_device->geometry;
geoemtry shouldn't be set here, it is overriden by
pt_iommu_x86_64_init() with the exact page table configuration
> +static void __init hv_init_iommu_device(struct hv_iommu_dev *hv_iommu,
> + struct hv_output_get_iommu_capabilities *hv_iommu_cap)
> +{
> + ida_init(&hv_iommu->domain_ids);
> +
> + hv_iommu->cap = hv_iommu_cap->iommu_cap;
> + hv_iommu->max_iova_width = hv_iommu_cap->max_iova_width;
> + if (!hv_iommu_5lvl_supported(hv_iommu->cap) &&
> + hv_iommu->max_iova_width > 48) {
> + pr_info("5-level paging not supported, limiting iova width to 48.\n");
> + hv_iommu->max_iova_width = 48;
> + }
> +
> + hv_iommu->geometry = (struct iommu_domain_geometry) {
> + .aperture_start = 0,
> + .aperture_end = (((u64)1) << hv_iommu->max_iova_width) - 1,
> + .force_aperture = true,
> + };
> +
> + hv_iommu->first_domain = HV_DEVICE_DOMAIN_ID_DEFAULT + 1;
> + hv_iommu->last_domain = HV_DEVICE_DOMAIN_ID_NULL - 1;
> + /* Only x86 page sizes (4K/2M/1G) are supported */
> + hv_iommu->pgsize_bitmap = hv_iommu_cap->pgsize_bitmap &
> + (SZ_4K | SZ_2M | SZ_1G);
> + if (hv_iommu->pgsize_bitmap != hv_iommu_cap->pgsize_bitmap)
> + pr_warn("unsupported page sizes masked: 0x%llx -> 0x%llx\n",
> + hv_iommu_cap->pgsize_bitmap, hv_iommu->pgsize_bitmap);
IKD if you need this logic, the way the page table code is used it
just sort of falls out naturally that other page sizes are ignored.
> +struct hv_iommu_domain {
> + union {
> + struct iommu_domain domain;
> + struct pt_iommu pt_iommu;
> + struct pt_iommu_x86_64 pt_iommu_x86_64;
> + };
You should retain the build assertions here
Jason
^ permalink raw reply
* Re: [PATCH v3 11/41] x86/kvm: Don't disable kvmclock on BSP in syscore_suspend()
From: Sean Christopherson @ 2026-05-15 22:29 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hyperv
In-Reply-To: <20260515203447.ED0E7C2BCB0@smtp.kernel.org>
On Fri, May 15, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> > index df95516a9d899..006e3a13500b4 100644
> > --- a/arch/x86/kernel/kvmclock.c
> > +++ b/arch/x86/kernel/kvmclock.c
> [ ... ]
> > @@ -178,8 +178,22 @@ static void kvm_register_clock(char *txt)
> > pr_debug("kvm-clock: cpu %d, msr %llx, %s", smp_processor_id(), pa, txt);
> > }
> >
> > +static void kvmclock_disable(void)
> > +{
> > + if (msr_kvm_system_time)
> > + native_write_msr(msr_kvm_system_time, 0);
> > +}
> > +
> > static void kvm_save_sched_clock_state(void)
> > {
> > + /*
> > + * Stop host writes to kvmclock immediately prior to suspend/hibernate.
> > + * If the system is hibernating, then kvmclock will likely reside at a
> > + * different physical address when the system awakens, and host writes
> > + * to the old address prior to reconfiguring kvmclock would clobber
> > + * random memory.
> > + */
> > + kvmclock_disable();
> > }
>
> Can this lead to memory corruption if another clocksource overwrites the
> x86_platform.save_sched_clock_state callback?
Yes, that's why the kernel only allows one "main" hypervisor.
^ permalink raw reply
* Re: [PATCH v1 1/4] iommu: Move Hyper-V IOMMU driver to its own subdirectory
From: Jason Gunthorpe @ 2026-05-15 22:19 UTC (permalink / raw)
To: Yu Zhang
Cc: linux-kernel, linux-hyperv, iommu, linux-pci, linux-arch, wei.liu,
kys, haiyangz, decui, longli, joro, will, robin.murphy, bhelgaas,
kwilczynski, lpieralisi, mani, robh, arnd, mhklinux, jacob.pan,
tgopinath, easwar.hariharan
In-Reply-To: <20260511162408.1180069-2-zhangyu1@linux.microsoft.com>
On Tue, May 12, 2026 at 12:24:05AM +0800, Yu Zhang wrote:
> From: Easwar Hariharan <eahariha@linux.microsoft.com>
>
> The Hyper-V IOMMU driver currently only supports IRQ remapping.
> As it will be adding DMA remapping support, prepare a directory
> to contain all the different feature files.
Any possibility we could put the irq remapping thing under the irq
directory?
The other drivers have it here because they are co-mingled with their
iommu HW, will hyperv have the same issue?
Jason
^ permalink raw reply
* Re: [PATCH v3 11/41] x86/kvm: Don't disable kvmclock on BSP in syscore_suspend()
From: sashiko-bot @ 2026-05-15 20:34 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-hyperv
In-Reply-To: <20260515191942.1892718-12-seanjc@google.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Moving kvmclock_disable() for the BSP to x86_platform.save_sched_clock_state leaves kvmclock enabled across hibernation if the callback is overwritten by another clocksource (e.g., Hyper-V), leading to memory corruption.
--
commit 1ada3230c82e463b5d5311eb45fc919a4e06098c
Author: Sean Christopherson <seanjc@google.com>
x86/kvm: Don't disable kvmclock on BSP in syscore_suspend()
This commit changes the suspend logic to avoid disabling kvmclock on the BSP
during syscore_suspend(). It defers the operation to the sched_clock restore
callback and introduces a new action enum to handle different offline and
suspend cases.
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index df95516a9d899..006e3a13500b4 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
[ ... ]
> @@ -178,8 +178,22 @@ static void kvm_register_clock(char *txt)
> pr_debug("kvm-clock: cpu %d, msr %llx, %s", smp_processor_id(), pa, txt);
> }
>
> +static void kvmclock_disable(void)
> +{
> + if (msr_kvm_system_time)
> + native_write_msr(msr_kvm_system_time, 0);
> +}
> +
> static void kvm_save_sched_clock_state(void)
> {
> + /*
> + * Stop host writes to kvmclock immediately prior to suspend/hibernate.
> + * If the system is hibernating, then kvmclock will likely reside at a
> + * different physical address when the system awakens, and host writes
> + * to the old address prior to reconfiguring kvmclock would clobber
> + * random memory.
> + */
> + kvmclock_disable();
> }
Can this lead to memory corruption if another clocksource overwrites the
x86_platform.save_sched_clock_state callback?
In environments where KVM emulates Hyper-V, both KVM and Hyper-V clocksources
might be initialized. If Hyper-V overwrites
x86_platform.save_sched_clock_state with its own callback, and previous
callbacks are not chained or invoked, kvm_save_sched_clock_state() will never
be called.
Because kvmclock clears its clocksource.suspend hook when registering as
sched_clock, the clocksource framework cannot act as a fallback.
This would leave kvmclock enabled on the BSP across hibernation. As mentioned
in the comment, could this result in the hypervisor writing to the old physical
memory address after resume and clobbering random memory?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515191942.1892718-1-seanjc@google.com?part=11
^ permalink raw reply
* Re: [PATCH v3 32/41] x86/tsc: Rejects attempts to override TSC calibration with lesser routine
From: sashiko-bot @ 2026-05-15 20:16 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-hyperv
In-Reply-To: <20260515191942.1892718-33-seanjc@google.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Spurious WARN_ON stack trace on every boot for CoCo (SEV-SNP/TDX) guests under KVM.
--
commit e57255e4d076143a67d0e6e44f914aec31aed8c7
Author: Sean Christopherson <seanjc@google.com>
x86/tsc: Rejects attempts to override TSC calibration with lesser routine
This commit adds a sanity check when registering a TSC frequency calibration
routine to ensure the incoming routine is as robust as the outgoing one,
rejecting it with a warning if it is not. This prevents replacing a trusted
calibration source with a less trusted one.
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 98bef1d06fa9d..7a261214fa3ef 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1319,8 +1319,13 @@ void tsc_register_calibration_routines(unsigned long (*calibrate_tsc)(void),
>
> if (properties & TSC_FREQUENCY_KNOWN)
> setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
> + else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)))
> + return;
> +
> if (properties & TSC_RELIABLE)
> setup_force_cpu_cap(X86_FEATURE_TSC_RELIABLE);
> + else if (WARN_ON(boot_cpu_has(X86_FEATURE_TSC_RELIABLE)))
> + return;
Will this cause a spurious warning stack trace on every boot for CoCo
(SEV-SNP/TDX) guests under KVM?
For KVM CoCo guests, CoCo registers first via tsc_early_init(), setting both
TSC_FREQUENCY_KNOWN and X86_FEATURE_TSC_RELIABLE.
KVM then registers second via kvmclock_init(). At this point in the patch
series, KVM only passes TSC_FREQUENCY_KNOWN, so the properties & TSC_RELIABLE
check is false, and it falls through to this WARN_ON. Since CoCo already set
the capability, the condition evaluates to true.
I see this is fixed later in the series by the commit "x86/kvmclock: Mark TSC
as reliable when it's constant and nonstop", which modifies KVM to pass
TSC_FREQ_KNOWN_AND_RELIABLE on modern hardware, preventing this warning.
Could the patch ordering be adjusted to prevent this intermediate regression?
>
> x86_platform.calibrate_tsc = calibrate_tsc;
> if (calibrate_cpu)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515191942.1892718-1-seanjc@google.com?part=32
^ permalink raw reply
* Re: [PATCH v3 37/41] x86/kvmclock: Use TSC for sched_clock if it's constant and non-stop
From: sashiko-bot @ 2026-05-15 20:09 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-hyperv
In-Reply-To: <20260515191942.1892718-38-seanjc@google.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Bypassing `kvm_sched_clock_init()` when `CONSTANT_TSC` is present ignores KVM's `PVCLOCK_TSC_STABLE_BIT`, leading to unfiltered backward time jumps in the core scheduler.
--
commit 57cf68835ef70733c037ca9ffa04fd39e1d70027
Author: Sean Christopherson <seanjc@google.com>
x86/kvmclock: Use TSC for sched_clock if it's constant and non-stop
This commit updates kvmclock_init to prefer the TSC over kvmclock for
sched_clock when the TSC is constant, non-stop, and not marked unstable.
It does this by bypassing kvm_sched_clock_init and instead modifying the
TSC properties so that TSC becomes the preferred clocksource.
Link: https://lore.kernel.org/all/Z4hDK27OV7wK572A@google.com
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
[ ... ]
> @@ -416,22 +416,22 @@ void __init kvmclock_init(void)
[ ... ]
> if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> boot_cpu_has(X86_FEATURE_NONSTOP_TSC) &&
> !check_tsc_unstable()) {
> kvm_clock.rating = 299;
> tsc_properties = TSC_FREQ_KNOWN_AND_RELIABLE;
> + } else {
> + kvm_sched_clock_init(stable);
> }
>
> - kvm_sched_clock_init(stable);
Does bypassing kvm_sched_clock_init() when CONSTANT_TSC is present
accidentally ignore KVM's PVCLOCK_TSC_STABLE_BIT?
CONSTANT_TSC indicates the hardware TSC rate doesn't change with power
states, but it doesn't guarantee cross-CPU synchronization, such as across
multi-socket systems or differing vCPU offsets from hotplug and migration.
KVM explicitly signals this cross-CPU stability via PVCLOCK_TSC_STABLE_BIT,
which is mapped to the stable variable.
Prior to this patch, kvm_sched_clock_init() was always called, invoking
__paravirt_set_sched_clock() with the stable flag. This allowed the system
to call clear_sched_clock_stable() if the hypervisor could not guarantee
synchronization, enabling crucial per-CPU monotonicity filters in
sched_clock_cpu().
By skipping kvm_sched_clock_init() entirely and registering
TSC_FREQ_KNOWN_AND_RELIABLE, the kernel's native cross-CPU TSC synchronization
checks and the clocksource watchdog appear to be bypassed.
If KVM clears PVCLOCK_TSC_STABLE_BIT, could this result in the guest
ignoring the condition and using raw, unfiltered rdtsc() values? When tasks
migrate between vCPUs with differing TSC offsets, this regression might cause
backward time jumps in sched_clock(), which breaks rq_clock monotonicity
and corrupts CFS vruntime and min_vruntime calculations.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515191942.1892718-1-seanjc@google.com?part=37
^ permalink raw reply
* Re: [PATCH v3 02/41] x86/tsc: Add helper to register CPU and TSC freq calibration routines
From: sashiko-bot @ 2026-05-15 20:06 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-hyperv
In-Reply-To: <20260515191942.1892718-3-seanjc@google.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The new helper function `tsc_register_calibration_routines` is missing the `__init` attribute.
--
commit ac711dd28fcd18f28a56325512fc567fdd7d6098
Author: Sean Christopherson <seanjc@google.com>
x86/tsc: Add helper to register CPU and TSC freq calibration routines
Add a helper to register non-native CPU and TSC frequency calibration routines.
This consolidates handling of common TSC properties forced by hypervisors
and guards against overriding a TSC calibration routine with a less trusted one.
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index f92236f40cbcc..7e639c0a94a21 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1281,6 +1281,23 @@ static void __init check_system_tsc_reliable(void)
> tsc_disable_clocksource_watchdog();
> }
>
> +/*
> + * TODO: Disentangle AMD_MEM_ENCRYPT and make SEV guest support depend on
> + * HYPERVISOR_GUEST.
> + */
> +#if defined(CONFIG_HYPERVISOR_GUEST) || defined(CONFIG_AMD_MEM_ENCRYPT)
> +void tsc_register_calibration_routines(unsigned long (*calibrate_tsc)(void),
> + unsigned long (*calibrate_cpu)(void))
This isn't a bug, but should tsc_register_calibration_routines() be marked
with the __init attribute?
Since all callers such as kvmclock_init(), snp_secure_tsc_init(), and
ms_hyperv_init_platform() are __init routines, and this function writes to
the x86_platform structure which is typically __ro_after_init, adding the
__init annotation would allow it to be discarded after the boot process
completes.
> +{
> + if (WARN_ON_ONCE(!calibrate_tsc))
> + return;
> +
> + x86_platform.calibrate_tsc = calibrate_tsc;
> + if (calibrate_cpu)
> + x86_platform.calibrate_cpu = calibrate_cpu;
> +}
> +#endif
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515191942.1892718-1-seanjc@google.com?part=2
^ permalink raw reply
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