Change in coreboot[master]: soc/amd/picasso: Add SMMSTORE support

[Marshall Dawson (Code Review)]

Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

soc/amd/picasso: Add SMMSTORE support

Add SMMSTORE support for saving EFI NVRAM variables in
conjuction with Tianocore payload.

Test: none, as this duplicates tested functionality in
amd/stoneyridge.

Signed-off-by: Marshall Dawson 
Change-Id: Id05b33edf949611c3f9eac94e7b63a4266c6c4d0
---
M src/soc/amd/picasso/smihandler.c
1 file changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/38471/1

diff --git a/src/soc/amd/picasso/smihandler.c b/src/soc/amd/picasso/smihandler.c
index 39c2dfd..d987a50 100644
--- a/src/soc/amd/picasso/smihandler.c
+++ b/src/soc/amd/picasso/smihandler.c
@@ -23,6 +23,7 @@
 #include <arch/acpi.h>
 #include <arch/hlt.h>
 #include <device/pci_def.h>
+#include <smmstore.h>
 #include <soc/smi.h>
 #include <soc/southbridge.h>
 #include <amdblocks/acpimmio.h>
@@ -88,6 +89,25 @@
    io_smi->rax = gsmi_exec(sub_command, &reg_ebx);
 }

+static void southbridge_smi_store(void)
+{
+	u8 sub_command;
+	amd64_smm_state_save_area_t *io_smi;
+	u32 reg_ebx;
+
+	io_smi = find_save_state(APM_CNT_SMMSTORE);
+	if (!io_smi)
+		return;
+	/* Command and return value in EAX */
+	sub_command = (io_smi->rax >> 8) & 0xff;
+
+	/* Parameter buffer in EBX */
+	reg_ebx = io_smi->rbx;
+
+	/* drivers/smmstore/smi.c */
+	io_smi->rax = smmstore_exec(sub_command, (void *)reg_ebx);
+}
+
 static void sb_apmc_smi_handler(void)
 {
    const uint8_t cmd = inb(pm_acpi_smi_cmd_port());
@@ -103,6 +123,10 @@
    	if (CONFIG(ELOG_GSMI))
    		southbridge_smi_gsmi();
    	break;
+	case APM_CNT_SMMSTORE:
+		if (CONFIG(SMMSTORE))
+			southbridge_smi_store();
+		break;
    }

mainboard_smi_apmc(cmd);

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38471/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38471/1//COMMIT_MSG@12 
PS1, Line 12: duplicates
Hmmmmm... Maybe stoney should be less selfish, and share some code with picasso?

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38471/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38471/1//COMMIT_MSG@12 
PS1, Line 12: duplicates
> Hmmmmm... […]

I was thinking the same thing, perhaps an soc/amd/common layout to mimic what's done on the Intel side?

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 1:

Of course I'm on onboard with that idea, however I would really like to get picasso functional in the repo first and then de-duplicate.  Then that way we'll have two functioning generations where we can ensure it's done properly.  At the moment, I'm doing work in picasso for S3 and discovering subtle differences from stoneyridge; it seems more logical to discover any others first and de-duplicate once.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Of course I'm on onboard with that idea, however I would really like to get picasso functional in the repo first and then de-duplicate.  Then that way we'll have two functioning generations where we can ensure it's done properly.  At the moment, I'm doing work in picasso for S3 and discovering subtle differences from stoneyridge; it seems more logical to discover any others first and de-duplicate once.

Last time I checked AMD code (fam10 and fam15 stuff), there were many generations of copied files. Personally, I would not duplicate things in the first place. With some restructuring, I'm pretty sure it's easier to not duplicate in the first place. For example, adding SMBus support for picasso could be as simple as selecting SOC_AMD_COMMON_BLOCK_SMBUS.

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > Of course I'm on onboard with that idea, however I would really like to get picasso functional in the repo first and then de-duplicate.  Then that way we'll have two functioning generations where we can ensure it's done properly.  At the moment, I'm doing work in picasso for S3 and discovering subtle differences from stoneyridge; it seems more logical to discover any others first and de-duplicate once.
>
> Last time I checked AMD code (fam10 and fam15 stuff), there were many generations of copied files. Personally, I would not duplicate things in the first place. With some restructuring, I'm pretty sure it's easier to not duplicate in the first place. For example, adding SMBus support for picasso could be as simple as selecting SOC_AMD_COMMON_BLOCK_SMBUS.

I realize we're swerving pretty far off topic now...  I'll consider abandoning this patch simply to end the conversation and get back to productive work.  My goal was solely to keep picasso in sync with stoneyridge until commonizing can be prioritized.  I already require, and have, unrelated local changes to this file, however they won't be affected by a rebase.

Prior to starting picasso, I scrubbed the differences between the onboard FCH for Family 17h and Family 15h.  That was the basis for moving so much so much of stoneyridge's functionality into soc/amd/common.  Some features were obvious moves, some obviously needed to stay, and others were borderline i.e. I really wanted to get a better feel for their behavior on picasso first.  The smi and smbus registers fell into the 3rd category.
> ...AMD code (fam10 and fam15 stuff), there were many generations of copied files.

I completely agree, and I wouldn't try to defend the development practices from years ago.  But common doesn't always mean better; I'll suggest the example where multiple generations relied on a common sb//hudson directory.  Hudson was a codename of a specific family of discrete controller hubs from exactly 10 years ago.  But then every time a different FCH was used, or AMD released a new APU with an internal hub, the implementation relied on the hudson source -- apparently nobody took the time to study the differences, and subsequently there were bugs that went unnoticed for years.

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 1: Code-Review+2

good enough for me :)

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/38471/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38471/1//COMMIT_MSG@12 
PS1, Line 12: duplicates
> I was thinking the same thing, perhaps an soc/amd/common layout to mimic what's done on the Intel si […]

Once there's deduplication going on, sure. This change mostly seems to ensure that the duplication we already have remains synchronized, making the deduplication job easier down the road, no?

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38471/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38471/1//COMMIT_MSG@12 
PS1, Line 12: duplicates
> Once there's deduplication going on, sure. […]

Patrick, yes that's 100% correct.  I'd made a conscious decision balancing keeping copies in sync vs. trying to commonize features where the register sets didn't match.  More specifically, since the SMI block of registers are defined only somewhat similarly, (when the time comes) having two functioning platforms will help me decide what part moves to common and what part must remain in the amd/<apu_name> directory.

[Marshall Dawson (Code Review)]

Marshall Dawson has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

soc/amd/picasso: Add SMMSTORE support

Add SMMSTORE support for saving EFI NVRAM variables in
conjuction with Tianocore payload.

Test: none, as this duplicates tested functionality in
amd/stoneyridge.

Signed-off-by: Marshall Dawson 
Change-Id: Id05b33edf949611c3f9eac94e7b63a4266c6c4d0
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38471
Tested-by: build bot (Jenkins) 
Reviewed-by: Matt DeVillier 
Reviewed-by: Patrick Georgi 
---
M src/soc/amd/picasso/smihandler.c
1 file changed, 24 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Matt DeVillier: Looks good to me, approved

diff --git a/src/soc/amd/picasso/smihandler.c b/src/soc/amd/picasso/smihandler.c
index 39c2dfd..d987a50 100644
--- a/src/soc/amd/picasso/smihandler.c
+++ b/src/soc/amd/picasso/smihandler.c
@@ -23,6 +23,7 @@
 #include <arch/acpi.h>
 #include <arch/hlt.h>
 #include <device/pci_def.h>
+#include <smmstore.h>
 #include <soc/smi.h>
 #include <soc/southbridge.h>
 #include <amdblocks/acpimmio.h>
@@ -88,6 +89,25 @@
    io_smi->rax = gsmi_exec(sub_command, &reg_ebx);
 }

+static void southbridge_smi_store(void)
+{
+	u8 sub_command;
+	amd64_smm_state_save_area_t *io_smi;
+	u32 reg_ebx;
+
+	io_smi = find_save_state(APM_CNT_SMMSTORE);
+	if (!io_smi)
+		return;
+	/* Command and return value in EAX */
+	sub_command = (io_smi->rax >> 8) & 0xff;
+
+	/* Parameter buffer in EBX */
+	reg_ebx = io_smi->rbx;
+
+	/* drivers/smmstore/smi.c */
+	io_smi->rax = smmstore_exec(sub_command, (void *)reg_ebx);
+}
+
 static void sb_apmc_smi_handler(void)
 {
    const uint8_t cmd = inb(pm_acpi_smi_cmd_port());
@@ -103,6 +123,10 @@
    	if (CONFIG(ELOG_GSMI))
    		southbridge_smi_gsmi();
    	break;
+	case APM_CNT_SMMSTORE:
+		if (CONFIG(SMMSTORE))
+			southbridge_smi_store();
+		break;
    }

mainboard_smi_apmc(cmd);

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38471 )

Change subject: soc/amd/picasso: Add SMMSTORE support
......................................................................

Patch Set 2:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/201
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/200
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/199

Please note: This test is under development and might not be accurate at all!