Change in coreboot[master]: intel/stm: Place resource list right below MSEG

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Name of user not set (Code Review)

16 Apr 2020 16 Apr '20

7:06 p.m.

cedarhouse1@comcast.net has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40437 )

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

intel/stm: Place resource list right below MSEG

Suggested by Nico Huber in CL 38765.

This placement makes the address calculation simpler and makes its location indepedent of the number of CPUs.

Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Signed-off-by: Eugene D Myers cedarhouse@comcast.net --- M src/cpu/x86/smm/smm_module_loader.c M src/security/intel/stm/StmPlatformSmm.c 2 files changed, 8 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/40437/1

diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index 7c23ef8..c50e97d 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -308,13 +308,13 @@ /* The SMM module is placed within the provided region in the following * manner: * +-----------------+ <- smram + size - * | stacks | - * +-----------------+ <- smram + size - total_stack_size - * | fxsave area | - * +-----------------+ <- smram + size - total_stack_size - fxsave_size * | BIOS resource | * | list (STM) | - * +-----------------+ <- .. - CONFIG_BIOS_RESOURCE_LIST_SIZE + * +-----------------+ <- smram + size - CONFIG_BIOS_RESOURCE_LIST_SIZE + * | stacks | + * +-----------------+ <- .. - total_stack_size + * | fxsave area | + * +-----------------+ <- .. - total_stack_size - fxsave_size * | ... | * +-----------------+ <- smram + handler_size + SMM_DEFAULT_SIZE * | handler | @@ -355,11 +355,10 @@

/* Stacks start at the top of the region. */ base = smram; + base += size;

if (CONFIG(STM)) - base += size - CONFIG_MSEG_SIZE; // take out the mseg - else - base += size; + base -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE;

params->stack_top = base;

@@ -389,9 +388,6 @@ /* Does the required amount of memory exceed the SMRAM region size? */ total_size = total_stack_size + handler_size; total_size += fxsave_size + SMM_DEFAULT_SIZE; - /* Account for the BIOS resource list */ - if (CONFIG(STM)) - total_size += CONFIG_BIOS_RESOURCE_LIST_SIZE;

if (total_size > size) return -1; diff --git a/src/security/intel/stm/StmPlatformSmm.c b/src/security/intel/stm/StmPlatformSmm.c index 45db0e0..248ccc0 100644 --- a/src/security/intel/stm/StmPlatformSmm.c +++ b/src/security/intel/stm/StmPlatformSmm.c @@ -177,12 +177,7 @@

// need to create the BIOS resource list once // first calculate the location in SMRAM - addr_calc = (mseg - (CONFIG_SMM_MODULE_STACK_SIZE * num_cpus)); - - if (CONFIG(SSE)) - addr_calc -= FXSAVE_SIZE * num_cpus; - - addr_calc -= CONFIG_BIOS_RESOURCE_LIST_SIZE; + addr_calc = mseg - CONFIG_BIOS_RESOURCE_LIST_SIZE; stm_resource_heap = (uint8_t *) addr_calc; printk(BIOS_DEBUG, "STM: stm_resource_heap located at %p\n", stm_resource_heap);

Attachments:

attachment.htm (text/html — 6.0 KB)

Show replies by date

Angel Pons (Code Review)

17 Apr 17 Apr

12:15 a.m.

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

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 1: Code-Review+1

(1 comment)

Has it been tested?

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

https://review.coreboot.org/c/coreboot/+/40437/1//COMMIT_MSG@9 PS1, Line 9: CL 38765 CB:38765

Name of user not set (Code Review)

12:19 a.m.

cedarhouse1@comcast.net has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40437 )

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40437/1//COMMIT_MSG@9 PS1, Line 9: CL 38765

...

CB:38765

Yes - They have been active on my system since that CL

Angel Pons (Code Review)

12:25 a.m.

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

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40437/1//COMMIT_MSG@9 PS1, Line 9: CL 38765

...

Yes - They have been active on my system since that CL

Apologies if I wasn't clear. What I meant is that the commit message should say "CB:38765" instead.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Gerrit-Change-Number: 40437 Gerrit-PatchSet: 1 Gerrit-Owner: cedarhouse1@comcast.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 16 Apr 2020 22:25:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: cedarhouse1@comcast.net Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Name of user not set (Code Review)

12:27 a.m.

cedarhouse1@comcast.net has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40437 )

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 1:

...

Patch Set 1:

(1 comment)

OK. I'll fix that in all three commits along with some additional information.

Paul Menzel (Code Review)

12:20 p.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40437 )

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/40437/1//COMMIT_MSG@9 PS1, Line 9: CL 38765

...

Apologies if I wasn't clear. What I meant is that the commit message should say "CB:38765" instead.

… so Gerrit marks them up as links. CL:… goes to Chromium OS, so CB: is correct here.

https://review.coreboot.org/c/coreboot/+/40437/1/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loader.c:

https://review.coreboot.org/c/coreboot/+/40437/1/src/cpu/x86/smm/smm_module_... PS1, Line 361: base -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE; Without background, the removal of the else branch is not obvious.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Gerrit-Change-Number: 40437 Gerrit-PatchSet: 1 Gerrit-Owner: Eugene Myers cedarhouse1@comcast.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 17 Apr 2020 10:20:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Eugene Myers cedarhouse1@comcast.net Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Eugene Myers (Code Review)

6:53 p.m.

Hello build bot (Jenkins), Angel Pons, HAOUAS Elyes,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40437

to look at the new patch set (#2).

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

intel/stm: Place resource list right below MSEG

Suggested by Nico Huber in CB:38765.

This placement makes the address calculation simpler and makes its location indepedent of the number of CPUs.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/40437/2

Eugene Myers (Code Review)

7:19 p.m.

Hello build bot (Jenkins), Angel Pons, HAOUAS Elyes,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40437

to look at the new patch set (#3).

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

intel/stm: Place resource list right below MSEG

Suggested by Nico Huber in CB:38765.

This placement makes the address calculation simpler and makes its location indepedent of the number of CPUs.

As part of the change in the BIOS resource list address calculation, the `size` variable was factored out of the conditional in line 361, thus eliminating the else.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/40437/3

Eugene Myers (Code Review)

7:20 p.m.

Eugene Myers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40437 )

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40437/1/src/cpu/x86/smm/smm_module_... File src/cpu/x86/smm/smm_module_loader.c:

https://review.coreboot.org/c/coreboot/+/40437/1/src/cpu/x86/smm/smm_module_... PS1, Line 361: base -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE;

...

Without background, the removal of the else branch is not obvious.

The following was added to the commit:

As part of the change in the BIOS resource list address calculation, the `size` variable was factored out of the conditional in line 361, thus eliminating the else.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Gerrit-Change-Number: 40437 Gerrit-PatchSet: 3 Gerrit-Owner: Eugene Myers cedarhouse1@comcast.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 17 Apr 2020 17:20:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Patrick Georgi (Code Review)

20 Apr 20 Apr

8:38 a.m.

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

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40437/1//COMMIT_MSG@9 PS1, Line 9: CL 38765

...

… so Gerrit marks them up as links. CL:… goes to Chromium OS, so CB: is correct here.

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/40437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Gerrit-Change-Number: 40437 Gerrit-PatchSet: 3 Gerrit-Owner: Eugene Myers cedarhouse1@comcast.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 20 Apr 2020 06:38:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Eugene Myers cedarhouse1@comcast.net Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Patrick Georgi (Code Review)

8:39 a.m.

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

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 3:

Since this is referring to Nico, adding him to the reviewers.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Gerrit-Change-Number: 40437 Gerrit-PatchSet: 3 Gerrit-Owner: Eugene Myers cedarhouse1@comcast.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 20 Apr 2020 06:39:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nico Huber (Code Review)

1 May 1 May

12:43 p.m.

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40437 )

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

