|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/2] Use batched hypercalls for mapping foreign pages
Hi,
Do you have some tools that can test the gnttab/evtchn ioctl?
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.
>
> - 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.
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.
> + }
> +
> +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);
>
--
Ngoc Tu Dinh | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |