|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 17/29] Ovmf/Xen: refactor XenBusDxe hypercall implementation
On 27 January 2015 at 12:43, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> 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@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 | 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
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |