Hi,
these patches provide a small set of improvements and cleanups for the current SMM code, making it more consistent with the chipset's datasheets. QEMU does not yet implement most of the new registers used here, but it might soon...
Paolo Bonzini (5): piix: add and use dev-piix.h smm: remove code to handle ACPI disable/enable smm: complete SMM setup smm: unify SMM handlers smm: communicate end of SMI to chipset
src/fw/acpi.c | 18 ++++------ src/fw/dev-piix.h | 32 ++++++++++++++++++ src/fw/dev-q35.h | 6 ++++ src/fw/pciinit.c | 13 ++++---- src/fw/shadow.c | 3 +- src/fw/smm.c | 99 ++++++++++++++++++++++++++++--------------------------- 6 files changed, 103 insertions(+), 68 deletions(-) create mode 100644 src/fw/dev-piix.h
Move all definitions for PIIX registers to a single header file, like there is one already for Q35, and make the naming more consistent.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/fw/acpi.c | 18 ++++++------------ src/fw/dev-piix.h | 27 +++++++++++++++++++++++++++ src/fw/pciinit.c | 13 +++++++------ src/fw/shadow.c | 3 +-- src/fw/smm.c | 9 +++------ 5 files changed, 44 insertions(+), 26 deletions(-) create mode 100644 src/fw/dev-piix.h
diff --git a/src/fw/acpi.c b/src/fw/acpi.c index 5ad2eec..5f751e6 100644 --- a/src/fw/acpi.c +++ b/src/fw/acpi.c @@ -9,6 +9,7 @@ #include "byteorder.h" // cpu_to_le16 #include "config.h" // CONFIG_* #include "dev-q35.h" +#include "dev-piix.h" #include "hw/pci.h" // pci_find_init_device #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL #include "hw/pci_regs.h" // PCI_INTERRUPT_LINE @@ -38,31 +39,24 @@ build_header(struct acpi_table_header *h, u32 sig, int len, u8 rev) h->checksum -= checksum(h, len); }
-#define PIIX4_ACPI_ENABLE 0xf1 -#define PIIX4_ACPI_DISABLE 0xf0 -#define PIIX4_GPE0_BLK 0xafe0 -#define PIIX4_GPE0_BLK_LEN 4 - -#define PIIX4_PM_INTRRUPT 9 // irq 9 - static void piix4_fadt_setup(struct pci_device *pci, void *arg) { struct fadt_descriptor_rev1 *fadt = arg;
fadt->model = 1; fadt->reserved1 = 0; - fadt->sci_int = cpu_to_le16(PIIX4_PM_INTRRUPT); + fadt->sci_int = cpu_to_le16(PIIX_PM_INTRRUPT); fadt->smi_cmd = cpu_to_le32(PORT_SMI_CMD); - fadt->acpi_enable = PIIX4_ACPI_ENABLE; - fadt->acpi_disable = PIIX4_ACPI_DISABLE; + fadt->acpi_enable = PIIX_ACPI_ENABLE; + fadt->acpi_disable = PIIX_ACPI_DISABLE; fadt->pm1a_evt_blk = cpu_to_le32(PORT_ACPI_PM_BASE); fadt->pm1a_cnt_blk = cpu_to_le32(PORT_ACPI_PM_BASE + 0x04); fadt->pm_tmr_blk = cpu_to_le32(PORT_ACPI_PM_BASE + 0x08); - fadt->gpe0_blk = cpu_to_le32(PIIX4_GPE0_BLK); + fadt->gpe0_blk = cpu_to_le32(PIIX_GPE0_BLK); fadt->pm1_evt_len = 4; fadt->pm1_cnt_len = 2; fadt->pm_tmr_len = 4; - fadt->gpe0_blk_len = PIIX4_GPE0_BLK_LEN; + fadt->gpe0_blk_len = PIIX_GPE0_BLK_LEN; fadt->plvl2_lat = cpu_to_le16(0xfff); // C2 state not supported fadt->plvl3_lat = cpu_to_le16(0xfff); // C3 state not supported /* WBINVD + PROC_C1 + SLP_BUTTON + RTC_S4 + USE_PLATFORM_CLOCK */ diff --git a/src/fw/dev-piix.h b/src/fw/dev-piix.h new file mode 100644 index 0000000..c6dce03 --- /dev/null +++ b/src/fw/dev-piix.h @@ -0,0 +1,27 @@ +#ifndef __DEV_PIIX_H +#define __DEV_PIIX_H + +#define I440FX_PAM0 0x59 +#define I440FX_SMRAM 0x72 + +#define PIIX_PMBASE 0x40 +#define PIIX_PMREGMISC 0x80 +#define PIIX_SMBHSTBASE 0x90 +#define PIIX_SMBHSTCFG 0xd2 +#define PIIX_DEVACTB 0x58 +#define PIIX_DEVACTB_APMC_EN (1 << 25) + +#define PIIX_PORT_ELCR1 0x4d0 +#define PIIX_PORT_ELCR2 0x4d1 + +/* ICH9 PM I/O registers */ +#define PIIX_GPE0_BLK 0xafe0 +#define PIIX_GPE0_BLK_LEN 4 + +/* FADT ACPI_ENABLE/ACPI_DISABLE */ +#define PIIX_ACPI_ENABLE 0xf1 +#define PIIX_ACPI_DISABLE 0xf0 + +#define PIIX_PM_INTRRUPT 9 // irq 9 + +#endif // dev-piix.h diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index bbaecd6..8112539 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -8,6 +8,7 @@ #include "byteorder.h" // le64_to_cpu #include "config.h" // CONFIG_* #include "dev-q35.h" // Q35_HOST_BRIDGE_PCIEXBAR_ADDR +#include "dev-piix.h" // PIIX_* #include "hw/ata.h" // PORT_ATA1_CMD_BASE #include "hw/pci.h" // pci_config_readl #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL @@ -151,8 +152,8 @@ static void piix_isa_bridge_setup(struct pci_device *pci, void *arg) /* activate irq remapping in PIIX */ pci_config_writeb(pci->bdf, 0x60 + i, irq); } - outb(elcr[0], 0x4d0); - outb(elcr[1], 0x4d1); + outb(elcr[0], PIIX_PORT_ELCR1); + outb(elcr[1], PIIX_PORT_ELCR2); dprintf(1, "PIIX3/PIIX4 init: elcr=%02x %02x\n", elcr[0], elcr[1]); }
@@ -228,10 +229,10 @@ static void piix4_pm_config_setup(u16 bdf) // acpi sci is hardwired to 9 pci_config_writeb(bdf, PCI_INTERRUPT_LINE, 9);
- pci_config_writel(bdf, 0x40, PORT_ACPI_PM_BASE | 1); - pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ - pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); - pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ + pci_config_writel(bdf, PIIX_PMBASE, PORT_ACPI_PM_BASE | 1); + pci_config_writeb(bdf, PIIX_PMREGMISC, 0x01); /* enable PM io space */ + pci_config_writel(bdf, PIIX_SMBHSTBASE, PORT_SMB_BASE | 1); + pci_config_writeb(bdf, PIIX_SMBHSTCFG, 0x09); /* enable SMBus io space */ }
static int PiixPmBDF = -1; diff --git a/src/fw/shadow.c b/src/fw/shadow.c index 82d6753..4f00006 100644 --- a/src/fw/shadow.c +++ b/src/fw/shadow.c @@ -7,6 +7,7 @@
#include "config.h" // CONFIG_* #include "dev-q35.h" // PCI_VENDOR_ID_INTEL +#include "dev-piix.h" // I440FX_PAM0 #include "hw/pci.h" // pci_config_writeb #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL #include "hw/pci_regs.h" // PCI_VENDOR_ID @@ -20,8 +21,6 @@ // On the emulators, the bios at 0xf0000 is also at 0xffff0000 #define BIOS_SRC_OFFSET 0xfff00000
-#define I440FX_PAM0 0x59 - // Enable shadowing and copy bios. static void __make_bios_writable_intel(u16 bdf, u32 pam0) diff --git a/src/fw/smm.c b/src/fw/smm.c index 1032ffb..4176e0c 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -7,6 +7,7 @@
#include "config.h" // CONFIG_* #include "dev-q35.h" +#include "dev-piix.h" #include "hw/pci.h" // pci_config_writel #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL #include "hw/pci_regs.h" // PCI_DEVICE_ID @@ -111,16 +112,12 @@ smm_relocate_and_restore(void) wbinvd(); }
-#define I440FX_SMRAM 0x72 -#define PIIX_DEVACTB 0x58 -#define PIIX_APMC_EN (1 << 25) - // This code is hardcoded for PIIX4 Power Management device. static void piix4_apmc_smm_setup(int isabdf, int i440_bdf) { /* check if SMM init is already done */ u32 value = pci_config_readl(isabdf, PIIX_DEVACTB); - if (value & PIIX_APMC_EN) + if (value & PIIX_DEVACTB_APMC_EN) return;
/* enable the SMM memory window */ @@ -129,7 +126,7 @@ static void piix4_apmc_smm_setup(int isabdf, int i440_bdf) smm_save_and_copy();
/* enable SMI generation when writing to the APMC register */ - pci_config_writel(isabdf, PIIX_DEVACTB, value | PIIX_APMC_EN); + pci_config_writel(isabdf, PIIX_DEVACTB, value | PIIX_DEVACTB_APMC_EN);
smm_relocate_and_restore();
This is handled already in QEMU, no need to do it in SMM.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/fw/smm.c | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/src/fw/smm.c b/src/fw/smm.c index 4176e0c..07f9234 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -46,34 +46,9 @@ ASM32FLAT(
extern u8 smm_code_start, smm_code_end; ASM32FLAT( - /* minimal SMM code to enable or disable ACPI */ ".global smm_code_start, smm_code_end\n" " .code16gcc\n" "smm_code_start:\n" - " movw $" __stringify(PORT_SMI_CMD) ", %dx\n" - " inb %dx, %al\n" - " cmpb $0xf0, %al\n" - " jne 1f\n" - - /* ACPI disable */ - " movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */ - " inw %dx, %ax\n" - " andw $~1, %ax\n" - " outw %ax, %dx\n" - - " jmp 2f\n" - - "1:\n" - " cmpb $0xf1, %al\n" - " jne 2f\n" - - /* ACPI enable */ - " movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */ - " inw %dx, %ax\n" - " orw $1, %ax\n" - " outw %ax, %dx\n" - - "2:\n" " rsm\n" "smm_code_end:\n" " .code32\n"
On Thu, 15 May 2014 13:22:27 +0200 Paolo Bonzini pbonzini@redhat.com wrote:
This is handled already in QEMU, no need to do it in SMM.
Is it needed by coreboot?
-d
Signed-off-by: Paolo Bonzini pbonzini@redhat.com
src/fw/smm.c | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/src/fw/smm.c b/src/fw/smm.c index 4176e0c..07f9234 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -46,34 +46,9 @@ ASM32FLAT(
extern u8 smm_code_start, smm_code_end; ASM32FLAT(
- /* minimal SMM code to enable or disable ACPI */ ".global smm_code_start, smm_code_end\n" " .code16gcc\n" "smm_code_start:\n"
- " movw $" __stringify(PORT_SMI_CMD) ", %dx\n"
- " inb %dx, %al\n"
- " cmpb $0xf0, %al\n"
- " jne 1f\n"
- /* ACPI disable */
- " movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
- " inw %dx, %ax\n"
- " andw $~1, %ax\n"
- " outw %ax, %dx\n"
- " jmp 2f\n"
- "1:\n"
- " cmpb $0xf1, %al\n"
- " jne 2f\n"
- /* ACPI enable */
- " movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
- " inw %dx, %ax\n"
- " orw $1, %ax\n"
- " outw %ax, %dx\n"
- "2:\n" " rsm\n" "smm_code_end:\n" " .code32\n"
-- 1.9.0
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Il 15/05/2014 14:56, Don Koch ha scritto:
On Thu, 15 May 2014 13:22:27 +0200 Paolo Bonzini pbonzini@redhat.com wrote:
This is handled already in QEMU, no need to do it in SMM.
Is it needed by coreboot?
This code doesn't run on coreboot.
Paolo
-d
Signed-off-by: Paolo Bonzini pbonzini@redhat.com
src/fw/smm.c | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/src/fw/smm.c b/src/fw/smm.c index 4176e0c..07f9234 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -46,34 +46,9 @@ ASM32FLAT(
extern u8 smm_code_start, smm_code_end; ASM32FLAT(
- /* minimal SMM code to enable or disable ACPI */ ".global smm_code_start, smm_code_end\n" " .code16gcc\n" "smm_code_start:\n"
- " movw $" __stringify(PORT_SMI_CMD) ", %dx\n"
- " inb %dx, %al\n"
- " cmpb $0xf0, %al\n"
- " jne 1f\n"
- /* ACPI disable */
- " movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
- " inw %dx, %ax\n"
- " andw $~1, %ax\n"
- " outw %ax, %dx\n"
- " jmp 2f\n"
- "1:\n"
- " cmpb $0xf1, %al\n"
- " jne 2f\n"
- /* ACPI enable */
- " movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
- " inw %dx, %ax\n"
- " orw $1, %ax\n"
- " outw %ax, %dx\n"
- "2:\n" " rsm\n" "smm_code_end:\n" " .code32\n"
-- 1.9.0
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Il 15/05/2014 14:56, Don Koch ha scritto:
On Thu, 15 May 2014 13:22:27 +0200 Paolo Bonzini pbonzini@redhat.com wrote:
This is handled already in QEMU, no need to do it in SMM.
Is it needed by coreboot?
CoreBoot does not run this code at all.
Paolo
-d
Signed-off-by: Paolo Bonzini pbonzini@redhat.com
src/fw/smm.c | 25 ------------------------- 1 file changed, 25 deletions(-)
diff --git a/src/fw/smm.c b/src/fw/smm.c index 4176e0c..07f9234 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -46,34 +46,9 @@ ASM32FLAT(
extern u8 smm_code_start, smm_code_end; ASM32FLAT(
- /* minimal SMM code to enable or disable ACPI */ ".global smm_code_start, smm_code_end\n" " .code16gcc\n" "smm_code_start:\n"
- " movw $" __stringify(PORT_SMI_CMD) ", %dx\n"
- " inb %dx, %al\n"
- " cmpb $0xf0, %al\n"
- " jne 1f\n"
- /* ACPI disable */
- " movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
- " inw %dx, %ax\n"
- " andw $~1, %ax\n"
- " outw %ax, %dx\n"
- " jmp 2f\n"
- "1:\n"
- " cmpb $0xf1, %al\n"
- " jne 2f\n"
- /* ACPI enable */
- " movw $" __stringify(PORT_ACPI_PM_BASE) " + 0x04, %dx\n" /* PMCNTRL */
- " inw %dx, %ax\n"
- " orw $1, %ax\n"
- " outw %ax, %dx\n"
- "2:\n" " rsm\n" "smm_code_end:\n" " .code32\n"
-- 1.9.0
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
On Do, 2014-05-15 at 08:56 -0400, Don Koch wrote:
This is handled already in QEMU, no need to do it in SMM.
Is it needed by coreboot?
No. The smm code doesn't run at all with coreboot builds. Beside that coreboot installs its own smm handler in case it needs one.
cheers, Gerd
SMI generation requires two bits to be set in PIIX4, one for APMC interrupts specifically and a general one.
For Q35 it is the same, plus it is a good thing to lock SMIs after enabling them.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/fw/dev-piix.h | 2 ++ src/fw/dev-q35.h | 3 +++ src/fw/smm.c | 12 +++++++++++- 3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/fw/dev-piix.h b/src/fw/dev-piix.h index c6dce03..c389f17 100644 --- a/src/fw/dev-piix.h +++ b/src/fw/dev-piix.h @@ -17,6 +17,8 @@ /* ICH9 PM I/O registers */ #define PIIX_GPE0_BLK 0xafe0 #define PIIX_GPE0_BLK_LEN 4 +#define PIIX_PMIO_GLBCTL 0x28 +#define PIIX_PMIO_GLBCTL_SMI_EN 1
/* FADT ACPI_ENABLE/ACPI_DISABLE */ #define PIIX_ACPI_ENABLE 0xf1 diff --git a/src/fw/dev-q35.h b/src/fw/dev-q35.h index 6ae039f..c6f8bd9 100644 --- a/src/fw/dev-q35.h +++ b/src/fw/dev-q35.h @@ -23,6 +23,8 @@ #define ICH9_LPC_PIRQA_ROUT 0x60 #define ICH9_LPC_PIRQE_ROUT 0x68 #define ICH9_LPC_PIRQ_ROUT_IRQEN 0x80 +#define ICH9_LPC_GEN_PMCON_1 0xa0 +#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK (1 << 4) #define ICH9_LPC_PORT_ELCR1 0x4d0 #define ICH9_LPC_PORT_ELCR2 0x4d1 #define PCI_DEVICE_ID_INTEL_ICH9_SMBUS 0x2930 @@ -38,6 +40,7 @@ #define ICH9_PMIO_GPE0_BLK_LEN 0x10 #define ICH9_PMIO_SMI_EN 0x30 #define ICH9_PMIO_SMI_EN_APMC_EN (1 << 5) +#define ICH9_PMIO_SMI_EN_GLB_SMI_EN (1 << 0)
/* FADT ACPI_ENABLE/ACPI_DISABLE */ #define ICH9_APM_ACPI_ENABLE 0x2 diff --git a/src/fw/smm.c b/src/fw/smm.c index 07f9234..ca0e2fc 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -103,6 +103,11 @@ static void piix4_apmc_smm_setup(int isabdf, int i440_bdf) /* enable SMI generation when writing to the APMC register */ pci_config_writel(isabdf, PIIX_DEVACTB, value | PIIX_DEVACTB_APMC_EN);
+ /* enable SMI generation */ + value = inl(PORT_ACPI_PM_BASE + PIIX_PMIO_GLBCTL); + outl(PORT_ACPI_PM_BASE + PIIX_PMIO_GLBCTL, + value | PIIX_PMIO_GLBCTL_SMI_EN); + smm_relocate_and_restore();
/* close the SMM memory window and enable normal SMM */ @@ -123,9 +128,14 @@ void ich9_lpc_apmc_smm_setup(int isabdf, int mch_bdf) smm_save_and_copy();
/* enable SMI generation when writing to the APMC register */ - outl(value | ICH9_PMIO_SMI_EN_APMC_EN, + outl(value | ICH9_PMIO_SMI_EN_APMC_EN | ICH9_PMIO_SMI_EN_GLB_SMI_EN, PORT_ACPI_PM_BASE + ICH9_PMIO_SMI_EN);
+ /* lock SMI generation */ + value = pci_config_readw(isabdf, ICH9_LPC_GEN_PMCON_1); + pci_config_writel(isabdf, ICH9_LPC_GEN_PMCON_1, + value | ICH9_LPC_GEN_PMCON_1_SMI_LOCK); + smm_relocate_and_restore();
/* close the SMM memory window and enable normal SMM */
The next patch will add shared code between the initial handler for SMBASE relocation and the actual SMI handler. Combine the code of the two handlers for simplicity.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/fw/smm.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/src/fw/smm.c b/src/fw/smm.c index ca0e2fc..b0cc6f5 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -17,13 +17,17 @@ #include "util.h" // smm_setup #include "x86.h" // wbinvd
-extern u8 smm_relocation_start, smm_relocation_end; +extern u8 smm_code_start, smm_code_end; + ASM32FLAT( - ".global smm_relocation_start, smm_relocation_end\n" + ".global smm_code_start, smm_code_end\n" " .code16gcc\n" + "smm_code_start:\n" + " mov %cs, %ax\n" + " cmp $0xa000, %ax\n" + " je smm_exit\n"
/* code to relocate SMBASE to 0xa0000 */ - "smm_relocation_start:\n" " movl $" __stringify(BUILD_SMM_INIT_ADDR) " + 0x7efc, %ebx\n" " addr32 movb (%ebx), %al\n" /* revision ID to see if x86_64 or x86 */ " cmpb $0x64, %al\n" @@ -39,16 +43,7 @@ ASM32FLAT( " movb $0x00, %al\n" " movw $" __stringify(PORT_SMI_STATUS) ", %dx\n" " outb %al, %dx\n" - " rsm\n" - "smm_relocation_end:\n" - " .code32\n" - ); - -extern u8 smm_code_start, smm_code_end; -ASM32FLAT( - ".global smm_code_start, smm_code_end\n" - " .code16gcc\n" - "smm_code_start:\n" + "smm_exit:\n" " rsm\n" "smm_code_end:\n" " .code32\n" @@ -60,9 +55,9 @@ smm_save_and_copy(void) /* save original memory content */ memcpy((void *)BUILD_SMM_ADDR, (void *)BUILD_SMM_INIT_ADDR, BUILD_SMM_SIZE);
- /* copy the SMM relocation code */ - memcpy((void *)BUILD_SMM_INIT_ADDR, &smm_relocation_start, - &smm_relocation_end - &smm_relocation_start); + /* copy the SMM code, which will relocate itself on the first execution */ + memcpy((void *)BUILD_SMM_INIT_ADDR, &smm_code_start, + &smm_code_end - &smm_code_start); }
static void
Both the PIIX and ICH9 require two writes to re-enable SMIs, one for all SMIs (EOS) and one specific to port 0xb2 SMIs (APM_STS). Configure the values at setup time, and perform the I/O while in SMM. The variables reside in SMRAM themselves, and are accessed with a CS segment override.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- src/fw/dev-piix.h | 3 +++ src/fw/dev-q35.h | 3 +++ src/fw/smm.c | 26 ++++++++++++++++++++++++++ 3 files changed, 32 insertions(+)
diff --git a/src/fw/dev-piix.h b/src/fw/dev-piix.h index c389f17..5df0a89 100644 --- a/src/fw/dev-piix.h +++ b/src/fw/dev-piix.h @@ -17,7 +17,10 @@ /* ICH9 PM I/O registers */ #define PIIX_GPE0_BLK 0xafe0 #define PIIX_GPE0_BLK_LEN 4 +#define PIIX_PMIO_GLBSTS 0x18 +#define PIIX_PMIO_GLBSTS_APM_STS (1 << 5) #define PIIX_PMIO_GLBCTL 0x28 +#define PIIX_PMIO_GLBCTL_EOS (1 << 16) #define PIIX_PMIO_GLBCTL_SMI_EN 1
/* FADT ACPI_ENABLE/ACPI_DISABLE */ diff --git a/src/fw/dev-q35.h b/src/fw/dev-q35.h index c6f8bd9..3f8b8d6 100644 --- a/src/fw/dev-q35.h +++ b/src/fw/dev-q35.h @@ -40,7 +40,10 @@ #define ICH9_PMIO_GPE0_BLK_LEN 0x10 #define ICH9_PMIO_SMI_EN 0x30 #define ICH9_PMIO_SMI_EN_APMC_EN (1 << 5) +#define ICH9_PMIO_SMI_EN_EOS (1 << 1) #define ICH9_PMIO_SMI_EN_GLB_SMI_EN (1 << 0) +#define ICH9_PMIO_SMI_STS 0x34 +#define ICH9_PMIO_SMI_STS_APM_STS (1 << 5)
/* FADT ACPI_ENABLE/ACPI_DISABLE */ #define ICH9_APM_ACPI_ENABLE 0x2 diff --git a/src/fw/smm.c b/src/fw/smm.c index b0cc6f5..dbb9f82 100644 --- a/src/fw/smm.c +++ b/src/fw/smm.c @@ -19,6 +19,10 @@
extern u8 smm_code_start, smm_code_end;
+extern u16 smm_eos_port, smm_status_port; +extern u32 smm_eos_value, smm_status_value; +#define SMMVAR(x) "%cs:("__stringify(x)" - smm_code_start + 0x8000)" + ASM32FLAT( ".global smm_code_start, smm_code_end\n" " .code16gcc\n" @@ -44,7 +48,19 @@ ASM32FLAT( " movw $" __stringify(PORT_SMI_STATUS) ", %dx\n" " outb %al, %dx\n" "smm_exit:\n" + " movw "SMMVAR(smm_status_port)", %dx\n" + " movl "SMMVAR(smm_status_value)", %eax\n" + " outl %eax, %dx\n" // The SMM status register is write-1-clears + " movw "SMMVAR(smm_eos_port)", %dx\n" + " inl %dx, %eax\n" + " orl "SMMVAR(smm_eos_value)", %eax\n" + " outl %eax, %dx\n" " rsm\n" + ".align 4\n" + "smm_eos_port: .word 0\n" + "smm_status_port: .word 0\n" + "smm_eos_value: .long 0\n" + "smm_status_value: .long 0\n" "smm_code_end:\n" " .code32\n" ); @@ -90,6 +106,11 @@ static void piix4_apmc_smm_setup(int isabdf, int i440_bdf) if (value & PIIX_DEVACTB_APMC_EN) return;
+ smm_eos_port = PORT_ACPI_PM_BASE + PIIX_PMIO_GLBCTL; + smm_eos_value = PIIX_PMIO_GLBCTL_EOS; + smm_status_port = PORT_ACPI_PM_BASE + PIIX_PMIO_GLBSTS; + smm_status_value = PIIX_PMIO_GLBSTS_APM_STS; + /* enable the SMM memory window */ pci_config_writeb(i440_bdf, I440FX_SMRAM, 0x02 | 0x48);
@@ -117,6 +138,11 @@ void ich9_lpc_apmc_smm_setup(int isabdf, int mch_bdf) if (value & ICH9_PMIO_SMI_EN_APMC_EN) return;
+ smm_eos_port = PORT_ACPI_PM_BASE + ICH9_PMIO_SMI_EN; + smm_eos_value = ICH9_PMIO_SMI_EN_EOS; + smm_status_port = PORT_ACPI_PM_BASE + ICH9_PMIO_SMI_STS; + smm_status_value = ICH9_PMIO_SMI_STS_APM_STS; + /* enable the SMM memory window */ pci_config_writeb(mch_bdf, Q35_HOST_BRIDGE_SMRAM, 0x02 | 0x48);
On Thu, May 15, 2014 at 01:22:25PM +0200, Paolo Bonzini wrote:
Hi,
these patches provide a small set of improvements and cleanups for the current SMM code, making it more consistent with the chipset's datasheets. QEMU does not yet implement most of the new registers used here, but it might soon...
Thanks. Looks good to me.
I think this series would be better for the next release though.
Also, have you seen the patch I have to convert the SMM handler to C code? https://github.com/KevinOConnor/seabios/commit/67fd7b76ec5b511589afb909b421c... I think the later parts of your series might be better applied after the handler is in C.
-Kevin
Il 15/05/2014 17:05, Kevin O'Connor ha scritto:
On Thu, May 15, 2014 at 01:22:25PM +0200, Paolo Bonzini wrote:
Hi,
these patches provide a small set of improvements and cleanups for the current SMM code, making it more consistent with the chipset's datasheets. QEMU does not yet implement most of the new registers used here, but it might soon...
Thanks. Looks good to me.
I think this series would be better for the next release though.
Sure.
Also, have you seen the patch I have to convert the SMM handler to C code? https://github.com/KevinOConnor/seabios/commit/67fd7b76ec5b511589afb909b421c... I think the later parts of your series might be better applied after the handler is in C.
Yes, I have seen it.
A simple way to reconcile the C code with the changes I'm making could be to make the relocation code associated to one particular value of APMC.
So the assembly trampoline would be just (untested):
xorl %eax, %eax movl %ax, %cs shl $4, eax data32 ljmp $0, 1f # make flat code segment 1: data32 call smm_handler # near call rsm
and the C handler like this:
smm_handler(void *smbase) { switch (inb(0xb2)) { case 0xAA: if (smbase != (void *)0xA0000L) { ... } outb(PORT_SMI_STATUS, 0); } outl(smm_eos_port, inl(smm_eos_port)|smm_eos_value); outl(smm_status_port, smm_status_value); }
I think this is more easily visible on top of these patches more than the other way round, but I'm biased of course...
Paolo
On Thu, May 15, 2014 at 06:36:08PM +0200, Paolo Bonzini wrote:
A simple way to reconcile the C code with the changes I'm making could be to make the relocation code associated to one particular value of APMC.
It's not hard to pass in the %cs segment to the C code - this works (with some other minor changes):
-// ljmpw $SEG_BIOS, $(entry_smi - BUILD_BIOS_ADDR) -#define SMI_INSN (0xea | ((u64)SEG_BIOS<<24) \ - | (((u32)entry_smi - BUILD_BIOS_ADDR) << 8)) +// movw %cs, %ax; ljmpw $SEG_BIOS, $(entry_smi - BUILD_BIOS_ADDR) +#define SMI_INSN (0xeac88c | ((u64)SEG_BIOS<<40) \ + | ((u64)((u32)entry_smi - BUILD_BIOS_ADDR) << 24))
[...]
smm_handler(void *smbase) { switch (inb(0xb2)) { case 0xAA: if (smbase != (void *)0xA0000L) { ... } outb(PORT_SMI_STATUS, 0); } outl(smm_eos_port, inl(smm_eos_port)|smm_eos_value); outl(smm_status_port, smm_status_value); }
I think this is more easily visible on top of these patches more than the other way round, but I'm biased of course...
I was referring to your patch 5, which adds a bunch of assembler variables - I think it would be clearer to add them in C to start.
-Kevin
Il 15/05/2014 20:57, Kevin O'Connor ha scritto:
On Thu, May 15, 2014 at 06:36:08PM +0200, Paolo Bonzini wrote:
I think this is more easily visible on top of these patches more than the other way round, but I'm biased of course...
I was referring to your patch 5, which adds a bunch of assembler variables - I think it would be clearer to add them in C to start.
Ah, sure. The C conversion patch belongs between patch 4 and patch 5 in this series, roughly.
Paolo
On Fri, May 16, 2014 at 09:29:59AM +0200, Paolo Bonzini wrote:
Il 15/05/2014 20:57, Kevin O'Connor ha scritto:
On Thu, May 15, 2014 at 06:36:08PM +0200, Paolo Bonzini wrote:
I think this is more easily visible on top of these patches more than the other way round, but I'm biased of course...
I was referring to your patch 5, which adds a bunch of assembler variables - I think it would be clearer to add them in C to start.
Ah, sure. The C conversion patch belongs between patch 4 and patch 5 in this series, roughly.
FYI, I rebased your first four patches on to the seabios tip and then applied my "SMM in C" patch on top of that. There were several conflicts during the merge of your patches, so I hope I got them correct. The series is at:
https://github.com/KevinOConnor/seabios/commits/work-smm-in-c-20140528
I didn't attempt to merge your patch 5 (I don't really know what the EOS stuff is).
-Kevin
Il 28/05/2014 18:04, Kevin O'Connor ha scritto:
On Fri, May 16, 2014 at 09:29:59AM +0200, Paolo Bonzini wrote:
Il 15/05/2014 20:57, Kevin O'Connor ha scritto:
On Thu, May 15, 2014 at 06:36:08PM +0200, Paolo Bonzini wrote:
I think this is more easily visible on top of these patches more than the other way round, but I'm biased of course...
I was referring to your patch 5, which adds a bunch of assembler variables - I think it would be clearer to add them in C to start.
Ah, sure. The C conversion patch belongs between patch 4 and patch 5 in this series, roughly.
FYI, I rebased your first four patches on to the seabios tip and then applied my "SMM in C" patch on top of that. There were several conflicts during the merge of your patches, so I hope I got them correct. The series is at:
https://github.com/KevinOConnor/seabios/commits/work-smm-in-c-20140528
Looks great.
Passing CS to handle_smi is not strictly necessary; you could instead just reserve writing some value (e.g. 0x00) to port 0xb2 for relocation. That's really my only comment. I will rewrite patch 5 in C as soon as your series hits master.
Paolo
On Wed, May 28, 2014 at 06:33:05PM +0200, Paolo Bonzini wrote:
Passing CS to handle_smi is not strictly necessary; you could instead just reserve writing some value (e.g. 0x00) to port 0xb2 for relocation. That's really my only comment.
That's the way I originally had it, but tracking if the smbase was already moved was annoying (to handle the obscure case where an OS writes 0x00 to 0xb2).
-Kevin
-
Don Koch -
Gerd Hoffmann -
Kevin O'Connor -
Paolo Bonzini