Patch Set 3: Code-Review+2

Thanks for testing my idea. And sorry for the huge delay. I was waiting for feedback from more SMM experienced folks on my patch series.

-- To view, visit https://review.coreboot.org/c/coreboot/+/40437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Gerrit-Change-Number: 40437 Gerrit-PatchSet: 3 Gerrit-Owner: Eugene Myers cedarhouse1@comcast.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Patrick Georgi pgeorgi@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 01 May 2020 10:43:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

2 May 2 May

7:03 p.m.

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40437 )

Change subject: intel/stm: Place resource list right below MSEG ......................................................................

intel/stm: Place resource list right below MSEG

Suggested by Nico Huber in CB:38765.

This placement makes the address calculation simpler and makes its location indepedent of the number of CPUs.

As part of the change in the BIOS resource list address calculation, the `size` variable was factored out of the conditional in line 361, thus eliminating the else.

Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Signed-off-by: Eugene D Myers cedarhouse@comcast.net Reviewed-on: https://review.coreboot.org/c/coreboot/+/40437 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/cpu/x86/smm/smm_module_loader.c M src/security/intel/stm/StmPlatformSmm.c 2 files changed, 8 insertions(+), 17 deletions(-)

Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve

diff --git a/src/cpu/x86/smm/smm_module_loader.c b/src/cpu/x86/smm/smm_module_loader.c index bdcf283..c08e833 100644 --- a/src/cpu/x86/smm/smm_module_loader.c +++ b/src/cpu/x86/smm/smm_module_loader.c @@ -307,13 +307,13 @@ /* The SMM module is placed within the provided region in the following * manner: * +-----------------+ <- smram + size - * | stacks | - * +-----------------+ <- smram + size - total_stack_size - * | fxsave area | - * +-----------------+ <- smram + size - total_stack_size - fxsave_size * | BIOS resource | * | list (STM) | - * +-----------------+ <- .. - CONFIG_BIOS_RESOURCE_LIST_SIZE + * +-----------------+ <- smram + size - CONFIG_BIOS_RESOURCE_LIST_SIZE + * | stacks | + * +-----------------+ <- .. - total_stack_size + * | fxsave area | + * +-----------------+ <- .. - total_stack_size - fxsave_size * | ... | * +-----------------+ <- smram + handler_size + SMM_DEFAULT_SIZE * | handler | @@ -354,11 +354,10 @@

/* Stacks start at the top of the region. */ base = smram; + base += size;

if (CONFIG(STM)) - base += size - CONFIG_MSEG_SIZE; // take out the mseg - else - base += size; + base -= CONFIG_MSEG_SIZE + CONFIG_BIOS_RESOURCE_LIST_SIZE;

params->stack_top = base;

@@ -388,9 +387,6 @@ /* Does the required amount of memory exceed the SMRAM region size? */ total_size = total_stack_size + handler_size; total_size += fxsave_size + SMM_DEFAULT_SIZE; - /* Account for the BIOS resource list */ - if (CONFIG(STM)) - total_size += CONFIG_BIOS_RESOURCE_LIST_SIZE;

-- To view, visit https://review.coreboot.org/c/coreboot/+/40437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ee2747474df02b0306530048bdec75e95413b5d Gerrit-Change-Number: 40437 Gerrit-PatchSet: 4 Gerrit-Owner: Eugene Myers cedarhouse1@comcast.net Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1666

days inactive

1682

days old

coreboot-gerrit@coreboot.org

12 comments

6 participants

tags (0)

participants (6)

Angel Pons (Code Review)
Eugene Myers (Code Review)
Name of user not set (Code Review)
Nico Huber (Code Review)
Patrick Georgi (Code Review)
Paul Menzel (Code Review)