OvmfPkg: Q35: Use correct ACPI PM control register:bit

On PIIX4, function 3, the PMREGMISC register at offset 0x80, with
default value 0x00 has its bit 0 (PMIOSE) indicate whether the PM
IO space given in the PMBA register (offset 0x40) is enabled.
PMBA must be configured *before* setting this bit.

On Q35/ICH9+, function 0x1f, the equivalent role is fulfilled by
bit 7 (ACPI_EN) in the ACPI Control Register (ACPI_CNTL) at offset
0x44, also with a default value of 0x00.

Currently, OVMF hangs when Q35 reboots, because while PMBA is reset
by QEMU, the register at offset 0x80 (matching PMREGMISC on PIIX4)
is not reset, since it has a completely different meaning on LPC.
As such, the power management initialization logic in OVMF finds
the "PMIOSE" bit enabled after a reboot and decides to skip setting
PMBA. This causes the ACPI timer tick routine to read a constant
value from the wrong register, which in turn causes the ACPI delay
loop to hang indefinitely.

This patch modifies the Base[Rom]AcpiTimerLib constructors and the
PlatformPei ACPI PM init routines to use ACPI_CNTL:ACPI_EN instead
of PMREGMISC:PMIOSE when running on Q35.

Reported-by: Reza Jelveh <reza.jelveh@tuhh.de>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@17076 6f19259b-4bc3-4df7-8a09-765794883524

3 files changed, 45 insertions, 34 deletions

diff --git a/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c b/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
index 2828053..58938b5 100644
--- a/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
+++ b/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c
@@ -21,8 +21,9 @@
 //

 // Power Management PCI Configuration Register fields

 //

-#define PMBA_RTE  BIT0

-#define PMIOSE    BIT0

+#define PMBA_RTE      BIT0

+#define PIIX4_PMIOSE  BIT0

+#define Q35_ACPI_EN   BIT7

 

 //

 // Offset in the Power Management Base Address to the ACPI Timer

