Re: [win-pv-devel] [PATCH 1/2] Add foreign page mapping functions to the GNTTAB interface

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:win-pv-devel-
> bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Rafal Wojdyla
> Sent: 25 August 2015 17:16
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: [win-pv-devel] [PATCH 1/2] Add foreign page mapping functions to
> the GNTTAB interface
> 
> GNTTAB interface now includes functions to map and unmap memory pages
> granted by a foreign domain. The page(s) are mapped under an address
> allocated from the PCI BAR space.
> 
> Signed-off-by: RafaÅ WojdyÅa <omeg@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  include/gnttab_interface.h |  62 +++++++++++++++++++++++-
>  include/xen.h              |  19 ++++++++
>  src/xen/grant_table.c      |  85 +++++++++++++++++++++++++++++++++
>  src/xenbus/gnttab.c        | 114
> +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 278 insertions(+), 2 deletions(-)
> 
> diff --git a/include/gnttab_interface.h b/include/gnttab_interface.h
> index d29440a..0016888 100644
> --- a/include/gnttab_interface.h
> +++ b/include/gnttab_interface.h
> @@ -163,6 +163,47 @@ typedef VOID
>      IN  PXENBUS_GNTTAB_CACHE    Cache
>      );
> 
> +/*! \typedef XENBUS_GNTTAB_MAP_FOREIGN_PAGES
> +    \brief Map foreign memory pages into the system address space
> +
> +    \param Interface The interface header
> +    \param Domain The domid of the foreign domain that granted the pages
> +    \param NumberPages Number of pages to map
> +    \param References Array of grant reference numbers shared by the
> foreign domain
> +    \param ReadOnly If TRUE, pages are mapped with read-only access
> +    \param Address The physical address that the foreign pages are mapped
> under
> +    (allocated from the PCI IO space)

They need not be - we could decrease_reservation out some RAM - so I'd see this 
as an implementation detail rather than something to be documented in the 
interface.

> +    \param Handles An array of tracking numbers that represent the mapping
> +    of each individual page
> + */

It's kind of ugly to expose Xen's grant mapping handles out through this 
interface. Could we do better? I think we could: How about using a hash table 
of base addresses to grant map handles? Also, that means the unmap-pages 
function only need take the base address; no need for number of pages or handle 
array. That makes the functions look for like a malloc()/free() pair which I 
think is nicer.

