| Message ID | 1422025390-8036-15-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 26 January 2015 at 09:27, Laszlo Ersek <lersek@redhat.com> wrote: > On 01/23/15 16:03, Ard Biesheuvel wrote: >> While Xen on Intel uses a virtual PCI device to communicate the >> base address of the grant table, the ARM implementation uses a DT >> node, which is fundamentally incompatible with the way XenBusDxe is >> implemented, i.e., as a UEFI Driver Model implementation for a PCI >> device. >> >> To allow the non-PCI implementations to use this driver anyway, this >> patch introduces an abstract XENIO_PROTOCOL protocol, which contains >> just the grant table base address. The Intel implementation is adapted >> to allocate such a protocol on the fly based on the PCI config space >> metadata, so it operates as before. Other users can invoke the driver >> by installing a XENIO_PROTOCOL instance on a handle, and invoking >> ConnectController() >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> OvmfPkg/Include/Protocol/XenIo.h | 48 ++++++++++ >> OvmfPkg/OvmfPkg.dec | 1 + >> OvmfPkg/XenBusDxe/ComponentName.c | 2 +- >> OvmfPkg/XenBusDxe/GrantTable.c | 5 +- >> OvmfPkg/XenBusDxe/GrantTable.h | 3 +- >> OvmfPkg/XenBusDxe/XenBus.c | 6 +- >> OvmfPkg/XenBusDxe/XenBusDxe.c | 190 ++++++++++++++++++++++++++++++++------ >> OvmfPkg/XenBusDxe/XenBusDxe.h | 2 + >> OvmfPkg/XenBusDxe/XenBusDxe.inf | 1 + >> 9 files changed, 220 insertions(+), 38 deletions(-) >> create mode 100644 OvmfPkg/Include/Protocol/XenIo.h > > This is what we have with virtio: > > EFI_BLOCK_IO_PROTOCOL EFI_SIMPLE_NETWORK_PROTOCOL > [OvmfPkg/VirtioBlkDxe] [OvmfPkg/VirtioNetDxe] > | | > | EFI_EXT_SCSI_PASS_THRU_PROTOCOL | > | [OvmfPkg/VirtioScsiDxe] | > | | | > +------------------------+--------------------------+ > | > VIRTIO_DEVICE_PROTOCOL > | > +---------------------+---------------------+ > | | > [OvmfPkg/VirtioPciDeviceDxe] [custom platform drivers] > | | > | | > EFI_PCI_IO_PROTOCOL [OvmfPkg/Library/VirtioMmioDeviceLib] > [MdeModulePkg/Bus/Pci/PciBusDxe] direct MMIO register access > > > And this is what we should have for Xen, I think: > > > EFI_BLOCK_IO_PROTOCOL > [OvmfPkg/XenPvBlkDxe] > | > XENBUS_PROTOCOL > [OvmfPkg/XenBusDxe] > | > XENIO_PROTOCOL > | > +---------------------+-----------------------+ > | | > [OvmfPkg/XenIoPciDxe] [ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe] > | | > EFI_PCI_IO_PROTOCOL [ArmPlatformPkg/ArmVirtualizationPkg/XenIoMmioLib] > [MdeModulePkg/Bus/Pci/PciBusDxe] > > Very helpful, thanks! > XENIO_PROTOCOL should abstract both of the following: > - hypercall implementation > - grant table base address > > A new driver, OvmfPkg/XenIoPciDxe, would take over the "bottom half" of > the current OvmfPkg/XenBusDxe driver (discovery, hypercalls etc). It > would install the XENIO_PROTOCOL on the PCI device handle. (Meaning, it > would be a device driver.) > Exactly. This is what I started implementing, but decided to get something working first before burning a lot of time on this. (It is always easier to get comments on working patches than on abstract architectural design questions) So it would be sufficient to install the XENIO_PROTOCOL on the existing ControllerHandle containing the EFI_PCI_IO_PROTOCOL? The recursion in the UEFI driver model would take care that the subordinate bus and devices are discovered as well? > A new library, ArmPlatformPkg/ArmVirtualizationPkg/XenIoMmioLib, would > allow its caller, ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe, to > discover the grant table address in the DTB, and call it with the grant > table base address. The library would then create a new handle, with > instances of the XENIO_PROTOCOL and the EFI_DEVICE_PATH_PROTOCOL on it. > The XENIO_PROTOCOL would again abstract the hypercall implementation as > well. > Re hypercall (as you observe below) this is actually orthogonal, i.e., not tied to the PCI vs MMIO question > OvmfPkg/XenBusDxe would preserve only its current "top half". All > hypercalls and all PciIo accesses would be rebased to XENIO_PROTOCOL > member calls. Ie. first you have to collect all those accesses and > distill the member functions of XENIO_PROTOCOL first. > > This is probably the hardest part of the design. When Olivier did the > same for VIRTIO_DEVICE_PROTOCOL, he was certainly helped by the virtio > specification (and my original, PCI-only source code that carefully > referenced each step in the spec), which had formalized the virtio > operations in human language. This could be a good opportunity to force > the Xen guys to produce a similar document (a text file would suffice). > Well, the point here is that the Xen PCI device is only used as a vehicle to convey the grant table address, nothing else. After reading the grant table address from BAR1, no other calls into the EFI_PCI_IO_PROTOCOL are made at all. > No changes for XenPvBlkDxe. > > If you want, you can still factor out the hypercall implementation to a > separate library. Then use the Intel library instance with the > XenIoPciDxe driver, and the ARM library instance with VirtFdtDxe + > XenIoMmioLib. Then, once we have PCI in ArmVirtualizationPkg, you can > use XenIoPciDxe even there, just with the ARM-specific hypercall > library. > Yes. I agree I need to rework that patch, but the existing XenHypercallLib works pretty well, I think. > ... I'm uncertain how much sense the above makes for Xen specifics, and > how much it overlaps with your current patchset, but if I understood > correctly, these layers were your main question, and I think we should > go with the above structure. It follows how virtio is split into layers, > and it puts the UEFI driver model to good use. (One difference is that > Xen has one more layers AIUI.) > > If I understand the current patch right, looking at the > XenBusDxeDriverBindingSupported() hunk, it would make OvmfPkg/XenBusDxe > capable of using both PciIo and XenIo. I don't like that (nor do you, I > believe). :) > Indeed. As I said, the idea was to produce something working to get the discussion going. I will proceed and split off XenIoPciDxe from XenBusDxe, which installs my existing XENIO_PROTOCOL on the existing ControllerHandle if its EFI_PCI_IO_PROTOCOL produces the correct vid/pid and BAR1 mem type. Thanks again for taking the time to look into these patches.
On 26 January 2015 at 10:28, Laszlo Ersek <lersek@redhat.com> wrote: > On 01/26/15 10:46, Ard Biesheuvel wrote: > >> So it would be sufficient to install the XENIO_PROTOCOL on the >> existing ControllerHandle containing the EFI_PCI_IO_PROTOCOL? > > Yes. > > Because there would be only one PCI (b,d,f) that would qualify (you'd > write up the Supported() check appropriately), there'd be only one > instance of XENIO_PROTOCOL created in the system as well. > >> The >> recursion in the UEFI driver model would take care that the >> subordinate bus and devices are discovered as well? > > Right. For example, when connecting all drivers to all devices, or when > connecting the device handle coming out of "XenIoMmioLib" recursively, > or when connecting the PCI (b, d, f) in question recursively, XenBusDxe > would report support for the XENIO protocol instance, then it would be > connected to it, producing a new handle with a XENBUS_PROTOCOL instance > on it for each child. (In fact XENBUS_PROTOCOL should have been called > XEN_DEVICE_PROTOCOL as I understand it, but that's water under the > bridge.) Then the individual device drivers like XenPvBlkDxe would > report support for some of those, etc. > >> Well, the point here is that the Xen PCI device is only used as a >> vehicle to convey the grant table address, nothing else. After reading >> the grant table address from BAR1, no other calls into the >> EFI_PCI_IO_PROTOCOL are made at all. > > I see. It should still build from the bottom up. In the PCI case, your > "chain-reaction" is ignited by PCI enumeration, and you should key off > the appropriate PCI (b,d,f) -- see above. In the MMIO case, you do the > DTB-based enumeration yourself, and start it all by calling the library > function on the appropriate register block (or grant table base > address), which then produces a new handle with XENIO_PROTOCOL on it. > > It's okay that the hypercall mechanism is orthogonal, but that's an > implementation detail. It should still be hidden behind the > XENIO_PROTOCOL interface in my opinion. You can express the Well, the problem is that the XenConsoleSerialPortLib implementation also needs to issue Xen hypercalls, and needs to do so very early. We could still make XENIO_PROTOCOL take its implementations of those hypercalls form XenHypercallLib, as XenConsoleSerialPortLib does, but I don't think it makes the code more understandable in that case. In particular, we would have two different ways of issuing hypercalls from code that is Xen-specific, one directly and one through the IO protocol. > orthogonality inside the implementation of XENIO_PROTOCOL, by delegating > the hypercalls to a library instance. In theory you could have four > providers of XENIO_PROTOCOL: > - a standalone driver that binds PciIo and uses the Intel hypercall > style. > - a standalone driver that binds PciIo and uses the ARM hypercall style. > This difference would be controlled by the library class to library > instance resolution in the DSC file(s). > - A library that takes the grant table base address as an input > parameter, and uses the Intel hypercall style. > - A library that takes the grant table base address as an input > parameter, and uses the ARM hypercall style. (Obviously not > explicitly, but by delegating hypercalls to whatever library instance > that the hypercall library class is resolved to.) > > Ad absurdum, you could use "the library that takes the grant table > address as an input parameter" *inside* your PciIo-based driver. Then, > VirtFdtDxe would scan the DTB and call the main library function > directly, whereas your *thin* PciIo-based, standalone driver would > interrogate the PCI device that it recognizes, and delegate the main > work to the library. For this the library would have to be able to take > a device handle from the caller (would be the PCI device handle in the > PCI case, and a fresh handle with just DevicePath on it in the other > case), and install XenIo on that. > > If this is feasible or not should become very clear when you get that > far in coding, I think. > >> Yes. I agree I need to rework that patch, but the existing >> XenHypercallLib works pretty well, I think. > > Absolutely, it makes sense. > >> I will proceed and split off XenIoPciDxe from XenBusDxe, which >> installs my existing XENIO_PROTOCOL on the existing ControllerHandle >> if its EFI_PCI_IO_PROTOCOL produces the correct vid/pid and BAR1 mem >> type. > > Well... if you don't want to make the hypercall stuff part of the > XENIO_PROTOCOL interface, I can accept that too. My proposal above was: > > XenBusDxe > | > XenIo > | > / \ > / \ > PCI vs. DTB ARM vs. Intel hypercall Lib Instance > > (Ie. XenBusDxe would depend on one abstraction (== XenIo), and XenIo > would depend on two independent abstractions, each of which would have > two possible implementations.) > > But I guess the following also makes sense: > > XenBusDxe > / \ > XenIo ARM vs. Intel hypercall Lib Instance > | > PCI vs. DTB > > The second architecture would keep XenIo much thinner, and it's > certainly sufficient to have only one hypercall method built into the > entire firmware -- you could decide that question at build time with a > "global" library class resolution. > Yes, I think this is the pragmatic choice, and I happen to be feeling very pragmatic at the moment :-) > Please use --stat=150 in the next version, and spoon-feed us the code in > baby bite sized patches! :) > Sure thing.
On 26 January 2015 at 14:10, Laszlo Ersek <lersek@redhat.com> wrote: > On 01/26/15 14:52, Ard Biesheuvel wrote: > >> Well, the problem is that the XenConsoleSerialPortLib implementation >> also needs to issue Xen hypercalls, and needs to do so very early. > > In general virtual serial consoles, be they Xen or virtio, are a huge > "impedance mismatch" (is that the right term?) for UEFI / edk2. For UEFI > / edk2, the serial port library is one of the lowest level libraries, > because it must be able to give you debug output as early as SEC. Fixed > addresses, minimal state, minimal setup. > > However, the virtual serial ports are very stateful and require > elaborate device setup. That matches the DXE phase alright, but nothing > before. > >> We >> could still make XENIO_PROTOCOL take its implementations of those >> hypercalls form XenHypercallLib, as XenConsoleSerialPortLib does, but >> I don't think it makes the code more understandable in that case. In >> particular, we would have two different ways of issuing hypercalls >> from code that is Xen-specific, one directly and one through the IO >> protocol. > > Agreed. The root cause of that is that the virtual (Xen) serial port is > unsuitable for the original purpose of SerialPortLib -- be super > low-level, available as soon as in SEC, without any kind of discovery, > and just have minimal state. It's a debug device on which everything > else relies on. > > (The same is true of virtio-serial, obviously.) > In fact, this is working fine for the Xen console. My SerialPortLib implementation has a constructor that issues 2 hypercalls through XenHypercallLib to initialize the console, and the PrePi SEC implementation calls the SerialPortLib constructor explicitly. For ARM, this is essential as there is no other console. For x86, this wouldn't work as its XenHypercallLib depends on the hyperpage HOB to be available, but then, it doesn't need the Xen console anyway. > For QEMU ARM guests, we use the emulated (PL011) serial port, which is > easy to program. But you do remember the sad hoops you had jump through > with the DTB because you wanted to make the base address dynamic. > Yes. But one of the primary issues is that the emulated NOR flash breaks global variables, making it difficult to retain the state created by a constructor. >>> But I guess the following also makes sense: >>> >>> XenBusDxe >>> / \ >>> XenIo ARM vs. Intel hypercall Lib Instance >>> | >>> PCI vs. DTB >>> >>> The second architecture would keep XenIo much thinner, and it's >>> certainly sufficient to have only one hypercall method built into the >>> entire firmware -- you could decide that question at build time with a >>> "global" library class resolution. >>> >> >> Yes, I think this is the pragmatic choice, and I happen to be feeling >> very pragmatic at the moment :-) > > "At the moment"? :) No doubt. :) >
diff --git a/OvmfPkg/Include/Protocol/XenIo.h b/OvmfPkg/Include/Protocol/XenIo.h new file mode 100644 index 000000000000..510391f3b3e8 --- /dev/null +++ b/OvmfPkg/Include/Protocol/XenIo.h @@ -0,0 +1,48 @@ +/** @file + XenIo protocol to abstract arch specific details + + The Xen implementations for the Intel and ARM archictures differ in the way + the base address of the grant table is communicated to the guest. The former + uses a virtual PCI device, while the latter uses a device tree node. + In order to allow the XenBusDxe UEFI driver to be reused for the non-PCI + Xen implementation, this abstract protocol can be installed on a handle + with the appropriate base address. + + Copyright (C) 2014, Linaro Ltd. + + 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. + +**/ + +#ifndef __PROTOCOL_XENIO_H__ +#define __PROTOCOL_XENIO_H__ + +#include <IndustryStandard/Xen/xen.h> + +#define XENIO_PROTOCOL_GUID \ + {0x6efac84f, 0x0ab0, 0x4747, {0x81, 0xbe, 0x85, 0x55, 0x62, 0x59, 0x04, 0x49}} + +/// +/// Forward declaration +/// +typedef struct _XENIO_PROTOCOL XENIO_PROTOCOL; + +/// +/// Protocol structure +/// +struct _XENIO_PROTOCOL { + // + // Protocol data fields + // + EFI_PHYSICAL_ADDRESS GrantTableAddress; +}; + +extern EFI_GUID gXenIoProtocolGuid; + +#endif diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec index 30a9fb1e9b42..3711fa922311 100644 --- a/OvmfPkg/OvmfPkg.dec +++ b/OvmfPkg/OvmfPkg.dec @@ -58,6 +58,7 @@ gVirtioDeviceProtocolGuid = {0xfa920010, 0x6785, 0x4941, {0xb6, 0xec, 0x49, 0x8c, 0x57, 0x9f, 0x16, 0x0a}} gBlockMmioProtocolGuid = {0x6b558ce3, 0x69e5, 0x4c67, {0xa6, 0x34, 0xf7, 0xfe, 0x72, 0xad, 0xbe, 0x84}} gXenBusProtocolGuid = {0x3d3ca290, 0xb9a5, 0x11e3, {0xb7, 0x5d, 0xb8, 0xac, 0x6f, 0x7d, 0x65, 0xe6}} + gXenIoProtocolGuid = {0x6efac84f, 0x0ab0, 0x4747, {0x81, 0xbe, 0x85, 0x55, 0x62, 0x59, 0x04, 0x49}} [PcdsFixedAtBuild] gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase|0x0|UINT32|0 diff --git a/OvmfPkg/XenBusDxe/ComponentName.c b/OvmfPkg/XenBusDxe/ComponentName.c index 4530509e65dc..3f2dd406c77d 100644 --- a/OvmfPkg/XenBusDxe/ComponentName.c +++ b/OvmfPkg/XenBusDxe/ComponentName.c @@ -155,7 +155,7 @@ XenBusDxeComponentNameGetControllerName ( Status = EfiTestManagedDevice ( ControllerHandle, gXenBusDxeDriverBinding.DriverBindingHandle, - &gEfiPciIoProtocolGuid + &gXenIoProtocolGuid ); if (EFI_ERROR (Status)) { return Status; diff --git a/OvmfPkg/XenBusDxe/GrantTable.c b/OvmfPkg/XenBusDxe/GrantTable.c index 52ff045a74db..e825c7c76fa5 100644 --- a/OvmfPkg/XenBusDxe/GrantTable.c +++ b/OvmfPkg/XenBusDxe/GrantTable.c @@ -139,8 +139,7 @@ XenGrantTableEndAccess ( VOID XenGrantTableInit ( - IN XENBUS_DEVICE *Dev, - IN UINT64 MmioAddr + IN XENBUS_DEVICE *Dev ) { xen_add_to_physmap_t Parameters; @@ -155,7 +154,7 @@ XenGrantTableInit ( XenGrantTablePutFreeEntry ((grant_ref_t)Index); } - GrantTable = (VOID*)(UINTN) MmioAddr; + GrantTable = (VOID*)(UINTN) Dev->XenIo->GrantTableAddress; for (Index = 0; Index < NR_GRANT_FRAMES; Index++) { Parameters.domid = DOMID_SELF; Parameters.idx = Index; diff --git a/OvmfPkg/XenBusDxe/GrantTable.h b/OvmfPkg/XenBusDxe/GrantTable.h index 5772c56662df..194275ba7ed5 100644 --- a/OvmfPkg/XenBusDxe/GrantTable.h +++ b/OvmfPkg/XenBusDxe/GrantTable.h @@ -29,8 +29,7 @@ **/ VOID XenGrantTableInit ( - IN XENBUS_DEVICE *Dev, - IN UINT64 MmioAddr + IN XENBUS_DEVICE *Dev ); /** diff --git a/OvmfPkg/XenBusDxe/XenBus.c b/OvmfPkg/XenBusDxe/XenBus.c index f69c27dd184a..ee9526c33252 100644 --- a/OvmfPkg/XenBusDxe/XenBus.c +++ b/OvmfPkg/XenBusDxe/XenBus.c @@ -138,7 +138,7 @@ XenBusAddDevice ( XENBUS_PRIVATE_DATA *Private; EFI_STATUS Status; XENBUS_DEVICE_PATH *TempXenBusPath; - VOID *ChildPciIo; + VOID *ChildXenIo; AsciiSPrint (DevicePath, sizeof (DevicePath), "device/%a/%a", Type, Id); @@ -208,8 +208,8 @@ XenBusAddDevice ( } Status = gBS->OpenProtocol (Dev->ControllerHandle, - &gEfiPciIoProtocolGuid, - &ChildPciIo, Dev->This->DriverBindingHandle, + &gXenIoProtocolGuid, + &ChildXenIo, Dev->This->DriverBindingHandle, Private->Handle, EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER); if (EFI_ERROR (Status)) { diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.c b/OvmfPkg/XenBusDxe/XenBusDxe.c index cc334c086c1f..efa9bb0ccd23 100644 --- a/OvmfPkg/XenBusDxe/XenBusDxe.c +++ b/OvmfPkg/XenBusDxe/XenBusDxe.c @@ -234,8 +234,29 @@ XenBusDxeDriverBindingSupported ( { EFI_STATUS Status; EFI_PCI_IO_PROTOCOL *PciIo; + XENIO_PROTOCOL *XenIo; PCI_TYPE00 Pci; + // + // If the ControllerHandle supports the XENIO_PROTOCOL, we can use + // it directly without having to bother with the PCI representation. + // + Status = gBS->OpenProtocol ( + ControllerHandle, + &gXenIoProtocolGuid, + (VOID **)&XenIo, + This->DriverBindingHandle, + ControllerHandle, + EFI_OPEN_PROTOCOL_BY_DRIVER + ); + + gBS->CloseProtocol (ControllerHandle, &gXenIoProtocolGuid, + This->DriverBindingHandle, ControllerHandle); + + if (!EFI_ERROR (Status) || Status == EFI_ALREADY_STARTED) { + return Status; + } + Status = gBS->OpenProtocol ( ControllerHandle, &gEfiPciIoProtocolGuid, @@ -280,6 +301,137 @@ NotifyExitBoot ( } /** + Opens the XENIO_PROTOCOL on ControllerHandle. + + If the protocol is not available, but the EFI_PCI_IO_PROTOCOL is, create + the XENIO_PROTOCOL protocol instance on the fly based on the PCI metadata + and install it on Handle. + + @param ControllerHandle The controller handle + @param DriverBindingHandle The driver binding handle + @param XenIo The XENIO_PROTOCOL return value + @param PciIo The EFI_PCI_IO_PROTOCOL return value, or NULL if + the XENIO_PROTOCOL already existed on Handle +**/ +STATIC +EFI_STATUS +OpenOrInstallXenIoProtocolOnHandle ( + IN EFI_HANDLE ControllerHandle, + IN EFI_HANDLE DriverBindingHandle, + OUT XENIO_PROTOCOL **XenIo, + OUT EFI_PCI_IO_PROTOCOL **PciIo + ) +{ + EFI_STATUS Status; + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *BarDesc; + + // + // We support both EFI_PCI_IO_PROTOCOL and XENIO_PROTOCOL, the former only + // if the vendor and product IDs match up (as verified in .Supported()). + // The latter has precedence, and we install it on the fly if it is not + // supported. + // + *PciIo = NULL; + Status = gBS->OpenProtocol ( + ControllerHandle, + &gXenIoProtocolGuid, + (VOID**)XenIo, + DriverBindingHandle, + ControllerHandle, + EFI_OPEN_PROTOCOL_BY_DRIVER + ); + if (EFI_ERROR (Status)) { + // + // This handle does not support XENIO_PROTOCOL yet, which implies that it + // does support the EFI_PCI_IO_PROTOCOL, or we wouldn't have been invoked. + // Get the grant table base address from the PCI config space, and allocate + // and install the XENIO_PROTOCOL instance on the fly. + // + Status = gBS->OpenProtocol ( + ControllerHandle, + &gEfiPciIoProtocolGuid, + (VOID**)PciIo, + DriverBindingHandle, + ControllerHandle, + EFI_OPEN_PROTOCOL_BY_DRIVER + ); + ASSERT_EFI_ERROR (Status); + + *XenIo = AllocateZeroPool (sizeof(XENIO_PROTOCOL)); + ASSERT (*XenIo != NULL); + + // + // The BAR1 of this PCI device is used for shared memory and is supposed to + // look like MMIO. The address space of the BAR1 will be used to map the + // Grant Table. + // + Status = (*PciIo)->GetBarAttributes (*PciIo, PCI_BAR_IDX1, NULL, (VOID**) &BarDesc); + ASSERT_EFI_ERROR (Status); + ASSERT (BarDesc->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM); + + /* Get a Memory address for mapping the Grant Table. */ + DEBUG ((EFI_D_INFO, "XenBus: BAR at %LX\n", BarDesc->AddrRangeMin)); + (*XenIo)->GrantTableAddress = BarDesc->AddrRangeMin; + FreePool (BarDesc); + + // + // Now install the XENIO_PROTOCOL protocol instance on Handle. + // This should only fail in extraordinary cases, as we have already + // established that the protocol does not exist yet on the handle. + // + Status = gBS->InstallProtocolInterface (ControllerHandle, + &gXenIoProtocolGuid, EFI_NATIVE_INTERFACE, *XenIo); + ASSERT_EFI_ERROR (Status); + + Status = gBS->OpenProtocol ( + ControllerHandle, + &gXenIoProtocolGuid, + (VOID**)XenIo, + DriverBindingHandle, + ControllerHandle, + EFI_OPEN_PROTOCOL_BY_DRIVER + ); + if (EFI_ERROR (Status)) { + gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid, + DriverBindingHandle, ControllerHandle); + } + } + return Status; +} + +/** + Close or uninstall the XENIO_PROTOCOL instance on ControllerHandle + + Close the XENIO_PROTOCOL protocol instance on ControllerHandle, and + in case PciIo != NULL, uninstall and deallocate it as well. + + @param ControllerHandle The controller handle + @param DriverBindingHandle The driver binding handle + @param XenIo The XENIO_PROTOCOL protocol instance + @param PciIo The EFI_PCI_IO_PROTOCOL protocol instance, or NULL + if the XENIO_PROTOCOL already existed on + ControllerHandle +**/ +STATIC +VOID +CloseOrUninstallXenIoProtocolOnHandle ( + IN EFI_HANDLE ControllerHandle, + IN EFI_HANDLE DriverBindingHandle, + IN XENIO_PROTOCOL *XenIo, + IN EFI_PCI_IO_PROTOCOL *PciIo + ) +{ + gBS->CloseProtocol (ControllerHandle, &gXenIoProtocolGuid, + DriverBindingHandle, ControllerHandle); + if (PciIo != NULL) { + gBS->UninstallProtocolInterface (ControllerHandle, &gXenIoProtocolGuid, XenIo); + FreePool (XenIo); + gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid, + DriverBindingHandle, ControllerHandle); + } +} + +/** Starts a bus controller. The Start() function is designed to be invoked from the EFI boot service ConnectController(). @@ -326,19 +478,12 @@ XenBusDxeDriverBindingStart ( { EFI_STATUS Status; XENBUS_DEVICE *Dev; + XENIO_PROTOCOL *XenIo; EFI_PCI_IO_PROTOCOL *PciIo; - EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *BarDesc; - UINT64 MmioAddr; EFI_DEVICE_PATH_PROTOCOL *DevicePath; - Status = gBS->OpenProtocol ( - ControllerHandle, - &gEfiPciIoProtocolGuid, - (VOID **) &PciIo, - This->DriverBindingHandle, - ControllerHandle, - EFI_OPEN_PROTOCOL_BY_DRIVER - ); + Status = OpenOrInstallXenIoProtocolOnHandle (ControllerHandle, + This->DriverBindingHandle, &XenIo, &PciIo); if (EFI_ERROR (Status)) { return Status; } @@ -361,6 +506,7 @@ XenBusDxeDriverBindingStart ( Dev->This = This; Dev->ControllerHandle = ControllerHandle; Dev->PciIo = PciIo; + Dev->XenIo = XenIo; Dev->DevicePath = DevicePath; InitializeListHead (&Dev->ChildList); @@ -376,20 +522,6 @@ XenBusDxeDriverBindingStart ( mMyDevice = Dev; EfiReleaseLock (&mMyDeviceLock); - // - // The BAR1 of this PCI device is used for shared memory and is supposed to - // look like MMIO. The address space of the BAR1 will be used to map the - // Grant Table. - // - Status = PciIo->GetBarAttributes (PciIo, PCI_BAR_IDX1, NULL, (VOID**) &BarDesc); - ASSERT_EFI_ERROR (Status); - ASSERT (BarDesc->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM); - - /* Get a Memory address for mapping the Grant Table. */ - DEBUG ((EFI_D_INFO, "XenBus: BAR at %LX\n", BarDesc->AddrRangeMin)); - MmioAddr = BarDesc->AddrRangeMin; - FreePool (BarDesc); - Status = XenGetSharedInfoPage (Dev); if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "XenBus: Unable to get the shared info page.\n")); @@ -397,7 +529,7 @@ XenBusDxeDriverBindingStart ( goto ErrorAllocated; } - XenGrantTableInit (Dev, MmioAddr); + XenGrantTableInit (Dev); Status = XenStoreInit (Dev); ASSERT_EFI_ERROR (Status); @@ -417,8 +549,8 @@ ErrorAllocated: gBS->CloseProtocol (ControllerHandle, &gEfiDevicePathProtocolGuid, This->DriverBindingHandle, ControllerHandle); ErrorOpenningProtocol: - gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid, - This->DriverBindingHandle, ControllerHandle); + CloseOrUninstallXenIoProtocolOnHandle (ControllerHandle, + This->DriverBindingHandle, XenIo, PciIo); return Status; } @@ -507,8 +639,8 @@ XenBusDxeDriverBindingStop ( gBS->CloseProtocol (ControllerHandle, &gEfiDevicePathProtocolGuid, This->DriverBindingHandle, ControllerHandle); - gBS->CloseProtocol (ControllerHandle, &gEfiPciIoProtocolGuid, - This->DriverBindingHandle, ControllerHandle); + CloseOrUninstallXenIoProtocolOnHandle (ControllerHandle, + This->DriverBindingHandle, Dev->XenIo, Dev->PciIo); mMyDevice = NULL; FreePool (Dev); diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h index 9b7219906a69..596e5f04a03b 100644 --- a/OvmfPkg/XenBusDxe/XenBusDxe.h +++ b/OvmfPkg/XenBusDxe/XenBusDxe.h @@ -40,6 +40,7 @@ // Consumed Protocols // #include <Protocol/PciIo.h> +#include <Protocol/XenIo.h> // @@ -87,6 +88,7 @@ struct _XENBUS_DEVICE { EFI_DRIVER_BINDING_PROTOCOL *This; EFI_HANDLE ControllerHandle; EFI_PCI_IO_PROTOCOL *PciIo; + XENIO_PROTOCOL *XenIo; EFI_EVENT ExitBootEvent; EFI_DEVICE_PATH_PROTOCOL *DevicePath; LIST_ENTRY ChildList; diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf index 714607dbd6f8..8294997e20db 100644 --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf @@ -71,4 +71,5 @@ gEfiComponentName2ProtocolGuid gEfiComponentNameProtocolGuid gXenBusProtocolGuid + gXenIoProtocolGuid
While Xen on Intel uses a virtual PCI device to communicate the base address of the grant table, the ARM implementation uses a DT node, which is fundamentally incompatible with the way XenBusDxe is implemented, i.e., as a UEFI Driver Model implementation for a PCI device. To allow the non-PCI implementations to use this driver anyway, this patch introduces an abstract XENIO_PROTOCOL protocol, which contains just the grant table base address. The Intel implementation is adapted to allocate such a protocol on the fly based on the PCI config space metadata, so it operates as before. Other users can invoke the driver by installing a XENIO_PROTOCOL instance on a handle, and invoking ConnectController() Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- OvmfPkg/Include/Protocol/XenIo.h | 48 ++++++++++ OvmfPkg/OvmfPkg.dec | 1 + OvmfPkg/XenBusDxe/ComponentName.c | 2 +- OvmfPkg/XenBusDxe/GrantTable.c | 5 +- OvmfPkg/XenBusDxe/GrantTable.h | 3 +- OvmfPkg/XenBusDxe/XenBus.c | 6 +- OvmfPkg/XenBusDxe/XenBusDxe.c | 190 ++++++++++++++++++++++++++++++++------ OvmfPkg/XenBusDxe/XenBusDxe.h | 2 + OvmfPkg/XenBusDxe/XenBusDxe.inf | 1 + 9 files changed, 220 insertions(+), 38 deletions(-) create mode 100644 OvmfPkg/Include/Protocol/XenIo.h