@@ -49,7 +50,8 @@ AcpiTimerLibConstructor (
 {

   UINT16 HostBridgeDevId;

   UINTN Pmba;

-  UINTN PmRegMisc;

+  UINTN AcpiCtlReg;

+  UINT8 AcpiEnBit;

 

   //

   // Query Host Bridge DID to determine platform type

@@ -57,12 +59,14 @@ AcpiTimerLibConstructor (
   HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);

   switch (HostBridgeDevId) {

     case INTEL_82441_DEVICE_ID:

-      Pmba      = POWER_MGMT_REGISTER_PIIX4 (0x40);

-      PmRegMisc = POWER_MGMT_REGISTER_PIIX4 (0x80);

+      Pmba       = POWER_MGMT_REGISTER_PIIX4 (0x40);

+      AcpiCtlReg = POWER_MGMT_REGISTER_PIIX4 (0x80); // PMREGMISC

+      AcpiEnBit  = PIIX4_PMIOSE;

       break;

     case INTEL_Q35_MCH_DEVICE_ID:

-      Pmba      = POWER_MGMT_REGISTER_Q35 (0x40);

-      PmRegMisc = POWER_MGMT_REGISTER_Q35 (0x80);

+      Pmba       = POWER_MGMT_REGISTER_Q35 (0x40);

+      AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (0x44); // ACPI_CNTL

+      AcpiEnBit  = Q35_ACPI_EN;

       break;

     default:

       DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",

@@ -76,7 +80,7 @@ AcpiTimerLibConstructor (
   //

   // Check to see if the Power Management Base Address is already enabled

   //

-  if ((PciRead8 (PmRegMisc) & PMIOSE) == 0) {

+  if ((PciRead8 (AcpiCtlReg) & AcpiEnBit) == 0) {

     //

     // If the Power Management Base Address is not programmed,

     // then program the Power Management Base Address from a PCD.

@@ -84,9 +88,9 @@ AcpiTimerLibConstructor (
     PciAndThenOr32 (Pmba, (UINT32) ~0xFFC0, PcdGet16 (PcdAcpiPmBaseAddress));

 

     //

-    // Enable PMBA I/O port decodes in PMREGMISC

+    // Enable PMBA I/O port decodes

     //

-    PciOr8 (PmRegMisc, PMIOSE);

+    PciOr8 (AcpiCtlReg, AcpiEnBit);

   }

 

   return RETURN_SUCCESS;

diff --git a/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c b/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c
index 05b14d7..c9601d8 100644
--- a/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c
+++ b/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c
@@ -22,8 +22,9 @@
 //

 // Power Management PCI Configuration Register fields

 //

-#define PMBA_RTE  BIT0

-#define PMIOSE    BIT0

+#define PMBA_RTE      BIT0

+#define PIIX4_PMIOSE  BIT0

+#define Q35_ACPI_EN   BIT7

 

 //

 // Offset in the Power Management Base Address to the ACPI Timer

@@ -47,7 +48,8 @@ AcpiTimerLibConstructor (
 {

   UINT16 HostBridgeDevId;

   UINTN Pmba;

-  UINTN PmRegMisc;

+  UINTN AcpiCtlReg;

+  UINT8 AcpiEnBit;

 

   //

   // Query Host Bridge DID to determine platform type

@@ -55,12 +57,14 @@ AcpiTimerLibConstructor (
   HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);

   switch (HostBridgeDevId) {

     case INTEL_82441_DEVICE_ID:

-      Pmba      = POWER_MGMT_REGISTER_PIIX4 (0x40);

-      PmRegMisc = POWER_MGMT_REGISTER_PIIX4 (0x80);

+      Pmba       = POWER_MGMT_REGISTER_PIIX4 (0x40);

+      AcpiCtlReg = POWER_MGMT_REGISTER_PIIX4 (0x80); // PMREGMISC

+      AcpiEnBit  = PIIX4_PMIOSE;

       break;

     case INTEL_Q35_MCH_DEVICE_ID:

-      Pmba      = POWER_MGMT_REGISTER_Q35 (0x40);

-      PmRegMisc = POWER_MGMT_REGISTER_Q35 (0x80);

+      Pmba       = POWER_MGMT_REGISTER_Q35 (0x40);

+      AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (0x44); // ACPI_CNTL

+      AcpiEnBit  = Q35_ACPI_EN;

       break;

     default:

       DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",

@@ -72,7 +76,7 @@ AcpiTimerLibConstructor (
   //

   // Check to see if the Power Management Base Address is already enabled

   //

-  if ((PciRead8 (PmRegMisc) & PMIOSE) == 0) {

+  if ((PciRead8 (AcpiCtlReg) & AcpiEnBit) == 0) {

     //

     // If the Power Management Base Address is not programmed,

     // then program the Power Management Base Address from a PCD.

@@ -80,9 +84,9 @@ AcpiTimerLibConstructor (
     PciAndThenOr32 (Pmba, (UINT32) ~0xFFC0, PcdGet16 (PcdAcpiPmBaseAddress));

 

     //

-    // Enable PMBA I/O port decodes in PMREGMISC

+    // Enable PMBA I/O port decodes

     //

-    PciOr8 (PmRegMisc, PMIOSE);

+    PciOr8 (AcpiCtlReg, AcpiEnBit);

   }

 

   return RETURN_SUCCESS;

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 1940e01..5ce0a90 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -232,7 +232,8 @@ MiscInitialization (
   UINT16 HostBridgeDevId;

   UINTN  PmCmd;

   UINTN  Pmba;

-  UINTN  PmRegMisc;

+  UINTN  AcpiCtlReg;

+  UINT8  AcpiEnBit;

 

   //

   // Disable A20 Mask

@@ -250,14 +251,16 @@ MiscInitialization (
   HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);

   switch (HostBridgeDevId) {

     case INTEL_82441_DEVICE_ID:

-      PmCmd     = POWER_MGMT_REGISTER_PIIX4 (PCI_COMMAND_OFFSET);

-      Pmba      = POWER_MGMT_REGISTER_PIIX4 (0x40);

-      PmRegMisc = POWER_MGMT_REGISTER_PIIX4 (0x80);

+      PmCmd      = POWER_MGMT_REGISTER_PIIX4 (PCI_COMMAND_OFFSET);

+      Pmba       = POWER_MGMT_REGISTER_PIIX4 (0x40);

+      AcpiCtlReg = POWER_MGMT_REGISTER_PIIX4 (0x80); // PMREGMISC

+      AcpiEnBit  = BIT0; // PIIX4_PMIOSE

       break;

     case INTEL_Q35_MCH_DEVICE_ID:

-      PmCmd     = POWER_MGMT_REGISTER_Q35 (PCI_COMMAND_OFFSET);

-      Pmba      = POWER_MGMT_REGISTER_Q35 (0x40);

-      PmRegMisc = POWER_MGMT_REGISTER_Q35 (0x80);

+      PmCmd      = POWER_MGMT_REGISTER_Q35 (PCI_COMMAND_OFFSET);

+      Pmba       = POWER_MGMT_REGISTER_Q35 (0x40);

+      AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (0x44); // ACPI_CNTL

+      AcpiEnBit  = BIT7; // Q35_ACPI_EN

       break;

     default:

       DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",

@@ -268,13 +271,13 @@ MiscInitialization (
   PcdSet16 (PcdOvmfHostBridgePciDevId, HostBridgeDevId);

 

   //

-  // If PMREGMISC/PMIOSE is set, assume the ACPI PMBA has been configured (for

-  // example by Xen) and skip the setup here. This matches the logic in

-  // AcpiTimerLibConstructor ().

+  // If the appropriate IOspace enable bit is set, assume the ACPI PMBA

+  // has been configured (e.g., by Xen) and skip the setup here.

+  // This matches the logic in AcpiTimerLibConstructor ().

   //

-  if ((PciRead8 (PmRegMisc) & 0x01) == 0) {

+  if ((PciRead8 (AcpiCtlReg) & AcpiEnBit) == 0) {

     //

-    // The PEI phase should be exited with fully accessibe PIIX4 IO space:

+    // The PEI phase should be exited with fully accessibe ACPI PM IO space:

     // 1. set PMBA

     //

     PciAndThenOr32 (Pmba, (UINT32) ~0xFFC0, PcdGet16 (PcdAcpiPmBaseAddress));

@@ -285,9 +288,9 @@ MiscInitialization (
     PciOr8 (PmCmd, EFI_PCI_COMMAND_IO_SPACE);

 

     //

-    // 3. set PMREGMISC/PMIOSE

+    // 3. set ACPI PM IO enable bit (PMREGMISC:PMIOSE or ACPI_CNTL:ACPI_EN)

     //

-    PciOr8 (PmRegMisc, 0x01);

+    PciOr8 (AcpiCtlReg, AcpiEnBit);

   }

 }