> +
> +typedef NTSTATUS
> +(*XENBUS_GNTTAB_MAP_FOREIGN_PAGES)(
> +    IN  PINTERFACE              Interface,
> +    IN  USHORT                  Domain,
> +    IN  ULONG                   NumberPages,
> +    IN  PULONG                  References,
> +    IN  BOOLEAN                 ReadOnly,
> +    OUT PHYSICAL_ADDRESS        *Address,
> +    OUT ULONG                   *Handles
> +    );
> +
> +/*! \typedef XENBUS_GNTTAB_UNMAP_FOREIGN_PAGES
> +    \brief Unmap foreign memory pages from the system address space
> +
> +    \param Interface The interface header
> +    \param NumberPages Number of pages to unmap
> +    \param Address The physical address that the foreign pages are mapped
> under
> +    \param Handles An array of tracking numbers that represent the mapping
> + */
> +typedef NTSTATUS
> +(*XENBUS_GNTTAB_UNMAP_FOREIGN_PAGES)(
> +    IN  PINTERFACE              Interface,
> +    IN  ULONG                   NumberPages,
> +    IN  PHYSICAL_ADDRESS        Address,
> +    IN  PULONG                  Handles
> +    );
> +
>  // {763679C5-E5C2-4A6D-8B88-6BB02EC42D8E}
>  DEFINE_GUID(GUID_XENBUS_GNTTAB_INTERFACE,
>  0x763679c5, 0xe5c2, 0x4a6d, 0x8b, 0x88, 0x6b, 0xb0, 0x2e, 0xc4, 0x2d, 0x8e);
> @@ -182,7 +223,24 @@ struct _XENBUS_GNTTAB_INTERFACE_V1 {
>      XENBUS_GNTTAB_DESTROY_CACHE         GnttabDestroyCache;
>  };
> 
> -typedef struct _XENBUS_GNTTAB_INTERFACE_V1
> XENBUS_GNTTAB_INTERFACE, *PXENBUS_GNTTAB_INTERFACE;
> +/*! \struct _XENBUS_GNTTAB_INTERFACE_V2
> +    \brief GNTTAB interface version 2 (added map/unmap foreign pages)
> +    \ingroup interfaces
> + */
> +struct _XENBUS_GNTTAB_INTERFACE_V2 {
> +    INTERFACE                           Interface;
> +    XENBUS_GNTTAB_ACQUIRE               GnttabAcquire;
> +    XENBUS_GNTTAB_RELEASE               GnttabRelease;
> +    XENBUS_GNTTAB_CREATE_CACHE          GnttabCreateCache;
> +    XENBUS_GNTTAB_PERMIT_FOREIGN_ACCESS
> GnttabPermitForeignAccess;
> +    XENBUS_GNTTAB_REVOKE_FOREIGN_ACCESS
> GnttabRevokeForeignAccess;
> +    XENBUS_GNTTAB_GET_REFERENCE         GnttabGetReference;
> +    XENBUS_GNTTAB_DESTROY_CACHE         GnttabDestroyCache;
> +    XENBUS_GNTTAB_MAP_FOREIGN_PAGES     GnttabMapForeignPages;
> +    XENBUS_GNTTAB_UNMAP_FOREIGN_PAGES
> GnttabUnmapForeignPages;
> +};
> +
> +typedef struct _XENBUS_GNTTAB_INTERFACE_V2
> XENBUS_GNTTAB_INTERFACE, *PXENBUS_GNTTAB_INTERFACE;
> 
>  /*! \def XENBUS_GNTTAB
>      \brief Macro at assist in method invocation
> @@ -193,7 +251,7 @@ typedef struct _XENBUS_GNTTAB_INTERFACE_V1
> XENBUS_GNTTAB_INTERFACE, *PXENBUS_GNT
>  #endif  // _WINDLL
> 
>  #define XENBUS_GNTTAB_INTERFACE_VERSION_MIN 1
> -#define XENBUS_GNTTAB_INTERFACE_VERSION_MAX 1
> +#define XENBUS_GNTTAB_INTERFACE_VERSION_MAX 2
> 
>  #endif  // _XENBUS_GNTTAB_INTERFACE_H
> 
> diff --git a/include/xen.h b/include/xen.h
> index 6007582..23c7ac0 100644
> --- a/include/xen.h
> +++ b/include/xen.h
> @@ -258,6 +258,25 @@ GrantTableCopy(
>      IN  ULONG               Count
>      );
> 
> +__checkReturn
> +XEN_API
> +NTSTATUS
> +GrantTableMapForeignPage(
> +    IN  USHORT                  Domain,
> +    IN  ULONG                   GrantRef,
> +    IN  PHYSICAL_ADDRESS        Address,
> +    IN  BOOLEAN                 ReadOnly,
> +    OUT ULONG                   *Handle
> +    );
> +
> +__checkReturn
> +XEN_API
> +NTSTATUS
> +GrantTableUnmapForeignPage(
> +    IN  ULONG                   Handle,
> +    IN  PHYSICAL_ADDRESS        Address
> +    );
> +
>  // SCHED
> 
>  __checkReturn
> diff --git a/src/xen/grant_table.c b/src/xen/grant_table.c
> index 6facb3f..6450062 100644
> --- a/src/xen/grant_table.c
> +++ b/src/xen/grant_table.c
> @@ -131,3 +131,88 @@ fail1:
> 
>      return status;
>  }
> +
> +__checkReturn
> +XEN_API
> +NTSTATUS
> +GrantTableMapForeignPage(
> +    IN  USHORT                  Domain,
> +    IN  ULONG                   GrantRef,
> +    IN  PHYSICAL_ADDRESS        Address,
> +    IN  BOOLEAN                 ReadOnly,
> +    OUT ULONG                   *Handle
> +    )
> +{
> +    struct gnttab_map_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.host_addr = Address.QuadPart;
> +
> +    rc = GrantTableOp(GNTTABOP_map_grant_ref, &op, 1);
> +
> +    if (rc < 0) {
> +        ERRNO_TO_STATUS(-rc, status);
> +        goto fail1;
> +    }
> +
> +    if (op.status != GNTST_okay) {
> +        status = STATUS_UNSUCCESSFUL;
> +        goto fail2;
> +    }
> +
> +    *Handle = op.handle;
> +
> +    return STATUS_SUCCESS;
> +
> +fail2:
> +    Error("fail2: op.status = %d\n", op.status);

In general I've tried to keep 'Error' messages other than 'fail1:'  plain. 
Rather than just setting status to unsuccessful and logging the raw grant 
status here it would be nicer to have a GNTST_TO_STATUS() macro to set status 
to something meaningful and then just log that in the 'fail1:' below.

> +fail1:
> +    Error("fail1 (%08x)\n", status);
> +
> +    return status;
> +}
> +
> +__checkReturn
> +XEN_API
> +NTSTATUS
> +GrantTableUnmapForeignPage(
> +    IN  ULONG                   Handle,
> +    IN  PHYSICAL_ADDRESS        Address
> +    )
> +{
> +    struct gnttab_unmap_grant_ref op;
> +    LONG_PTR rc;
> +    NTSTATUS status;
> +
> +    RtlZeroMemory(&op, sizeof(op));
> +    op.handle = Handle;
> +    op.host_addr = Address.QuadPart;
> +
> +    rc = GrantTableOp(GNTTABOP_unmap_grant_ref, &op, 1);
> +
> +    if (rc < 0) {
> +        ERRNO_TO_STATUS(-rc, status);
> +        goto fail1;
> +    }
> +
> +    if (op.status != GNTST_okay) {
> +        status = STATUS_UNSUCCESSFUL;
> +        goto fail2;
> +    }
> +
> +    return STATUS_SUCCESS;
> +
> +fail2:
> +    Error("op.status = %d\n", op.status);

Same here.

> +fail1:
> +    Error("fail1 (%08x)\n", status);
> +
> +    return status;
> +}
> diff --git a/src/xenbus/gnttab.c b/src/xenbus/gnttab.c
> index 165e38f..24d45af 100644
> --- a/src/xenbus/gnttab.c
> +++ b/src/xenbus/gnttab.c
> @@ -534,6 +534,90 @@ GnttabGetReference(
>      return (ULONG)Entry->Reference;
>  }
> 
> +static NTSTATUS
> +GnttabMapForeignPages(
> +    IN  PINTERFACE              Interface,
> +    IN  USHORT                  Domain,
> +    IN  ULONG                   NumberPages,
> +    IN  PULONG                  References,
> +    IN  BOOLEAN                 ReadOnly,
> +    OUT PHYSICAL_ADDRESS        *Address,
> +    OUT ULONG                   *Handles
> +    )
> +{
> +    NTSTATUS                    status;
> +    PXENBUS_GNTTAB_CONTEXT      Context = Interface->Context;
> +    ULONG                       PageIndex;
> +    PHYSICAL_ADDRESS            PageAddress;
> +
> +    status = FdoAllocateIoSpace(Context->Fdo, NumberPages * PAGE_SIZE,
> Address);
> +    if (!NT_SUCCESS(status))
> +        goto fail1;
> +
> +    PageAddress.QuadPart = Address->QuadPart;
> +
> +    for (PageIndex = 0; PageIndex < NumberPages; PageIndex++) {
> +        status = GrantTableMapForeignPage(Domain,
> +                                          References[PageIndex],
> +                                          PageAddress,
> +                                          ReadOnly,
> +                                          &(Handles[PageIndex]));
> +        if (!NT_SUCCESS(status))
> +            goto fail2;
> +
> +        PageAddress.QuadPart += PAGE_SIZE;
> +    }
> +
> +    return STATUS_SUCCESS;
> +
> +fail2:
> +    Error("fail2: PageIndex %lu, PageAddress %p, Handle %lu\n", PageIndex,
> PageAddress.QuadPart, Handles[PageIndex]);
> +
> +    while (PageIndex > 0) {
> +        --PageIndex;
> +        PageAddress.QuadPart -= PAGE_SIZE;
> +
> ASSERT(NT_SUCCESS(GrantTableUnmapForeignPage(Handles[PageIndex],
> PageAddress)));
> +    }
> +
> +    FdoFreeIoSpace(Context->Fdo, *Address, NumberPages * PAGE_SIZE);
> +
> +fail1:
> +    Error("fail1: (%08x)\n", status);
> +    return status;
> +}
> +
> +static NTSTATUS
> +GnttabUnmapForeignPages(
> +    IN  PINTERFACE              Interface,
> +    IN  ULONG                   NumberPages,
> +    IN  PHYSICAL_ADDRESS        Address,
> +    IN  PULONG                  Handles
> +    )
> +{
> +    NTSTATUS                    status;
> +    PXENBUS_GNTTAB_CONTEXT      Context = Interface->Context;
> +    ULONG                       PageIndex;
> +    PHYSICAL_ADDRESS            PageAddress;
> +
> +    PageAddress.QuadPart = Address.QuadPart;
> +
> +    for (PageIndex = 0; PageIndex < NumberPages; PageIndex++) {
> +        status = GrantTableUnmapForeignPage(Handles[PageIndex],
> PageAddress);
> +        if (!NT_SUCCESS(status))
> +            goto fail1;
> +
> +        PageAddress.QuadPart += PAGE_SIZE;
> +    }
> +
> +    FdoFreeIoSpace(Context->Fdo, Address, NumberPages * PAGE_SIZE);
> +    return STATUS_SUCCESS;
> +
> +fail1:
> +    Error("fail1: (%08x), leaking memory at %p, size 0x%lx. PageIndex = %lu,
> PageAddress = %p, Handle = %lu\n",
> +          status, Address.QuadPart, NumberPages * PAGE_SIZE, PageIndex,
> PageAddress.QuadPart, Handles[PageIndex]);

Hmm. What do we want to do on an unmap failure? I suspect such a failure 
probably means something has gone pretty disastrously wrong somewhere. We 
should probably bugcheck and let Xen clean up, otherwise we're potentially 
preventing another domain from dying.

  Paul

> +    return status;
> +}
> +
>  static VOID
>  GnttabSuspendCallbackEarly(
>      IN  PVOID               Argument
> @@ -789,6 +873,19 @@ static struct _XENBUS_GNTTAB_INTERFACE_V1
> GnttabInterfaceVersion1 = {
>      GnttabDestroyCache
>  };
> 
> +static struct _XENBUS_GNTTAB_INTERFACE_V2   GnttabInterfaceVersion2 =
> {
> +    { sizeof(struct _XENBUS_GNTTAB_INTERFACE_V2), 2, NULL, NULL, NULL
> },
> +    GnttabAcquire,
> +    GnttabRelease,
> +    GnttabCreateCache,
> +    GnttabPermitForeignAccess,
> +    GnttabRevokeForeignAccess,
> +    GnttabGetReference,
> +    GnttabDestroyCache,
> +    GnttabMapForeignPages,
> +    GnttabUnmapForeignPages
> +};
> +
>  NTSTATUS
>  GnttabInitialize(
>      IN  PXENBUS_FDO             Fdo,
> @@ -878,6 +975,23 @@ GnttabGetInterface(
>          status = STATUS_SUCCESS;
>          break;
>      }
> +    case 2: {
> +        struct _XENBUS_GNTTAB_INTERFACE_V2  *GnttabInterface;
> +
> +        GnttabInterface = (struct _XENBUS_GNTTAB_INTERFACE_V2
> *)Interface;
> +
> +        status = STATUS_BUFFER_OVERFLOW;
> +        if (Size < sizeof(struct _XENBUS_GNTTAB_INTERFACE_V2))
> +            break;
> +
> +        *GnttabInterface = GnttabInterfaceVersion2;
> +
> +        ASSERT3U(Interface->Version, ==, Version);
> +        Interface->Context = Context;
> +
> +        status = STATUS_SUCCESS;
> +        break;
> +    }
>      default:
>          status = STATUS_NOT_SUPPORTED;
>          break;
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel