Re: [edk2-platforms][PATCH 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address

["Pete Batard" <pete@akeo.ie>]

On 2020.01.23 13:50, Ard Biesheuvel wrote:
> On Thu, 23 Jan 2020 at 13:00, Pete Batard <pete@akeo.ie> wrote:
>>
>> The Genet driver stub used by the Raspberry Pi 4 platform is
>> designed to set the MAC address according to a PCD.
>>
>> To be able to set that PCD at runtime, by using the Raspberry
>> Pi firmware interface, that has a dedicated call to retrieve
>> the MAC address, and satisfy driver dependencies in a generic
>> manner, we create a new PlatformPcdLib that can be referenced
>> by the Genet driver, to set the MAC PCD before use there.
>>
>> While it is currently only tailored around MAC PCD population
>> for Genet, we do expect this PCD library to be extended in the
>> future, to provide additional PCD facilities for other drivers.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>   Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c   | 61 ++++++++++++++++++++
>>   Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf | 44 ++++++++++++++
>>   2 files changed, 105 insertions(+)
>>
>> diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c
>> new file mode 100644
>> index 000000000000..78f3679e2e48
>> --- /dev/null
>> +++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.c
>> @@ -0,0 +1,61 @@
>> +/** @file
>> + *
>> + *  Copyright (c) 2020, Pete Batard <pete@akeo.ie>
>> + *
>> + *  SPDX-License-Identifier: BSD-2-Clause-Patent
>> + *
>> + **/
>> +
>> +#include <Library/DebugLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/PrintLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiLib.h>
>> +#include <Library/UefiRuntimeServicesTableLib.h>
>> +#include <Protocol/RpiFirmware.h>
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +PlatformPcdLibConstructor (
>> +  IN EFI_HANDLE ImageHandle,
>> +  IN EFI_SYSTEM_TABLE *SystemTable
>> +  )
>> +{
>> +
>> +  //
>> +  // If Genet is in use and a MAC address PCD has not been set, do it here.
>> +  //
>> +#if (FixedPcdGet64 (PcdBcmGenetRegistersAddress) != 0)
> 
> Can we drop this check? Why would you include this library in your
> .DSC and set the fixed PCD to 0?

Well, the idea is that we may use this PCD library on the Pi 3 as well, 
if we want to use a similar approach to this for other features, in 
which case PcdBcmGenetRegistersAddress will be 0.

The fact that this library resides in Platform/RaspberryPi/Library/ 
rather than Platform/RaspberryPi/RPi4/Library/ means that it is designed 
to be used on more than the Pi 4 platform if needed.

> 
>> +  EFI_STATUS                       Status;
>> +  UINT64                           MacAddr;
>> +  RASPBERRY_PI_FIRMWARE_PROTOCOL   *mFwProtocol;
>> +
>> +  if (PcdGet64 (PcdBcmGenetMacAddress) == 0) {
> 
> This is policy, which we don't need today, and may well be needed
> elsewhere once we do need it. So I'd prefer it if we just drop this
> check.

I'm not sure I understand your point.

This patchset is designed around the idea that someone may also provide 
the MAC address at build time to force a specific MAC on their platform.

I can see specific scenarios for such a use (e.g. someone using Pi 4's 
in a environment where they assign a specific IP according to the MAC 
and planning for drop-in replacement in case of hardware failures) and 
even outside of these, I can see people wanting for force a MAC that is 
different than the hardware's (e.g. could also be handy for spoofing an 
existing network device with an inexpensive Pi 4), without having to 
touch the code. Granted, they should be able to perform the same in high 
level OS, but once we add a full NIC driver in UEFI, we may announce a 
different MAC prior to OS network init, and I think, since we can do it 
here, and it will be picked up by the OS, we might as well do so since 
it's easy to add.

As such, if someone did set the MAC PCD at build time, we shouldn't 
override it.

> 
>> +    Status = gBS->LocateProtocol (&gRaspberryPiFirmwareProtocolGuid, NULL,
>> +                    (VOID**)&mFwProtocol);
>> +    ASSERT_EFI_ERROR(Status);
>> +
>> +    //
>> +    // Get the MAC address from the firmware
>> +    //
>> +    Status = mFwProtocol->GetMacAddress ((UINT8*) &MacAddr);
>> +    if (EFI_ERROR (Status)) {
>> +      DEBUG ((DEBUG_WARN, "%a: failed to retrieve MAC address\n", __FUNCTION__));
>> +    } else {
>> +      PcdSet64 (PcdBcmGenetMacAddress, MacAddr);
> 
> Please use PcdSet64S () instead - the unsafe ones are deprecated AFAIR

Will do.

> 
>> +    }
>> +  }
>> +#endif
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +PlatformPcdLibDestructor (
>> +  IN EFI_HANDLE ImageHandle,
>> +  IN EFI_SYSTEM_TABLE *SystemTable
>> +  )
>> +{
>> +  return EFI_SUCCESS;
>> +}
> 
> No need for a destructor

Yeah, I wasn't sure if that worked without one, and PlatformUiAppLib has 
a similar empty destructor, so I preferred to ensure it was present.

I'll remove it form the patchset.

> 
>> diff --git a/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf
>> new file mode 100644
>> index 000000000000..a564ddfbb2a3
>> --- /dev/null
>> +++ b/Platform/RaspberryPi/Library/PlatformPcdLib/PlatformPcdLib.inf
>> @@ -0,0 +1,44 @@
>> +#/** @file
>> +#
>> +#  Copyright (c) 2020, Pete Batard <pete@akeo.ie>
>> +#
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +#**/
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x0001001A
>> +  BASE_NAME                      = PlatformPcdLib
>> +  FILE_GUID                      = 3B8409D7-D3C7-4006-823B-BFB184435363
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = NULL|DXE_DRIVER UEFI_APPLICATION
>> +  CONSTRUCTOR                    = PlatformPcdLibConstructor
>> +  DESTRUCTOR                     = PlatformPcdLibDestructor
>> +
>> +[Sources]
>> +  PlatformPcdLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  MdeModulePkg/MdeModulePkg.dec
>> +  Platform/RaspberryPi/RaspberryPi.dec
>> +  Silicon/Broadcom/Drivers/Net/BcmNet.dec
>> +
>> +[LibraryClasses]
>> +  UefiLib
>> +  DebugLib
>> +  PrintLib
>> +
>> +[Protocols]
>> +  gRaspberryPiFirmwareProtocolGuid              ## CONSUMES
>> +
>> +[Pcd]
>> +  gBcmNetTokenSpaceGuid.PcdBcmGenetMacAddress   ## SOMETIMES_PRODUCES
>> +
>> +[FixedPcd]
>> +  gBcmNetTokenSpaceGuid.PcdBcmGenetRegistersAddress
>> +
>> +[Depex]
>> +  gPcdProtocolGuid AND gRaspberryPiFirmwareProtocolGuid
>> +
> 
> Use the PcdLib library class instead - this will depex on the
> gPcdProtocolGuid if needed.
> 
> I don't mind this approach, but in general, I'd be happier with a
> specific library class to discover the MAC address.
> 
> I.e., a library class in BcmNet.dec called BcmGenetPlatformLib, which
> has [for now] a single function called BcmGenetGetMacFromPlatform().
> The RPi4 implementation would provide that routine, and depex on the
> firmware protocol, ensuring the ordering is correct. That way, we can
> get rid of the PCDs entirely.

I get your point. But I'd rather not do that, since I do see some use 
for a PCD provided MAC.

Regards,

/Pete