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 <p...@akeo.ie>
> ---
> 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 <Library/UefiDriverEntryPoint.h>
> #include <Library/UefiLib.h>
> #include <Library/BaseLib.h>
> +#include <Library/PcdLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/PrintLib.h>
>
> -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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48714): https://edk2.groups.io/g/devel/message/48714
Mute This Topic: https://groups.io/mt/34441818/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
