From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9AF3B81992 for ; Thu, 5 Jan 2017 08:58:07 -0800 (PST) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1C48AC05CDEF; Thu, 5 Jan 2017 16:58:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-100.phx2.redhat.com [10.3.116.100]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v05Gw6Dj021093; Thu, 5 Jan 2017 11:58:06 -0500 To: Anthony PERARD , edk2-devel@ml01.01.org, xen-devel@lists.xenproject.org References: <20161208153340.2285-1-anthony.perard@citrix.com> <20161208153340.2285-14-anthony.perard@citrix.com> From: Laszlo Ersek Message-ID: Date: Thu, 5 Jan 2017 17:58:04 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20161208153340.2285-14-anthony.perard@citrix.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 05 Jan 2017 16:58:08 +0000 (UTC) Subject: Re: [PATCH RFC 13/14] OvmfPkg: Introduce XenIoPvhDxe to initialize Grant Tables X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jan 2017 16:58:07 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 12/08/16 16:33, Anthony PERARD wrote: > This "device" use XenIoMmioLib to reserve some space to be use by grant > tables. It's use 0xfc000000, which might not be a good choice... > > There is probably a better way that using a device for this. > --- > OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c | 263 ++++++++++++++++++++++++++++++++++++ > OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf | 45 ++++++ > OvmfPkg/XenOvmf.dsc | 2 + > OvmfPkg/XenOvmf.fdf | 1 + > 4 files changed, 311 insertions(+) > create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c > create mode 100644 OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf I recommend to check out the existent use of XenIoMmioLib, namely in "ArmVirtPkg/XenioFdtDxe". In brief, for such purposes a DXE_DRIVER is appropriate, where you simply do the deed (call XenIoMmioInstall()) in the driver's entry point function. No need for a UEFI_DRIVER module which conforms to the UEFI Driver Model. Regarding where to put the area: no clue. If it doesn't overlap any memory area added in (Xen)PlatformPei with memory resource descriptors, nor areas added later in DXE, with gDS->AddMemorySpace(), then it should be fine. >>From a quick look, 0xFC000000 should work. For completeness, I'd also modify the (DXE) driver to call gDS->AddMemorySpace() with type EfiGcdMemoryTypeReserved, and also immediately call gDS->AllocateMemorySpace() with the same type and EfiGcdAllocateAddress, in order to allocate the exact reserved chunk. (See "7.2 Global Coherency Domain Services" in vol2 of the Platform Init spec for background.) OTOH, I don't see why a simple AllocateReservedPages() call wouldn't work (which would carve a chunk out of normal system memory for this). Why did you comment that out in the code below? Also, please don't forget the Citrix copyright notice etc etc. Thanks Laszlo > > diff --git a/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c > new file mode 100644 > index 0000000..12e076f > --- /dev/null > +++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.c > @@ -0,0 +1,263 @@ > +/** @file > + > + XXX > + > + XXX > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which accompanies this > + distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* #include */ > +STATIC BOOLEAN mXenIoInitialized = FALSE; > + > +/** > + > + Device probe function for this driver. > + > + The DXE core calls this function for any given device in order to see if the > + driver can drive the device. > + > + @param[in] This The EFI_DRIVER_BINDING_PROTOCOL object > + incorporating this driver (independently of > + any device). > + > + @param[in] DeviceHandle The device to probe. > + > + @param[in] RemainingDevicePath Relevant only for bus drivers, ignored. > + > + > + @retval EFI_SUCCESS The driver supports the device being probed. > + > + @retval EFI_UNSUPPORTED The driver does not support the device being probed. > + > + @return Error codes from the OpenProtocol() boot service or > + the PciIo protocol. > + > +**/ > +#if 1 > +STATIC > +EFI_STATUS > +EFIAPI > +XenIoPvhDeviceBindingSupported ( > + IN EFI_DRIVER_BINDING_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath > + ) > +{ > + > + // XXX check if running Xen PVH > + // > + > + if (mXenIoInitialized) { > + return EFI_ALREADY_STARTED; > + } > + > + DEBUG((EFI_D_INFO, "%a %d\n", __FUNCTION__, __LINE__)); > + return EFI_SUCCESS; > +} > +#endif > + > +/** > + > + After we've pronounced support for a specific device in > + DriverBindingSupported(), we start managing said device (passed in by the > + Driver Execution Environment) with the following service. > + > + See DriverBindingSupported() for specification references. > + > + @param[in] This The EFI_DRIVER_BINDING_PROTOCOL object > + incorporating this driver (independently of > + any device). > + > + @param[in] DeviceHandle The supported device to drive. > + > + @param[in] RemainingDevicePath Relevant only for bus drivers, ignored. > + > + > + @retval EFI_SUCCESS The device was started. > + > + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > + > + @return Error codes from the OpenProtocol() boot > + service, the PciIo protocol or the > + InstallProtocolInterface() boot service. > + > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +XenIoPvhDeviceBindingStart ( > + IN EFI_DRIVER_BINDING_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN EFI_DEVICE_PATH_PROTOCOL *RemainingDevicePath > + ) > +{ > + EFI_STATUS Status; > + EFI_HANDLE Handle = NULL; > + > + /* Status = XenIoMmioInstall(&Handle, (UINTN)AllocateReservedPages(4)); */ > + Status = XenIoMmioInstall(&Handle, (UINTN)0xfc000000); > + > + if (!EFI_ERROR (Status)) { > + mXenIoInitialized = TRUE; > + return EFI_SUCCESS; > + } > + > + return Status; > +} > + > +#if 1 > +/** > + > + Stop driving the XenIo PCI device > + > + @param[in] This The EFI_DRIVER_BINDING_PROTOCOL object > + incorporating this driver (independently of any > + device). > + > + @param[in] DeviceHandle Stop driving this device. > + > + @param[in] NumberOfChildren Since this function belongs to a device driver > + only (as opposed to a bus driver), the caller > + environment sets NumberOfChildren to zero, and > + we ignore it. > + > + @param[in] ChildHandleBuffer Ignored (corresponding to NumberOfChildren). > + > + @retval EFI_SUCCESS Driver instance has been stopped and the PCI > + configuration attributes have been restored. > + > + @return Error codes from the OpenProtocol() or > + CloseProtocol(), UninstallProtocolInterface() > + boot services. > + > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +XenIoPvhDeviceBindingStop ( > + IN EFI_DRIVER_BINDING_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN UINTN NumberOfChildren, > + IN EFI_HANDLE *ChildHandleBuffer > + ) > +{ > + /* return XenIoMmioUninstall(Handle); */ > + return EFI_SUCCESS; > +} > + > + > +// > +// The static object that groups the Supported() (ie. probe), Start() and > +// Stop() functions of the driver together. Refer to UEFI Spec 2.3.1 + Errata > +// C, 10.1 EFI Driver Binding Protocol. > +// > +STATIC EFI_DRIVER_BINDING_PROTOCOL gDriverBinding = { > + &XenIoPvhDeviceBindingSupported, > + &XenIoPvhDeviceBindingStart, > + &XenIoPvhDeviceBindingStop, > + 0x10, // Version, must be in [0x10 .. 0xFFFFFFEF] for IHV-developed drivers > + NULL, // ImageHandle, to be overwritten by > + // EfiLibInstallDriverBindingComponentName2() in XenIoPvhDeviceEntryPoint() > + NULL // DriverBindingHandle, ditto > +}; > + > + > +// > +// The purpose of the following scaffolding (EFI_COMPONENT_NAME_PROTOCOL and > +// EFI_COMPONENT_NAME2_PROTOCOL implementation) is to format the driver's name > +// in English, for display on standard console devices. This is recommended for > +// UEFI drivers that follow the UEFI Driver Model. Refer to the Driver Writer's > +// Guide for UEFI 2.3.1 v1.01, 11 UEFI Driver and Controller Names. > +// > +STATIC > +EFI_UNICODE_STRING_TABLE mDriverNameTable[] = { > + { "eng;en", L"XenIo PVH Driver" }, > + { NULL, NULL } > +}; > + > +STATIC > +EFI_COMPONENT_NAME_PROTOCOL gComponentName; > + > +EFI_STATUS > +EFIAPI > +XenIoPvhGetDriverName ( > + IN EFI_COMPONENT_NAME_PROTOCOL *This, > + IN CHAR8 *Language, > + OUT CHAR16 **DriverName > + ) > +{ > + return LookupUnicodeString2 ( > + Language, > + This->SupportedLanguages, > + mDriverNameTable, > + DriverName, > + (BOOLEAN)(This == &gComponentName) // Iso639Language > + ); > +} > + > +EFI_STATUS > +EFIAPI > +XenIoPvhGetDeviceName ( > + IN EFI_COMPONENT_NAME_PROTOCOL *This, > + IN EFI_HANDLE DeviceHandle, > + IN EFI_HANDLE ChildHandle, > + IN CHAR8 *Language, > + OUT CHAR16 **ControllerName > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > +STATIC > +EFI_COMPONENT_NAME_PROTOCOL gComponentName = { > + &XenIoPvhGetDriverName, > + &XenIoPvhGetDeviceName, > + "eng" // SupportedLanguages, ISO 639-2 language codes > +}; > + > +STATIC > +EFI_COMPONENT_NAME2_PROTOCOL gComponentName2 = { > + (EFI_COMPONENT_NAME2_GET_DRIVER_NAME) &XenIoPvhGetDriverName, > + (EFI_COMPONENT_NAME2_GET_CONTROLLER_NAME) &XenIoPvhGetDeviceName, > + "en" // SupportedLanguages, RFC 4646 language codes > +}; > + > +#endif > + > +// > +// Entry point of this driver. > +// > +EFI_STATUS > +EFIAPI > +XenIoPvhDeviceEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + /* DEBUG((EFI_D_ERROR, "%a %d\n", __FUNCTION__, __LINE__)); */ > + /* return XenIoPvhDeviceBindingStart(ImageHandle); */ > + > + return EfiLibInstallDriverBindingComponentName2 ( > + ImageHandle, > + SystemTable, > + &gDriverBinding, > + ImageHandle, > + &gComponentName, > + &gComponentName2 > + ); > +} > diff --git a/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf > new file mode 100644 > index 0000000..dbdfd6e > --- /dev/null > +++ b/OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf > @@ -0,0 +1,45 @@ > +## @file > +# XXX > +# > +# Copyright (C) XXX > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = XenIoPvhDxe > + FILE_GUID = 7a567cc4-0e75-4d7a-a305-c3db109b53ad > + MODULE_TYPE = UEFI_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = XenIoPvhDeviceEntryPoint > + > +[Packages] > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[Sources] > + XenIoPvhDxe.c > + > +[LibraryClasses] > + UefiDriverEntryPoint > + UefiBootServicesTableLib > + MemoryAllocationLib > + BaseMemoryLib > + BaseLib > + UefiLib > + DebugLib > + XenIoMmioLib > + > +[Protocols] > + gEfiDriverBindingProtocolGuid > + gEfiComponentName2ProtocolGuid > + gEfiComponentNameProtocolGuid > + gXenIoProtocolGuid > diff --git a/OvmfPkg/XenOvmf.dsc b/OvmfPkg/XenOvmf.dsc > index 8bce996..a7a884e 100644 > --- a/OvmfPkg/XenOvmf.dsc > +++ b/OvmfPkg/XenOvmf.dsc > @@ -168,6 +168,7 @@ > SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf > OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf > XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf > + XenIoMmioLib|OvmfPkg/Library/XenIoMmioLib/XenIoMmioLib.inf > > [LibraryClasses.common] > !if $(SECURE_BOOT_ENABLE) == TRUE > @@ -587,6 +588,7 @@ > !endif > } > OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf > + OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf > OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > OvmfPkg/XenBusDxe/XenBusDxe.inf > OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf > diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf > index a40d186..a500ab6 100644 > --- a/OvmfPkg/XenOvmf.fdf > +++ b/OvmfPkg/XenOvmf.fdf > @@ -220,6 +220,7 @@ INF MdeModulePkg/Universal/Metronome/Metronome.inf > INF PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf > > INF OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf > +INF OvmfPkg/XenIoPvhDxe/XenIoPvhDxe.inf > INF OvmfPkg/XenIoPciDxe/XenIoPciDxe.inf > INF OvmfPkg/XenBusDxe/XenBusDxe.inf > INF OvmfPkg/XenPvBlkDxe/XenPvBlkDxe.inf >