* [edk2-[PATCH v1 0/2] APIC table updates
@ 2021-10-22 14:47 Abdul Lateef Attar
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode Abdul Lateef Attar
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Abdul Lateef Attar @ 2021-10-22 14:47 UTC (permalink / raw)
To: devel; +Cc: Chasel Chiu, Nate DeSimone, Liming Gao, Eric Dong
Fixes the APIC table creation based on APIC mode.
Also fixes the AcpiProcessorUid for multi-socket system.
Abdul Lateef Attar (2):
MinPlatformPkg: Update APIC Table based on mode
MinPlatformPkg: AcpiProcessorUid for multi-socket
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 37 ++++++++++++--------
1 file changed, 22 insertions(+), 15 deletions(-)
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
2021-10-22 14:47 [edk2-[PATCH v1 0/2] APIC table updates Abdul Lateef Attar
@ 2021-10-22 14:47 ` Abdul Lateef Attar
2021-10-27 8:16 ` [edk2-devel] " Nate DeSimone
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 2/2] MinPlatformPkg: AcpiProcessorUid for multi-socket Abdul Lateef Attar
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Abdul Lateef Attar @ 2021-10-22 14:47 UTC (permalink / raw)
To: devel; +Cc: Chasel Chiu, Nate DeSimone, Liming Gao, Eric Dong
Detects the APIC mode by calling GetApicMode().
if current platform is in X2APIC mode sets the
global mX2ApicEnabled.
Also install the APIC NMI structure only if
platform is not in X2APIC mode.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 35 ++++++++++++--------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index f5c4f2c3f1a9..032903029a95 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -3,6 +3,7 @@
Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) Microsoft Corporation.<BR>
+Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -947,21 +948,23 @@ InstallMadtFromScratch (
//
// Build Local APIC NMI Structures
//
- LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
- LocalApciNmiStruct.Length = sizeof (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
- LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
- LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
- LocalApciNmiStruct.LocalApicLint = 0x1;
+ if (!mX2ApicEnabled) {
+ LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
+ LocalApciNmiStruct.Length = sizeof (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
+ LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
+ LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
+ LocalApciNmiStruct.LocalApicLint = 0x1;
- ASSERT (MadtStructsIndex < MaxMadtStructCount);
- Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &LocalApciNmiStruct,
- &MadtStructs[MadtStructsIndex++]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
- goto Done;
+ ASSERT (MadtStructsIndex < MaxMadtStructCount);
+ Status = CopyStructure (
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &LocalApciNmiStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
+ goto Done;
+ }
}
//
@@ -1454,6 +1457,10 @@ InstallAcpiPlatform (
DEBUG ((DEBUG_INFO, "mNumberOfCpus - %d\n", mNumberOfCpus));
DEBUG ((DEBUG_INFO, "mNumberOfEnabledCPUs - %d\n", mNumberOfEnabledCPUs));
+ if (LOCAL_APIC_MODE_X2APIC == GetApicMode ()) {
+ mX2ApicEnabled = TRUE;
+ }
+
DEBUG ((DEBUG_INFO, "mX2ApicEnabled - 0x%x\n", mX2ApicEnabled));
DEBUG ((DEBUG_INFO, "mForceX2ApicId - 0x%x\n", mForceX2ApicId));
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [edk2-platforms] [PATCH v1 2/2] MinPlatformPkg: AcpiProcessorUid for multi-socket
2021-10-22 14:47 [edk2-[PATCH v1 0/2] APIC table updates Abdul Lateef Attar
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode Abdul Lateef Attar
@ 2021-10-22 14:47 ` Abdul Lateef Attar
2021-10-27 8:16 ` [edk2-devel] " Nate DeSimone
2021-10-27 8:16 ` [edk2-[PATCH v1 0/2] APIC table updates Nate DeSimone
2021-11-02 21:42 ` Nate DeSimone
3 siblings, 1 reply; 12+ messages in thread
From: Abdul Lateef Attar @ 2021-10-22 14:47 UTC (permalink / raw)
To: devel; +Cc: Chasel Chiu, Nate DeSimone, Liming Gao, Eric Dong
Corrects the AcpiProcessorUid for Multi socket/package.
The ProcessorInfoBuffer.Location.Package holds the stale entry
of the last processor. In multi-socket system it holds
the Socket/Package number of last socket/package, resulting
wrong AcpiProcessorUid assignment for socket 0 processors.
Hence do the bit shift on stored SocketNum.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index 032903029a95..785cf4c2f911 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -287,7 +287,7 @@ SortCpuLocalApicInTable (
for (Socket = 0; Socket < FixedPcdGet32 (PcdMaxCpuSocketCount); Socket++) {
for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++) {
if (mCpuApicIdOrderTable[CurrProcessor].Flags && (mCpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
- mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorUid = (ProcessorInfoBuffer.Location.Package << mNumOfBitShift) + Index;
+ mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorUid = (mCpuApicIdOrderTable[CurrProcessor].SocketNum << mNumOfBitShift) + Index;
Index++;
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-[PATCH v1 0/2] APIC table updates
2021-10-22 14:47 [edk2-[PATCH v1 0/2] APIC table updates Abdul Lateef Attar
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode Abdul Lateef Attar
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 2/2] MinPlatformPkg: AcpiProcessorUid for multi-socket Abdul Lateef Attar
@ 2021-10-27 8:16 ` Nate DeSimone
2021-10-28 4:05 ` Attar, AbdulLateef (Abdul Lateef)
2021-11-02 21:42 ` Nate DeSimone
3 siblings, 1 reply; 12+ messages in thread
From: Nate DeSimone @ 2021-10-27 8:16 UTC (permalink / raw)
To: Abdul Lateef Attar, devel@edk2.groups.io
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
Hi Abdul,
First of all thank you for the patch series. One thing to take note of; unfortunately, the groups.io web interface messes up the line endings and makes it difficult to apply any patches sent through it. I was able to get your patch series to apply, but in the future, would it be possible for you to send your patches without using the groups.io web interface?
Thanks Again!
Nate
-----Original Message-----
From: Abdul Lateef Attar <abdattar@amd.com>
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-[PATCH v1 0/2] APIC table updates
Fixes the APIC table creation based on APIC mode.
Also fixes the AcpiProcessorUid for multi-socket system.
Abdul Lateef Attar (2):
MinPlatformPkg: Update APIC Table based on mode
MinPlatformPkg: AcpiProcessorUid for multi-socket
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 37 ++++++++++++--------
1 file changed, 22 insertions(+), 15 deletions(-)
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode Abdul Lateef Attar
@ 2021-10-27 8:16 ` Nate DeSimone
2021-10-27 16:33 ` Attar, AbdulLateef (Abdul Lateef)
0 siblings, 1 reply; 12+ messages in thread
From: Nate DeSimone @ 2021-10-27 8:16 UTC (permalink / raw)
To: devel@edk2.groups.io, abdattar@amd.com
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
Hi Abdul,
I do agree with you that logically it seems to be the right thing to not install the local APIC NMI structure if we install the X2APIC NMI structure instead. However, I went and took a look at some of our internal closed source code and it appears that even on X2APIC systems we actually install both NMI structures. My immediate reaction was to simply consider that a bug, but I am worried that this might have been done to workaround some buggy and/or legacy OS behavior. I've been trying to find more history on it but that behavior has been in our codebase for a very long time. IMHO it would be extremely weird for an OS to support X2APIC and have some bug handling X2APIC NMI structures... but it is always best to check assumptions.
Can you elaborate on any testing that you have done? Have you tested this change on a real HW? If yes, which OS did you try booting? Have you tried any older OSes?
Thanks,
Nate
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
Detects the APIC mode by calling GetApicMode().
if current platform is in X2APIC mode sets the global mX2ApicEnabled.
Also install the APIC NMI structure only if platform is not in X2APIC mode.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 35 ++++++++++++--------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index f5c4f2c3f1a9..032903029a95 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -3,6 +3,7 @@
Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) Microsoft Corporation.<BR>
+Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -947,21 +948,23 @@ InstallMadtFromScratch (
//
// Build Local APIC NMI Structures
//
- LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
- LocalApciNmiStruct.Length = sizeof (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
- LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
- LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
- LocalApciNmiStruct.LocalApicLint = 0x1;
+ if (!mX2ApicEnabled) {
+ LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
+ LocalApciNmiStruct.Length = sizeof
+ (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
+ LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
+ LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
+ LocalApciNmiStruct.LocalApicLint = 0x1;
- ASSERT (MadtStructsIndex < MaxMadtStructCount);
- Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &LocalApciNmiStruct,
- &MadtStructs[MadtStructsIndex++]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
- goto Done;
+ ASSERT (MadtStructsIndex < MaxMadtStructCount);
+ Status = CopyStructure (
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &LocalApciNmiStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n",
+ Status));
+ goto Done;
+ }
}
//
@@ -1454,6 +1457,10 @@ InstallAcpiPlatform (
DEBUG ((DEBUG_INFO, "mNumberOfCpus - %d\n", mNumberOfCpus));
DEBUG ((DEBUG_INFO, "mNumberOfEnabledCPUs - %d\n", mNumberOfEnabledCPUs));
+ if (LOCAL_APIC_MODE_X2APIC == GetApicMode ()) {
+ mX2ApicEnabled = TRUE;
+ }
+
DEBUG ((DEBUG_INFO, "mX2ApicEnabled - 0x%x\n", mX2ApicEnabled));
DEBUG ((DEBUG_INFO, "mForceX2ApicId - 0x%x\n", mForceX2ApicId));
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-platforms] [PATCH v1 2/2] MinPlatformPkg: AcpiProcessorUid for multi-socket
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 2/2] MinPlatformPkg: AcpiProcessorUid for multi-socket Abdul Lateef Attar
@ 2021-10-27 8:16 ` Nate DeSimone
0 siblings, 0 replies; 12+ messages in thread
From: Nate DeSimone @ 2021-10-27 8:16 UTC (permalink / raw)
To: devel@edk2.groups.io, abdattar@amd.com
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [edk2-platforms] [PATCH v1 2/2] MinPlatformPkg: AcpiProcessorUid for multi-socket
Corrects the AcpiProcessorUid for Multi socket/package.
The ProcessorInfoBuffer.Location.Package holds the stale entry of the last processor. In multi-socket system it holds the Socket/Package number of last socket/package, resulting wrong AcpiProcessorUid assignment for socket 0 processors.
Hence do the bit shift on stored SocketNum.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index 032903029a95..785cf4c2f911 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -287,7 +287,7 @@ SortCpuLocalApicInTable (
for (Socket = 0; Socket < FixedPcdGet32 (PcdMaxCpuSocketCount); Socket++) {
for (CurrProcessor = 0, Index = 0; CurrProcessor < mNumberOfCpus; CurrProcessor++) {
if (mCpuApicIdOrderTable[CurrProcessor].Flags && (mCpuApicIdOrderTable[CurrProcessor].SocketNum == Socket)) {
- mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorUid = (ProcessorInfoBuffer.Location.Package << mNumOfBitShift) + Index;
+ mCpuApicIdOrderTable[CurrProcessor].AcpiProcessorUid =
+ (mCpuApicIdOrderTable[CurrProcessor].SocketNum << mNumOfBitShift) +
+ Index;
Index++;
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
2021-10-27 8:16 ` [edk2-devel] " Nate DeSimone
@ 2021-10-27 16:33 ` Attar, AbdulLateef (Abdul Lateef)
2021-10-28 2:48 ` Nate DeSimone
0 siblings, 1 reply; 12+ messages in thread
From: Attar, AbdulLateef (Abdul Lateef) @ 2021-10-27 16:33 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
[Public]
Hi Nate,
I tested on real hardware with latest linux(didn't tested with older OSes).
Note that with X2APIC mode I don't see any issue having APIC NMI structure and not having NMI structure.
In both cases OS boots without any issue. Just thought that why to include APIC NMI structure in X2APIC mode.
Do you want me to resubmit the patch and include the APIC NMI for X2APIC mode?
Thanks
Abdul
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: 27 October 2021 13:47
To: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
[CAUTION: External Email]
Hi Abdul,
I do agree with you that logically it seems to be the right thing to not install the local APIC NMI structure if we install the X2APIC NMI structure instead. However, I went and took a look at some of our internal closed source code and it appears that even on X2APIC systems we actually install both NMI structures. My immediate reaction was to simply consider that a bug, but I am worried that this might have been done to workaround some buggy and/or legacy OS behavior. I've been trying to find more history on it but that behavior has been in our codebase for a very long time. IMHO it would be extremely weird for an OS to support X2APIC and have some bug handling X2APIC NMI structures... but it is always best to check assumptions.
Can you elaborate on any testing that you have done? Have you tested this change on a real HW? If yes, which OS did you try booting? Have you tried any older OSes?
Thanks,
Nate
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
Detects the APIC mode by calling GetApicMode().
if current platform is in X2APIC mode sets the global mX2ApicEnabled.
Also install the APIC NMI structure only if platform is not in X2APIC mode.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 35 ++++++++++++--------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index f5c4f2c3f1a9..032903029a95 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -3,6 +3,7 @@
Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) Microsoft Corporation.<BR>
+Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -947,21 +948,23 @@ InstallMadtFromScratch (
//
// Build Local APIC NMI Structures
//
- LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
- LocalApciNmiStruct.Length = sizeof (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
- LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
- LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
- LocalApciNmiStruct.LocalApicLint = 0x1;
+ if (!mX2ApicEnabled) {
+ LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
+ LocalApciNmiStruct.Length = sizeof
+ (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
+ LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
+ LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
+ LocalApciNmiStruct.LocalApicLint = 0x1;
- ASSERT (MadtStructsIndex < MaxMadtStructCount);
- Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &LocalApciNmiStruct,
- &MadtStructs[MadtStructsIndex++]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
- goto Done;
+ ASSERT (MadtStructsIndex < MaxMadtStructCount);
+ Status = CopyStructure (
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &LocalApciNmiStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n",
+ Status));
+ goto Done;
+ }
}
//
@@ -1454,6 +1457,10 @@ InstallAcpiPlatform (
DEBUG ((DEBUG_INFO, "mNumberOfCpus - %d\n", mNumberOfCpus));
DEBUG ((DEBUG_INFO, "mNumberOfEnabledCPUs - %d\n", mNumberOfEnabledCPUs));
+ if (LOCAL_APIC_MODE_X2APIC == GetApicMode ()) {
+ mX2ApicEnabled = TRUE;
+ }
+
DEBUG ((DEBUG_INFO, "mX2ApicEnabled - 0x%x\n", mX2ApicEnabled));
DEBUG ((DEBUG_INFO, "mForceX2ApicId - 0x%x\n", mForceX2ApicId));
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
2021-10-27 16:33 ` Attar, AbdulLateef (Abdul Lateef)
@ 2021-10-28 2:48 ` Nate DeSimone
2021-10-28 12:50 ` Attar, AbdulLateef (Abdul Lateef)
0 siblings, 1 reply; 12+ messages in thread
From: Nate DeSimone @ 2021-10-28 2:48 UTC (permalink / raw)
To: Attar, AbdulLateef (Abdul Lateef), devel@edk2.groups.io
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
Hi Abdul,
I would prefer to clean up the technical debt and not have the APIC NMI if we are in X2APIC mode. Is there any chance you could test this change with Windows Server 2019? As long as semi-current Windows + Linux boots I'm fine with merging your patch as-is.
Thanks,
Nate
On 10/27/21, 9:33 AM, "Attar, AbdulLateef (Abdul Lateef)" <AbdulLateef.Attar@amd.com> wrote:
[Public]
Hi Nate,
I tested on real hardware with latest linux(didn't tested with older OSes).
Note that with X2APIC mode I don't see any issue having APIC NMI structure and not having NMI structure.
In both cases OS boots without any issue. Just thought that why to include APIC NMI structure in X2APIC mode.
Do you want me to resubmit the patch and include the APIC NMI for X2APIC mode?
Thanks
Abdul
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: 27 October 2021 13:47
To: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
[CAUTION: External Email]
Hi Abdul,
I do agree with you that logically it seems to be the right thing to not install the local APIC NMI structure if we install the X2APIC NMI structure instead. However, I went and took a look at some of our internal closed source code and it appears that even on X2APIC systems we actually install both NMI structures. My immediate reaction was to simply consider that a bug, but I am worried that this might have been done to workaround some buggy and/or legacy OS behavior. I've been trying to find more history on it but that behavior has been in our codebase for a very long time. IMHO it would be extremely weird for an OS to support X2APIC and have some bug handling X2APIC NMI structures... but it is always best to check assumptions.
Can you elaborate on any testing that you have done? Have you tested this change on a real HW? If yes, which OS did you try booting? Have you tried any older OSes?
Thanks,
Nate
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
Detects the APIC mode by calling GetApicMode().
if current platform is in X2APIC mode sets the global mX2ApicEnabled.
Also install the APIC NMI structure only if platform is not in X2APIC mode.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 35 ++++++++++++--------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index f5c4f2c3f1a9..032903029a95 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -3,6 +3,7 @@
Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) Microsoft Corporation.<BR>
+Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -947,21 +948,23 @@ InstallMadtFromScratch (
//
// Build Local APIC NMI Structures
//
- LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
- LocalApciNmiStruct.Length = sizeof (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
- LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
- LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
- LocalApciNmiStruct.LocalApicLint = 0x1;
+ if (!mX2ApicEnabled) {
+ LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
+ LocalApciNmiStruct.Length = sizeof
+ (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
+ LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
+ LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
+ LocalApciNmiStruct.LocalApicLint = 0x1;
- ASSERT (MadtStructsIndex < MaxMadtStructCount);
- Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &LocalApciNmiStruct,
- &MadtStructs[MadtStructsIndex++]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
- goto Done;
+ ASSERT (MadtStructsIndex < MaxMadtStructCount);
+ Status = CopyStructure (
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &LocalApciNmiStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n",
+ Status));
+ goto Done;
+ }
}
//
@@ -1454,6 +1457,10 @@ InstallAcpiPlatform (
DEBUG ((DEBUG_INFO, "mNumberOfCpus - %d\n", mNumberOfCpus));
DEBUG ((DEBUG_INFO, "mNumberOfEnabledCPUs - %d\n", mNumberOfEnabledCPUs));
+ if (LOCAL_APIC_MODE_X2APIC == GetApicMode ()) {
+ mX2ApicEnabled = TRUE;
+ }
+
DEBUG ((DEBUG_INFO, "mX2ApicEnabled - 0x%x\n", mX2ApicEnabled));
DEBUG ((DEBUG_INFO, "mForceX2ApicId - 0x%x\n", mForceX2ApicId));
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-[PATCH v1 0/2] APIC table updates
2021-10-27 8:16 ` [edk2-[PATCH v1 0/2] APIC table updates Nate DeSimone
@ 2021-10-28 4:05 ` Attar, AbdulLateef (Abdul Lateef)
0 siblings, 0 replies; 12+ messages in thread
From: Attar, AbdulLateef (Abdul Lateef) @ 2021-10-28 4:05 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
[AMD Official Use Only]
Hi Nate,
I'm following below Leszlo's document to submit the patch.
Please let me know how to remove the groups.io web interface.
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers
Thanks
AbduL
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: 27 October 2021 13:47
To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2-[PATCH v1 0/2] APIC table updates
[CAUTION: External Email]
Hi Abdul,
First of all thank you for the patch series. One thing to take note of; unfortunately, the groups.io web interface messes up the line endings and makes it difficult to apply any patches sent through it. I was able to get your patch series to apply, but in the future, would it be possible for you to send your patches without using the groups.io web interface?
Thanks Again!
Nate
-----Original Message-----
From: Abdul Lateef Attar <abdattar@amd.com>
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-[PATCH v1 0/2] APIC table updates
Fixes the APIC table creation based on APIC mode.
Also fixes the AcpiProcessorUid for multi-socket system.
Abdul Lateef Attar (2):
MinPlatformPkg: Update APIC Table based on mode
MinPlatformPkg: AcpiProcessorUid for multi-socket
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 37 ++++++++++++--------
1 file changed, 22 insertions(+), 15 deletions(-)
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
2021-10-28 2:48 ` Nate DeSimone
@ 2021-10-28 12:50 ` Attar, AbdulLateef (Abdul Lateef)
2021-10-29 21:59 ` Nate DeSimone
0 siblings, 1 reply; 12+ messages in thread
From: Attar, AbdulLateef (Abdul Lateef) @ 2021-10-28 12:50 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
[Public]
Hi Nate,
I tested on windows 2019 and RHEL 8.3 and it works fine on both OSes.
Thanks
AbduL
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: 28 October 2021 08:19
To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
[CAUTION: External Email]
Hi Abdul,
I would prefer to clean up the technical debt and not have the APIC NMI if we are in X2APIC mode. Is there any chance you could test this change with Windows Server 2019? As long as semi-current Windows + Linux boots I'm fine with merging your patch as-is.
Thanks,
Nate
On 10/27/21, 9:33 AM, "Attar, AbdulLateef (Abdul Lateef)" <AbdulLateef.Attar@amd.com> wrote:
[Public]
Hi Nate,
I tested on real hardware with latest linux(didn't tested with older OSes).
Note that with X2APIC mode I don't see any issue having APIC NMI structure and not having NMI structure.
In both cases OS boots without any issue. Just thought that why to include APIC NMI structure in X2APIC mode.
Do you want me to resubmit the patch and include the APIC NMI for X2APIC mode?
Thanks
Abdul
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: 27 October 2021 13:47
To: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
[CAUTION: External Email]
Hi Abdul,
I do agree with you that logically it seems to be the right thing to not install the local APIC NMI structure if we install the X2APIC NMI structure instead. However, I went and took a look at some of our internal closed source code and it appears that even on X2APIC systems we actually install both NMI structures. My immediate reaction was to simply consider that a bug, but I am worried that this might have been done to workaround some buggy and/or legacy OS behavior. I've been trying to find more history on it but that behavior has been in our codebase for a very long time. IMHO it would be extremely weird for an OS to support X2APIC and have some bug handling X2APIC NMI structures... but it is always best to check assumptions.
Can you elaborate on any testing that you have done? Have you tested this change on a real HW? If yes, which OS did you try booting? Have you tried any older OSes?
Thanks,
Nate
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
Detects the APIC mode by calling GetApicMode().
if current platform is in X2APIC mode sets the global mX2ApicEnabled.
Also install the APIC NMI structure only if platform is not in X2APIC mode.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 35 ++++++++++++--------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index f5c4f2c3f1a9..032903029a95 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -3,6 +3,7 @@
Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) Microsoft Corporation.<BR>
+Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -947,21 +948,23 @@ InstallMadtFromScratch (
//
// Build Local APIC NMI Structures
//
- LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
- LocalApciNmiStruct.Length = sizeof (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
- LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
- LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
- LocalApciNmiStruct.LocalApicLint = 0x1;
+ if (!mX2ApicEnabled) {
+ LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
+ LocalApciNmiStruct.Length = sizeof
+ (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
+ LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
+ LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
+ LocalApciNmiStruct.LocalApicLint = 0x1;
- ASSERT (MadtStructsIndex < MaxMadtStructCount);
- Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &LocalApciNmiStruct,
- &MadtStructs[MadtStructsIndex++]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
- goto Done;
+ ASSERT (MadtStructsIndex < MaxMadtStructCount);
+ Status = CopyStructure (
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &LocalApciNmiStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n",
+ Status));
+ goto Done;
+ }
}
//
@@ -1454,6 +1457,10 @@ InstallAcpiPlatform (
DEBUG ((DEBUG_INFO, "mNumberOfCpus - %d\n", mNumberOfCpus));
DEBUG ((DEBUG_INFO, "mNumberOfEnabledCPUs - %d\n", mNumberOfEnabledCPUs));
+ if (LOCAL_APIC_MODE_X2APIC == GetApicMode ()) {
+ mX2ApicEnabled = TRUE;
+ }
+
DEBUG ((DEBUG_INFO, "mX2ApicEnabled - 0x%x\n", mX2ApicEnabled));
DEBUG ((DEBUG_INFO, "mForceX2ApicId - 0x%x\n", mForceX2ApicId));
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
2021-10-28 12:50 ` Attar, AbdulLateef (Abdul Lateef)
@ 2021-10-29 21:59 ` Nate DeSimone
0 siblings, 0 replies; 12+ messages in thread
From: Nate DeSimone @ 2021-10-29 21:59 UTC (permalink / raw)
To: Attar, AbdulLateef (Abdul Lateef), devel@edk2.groups.io
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
Hi Abdul,
Excellent! I checked with the Windows kernel team as well and they don't believe there should be any issues. Therefore, I am approving your patch series as it is.
Thanks,
Nate
Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
-----Original Message-----
From: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>
Sent: Thursday, October 28, 2021 5:50 AM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
[Public]
Hi Nate,
I tested on windows 2019 and RHEL 8.3 and it works fine on both OSes.
Thanks
AbduL
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: 28 October 2021 08:19
To: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
[CAUTION: External Email]
Hi Abdul,
I would prefer to clean up the technical debt and not have the APIC NMI if we are in X2APIC mode. Is there any chance you could test this change with Windows Server 2019? As long as semi-current Windows + Linux boots I'm fine with merging your patch as-is.
Thanks,
Nate
On 10/27/21, 9:33 AM, "Attar, AbdulLateef (Abdul Lateef)" <AbdulLateef.Attar@amd.com> wrote:
[Public]
Hi Nate,
I tested on real hardware with latest linux(didn't tested with older OSes).
Note that with X2APIC mode I don't see any issue having APIC NMI structure and not having NMI structure.
In both cases OS boots without any issue. Just thought that why to include APIC NMI structure in X2APIC mode.
Do you want me to resubmit the patch and include the APIC NMI for X2APIC mode?
Thanks
Abdul
-----Original Message-----
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: 27 October 2021 13:47
To: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
[CAUTION: External Email]
Hi Abdul,
I do agree with you that logically it seems to be the right thing to not install the local APIC NMI structure if we install the X2APIC NMI structure instead. However, I went and took a look at some of our internal closed source code and it appears that even on X2APIC systems we actually install both NMI structures. My immediate reaction was to simply consider that a bug, but I am worried that this might have been done to workaround some buggy and/or legacy OS behavior. I've been trying to find more history on it but that behavior has been in our codebase for a very long time. IMHO it would be extremely weird for an OS to support X2APIC and have some bug handling X2APIC NMI structures... but it is always best to check assumptions.
Can you elaborate on any testing that you have done? Have you tested this change on a real HW? If yes, which OS did you try booting? Have you tried any older OSes?
Thanks,
Nate
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Abdul Lateef Attar via groups.io
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-devel] [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode
Detects the APIC mode by calling GetApicMode().
if current platform is in X2APIC mode sets the global mX2ApicEnabled.
Also install the APIC NMI structure only if platform is not in X2APIC mode.
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Abdul Lateef Attar <abdattar@amd.com>
---
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 35 ++++++++++++--------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
index f5c4f2c3f1a9..032903029a95 100644
--- a/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
+++ b/Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c
@@ -3,6 +3,7 @@
Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) Microsoft Corporation.<BR>
+Copyright (c) 2021, AMD Incorporated. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -947,21 +948,23 @@ InstallMadtFromScratch (
//
// Build Local APIC NMI Structures
//
- LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
- LocalApciNmiStruct.Length = sizeof (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
- LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
- LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
- LocalApciNmiStruct.LocalApicLint = 0x1;
+ if (!mX2ApicEnabled) {
+ LocalApciNmiStruct.Type = EFI_ACPI_6_3_LOCAL_APIC_NMI;
+ LocalApciNmiStruct.Length = sizeof
+ (EFI_ACPI_6_3_LOCAL_APIC_NMI_STRUCTURE);
+ LocalApciNmiStruct.AcpiProcessorUid = 0xFF; // Applies to all processors
+ LocalApciNmiStruct.Flags = 0x0005; // Flags - Edge-tiggered, Active High
+ LocalApciNmiStruct.LocalApicLint = 0x1;
- ASSERT (MadtStructsIndex < MaxMadtStructCount);
- Status = CopyStructure (
- &MadtTableHeader.Header,
- (STRUCTURE_HEADER *) &LocalApciNmiStruct,
- &MadtStructs[MadtStructsIndex++]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n", Status));
- goto Done;
+ ASSERT (MadtStructsIndex < MaxMadtStructCount);
+ Status = CopyStructure (
+ &MadtTableHeader.Header,
+ (STRUCTURE_HEADER *) &LocalApciNmiStruct,
+ &MadtStructs[MadtStructsIndex++]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "CopyMadtStructure (APIC NMI) failed: %r\n",
+ Status));
+ goto Done;
+ }
}
//
@@ -1454,6 +1457,10 @@ InstallAcpiPlatform (
DEBUG ((DEBUG_INFO, "mNumberOfCpus - %d\n", mNumberOfCpus));
DEBUG ((DEBUG_INFO, "mNumberOfEnabledCPUs - %d\n", mNumberOfEnabledCPUs));
+ if (LOCAL_APIC_MODE_X2APIC == GetApicMode ()) {
+ mX2ApicEnabled = TRUE;
+ }
+
DEBUG ((DEBUG_INFO, "mX2ApicEnabled - 0x%x\n", mX2ApicEnabled));
DEBUG ((DEBUG_INFO, "mForceX2ApicId - 0x%x\n", mForceX2ApicId));
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-[PATCH v1 0/2] APIC table updates
2021-10-22 14:47 [edk2-[PATCH v1 0/2] APIC table updates Abdul Lateef Attar
` (2 preceding siblings ...)
2021-10-27 8:16 ` [edk2-[PATCH v1 0/2] APIC table updates Nate DeSimone
@ 2021-11-02 21:42 ` Nate DeSimone
3 siblings, 0 replies; 12+ messages in thread
From: Nate DeSimone @ 2021-11-02 21:42 UTC (permalink / raw)
To: Abdul Lateef Attar, devel@edk2.groups.io
Cc: Chiu, Chasel, Liming Gao, Dong, Eric
The series has been pushed as 34df404~..30865b8
-----Original Message-----
From: Abdul Lateef Attar <abdattar@amd.com>
Sent: Friday, October 22, 2021 7:48 AM
To: devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
Subject: [edk2-[PATCH v1 0/2] APIC table updates
Fixes the APIC table creation based on APIC mode.
Also fixes the AcpiProcessorUid for multi-socket system.
Abdul Lateef Attar (2):
MinPlatformPkg: Update APIC Table based on mode
MinPlatformPkg: AcpiProcessorUid for multi-socket
Platform/Intel/MinPlatformPkg/Acpi/AcpiTables/AcpiPlatform.c | 37 ++++++++++++--------
1 file changed, 22 insertions(+), 15 deletions(-)
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-11-02 21:43 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-22 14:47 [edk2-[PATCH v1 0/2] APIC table updates Abdul Lateef Attar
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 1/2] MinPlatformPkg: Update APIC Table based on mode Abdul Lateef Attar
2021-10-27 8:16 ` [edk2-devel] " Nate DeSimone
2021-10-27 16:33 ` Attar, AbdulLateef (Abdul Lateef)
2021-10-28 2:48 ` Nate DeSimone
2021-10-28 12:50 ` Attar, AbdulLateef (Abdul Lateef)
2021-10-29 21:59 ` Nate DeSimone
2021-10-22 14:47 ` [edk2-platforms] [PATCH v1 2/2] MinPlatformPkg: AcpiProcessorUid for multi-socket Abdul Lateef Attar
2021-10-27 8:16 ` [edk2-devel] " Nate DeSimone
2021-10-27 8:16 ` [edk2-[PATCH v1 0/2] APIC table updates Nate DeSimone
2021-10-28 4:05 ` Attar, AbdulLateef (Abdul Lateef)
2021-11-02 21:42 ` Nate DeSimone
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox