From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=jordan.l.justen@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 668A321F9B6BC for ; Mon, 2 Oct 2017 10:39:52 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP; 02 Oct 2017 10:43:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,470,1500966000"; d="scan'208";a="157939087" Received: from gapokam-mobl2.amr.corp.intel.com (HELO localhost) ([10.254.35.226]) by fmsmga005.fm.intel.com with ESMTP; 02 Oct 2017 10:43:10 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <150696619010.2454.9266470245604846575@jljusten-skl> From: Jordan Justen In-Reply-To: <20170925195824.10866-8-lersek@redhat.com> Cc: Marcel Apfelbaum , Ruiyu Ni References: <20170925195824.10866-1-lersek@redhat.com> <20170925195824.10866-8-lersek@redhat.com> User-Agent: alot/0.6 Date: Mon, 02 Oct 2017 10:43:10 -0700 Subject: Re: [PATCH 7/7] OvmfPkg/PciHotPlugInitDxe: translate QEMU's resource reservation hints X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 02 Oct 2017 17:39:53 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 > 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 > .) > = > * 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 > Cc: Marcel Apfelbaum > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > 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/PciHo= tPlugInitDxe/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/PciHotP= lugInitDxe/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 > +#include > = > #include > #include > #include > #include > #include > +#include > #include > = > #include > #include > @@ -245,8 +247,239 @@ HighBitSetRoundUp64 ( > return (HighBit < 64) ? HighBit : -1; > } > = > = > +/** > + Read a slice from conventional PCI config space at the given offset, t= hen > + advance the offset. > + > + @param[in] PciAddress The address of the PCI Device -- Bus, Device, F= unction > + -- 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 +=3D 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 con= fig > + space of the PCI device identified by PciAddress, > + > + - *Offset points one past HeaderField in the conventional PCI config s= pace of > + the PCI device identified by PciAddress, > + > + populate the rest of *Struct from conventional PCI config space, start= ing at > + *Offset. Finally, increment *Offset so that it point one past *Struct. > + > + @param[in] PciAddress The address of the PCI Device -- Bus, Device, F= unction > + -- in UEFI (not PciLib) encoding. Type: pointer= to > + CONST EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRE= SS. > + > + @param[in,out] Offset On input, the offset in conventional PCI config= space > + to start reading from; one past Struct->HeaderF= ield. > + On output, the offset of the first byte that wa= s not > + read; one past *Struct. Type: pointer to UINT8. > + > + @param[out] Struct The structure to complete. Type: pointer to str= ucture > + object. > + > + @param[in] HeaderField The name of the first field in *Struct, after w= hich > + *Struct should be populated. Type: structure me= mber > + identifier. > +**/ > +#define COMPLETE_CONFIG_SPACE_STRUCT(PciAddress, Offset, Struct, HeaderF= ield) \ > + 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. Anyway, for this series: Reviewed-by: Jordan Justen > + > + > +/** > + Look up the QEMU-specific Resource Reservation capability in the conve= ntional > + 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, Dev= ice, > + Function -- in UEFI (not PciLib) encoding. > + > + @param[out] ReservationHint The caller-allocated capability structure= to > + populate from the PCI Bridge's config spa= ce. > + > + @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 =3D PciRead16 ( > + PCI_LIB_ADDRESS ( > + HpcPciAddress->Bus, > + HpcPciAddress->Device, > + HpcPciAddress->Function, > + PCI_VENDOR_ID_OFFSET > + ) > + ); > + if (PciVendorId !=3D QEMU_PCI_BRIDGE_VENDOR_ID_REDHAT) { > + return EFI_NOT_FOUND; > + } > + > + // > + // Check the Capabilities List bit in the PCI Status Register. > + // > + PciStatus =3D PciRead16 ( > + PCI_LIB_ADDRESS ( > + HpcPciAddress->Bus, > + HpcPciAddress->Device, > + HpcPciAddress->Function, > + PCI_PRIMARY_STATUS_OFFSET > + ) > + ); > + if ((PciStatus & EFI_PCI_STATUS_CAPABILITY) =3D=3D 0) { > + return EFI_NOT_FOUND; > + } > + > + // > + // Fetch the start of the Capabilities List. > + // > + PciCapPtr =3D 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 =3D PciCapPtr & 0xFC; > + Offset > 0; > + Offset =3D ReservationHint->BridgeHdr.VendorHdr.Hdr.NextItemPtr &= 0xFC) { > + BOOLEAN EnoughRoom; > + > + // > + // Check if the Resource Reservation capability would fit into confi= g space > + // at this offset. > + // > + EnoughRoom =3D (BOOLEAN)( > + Offset <=3D PCI_MAX_CONFIG_OFFSET - sizeof *Reservati= onHint > + ); > + > + // > + // Read the standard capability header so we can check the capabilit= y 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 !=3D > + 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 !=3D > + sizeof *ReservationHint) { > + continue; > + } > + > + // > + // Read the rest of the QEMU bridge capability header so we can chec= k the > + // capability type. > + // > + COMPLETE_CONFIG_SPACE_STRUCT ( > + HpcPciAddress, > + &Offset, > + &ReservationHint->BridgeHdr, > + VendorHdr > + ); > + if (ReservationHint->BridgeHdr.Type !=3D > + 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 =3D (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAddr= ess; > = > DEBUG_CODE ( > - EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *Address; > CHAR16 *DevicePathString; > = > - Address =3D (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS *)&HpcPciAd= dress; > DevicePathString =3D ConvertDevicePathToText (HpcDevicePath, FALSE, = FALSE); > = > DEBUG ((EFI_D_VERBOSE, "%a: Address=3D%02x:%02x.%x DevicePath=3D%s\n= ", > __FUNCTION__, Address->Bus, Address->Device, Address->Function, > @@ -439,20 +675,143 @@ GetResourcePadding ( > FirstResource =3D ReservationRequest.Padding + > ARRAY_SIZE (ReservationRequest.Padding); > = > // > - // (b) Reserve IO space. > + // Try to get the QEMU-specific Resource Reservation capability. > // > + ReservationHintStatus =3D QueryReservationHint (Address, &ReservationH= int); > + if (!EFI_ERROR (ReservationHintStatus)) { > + INTN HighBit; > + > + DEBUG (( > + DEBUG_VERBOSE, > + "%a: BusNumbers=3D0x%x Io=3D0x%Lx NonPrefetchable32BitMmio=3D0x%x\= n" > + "%a: Prefetchable32BitMmio=3D0x%x Prefetchable64BitMmio=3D0x%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 nee= ded". > + // > + break; > + default: > + // > + // Request the specified amount. > + // > + --FirstResource; > + FirstResource->ResType =3D ACPI_ADDRESS_SPACE_TYPE_BUS; > + FirstResource->AddrLen =3D ReservationHint.BusNumbers; > + break; > + } > + > + // > + // (b) Reserve IO space. > + // > + switch (ReservationHint.Io) { > + case 0: > + // > + // No reservation needed, disable our built-in. > + // > + DefaultIo =3D 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 roun= ding is > + // successful, reserve the rounded value. Fall back to the default > + // otherwise. > + // > + HighBit =3D HighBitSetRoundUp64 (ReservationHint.Io); > + if (HighBit !=3D -1) { > + SetIoPadding (--FirstResource, (UINTN)HighBit); > + DefaultIo =3D FALSE; > + } > + break; > + } > + > + // > + // (c) Reserve non-prefetchable MMIO space (32-bit only). > + // > + switch (ReservationHint.NonPrefetchable32BitMmio) { > + case 0: > + // > + // No reservation needed, disable our built-in. > + // > + DefaultMmio =3D 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 roun= ding is > + // successful, reserve the rounded value. Fall back to the default > + // otherwise. > + // > + HighBit =3D HighBitSetRoundUp32 (ReservationHint.NonPrefetchable32= BitMmio); > + if (HighBit !=3D -1) { > + SetMmioPadding (--FirstResource, FALSE, TRUE, (UINTN)HighBit); > + DefaultMmio =3D FALSE; > + } > + break; > + } > + > + // > + // (d) Reserve prefetchable MMIO space (either 32-bit or 64-bit, nev= er > + // both). > + // > + // For either space, we treat 0 as "no reservation needed", and the = maximum > + // value as "firmware default". The latter is unspecified, and we in= terpret > + // 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 =3D HighBitSetRoundUp32 (ReservationHint.Prefetchable32Bit= Mmio); > + if (HighBit !=3D -1) { > + SetMmioPadding (--FirstResource, TRUE, TRUE, (UINTN)HighBit); > + } > + } else if (ReservationHint.Prefetchable64BitMmio > 0 && > + ReservationHint.Prefetchable64BitMmio < MAX_UINT64) { > + HighBit =3D HighBitSetRoundUp64 (ReservationHint.Prefetchable64Bit= Mmio); > + if (HighBit !=3D -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 >=20