[PATCH edk2-platforms 0/2] SynQuacer: fix driver model integration of NetSec

[PATCH edk2-platforms 0/2] SynQuacer: fix driver model integration of NetSec

[Ard Biesheuvel]

Currently, the way the SynQuacer NetSec driver implements the EFI driver
model is slightly quirky. Two changes are needed to move it closer to the
intent of the driver model, which is the ability to boot using only the
drivers that are required to drive the devices that are needed at boot.
This series does not complete the picture entirely yet for SynQuacer, and
it still relies on the unconditional EfiBootManagerConnectAll() call that
exists in the PlatformBootManagerLib implementation in ArmPkg. This is left
for future work.

Ard Biesheuvel (2):
  Silicon/SynQuacer/PlatformDxe: defer device registration until
    EndOfDxe
  Silicon/SynQuacer/NetsecDxe: move device path to root device

 .../Drivers/Net/NetsecDxe/NetsecDxe.c         | 12 ---
 .../Drivers/Net/NetsecDxe/NetsecDxe.h         |  9 --
 .../Drivers/PlatformDxe/PlatformDxe.c         | 92 +++++++++++++++++--
 .../Drivers/PlatformDxe/PlatformDxe.h         |  1 +
 .../Drivers/PlatformDxe/PlatformDxe.inf       |  3 +
 5 files changed, 86 insertions(+), 31 deletions(-)

-- 
2.17.1

[PATCH edk2-platforms 1/2] Silicon/SynQuacer/PlatformDxe: defer device registration until EndOfDxe

[Ard Biesheuvel]

EDK2 carries an interesting quirk which dates back to the EFI 1.02 days,
where each protocol that is installed onto a handle during the execution
of a DXE driver's entrypoint is connected immediately (in the UEFI driver
model sense) after the entry point returns.

This means that, depending on the order that these drivers happen to end
up being dispatched, we may enter the BDS phase with the device's driver
stack fully connected, even if the device in question is not being used
to boot the machine.

Given the substantial delays that this might incur, let's work around
this 'feature' by deferring the registration of the network and eMMC
controllers to an EndOfDxe event handler.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c   | 37 ++++++++++++++------
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf |  1 +
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
index b6e2789cb9d2..09200a918009 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
@@ -301,6 +301,28 @@ InstallAcpiTables (
   }
 }
 
+STATIC
+VOID
+EFIAPI
+RegisterDevices (
+  EFI_EVENT           Event,
+  VOID                *Context
+  )
+{
+  EFI_HANDLE                      Handle;
+  EFI_STATUS                      Status;
+
+  Handle = NULL;
+  Status = RegisterDevice (&gNetsecNonDiscoverableDeviceGuid, mNetsecDesc,
+             &Handle);
+  ASSERT_EFI_ERROR (Status);
+
+  if (mHiiSettings->EnableEmmc == EMMC_ENABLED) {
+    Status = RegisterEmmc ();
+    ASSERT_EFI_ERROR (Status);
+  }
+}
+
 EFI_STATUS
 EFIAPI
 PlatformDxeEntryPoint (
@@ -315,6 +337,7 @@ PlatformDxeEntryPoint (
   EFI_ACPI_DESCRIPTION_HEADER     *Ssdt;
   UINTN                           SsdtSize;
   UINTN                           Index;
+  EFI_EVENT                       EndOfDxeEvent;
 
   mHiiSettingsVal = PcdGet64 (PcdPlatformSettings);
   mHiiSettings = (SYNQUACER_PLATFORM_VARSTORE_DATA *)&mHiiSettingsVal;
@@ -344,11 +367,6 @@ PlatformDxeEntryPoint (
     }
   }
 
-  Handle = NULL;
-  Status = RegisterDevice (&gNetsecNonDiscoverableDeviceGuid, mNetsecDesc,
-             &Handle);
-  ASSERT_EFI_ERROR (Status);
-
   Handle = NULL;
   Status = RegisterDevice (&gSynQuacerNonDiscoverableRuntimeI2cMasterGuid,
              mI2c0Desc, &Handle);
@@ -387,11 +405,6 @@ PlatformDxeEntryPoint (
   Status = EnableSettingsForm ();
   ASSERT_EFI_ERROR (Status);
 
-  if (mHiiSettings->EnableEmmc == EMMC_ENABLED) {
-    Status = RegisterEmmc ();
-    ASSERT_EFI_ERROR (Status);
-  }
-
   if (mHiiSettings->AcpiPref == ACPIPREF_ACPI) {
     //
     // Load the SSDT tables from a raw section in this FFS file.
@@ -431,5 +444,9 @@ PlatformDxeEntryPoint (
     }
   }
 
+  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL, TPL_NOTIFY, RegisterDevices,
+                  NULL, &gEfiEndOfDxeEventGroupGuid, &EndOfDxeEvent);
+  ASSERT_EFI_ERROR (Status);
+
   return EFI_SUCCESS;
 }
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
index 57f2d071c14e..157f914ea85d 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
@@ -57,6 +57,7 @@ [LibraryClasses]
 [Guids]
   g96BoardsI2c0MasterGuid
   gEdkiiPlatformHasAcpiGuid
