* [edk2-plaforms PATCH 1/1] SbsaQemu: Fix numerous SSDT generation problems
@ 2020-08-27 15:21 Graeme Gregory
2020-08-27 17:10 ` Leif Lindholm
0 siblings, 1 reply; 2+ messages in thread
From: Graeme Gregory @ 2020-08-27 15:21 UTC (permalink / raw)
To: devel; +Cc: leif, tanmay.jagdale, Graeme Gregory
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
the AsciiSPrint function from PrintLib.
2 - The _UID fields were bogus, and indicated as bytes in AML instead of
a word. This caused extra Zero's to appear in disassembly. Fixed by
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
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>
---
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [edk2-plaforms PATCH 1/1] SbsaQemu: Fix numerous SSDT generation problems
2020-08-27 15:21 [edk2-plaforms PATCH 1/1] SbsaQemu: Fix numerous SSDT generation problems Graeme Gregory
@ 2020-08-27 17:10 ` Leif Lindholm
0 siblings, 0 replies; 2+ messages in thread
From: Leif Lindholm @ 2020-08-27 17:10 UTC (permalink / raw)
To: Graeme Gregory; +Cc: devel, tanmay.jagdale
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-08-27 17:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-27 15:21 [edk2-plaforms PATCH 1/1] SbsaQemu: Fix numerous SSDT generation problems Graeme Gregory
2020-08-27 17:10 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox