From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony PERARD Subject: Re: [PATCH v4 17/29] Ovmf/Xen: refactor XenBusDxe hypercall implementation Date: Thu, 19 Feb 2015 16:05:09 +0000 Message-ID: <20150219160509.GP1345__1420.38254202834$1424362232$gmane$org@perard.uk.xensource.com> References: <1423739961-5945-1-git-send-email-ard.biesheuvel@linaro.org> <1423739961-5945-18-git-send-email-ard.biesheuvel@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1423739961-5945-18-git-send-email-ard.biesheuvel@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ard Biesheuvel Cc: feng.tian@intel.com, julien.grall@linaro.org, ian.campbell@citrix.com, olivier.martin@arm.com, stefano.stabellini@eu.citrix.com, jordan.l.justen@intel.com, edk2-devel@lists.sourceforge.net, leif.lindholm@linaro.org, xen-devel@lists.xen.org, roy.franz@linaro.org, michael.d.kinney@intel.com, lersek@redhat.com List-Id: xen-devel@lists.xenproject.org On Thu, Feb 12, 2015 at 07:19:09PM +0800, 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 > Acked-by: Laszlo Ersek > Signed-off-by: Ard Biesheuvel > --- > 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 | 61 +++++++++++++++++++++++++------------------------------------ > OvmfPkg/XenBusDxe/XenHypercall.h | 28 +++------------------------- > OvmfPkg/XenBusDxe/XenStore.c | 4 ++-- > 9 files changed, 84 insertions(+), 81 deletions(-) > > diff --git a/OvmfPkg/XenBusDxe/XenHypercall.c b/OvmfPkg/XenBusDxe/XenHypercall.c > index 34d92e76b7e3..19c34bdd0cec 100644 > --- a/OvmfPkg/XenBusDxe/XenHypercall.c > +++ b/OvmfPkg/XenBusDxe/XenHypercall.c > @@ -23,9 +23,21 @@ > #include > #include > > +STATIC VOID *HyperPage; > + > +// > +// Interface exposed by the ASM implementation of the core hypercall > +// > +INTN > +EFIAPI > +__XenHypercall2 ( > + IN VOID *HypercallAddr, > + IN OUT INTN Arg1, > + IN OUT INTN Arg2 > + ); > + > EFI_STATUS > XenHyperpageInit ( > - IN OUT XENBUS_DEVICE *Dev > ) > { > EFI_HOB_GUID_TYPE *GuidHob; > @@ -36,24 +48,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 +75,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, This could be an unsigned since HypercallID should probably not be negative. Beside this comment, Reviewed-by: Anthony PERARD > + IN OUT INTN Arg1, > + IN OUT INTN Arg2 > ) > { > - xen_add_to_physmap_t Parameter; > + ASSERT (HyperPage != NULL); > > - 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 (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); > } -- Anthony PERARD