|
[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:46, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
> 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.
>
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
--
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 |