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

[RafaÅ WojdyÅa <omeg@xxxxxxxxxxxxxxxxxxxxxx>]

On 2015-09-04 10:58, Paul Durrant wrote:
>> -----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.
> 
True.

>> +    \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.
> 
Agreed. In my current code xeniface does most of that encapsulation but your 
approach makes more sense.
>> +
>> +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.
Noted.
> 
>> +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.
> 
Yeah, failures with mapping/unmapping should probably be fatal. I've never 
encountered any so it's probably safer to just bugcheck.

>   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;
>>

Thanks for the input, I'll send a revised version in a few days.

-- 
RafaÅ WojdyÅa
Qubes Tools for Windows developer
https://www.qubes-os.org/

_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel