Re: [edk2-devel] [PATCH v6 15/22] ArmPkg: Add Universal/Smbios/SmbiosMiscDxe/Type00

["Samer El-Haj-Mahmoud" <samer.el-haj-mahmoud@arm.com>]

With a minor feedback below:
Reviewed-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>

+  }
> +
> +  (VOID)CopyMem (SmbiosRecord, InputData, sizeof (SMBIOS_TABLE_TYPE0));
> +
> +  SmbiosRecord->Hdr.Length = sizeof (SMBIOS_TABLE_TYPE0);
> + SmbiosRecord->BiosSegment = (UINT16)(FixedPcdGet32 (PcdFdBaseAddress)
> + / SIZE_64KB);  if (BiosPhysicalSize < SIZE_16MB) {
> +    SmbiosRecord->BiosSize = Base2ToByteWith64KUnit (BiosPhysicalSize) - 1;
> +    SmbiosRecord->ExtendedBiosSize.Size = BiosPhysicalSize / SIZE_1MB;
> +    SmbiosRecord->ExtendedBiosSize.Unit = 0; // Size is in MB  } else {
> +    SmbiosRecord->BiosSize = 0xFF;
> +    if (BiosPhysicalSize > 0x3FFF) {
> +      SmbiosRecord->ExtendedBiosSize.Size = BiosPhysicalSize / SIZE_1GB;
> +      SmbiosRecord->ExtendedBiosSize.Unit = 1; // Size is in GB
> +    }
> +  }
> +
> +  SmbiosRecord->SystemBiosMajorRelease = (UINT8) (PcdGet16
> + (PcdSystemBiosRelease) >> 8);  SmbiosRecord->SystemBiosMinorRelease =
> + (UINT8) (PcdGet16 (PcdSystemBiosRelease) & 0xFF);
> +
> +  SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)
> +    (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
> + SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)
> +    (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
> +

Not really an issue, but a function in OemMiscLib is possibly a better option than using PCDs for these. Platforms may have an easy way to build the Major.Minor FW values (such as by parsing PcdFirmwareVersionString and splitting into major and minor values). See for example for RPi4 at: https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/Drivers/PlatformSmbiosDxe/PlatformSmbiosDxe.c#L754
Using PCDs will still work, but the platform will need to put this code in some other module/library just to initialize the dynamic PCD.