[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] Use batched hypercalls for mapping foreign pages
- To: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>, win-pv-devel@xxxxxxxxxxxxxxxxxxxx
- From: Rafał Wojdyła <omeg@xxxxxxxxxxxxxxxxxxxxxx>
- Date: Mon, 8 Jun 2026 10:49:46 +0200
- Authentication-results: eu.smtp.expurgate.cloud; dkim=pass header.s=fm1 header.d=invisiblethingslab.com header.i="@invisiblethingslab.com" header.h="Content-Transfer-Encoding:Content-Type:Date:From:In-Reply-To:Message-ID:MIME-Version:References:Subject:To"; dkim=pass header.s=fm1 header.d=messagingengine.com header.i="@messagingengine.com" header.h="Content-Transfer-Encoding:Content-Type:Date:Feedback-ID:From:In-Reply-To:Message-ID:MIME-Version:References:Subject:To:X-ME-Proxy:X-ME-Sender"
- Delivery-date: Mon, 08 Jun 2026 08:49:56 +0000
- Feedback-id: i409c4082:Fastmail
- List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>
W dniu 8.06.2026 o 10:17, Tu Dinh pisze:
Hi,
Do you have some tools that can test the gnttab/evtchn ioctl?
We use these extensively in Qubes OS in vchan implementation and sharing
guest's emulated GPU framebuffer with our GUI domain. Adding a
standalone test tool is a good idea though. We have a simple vchan test
tool in our repos, I can add something similar to xeniface perhaps?
Please see comments below:
On 05/06/2026 23:18, Rafał Wojdyła wrote:
Use one hypercall op with multiple pages instead of
separate hypercalls for each page.
Signed-off-by: Rafał Wojdyła <omeg@xxxxxxxxxxxxxxxxxxxxxx>
---
include/xen.h | 20 +++--
src/xen/grant_table.c | 187 +++++++++++++++++++++++++++++++++---------
src/xenbus/gnttab.c | 47 ++++-------
3 files changed, 177 insertions(+), 77 deletions(-)
diff --git a/include/xen.h b/include/xen.h
index 0c3e8a4..eaab567 100644
--- a/include/xen.h
+++ b/include/xen.h
@@ -265,20 +265,22 @@ GrantTableCopy(
_Check_return_
XEN_API
NTSTATUS
-GrantTableMapForeignPage(
- _In_ USHORT Domain,
- _In_ ULONG GrantRef,
- _In_ PHYSICAL_ADDRESS Address,
- _In_ BOOLEAN ReadOnly,
- _Out_ ULONG *Handle
+GrantTableMapForeignPages(
+ _In_ USHORT Domain,
+ _In_ ULONG NumberPages,
+ _In_reads_(NumberPages) PULONG GrantRefs,
+ _In_ PHYSICAL_ADDRESS Address,
+ _In_ BOOLEAN ReadOnly,
+ _Out_writes_(NumberPages) PULONG Handles
);
_Check_return_
XEN_API
NTSTATUS
-GrantTableUnmapForeignPage(
- _In_ ULONG Handle,
- _In_ PHYSICAL_ADDRESS Address
+GrantTableUnmapForeignPages(
+ _In_ ULONG NumberPages,
+ _In_reads_(NumberPages) PULONG Handles,
+ _In_ PHYSICAL_ADDRESS Address
);
_Check_return_
diff --git a/src/xen/grant_table.c b/src/xen/grant_table.c
index 96a5a26..a9099c6 100644
--- a/src/xen/grant_table.c
+++ b/src/xen/grant_table.c
@@ -38,10 +38,13 @@
#include "hypercall.h"
#include "dbg_print.h"
#include "assert.h"
+#include "util.h"
#pragma warning(push)
#pragma warning(disable:4127) // conditional expression is constant
+#define XEN_GNTTAB_TAG 'TNGX'
+
// Most of the GNTST_* values don't have meaningful NTSTATUS counterparts,
// this macro translates those that do.
#define GNTST_TO_STATUS(_gntst, _status) \
@@ -168,27 +171,20 @@ fail1:
_Check_return_
XEN_API
NTSTATUS
-GrantTableMapForeignPage(
- _In_ USHORT Domain,
- _In_ ULONG GrantRef,
- _In_ PHYSICAL_ADDRESS Address,
- _In_ BOOLEAN ReadOnly,
- _Out_ ULONG *Handle
+GrantTableUnmapForeignPage(
+ _In_ ULONG Handle,
+ _In_ PHYSICAL_ADDRESS Address
)
{
- struct gnttab_map_grant_ref op;
- LONG_PTR rc;
- NTSTATUS status;
+ struct gnttab_unmap_grant_ref op;
+ LONG_PTR rc;
+ NTSTATUS status;
RtlZeroMemory(&op, sizeof(op));
- op.dom = Domain;
- op.ref = GrantRef;
- op.flags = GNTMAP_host_map;
- if (ReadOnly)
- op.flags |= GNTMAP_readonly;
+ op.handle = Handle;
op.host_addr = Address.QuadPart;
- rc = GrantTableOp(GNTTABOP_map_grant_ref, &op, 1);
+ rc = GrantTableOp(GNTTABOP_unmap_grant_ref, &op, 1);
if (rc < 0) {
ERRNO_TO_STATUS(-rc, status);
@@ -196,9 +192,7 @@ GrantTableMapForeignPage(
}
if (op.status != GNTST_okay) {
- Warning("%u:%u -> %u.%u failed (%d)\n",
- op.dom,
- op.ref,
+ Warning("%u.%u failed (%d)\n",
Address.HighPart,
Address.LowPart,
op.status);
@@ -207,8 +201,6 @@ GrantTableMapForeignPage(
goto fail2;
}
- *Handle = op.handle;
-
return STATUS_SUCCESS;
fail2:
@@ -223,40 +215,161 @@ fail1:
_Check_return_
XEN_API
NTSTATUS
-GrantTableUnmapForeignPage(
- _In_ ULONG Handle,
- _In_ PHYSICAL_ADDRESS Address
+GrantTableMapForeignPages(
+ _In_ USHORT Domain,
+ _In_ ULONG NumberPages,
+ _In_reads_(NumberPages) PULONG GrantRefs,
+ _In_ PHYSICAL_ADDRESS Address,
+ _In_ BOOLEAN ReadOnly,
+ _Out_writes_(NumberPages) PULONG Handles
)
{
- struct gnttab_unmap_grant_ref op;
- LONG_PTR rc;
- NTSTATUS status;
+ struct gnttab_map_grant_ref *ops;
+ LONG_PTR rc;
+ NTSTATUS status;
+ ULONG i;
+ PHYSICAL_ADDRESS page_address;
+
+ status = STATUS_NO_MEMORY;
+ ops = __AllocatePoolWithTag(NonPagedPool,
+ NumberPages * sizeof(*ops),
+ XEN_GNTTAB_TAG);
+ if (ops == NULL)
+ goto fail1;
An unconstrained array of NumberPages * sizeof(*ops) is being allocated
from the nonpaged pool. As the number of potential pages coming from
callers could be fairly large, perhaps the batching could be done in a
limited fashion (every N pages, and specifically GNTTAB_UNMAP_BATCH_SIZE
for the unmap operation) and therefore get much of the benefit without
needing a large allocation.
Fair, I'll chunk this allocation to be bounded. The IOCTL for this
currently has a limit of 1024*1024 pages which is also probably too large...
- RtlZeroMemory(&op, sizeof(op));
- op.handle = Handle;
- op.host_addr = Address.QuadPart;
+ page_address.QuadPart = Address.QuadPart;
+ for (i = 0; i < NumberPages; i++) {
+ ops[i].dom = Domain;
+ ops[i].ref = GrantRefs[i];
+ ops[i].flags = GNTMAP_host_map;
+ if (ReadOnly)
+ ops[i].flags |= GNTMAP_readonly;
- rc = GrantTableOp(GNTTABOP_unmap_grant_ref, &op, 1);
+ ops[i].host_addr = page_address.QuadPart;
+ page_address.QuadPart += PAGE_SIZE;
+ }
+
+ rc = GrantTableOp(GNTTABOP_map_grant_ref, ops, NumberPages);
if (rc < 0) {
ERRNO_TO_STATUS(-rc, status);
+ goto fail2;
+ }
+
+ // check every op, they are independently processed
+ status = STATUS_SUCCESS;
+ for (i = 0; i < NumberPages; i++) {
+ if (ops[i].status != GNTST_okay) {
+ Warning("op[%u] %u:%u -> %u.%u failed (%d)\n",
If the mapped area is big, this would effectively print millions of
warnings to the guest debug output. I think this failure path should be
limited to one or very few prints per call to GrantTableMapForeignPages.
Agreed, I'll also reduce verbosity of other per-page prints.
Also, the printf specifier here should be "op[%u] %hu:%u -> 0x%08x%08x
failed (%hd)\n" since ops[i].dom and ops[i].status are 16-bit types.
+ i, ops[i].dom, ops[i].ref,
+ (ULONG)(ops[i].host_addr >> 32),
+ (ULONG)ops[i].host_addr,
+ ops[i].status);
+
+ if (NT_SUCCESS(status))
+ GNTST_TO_STATUS(ops[i].status, status);
+ continue;
+ }
+
+ Handles[i] = ops[i].handle;
+ }
+
+ if (!NT_SUCCESS(status))
+ goto fail3;
+
+ __FreePoolWithTag(ops, XEN_GNTTAB_TAG);
+ return STATUS_SUCCESS;
+
+fail3:
+ Error("fail3 (%08x)\n", status);
+
+ // undo successful ops
+ for (i = 0; i < NumberPages; i++) {
+ PHYSICAL_ADDRESS unmap_address;
+ NTSTATUS unmap_status;
+
+ if (ops[i].status != GNTST_okay)
+ continue;
+
+ unmap_address.QuadPart = ops[i].host_addr;
+ unmap_status = GrantTableUnmapForeignPage(ops[i].handle,
+ unmap_address);
+ // don't leave the memory in inconsistent state
+ BUG_ON(!NT_SUCCESS(unmap_status));
I'm not sure a bugcheck is desirable here, as it might be reachable from
userspace when resources are low.
Yeah, I wasn't sure about this one. I guess we can indicate failed pages
by invalid Handles output entries and let the caller deal with that.
+ }
+
+fail2:
+ Error("fail2 (%08x)\n", status);
+ __FreePoolWithTag(ops, XEN_GNTTAB_TAG);
+
+fail1:
+ Error("fail1 (%08x)\n", status);
+
+ return status;
+}
+
+_Check_return_
+XEN_API
+NTSTATUS
+GrantTableUnmapForeignPages(
+ _In_ ULONG NumberPages,
+ _In_reads_(NumberPages) PULONG Handles,
+ _In_ PHYSICAL_ADDRESS Address
+ )
+{
+ struct gnttab_unmap_grant_ref *ops;
+ LONG_PTR rc;
+ NTSTATUS status;
+ ULONG i;
+ PHYSICAL_ADDRESS page_address;
+
+ status = STATUS_NO_MEMORY;
+ ops = __AllocatePoolWithTag(NonPagedPool,
+ NumberPages * sizeof(*ops),
+ XEN_GNTTAB_TAG);
+ if (ops == NULL)
goto fail1;
+
+ page_address.QuadPart = Address.QuadPart;
+ for (i = 0; i < NumberPages; i++) {
+ ops[i].handle = Handles[i];
+ ops[i].host_addr = page_address.QuadPart;
+ page_address.QuadPart += PAGE_SIZE;
}
- if (op.status != GNTST_okay) {
- Warning("%u.%u failed (%d)\n",
- Address.HighPart,
- Address.LowPart,
- op.status);
+ rc = GrantTableOp(GNTTABOP_unmap_grant_ref, ops, NumberPages);
- GNTST_TO_STATUS(op.status, status);
+ if (rc < 0) {
+ ERRNO_TO_STATUS(-rc, status);
goto fail2;
}
+ status = STATUS_SUCCESS;
+ for (i = 0; i < NumberPages; i++) {
+ if (ops[i].status != GNTST_okay) {
+ Warning("op[%u] %u.%u failed (%d)\n",
ops[i].status should similarly should be "(%hd)\n" here.
+ i,
+ (ULONG)(ops[i].host_addr >> 32),
+ (ULONG)ops[i].host_addr,
+ ops[i].status);
+
+ if (NT_SUCCESS(status))
+ GNTST_TO_STATUS(ops[i].status, status);
+ }
+ }
+
+ __FreePoolWithTag(ops, XEN_GNTTAB_TAG);
+
+ if (!NT_SUCCESS(status)) {
+ Error("fail3 (%08x)\n", status);
+ return status;
+ }
+
return STATUS_SUCCESS;
fail2:
- Error("fail2\n");
+ Error("fail2 (%08x)\n", status);
+ __FreePoolWithTag(ops, XEN_GNTTAB_TAG);
fail1:
Error("fail1 (%08x)\n", status);
diff --git a/src/xenbus/gnttab.c b/src/xenbus/gnttab.c
index 71f8ec1..f6fcef6 100644
--- a/src/xenbus/gnttab.c
+++ b/src/xenbus/gnttab.c
@@ -667,8 +667,6 @@ GnttabMapForeignPages(
{
PXENBUS_GNTTAB_CONTEXT Context = Interface->Context;
PMDL Mdl;
- LONG PageIndex;
- PHYSICAL_ADDRESS PageAddress;
PXENBUS_GNTTAB_MAP_ENTRY MapEntry;
NTSTATUS status;
@@ -689,19 +687,15 @@ GnttabMapForeignPages(
MapEntry->Mdl = Mdl;
Address->QuadPart = MmGetMdlPfnArray(Mdl)[0] << PAGE_SHIFT;
- PageAddress.QuadPart = Address->QuadPart;
-
- for (PageIndex = 0; PageIndex < (LONG)NumberPages; PageIndex++) {
- status = GrantTableMapForeignPage(Domain,
- References[PageIndex],
- PageAddress,
- ReadOnly,
- &MapEntry->MapHandles[PageIndex]);
- if (!NT_SUCCESS(status))
- goto fail3;
- PageAddress.QuadPart += PAGE_SIZE;
- }
+ status = GrantTableMapForeignPages(Domain,
+ NumberPages,
+ References,
+ *Address,
+ ReadOnly,
+ MapEntry->MapHandles);
+ if (!NT_SUCCESS(status))
+ goto fail3;
status = HashTableAdd(Context->MapTable,
(ULONG_PTR)Address->QuadPart,
@@ -714,15 +708,13 @@ GnttabMapForeignPages(
fail4:
Error("fail4\n");
+ (VOID) GrantTableUnmapForeignPages(NumberPages,
+ MapEntry->MapHandles,
+ *Address);
+
fail3:
Error("fail3\n");
- while (--PageIndex >= 0) {
- PageAddress.QuadPart -= PAGE_SIZE;
- (VOID) GrantTableUnmapForeignPage(MapEntry->MapHandles[PageIndex],
- PageAddress);
- }
-
Address->QuadPart = 0;
__GnttabFree(MapEntry);
@@ -746,8 +738,6 @@ GnttabUnmapForeignPages(
{
PXENBUS_GNTTAB_CONTEXT Context = Interface->Context;
ULONG NumberPages;
- PHYSICAL_ADDRESS PageAddress;
- ULONG PageIndex;
PXENBUS_GNTTAB_MAP_ENTRY MapEntry;
PMDL Mdl;
NTSTATUS status;
@@ -763,18 +753,13 @@ GnttabUnmapForeignPages(
if (!NT_SUCCESS(status))
goto fail2;
- PageAddress.QuadPart = Address.QuadPart;
-
Mdl = MapEntry->Mdl;
NumberPages = Mdl->ByteCount >> PAGE_SHIFT;
- for (PageIndex = 0; PageIndex < NumberPages; PageIndex++) {
- status = GrantTableUnmapForeignPage(MapEntry->MapHandles[PageIndex],
- PageAddress);
- BUG_ON(!NT_SUCCESS(status));
-
- PageAddress.QuadPart += PAGE_SIZE;
- }
+ status = GrantTableUnmapForeignPages(NumberPages,
+ MapEntry->MapHandles,
+ Address);
+ BUG_ON(!NT_SUCCESS(status));
__GnttabFree(MapEntry);
|
