Attention is currently required from: Arthur Heymans, Robert Zieba, Raul Rangel, Nico Huber, Angel Pons, Karthik Ramasubramanian, Felix Held.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67931 )
Change subject: cpu/x86/smm: Add PCI BAR store functionality ......................................................................
Patch Set 5:
(4 comments)
File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/67931/comment/c27c5d1a_9b0ad586 PS5, Line 195: : config SMM_PCI_BAR_STORE : bool : depends on SMM_CHIPSET_STORE : help : This option enables support for storing PCI BARs in SMRAM so they : can't be tampered with. : : config SMM_PCI_BAR_STORE_NUM_SLOTS : int : default 8 : help : Number of slots available to store PCI BARs in SMRAM
I think the BAR store part of this is generic enough to leave in. […]
Robert, that makes sense to me. I think that this is a useful feature for x86 in general just to verify that any BAR based address we're using hasn't changed.
File src/cpu/x86/smm/pci_bar_store.c:
https://review.coreboot.org/c/coreboot/+/67931/comment/6e28bd21_dc863c32 PS5, Line 7: smm_pci_query_stored_bar
I don't think we need anything this complicated. […]
Raul, you're right that we don't in this case, but is there an argument against making it generic?
https://review.coreboot.org/c/coreboot/+/67931/comment/df70ac61_67415c57 PS5, Line 30: pci_devfn_t pci_addr,
We are in ramstage and resource allocation has already been done. […]
Definitely seems reasonable.
https://review.coreboot.org/c/coreboot/+/67931/comment/3b0837f7_09aa33dd PS5, Line 58: smm_mainboard_pci_bar_store_init(&smm_runtime->chipset_store.pci_bar_store[0], : CONFIG_SMM_PCI_BAR_STORE_NUM_SLOTS);
Would the API make more sense the other way around. […]
Also a good suggestion.