Re: [PATCH 2/7] OvmfPkg/PlatformPei: parse "X-PciMmio64Mb" with QemuFwCfgSimpleParserLib

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/24/20 9:53 AM, Laszlo Ersek wrote:
> Replace the
> 
> - QemuFwCfgFindFile(),
> - QemuFwCfgSelectItem(),
> - QemuFwCfgReadBytes(),
> - AsciiStrDecimalToUint64()
> 
> sequence in the GetFirstNonAddress() function with a call to
> QemuFwCfgSimpleParserLib.
> 
> This change is compatible with valid strings accepted previously.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Per Sundstrom <per_sundstrom@yahoo.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2681
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
>   OvmfPkg/PlatformPei/MemDetect.c     | 36 ++++++++++++--------
>   2 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 19f2424981bc..e72ef7963d97 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -60,6 +60,7 @@ [LibraryClasses]
>     PeimEntryPoint
>     QemuFwCfgLib
>     QemuFwCfgS3Lib
> +  QemuFwCfgSimpleParserLib
>     MtrrLib
>     MemEncryptSevLib
>     PcdLib
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 47dc9c543719..f32df937f9ba 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -33,6 +33,7 @@ Module Name:
>   #include <Library/ResourcePublicationLib.h>
>   #include <Library/MtrrLib.h>
>   #include <Library/QemuFwCfgLib.h>
> +#include <Library/QemuFwCfgSimpleParserLib.h>
>   
>   #include "Platform.h"
>   #include "Cmos.h"
> @@ -336,7 +337,7 @@ GetFirstNonAddress (
>   {
>     UINT64               FirstNonAddress;
>     UINT64               Pci64Base, Pci64Size;
> -  CHAR8                MbString[7 + 1];

Replaced by UINT64_STRING_MAX_SIZE + CRLF_LENGTH, way safer.

> +  UINT32               FwCfgPciMmio64Mb;
>     EFI_STATUS           Status;
>     FIRMWARE_CONFIG_ITEM FwCfgItem;
>     UINTN                FwCfgSize;
> @@ -379,25 +380,30 @@ GetFirstNonAddress (
>   
>     //
>     // See if the user specified the number of megabytes for the 64-bit PCI host
> -  // aperture. The number of non-NUL characters in MbString allows for
> -  // 9,999,999 MB, which is approximately 10 TB.
> +  // aperture. Accept an aperture size up to 16TB.
>     //
>     // As signaled by the "X-" prefix, this knob is experimental, and might go
>     // away at any time.
>     //
> -  Status = QemuFwCfgFindFile ("opt/ovmf/X-PciMmio64Mb", &FwCfgItem,
> -             &FwCfgSize);
> -  if (!EFI_ERROR (Status)) {
> -    if (FwCfgSize >= sizeof MbString) {
> -      DEBUG ((EFI_D_WARN,
> -        "%a: ignoring malformed 64-bit PCI host aperture size from fw_cfg\n",
> -        __FUNCTION__));
> -    } else {
> -      QemuFwCfgSelectItem (FwCfgItem);
> -      QemuFwCfgReadBytes (FwCfgSize, MbString);

Up to here is QemuFwCfgGetAsString(), OK.

> -      MbString[FwCfgSize] = '\0';

StripNewline(), OK.

> -      Pci64Size = LShiftU64 (AsciiStrDecimalToUint64 (MbString), 20);
> +  Status = QemuFwCfgParseUint32 ("opt/ovmf/X-PciMmio64Mb", FALSE,

ParseAsHex=False, so we use AsciiStrDecimalToUint64S(), OK.

> +             &FwCfgPciMmio64Mb);
> +  switch (Status) {
> +  case EFI_UNSUPPORTED:
> +  case EFI_NOT_FOUND:
> +    break;
> +  case EFI_SUCCESS:
> +    if (FwCfgPciMmio64Mb <= 0x1000000) {
> +      Pci64Size = LShiftU64 (FwCfgPciMmio64Mb, 20);

16TB, OK.

> +      break;
>       }
> +    //
> +    // fall through
> +    //
> +  default:
> +    DEBUG ((DEBUG_WARN,
> +      "%a: ignoring malformed 64-bit PCI host aperture size from fw_cfg\n",
> +      __FUNCTION__));
> +    break;
>     }
>   
>     if (Pci64Size == 0) {
> 

The new API is very clean!

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>