[L] Change in coreboot[master]: drivers/smm_payload_interface: Add initial support for SMM payload

[Benjamin Doron (Code Review)]

Attention is currently required from: Arthur Heymans, Martin L Roth, Michał Żygowski, Patrick Rudolph, Paul Menzel.

Benjamin Doron 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 12:

(11 comments)

Patchset:

PS11:
> So much of SMM setup is SoC specific and this code is very EDK2 specific that you might even want to […]

I've put some work into making the coreboot code a little more hardware-agnostic - at least ramstage.c for now.

I'm experimenting with standalone MM, which we may be able to make do what we want, but I don't think it'll be simpler. It will be less SoC-specific, though lockdown must be performed: maybe the "FINALIZE" SMI works here. One issue is that they have different designs: coreboot reinstalls SMM on S3 resume, EDK2 preserves SMRAM and just relocates CPU SMBASE again.

File Documentation/drivers/s3_save_restore.md:

https://review.coreboot.org/c/coreboot/+/70378/comment/b0957e5c_31675ee5 :
PS8, Line 10:  Payload does not know SPI layout
            :
> Is this needed? coreboot tables already expose FMAP so it's just a matter of parsing that for the co […]

Right, thanks. Done, though new payload updates not pushed yet, and I may need to update the documentation again...

File src/drivers/smm_payload_interface/cbtable.c:

https://review.coreboot.org/c/coreboot/+/70378/comment/a8e3681d_8a38d03c :
PS11, Line 27: static int lookup_store(struct region_device *rstore)
             : {
             : 	struct region_device read_rdev, write_rdev;
             : 	struct incoherent_rdev store_irdev;
             : 	struct region fmap_smmstore;
             : 	const struct region_device *rdev;
             : 
             : 	/* TODO: Don't `struct region` from FMAP's RW `struct region_device`? */
             : 	if (fmap_locate_area("SMMSTORE", &fmap_smmstore) != 0)
             : 		return -1;
             : 
             : 	if (boot_device_ro_subregion(&fmap_smmstore, &read_rdev) < 0)
             : 		return -1;
             : 
             : 	if (boot_device_rw_subregion(&fmap_smmstore, &write_rdev) < 0)
             : 		return -1;
             : 
             : 	rdev = incoherent_rdev_init(&store_irdev, &fmap_smmstore, &read_rdev, &write_rdev);
             : 
             : 	if (rdev == NULL)
             : 		return -1;
             : 
             : 	return rdev_chain(rstore, rdev, 0, region_device_sz(rdev));
             : }
> This already exists somewhere else? Can you reuse it?

Gone, payload parses FMAP (not pushed yet)

https://review.coreboot.org/c/coreboot/+/70378/comment/96606b57_f9f52d40 :
PS11, Line 114: // FIXME: Consider individual open/close/lock fields
              : 	smm_reg->registers[5].register_id = REGISTER_ID_SMRAMC;
              : 	smm_reg->registers[5].address_space_id = PLD_EFI_ACPI_3_0_PCI_CONFIGURATION_SPACE;
              : 	smm_reg->registers[5].register_bit_width = 8;
              : 	smm_reg->registers[5].register_bit_offset = 0;
              : 	smm_reg->registers[5].address = PCI_BDF(SA_DEV_ROOT) + SMRAM;
> SoC specific. On xeon_sp there is no such thing, let alone on AMD.

I do use `#ifdef` for this reason. Though AMD is a bigger question

https://review.coreboot.org/c/coreboot/+/70378/comment/455ed6c8_5d293904 :
PS11, Line 164: spi_info->spi_address.address_space_id = PLD_EFI_ACPI_3_0_PCI_CONFIGURATION_SPACE;
              : 	spi_info->spi_address.register_bit_width = 32;
              : 	spi_info->spi_address.register_bit_offset = 0;
              : 	spi_info->spi_address.address = CONFIG_ECAM_MMCONF_BASE_ADDRESS | PCI_BDF(PCH_DEV_SPI);
> Do you need this? I'd expect the payload to have PCI drivers based VID/DID. […]

It does not, it has one set of libraries that consume this data (SPI). Seems that Intel thinks of all this as different IPs, with known register definition differences.

File src/drivers/smm_payload_interface/util_common.c:

https://review.coreboot.org/c/coreboot/+/70378/comment/0240d896_843a64e9 :
PS11, Line 30: smrr_base
> smrr is Intel specific.

Gone, using a `platform_get_smm_info()` call-out rather than overriding the function

File src/include/smm_payload_interface.h:

https://review.coreboot.org/c/coreboot/+/70378/comment/27b0699d_0fdd4487 :
PS11, Line 14: __packed
> Why?

Copied from FSP driver code. Seems naturally aligned, will confirm

https://review.coreboot.org/c/coreboot/+/70378/comment/9afd096c_e01d54e1 :
PS11, Line 27: #pragma
> why?

Copied from EDK2 header, they usually packs structs. Seems naturally aligned, will confirm

https://review.coreboot.org/c/coreboot/+/70378/comment/c2b5989e_11e43ec1 :
PS11, Line 55: static __always_inline void write_cr2(CRx_TYPE data)
             : {
             : 	__asm__ __volatile__ (
             : 		"mov %0, %%cr2"
             : 		:
             : 		: CRx_IN(data)
             : 		: COMPILER_BARRIER
             : 	);
             : }
> move to cr. […]

Gone

https://review.coreboot.org/c/coreboot/+/70378/comment/d8bcb8bf_9a319bcf :
PS11, Line 69: bool pld_guid_compare
> guid_compare already exists

That's in commonlib/fsp_relocate.c, and it's static. Or fsp_guid_compare, in the FSP driver.

File src/security/lockdown/Kconfig:

https://review.coreboot.org/c/coreboot/+/70378/comment/7ce5c6f0_95bb4328 :
PS11, Line 87: BOOTMEDIA_SMM_BWP
> How can the payload install something in SMM when it's readonly? […]

I'm not sure I understand. Do you mean "install something in SPI when SPI's readonly?"

In the standard case, this Kconfig determines performing lockdown (SPI not write-enabled, writes to SPI bios control register trigger SMI), and relocking (write-protect SPI again) inside the SMI handler.

In this case, we should still perform lockdown, and make the payload handle relocking SPI. Although: currently, UefiPayload only uses its flag to unlock/lock SPI when it wants to use SPI. It **must. actually. install an SMI handler** to perform relocking on other attempts.