On 10/02/17 19:43, Jordan Justen wrote: > On 2017-09-25 12:58:24, Laszlo Ersek wrote: >> Parse QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION from the bridges' >> conventional config spaces. Translate the fields as follows: >> >> * BusNumbers: >> * 0 -- no reservation; >> * (-1) -- firmware default, i.e. no reservation; >> * otherwise -- reserve the requested value. (NB, bus number reservation >> is not supposed to work before >> <https://bugzilla.tianocore.org/show_bug.cgi?id=656> is fixed.) >> >> * Io: >> * 0 -- no reservation; >> * (-1) -- keep our current default (512B); >> * otherwise -- round up the requested value and reserve that. >> >> * NonPrefetchable32BitMmio: >> * 0 -- no reservation; >> * (-1) -- keep our current default (2MB); >> * otherwise -- round up the requested value and reserve that. >> >> * Prefetchable32BitMmio: >> * 0 -- no reservation, proceed to Prefetchable64BitMmio; >> * (-1) -- firmware default, i.e. no reservation, proceed to >> Prefetchable64BitMmio; >> * otherwise -- round up the requested value and reserve that. (NB, if >> Prefetchable32BitMmio is reserved in addition to >> NonPrefetchable32BitMmio, then PciBusDxe currently runs into an >> assertion failure. Refer to >> <https://bugzilla.tianocore.org/show_bug.cgi?id=720>.) >> >> * Prefetchable64BitMmio: >> * only reached if Prefetchable32BitMmio was not reserved; >> * 0 -- no reservation; >> * (-1) -- firmware default, i.e. no reservation; >> * otherwise -- round up the requested value and reserve that. >> >> If QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION is missing, plus any >> time the rounding fails, fall back to the current defaults. >> >> Cc: Jordan Justen <[email protected]> >> Cc: Marcel Apfelbaum <[email protected]> >> Cc: Ruiyu Ni <[email protected]> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Laszlo Ersek <[email protected]> >> --- >> OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf | 2 + >> OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c | 371 +++++++++++++++++++- >> 2 files changed, 367 insertions(+), 6 deletions(-) >> >> diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf >> b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf >> index e0ec9baae1c2..38043986eb67 100644 >> --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf >> +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.inf >> @@ -26,15 +26,17 @@ [Sources] >> >> [Packages] >> MdeModulePkg/MdeModulePkg.dec >> MdePkg/MdePkg.dec >> + OvmfPkg/OvmfPkg.dec >> >> [LibraryClasses] >> BaseLib >> BaseMemoryLib >> DebugLib >> DevicePathLib >> MemoryAllocationLib >> + PciLib >> UefiBootServicesTableLib >> UefiDriverEntryPoint >> >> [Protocols] >> diff --git a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c >> b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c >> index 39646973794b..177e1a62120d 100644 >> --- a/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c >> +++ b/OvmfPkg/PciHotPlugInitDxe/PciHotPlugInit.c >> @@ -13,14 +13,16 @@ >> WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> **/ >> >> #include <IndustryStandard/Acpi10.h> >> +#include <IndustryStandard/QemuPciBridgeCapabilities.h> >> >> #include <Library/BaseLib.h> >> #include <Library/BaseMemoryLib.h> >> #include <Library/DebugLib.h> >> #include <Library/DevicePathLib.h> >> #include <Library/MemoryAllocationLib.h> >> +#include <Library/PciLib.h> >> #include <Library/UefiBootServicesTableLib.h> >> >> #include <Protocol/PciHotPlugInit.h> >> #include <Protocol/PciRootBridgeIo.h> >> @@ -245,8 +247,239 @@ HighBitSetRoundUp64 ( >> return (HighBit < 64) ? HighBit : -1; >> } >> >> >> +/** >> + Read a slice from conventional PCI config space at the given offset, then >> + advance the offset. >> + >> + @param[in] PciAddress The address of the PCI Device -- Bus, Device, >> Function >> + -- in UEFI (not PciLib) encoding. >> + >> + @param[in,out] Offset On input, the offset in conventional PCI config >> space >> + to start reading from. On output, the offset of the >> + first byte that was not read. >> + >> + @param[in] Size The number of bytes to read. >> + >> + @param[out] Buffer On output, the bytes read from PCI config space are >> + stored in this object. >> +**/ >> +STATIC >> +VOID >> +ReadConfigSpace ( >> + IN CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *PciAddress, >> + IN OUT UINT8 *Offset, >> + IN UINT8 Size, >> + OUT VOID *Buffer >> + ) >> +{ >> + PciReadBuffer ( >> + PCI_LIB_ADDRESS ( >> + PciAddress->Bus, >> + PciAddress->Device, >> + PciAddress->Function, >> + *Offset >> + ), >> + Size, >> + Buffer >> + ); >> + *Offset += Size; >> +} >> + >> + >> +/** >> + Convenience wrapper macro for ReadConfigSpace(). >> + >> + Given the following conditions: >> + >> + - HeaderField is the first field in the structure pointed-to by Struct, >> + >> + - Struct->HeaderField has been populated from the conventional PCI config >> + space of the PCI device identified by PciAddress, >> + >> + - *Offset points one past HeaderField in the conventional PCI config >> space of >> + the PCI device identified by PciAddress, >> + >> + populate the rest of *Struct from conventional PCI config space, starting >> at >> + *Offset. Finally, increment *Offset so that it point one past *Struct. >> + >> + @param[in] PciAddress The address of the PCI Device -- Bus, Device, >> Function >> + -- in UEFI (not PciLib) encoding. Type: pointer to >> + CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS. >> + >> + @param[in,out] Offset On input, the offset in conventional PCI config >> space >> + to start reading from; one past >> Struct->HeaderField. >> + On output, the offset of the first byte that was >> not >> + read; one past *Struct. Type: pointer to UINT8. >> + >> + @param[out] Struct The structure to complete. Type: pointer to >> structure >> + object. >> + >> + @param[in] HeaderField The name of the first field in *Struct, after which >> + *Struct should be populated. Type: structure member >> + identifier. >> +**/ >> +#define COMPLETE_CONFIG_SPACE_STRUCT(PciAddress, Offset, Struct, >> HeaderField) \ >> + ReadConfigSpace ( >> \ >> + (PciAddress), >> \ >> + (Offset), >> \ >> + (UINT8)(sizeof *(Struct) - sizeof ((Struct)->HeaderField)), >> \ >> + &((Struct)->HeaderField) + 1 >> \ >> + ) > > There is a lot of assumptions about inputs with usage of this macro. > (Even the in/out offset on ReadConfigSpace seems a little unexpected.) > > Based on that, I was hoping to come up with some suggestion. The two > thoughts I had seemed more appropriate for a more generic PCI helper > lib than for this driver: > > * A 'reader' struct that bundles the state of the pci address, read > buffer and offset read. > > * More generically, a capabilities helper lib that could iterate & > read the PCI capability structs.
*Absolutely* this ^^^ I've been much missing a canned function that just searches the config space (conventional or extended, as requested) for a capability, and returns an offset. Possibly taking an expected size parameter too, for safety. PciBusDxe has such function(s), but they are not factored out. I wondered if such a function (or a family of functions) should be part of the PciLib class, but seeing the number of PciLib implementations, I didn't even try. :/ I don't know if a standalone PciCapabilityLib class would be acceptable, if it had (say) 3-4 functions in total. The IoFifo experience doesn't seem to support that -- the IoFifo functions were ultimately added to the IoLib class. Also, I think I've implemented two config space scanning functions in my career :) ; we'd probably want someone with more such experience to *design* the lib class. > > Anyway, for this series: > > Reviewed-by: Jordan Justen <[email protected]> Thank you! Laszlo >> + >> + >> +/** >> + Look up the QEMU-specific Resource Reservation capability in the >> conventional >> + config space of a Hotplug Controller (that is, PCI Bridge). >> + >> + This function performs as few config space reads as possible. >> + >> + @param[in] HpcPciAddress The address of the PCI Bridge -- Bus, Device, >> + Function -- in UEFI (not PciLib) encoding. >> + >> + @param[out] ReservationHint The caller-allocated capability structure to >> + populate from the PCI Bridge's config space. >> + >> + @retval EFI_SUCCESS The capability has been found, ReservationHint has >> + been populated. >> + >> + @retval EFI_NOT_FOUND The capability is missing. The contents of >> + ReservationHint are now indeterminate. >> +**/ >> +STATIC >> +EFI_STATUS >> +QueryReservationHint ( >> + IN CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *HpcPciAddress, >> + OUT QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION *ReservationHint >> +) >> +{ >> + UINT16 PciVendorId; >> + UINT16 PciStatus; >> + UINT8 PciCapPtr; >> + UINT8 Offset; >> + >> + // >> + // Check the vendor identifier. >> + // >> + PciVendorId = PciRead16 ( >> + PCI_LIB_ADDRESS ( >> + HpcPciAddress->Bus, >> + HpcPciAddress->Device, >> + HpcPciAddress->Function, >> + PCI_VENDOR_ID_OFFSET >> + ) >> + ); >> + if (PciVendorId != QEMU_PCI_BRIDGE_VENDOR_ID_REDHAT) { >> + return EFI_NOT_FOUND; >> + } >> + >> + // >> + // Check the Capabilities List bit in the PCI Status Register. >> + // >> + PciStatus = PciRead16 ( >> + PCI_LIB_ADDRESS ( >> + HpcPciAddress->Bus, >> + HpcPciAddress->Device, >> + HpcPciAddress->Function, >> + PCI_PRIMARY_STATUS_OFFSET >> + ) >> + ); >> + if ((PciStatus & EFI_PCI_STATUS_CAPABILITY) == 0) { >> + return EFI_NOT_FOUND; >> + } >> + >> + // >> + // Fetch the start of the Capabilities List. >> + // >> + PciCapPtr = PciRead8 ( >> + PCI_LIB_ADDRESS ( >> + HpcPciAddress->Bus, >> + HpcPciAddress->Device, >> + HpcPciAddress->Function, >> + PCI_CAPBILITY_POINTER_OFFSET >> + ) >> + ); >> + >> + // >> + // Scan the Capabilities List until we find the terminator element, or the >> + // Resource Reservation capability. >> + // >> + for (Offset = PciCapPtr & 0xFC; >> + Offset > 0; >> + Offset = ReservationHint->BridgeHdr.VendorHdr.Hdr.NextItemPtr & >> 0xFC) { >> + BOOLEAN EnoughRoom; >> + >> + // >> + // Check if the Resource Reservation capability would fit into config >> space >> + // at this offset. >> + // >> + EnoughRoom = (BOOLEAN)( >> + Offset <= PCI_MAX_CONFIG_OFFSET - sizeof *ReservationHint >> + ); >> + >> + // >> + // Read the standard capability header so we can check the capability ID >> + // (if necessary) and advance to the next capability. >> + // >> + ReadConfigSpace ( >> + HpcPciAddress, >> + &Offset, >> + (UINT8)sizeof ReservationHint->BridgeHdr.VendorHdr.Hdr, >> + &ReservationHint->BridgeHdr.VendorHdr.Hdr >> + ); >> + if (!EnoughRoom || >> + (ReservationHint->BridgeHdr.VendorHdr.Hdr.CapabilityID != >> + EFI_PCI_CAPABILITY_ID_VENDOR)) { >> + continue; >> + } >> + >> + // >> + // Read the rest of the vendor capability header so we can check the >> + // capability length. >> + // >> + COMPLETE_CONFIG_SPACE_STRUCT ( >> + HpcPciAddress, >> + &Offset, >> + &ReservationHint->BridgeHdr.VendorHdr, >> + Hdr >> + ); >> + if (ReservationHint->BridgeHdr.VendorHdr.Length != >> + sizeof *ReservationHint) { >> + continue; >> + } >> + >> + // >> + // Read the rest of the QEMU bridge capability header so we can check >> the >> + // capability type. >> + // >> + COMPLETE_CONFIG_SPACE_STRUCT ( >> + HpcPciAddress, >> + &Offset, >> + &ReservationHint->BridgeHdr, >> + VendorHdr >> + ); >> + if (ReservationHint->BridgeHdr.Type != >> + QEMU_PCI_BRIDGE_CAPABILITY_TYPE_RESOURCE_RESERVATION) { >> + continue; >> + } >> + >> + // >> + // Read the body of the reservation hint. >> + // >> + COMPLETE_CONFIG_SPACE_STRUCT ( >> + HpcPciAddress, >> + &Offset, >> + ReservationHint, >> + BridgeHdr >> + ); >> + return EFI_SUCCESS; >> + } >> + >> + return EFI_NOT_FOUND; >> +} >> + >> + >> /** >> Returns a list of root Hot Plug Controllers (HPCs) that require >> initialization during the boot process. >> >> @@ -401,18 +634,21 @@ GetResourcePadding ( >> OUT VOID **Padding, >> OUT EFI_HPC_PADDING_ATTRIBUTES *Attributes >> ) >> { >> + EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *Address; >> BOOLEAN DefaultIo; >> BOOLEAN DefaultMmio; >> RESOURCE_PADDING ReservationRequest; >> EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *FirstResource; >> + EFI_STATUS ReservationHintStatus; >> + QEMU_PCI_BRIDGE_CAPABILITY_RESOURCE_RESERVATION ReservationHint; >> + >> + Address = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddress; >> >> DEBUG_CODE ( >> - EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *Address; >> CHAR16 *DevicePathString; >> >> - Address = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddress; >> DevicePathString = ConvertDevicePathToText (HpcDevicePath, FALSE, >> FALSE); >> >> DEBUG ((EFI_D_VERBOSE, "%a: Address=%02x:%02x.%x DevicePath=%s\n", >> __FUNCTION__, Address->Bus, Address->Device, Address->Function, >> @@ -439,20 +675,143 @@ GetResourcePadding ( >> FirstResource = ReservationRequest.Padding + >> ARRAY_SIZE (ReservationRequest.Padding); >> >> // >> - // (b) Reserve IO space. >> + // Try to get the QEMU-specific Resource Reservation capability. >> // >> + ReservationHintStatus = QueryReservationHint (Address, &ReservationHint); >> + if (!EFI_ERROR (ReservationHintStatus)) { >> + INTN HighBit; >> + >> + DEBUG (( >> + DEBUG_VERBOSE, >> + "%a: BusNumbers=0x%x Io=0x%Lx NonPrefetchable32BitMmio=0x%x\n" >> + "%a: Prefetchable32BitMmio=0x%x Prefetchable64BitMmio=0x%Lx\n", >> + __FUNCTION__, >> + ReservationHint.BusNumbers, >> + ReservationHint.Io, >> + ReservationHint.NonPrefetchable32BitMmio, >> + __FUNCTION__, >> + ReservationHint.Prefetchable32BitMmio, >> + ReservationHint.Prefetchable64BitMmio >> + )); >> + >> + // >> + // (a) Reserve bus numbers. >> + // >> + switch (ReservationHint.BusNumbers) { >> + case 0: >> + // >> + // No reservation needed. >> + // >> + break; >> + case MAX_UINT32: >> + // >> + // Firmware default (unspecified). Treat it as "no reservation >> needed". >> + // >> + break; >> + default: >> + // >> + // Request the specified amount. >> + // >> + --FirstResource; >> + FirstResource->ResType = ACPI_ADDRESS_SPACE_TYPE_BUS; >> + FirstResource->AddrLen = ReservationHint.BusNumbers; >> + break; >> + } >> + >> + // >> + // (b) Reserve IO space. >> + // >> + switch (ReservationHint.Io) { >> + case 0: >> + // >> + // No reservation needed, disable our built-in. >> + // >> + DefaultIo = FALSE; >> + break; >> + case MAX_UINT64: >> + // >> + // Firmware default (unspecified). Stick with our built-in. >> + // >> + break; >> + default: >> + // >> + // Round the specified amount up to the next power of two. If >> rounding is >> + // successful, reserve the rounded value. Fall back to the default >> + // otherwise. >> + // >> + HighBit = HighBitSetRoundUp64 (ReservationHint.Io); >> + if (HighBit != -1) { >> + SetIoPadding (--FirstResource, (UINTN)HighBit); >> + DefaultIo = FALSE; >> + } >> + break; >> + } >> + >> + // >> + // (c) Reserve non-prefetchable MMIO space (32-bit only). >> + // >> + switch (ReservationHint.NonPrefetchable32BitMmio) { >> + case 0: >> + // >> + // No reservation needed, disable our built-in. >> + // >> + DefaultMmio = FALSE; >> + break; >> + case MAX_UINT32: >> + // >> + // Firmware default (unspecified). Stick with our built-in. >> + // >> + break; >> + default: >> + // >> + // Round the specified amount up to the next power of two. If >> rounding is >> + // successful, reserve the rounded value. Fall back to the default >> + // otherwise. >> + // >> + HighBit = HighBitSetRoundUp32 >> (ReservationHint.NonPrefetchable32BitMmio); >> + if (HighBit != -1) { >> + SetMmioPadding (--FirstResource, FALSE, TRUE, (UINTN)HighBit); >> + DefaultMmio = FALSE; >> + } >> + break; >> + } >> + >> + // >> + // (d) Reserve prefetchable MMIO space (either 32-bit or 64-bit, never >> + // both). >> + // >> + // For either space, we treat 0 as "no reservation needed", and the >> maximum >> + // value as "firmware default". The latter is unspecified, and we >> interpret >> + // it as the former. >> + // >> + // Otherwise, round the specified amount up to the next power of two. If >> + // rounding is successful, reserve the rounded value. Do not reserve >> + // prefetchable MMIO space otherwise. >> + // >> + if (ReservationHint.Prefetchable32BitMmio > 0 && >> + ReservationHint.Prefetchable32BitMmio < MAX_UINT32) { >> + HighBit = HighBitSetRoundUp32 (ReservationHint.Prefetchable32BitMmio); >> + if (HighBit != -1) { >> + SetMmioPadding (--FirstResource, TRUE, TRUE, (UINTN)HighBit); >> + } >> + } else if (ReservationHint.Prefetchable64BitMmio > 0 && >> + ReservationHint.Prefetchable64BitMmio < MAX_UINT64) { >> + HighBit = HighBitSetRoundUp64 (ReservationHint.Prefetchable64BitMmio); >> + if (HighBit != -1) { >> + SetMmioPadding (--FirstResource, TRUE, FALSE, (UINTN)HighBit); >> + } >> + } >> + } >> + >> if (DefaultIo) { >> // >> // Request defaults. >> // >> SetIoPadding (--FirstResource, (UINTN)HighBitSetRoundUp64 (512)); >> } >> >> - // >> - // (c) Reserve non-prefetchable MMIO space (32-bit only). >> - // >> if (DefaultMmio) { >> // >> // Request defaults. >> // >> -- >> 2.14.1.3.gb7cf6e02401b >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
