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.web11.3776.1570697956669686332 for ; Thu, 10 Oct 2019 01:59:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=GoS/Lr5I; spf=pass (domain: linaro.org, ip: 209.85.221.67, mailfrom: leif.lindholm@linaro.org) Received: by mail-wr1-f67.google.com with SMTP id q9so6825651wrm.8 for ; Thu, 10 Oct 2019 01:59:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=3NnCMcbsk9ChcNLPGnyRUnzuRZWjwOgwgvWxkwSFojY=; b=GoS/Lr5IbV+t8ytotGB6RMpudLzmE4Mxb7CLCnBQeHXNGNJMTPXcIOkiAl7Am0LRdU 8JzXVBuReoVgmIztNnzdQKUJwYRrNiihQKSUhEYWCQEKgif4w9gS4zk9S1+zvSiEDH46 QE0Y19t+cK0ONpVrpWQGBdhaBt6jKKgBibbVu5tGV/Ab1wMYSdbNltenKPtBtr1ke7+d 71HdRnHaKEglmkfM0+5hzwqGgejccX69X8Z0K1TKiYlMzPfK7TpYfeRogHDCPYhkaVYf Sv+JrjrYm+Ptz0Swr0KrrzVnImCL/VBsL7Lx+F6w+1wUt9ieLHDI73c8T06I5k1KO2fJ dSMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=3NnCMcbsk9ChcNLPGnyRUnzuRZWjwOgwgvWxkwSFojY=; b=pVtYzNqYDb6Th3JF3hl2+/eK6IJasgrMCxeeO7+7v4fPbGqQCoyqKL/dt/tUsXGjOG R106QRScSX/zGRJGd/LaMAVTCTv9PGuWfMoWjKO6bvdGe/AyhscSeCUe1enPZiFWOdUP 65PN/XOyK1vXcXWIbn94OKMxdQHuk36bo+zIfyhVoft84Y2+0pqeU5+MD9WYlGNzfq5f YcPGRTIspCB3ooSttZyryvuzThRP2bV++lhN6VUfmhetiPcy3KKDAmnq+IPxJTL/EL2/ 0Ui+PLCsXXFd6OL7k/nK4Pd3uSq6UYxKId29etLK+8nTtaUDIsV+KH4x8+Qv+IexfQWV EabA== X-Gm-Message-State: APjAAAWMSfSzf+YWms6biJPwSJ45rz75/cr3inoHGAL/QUby1xYycXbZ Cz1mI91fMbIZIvkBXDPKLb5ARg== X-Google-Smtp-Source: APXvYqzYViUW2lz4flo3CnrdtPO9QSW0rBBIyffmK+eWjw6nuwty0SUQ/MecsT+yg6Rip6QSCkhtjQ== X-Received: by 2002:a5d:4748:: with SMTP id o8mr7323705wrs.202.1570697955044; Thu, 10 Oct 2019 01:59:15 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id s9sm6348342wme.36.2019.10.10.01.59.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Oct 2019 01:59:14 -0700 (PDT) Date: Thu, 10 Oct 2019 09:59:12 +0100 From: "Leif Lindholm" To: Pete Batard Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org Subject: Re: [edk2-platforms][PATCH v2 3/5] Platform/RPi3/PlatformSmbiosDxe: Improve population of SMBIOS entries Message-ID: <20191010085912.GU25504@bivouac.eciton.net> References: <20191008123841.12952-1-pete@akeo.ie> <20191008123841.12952-4-pete@akeo.ie> MIME-Version: 1.0 In-Reply-To: <20191008123841.12952-4-pete@akeo.ie> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Oct 08, 2019 at 01:38:39PM +0100, Pete Batard wrote: > This patch cleans up the population SMBIOS entries by removing elements > that we don't have data for, as well as properly filling the ones for > which we do, through the newly added queries from RpiFirmwareDxe. > > Additional minor improvements are also applied, such as consistent use > of uppercase values. Like for 2/5: mistake, misunderstanding, or disagreement? I appreciate style fixes, but I appreciate a consistent git history more. > Signed-off-by: Pete Batard > --- > Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c | 153 ++++++++++---------- > Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf | 1 + > Platform/RaspberryPi/RPi3/RPi3.dsc | 2 +- > 3 files changed, 76 insertions(+), 80 deletions(-) > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > index bc35175279f2..8a4840267780 100644 > --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c > @@ -35,12 +35,13 @@ > #include > #include > #include > +#include > #include > #include > #include > #include > > -STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; > +static RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol; Style fix only. > > /*********************************************************************** > SMBIOS data definition TYPE0 BIOS Information > @@ -49,7 +50,7 @@ SMBIOS_TABLE_TYPE0 mBIOSInfoType0 = { > { EFI_SMBIOS_TYPE_BIOS_INFORMATION, sizeof (SMBIOS_TABLE_TYPE0), 0 }, > 1, // Vendor String > 2, // BiosVersion String > - 0x0, // BiosSegment > + 0, // BiosSegment Style change only. > 3, // BiosReleaseDate String > 0x1F, // BiosSize > { // BiosCharacteristics > @@ -97,23 +98,25 @@ SMBIOS_TABLE_TYPE0 mBIOSInfoType0 = { > // Boot1394IsSupported :1; > // SmartBatteryIsSupported :1; > // BIOSCharacteristicsExtensionBytes[1] > - 0x0e, // BiosBootSpecIsSupported :1; > + 0x0E, // BiosBootSpecIsSupported :1; Case change only. > // FunctionKeyNetworkBootIsSupported :1; > // TargetContentDistributionEnabled :1; > // UefiSpecificationSupported :1; > // VirtualMachineSupported :1; > // ExtensionByte2Reserved :3; > }, > - 0xFF, // SystemBiosMajorRelease > - 0xFF, // SystemBiosMinorRelease > - 0xFF, // EmbeddedControllerFirmwareMajorRelease > - 0xFF, // EmbeddedControllerFirmwareMinorRelease > + 0, // SystemBiosMajorRelease > + 0, // SystemBiosMinorRelease > + 0, // EmbeddedControllerFirmwareMajorRelease > + 0, // EmbeddedControllerFirmwareMinorRelease > }; > > +CHAR8 mBiosVersion[128] = "EDK2-DEV"; > + > CHAR8 *mBIOSInfoType0Strings[] = { > - "https://github.com/andreiw/RaspberryPiPkg", // Vendor String > - "Raspberry Pi 64-bit UEFI (" __DATE__ " " __TIME__ ")", // BiosVersion String > - __DATE__, > + "TianoCore", // Vendor Could alternatively use gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor here? I'm not sure "EDK II" (the default value) is a more useful description, but it would make this platform more consistent with others. > + mBiosVersion, // Version > + __DATE__ " " __TIME__, // Release Date > NULL > }; > > @@ -132,42 +135,19 @@ SMBIOS_TABLE_TYPE1 mSysInfoType1 = { > 6, // Family String > }; > > -#define PROD_BASE 8 > -#define PROD_KNOWN 13 > -#define PROD_UNKNOWN 11 > -CHAR8 *ProductNames[] = { > - /* 8 */ "3", > - /* 9 */ "Zero", > - /* 10 */ "CM3", > - /* 11 */ "???", > - /* 12 */ "Zero W", > - /* 13 */ "3B+" > -}; > - > -#define MANU_UNKNOWN 0 > -#define MANU_KNOWN 4 > -#define MANU_BASE 1 > -CHAR8 *ManufNames[] = { > - "???", > - /* 0 */ "Sony", > - /* 1 */ "Egoman", > - /* 2 */ "Embest", > - /* 3 */ "Sony Japan", > - /* 4 */ "Embest" > -}; > - > -CHAR8 mSysInfoManufName[sizeof ("Sony Japan")]; > -CHAR8 mSysInfoProductName[sizeof ("64-bit Raspberry Pi XXXXXX (rev. xxxxxxxx)")]; > +CHAR8 mSysInfoManufName[128]; > +CHAR8 mSysInfoProductName[128]; > +CHAR8 mSysInfoVersionName[128]; > CHAR8 mSysInfoSerial[sizeof (UINT64) * 2 + 1]; > CHAR8 mSysInfoSKU[sizeof (UINT64) * 2 + 1]; > > CHAR8 *mSysInfoType1Strings[] = { > mSysInfoManufName, > mSysInfoProductName, > - mSysInfoProductName, > + mSysInfoVersionName, > mSysInfoSerial, > mSysInfoSKU, > - "edk2", > + "Raspberry Pi", > NULL > }; > > @@ -180,7 +160,7 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = { > 2, // ProductName String > 3, // Version String > 4, // SerialNumber String > - 5, // AssetTag String > + 0, // AssetTag String > { // FeatureFlag > 1, // Motherboard :1; > 0, // RequiresDaughterCard :1; > @@ -189,7 +169,7 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = { > 0, // HotSwappable :1; > 0, // Reserved :3; > }, > - 6, // LocationInChassis String > + 0, // LocationInChassis String > 0, // ChassisHandle; > BaseBoardTypeMotherBoard, // BoardType; > 0, // NumberOfContainedObjectHandles; > @@ -198,10 +178,8 @@ SMBIOS_TABLE_TYPE2 mBoardInfoType2 = { > CHAR8 *mBoardInfoType2Strings[] = { > mSysInfoManufName, > mSysInfoProductName, > - mSysInfoProductName, > + mSysInfoVersionName, > mSysInfoSerial, > - "None", > - mSysInfoSKU, > NULL > }; > > @@ -214,7 +192,7 @@ SMBIOS_TABLE_TYPE3 mEnclosureInfoType3 = { > MiscChassisEmbeddedPc, // Type; > 2, // Version String > 3, // SerialNumber String > - 4, // AssetTag String > + 0, // AssetTag String > ChassisStateSafe, // BootupState; > ChassisStateSafe, // PowerSupplyState; > ChassisStateSafe, // ThermalState; > @@ -230,7 +208,6 @@ CHAR8 *mEnclosureInfoType3Strings[] = { > mSysInfoManufName, > mSysInfoProductName, > mSysInfoSerial, > - "None", > NULL > }; > > @@ -240,9 +217,9 @@ CHAR8 *mEnclosureInfoType3Strings[] = { > SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = { > { EFI_SMBIOS_TYPE_PROCESSOR_INFORMATION, sizeof (SMBIOS_TABLE_TYPE4), 0}, > 1, // Socket String > - CentralProcessor, // ProcessorType; ///< The enumeration value from PROCESSOR_TYPE_DATA. > + CentralProcessor, // ProcessorType; ///< The enumeration value from PROCESSOR_TYPE_DATA. Whitespace change only. > ProcessorFamilyIndicatorFamily2, // ProcessorFamily; ///< The enumeration value from PROCESSOR_FAMILY2_DATA. > - 2, // ProcessorManufacture String; > + 2, // ProcessorManufacturer String; Comment typo fix only. > { // ProcessorId; > { // PROCESSOR_SIGNATURE > 0, // ProcessorSteppingId:4; > @@ -306,9 +283,9 @@ SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = { > 0, // L1CacheHandle; > 0, // L2CacheHandle; > 0, // L3CacheHandle; > - 4, // SerialNumber; > - 5, // AssetTag; > - 6, // PartNumber; > + 0, // SerialNumber; > + 0, // AssetTag; > + 0, // PartNumber; > 4, // CoreCount; > 4, // EnabledCoreCount; > 4, // ThreadCount; > @@ -316,13 +293,12 @@ SMBIOS_TABLE_TYPE4 mProcessorInfoType4 = { > ProcessorFamilyARM, // ARM Processor Family; > }; > > +CHAR8 mCpuName[128] = "Unknown ARM CPU"; > + > CHAR8 *mProcessorInfoType4Strings[] = { > "Socket", > - "ARM", > - "BCM2837 ARMv8", > - "1.0", > - "1.0", > - "1.0", > + "Broadcom", > + mCpuName, > NULL > }; > > @@ -430,7 +406,7 @@ SMBIOS_TABLE_TYPE17 mMemDevInfoType17 = { > 0x0400, // Size; // When bit 15 is 0: Size in MB > // When bit 15 is 1: Size in KB, and continues in ExtendedSize > MemoryFormFactorUnknown, // FormFactor; ///< The enumeration value from MEMORY_FORM_FACTOR. > - 0xff, // DeviceSet; > + 0xFF, // DeviceSet; Style fix only. > 1, // DeviceLocator String > 2, // BankLocator String > MemoryTypeDram, // MemoryType; ///< The enumeration value from MEMORY_DEVICE_TYPE. > @@ -618,6 +594,30 @@ BIOSInfoUpdateSmbiosType0 ( > VOID > ) > { > + UINT32 FirmwareRevision = 0; > + EFI_STATUS Status = EFI_SUCCESS; > + INTN i; > + INTN State = 0; > + INTN Value[2]; The three last variables defined above are only used by code added by the next patch, meaning this patch would break git bisect with many toolchains (defined but not used/set but not used). > + > + // Populate the Firmware major and minor. > + Status = mFwProtocol->GetFirmwareRevision (&FirmwareRevision); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to get firmware revision: %r\n", Status)); > + } else { > + // This expects Broadcom / The Raspberry Pi Foundation to switch to > + // less nonsensical VideoCore firmware revisions in the future... > + mBIOSInfoType0.EmbeddedControllerFirmwareMajorRelease = > + (UINT8)((FirmwareRevision >> 16) & 0xFF); > + mBIOSInfoType0.EmbeddedControllerFirmwareMinorRelease = > + (UINT8)(FirmwareRevision & 0xFF); > + } > + > + // mBiosVersion, which is referenced in mBIOSInfoType0Strings, > + // is not modified if the following call fails. > + UnicodeStrToAsciiStrS ((CHAR16*)PcdGetPtr (PcdFirmwareVersionString), > + mBiosVersion, sizeof (mBiosVersion)); > + > LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mBIOSInfoType0, mBIOSInfoType0Strings, NULL); > } > > @@ -631,7 +631,7 @@ I64ToHexString ( > IN UINT64 Value > ) > { > - static CHAR8 ItoH[] = { '0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F' }; > + STATIC CHAR8 ItoH[] = { '0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F' }; Style fix only. / Leif > UINT8 StringInx; > INT8 NibbleInx; > > @@ -664,34 +664,25 @@ SysInfoUpdateSmbiosType1 ( > UINT32 BoardRevision = 0; > EFI_STATUS Status = EFI_SUCCESS; > UINT64 BoardSerial = 0; > - UINTN Prod = PROD_UNKNOWN; > - UINTN Manu = MANU_UNKNOWN; > + INTN Prod = -1; > + INTN Manu = -1; > > Status = mFwProtocol->GetModelRevision (&BoardRevision); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "Failed to get board model: %r\n", Status)); > } else { > Prod = (BoardRevision >> 4) & 0xFF; > + Manu = (BoardRevision >> 16) & 0x0F; > } > > - if (Prod > PROD_KNOWN) { > - Prod = PROD_UNKNOWN; > - } > - Prod -= PROD_BASE; > - AsciiSPrint (mSysInfoProductName, sizeof (mSysInfoProductName), > - "64-bit Raspberry Pi %a (rev. %x)", ProductNames[Prod], BoardRevision); > - > - Manu = (BoardRevision >> 16) & 0xF; > - if (Manu > MANU_KNOWN) { > - Manu = MANU_UNKNOWN; > - } else { > - Manu += MANU_BASE; > - } > - AsciiSPrint (mSysInfoManufName, sizeof (mSysInfoManufName), "%a", ManufNames[Manu]); > + AsciiStrCpyS (mSysInfoProductName, sizeof (mSysInfoProductName), > + mFwProtocol->GetModelName (Prod)); > + AsciiStrCpyS (mSysInfoManufName, sizeof (mSysInfoManufName), > + mFwProtocol->GetManufacturerName (Manu)); > + AsciiSPrint (mSysInfoVersionName, sizeof (mSysInfoVersionName), > + "%X", BoardRevision); > > - I64ToHexString (mSysInfoSKU, > - sizeof (mSysInfoSKU), > - BoardRevision); > + I64ToHexString (mSysInfoSKU, sizeof (mSysInfoSKU), BoardRevision); > > Status = mFwProtocol->GetSerial (&BoardSerial); > if (EFI_ERROR (Status)) { > @@ -702,9 +693,11 @@ SysInfoUpdateSmbiosType1 ( > > DEBUG ((DEBUG_ERROR, "Board Serial Number: %a\n", mSysInfoSerial)); > > - mSysInfoType1.Uuid.Data1 = *(UINT32*)"RPi3"; > - mSysInfoType1.Uuid.Data2 = 0x0; > - mSysInfoType1.Uuid.Data3 = 0x0; > + mSysInfoType1.Uuid.Data1 = BoardRevision; > + mSysInfoType1.Uuid.Data2 = 0; > + mSysInfoType1.Uuid.Data3 = 0; > + // Swap endianness, so that the serial is more user-friendly as a UUID > + BoardSerial = SwapBytes64 (BoardSerial); > CopyMem (mSysInfoType1.Uuid.Data4, &BoardSerial, sizeof (BoardSerial)); > > LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mSysInfoType1, mSysInfoType1Strings, NULL); > @@ -763,6 +756,8 @@ ProcessorInfoUpdateSmbiosType4 ( > DEBUG ((DEBUG_INFO, "Current CPU speed: %uHz\n", Rate)); > } > > + AsciiStrCpyS (mCpuName, sizeof (mCpuName), mFwProtocol->GetCpuName (-1)); > + > LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER*)&mProcessorInfoType4, mProcessorInfoType4Strings, NULL); > } > > diff --git a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf > index f7c74f7f5456..fde194ea5d90 100644 > --- a/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf > +++ b/Platform/RaspberryPi/RPi3/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.inf > @@ -48,3 +48,4 @@ [Depex] > > [Pcd] > gArmTokenSpaceGuid.PcdSystemMemorySize > + gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString > diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc > index b37a02e97da7..6adc21bcc370 100644 > --- a/Platform/RaspberryPi/RPi3/RPi3.dsc > +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc > @@ -309,7 +309,7 @@ [PcdsFixedAtBuild.common] > > gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0xc0000000 > > - gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"edk2-1.0" > + gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString|L"EDK2-DEV" > > !if $(SECURE_BOOT_ENABLE) == TRUE > # override the default values from SecurityPkg to ensure images from all sources are verified in secure boot > -- > 2.21.0.windows.1 >