Attention is currently required from: Benjamin Doron, Martin L Roth, Michał Żygowski, Patrick Rudolph, Sean Rhodes.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/70378?usp=email )
Change subject: drivers/smm_payload_interface: Add initial support for SMM payload ......................................................................
Patch Set 18:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/70378/comment/c1ecba72_b2b4d03a : PS18, Line 37: - coreboot provides register definitions in CBMEM to avoid : silicon-dependence in payload. I don't think coreboot informing the payload should be done in this case. The payload can figure out just fine how to do all those things based on runtime detection of SOC. Having code directly in the payload instead of some ACPI like instructions on how to do things is much less error prone. After all there aren't that many variations to support: AMD (same since AMD k8 in 2003), Intel starting with skylake, older Intel with PCH, even older Intel with special SMMR.
I don't think it's desirable to inform the payload like this. There is no usecase where you want to couple a new coreboot with and payload. You pretty much always develop those things in parallel.
Patchset:
PS18: Overall quite reasonable to me except the part about informing CPU/SOC specific details to the payload. I think the payload should be able to figure that out on its own, be it with compilation option or runtime detection (which should be possible). After all you also don't pass on details on how the SPI controller works in the payload too, why would SMM related registers be any different?
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/70378/comment/e9cafdbd_99755ebe : PS18, Line 620: lb_pld_smram_descriptor_block what is this? Are there systems that can have multiple SMRAM regions?
File src/drivers/smm_payload_interface/cbtable.c:
https://review.coreboot.org/c/coreboot/+/70378/comment/049a2b4a_bfb95503 : PS18, Line 14: lb_save_restore can a better name be used? lb_payload_smm_info ?
https://review.coreboot.org/c/coreboot/+/70378/comment/099c8320_118a6305 : PS18, Line 35: ACPI_BASE_ADDRESS + SMI_EN; Not safe. ACPI_BASE_ADDRESS can be reconfigured. The SMI handler needs to fetch that at runtime from the LPC device on Intel.
https://review.coreboot.org/c/coreboot/+/70378/comment/ad822861_36fe012d : PS18, Line 30: smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_GBL_EN; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_IO; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = log2(GBL_SMI_EN); : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = ACPI_BASE_ADDRESS + SMI_EN; : smm_reg->count++; : : smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_GBL_EN_LOCK; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_MEMORY; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = log2(SMI_LOCK); : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = PCH_PWRM_BASE_ADDRESS + GEN_PMCON_B; : smm_reg->count++; : : smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_EOS; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_IO; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = log2(EOS); : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = ACPI_BASE_ADDRESS + SMI_EN; : smm_reg->count++; : : smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_APM_EN; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_IO; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = log2(APMC_EN); : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = ACPI_BASE_ADDRESS + SMI_EN; : smm_reg->count++; : : smm_reg->registers[smm_reg->count].register_id = REGISTER_ID_SMI_APM_STS; : smm_reg->registers[smm_reg->count].address_space_id = PLD_EFI_ACPI_3_0_SYSTEM_IO; : smm_reg->registers[smm_reg->count].register_bit_width = 1; : smm_reg->registers[smm_reg->count].register_bit_offset = APM_STS_BIT; : smm_reg->registers[smm_reg->count].value = 1; : smm_reg->registers[smm_reg->count].address = ACPI_BASE_ADDRESS + SMI_STS; : smm_reg->count++; All very specific to Intel.
File src/drivers/smm_payload_interface/smm_core_support.c:
https://review.coreboot.org/c/coreboot/+/70378/comment/23ed944d_6afb576b : PS18, Line 119: d perform_save(void *unused) : { : uint32_t reg32; : : // Payload does not enable sleep SMI : reg32 = inl(ACPI_BASE_ADDRESS + SMI_EN); : reg32 &= ~SLP_SMI_EN; : outl(reg32, ACPI_BASE_ADDRESS + SMI_EN); : : // Enable SCI mode when there's no 'ACPI enable' SMI handler : reg32 = inl(ACPI_BASE_ADDRESS + PM1_CNT); : reg32 |= SCI_EN; : outl(reg32, ACPI_BASE_ADDRESS + PM1_CNT); : : clear_s3_save_region(); : } : : static void perform_s3_restore(void *unused) : { : uint32_t reg32; : : // Payload does not enable sleep SMI : reg32 = inl(ACPI_BASE_ADDRESS + SMI_EN); : reg32 &= ~SLP_SMI_EN; : outl(reg32, ACPI_BASE_ADDRESS + SMI_EN); : : // Enable SCI mode when there's no 'ACPI enable' SMI handler : reg32 = inl(ACPI_BASE_ADDRESS + PM1_CNT); : reg32 |= SCI_EN; : outl(reg32, ACPI_BASE_ADDRESS + PM1_CNT); : intel specific.
File src/include/smm_payload_interface.h:
https://review.coreboot.org/c/coreboot/+/70378/comment/529694e4_c3afb0ff : PS18, Line 44: ApicId Call it lapicid or initial_lapic to make it clear. With apicid this could be the ID the apic (IOAPIC or LAPIC) has which is typically changeable at runtime. The initial_lapic from cpuid cannot be changed.
https://review.coreboot.org/c/coreboot/+/70378/comment/97860077_bd784152 : PS18, Line 43: truct CPU_SMMBASE { : uint32_t ApicId; : uint32_t SmmBase; : }; : : struct SMM_S3_INFO { : uint8_t SwSmiData; : uint8_t SwSmiTriggerValue; : uint16_t Reserved; : uint32_t CpuCount; : struct CPU_SMMBASE SmmBase[0]; Any reason to deviate from coreboot style?