Re: [PATCH RFC 08/18] OvmfPkg/XenbusDxe: Add Grant Table functions.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jul 16, 2014 at 04:15:37PM +0100, Anthony PERARD wrote:
> There are used to grant access of pages to other Xen domains.
> 
> This code originaly comes from the Xen Project, and more precisely from
> MiniOS.

Should you mention something about its license?

> 
> Signed-off-by: Steven Smith <sos22@cam.ac.uk>
> Signed-off-by: Grzegorz Milos <gm281@cam.ac.uk>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/XenbusDxe/GrantTable.c  | 204 ++++++++++++++++++++++++++++++++++++++++
>  OvmfPkg/XenbusDxe/GrantTable.h  |  34 +++++++
>  OvmfPkg/XenbusDxe/XenbusDxe.c   |  15 +++
>  OvmfPkg/XenbusDxe/XenbusDxe.inf |   2 +
>  4 files changed, 255 insertions(+)
>  create mode 100644 OvmfPkg/XenbusDxe/GrantTable.c
>  create mode 100644 OvmfPkg/XenbusDxe/GrantTable.h
> 
> diff --git a/OvmfPkg/XenbusDxe/GrantTable.c b/OvmfPkg/XenbusDxe/GrantTable.c
> new file mode 100644
> index 0000000..97bd15a
> --- /dev/null
> +++ b/OvmfPkg/XenbusDxe/GrantTable.c
> @@ -0,0 +1,204 @@
> +/*
> + ****************************************************************************
> + * (C) 2006 - Cambridge University
> + ****************************************************************************
> + *
> + *        File: GrantTable.c
> + *      Author: Steven Smith (sos22@cam.ac.uk)
> + *     Changes: Grzegorz Milos (gm281@cam.ac.uk)
> + *
> + *        Date: July 2006
> + *
> + * Environment: Xen Minimal OS
> + * Description: Simple grant tables implementation. About as stupid as it's
> + *  possible to be and still work.
> + *
> + ****************************************************************************
> + */
> +#include "XenbusDxe.h"
> +
> +#include <IndustryStandard/Xen/memory.h>
> +
> +#include "XenHypercall.h"
> +
> +#include "GrantTable.h"
> +#include "InterlockedCompareExchange16.h"
> +
> +#define NR_RESERVED_ENTRIES 8
> +
> +/* NR_GRANT_FRAMES must be less than or equal to that configured in Xen */
> +#define NR_GRANT_FRAMES 4
> +#define NR_GRANT_ENTRIES (NR_GRANT_FRAMES * EFI_PAGE_SIZE / sizeof(grant_entry_v1_t))
> +
> +STATIC grant_entry_v1_t *GrantTable = NULL;
> +STATIC grant_ref_t GrantList[NR_GRANT_ENTRIES];
> +#ifdef GNT_DEBUG
> +STATIC BOOLEAN GrantInUseList[NR_GRANT_ENTRIES];
> +#endif
> +
> +STATIC
> +VOID
> +XenGrantTablePutFreeEntry (
> +  grant_ref_t Ref
> +  )
> +{
> +  //local_irq_save(flags);
> +  /* MemoryFence (); */
> +  // lock ?

You just need some form of locking. If EFI has some simple Mutex() you can
use that.

> +#ifdef GNT_DEBUG
> +  ASSERT (GrantInUseList[Ref]);
> +  GrantInUseList[Ref] = FALSE;
> +#endif
> +  GrantList[Ref] = GrantList[0];
> +  GrantList[0] = Ref;
> +  //local_irq_restore(flags);
> +  /* MemoryFence (); */
> +}
> +
> +STATIC
> +grant_ref_t
> +XenGrantTableGetFreeEntry (
> +  VOID
> +  )
> +{
> +  UINTN Ref;
> +  /* local_irq_save(flags); */
> +  // lock ??
> +  Ref = GrantList[0];
> +  ASSERT (Ref >= NR_RESERVED_ENTRIES && Ref < NR_GRANT_ENTRIES);
> +  GrantList[0] = GrantList[Ref];
> +#ifdef GNT_DEBUG
> +  ASSERT (!GrantInUseList[Ref]);
> +  GrantInUseList[Ref] = TRUE;
> +#endif
> +  /* local_irq_restore(flags); */
> +  return Ref;
> +}
> +
> +STATIC
> +grant_ref_t
> +XenGrantTableGrantAccess (
> +  IN domid_t  DomainId,
> +  IN UINTN    Frame,
> +  IN BOOLEAN  ReadOnly
> +  )
> +{
> +  grant_ref_t Ref;
> +  UINT32 Flags;
> +
> +  ASSERT (GrantTable != NULL);
> +  Ref = XenGrantTableGetFreeEntry ();
> +  GrantTable[Ref].frame = Frame;
> +  GrantTable[Ref].domid = DomainId;
> +  MemoryFence ();
> +  Flags = GTF_permit_access;
> +  if (ReadOnly) {
> +    Flags |= GTF_readonly;
> +  }
> +  GrantTable[Ref].flags = Flags;
> +
> +  return Ref;
> +}
> +
> +STATIC
> +EFI_STATUS
> +XenGrantTableEndAccess (
> +  grant_ref_t Ref
> +  )
> +{
> +  UINT16 Flags, OldFlags;
> +
> +  ASSERT (GrantTable != NULL);
> +  ASSERT (Ref >= NR_RESERVED_ENTRIES && Ref < NR_GRANT_ENTRIES);
> +
> +  OldFlags = GrantTable[Ref].flags;
> +  do {
> +    if ((Flags = OldFlags) & (GTF_reading | GTF_writing)) {
> +      DEBUG ((EFI_D_WARN, "WARNING: g.e. still in use! (%x)\n", Flags));
> +      return EFI_NOT_READY;
> +    }
> +    OldFlags = InterlockedCompareExchange16 (&GrantTable[Ref].flags, Flags, 0);
> +  } while (OldFlags != Flags);
> +
> +  XenGrantTablePutFreeEntry (Ref);
> +  return EFI_SUCCESS;
> +}
> +
> +VOID
> +XenGrantTableInit (
> +  IN XENBUS_DEVICE  *Dev,
> +  IN UINT64         MmioAddr
> +  )
> +{
> +  xen_add_to_physmap_t Args;
> +  INTN Index;
> +  INTN ReturnCode;
> +
> +#ifdef GNT_DEBUG
> +  SetMem(GrantInUseList, sizeof (GrantInUseList), 1);
> +#endif
> +  for (Index = NR_RESERVED_ENTRIES; Index < NR_GRANT_ENTRIES; Index++) {
> +    XenGrantTablePutFreeEntry (Index);
> +  }
> +
> +  GrantTable = (VOID*)(UINTN) MmioAddr;
> +  for (Index = 0; Index < NR_GRANT_FRAMES; Index++) {
> +    Args.domid = DOMID_SELF;
> +    Args.idx = Index;
> +    Args.space = XENMAPSPACE_grant_table;
> +    Args.gpfn = (((xen_pfn_t) GrantTable) >> EFI_PAGE_SHIFT) + Index;
> +    ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_add_to_physmap, &Args);
> +    if (ReturnCode != 0) {
> +      DEBUG ((EFI_D_ERROR, "Xen GrantTable, add_to_physmap hypercall error: %d\n", ReturnCode));
> +    }
> +  }
> +}
> +
> +VOID
> +XenGrantTableDeinit (
> +  XENBUS_DEVICE *Dev
> +  )
> +{
> +  INTN ReturnCode, Index;
> +  xen_remove_from_physmap_t Args;
> +
> +  if (GrantTable == NULL) {
> +    return;
> +  }
> +
> +  for (Index = NR_GRANT_FRAMES - 1; Index >= 0; Index--) {
> +    Args.domid = DOMID_SELF;
> +    Args.gpfn = (((xen_pfn_t) GrantTable) >> EFI_PAGE_SHIFT) + Index;
> +    DEBUG ((EFI_D_INFO, "%a %d, removing %X\n", __func__, __LINE__, Args.gpfn));
> +    ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_remove_from_physmap, &Args);
> +    if (ReturnCode != 0) {
> +      DEBUG ((EFI_D_ERROR, "Xen GrantTable, remove_from_physmap hypercall error: %d\n", ReturnCode));
> +    }
> +  }
> +  // XXX remove it from the physmap?

