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.at...@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.desim...@intel.com>
Sent: 27 October 2021 13:47
To: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef)
<abdullateef.at...@amd.com>
Cc: Chiu, Chasel <chasel.c...@intel.com>; Liming Gao
<gaolim...@byosoft.com.cn>; Dong, Eric <eric.d...@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.c...@intel.com>; Desimone, Nathaniel L
<nathaniel.l.desim...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; Dong,
Eric <eric.d...@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.c...@intel.com>
Cc: Nate DeSimone <nathaniel.l.desim...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Eric Dong <eric.d...@intel.com>
Signed-off-by: Abdul Lateef Attar <abdat...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82787): https://edk2.groups.io/g/devel/message/82787
Mute This Topic: https://groups.io/mt/86515674/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
