From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web10.8681.1579791702720417538 for ; Thu, 23 Jan 2020 07:01:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=Vv/igH8K; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.67, mailfrom: pete@akeo.ie) Received: by mail-wr1-f67.google.com with SMTP id g17so3445209wro.2 for ; Thu, 23 Jan 2020 07:01:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=mGk3UNEvo9sra6dCQGMBzd3YcdxWp0MeYFzIqLV1XVM=; b=Vv/igH8K1wk6HAwNgAMIEINwMRfXhUQlprUT47o72nzZZ+pMQ2/tZzajTbnl3eFl5S slJdfe3fSOnsVcOsmCq/ukllOHfJwJeAJhG/YaJMXZ2np2RoiLf+0YRWX/QjxxAmTrh6 cp4loWx2IdiFAKLHUxsoAldUIJXOealGphQu1fHcGsIkJZILnz6eWzN0HZMlKBMd9Kko rEKlZeQblrOZ0iUz6+frZnt+XTJgpg/epU/WaFa1DaZWIpJdUZdRvK0lF0tXLplBWQrP OyqVMGQvKIa8eWZRqBjBN8Si+nTBAl7O0F+0WG/NMaWwk+IdCxbGMo3wbRefioYFa7AH qCIA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=mGk3UNEvo9sra6dCQGMBzd3YcdxWp0MeYFzIqLV1XVM=; b=d9++s6JomV7vEPcczMoCT5C9NrK9OzDaaFAtpDkw0/8uo4vYWTXWMPQGpP61qMCX9U D7soG+5vlrT1I4VDo7hfGpEL0xJqiG44tzFKLEuRLS+Q3jCvTNdaPJjxVlZTmPDjFTRu tTZGzUQkz+nGRpK1/Lad5aDGp86GUtRtkij2v9sByEi3mxB8IVnCKobrQPv/wPEXwt+y 9sq5QuVCp1JX7fv5ky/5esNdBW34JlmzvoTzFpGKvszH7orT6E5uAGhmoyy2qI0TdBRw CuntQoW3eTYl0ZDdf9piicMdU7coIQ5a7mTYPhYx/dsjKW7J86NX7cVgqAl08Tra2YAw cy7w== X-Gm-Message-State: APjAAAVwoJvvf+PaZ4l6HnrtZt7QWyCo+JCv/QBhH5/zhSs01uADn35t VgBJ3yS+/4xKdYAHAEhJsUaI1g== X-Google-Smtp-Source: APXvYqxJo5UeNYjiL5GX6Ns3izKoGZcJ91uhcPwOlRSycR9ieNt4uKH+2BSEr6OKOPOPNjNN5qUvpw== X-Received: by 2002:a5d:6a10:: with SMTP id m16mr17924492wru.411.1579791701224; Thu, 23 Jan 2020 07:01:41 -0800 (PST) Return-Path: Received: from [10.0.0.122] ([84.203.95.128]) by smtp.googlemail.com with ESMTPSA id v14sm3355917wrm.28.2020.01.23.07.01.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 23 Jan 2020 07:01:40 -0800 (PST) Subject: Re: [edk2-platforms][PATCH 2/3] Platform/RPi: Add PlatformPcdLib to set the Genet MAC address To: Ard Biesheuvel Cc: edk2-devel-groups-io , Leif Lindholm , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Jeremy Linton References: <20200123120007.4784-1-pete@akeo.ie> <20200123120007.4784-3-pete@akeo.ie> From: "Pete Batard" Message-ID: <69df8e67-3f0a-986b-527a-1f937348cccf@akeo.ie> Date: Thu, 23 Jan 2020 15:01:39 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit On 2020.01.23 13:50, Ard Biesheuvel wrote: > On Thu, 23 Jan 2020 at 13:00, Pete Batard 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 >> --- >> 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 >> + * >> + * SPDX-License-Identifier: BSD-2-Clause-Patent >> + * >> + **/ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +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 >> +# >> +# 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