Didn't you just do that?

> +  GrantTable = NULL;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +XenbusGrantAccess (
> +  IN  XENBUS_PROTOCOL *This,
> +  IN  domid_t         DomainId,
> +  IN  UINTN           Frame, // MFN
> +  IN  BOOLEAN         ReadOnly,
> +  OUT grant_ref_t     *RefPtr
> +  )
> +{
> +  *RefPtr = XenGrantTableGrantAccess (DomainId, Frame, ReadOnly);
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +XenbusGrantEndAccess (
> +  IN XENBUS_PROTOCOL  *This,
> +  IN grant_ref_t      Ref
> +  )
> +{
> +  return XenGrantTableEndAccess (Ref);
> +}
> diff --git a/OvmfPkg/XenbusDxe/GrantTable.h b/OvmfPkg/XenbusDxe/GrantTable.h
> new file mode 100644
> index 0000000..05ab4be
> --- /dev/null
> +++ b/OvmfPkg/XenbusDxe/GrantTable.h
> @@ -0,0 +1,34 @@
> +#ifndef __GNTTAB_H__
> +#define __GNTTAB_H__
> +
> +#include <IndustryStandard/Xen/grant_table.h>
> +
> +VOID
> +XenGrantTableInit (
> +  IN XENBUS_DEVICE  *Dev,
> +  IN UINT64         MmioAddr
> +  );
> +
> +VOID
> +XenGrantTableDeinit (
> +  IN XENBUS_DEVICE  *Dev
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +XenbusGrantAccess (
> +  IN  XENBUS_PROTOCOL *This,
> +  IN  domid_t         DomainId,
> +  IN  UINTN           Frame, // MFN
> +  IN  BOOLEAN         ReadOnly,
> +  OUT grant_ref_t     *RefPtr
> +  );
> +
> +EFI_STATUS
> +EFIAPI
> +XenbusGrantEndAccess (
> +  IN XENBUS_PROTOCOL  *This,
> +  IN grant_ref_t      Ref
> +  );
> +
> +#endif /* !__GNTTAB_H__ */
> diff --git a/OvmfPkg/XenbusDxe/XenbusDxe.c b/OvmfPkg/XenbusDxe/XenbusDxe.c
> index ba5e8f4..b19055d 100644
> --- a/OvmfPkg/XenbusDxe/XenbusDxe.c
> +++ b/OvmfPkg/XenbusDxe/XenbusDxe.c
> @@ -17,6 +17,7 @@
>  #include "XenbusDxe.h"
>  
>  #include "XenHypercall.h"
> +#include "GrantTable.h"
>  
>  ///
>  /// Driver Binding Protocol instance
> @@ -291,6 +292,8 @@ XenbusDxeDriverBindingStart (
>    EFI_STATUS Status;
>    XENBUS_DEVICE *Dev;
>    EFI_PCI_IO_PROTOCOL *PciIo;
> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *BarDesc;
> +  UINT64 MmioAddr;
>  
>    Status = gBS->OpenProtocol (
>                       ControllerHandle,
> @@ -310,12 +313,23 @@ XenbusDxeDriverBindingStart (
>    Dev->ControllerHandle = ControllerHandle;
>    Dev->PciIo = PciIo;
>  
> +  Status = PciIo->GetBarAttributes (PciIo, PCI_BAR_IDX1, NULL, (VOID**) &BarDesc);

I think we have a spec somewhere for this device. Do you think
it might make sense to reference it here?

> +  ASSERT_EFI_ERROR (Status);
> +  ASSERT (BarDesc->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM);

I sooo hope the spec mandates that it is always this type of BAR.