[PATCH edk2-platforms] Platform/OverdriveBoard: work around network ConnectAll() dependency

[PATCH edk2-platforms] Platform/OverdriveBoard: work around network ConnectAll() dependency

[Ard Biesheuvel]

The AMD Seattle based platforms have been kept up to date in recent
years, even though the hardware is obsolete and was never available
that widely in the first place.

However, one aspect that has sadly been left behind is the support for
the builtin network controllers. These are only wired up and enabled
on the Overdrive board to begin with, and the driver was only made
available as two separate binary blobs implementing the SNP protocol
for each controller separately, without taking the UEFI driver model
into account.

Even worse, the PHY initialization code that needs to run at boot in
order for the OS to be able to use the device never executes unless
the upper networking layers start the SNP protocol, which doesn't
happen on a fast boot (one that does not use ConnectAll()) unless the
boot target is a network device path.

We cannot fix the driver, but fortunately, there is another way out:
protocols that are installed on a handle during the execution of the
entrypoint of a driver will be connected by the DXE core, and so we
can ensure that the old behavior is retained regardless of whether
ConnectAll() is ever invoked, by reordering the load sequence so that
the upper layer drivers have all been registered by the time the
entrypoints of the SNP drivers are called.

This relies on FV contents to be dispatched in the order they appear
in the .FDF file. The AMD SNP driver as well as the upper layer
drivers in NetworkPkg are UEFI_DRIVER modules, which means their
DEPEXes are implicitly defined as the full set of architectural
PI protocols. This means that all these modules become available for
dispatch at the same time, and their dispatch order is fully defined
by the order of appearance in the FV. Unfortunately, this is an
implementation detail rather than something that is supported by the
PI spec, but this is unlikely to ever change since other platforms
undoubtedly exist that depend on this behavior as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---

This is a preparatory patch to ensure that this platform does not break
when core EDK2 drops the EfiBootManagerConnectAll() from the ArmPkg
version of PlatformBootManagerLib.

 Platform/AMD/OverdriveBoard/OverdriveBoard.fdf | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.fdf b/Platform/AMD/OverdriveBoard/OverdriveBoard.fdf
index 851ae65b5be9..15b5b1bc317f 100644
--- a/Platform/AMD/OverdriveBoard/OverdriveBoard.fdf
+++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.fdf
@@ -178,18 +178,18 @@ [FV.FvMain]
   INF MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
   INF MdeModulePkg/Bus/Usb/UsbKbDxe/UsbKbDxe.inf
 
-  #
-  # SNP support
-  #
-  INF Silicon/AMD/Styx/AmdModulePkg/SnpDxe/SnpDxePort0.inf
-  INF Silicon/AMD/Styx/AmdModulePkg/SnpDxe/SnpDxePort1.inf
-
   #
   # Networking stack
   #
 !include NetworkPkg/Network.fdf.inc
   INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 
+  #
+  # SNP support
+  #
+  INF Silicon/AMD/Styx/AmdModulePkg/SnpDxe/SnpDxePort0.inf
+  INF Silicon/AMD/Styx/AmdModulePkg/SnpDxe/SnpDxePort1.inf
+
   #
   # Core Info
   #
-- 
2.26.2

Re: [edk2-devel] [PATCH edk2-platforms] Platform/OverdriveBoard: work around network ConnectAll() dependency

[Leif Lindholm]

On Fri, May 29, 2020 at 23:40:19 +0200, Ard Biesheuvel wrote:
> The AMD Seattle based platforms have been kept up to date in recent
> years, even though the hardware is obsolete and was never available
> that widely in the first place.
> 
> However, one aspect that has sadly been left behind is the support for
> the builtin network controllers. These are only wired up and enabled
> on the Overdrive board to begin with, and the driver was only made
> available as two separate binary blobs implementing the SNP protocol
> for each controller separately, without taking the UEFI driver model
> into account.
> 
> Even worse, the PHY initialization code that needs to run at boot in
> order for the OS to be able to use the device never executes unless
> the upper networking layers start the SNP protocol, which doesn't
> happen on a fast boot (one that does not use ConnectAll()) unless the
> boot target is a network device path.
> 
> We cannot fix the driver, but fortunately, there is another way out:
> protocols that are installed on a handle during the execution of the
> entrypoint of a driver will be connected by the DXE core, and so we
> can ensure that the old behavior is retained regardless of whether
> ConnectAll() is ever invoked, by reordering the load sequence so that
> the upper layer drivers have all been registered by the time the
> entrypoints of the SNP drivers are called.
> 
> This relies on FV contents to be dispatched in the order they appear
> in the .FDF file. The AMD SNP driver as well as the upper layer
> drivers in NetworkPkg are UEFI_DRIVER modules, which means their
> DEPEXes are implicitly defined as the full set of architectural
> PI protocols. This means that all these modules become available for
> dispatch at the same time, and their dispatch order is fully defined
> by the order of appearance in the FV. Unfortunately, this is an
> implementation detail rather than something that is supported by the
> PI spec, but this is unlikely to ever change since other platforms
> undoubtedly exist that depend on this behavior as well.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

