Re: [edk2-plaforms PATCH 1/1] SbsaQemu: Fix numerous SSDT generation problems

["Leif Lindholm" <leif@nuviainc.com>]

A few minor typos in the commit message:

On Thu, Aug 27, 2020 at 16:21:33 +0100, Graeme Gregory wrote:
> 1 - The SBSAQEMU_ACPI_ITOA contained a typo that put bogus characters
> in the name if number of CPUs was greater than 10. It is safter to use

safter -> safer

> the AsciiSPrint function from PrintLib.
> 
> 2 - The _UID fields were bogus, and indicated as bytes in AML instead of
> a word. This caused extra Zeros to appear in disassembly. Fixed by

Zero's -> Zeros

> making them AML_WORD_PREFIX and putting CpuId in little endian.
> 
> 3 - The table was a number of bytes too long causes bogus Zero in

missing ", which"

> dissassembly at end of table. Re-adjust code slightly to reduce table
> size once we know the size of the length field.
> 
> Signed-off-by: Graeme Gregory <graeme@nuviainc.com>

Reviewed-by: Leif Lindholm <leif@nuviainc.com>
I took the liberty to address the typos locally before pushing as
b54d65a4a671.

Thanks!

/
    Leif

> ---
> 
> This patch supercedes "SbsaQemu: Fix CPUID generation in SSDT"
> as while fixing review comments on that I found the other issues.
> 
>  .../SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf       |  1 +
>  .../Include/IndustryStandard/SbsaQemuAcpi.h   |  5 +---
>  .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 25 +++++++++++--------
>  3 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> index cde9d02f7f90..127eef029f3c 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> @@ -36,6 +36,7 @@ [LibraryClasses]
>    DxeServicesLib
>    FdtLib
>    PcdLib
> +  PrintLib
>    UefiDriverEntryPoint
>    UefiLib
>    UefiRuntimeServicesTableLib
> diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
> index 1a7d9dda2b99..f085765d2677 100644
> --- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
> +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuAcpi.h
> @@ -50,9 +50,6 @@
>  #define SBSAQEMU_ACPI_CPU_DEV_LEN        0x1C
>  #define SBSAQEMU_ACPI_CPU_DEV_NAME       { 'C', '0', '0', '0' }
>  
> -// Macro to convert Integer to Character
> -#define SBSAQEMU_ACPI_ITOA(Byte)         (0x30 + (Byte > 9 ? (Byte + 1) : Byte))
> -
>  #define SBSAQEMU_ACPI_CPU_HID           {                                      \
>    AML_NAME_OP, AML_NAME_CHAR__, 'H', 'I', 'D',                                 \
>    AML_STRING_PREFIX, 'A', 'C', 'P', 'I', '0', '0', '0', '7',                   \
> @@ -60,7 +57,7 @@
>    }
>  
>  #define SBSAQEMU_ACPI_CPU_UID            {                                     \
> -   AML_NAME_OP, AML_NAME_CHAR__, 'U', 'I', 'D', AML_BYTE_PREFIX,               \
> +   AML_NAME_OP, AML_NAME_CHAR__, 'U', 'I', 'D', AML_WORD_PREFIX,               \
>     AML_ZERO_OP, AML_ZERO_OP                                                    \
>     }
>  
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> index 06552f4b22f3..47a9bd1d423a 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> @@ -14,6 +14,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiDriverEntryPoint.h>
>  #include <Library/UefiLib.h>
> @@ -248,6 +249,7 @@ AddSsdtTable (
>    UINT32                TableSize;
>    EFI_PHYSICAL_ADDRESS  PageAddress;
>    UINT8                 *New;
> +  UINT8                 *HeaderAddr;
>    UINT32                CpuId;
>    UINT32                Offset;
>    UINT8                 ScopeOpName[] =  SBSAQEMU_ACPI_SCOPE_NAME;
> @@ -283,12 +285,12 @@ AddSsdtTable (
>      return EFI_OUT_OF_RESOURCES;
>    }
>  
> -  New = (UINT8 *)(UINTN) PageAddress;
> +  HeaderAddr = New = (UINT8 *)(UINTN) PageAddress;
>    ZeroMem (New, TableSize);
>  
>    // Add the ACPI Description table header
>    CopyMem (New, &Header, sizeof (EFI_ACPI_DESCRIPTION_HEADER));
> -  ((EFI_ACPI_DESCRIPTION_HEADER*) New)->Length = TableSize;
> +
>    New += sizeof (EFI_ACPI_DESCRIPTION_HEADER);
>  
>    // Insert the top level ScopeOp
> @@ -296,6 +298,11 @@ AddSsdtTable (
>    New++;
>    Offset = SetPkgLength (New,
>               (TableSize - sizeof (EFI_ACPI_DESCRIPTION_HEADER) - 1));
> +
> +  // Adjust TableSize now we know header length of _SB
> +  TableSize -= (SBSAQEMU_ACPI_SCOPE_OP_MAX_LENGTH - Offset);
> +  ((EFI_ACPI_DESCRIPTION_HEADER*) HeaderAddr)->Length = TableSize;
> +
>    New += Offset;
>    CopyMem (New, &ScopeOpName, sizeof (ScopeOpName));
>    New += sizeof (ScopeOpName);
> @@ -303,21 +310,17 @@ AddSsdtTable (
>    // Add new Device structures for the Cores
>    for (CpuId = 0; CpuId < NumCores; CpuId++) {
>      SBSAQEMU_ACPI_CPU_DEVICE *CpuDevicePtr;
> -    UINT8 CpuIdByte1, CpuIdByte2, CpuIdByte3;
>  
>      CopyMem (New, &CpuDevice, sizeof (SBSAQEMU_ACPI_CPU_DEVICE));
>      CpuDevicePtr = (SBSAQEMU_ACPI_CPU_DEVICE *) New;
>  
> -    CpuIdByte1 = CpuId & 0xF;
> -    CpuIdByte2 = (CpuId >> 4) & 0xF;
> -    CpuIdByte3 = (CpuId >> 8) & 0xF;
> +    AsciiSPrint((CHAR8 *)&CpuDevicePtr->dev_name[1], 4, "%03X", CpuId);
>  
> -    CpuDevicePtr->dev_name[1] = SBSAQEMU_ACPI_ITOA(CpuIdByte3);
> -    CpuDevicePtr->dev_name[2] = SBSAQEMU_ACPI_ITOA(CpuIdByte2);
> -    CpuDevicePtr->dev_name[3] = SBSAQEMU_ACPI_ITOA(CpuIdByte1);
> +    /* replace character lost by above NULL termination */
> +    CpuDevicePtr->hid[0] = AML_NAME_OP;
>  
> -    CpuDevicePtr->uid[6] = CpuIdByte1 | CpuIdByte2;
> -    CpuDevicePtr->uid[7] = CpuIdByte3;
> +    CpuDevicePtr->uid[6] = CpuId & 0xFF;
> +    CpuDevicePtr->uid[7] = (CpuId >> 8) & 0xFF;
>      New += sizeof (SBSAQEMU_ACPI_CPU_DEVICE);
>    }
>  
> -- 
> 2.25.1
>