|
[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 01/27/15 14:10, Ard Biesheuvel wrote:
> 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
>
I skimmed this patch. It makes sense to me as preparation for
librarizing the hypercall machinery (as you say in the commit message),
if the Xen guys don't have any objections.
I peeked forward at patch 18. The librarization is certainly possible
given that the origin of your info is the GUID HOB with gEfiXenInfoGuid.
So, I got curious about the data pointed-to by the gEfiXenInfoGuid
HOB... It's set up in OvmfPkg/PlatformPei/Xen.c. I froze for a second,
but then I noticed it uses BuildGuidDataHob(), *not* BuildGuidHob(); ie.
it *copies* mXenInfo into the HOB. Good.
I think this patch (and the next one) improve OVMF/Xen even in isolation
(== without thinking of ARM at all).
For v3:
Acked-by: Laszlo Ersek <lersek@xxxxxxxxxx>
Thanks
Laszlo
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |