Re: [PATCH v2 04/18] OvmfPkg/XenBusDxe: Add support to make Xen Hypercalls.

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

On Thu, Sep 04, 2014 at 05:50:59PM +0100, Anthony PERARD wrote:
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

One comment below, otherwise

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> Change in V2:
> - file header, copyright
> - Add License
> - Add push/pop instruction.
> - fix types
> - Comment of exported functions
> - Improve coding style
> - Add error handling in the main init function (of the drivers)
> - Comment assembly
> ---
>  OvmfPkg/XenBusDxe/X64/hypercall.S   |  23 +++++++
>  OvmfPkg/XenBusDxe/X64/hypercall.asm |  27 ++++++++
>  OvmfPkg/XenBusDxe/XenBusDxe.c       |  22 ++++++
>  OvmfPkg/XenBusDxe/XenBusDxe.h       |   5 ++
>  OvmfPkg/XenBusDxe/XenBusDxe.inf     |   6 ++
>  OvmfPkg/XenBusDxe/XenHypercall.c    | 134 ++++++++++++++++++++++++++++++++++++
>  OvmfPkg/XenBusDxe/XenHypercall.h    | 100 +++++++++++++++++++++++++++
>  7 files changed, 317 insertions(+)
>  create mode 100644 OvmfPkg/XenBusDxe/X64/hypercall.S
>  create mode 100644 OvmfPkg/XenBusDxe/X64/hypercall.asm
>  create mode 100644 OvmfPkg/XenBusDxe/XenHypercall.c
>  create mode 100644 OvmfPkg/XenBusDxe/XenHypercall.h
> 
> diff --git a/OvmfPkg/XenBusDxe/X64/hypercall.S b/OvmfPkg/XenBusDxe/X64/hypercall.S
> new file mode 100644
> index 0000000..3b18e48
> --- /dev/null
> +++ b/OvmfPkg/XenBusDxe/X64/hypercall.S
> @@ -0,0 +1,23 @@
> +# Hypercall with 2 arguments
> +# INTN
> +# EFIAPI
> +# XenHypercall2 (
> +#   IN     VOID *HypercallAddr,
> +#   IN OUT INTN Arg1,
> +#   IN OUT INTN Arg2
> +#   );
> +ASM_GLOBAL ASM_PFX(XenHypercall2)
> +ASM_PFX(XenHypercall2):
> +  push %rdi
> +  push %rsi
> +  # Copy HypercallAddr to rax
> +  movq %rcx, %rax
> +  # Copy Arg1 to the register expected by Xen
> +  movq %rdx, %rdi
> +  # Copy Arg2 to the register expected by Xen
> +  movq %r8, %rsi
> +  # Call HypercallAddr
> +  call *%rax
> +  pop %rsi
> +  pop %rdi
> +  ret
> diff --git a/OvmfPkg/XenBusDxe/X64/hypercall.asm b/OvmfPkg/XenBusDxe/X64/hypercall.asm
> new file mode 100644
> index 0000000..a39f240
> --- /dev/null
> +++ b/OvmfPkg/XenBusDxe/X64/hypercall.asm
> @@ -0,0 +1,27 @@
> +.code
> +
> +; Hypercall with 2 arguments
> +; INTN
> +; EFIAPI
> +; XenHypercall2 (
> +;   IN     VOID *HypercallAddr,
> +;   IN OUT INTN Arg1,
> +;   IN OUT INTN Arg2
> +;   );
> +XenHypercall2 PROC
> +  push rdi
> +  push rsi
> +  ; Copy HypercallAddr to rax
> +  mov rax, rcx
> +  ; Copy Arg1 to the register expected by Xen
> +  mov rdi, rdx
> +  ; Copy Arg2 to the register expected by Xen
> +  mov rsi, r8
> +  ; Call HypercallAddr
> +  call rax
> +  pop rsi
> +  pop rdi
> +  ret
> +XenHypercall2 ENDP
> +
> +END
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
> index e125ed7..db88cb4 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
> @@ -45,6 +45,8 @@
>  
>  #include "XenBusDxe.h"
>  
> +#include "XenHypercall.h"
> +
>  
>  ///
>  /// Driver Binding Protocol instance
> @@ -279,6 +281,8 @@ NotifyExitBoot (
>    @retval EFI_SUCCESS              The device was started.
>    @retval EFI_DEVICE_ERROR         The device could not be started due to a device error.Currently not implemented.
>    @retval EFI_OUT_OF_RESOURCES     The request could not be completed due to a lack of resources.
> +  @retval EFI_UNSUPPORTED          Something is missing on the system that
> +                                   prevent to start the edvice.
>    @retval Others                   The driver failded to start the device.
>  
>  **/
> @@ -298,6 +302,20 @@ XenBusDxeDriverBindingStart (
>    Dev->This = This;
>    Dev->ControllerHandle = ControllerHandle;
>  
> +  Status = XenHyperpageInit (Dev);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "XenBus: Unable to retrieve the hyperpage."));
> +    Status = EFI_UNSUPPORTED;
> +    goto ErrorNoHyperpage;
> +  }
> +
> +  Status = XenGetSharedInfoPage (Dev);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "XenBus: Unable to get the shared info page."));
> +    Status = EFI_UNSUPPORTED;
> +    goto ErrorNoHyperpage;
> +  }
> +
>    Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
>                               NotifyExitBoot,
>                               (VOID*) Dev,
> @@ -306,6 +324,10 @@ XenBusDxeDriverBindingStart (
>  
>    mMyDevice = Dev;
>    return EFI_SUCCESS;
> +
> +ErrorNoHyperpage:
> +  FreePool (Dev);
> +  return Status;
>  }
>  
>  /**
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
> index 9e4860a..af0510a 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.h
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
> @@ -86,6 +86,8 @@ extern EFI_COMPONENT_NAME_PROTOCOL  gXenBusDxeComponentName;
>  //
>  // Other stuff
>  //
> +#include <IndustryStandard/Xen/xen.h>
> +
>  #define PCI_VENDOR_ID_XEN                0x5853
>  #define PCI_DEVICE_ID_XEN_PLATFORM       0x0001
>  
> @@ -99,6 +101,9 @@ struct _XENBUS_DEVICE {
>    EFI_DRIVER_BINDING_PROTOCOL   *This;
>    EFI_HANDLE                    ControllerHandle;
>    EFI_EVENT                     ExitBootEvent;
> +
> +  VOID                          *Hyperpage;
> +  shared_info_t                 *SharedInfo;
>  };
>  
>  #endif
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> index b4d7551..edc3e51 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> @@ -31,6 +31,12 @@
>    DriverBinding.h
>    ComponentName.c
>    ComponentName.h
> +  XenHypercall.c
> +  XenHypercall.h
> +
> +[Sources.X64]
> +  X64/hypercall.S
> +  X64/hypercall.asm
>  
>  [LibraryClasses]
>    UefiDriverEntryPoint
> diff --git a/OvmfPkg/XenBusDxe/XenHypercall.c b/OvmfPkg/XenBusDxe/XenHypercall.c
> new file mode 100644
> index 0000000..2e1f361
> --- /dev/null
> +++ b/OvmfPkg/XenBusDxe/XenHypercall.c
> @@ -0,0 +1,134 @@
> +/** @file
> +  Functions to make Xen hypercalls.
> +
> +  Copyright (C) 2014, Citrix Ltd.
> +
> +  Redistribution and use in source and binary forms, with or without
> +  modification, are permitted provided that the following conditions
> +  are met:
> +
> +  * Redistributions of source code must retain the above copyright
> +    notice, this list of conditions and the following disclaimer.
> +  * Redistributions in binary form must reproduce the above copyright
> +    notice, this list of conditions and the following disclaimer in
> +    the documentation and/or other materials provided with the
> +    distribution.
> +
> +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> +  FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> +  COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> +  INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> +  BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +  LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
> +  CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> +  LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
> +  ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> +  POSSIBILITY OF SUCH DAMAGE.
> +
> +**/
> +
> +#include <PiDxe.h>
> +#include <Library/HobLib.h>
> +#include <Guid/XenInfo.h>
> +
> +#include "XenBusDxe.h"
> +#include "XenHypercall.h"
> +
> +#include <IndustryStandard/Xen/hvm/params.h>
> +#include <IndustryStandard/Xen/memory.h>
> +
> +EFI_STATUS
> +XenHyperpageInit (
> +  IN OUT XENBUS_DEVICE *Dev
> +  )
> +{
> +  EFI_HOB_GUID_TYPE   *GuidHob;
> +  EFI_XEN_INFO        *XenInfo;
> +
> +  GuidHob = GetFirstGuidHob (&gEfiXenInfoGuid);
> +  if (GuidHob == NULL) {
> +    return EFI_NOT_FOUND;
> +  }
> +  XenInfo = (EFI_XEN_INFO *) GET_GUID_HOB_DATA (GuidHob);
> +  Dev->Hyperpage = XenInfo->HyperPages;
> +  return EFI_SUCCESS;
> +}
> +
> +UINT64
> +XenHypercallHvmGetParam (
> +  IN XENBUS_DEVICE *Dev,
> +  IN INTN          Index
> +  )
> +{
> +  xen_hvm_param_t     Parameter;
> +  INTN                Error;
> +
> +  ASSERT (Dev->Hyperpage != NULL);
> +
> +  Parameter.domid = DOMID_SELF;
> +  Parameter.index = Index;
> +  Error = XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_hvm_op * 32,
> +                         HVMOP_get_param, (INTN) &Parameter);
> +  if (Error != 0) {
> +    DEBUG ((EFI_D_ERROR,
> +            "XenHypercall: Error %d trying to get HVM parameter %d\n",
> +            Error, Index));
> +    return 0;
> +  }
> +  return Parameter.value;
> +}
> +
> +INTN
> +XenHypercallMemoryOp (
> +  IN     XENBUS_DEVICE *Dev,
> +  IN     UINTN Operation,
> +  IN OUT VOID *Arguments
> +  )
> +{
> +  ASSERT (Dev->Hyperpage != NULL);
> +  return XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_memory_op * 32,
> +                        Operation, (INTN) Arguments);
> +}
> +
> +INTN
> +XenHypercallEventChannelOp (
> +  IN     XENBUS_DEVICE *Dev,
> +  IN     INTN Operation,
> +  IN OUT VOID *Arguments
> +  )
> +{
> +  ASSERT (Dev->Hyperpage != NULL);
> +  return XenHypercall2 (Dev->Hyperpage + __HYPERVISOR_event_channel_op * 32,
> +                        Operation, (INTN) Arguments);
> +}
> +
> +EFI_STATUS
> +XenGetSharedInfoPage (
> +  IN OUT XENBUS_DEVICE *Dev
> +  )
> +{
> +  xen_add_to_physmap_t Parameter;
> +
> +  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

s/linux/Linux

> +  // 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;
> +}
> diff --git a/OvmfPkg/XenBusDxe/XenHypercall.h b/OvmfPkg/XenBusDxe/XenHypercall.h
> new file mode 100644
> index 0000000..29eaf9f
> --- /dev/null
> +++ b/OvmfPkg/XenBusDxe/XenHypercall.h
> @@ -0,0 +1,100 @@
> +#ifndef __XENBUS_DXE_HYPERCALL_H__
> +#define __XENBUS_DXE_HYPERCALL_H__
> +
> +typedef struct _XENBUS_DEVICE XENBUS_DEVICE;
> +
> +/**
> +  This function will put the two arguments in the right place (registers) and
> +  call HypercallAddr, which correspond to an entry in the hypercall pages.
> +
> +  @param HypercallAddr  A memory address where the hypercall to call is.
> +  @param Arg1           First argument.
> +  @param Arg2           Second argument.
> +
> +  @return   Return 0 if success otherwise it return an errno.
> +**/
> +INTN
> +EFIAPI
> +XenHypercall2 (
> +  IN     VOID *HypercallAddr,
> +  IN OUT INTN Arg1,
> +  IN OUT INTN Arg2
> +  );
> +
> +/**
> +  Get the page where all hypercall are from the XenInfo hob.
> +
> +  @param Dev    A XENBUS_DEVICE instance.
> +
> +  @retval EFI_NOT_FOUND   hyperpage could not be found.
> +  @retval EFI_SUCCESS     Successfully retrieve the hyperpage pointer.
> +**/
> +EFI_STATUS
> +XenHyperpageInit (
> +  XENBUS_DEVICE *Dev
> +  );
> +
> +/**
> +  Return the value of the HVM parameter Index.
> +
> +  @param Dev    A XENBUS_DEVICE instance.
> +  @param Index  The parameter to get, e.g. HVM_PARAM_STORE_EVTCHN.
> +
> +  @return   The value of the asked parameter or 0 in case of error.
> +**/
> +UINT64
> +XenHypercallHvmGetParam (
> +  XENBUS_DEVICE *Dev,
> +  INTN Index
> +  );
> +
> +/**
> +  Hypercall to do different operation on the memory.
> +
> +  @param Dev        A XENBUS_DEVICE instance.
> +  @param Operation  The operation number, e.g. XENMEM_add_to_physmap.
> +  @param Arguments  The arguments associated to the operation.
> +
> +  @return  Return the return value from the hypercall, 0 in case of success
> +           otherwise, an error code.
> +**/
> +INTN
> +XenHypercallMemoryOp (
> +  IN     XENBUS_DEVICE *Dev,
> +  IN     UINTN Operation,
> +  IN OUT VOID *Arguments
> +  );
> +
> +/**
> +  Do an operation on the event channels.
> +
> +  @param Dev        A XENBUS_DEVICE instance.
> +  @param Operation  The operation number, e.g. EVTCHNOP_send.
> +  @param Arguments  The argument associated to the operation.
> +
> +  @return  Return the return value from the hypercall, 0 in case of success
> +           otherwise, an error code.
> +**/
> +INTN
> +XenHypercallEventChannelOp (
> +  IN     XENBUS_DEVICE *Dev,
> +  IN     INTN Operation,
> +  IN OUT VOID *Arguments
> +  );
> +
> +/**
> +  Map the shared_info_t page into memory.
> +
> +  @param Dev    A XENBUS_DEVICE instance.
> +
> +  @retval EFI_SUCCESS     Dev->SharedInfo whill contain a pointer to
> +                          the shared info page
> +  @retval EFI_LOAD_ERROR  The shared info page could not be mapped. The
> +                          hypercall returned an error.
> +**/
> +EFI_STATUS
> +XenGetSharedInfoPage (
> +  IN OUT XENBUS_DEVICE *Dev
> +  );
> +
> +#endif
> -- 
> Anthony PERARD
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel