[edk2,v2,11/24] ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol

Message ID 1460108711-12122-12-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 7b6745cc11084ad7f8900a00c922cc112c405ac5
Headers show

Commit Message

Ard Biesheuvel April 8, 2016, 9:44 a.m.
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

Comments

Laszlo Ersek April 8, 2016, 5:20 p.m. | #1
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
Laszlo Ersek April 8, 2016, 5:29 p.m. | #2
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

Patch

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