Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30421
Change subject: Makefile.inc: Create a default SMMSTORE region ......................................................................
Makefile.inc: Create a default SMMSTORE region
Change-Id: I7b7b75050e0139ea9a0a4f2ad3c0d69a482fb38b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc M src/drivers/smmstore/Kconfig M util/cbfstool/default-x86.fmd 3 files changed, 26 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/30421/1
diff --git a/Makefile.inc b/Makefile.inc index 8af291f..7dee1d2 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -869,11 +869,27 @@ endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y)
# +# X86 SMMSTORE FMAP region +# +# position, size and entry line of SMMSTORE relative to BIOS_BASE, if enabled +ifeq ($(CONFIG_SMMSTORE),y) +FMAP_SMMSTORE_BASE := $(call int-align, $(call int-add, $(FMAP_CONSOLE_BASE) \ + $(FMAP_CONSOLE_SIZE) $(FMAP_MRC_CACHE_SIZE)), 0x10000) +FMAP_SMMSTORE_SIZE := $(CONFIG_SMMSTORE_SIZE) +FMAP_SMMSTORE_ENTRY := SMMSTORE@$(FMAP_SMMSTORE_BASE) $(FMAP_SMMSTORE_SIZE) +else # ifeq ($(CONFIG_SMMSTORE),y) +FMAP_SMMSTORE_BASE := 0 +FMAP_SMMSTORE_SIZE := 0 +FMAP_SMMSTORE_ENTRY := +endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) + +# # X86 FMAP region # # # position, size -FMAP_FMAP_BASE := $(call int-add, $(FMAP_CONSOLE_SIZE) $(FMAP_MRC_CACHE_SIZE)) +FMAP_FMAP_BASE := $(call int-add, $(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE) \ + $(FMAP_MRC_CACHE_SIZE) $(FMAP_SMMSTORE_SIZE)) FMAP_FMAP_SIZE := 0x200
# @@ -947,6 +963,7 @@ -e "s,##FMAP_SIZE##,$(FMAP_FMAP_SIZE)," \ -e "s,##CONSOLE_ENTRY##,$(FMAP_CONSOLE_ENTRY)," \ -e "s,##MRC_CACHE_ENTRY##,$(FMAP_MRC_CACHE_ENTRY)," \ + -e "s,##SMMSTORE_ENTRY##,$(FMAP_SMMSTORE_ENTRY)," \ -e "s,##CBFS_BASE##,$(FMAP_CBFS_BASE)," \ -e "s,##CBFS_SIZE##,$(FMAP_CBFS_SIZE)," \ $(DEFAULT_FLASHMAP) > $@.tmp diff --git a/src/drivers/smmstore/Kconfig b/src/drivers/smmstore/Kconfig index 1ab3a5e..f2c97ab 100644 --- a/src/drivers/smmstore/Kconfig +++ b/src/drivers/smmstore/Kconfig @@ -37,4 +37,11 @@ string "SMM store file name" if SMMSTORE_IN_CBFS default "smm_store"
+config SMMSTORE_SIZE + hex "size of the SMMSTORE FMAP region" + depends on !SMMSTORE_IN_CBFS + default 0x40000 + help + Set the size of the default SMMSTORE FMAP region. + endif diff --git a/util/cbfstool/default-x86.fmd b/util/cbfstool/default-x86.fmd index 174db1f..f0143e9 100644 --- a/util/cbfstool/default-x86.fmd +++ b/util/cbfstool/default-x86.fmd @@ -11,6 +11,7 @@ BIOS@##BIOS_BASE## ##BIOS_SIZE## { ##CONSOLE_ENTRY## ##MRC_CACHE_ENTRY## + ##SMMSTORE_ENTRY## FMAP@##FMAP_BASE## ##FMAP_SIZE## COREBOOT(CBFS)@##CBFS_BASE## ##CBFS_SIZE## }
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/30421
to look at the new patch set (#5).
Change subject: Makefile.inc: Create a default SMMSTORE region ......................................................................
Makefile.inc: Create a default SMMSTORE region
Change-Id: I7b7b75050e0139ea9a0a4f2ad3c0d69a482fb38b Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc M src/drivers/smmstore/Kconfig M util/cbfstool/default-x86.fmd 3 files changed, 30 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/21/30421/5
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30421 )
Change subject: Makefile.inc: Create a default SMMSTORE region ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/30421/5/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/30421/5/Makefile.inc@893 PS5, Line 893: FMAP_SMMSTORE_SIZE := 0 this means that we create a 0-sized fmap region in this case, right?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30421 )
Change subject: Makefile.inc: Create a default SMMSTORE region ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/30421/5/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/30421/5/Makefile.inc@893 PS5, Line 893: FMAP_SMMSTORE_SIZE := 0
this means that we create a 0-sized fmap region in this case, right?
No. There won't be an entry since FMAP_SMMSTORE_ENTRY is empty and ##SMMSTORE_ENTRY## gets replaced by this empty line after which there are no BASE/SIZE values to be replaced in the default fmap.
Btw I think this does not scale well at all. We now have over 150 lines of Makefile code to generate a very basic fmap, that is very error prone. This won't scale if we want more advanced FMAP layouts to be generated by default. Time to have a separate tool to generate useful fmap layouts?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30421 )
Change subject: Makefile.inc: Create a default SMMSTORE region ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/30421/5/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/30421/5/Makefile.inc@893 PS5, Line 893: FMAP_SMMSTORE_SIZE := 0
No. […]
I was advocating for an image assembly tool doing this and more before I wrote this script, but back then that was the only thing I could get through the design phase.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30421 )
Change subject: Makefile.inc: Create a default SMMSTORE region ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30421 )
Change subject: Makefile.inc: Create a default SMMSTORE region ......................................................................
Makefile.inc: Create a default SMMSTORE region
Change-Id: I7b7b75050e0139ea9a0a4f2ad3c0d69a482fb38b Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/30421 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M Makefile.inc M src/drivers/smmstore/Kconfig M util/cbfstool/default-x86.fmd 3 files changed, 30 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/Makefile.inc b/Makefile.inc index dfdd8f6..469ad7a 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -883,11 +883,27 @@ endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y)
# +# X86 SMMSTORE FMAP region +# +# position, size and entry line of SMMSTORE relative to BIOS_BASE, if enabled +ifeq ($(CONFIG_SMMSTORE),y) +FMAP_SMMSTORE_BASE := $(call int-align, $(call int-add, $(FMAP_CONSOLE_BASE) \ + $(FMAP_CONSOLE_SIZE) $(FMAP_MRC_CACHE_SIZE)), 0x10000) +FMAP_SMMSTORE_SIZE := $(CONFIG_SMMSTORE_SIZE) +FMAP_SMMSTORE_ENTRY := SMMSTORE@$(FMAP_SMMSTORE_BASE) $(FMAP_SMMSTORE_SIZE) +else # ifeq ($(CONFIG_SMMSTORE),y) +FMAP_SMMSTORE_BASE := 0 +FMAP_SMMSTORE_SIZE := 0 +FMAP_SMMSTORE_ENTRY := +endif # ifeq ($(CONFIG_CACHE_MRC_SETTINGS),y) + +# # X86 FMAP region # # # position, size -FMAP_FMAP_BASE := $(call int-add, $(FMAP_CONSOLE_SIZE) $(FMAP_MRC_CACHE_SIZE)) +FMAP_FMAP_BASE := $(call int-add, $(FMAP_CONSOLE_BASE) $(FMAP_CONSOLE_SIZE) \ + $(FMAP_MRC_CACHE_SIZE) $(FMAP_SMMSTORE_SIZE)) FMAP_FMAP_SIZE := 0x200
# @@ -961,6 +977,7 @@ -e "s,##FMAP_SIZE##,$(FMAP_FMAP_SIZE)," \ -e "s,##CONSOLE_ENTRY##,$(FMAP_CONSOLE_ENTRY)," \ -e "s,##MRC_CACHE_ENTRY##,$(FMAP_MRC_CACHE_ENTRY)," \ + -e "s,##SMMSTORE_ENTRY##,$(FMAP_SMMSTORE_ENTRY)," \ -e "s,##CBFS_BASE##,$(FMAP_CBFS_BASE)," \ -e "s,##CBFS_SIZE##,$(FMAP_CBFS_SIZE)," \ $(DEFAULT_FLASHMAP) > $@.tmp diff --git a/src/drivers/smmstore/Kconfig b/src/drivers/smmstore/Kconfig index 1ab3a5e..e59e78d 100644 --- a/src/drivers/smmstore/Kconfig +++ b/src/drivers/smmstore/Kconfig @@ -37,4 +37,15 @@ string "SMM store file name" if SMMSTORE_IN_CBFS default "smm_store"
+config SMMSTORE_SIZE + hex "size of the SMMSTORE FMAP region" + depends on !SMMSTORE_IN_CBFS + default 0x40000 + help + Sets the size of the default SMMSTORE FMAP region. + If using an UEFI payload, note that UEFI specifies at least 64K. + The current implementation of SMMSTORE is append only, so until + garbage collection is implemented it is better to set this to + a rather large value. + endif diff --git a/util/cbfstool/default-x86.fmd b/util/cbfstool/default-x86.fmd index 174db1f..f0143e9 100644 --- a/util/cbfstool/default-x86.fmd +++ b/util/cbfstool/default-x86.fmd @@ -11,6 +11,7 @@ BIOS@##BIOS_BASE## ##BIOS_SIZE## { ##CONSOLE_ENTRY## ##MRC_CACHE_ENTRY## + ##SMMSTORE_ENTRY## FMAP@##FMAP_BASE## ##FMAP_SIZE## COREBOOT(CBFS)@##CBFS_BASE## ##CBFS_SIZE## }
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30421 )
Change subject: Makefile.inc: Create a default SMMSTORE region ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/30421/5/Makefile.inc File Makefile.inc:
https://review.coreboot.org/#/c/30421/5/Makefile.inc@893 PS5, Line 893: FMAP_SMMSTORE_SIZE := 0
I was advocating for an image assembly tool doing this and more before I wrote this script, but back […]
Any documents/communications left on this? Handling what and what not to add to fmap regions seems to be a similar problem indeed.
-
Arthur Heymans (Code Review) -
Patrick Georgi (Code Review)