diff mbox

[edk2,v2,17/29] Ovmf/Xen: refactor XenBusDxe hypercall implementation

Message ID 1422299011-2409-18-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel Jan. 26, 2015, 7:03 p.m. UTC
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
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 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      | 50 ++++++++++++++------------------------------------
 OvmfPkg/XenBusDxe/XenHypercall.h      | 28 +++-------------------------
 OvmfPkg/XenBusDxe/XenStore.c          |  4 ++--
 9 files changed, 73 insertions(+), 81 deletions(-)

Comments

Ard Biesheuvel Jan. 27, 2015, 12:46 p.m. UTC | #1
On 27 January 2015 at 12:43, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 26 Jan 2015, 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
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  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      | 50 ++++++++++++++------------------------------------
>>  OvmfPkg/XenBusDxe/XenHypercall.h      | 28 +++-------------------------
>>  OvmfPkg/XenBusDxe/XenStore.c          |  4 ++--
>>  9 files changed, 73 insertions(+), 81 deletions(-)
>>
>> diff --git a/OvmfPkg/XenBusDxe/EventChannel.c b/OvmfPkg/XenBusDxe/EventChannel.c
>> index 03efaf9cb904..a86323e6adfd 100644
>> --- a/OvmfPkg/XenBusDxe/EventChannel.c
>> +++ b/OvmfPkg/XenBusDxe/EventChannel.c
>> @@ -28,7 +28,7 @@ XenEventChannelNotify (
>>    evtchn_send_t Send;
>>
>>    Send.port = Port;
>> -  ReturnCode = XenHypercallEventChannelOp (Dev, EVTCHNOP_send, &Send);
>> +  ReturnCode = XenHypercallEventChannelOp (EVTCHNOP_send, &Send);
>>    return (UINT32)ReturnCode;
>>  }
>>
>> @@ -40,15 +40,12 @@ XenBusEventChannelAllocate (
>>    OUT evtchn_port_t   *Port
>>    )
>>  {
>> -  XENBUS_PRIVATE_DATA *Private;
>>    evtchn_alloc_unbound_t Parameter;
>>    UINT32 ReturnCode;
>>
>> -  Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
>> -
>>    Parameter.dom = DOMID_SELF;
>>    Parameter.remote_dom = DomainId;
>> -  ReturnCode = (UINT32)XenHypercallEventChannelOp (Private->Dev,
>> +  ReturnCode = (UINT32)XenHypercallEventChannelOp (
>>                                     EVTCHNOP_alloc_unbound,
>>                                     &Parameter);
>>    if (ReturnCode != 0) {
>> @@ -79,10 +76,8 @@ XenBusEventChannelClose (
>>    IN evtchn_port_t   Port
>>    )
>>  {
>> -  XENBUS_PRIVATE_DATA *Private;
>>    evtchn_close_t Close;
>>
>> -  Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
>>    Close.port = Port;
>> -  return (UINT32)XenHypercallEventChannelOp (Private->Dev, EVTCHNOP_close, &Close);
>> +  return (UINT32)XenHypercallEventChannelOp (EVTCHNOP_close, &Close);
>>  }
>> diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c
>> index 8405edc51bc4..53cb99f0e004 100644
>> --- a/OvmfPkg/XenBusDxe/GrantTable.c
>> +++ b/OvmfPkg/XenBusDxe/GrantTable.c
>> @@ -161,7 +161,7 @@ XenGrantTableInit (
>>      Parameters.idx = Index;
>>      Parameters.space = XENMAPSPACE_grant_table;
>>      Parameters.gpfn = (xen_pfn_t) ((UINTN) GrantTable >> EFI_PAGE_SHIFT) + Index;
>> -    ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_add_to_physmap, &Parameters);
>> +    ReturnCode = XenHypercallMemoryOp (XENMEM_add_to_physmap, &Parameters);
>>      if (ReturnCode != 0) {
>>        DEBUG ((EFI_D_ERROR, "Xen GrantTable, add_to_physmap hypercall error: %d\n", ReturnCode));
>>      }
>> @@ -184,7 +184,7 @@ XenGrantTableDeinit (
>>      Parameters.domid = DOMID_SELF;
>>      Parameters.gpfn = (xen_pfn_t) ((UINTN) GrantTable >> EFI_PAGE_SHIFT) + Index;
>>      DEBUG ((EFI_D_INFO, "Xen GrantTable, removing %X\n", Parameters.gpfn));
>> -    ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_remove_from_physmap, &Parameters);
>> +    ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Parameters);
>>      if (ReturnCode != 0) {
>>        DEBUG ((EFI_D_ERROR, "Xen GrantTable, remove_from_physmap hypercall error: %d\n", ReturnCode));
>>      }
>> diff --git a/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm b/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
>> index 8547c30b81ee..e0fa71bb5ba8 100644
>> --- a/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
>> +++ b/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
>> @@ -2,13 +2,13 @@ SECTION .text
>>
>>  ; INTN
>>  ; EFIAPI
>> -; XenHypercall2 (
>> +; __XenHypercall2 (
>>  ;   IN     VOID *HypercallAddr,
>>  ;   IN OUT INTN Arg1,
>>  ;   IN OUT INTN Arg2
>>  ;   );
>> -global ASM_PFX(XenHypercall2)
>> -ASM_PFX(XenHypercall2):
>> +global ASM_PFX(__XenHypercall2)
>> +ASM_PFX(__XenHypercall2):
>>    ; Save only ebx, ecx is supposed to be a scratch register and needs to be
>>    ; saved by the caller
>>    push ebx
>> diff --git a/OvmfPkg/XenBusDxe/X64/hypercall.nasm b/OvmfPkg/XenBusDxe/X64/hypercall.nasm
>> index 177f271ef094..5e6a0c05c5c4 100644
>> --- a/OvmfPkg/XenBusDxe/X64/hypercall.nasm
>> +++ b/OvmfPkg/XenBusDxe/X64/hypercall.nasm
>> @@ -3,13 +3,13 @@ SECTION .text
>>
>>  ; INTN
>>  ; EFIAPI
>> -; XenHypercall2 (
>> +; __XenHypercall2 (
>>  ;   IN     VOID *HypercallAddr,
>>  ;   IN OUT INTN Arg1,
>>  ;   IN OUT INTN Arg2
>>  ;   );
>> -global ASM_PFX(XenHypercall2)
>> -ASM_PFX(XenHypercall2):
>> +global ASM_PFX(__XenHypercall2)
>> +ASM_PFX(__XenHypercall2):
>>    push rdi
>>    push rsi
>>    ; Copy HypercallAddr to rax
>> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
>> index 7a7fd82d559d..d333b331b6db 100644
>> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
>> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
>> @@ -34,6 +34,8 @@
>>  #include "XenStore.h"
>>  #include "XenBus.h"
>>
>> +#include <IndustryStandard/Xen/hvm/params.h>
>> +#include <IndustryStandard/Xen/memory.h>
>>
>>  ///
>>  /// Driver Binding Protocol instance
>> @@ -52,6 +54,46 @@ STATIC EFI_LOCK       mMyDeviceLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_CALLBACK
>>  STATIC XENBUS_DEVICE *mMyDevice = NULL;
>>
>>  /**
>> +  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.
>> +**/
>> +STATIC
>> +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
>> +  // 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 (XENMEM_add_to_physmap, &Parameter) != 0) {
>> +    FreePages (Dev->SharedInfo, 1);
>> +    Dev->SharedInfo = NULL;
>> +    return EFI_LOAD_ERROR;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +/**
>>    Unloads an image.
>>
>>    @param  ImageHandle           Handle that identifies the image to be unloaded.
>> @@ -348,7 +390,7 @@ XenBusDxeDriverBindingStart (
>>    MmioAddr = BarDesc->AddrRangeMin;
>>    FreePool (BarDesc);
>>
>> -  Status = XenHyperpageInit (Dev);
>> +  Status = XenHyperpageInit ();
>>    if (EFI_ERROR (Status)) {
>>      DEBUG ((EFI_D_ERROR, "XenBus: Unable to retrieve the hyperpage.\n"));
>>      Status = EFI_UNSUPPORTED;
>> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
>> index 80253b7d1ca9..9b7219906a69 100644
>> --- a/OvmfPkg/XenBusDxe/XenBusDxe.h
>> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
>> @@ -91,7 +91,6 @@ struct _XENBUS_DEVICE {
>>    EFI_DEVICE_PATH_PROTOCOL      *DevicePath;
>>    LIST_ENTRY                    ChildList;
>>
>> -  VOID                          *Hyperpage;
>>    shared_info_t                 *SharedInfo;
>>  };
>>
>> diff --git a/OvmfPkg/XenBusDxe/XenHypercall.c b/OvmfPkg/XenBusDxe/XenHypercall.c
>> index 34d92e76b7e3..9bcf3197633e 100644
>> --- a/OvmfPkg/XenBusDxe/XenHypercall.c
>> +++ b/OvmfPkg/XenBusDxe/XenHypercall.c
>> @@ -23,9 +23,10 @@
>>  #include <IndustryStandard/Xen/hvm/params.h>
>>  #include <IndustryStandard/Xen/memory.h>
>>
>> +STATIC VOID       *Hyperpage;
>> +
>>  EFI_STATUS
>>  XenHyperpageInit (
>> -  IN OUT XENBUS_DEVICE *Dev
>>    )
>>  {
>>    EFI_HOB_GUID_TYPE   *GuidHob;
>> @@ -36,24 +37,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 +64,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,
>> +  IN OUT INTN Arg1,
>> +  IN OUT INTN Arg2
>>    )
>>  {
>> -  xen_add_to_physmap_t Parameter;
>> -
>> -  ASSERT (Dev->SharedInfo == NULL);
>> +  ASSERT (HyperPage != 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);
>                                         ^ shouldn't it be Hyperpage?
>

Yes, you are quite right. My build test on x86 should have spotted
this, so apparently I screwed that up in some way as well.

Cheers,
Ard.

>>  }
>> diff --git a/OvmfPkg/XenBusDxe/XenHypercall.h b/OvmfPkg/XenBusDxe/XenHypercall.h
>> index 06693830e16e..9d49e33eb5af 100644
>> --- a/OvmfPkg/XenBusDxe/XenHypercall.h
>> +++ b/OvmfPkg/XenBusDxe/XenHypercall.h
>> @@ -18,9 +18,9 @@
>>
>>  /**
>>    This function will put the two arguments in the right place (registers) and
>> -  call HypercallAddr, which correspond to an entry in the hypercall pages.
>> +  invoke the hypercall identified by HypercallID.
>>
>> -  @param HypercallAddr  A memory address where the hypercall to call is.
>> +  @param HypercallID    The symbolic ID of the hypercall to be invoked
>>    @param Arg1           First argument.
>>    @param Arg2           Second argument.
>>
>> @@ -29,7 +29,7 @@
>>  INTN
>>  EFIAPI
>>  XenHypercall2 (
>> -  IN     VOID *HypercallAddr,
>> +  IN     INTN HypercallID,
>>    IN OUT INTN Arg1,
>>    IN OUT INTN Arg2
>>    );
>> @@ -44,27 +44,23 @@ XenHypercall2 (
>>  **/
>>  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,
>>    UINT32 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.
>>
>> @@ -73,7 +69,6 @@ XenHypercallHvmGetParam (
>>  **/
>>  INTN
>>  XenHypercallMemoryOp (
>> -  IN     XENBUS_DEVICE *Dev,
>>    IN     UINTN Operation,
>>    IN OUT VOID *Arguments
>>    );
>> @@ -81,7 +76,6 @@ XenHypercallMemoryOp (
>>  /**
>>    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.
>>
>> @@ -90,24 +84,8 @@ XenHypercallMemoryOp (
>>  **/
>>  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
>> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
>> index 2df8f5348585..7ec1e634bc5c 100644
>> --- a/OvmfPkg/XenBusDxe/XenStore.c
>> +++ b/OvmfPkg/XenBusDxe/XenStore.c
>> @@ -1057,8 +1057,8 @@ XenStoreInit (
>>
>>    xs.Dev = Dev;
>>
>> -  xs.EventChannel = (evtchn_port_t)XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_EVTCHN);
>> -  XenStoreGpfn = (UINTN)XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_PFN);
>> +  xs.EventChannel = (evtchn_port_t)XenHypercallHvmGetParam (HVM_PARAM_STORE_EVTCHN);
>> +  XenStoreGpfn = (UINTN)XenHypercallHvmGetParam (HVM_PARAM_STORE_PFN);
>>    xs.XenStore = (VOID *) (XenStoreGpfn << EFI_PAGE_SHIFT);
>>    DEBUG ((EFI_D_INFO, "XenBusInit: XenBus rings @%p, event channel %x\n",
>>            xs.XenStore, xs.EventChannel));
>> --
>> 1.8.3.2
>>

------------------------------------------------------------------------------
Dive into the World of Parallel Programming. The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net/
Ard Biesheuvel Jan. 27, 2015, 1:10 p.m. UTC | #2
On 27 January 2015 at 12:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 27 January 2015 at 12:43, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
>> On Mon, 26 Jan 2015, 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
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  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      | 50 ++++++++++++++------------------------------------
>>>  OvmfPkg/XenBusDxe/XenHypercall.h      | 28 +++-------------------------
>>>  OvmfPkg/XenBusDxe/XenStore.c          |  4 ++--
>>>  9 files changed, 73 insertions(+), 81 deletions(-)
>>>
>>> diff --git a/OvmfPkg/XenBusDxe/EventChannel.c b/OvmfPkg/XenBusDxe/EventChannel.c
>>> index 03efaf9cb904..a86323e6adfd 100644
>>> --- a/OvmfPkg/XenBusDxe/EventChannel.c
>>> +++ b/OvmfPkg/XenBusDxe/EventChannel.c
>>> @@ -28,7 +28,7 @@ XenEventChannelNotify (
>>>    evtchn_send_t Send;
>>>
>>>    Send.port = Port;
>>> -  ReturnCode = XenHypercallEventChannelOp (Dev, EVTCHNOP_send, &Send);
>>> +  ReturnCode = XenHypercallEventChannelOp (EVTCHNOP_send, &Send);
>>>    return (UINT32)ReturnCode;
>>>  }
>>>
>>> @@ -40,15 +40,12 @@ XenBusEventChannelAllocate (
>>>    OUT evtchn_port_t   *Port
>>>    )
>>>  {
>>> -  XENBUS_PRIVATE_DATA *Private;
>>>    evtchn_alloc_unbound_t Parameter;
>>>    UINT32 ReturnCode;
>>>
>>> -  Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
>>> -
>>>    Parameter.dom = DOMID_SELF;
>>>    Parameter.remote_dom = DomainId;
>>> -  ReturnCode = (UINT32)XenHypercallEventChannelOp (Private->Dev,
>>> +  ReturnCode = (UINT32)XenHypercallEventChannelOp (
>>>                                     EVTCHNOP_alloc_unbound,
>>>                                     &Parameter);
>>>    if (ReturnCode != 0) {
>>> @@ -79,10 +76,8 @@ XenBusEventChannelClose (
>>>    IN evtchn_port_t   Port
>>>    )
>>>  {
>>> -  XENBUS_PRIVATE_DATA *Private;
>>>    evtchn_close_t Close;
>>>
>>> -  Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
>>>    Close.port = Port;
>>> -  return (UINT32)XenHypercallEventChannelOp (Private->Dev, EVTCHNOP_close, &Close);
>>> +  return (UINT32)XenHypercallEventChannelOp (EVTCHNOP_close, &Close);
>>>  }
>>> diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c
>>> index 8405edc51bc4..53cb99f0e004 100644
>>> --- a/OvmfPkg/XenBusDxe/GrantTable.c
>>> +++ b/OvmfPkg/XenBusDxe/GrantTable.c
>>> @@ -161,7 +161,7 @@ XenGrantTableInit (
>>>      Parameters.idx = Index;
>>>      Parameters.space = XENMAPSPACE_grant_table;
>>>      Parameters.gpfn = (xen_pfn_t) ((UINTN) GrantTable >> EFI_PAGE_SHIFT) + Index;
>>> -    ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_add_to_physmap, &Parameters);
>>> +    ReturnCode = XenHypercallMemoryOp (XENMEM_add_to_physmap, &Parameters);
>>>      if (ReturnCode != 0) {
>>>        DEBUG ((EFI_D_ERROR, "Xen GrantTable, add_to_physmap hypercall error: %d\n", ReturnCode));
>>>      }
>>> @@ -184,7 +184,7 @@ XenGrantTableDeinit (
>>>      Parameters.domid = DOMID_SELF;
>>>      Parameters.gpfn = (xen_pfn_t) ((UINTN) GrantTable >> EFI_PAGE_SHIFT) + Index;
>>>      DEBUG ((EFI_D_INFO, "Xen GrantTable, removing %X\n", Parameters.gpfn));
>>> -    ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_remove_from_physmap, &Parameters);
>>> +    ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Parameters);
>>>      if (ReturnCode != 0) {
>>>        DEBUG ((EFI_D_ERROR, "Xen GrantTable, remove_from_physmap hypercall error: %d\n", ReturnCode));
>>>      }
>>> diff --git a/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm b/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
>>> index 8547c30b81ee..e0fa71bb5ba8 100644
>>> --- a/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
>>> +++ b/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
>>> @@ -2,13 +2,13 @@ SECTION .text
>>>
>>>  ; INTN
>>>  ; EFIAPI
>>> -; XenHypercall2 (
>>> +; __XenHypercall2 (
>>>  ;   IN     VOID *HypercallAddr,
>>>  ;   IN OUT INTN Arg1,
>>>  ;   IN OUT INTN Arg2
>>>  ;   );
>>> -global ASM_PFX(XenHypercall2)
>>> -ASM_PFX(XenHypercall2):
>>> +global ASM_PFX(__XenHypercall2)
>>> +ASM_PFX(__XenHypercall2):
>>>    ; Save only ebx, ecx is supposed to be a scratch register and needs to be
>>>    ; saved by the caller
>>>    push ebx
>>> diff --git a/OvmfPkg/XenBusDxe/X64/hypercall.nasm b/OvmfPkg/XenBusDxe/X64/hypercall.nasm
>>> index 177f271ef094..5e6a0c05c5c4 100644
>>> --- a/OvmfPkg/XenBusDxe/X64/hypercall.nasm
>>> +++ b/OvmfPkg/XenBusDxe/X64/hypercall.nasm
>>> @@ -3,13 +3,13 @@ SECTION .text
>>>
>>>  ; INTN
>>>  ; EFIAPI
>>> -; XenHypercall2 (
>>> +; __XenHypercall2 (
>>>  ;   IN     VOID *HypercallAddr,
>>>  ;   IN OUT INTN Arg1,
>>>  ;   IN OUT INTN Arg2
>>>  ;   );
>>> -global ASM_PFX(XenHypercall2)
>>> -ASM_PFX(XenHypercall2):
>>> +global ASM_PFX(__XenHypercall2)
>>> +ASM_PFX(__XenHypercall2):
>>>    push rdi
>>>    push rsi
>>>    ; Copy HypercallAddr to rax
>>> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
>>> index 7a7fd82d559d..d333b331b6db 100644
>>> --- a/OvmfPkg/XenBusDxe/XenBusDxe.c
>>> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
>>> @@ -34,6 +34,8 @@
>>>  #include "XenStore.h"
>>>  #include "XenBus.h"
>>>
>>> +#include <IndustryStandard/Xen/hvm/params.h>
>>> +#include <IndustryStandard/Xen/memory.h>
>>>
>>>  ///
>>>  /// Driver Binding Protocol instance
>>> @@ -52,6 +54,46 @@ STATIC EFI_LOCK       mMyDeviceLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_CALLBACK
>>>  STATIC XENBUS_DEVICE *mMyDevice = NULL;
>>>
>>>  /**
>>> +  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.
>>> +**/
>>> +STATIC
>>> +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
>>> +  // 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 (XENMEM_add_to_physmap, &Parameter) != 0) {
>>> +    FreePages (Dev->SharedInfo, 1);
>>> +    Dev->SharedInfo = NULL;
>>> +    return EFI_LOAD_ERROR;
>>> +  }
>>> +
>>> +  return EFI_SUCCESS;
>>> +}
>>> +
>>> +/**
>>>    Unloads an image.
>>>
>>>    @param  ImageHandle           Handle that identifies the image to be unloaded.
>>> @@ -348,7 +390,7 @@ XenBusDxeDriverBindingStart (
>>>    MmioAddr = BarDesc->AddrRangeMin;
>>>    FreePool (BarDesc);
>>>
>>> -  Status = XenHyperpageInit (Dev);
>>> +  Status = XenHyperpageInit ();
>>>    if (EFI_ERROR (Status)) {
>>>      DEBUG ((EFI_D_ERROR, "XenBus: Unable to retrieve the hyperpage.\n"));
>>>      Status = EFI_UNSUPPORTED;
>>> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
>>> index 80253b7d1ca9..9b7219906a69 100644
>>> --- a/OvmfPkg/XenBusDxe/XenBusDxe.h
>>> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
>>> @@ -91,7 +91,6 @@ struct _XENBUS_DEVICE {
>>>    EFI_DEVICE_PATH_PROTOCOL      *DevicePath;
>>>    LIST_ENTRY                    ChildList;
>>>
>>> -  VOID                          *Hyperpage;
>>>    shared_info_t                 *SharedInfo;
>>>  };
>>>
>>> diff --git a/OvmfPkg/XenBusDxe/XenHypercall.c b/OvmfPkg/XenBusDxe/XenHypercall.c
>>> index 34d92e76b7e3..9bcf3197633e 100644
>>> --- a/OvmfPkg/XenBusDxe/XenHypercall.c
>>> +++ b/OvmfPkg/XenBusDxe/XenHypercall.c
>>> @@ -23,9 +23,10 @@
>>>  #include <IndustryStandard/Xen/hvm/params.h>
>>>  #include <IndustryStandard/Xen/memory.h>
>>>
>>> +STATIC VOID       *Hyperpage;
>>> +
>>>  EFI_STATUS
>>>  XenHyperpageInit (
>>> -  IN OUT XENBUS_DEVICE *Dev
>>>    )
>>>  {
>>>    EFI_HOB_GUID_TYPE   *GuidHob;
>>> @@ -36,24 +37,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 +64,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,
>>> +  IN OUT INTN Arg1,
>>> +  IN OUT INTN Arg2
>>>    )
>>>  {
>>> -  xen_add_to_physmap_t Parameter;
>>> -
>>> -  ASSERT (Dev->SharedInfo == NULL);
>>> +  ASSERT (HyperPage != 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);
>>                                         ^ shouldn't it be Hyperpage?
>>
>
> Yes, you are quite right. My build test on x86 should have spotted
> this, so apparently I screwed that up in some way as well.
>

Turns out this was a refactoring error that got cleaned up by the next
patch, and I did not perform the x86 build test on each patch in
isolation.
Will be fixed in v3
diff mbox

Patch

diff --git a/OvmfPkg/XenBusDxe/EventChannel.c b/OvmfPkg/XenBusDxe/EventChannel.c
index 03efaf9cb904..a86323e6adfd 100644
--- a/OvmfPkg/XenBusDxe/EventChannel.c
+++ b/OvmfPkg/XenBusDxe/EventChannel.c
@@ -28,7 +28,7 @@  XenEventChannelNotify (
   evtchn_send_t Send;
 
   Send.port = Port;
-  ReturnCode = XenHypercallEventChannelOp (Dev, EVTCHNOP_send, &Send);
+  ReturnCode = XenHypercallEventChannelOp (EVTCHNOP_send, &Send);
   return (UINT32)ReturnCode;
 }
 
@@ -40,15 +40,12 @@  XenBusEventChannelAllocate (
   OUT evtchn_port_t   *Port
   )
 {
-  XENBUS_PRIVATE_DATA *Private;
   evtchn_alloc_unbound_t Parameter;
   UINT32 ReturnCode;
 
-  Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
-
   Parameter.dom = DOMID_SELF;
   Parameter.remote_dom = DomainId;
-  ReturnCode = (UINT32)XenHypercallEventChannelOp (Private->Dev,
+  ReturnCode = (UINT32)XenHypercallEventChannelOp (
                                    EVTCHNOP_alloc_unbound,
                                    &Parameter);
   if (ReturnCode != 0) {
@@ -79,10 +76,8 @@  XenBusEventChannelClose (
   IN evtchn_port_t   Port
   )
 {
-  XENBUS_PRIVATE_DATA *Private;
   evtchn_close_t Close;
 
-  Private = XENBUS_PRIVATE_DATA_FROM_THIS (This);
   Close.port = Port;
-  return (UINT32)XenHypercallEventChannelOp (Private->Dev, EVTCHNOP_close, &Close);
+  return (UINT32)XenHypercallEventChannelOp (EVTCHNOP_close, &Close);
 }
diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c
index 8405edc51bc4..53cb99f0e004 100644
--- a/OvmfPkg/XenBusDxe/GrantTable.c
+++ b/OvmfPkg/XenBusDxe/GrantTable.c
@@ -161,7 +161,7 @@  XenGrantTableInit (
     Parameters.idx = Index;
     Parameters.space = XENMAPSPACE_grant_table;
     Parameters.gpfn = (xen_pfn_t) ((UINTN) GrantTable >> EFI_PAGE_SHIFT) + Index;
-    ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_add_to_physmap, &Parameters);
+    ReturnCode = XenHypercallMemoryOp (XENMEM_add_to_physmap, &Parameters);
     if (ReturnCode != 0) {
       DEBUG ((EFI_D_ERROR, "Xen GrantTable, add_to_physmap hypercall error: %d\n", ReturnCode));
     }
@@ -184,7 +184,7 @@  XenGrantTableDeinit (
     Parameters.domid = DOMID_SELF;
     Parameters.gpfn = (xen_pfn_t) ((UINTN) GrantTable >> EFI_PAGE_SHIFT) + Index;
     DEBUG ((EFI_D_INFO, "Xen GrantTable, removing %X\n", Parameters.gpfn));
-    ReturnCode = XenHypercallMemoryOp (Dev, XENMEM_remove_from_physmap, &Parameters);
+    ReturnCode = XenHypercallMemoryOp (XENMEM_remove_from_physmap, &Parameters);
     if (ReturnCode != 0) {
       DEBUG ((EFI_D_ERROR, "Xen GrantTable, remove_from_physmap hypercall error: %d\n", ReturnCode));
     }
diff --git a/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm b/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
index 8547c30b81ee..e0fa71bb5ba8 100644
--- a/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
+++ b/OvmfPkg/XenBusDxe/Ia32/hypercall.nasm
@@ -2,13 +2,13 @@  SECTION .text
 
 ; INTN
 ; EFIAPI
-; XenHypercall2 (
+; __XenHypercall2 (
 ;   IN     VOID *HypercallAddr,
 ;   IN OUT INTN Arg1,
 ;   IN OUT INTN Arg2
 ;   );
-global ASM_PFX(XenHypercall2)
-ASM_PFX(XenHypercall2):
+global ASM_PFX(__XenHypercall2)
+ASM_PFX(__XenHypercall2):
   ; Save only ebx, ecx is supposed to be a scratch register and needs to be
   ; saved by the caller
   push ebx
diff --git a/OvmfPkg/XenBusDxe/X64/hypercall.nasm b/OvmfPkg/XenBusDxe/X64/hypercall.nasm
index 177f271ef094..5e6a0c05c5c4 100644
--- a/OvmfPkg/XenBusDxe/X64/hypercall.nasm
+++ b/OvmfPkg/XenBusDxe/X64/hypercall.nasm
@@ -3,13 +3,13 @@  SECTION .text
 
 ; INTN
 ; EFIAPI
-; XenHypercall2 (
+; __XenHypercall2 (
 ;   IN     VOID *HypercallAddr,
 ;   IN OUT INTN Arg1,
 ;   IN OUT INTN Arg2
 ;   );
-global ASM_PFX(XenHypercall2)
-ASM_PFX(XenHypercall2):
+global ASM_PFX(__XenHypercall2)
+ASM_PFX(__XenHypercall2):
   push rdi
   push rsi
   ; Copy HypercallAddr to rax
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c
index 7a7fd82d559d..d333b331b6db 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.c
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.c
@@ -34,6 +34,8 @@ 
 #include "XenStore.h"
 #include "XenBus.h"
 
+#include <IndustryStandard/Xen/hvm/params.h>
+#include <IndustryStandard/Xen/memory.h>
 
 ///
 /// Driver Binding Protocol instance
@@ -52,6 +54,46 @@  STATIC EFI_LOCK       mMyDeviceLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_CALLBACK
 STATIC XENBUS_DEVICE *mMyDevice = NULL;
 
 /**
+  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.
+**/
+STATIC
+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
+  // 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 (XENMEM_add_to_physmap, &Parameter) != 0) {
+    FreePages (Dev->SharedInfo, 1);
+    Dev->SharedInfo = NULL;
+    return EFI_LOAD_ERROR;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
   Unloads an image.
 
   @param  ImageHandle           Handle that identifies the image to be unloaded.
@@ -348,7 +390,7 @@  XenBusDxeDriverBindingStart (
   MmioAddr = BarDesc->AddrRangeMin;
   FreePool (BarDesc);
 
-  Status = XenHyperpageInit (Dev);
+  Status = XenHyperpageInit ();
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "XenBus: Unable to retrieve the hyperpage.\n"));
     Status = EFI_UNSUPPORTED;
diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h
index 80253b7d1ca9..9b7219906a69 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.h
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.h
@@ -91,7 +91,6 @@  struct _XENBUS_DEVICE {
   EFI_DEVICE_PATH_PROTOCOL      *DevicePath;
   LIST_ENTRY                    ChildList;
 
-  VOID                          *Hyperpage;
   shared_info_t                 *SharedInfo;
 };
 
diff --git a/OvmfPkg/XenBusDxe/XenHypercall.c b/OvmfPkg/XenBusDxe/XenHypercall.c
index 34d92e76b7e3..9bcf3197633e 100644
--- a/OvmfPkg/XenBusDxe/XenHypercall.c
+++ b/OvmfPkg/XenBusDxe/XenHypercall.c
@@ -23,9 +23,10 @@ 
 #include <IndustryStandard/Xen/hvm/params.h>
 #include <IndustryStandard/Xen/memory.h>
 
+STATIC VOID       *Hyperpage;
+
 EFI_STATUS
 XenHyperpageInit (
-  IN OUT XENBUS_DEVICE *Dev
   )
 {
   EFI_HOB_GUID_TYPE   *GuidHob;
@@ -36,24 +37,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 +64,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,
+  IN OUT INTN Arg1,
+  IN OUT INTN Arg2
   )
 {
-  xen_add_to_physmap_t Parameter;
-
-  ASSERT (Dev->SharedInfo == NULL);
+  ASSERT (HyperPage != 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);
 }
diff --git a/OvmfPkg/XenBusDxe/XenHypercall.h b/OvmfPkg/XenBusDxe/XenHypercall.h
index 06693830e16e..9d49e33eb5af 100644
--- a/OvmfPkg/XenBusDxe/XenHypercall.h
+++ b/OvmfPkg/XenBusDxe/XenHypercall.h
@@ -18,9 +18,9 @@ 
 
 /**
   This function will put the two arguments in the right place (registers) and
-  call HypercallAddr, which correspond to an entry in the hypercall pages.
+  invoke the hypercall identified by HypercallID.
 
-  @param HypercallAddr  A memory address where the hypercall to call is.
+  @param HypercallID    The symbolic ID of the hypercall to be invoked
   @param Arg1           First argument.
   @param Arg2           Second argument.
 
@@ -29,7 +29,7 @@ 
 INTN
 EFIAPI
 XenHypercall2 (
-  IN     VOID *HypercallAddr,
+  IN     INTN HypercallID,
   IN OUT INTN Arg1,
   IN OUT INTN Arg2
   );
@@ -44,27 +44,23 @@  XenHypercall2 (
 **/
 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,
   UINT32 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.
 
@@ -73,7 +69,6 @@  XenHypercallHvmGetParam (
 **/
 INTN
 XenHypercallMemoryOp (
-  IN     XENBUS_DEVICE *Dev,
   IN     UINTN Operation,
   IN OUT VOID *Arguments
   );
@@ -81,7 +76,6 @@  XenHypercallMemoryOp (
 /**
   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.
 
@@ -90,24 +84,8 @@  XenHypercallMemoryOp (
 **/
 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
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index 2df8f5348585..7ec1e634bc5c 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -1057,8 +1057,8 @@  XenStoreInit (
 
   xs.Dev = Dev;
 
-  xs.EventChannel = (evtchn_port_t)XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_EVTCHN);
-  XenStoreGpfn = (UINTN)XenHypercallHvmGetParam (Dev, HVM_PARAM_STORE_PFN);
+  xs.EventChannel = (evtchn_port_t)XenHypercallHvmGetParam (HVM_PARAM_STORE_EVTCHN);
+  XenStoreGpfn = (UINTN)XenHypercallHvmGetParam (HVM_PARAM_STORE_PFN);
   xs.XenStore = (VOID *) (XenStoreGpfn << EFI_PAGE_SHIFT);
   DEBUG ((EFI_D_INFO, "XenBusInit: XenBus rings @%p, event channel %x\n",
           xs.XenStore, xs.EventChannel));