I'm holding my nose a bit, but:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> ---
> 
> This is a preparatory patch to ensure that this platform does not break
> when core EDK2 drops the EfiBootManagerConnectAll() from the ArmPkg
> version of PlatformBootManagerLib.
> 
>  Platform/AMD/OverdriveBoard/OverdriveBoard.fdf | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Platform/AMD/OverdriveBoard/OverdriveBoard.fdf b/Platform/AMD/OverdriveBoard/OverdriveBoard.fdf
> index 851ae65b5be9..15b5b1bc317f 100644
> --- a/Platform/AMD/OverdriveBoard/OverdriveBoard.fdf
> +++ b/Platform/AMD/OverdriveBoard/OverdriveBoard.fdf
> @@ -178,18 +178,18 @@ [FV.FvMain]
>    INF MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
>    INF MdeModulePkg/Bus/Usb/UsbKbDxe/UsbKbDxe.inf
>  
> -  #
> -  # SNP support
> -  #
> -  INF Silicon/AMD/Styx/AmdModulePkg/SnpDxe/SnpDxePort0.inf
> -  INF Silicon/AMD/Styx/AmdModulePkg/SnpDxe/SnpDxePort1.inf
> -
>    #
>    # Networking stack
>    #
>  !include NetworkPkg/Network.fdf.inc
>    INF MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
>  
> +  #
> +  # SNP support
> +  #
> +  INF Silicon/AMD/Styx/AmdModulePkg/SnpDxe/SnpDxePort0.inf
> +  INF Silicon/AMD/Styx/AmdModulePkg/SnpDxe/SnpDxePort1.inf
> +
>    #
>    # Core Info
>    #
> -- 
> 2.26.2
> 
> 
> 
> 

Re: [edk2-devel] [PATCH edk2-platforms] Platform/OverdriveBoard: work around network ConnectAll() dependency

[Ard Biesheuvel]

On 6/1/20 3:19 PM, Leif Lindholm wrote:
> On Fri, May 29, 2020 at 23:40:19 +0200, Ard Biesheuvel wrote:
>> The AMD Seattle based platforms have been kept up to date in recent
>> years, even though the hardware is obsolete and was never available
>> that widely in the first place.
>>
>> However, one aspect that has sadly been left behind is the support for
>> the builtin network controllers. These are only wired up and enabled
>> on the Overdrive board to begin with, and the driver was only made
>> available as two separate binary blobs implementing the SNP protocol
>> for each controller separately, without taking the UEFI driver model
>> into account.
>>
>> Even worse, the PHY initialization code that needs to run at boot in
>> order for the OS to be able to use the device never executes unless
>> the upper networking layers start the SNP protocol, which doesn't
>> happen on a fast boot (one that does not use ConnectAll()) unless the
>> boot target is a network device path.
>>
>> We cannot fix the driver, but fortunately, there is another way out:
>> protocols that are installed on a handle during the execution of the
>> entrypoint of a driver will be connected by the DXE core, and so we
>> can ensure that the old behavior is retained regardless of whether
>> ConnectAll() is ever invoked, by reordering the load sequence so that
>> the upper layer drivers have all been registered by the time the
>> entrypoints of the SNP drivers are called.
>>
>> This relies on FV contents to be dispatched in the order they appear
>> in the .FDF file. The AMD SNP driver as well as the upper layer
>> drivers in NetworkPkg are UEFI_DRIVER modules, which means their
>> DEPEXes are implicitly defined as the full set of architectural
>> PI protocols. This means that all these modules become available for
>> dispatch at the same time, and their dispatch order is fully defined
>> by the order of appearance in the FV. Unfortunately, this is an
>> implementation detail rather than something that is supported by the
>> PI spec, but this is unlikely to ever change since other platforms
>> undoubtedly exist that depend on this behavior as well.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> I'm holding my nose a bit, but:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> 


I know :-)

Pushed as 747eeac8a8b9..608d71ec9396