On 2015-03-26 09:02:14, Laszlo Ersek wrote: > On 03/26/15 16:02, Gabriel L. Somlo wrote: > > 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+, 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 routine to hang indefinitely. > > > > This patch modifies the BaseRomAcpiTimerLib constructor and the > > I'll fix this up to name both BaseRomAcpiTimerLib and BaseAcpiTimerLib > when committing. > > Reviewed-by: Laszlo Ersek <[email protected]> > > Jordan, are you too okay with this patch? If so, I can commit it.
We should add Reported-by: Reza Jelveh <[email protected]> > > PlatformPei ACPI PM init routines to use ACPI_CNTL:ACPI_EN instead > > of PMREGMISC:PMIOSE when running on Q35. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Gabriel Somlo <[email protected]> > > --- > > OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c | 20 ++++++++------ > > OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c | 20 ++++++++------ > > OvmfPkg/PlatformPei/Platform.c | 31 > > ++++++++++++---------- > > 3 files changed, 41 insertions(+), 30 deletions(-) > > > > diff --git a/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c > > b/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c > > index 2828053..c3e5b67 100644 > > --- a/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c > > +++ b/OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.c > > @@ -23,6 +23,7 @@ > > // > > #define PMBA_RTE BIT0 > > #define PMIOSE BIT0 > > +#define ACPI_EN BIT7 Perhaps we should rename PMIOSE to PIIX4_PMIOSE and add this as Q35_ACPI_EN? The patch seems good though Reviewed-by: Jordan Justen <[email protected]> > > > > // > > // 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 = 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 = 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..5a07bd3 100644 > > --- a/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c > > +++ b/OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.c > > @@ -24,6 +24,7 @@ > > // > > #define PMBA_RTE BIT0 > > #define PMIOSE BIT0 > > +#define 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 = 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 = 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..e131b6a 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; // 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; // 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); > > } > > } > > > > > ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
