Re: [Xen-devel] [PATCH v4 17/29] Ovmf/Xen: refactor XenBusDxe hypercall implementation

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Feb 12, 2015 at 07:19:09PM +0800, Ard Biesheuvel wrote:
> This refactors the Xen hypercall implementation that is part of the
> XenBusDxe driver, in preparation of splitting it off entirely into
> a XenHypercallLib library. This involves:
> - removing the dependency on XENBUS_DEVICE* pointers in the XenHypercall()
>   prototypes
> - moving the discovered hyperpage address to a global variable
> - moving XenGetSharedInfoPage() to its only user XenBusDxe.c (the shared info
>   page is not strictly part of the Xen hypercall interface, and is not used
>   by other expected users of XenHypercallLib such as the Xen console version
>   of SerialPortLib
> - reimplement XenHypercall2() in C and move the indexing of the hyperpage
>   there; the existing asm implementations are renamed to __XenHypercall2() and
>   invoked from the new C implementation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Acked-by: Laszlo Ersek <lersek@xxxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> ---
>  OvmfPkg/XenBusDxe/EventChannel.c      | 11 +++--------
>  OvmfPkg/XenBusDxe/GrantTable.c        |  4 ++--
>  OvmfPkg/XenBusDxe/Ia32/hypercall.nasm |  6 +++---
>  OvmfPkg/XenBusDxe/X64/hypercall.nasm  |  6 +++---
>  OvmfPkg/XenBusDxe/XenBusDxe.c         | 44 
> +++++++++++++++++++++++++++++++++++++++++++-
>  OvmfPkg/XenBusDxe/XenBusDxe.h         |  1 -
>  OvmfPkg/XenBusDxe/XenHypercall.c      | 61 
> +++++++++++++++++++++++++------------------------------------
>  OvmfPkg/XenBusDxe/XenHypercall.h      | 28 +++-------------------------
>  OvmfPkg/XenBusDxe/XenStore.c          |  4 ++--
>  9 files changed, 84 insertions(+), 81 deletions(-)
> 


> diff --git a/OvmfPkg/XenBusDxe/XenHypercall.c 
> b/OvmfPkg/XenBusDxe/XenHypercall.c
> index 34d92e76b7e3..19c34bdd0cec 100644
> --- a/OvmfPkg/XenBusDxe/XenHypercall.c
> +++ b/OvmfPkg/XenBusDxe/XenHypercall.c
> @@ -23,9 +23,21 @@
>  #include <IndustryStandard/Xen/hvm/params.h>
>  #include <IndustryStandard/Xen/memory.h>
>  
> +STATIC VOID       *HyperPage;
> +
> +//
> +// Interface exposed by the ASM implementation of the core hypercall
> +//
> +INTN
> +EFIAPI
> +__XenHypercall2 (
> +  IN     VOID *HypercallAddr,
> +  IN OUT INTN Arg1,
> +  IN OUT INTN Arg2
> +  );
> +
>  EFI_STATUS
>  XenHyperpageInit (
> -  IN OUT XENBUS_DEVICE *Dev
>    )
>  {
>    EFI_HOB_GUID_TYPE   *GuidHob;
> @@ -36,24 +48,21 @@ XenHyperpageInit (
>      return EFI_NOT_FOUND;
>    }
>    XenInfo = (EFI_XEN_INFO *) GET_GUID_HOB_DATA (GuidHob);
> -  Dev->Hyperpage = XenInfo->HyperPages;
> +  HyperPage = XenInfo->HyperPages;
>    return EFI_SUCCESS;
>  }
>  
>  UINT64
>  XenHypercallHvmGetParam (
> -  IN XENBUS_DEVICE *Dev,
>    IN UINT32        Index
>    )
>  {
>    xen_hvm_param_t     Parameter;
>    INTN                Error;
>  
> -  ASSERT (Dev->Hyperpage != NULL);
> -
>    Parameter.domid = DOMID_SELF;
>    Parameter.index = Index;
> -  Error = XenHypercall2 ((UINT8*)Dev->Hyperpage + __HYPERVISOR_hvm_op * 32,
> +  Error = XenHypercall2 (__HYPERVISOR_hvm_op,
>                           HVMOP_get_param, (INTN) &Parameter);
>    if (Error != 0) {
>      DEBUG ((EFI_D_ERROR,
> @@ -66,53 +75,33 @@ XenHypercallHvmGetParam (
>  
>  INTN
>  XenHypercallMemoryOp (
> -  IN     XENBUS_DEVICE *Dev,
>    IN     UINTN Operation,
>    IN OUT VOID *Arguments
>    )
>  {
> -  ASSERT (Dev->Hyperpage != NULL);
> -  return XenHypercall2 ((UINT8*)Dev->Hyperpage + __HYPERVISOR_memory_op * 32,
> +  return XenHypercall2 (__HYPERVISOR_memory_op,
>                          Operation, (INTN) Arguments);
>  }
>  
>  INTN
>  XenHypercallEventChannelOp (
> -  IN     XENBUS_DEVICE *Dev,
>    IN     INTN Operation,
>    IN OUT VOID *Arguments
>    )
>  {
> -  ASSERT (Dev->Hyperpage != NULL);
> -  return XenHypercall2 ((UINT8*)Dev->Hyperpage + 
> __HYPERVISOR_event_channel_op * 32,
> +  return XenHypercall2 (__HYPERVISOR_event_channel_op,
>                          Operation, (INTN) Arguments);
>  }
>  
> -EFI_STATUS
> -XenGetSharedInfoPage (
> -  IN OUT XENBUS_DEVICE *Dev
> +INTN
> +EFIAPI
> +XenHypercall2 (
> +  IN     INTN HypercallID,

This could be an unsigned since HypercallID should probably not be
negative.

Beside this comment,
Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

> +  IN OUT INTN Arg1,
> +  IN OUT INTN Arg2
>    )
>  {
> -  xen_add_to_physmap_t Parameter;
> +  ASSERT (HyperPage != NULL);
>  
> -  ASSERT (Dev->SharedInfo == NULL);
> -
> -  Parameter.domid = DOMID_SELF;
> -  Parameter.space = XENMAPSPACE_shared_info;
> -  Parameter.idx = 0;
> -
> -  //
> -  // using reserved page because the page is not released when Linux is
> -  // starting because of the add_to_physmap. QEMU might try to access the
> -  // page, and fail because it have no right to do so (segv).
> -  //
> -  Dev->SharedInfo = AllocateReservedPages (1);
> -  Parameter.gpfn = (UINTN) Dev->SharedInfo >> EFI_PAGE_SHIFT;
> -  if (XenHypercallMemoryOp (Dev, XENMEM_add_to_physmap, &Parameter) != 0) {
> -    FreePages (Dev->SharedInfo, 1);
> -    Dev->SharedInfo = NULL;
> -    return EFI_LOAD_ERROR;
> -  }
> -
> -  return EFI_SUCCESS;
> +  return __XenHypercall2 ((UINT8*)HyperPage + HypercallID * 32, Arg1, Arg2);
>  }

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel