| Message ID | 1460108711-12122-12-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 7b6745cc11084ad7f8900a00c922cc112c405ac5 |
| Headers | show |
On 04/08/16 11:44, Ard Biesheuvel wrote: > Make this library depend on the FDT client protocol to access the > host supplied device tree directly rather than depending on VirtFdtDxe > to set them using dynamic PCDs. > > Since this library is used by several drivers (BdsDxe, SmbiosDxe and > QemuFwCfgAcpiPlatformDxe), one more, according to my build report file: SmbiosPlatformDxe. ( BdsDxe has the dependency through PlatformBdsLib (for the progress bar timeout and the boot order). SmbiosDxe depends on fw_cfg for setting the entry point version. (Funnily enough, that is implemented already through a plugin library with a constructor, SmbiosVersionLib :) So in the case of SmbiosDxe, QemuFwCfgLib's constructor will first parse the DTB and get access to fw_cfg, then SmbiosVersionLib's constructor will grab the entry point version through fw_cfg and set PCDs.) SmbiosPlatformDxe depends on fw_cfg for the individual SMBIOS tables. (It also depends on SmbiosDxe's protocol, for SMBIOS table installation.) QemuFwCfgAcpiPlatformDxe depends on fw_cfg for the ACPI tables. ) > we will end up parsing the device tree and > the fwcfg node at least three times. Hence, four. > However, no dynamic PCDs are > involved anymore, and will even be removed completely in a subsequent > patch. So the conversion is not optimal, but guaranteed to be safe. I agree it is safe. > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 63 ++++++++++++++++++-- > ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf | 10 ++-- > 2 files changed, 64 insertions(+), 9 deletions(-) > > diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > index 303dc520c6eb..34f31517d0fa 100644 > --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c > @@ -14,12 +14,17 @@ > WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > **/ > > +#include <Uefi.h> > + > #include <Library/BaseLib.h> > #include <Library/BaseMemoryLib.h> > #include <Library/DebugLib.h> > #include <Library/IoLib.h> > #include <Library/PcdLib.h> Please drop the PcdLib.h include. > #include <Library/QemuFwCfgLib.h> > +#include <Library/UefiBootServicesTableLib.h> Please add UefiBootServicesTableLib to [LibraryClasses] too. > + > +#include <Protocol/FdtClient.h> > > STATIC UINTN mFwCfgSelectorAddress; > STATIC UINTN mFwCfgDataAddress; > @@ -115,8 +120,59 @@ QemuFwCfgInitialize ( > VOID > ) > { > - mFwCfgSelectorAddress = (UINTN)PcdGet64 (PcdFwCfgSelectorAddress); > - mFwCfgDataAddress = (UINTN)PcdGet64 (PcdFwCfgDataAddress); > + EFI_STATUS Status; > + FDT_CLIENT_PROTOCOL *FdtClient; > + CONST UINT64 *Reg; > + UINT32 RegElemSize, RegSize; > + UINT64 FwCfgSelectorSize; > + UINT64 FwCfgDataSize; > + UINT64 FwCfgDmaSize; > + > + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, > + (VOID **)&FdtClient); > + ASSERT_EFI_ERROR (Status); > + > + Status = FdtClient->FindCompatibleNodeReg (FdtClient, "qemu,fw-cfg-mmio", > + (CONST VOID **)&Reg, &RegElemSize, &RegSize); > + if (EFI_ERROR (Status)) { > + return Status; > + } Hmmm, this is not good. The fw_cfg library class explicitly allows for the facility to be absent; see the public QemuFwCfgIsAvailable() function. I agree you should return early here, yes, but please return EFI_SUCCESS. Maybe add a DEBUG_WARN. > + > + ASSERT (RegElemSize == sizeof (UINT64)); > + ASSERT (RegSize == 2 * sizeof (UINT64)); > + > + mFwCfgDataAddress = SwapBytes64 (Reg[0]); > + FwCfgDataSize = 8; > + mFwCfgSelectorAddress = mFwCfgDataAddress + FwCfgDataSize; > + FwCfgSelectorSize = 2; Sorry, assigning mFwCfg* is "cart before horse". The static mFwCfg* variables in this library have type UINTN. The point of the ASSERT()s below is precisely to express that the assignment from UINT64 to UINTN will be safe. However, here you do the assignment first. Instead, please copy the FwCfgSelectorAddress, FwCfgDataAddress, and FwCfgDmaAddress local UINT64 variables too from VirtFdtDxe, perform the above computation and the below ASSERT()s on them, and only assign them to the mFwCfg* variables after the respective ASSERT()s. (The assignments to mFwCfg* that you remove from the top of this function, in this version of the patch, are so "cavalier" in the first place because they rely on the ASSERT()s being done in VirtFdtDxe.) > + > + // > + // The following ASSERT()s express > + // > + // Address + Size - 1 <= MAX_UINTN > + // > + // for both registers, that is, that the last byte in each MMIO range is > + // expressible as a MAX_UINTN. The form below is mathematically > + // equivalent, and it also prevents any unsigned overflow before the > + // comparison. > + // > + ASSERT (mFwCfgSelectorAddress <= MAX_UINTN - FwCfgSelectorSize + 1); > + ASSERT (mFwCfgDataAddress <= MAX_UINTN - FwCfgDataSize + 1); > + So, these ASSERT()s should cover FwCfgSelectorAddress and FwCfgDataAddress, and mFwCfgSelectorAddress and mFwCfgDataAddress should be assigned afterwards, at this spot. > + DEBUG ((EFI_D_INFO, "Found FwCfg @ 0x%Lx/0x%Lx\n", mFwCfgSelectorAddress, > + mFwCfgDataAddress)); This is affected too, it would break in a 32-bit build. (0x%Lx is UINT64, mFwCfg* is UINTN.) Hence, please drop the "m". > + > + if (SwapBytes64 (Reg[1]) >= 0x18) { > + mFwCfgDmaAddress = mFwCfgDataAddress + 0x10; Ditto; please drop the "m" from both. > + FwCfgDmaSize = 0x08; > + > + // > + // See explanation above. > + // > + ASSERT (mFwCfgDmaAddress <= MAX_UINTN - FwCfgDmaSize + 1); Please drop the "m". Furthermore, in the original code, PcdFwCfgDmaAddress is set here. However, in this code, we should *not* set mFwCfgDmaAddress just yet. > + > + DEBUG ((EFI_D_INFO, "Found FwCfg DMA @ 0x%Lx\n", mFwCfgDmaAddress)); Please drop the "m". > + } > > if (InternalQemuFwCfgIsAvailable ()) { > UINT32 Signature; > @@ -128,13 +184,12 @@ QemuFwCfgInitialize ( > // For DMA support, we require the DTB to advertise the register, and the > // feature bitmap (which we read without DMA) to confirm the feature. > // > - if (PcdGet64 (PcdFwCfgDmaAddress) != 0) { > + if (mFwCfgDmaAddress != 0) { Please drop the "m". > UINT32 Features; > > QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion); > Features = QemuFwCfgRead32 (); > if ((Features & BIT1) != 0) { > - mFwCfgDmaAddress = PcdGet64 (PcdFwCfgDmaAddress); And this assignment should be preserved, from FwCfgDmaAddress. Basically: - For all three of FwCfgSelectorAddress, FwCfgDataAddress, FwCfgDmaAddress, the computations and the ASSERT()s should be done on these local, UINT64 variables. - For FwCfgSelectorAddress and FwCfgDataAddress, their m* peers should be assigned right after the ASSERT()s -- where VirtFdtDxe has the PcdSet64() calls. Said ASSERT()s suffice to enable the basic (MMIO) functionality. - For FwCfgDmaAddress however, we shouldn't set it's m* peer right after the ASSERT() above -- i.e., where the VirtFdtDxe code has the PcdSet64(). The DMA feature needs more validation. Namely, the function DmaReadBytes, and its underlying "mFwCfgDmaAddress" variable, should be selected / configured shoulder-to-shoulder, right here. > InternalQemuFwCfgReadBytes = DmaReadBytes; > } > } > diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > index 298aa6edfb26..cab42e4fd532 100644 > --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > @@ -46,9 +46,9 @@ [LibraryClasses] > BaseMemoryLib > DebugLib > IoLib > - PcdLib > > -[Pcd] > - gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress > - gArmVirtTokenSpaceGuid.PcdFwCfgDataAddress > - gArmVirtTokenSpaceGuid.PcdFwCfgDmaAddress > +[Protocols] > + gFdtClientProtocolGuid ## CONSUMES > + > +[Depex] > + gFdtClientProtocolGuid > I think I'd like to verify version 3 of this patch from scratch. Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/08/16 19:20, Laszlo Ersek wrote: > On 04/08/16 11:44, Ard Biesheuvel wrote: >> + if (SwapBytes64 (Reg[1]) >= 0x18) { >> + mFwCfgDmaAddress = mFwCfgDataAddress + 0x10; > > Ditto; please drop the "m" from both. > >> + FwCfgDmaSize = 0x08; >> + >> + // >> + // See explanation above. >> + // >> + ASSERT (mFwCfgDmaAddress <= MAX_UINTN - FwCfgDmaSize + 1); > > Please drop the "m". > > Furthermore, in the original code, PcdFwCfgDmaAddress is set here. > However, in this code, we should *not* set mFwCfgDmaAddress just yet. > >> + >> + DEBUG ((EFI_D_INFO, "Found FwCfg DMA @ 0x%Lx\n", mFwCfgDmaAddress)); > > Please drop the "m". > >> + } >> >> if (InternalQemuFwCfgIsAvailable ()) { >> UINT32 Signature; >> @@ -128,13 +184,12 @@ QemuFwCfgInitialize ( >> // For DMA support, we require the DTB to advertise the register, and the >> // feature bitmap (which we read without DMA) to confirm the feature. >> // >> - if (PcdGet64 (PcdFwCfgDmaAddress) != 0) { >> + if (mFwCfgDmaAddress != 0) { > > Please drop the "m". ... and FwCfgDmaAddress should be set to zero if the Reg[1] check, at the top, fails. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c index 303dc520c6eb..34f31517d0fa 100644 --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c @@ -14,12 +14,17 @@ WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. **/ +#include <Uefi.h> + #include <Library/BaseLib.h> #include <Library/BaseMemoryLib.h> #include <Library/DebugLib.h> #include <Library/IoLib.h> #include <Library/PcdLib.h> #include <Library/QemuFwCfgLib.h> +#include <Library/UefiBootServicesTableLib.h> + +#include <Protocol/FdtClient.h> STATIC UINTN mFwCfgSelectorAddress; STATIC UINTN mFwCfgDataAddress; @@ -115,8 +120,59 @@ QemuFwCfgInitialize ( VOID ) { - mFwCfgSelectorAddress = (UINTN)PcdGet64 (PcdFwCfgSelectorAddress); - mFwCfgDataAddress = (UINTN)PcdGet64 (PcdFwCfgDataAddress); + EFI_STATUS Status; + FDT_CLIENT_PROTOCOL *FdtClient; + CONST UINT64 *Reg; + UINT32 RegElemSize, RegSize; + UINT64 FwCfgSelectorSize; + UINT64 FwCfgDataSize; + UINT64 FwCfgDmaSize; + + Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, + (VOID **)&FdtClient); + ASSERT_EFI_ERROR (Status); + + Status = FdtClient->FindCompatibleNodeReg (FdtClient, "qemu,fw-cfg-mmio", + (CONST VOID **)&Reg, &RegElemSize, &RegSize); + if (EFI_ERROR (Status)) { + return Status; + } + + ASSERT (RegElemSize == sizeof (UINT64)); + ASSERT (RegSize == 2 * sizeof (UINT64)); + + mFwCfgDataAddress = SwapBytes64 (Reg[0]); + FwCfgDataSize = 8; + mFwCfgSelectorAddress = mFwCfgDataAddress + FwCfgDataSize; + FwCfgSelectorSize = 2; + + // + // The following ASSERT()s express + // + // Address + Size - 1 <= MAX_UINTN + // + // for both registers, that is, that the last byte in each MMIO range is + // expressible as a MAX_UINTN. The form below is mathematically + // equivalent, and it also prevents any unsigned overflow before the + // comparison. + // + ASSERT (mFwCfgSelectorAddress <= MAX_UINTN - FwCfgSelectorSize + 1); + ASSERT (mFwCfgDataAddress <= MAX_UINTN - FwCfgDataSize + 1); + + DEBUG ((EFI_D_INFO, "Found FwCfg @ 0x%Lx/0x%Lx\n", mFwCfgSelectorAddress, + mFwCfgDataAddress)); + + if (SwapBytes64 (Reg[1]) >= 0x18) { + mFwCfgDmaAddress = mFwCfgDataAddress + 0x10; + FwCfgDmaSize = 0x08; + + // + // See explanation above. + // + ASSERT (mFwCfgDmaAddress <= MAX_UINTN - FwCfgDmaSize + 1); + + DEBUG ((EFI_D_INFO, "Found FwCfg DMA @ 0x%Lx\n", mFwCfgDmaAddress)); + } if (InternalQemuFwCfgIsAvailable ()) { UINT32 Signature; @@ -128,13 +184,12 @@ QemuFwCfgInitialize ( // For DMA support, we require the DTB to advertise the register, and the // feature bitmap (which we read without DMA) to confirm the feature. // - if (PcdGet64 (PcdFwCfgDmaAddress) != 0) { + if (mFwCfgDmaAddress != 0) { UINT32 Features; QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion); Features = QemuFwCfgRead32 (); if ((Features & BIT1) != 0) { - mFwCfgDmaAddress = PcdGet64 (PcdFwCfgDmaAddress); InternalQemuFwCfgReadBytes = DmaReadBytes; } } diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf index 298aa6edfb26..cab42e4fd532 100644 --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf @@ -46,9 +46,9 @@ [LibraryClasses] BaseMemoryLib DebugLib IoLib - PcdLib -[Pcd] - gArmVirtTokenSpaceGuid.PcdFwCfgSelectorAddress - gArmVirtTokenSpaceGuid.PcdFwCfgDataAddress - gArmVirtTokenSpaceGuid.PcdFwCfgDmaAddress +[Protocols] + gFdtClientProtocolGuid ## CONSUMES + +[Depex] + gFdtClientProtocolGuid
Make this library depend on the FDT client protocol to access the host supplied device tree directly rather than depending on VirtFdtDxe to set them using dynamic PCDs. Since this library is used by several drivers (BdsDxe, SmbiosDxe and QemuFwCfgAcpiPlatformDxe), we will end up parsing the device tree and the fwcfg node at least three times. However, no dynamic PCDs are involved anymore, and will even be removed completely in a subsequent patch. So the conversion is not optimal, but guaranteed to be safe. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 63 ++++++++++++++++++-- ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf | 10 ++-- 2 files changed, 64 insertions(+), 9 deletions(-) -- 2.5.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel