|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] Use batched hypercalls for mapping foreign pages
Hi,
On 10/06/2026 14:45, Rafał Wojdyła wrote:
> Use one hypercall op with multiple pages instead of
> separate hypercalls for each page.
>
> Also fix debug display of addresses.
>
> Signed-off-by: Rafał Wojdyła <omeg@xxxxxxxxxxxxxxxxxxxxxx>
A comment on unrelated code: I didn't realize that
gnttab.c:GnttabMapForeignPages already allocates one ULONG per requested
page, which can grow up to 4MB per call given the current NumberPages
constraint of 1M. I wonder if we can further constrain this value or
alternatively allocate the MapEntry buffer from paged pool instead,
given that this buffer isn't actively used.
More comments below.
> ---
> include/xen.h | 20 ++--
> src/xen/grant_table.c | 241 +++++++++++++++++++++++++++++++++++-------
> src/xenbus/gnttab.c | 47 +++-----
> 3 files changed, 228 insertions(+), 80 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..8935bd4 100644
> --- a/src/xen/grant_table.c
> +++ b/src/xen/grant_table.c
> @@ -38,10 +38,15 @@
> #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'
> +
> +#define GNTTAB_BATCH_SIZE 256
> +
> // 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 +173,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 +194,7 @@ GrantTableMapForeignPage(
> }
>
> if (op.status != GNTST_okay) {
> - Warning("%u:%u -> %u.%u failed (%d)\n",
> - op.dom,
> - op.ref,
> + Warning("0x%08x%08x failed (%hd)\n",
> Address.HighPart,
> Address.LowPart,
> op.status);
> @@ -207,8 +203,6 @@ GrantTableMapForeignPage(
> goto fail2;
> }
>
> - *Handle = op.handle;
> -
> return STATUS_SUCCESS;
>
> fail2:
> @@ -223,40 +217,207 @@ 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 chunk_start;
> + ULONG chunk_size;
> + ULONG i;
> + ULONG fail_count;
> + ULONG first_fail_index;
> + PHYSICAL_ADDRESS page_address;
> +
> + for (i = 0; i < NumberPages; i++)
> + Handles[i] = 0;
> +
> + status = STATUS_NO_MEMORY;
> + ops = __AllocatePoolWithTag(NonPagedPool,
> + GNTTAB_BATCH_SIZE * sizeof(*ops),
> + XEN_GNTTAB_TAG);
> + if (ops == NULL)
> + goto fail1;
>
> - RtlZeroMemory(&op, sizeof(op));
> - op.handle = Handle;
> - op.host_addr = Address.QuadPart;
> + status = STATUS_SUCCESS;
> + fail_count = 0;
> + first_fail_index = 0;
> + page_address.QuadPart = Address.QuadPart;
> +
> + for (chunk_start = 0;
> + chunk_start < NumberPages;
> + chunk_start += GNTTAB_BATCH_SIZE) {
> + chunk_size = (NumberPages - chunk_start < GNTTAB_BATCH_SIZE) ?
> + NumberPages - chunk_start : GNTTAB_BATCH_SIZE;
> +
> + for (i = 0; i < chunk_size; i++) {
> + ops[i].dom = Domain;
> + ops[i].ref = GrantRefs[chunk_start + i];
> + ops[i].flags = GNTMAP_host_map;
> + if (ReadOnly)
> + ops[i].flags |= GNTMAP_readonly;
> +
> + ops[i].host_addr = page_address.QuadPart +
> + ((ULONGLONG)i << PAGE_SHIFT);
> + }
> +
> + rc = GrantTableOp(GNTTABOP_map_grant_ref, ops, chunk_size);
> +
> + if (rc < 0) {
> + ERRNO_TO_STATUS(-rc, status);
> + goto fail2;
Going to fail2 here will skip cleanup of existing successful chunks, so
I think you might want to goto fail3 instead (while keeping track of
successful indices)
> + }
> +
> + // check every op, they are independently processed
> + for (i = 0; i < chunk_size; i++) {
> + if (ops[i].status != GNTST_okay) {
> + if (fail_count == 0) {
> + Warning("op[%u] %hu:%u -> 0x%08x%08x failed (%hd)\n",
> + chunk_start + i,
> + ops[i].dom, ops[i].ref,
> + (ULONG)(ops[i].host_addr >> 32),
> + (ULONG)ops[i].host_addr,
> + ops[i].status);
> + first_fail_index = chunk_start + i;
> + GNTST_TO_STATUS(ops[i].status, status);
> + }
> + fail_count++;
> + continue;
> + }
> +
> + Handles[chunk_start + i] = ops[i].handle;
> + }
> +
> + page_address.QuadPart += (ULONGLONG)chunk_size << PAGE_SHIFT;
> + }
>
> - rc = GrantTableOp(GNTTABOP_unmap_grant_ref, &op, 1);
> + if (fail_count > 0) {
> + Warning("%u of %u ops failed (first at %u)\n",
> + fail_count, NumberPages, first_fail_index);
> + goto fail3;
> + }
>
> - if (rc < 0) {
> - ERRNO_TO_STATUS(-rc, status);
> + __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 (Handles[i] == 0)
> + continue;
> +
> + unmap_address.QuadPart = Address.QuadPart + ((ULONGLONG)i <<
> PAGE_SHIFT);
> + unmap_status = GrantTableUnmapForeignPage(Handles[i],
> + unmap_address);
> + if (!NT_SUCCESS(unmap_status))
> + Warning("unmap of handle 0x%x at op %u failed (%08x), leaking\n",
> + Handles[i], i, unmap_status);
> + }
> +
> +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
> + )
> +{
Is there a way for callers to know which pages got successfully unmapped
and which pages were not?
> + struct gnttab_unmap_grant_ref *ops;
> + LONG_PTR rc;
> + NTSTATUS status;
> + ULONG chunk_start;
> + ULONG chunk_size;
> + ULONG i;
> + ULONG fail_count;
> + ULONG first_fail_index;
> + PHYSICAL_ADDRESS page_address;
> +
> + status = STATUS_NO_MEMORY;
> + ops = __AllocatePoolWithTag(NonPagedPool,
> + GNTTAB_BATCH_SIZE * sizeof(*ops),
> + XEN_GNTTAB_TAG);
> + if (ops == NULL)
> goto fail1;
> +
> + status = STATUS_SUCCESS;
> + fail_count = 0;
> + first_fail_index = 0;
> + page_address.QuadPart = Address.QuadPart;
> +
> + for (chunk_start = 0;
> + chunk_start < NumberPages;
> + chunk_start += GNTTAB_BATCH_SIZE) {
> + chunk_size = (NumberPages - chunk_start < GNTTAB_BATCH_SIZE) ?
> + NumberPages - chunk_start : GNTTAB_BATCH_SIZE;
> +
> + for (i = 0; i < chunk_size; i++) {
> + ops[i].handle = Handles[chunk_start + i];
> + ops[i].host_addr = page_address.QuadPart +
> + ((ULONGLONG)i << PAGE_SHIFT);
> + }
> +
> + rc = GrantTableOp(GNTTABOP_unmap_grant_ref, ops, chunk_size);
> +
> + if (rc < 0) {
> + ERRNO_TO_STATUS(-rc, status);
> + goto fail2;
Should we continue with cleanup of other chunks even if some chunks
might have failed?
> + }
> +
> + for (i = 0; i < chunk_size; i++) {
> + if (ops[i].status != GNTST_okay) {
> + if (fail_count == 0) {
> + Warning("op[%u] 0x%08x%08x failed (%hd)\n",
> + chunk_start + i,
> + (ULONG)(ops[i].host_addr >> 32),
> + (ULONG)ops[i].host_addr,
> + ops[i].status);
> + first_fail_index = chunk_start + i;
> + GNTST_TO_STATUS(ops[i].status, status);
> + }
> + fail_count++;
> + }
> + }
> +
> + page_address.QuadPart += (ULONGLONG)chunk_size << PAGE_SHIFT;
> }
>
> - if (op.status != GNTST_okay) {
> - Warning("%u.%u failed (%d)\n",
> - Address.HighPart,
> - Address.LowPart,
> - op.status);
> + __FreePoolWithTag(ops, XEN_GNTTAB_TAG);
>
> - GNTST_TO_STATUS(op.status, status);
> - goto fail2;
> + if (fail_count > 0) {
> + Warning("%u of %u ops failed (first at %u)\n",
> + fail_count, NumberPages, first_fail_index);
> + Error("fail3 (%08x)\n", status);
> + return status;
Was the intention here to have a fail3 block instead?
> }
>
> 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));
Similarly to my previous email, I don't think we should crash here since
we can be low on resources, or we can have some edge cases that allows
userspace to trigger unmap failures.
>
> __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 |