+  gEfiEndOfDxeEventGroupGuid
   gEfiHiiPlatformSetupFormsetGuid
   gFdtTableGuid
   gNetsecNonDiscoverableDeviceGuid
-- 
2.17.1

[PATCH edk2-platforms 2/2] Silicon/SynQuacer/NetsecDxe: move device path to root device

[Ard Biesheuvel]

Currently, SynQuacer's platform DXE driver installs a non-discoverable
device protocol onto a new handle to declare the existence of the NetSec
network controller. This protocol is consumed by the NetSec DXE driver
in its implementation of the UEFI driver model supported/start/stop
routines. Only when the driver is started, an instance of the device
path protocol is installed onto the handle, annotating the handle as
supporting the MAC flavor of the messaging device path.

This approach works as long as the non-discoverable driver is always
connected at some point during the boot. However, it breaks the intent
of the UEFI driver model, since it is not possible to defer connection
of the driver to the controller to the point where it is actually being
used.

For instance, the following device path could be attached to a boot
entry to trigger HTTP boot once the entry is chosen:

  MAC(408d5cb1e6fa,1)/IPv4(0.0.0.0,0,0)/Uri()

This only works as expected if some part of this device path resolves
to a controller which can be connected recursively. In other words,
an instance of the EFI device path protocol needs to already exist,
covering at least the first level of the path.

Fix this by moving the instantiation of the device path protocol to
the platform DXE code that instantiates the handle.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c   | 12 ----
 Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h   |  9 ---
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c   | 59 +++++++++++++++++++-
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h   |  1 +
 Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf |  2 +
 5 files changed, 60 insertions(+), 23 deletions(-)

diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c
index a304e02208fa..4e3c4e6c807a 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.c
@@ -1051,21 +1051,10 @@ NetsecInit (
 
   InitializeListHead (&LanDriver->TxBufferList);
 
-  LanDriver->DevicePath.Netsec.Header.Type = MESSAGING_DEVICE_PATH;
-  LanDriver->DevicePath.Netsec.Header.SubType = MSG_MAC_ADDR_DP;
-
-  SetDevicePathNodeLength (&LanDriver->DevicePath.Netsec.Header,
-    sizeof (LanDriver->DevicePath.Netsec));
-  CopyMem (&LanDriver->DevicePath.Netsec.MacAddress,
-    &SnpMode->PermanentAddress, PXE_HWADDR_LEN_ETHER);
-  LanDriver->DevicePath.Netsec.IfType = SnpMode->IfType;
-  SetDevicePathEndNode (&LanDriver->DevicePath.End);
-
   // Initialise the protocol
   Status = gBS->InstallMultipleProtocolInterfaces (
                   &ControllerHandle,
                   &gEfiSimpleNetworkProtocolGuid, Snp,
-                  &gEfiDevicePathProtocolGuid, &LanDriver->DevicePath,
                   NULL);
 
   LanDriver->ControllerHandle = ControllerHandle;
@@ -1111,7 +1100,6 @@ NetsecRelease (
 
   Status = gBS->UninstallMultipleProtocolInterfaces (ControllerHandle,
                   &gEfiSimpleNetworkProtocolGuid, Snp,
-                  &gEfiDevicePathProtocolGuid, &LanDriver->DevicePath,
                   NULL);
   if (EFI_ERROR (Status)) {
     return Status;
diff --git a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h
index c95ff215199d..17a7032f0f41 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h
+++ b/Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.h
@@ -38,13 +38,6 @@ extern EFI_COMPONENT_NAME2_PROTOCOL gNetsecDriverComponentName2;
   NETSEC Information Structure
 ------------------------------------------------------------------------------*/
 
-#pragma pack(1)
-typedef struct {
-  MAC_ADDR_DEVICE_PATH              Netsec;
-  EFI_DEVICE_PATH_PROTOCOL          End;
-} NETSEC_DEVICE_PATH;
-#pragma pack()
-
 typedef struct {
   // Driver signature
   UINT32                            Signature;
@@ -68,8 +61,6 @@ typedef struct {
   EFI_EVENT                         PhyStatusEvent;
 
   NON_DISCOVERABLE_DEVICE           *Dev;
-
-  NETSEC_DEVICE_PATH                DevicePath;
 } NETSEC_DRIVER;
 
 #define NETSEC_SIGNATURE            SIGNATURE_32('n', 't', 's', 'c')
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
index 09200a918009..2030ef7932e0 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.c
@@ -11,6 +11,18 @@
 UINT64                            mHiiSettingsVal;
 SYNQUACER_PLATFORM_VARSTORE_DATA  *mHiiSettings;
 
+#pragma pack (1)
+typedef struct {
+  MAC_ADDR_DEVICE_PATH                MacAddrDevicePath;
+  EFI_DEVICE_PATH_PROTOCOL            End;
+} NETSEC_DEVICE_PATH;
+
+typedef struct {
+  NETSEC_DEVICE_PATH                  DevicePath;
+  NON_DISCOVERABLE_DEVICE             NonDiscoverableDevice;
+} NETSEC_DEVICE;
+#pragma pack ()
+
 typedef struct {
   VENDOR_DEVICE_PATH              VendorDevicePath;
   EFI_DEVICE_PATH_PROTOCOL        End;
@@ -113,6 +125,31 @@ STATIC EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR mI2c1Desc[] = {
   }
 };
 
+STATIC NETSEC_DEVICE  mNetsecDevice = {
+  {
+    {
+      {
+        MESSAGING_DEVICE_PATH,
+        MSG_MAC_ADDR_DP,
+        { sizeof (MAC_ADDR_DEVICE_PATH), 0 },
+      },
+      {},
+      NET_IFTYPE_ETHERNET,
+    },
+    {
+      END_DEVICE_PATH_TYPE,
+      END_ENTIRE_DEVICE_PATH_SUBTYPE,
+      { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
+    }
+  },
+  {
+    &gNetsecNonDiscoverableDeviceGuid,
+    NonDiscoverableDeviceDmaTypeCoherent,
+    NULL,
+    mNetsecDesc
+  }
+};
+
 STATIC EFI_ACPI_DESCRIPTION_HEADER      *mEmmcSsdt;
 STATIC UINTN                            mEmmcSsdtSize;
 
@@ -301,6 +338,20 @@ InstallAcpiTables (
   }
 }
 
+STATIC
+VOID
+NetsecReadMacAddress (
+  OUT   EFI_MAC_ADDRESS     *MacAddress
+  )
+{
+  MacAddress->Addr[0] = MmioRead8 (FixedPcdGet32 (PcdNetsecEepromBase) + 3);
+  MacAddress->Addr[1] = MmioRead8 (FixedPcdGet32 (PcdNetsecEepromBase) + 2);
+  MacAddress->Addr[2] = MmioRead8 (FixedPcdGet32 (PcdNetsecEepromBase) + 1);
+  MacAddress->Addr[3] = MmioRead8 (FixedPcdGet32 (PcdNetsecEepromBase) + 0);
+  MacAddress->Addr[4] = MmioRead8 (FixedPcdGet32 (PcdNetsecEepromBase) + 7);
+  MacAddress->Addr[5] = MmioRead8 (FixedPcdGet32 (PcdNetsecEepromBase) + 6);
+}
+
 STATIC
 VOID
 EFIAPI
@@ -312,9 +363,13 @@ RegisterDevices (
   EFI_HANDLE                      Handle;
   EFI_STATUS                      Status;
 
+  NetsecReadMacAddress (&mNetsecDevice.DevicePath.MacAddrDevicePath.MacAddress);
+
   Handle = NULL;
-  Status = RegisterDevice (&gNetsecNonDiscoverableDeviceGuid, mNetsecDesc,
-             &Handle);
+  Status = gBS->InstallMultipleProtocolInterfaces (&Handle,
+                  &gEfiDevicePathProtocolGuid,              &mNetsecDevice.DevicePath,
+                  &gEdkiiNonDiscoverableDeviceProtocolGuid, &mNetsecDevice.NonDiscoverableDevice,
+                  NULL);
   ASSERT_EFI_ERROR (Status);
 
   if (mHiiSettings->EnableEmmc == EMMC_ENABLED) {
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h
index 692654687869..1cb95121d638 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.h
@@ -22,6 +22,7 @@
 #include <Library/HiiLib.h>
 #include <Library/IoLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/NetLib.h>
 #include <Library/NonDiscoverableDeviceRegistrationLib.h>
 #include <Library/OpteeLib.h>
 #include <Library/PcdLib.h>
diff --git a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
index 157f914ea85d..aa3cbdf35c73 100644
--- a/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
+++ b/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/PlatformDxe.inf
@@ -30,6 +30,7 @@ [Packages]
   EmbeddedPkg/EmbeddedPkg.dec
   MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
+  NetworkPkg/NetworkPkg.dec
   Platform/96Boards/96Boards.dec
   Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.dec
   Silicon/Socionext/SynQuacer/Drivers/Net/NetsecDxe/NetsecDxe.dec
@@ -46,6 +47,7 @@ [LibraryClasses]
   HiiLib
   IoLib
   MemoryAllocationLib
+  NetLib
   NonDiscoverableDeviceRegistrationLib
   OpteeLib
   PcdLib
-- 
2.17.1

Re: [PATCH edk2-platforms 0/2] SynQuacer: fix driver model integration of NetSec

[Leif Lindholm]

On Wed, Apr 08, 2020 at 18:11:36 +0200, Ard Biesheuvel wrote:
> Currently, the way the SynQuacer NetSec driver implements the EFI driver
> model is slightly quirky. Two changes are needed to move it closer to the
> intent of the driver model, which is the ability to boot using only the
> drivers that are required to drive the devices that are needed at boot.
> This series does not complete the picture entirely yet for SynQuacer, and
> it still relies on the unconditional EfiBootManagerConnectAll() call that
> exists in the PlatformBootManagerLib implementation in ArmPkg. This is left
> for future work.
> 
> Ard Biesheuvel (2):
>   Silicon/SynQuacer/PlatformDxe: defer device registration until
>     EndOfDxe
>   Silicon/SynQuacer/NetsecDxe: move device path to root device

LGTM.

For the series:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>


>  .../Drivers/Net/NetsecDxe/NetsecDxe.c         | 12 ---
>  .../Drivers/Net/NetsecDxe/NetsecDxe.h         |  9 --
>  .../Drivers/PlatformDxe/PlatformDxe.c         | 92 +++++++++++++++++--
>  .../Drivers/PlatformDxe/PlatformDxe.h         |  1 +
>  .../Drivers/PlatformDxe/PlatformDxe.inf       |  3 +
>  5 files changed, 86 insertions(+), 31 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH edk2-platforms 0/2] SynQuacer: fix driver model integration of NetSec

[Ard Biesheuvel]

On 4/8/20 6:26 PM, Leif Lindholm wrote:
> On Wed, Apr 08, 2020 at 18:11:36 +0200, Ard Biesheuvel wrote:
>> Currently, the way the SynQuacer NetSec driver implements the EFI driver
>> model is slightly quirky. Two changes are needed to move it closer to the
>> intent of the driver model, which is the ability to boot using only the
>> drivers that are required to drive the devices that are needed at boot.
>> This series does not complete the picture entirely yet for SynQuacer, and
>> it still relies on the unconditional EfiBootManagerConnectAll() call that
>> exists in the PlatformBootManagerLib implementation in ArmPkg. This is left
>> for future work.
>>
>> Ard Biesheuvel (2):
>>    Silicon/SynQuacer/PlatformDxe: defer device registration until
>>      EndOfDxe
>>    Silicon/SynQuacer/NetsecDxe: move device path to root device
> 
> LGTM.
> 
> For the series:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> 
> 

Thanks. Series pushed.