Change in coreboot[master]: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Nico Huber (Code Review)

27 May 2020 27 May '20

6:15 p.m.

Hello Kyösti Mälkki,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency

We want to use the CACHE_ROM_* macros in linker scripts. Avoid `commonlib/helpers.h` as it contains an incompatible ALIGN() definition.

Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/include/cpu/x86/mtrr.h 1 file changed, 2 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/41785/1

diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h index 50148ff..301ea30 100644 --- a/src/include/cpu/x86/mtrr.h +++ b/src/include/cpu/x86/mtrr.h @@ -1,7 +1,6 @@ #ifndef CPU_X86_MTRR_H #define CPU_X86_MTRR_H

-#include <commonlib/helpers.h> #ifndef __ASSEMBLER__ #include <cpu/x86/msr.h> #include <arch/cpu.h> @@ -187,8 +186,8 @@

/* Last but not least, most (if not all) chipsets have MMIO between 0xfe000000 and 0xff000000, so limit to 16MiB. */ -#if CAR_CACHE_ROM_SIZE >= 16 * MiB -# define CACHE_ROM_SIZE (16 * MiB) +#if CAR_CACHE_ROM_SIZE >= 16 << 20 +# define CACHE_ROM_SIZE (16 << 20) #else # define CACHE_ROM_SIZE CAR_CACHE_ROM_SIZE #endif

Attachments:

attachment.htm (text/html — 3.1 KB)

Show replies by date

Aaron Durbin (Code Review)

27 May 27 May

6:18 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41785 )

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/41785/1//COMMIT_MSG@11 PS1, Line 11: definition. Where is the ALIGN() macro being used in the CACHE_ROM_* macros? Sorry, I poked around and couldn't see it.

HAOUAS Elyes (Code Review)

6:21 p.m.

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41785 )

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 1: Code-Review+2

Aaron Durbin (Code Review)

6:21 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41785 )

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/41785/1//COMMIT_MSG@11 PS1, Line 11: definition.

...

Where is the ALIGN() macro being used in the CACHE_ROM_* macros? Sorry, I poked around and couldn't […]

Could we be more explicit that this is conflicting with ld ALIGN keyword? I was confused where the conflict was coming from, but saw comments in CB:30872 that explain it more.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Wed, 27 May 2020 16:21:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

HAOUAS Elyes (Code Review)

6:23 p.m.

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41785 )

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/41785/1//COMMIT_MSG@11 PS1, Line 11: definition.

...

Could we be more explicit that this is conflicting with ld ALIGN keyword? I was confused where the c […]

src/commonlib/bsd/include/commonlib/bsd/helpers.h

indirect included

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Wed, 27 May 2020 16:23:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Aaron Durbin (Code Review)

6:25 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41785 )

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/41785/1//COMMIT_MSG@11 PS1, Line 11: definition.

...

src/commonlib/bsd/include/commonlib/bsd/helpers.h […]

I understand where its coming from. I'm talking about it being the ld program's grammar that is conflicting -- not any of the code in coreboot.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Wed, 27 May 2020 16:25:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: HAOUAS Elyes ehaouas@noos.fr Gerrit-MessageType: comment

Nico Huber (Code Review)

6:28 p.m.

Hello Kyösti Mälkki, HAOUAS Elyes,

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

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

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

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency

We want to use the CACHE_ROM_* macros in linker scripts. Avoid `commonlib/helpers.h` as it contains an ALIGN() macro definition that conflicts with the ALIGN keyword in linker scripts.

Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/include/cpu/x86/mtrr.h 1 file changed, 2 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/41785/2

Nico Huber (Code Review)

6:29 p.m.

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

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/41785/1//COMMIT_MSG@11 PS1, Line 11: definition.

...

I understand where its coming from. […]

Reworded, better?

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Wed, 27 May 2020 16:29:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: HAOUAS Elyes ehaouas@noos.fr Gerrit-MessageType: comment

Aaron Durbin (Code Review)

6:35 p.m.

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41785 )

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/41785/1//COMMIT_MSG@11 PS1, Line 11: definition.

...

Reworded, better?

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Wed, 27 May 2020 16:35:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Comment-In-Reply-To: HAOUAS Elyes ehaouas@noos.fr Gerrit-MessageType: comment

Julius Werner (Code Review)

10:03 p.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41785 )

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 2:

How about just getting rid of ALIGN() from commonlib/helpers.h? I think people shouldn't use that anyway, they should use ALIGN_UP() for clarity. That way we aren't just kicking this can down the road until the next header runs into this problem.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-Comment-Date: Wed, 27 May 2020 20:03:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

29 May 29 May

12:25 a.m.

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

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 2: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 28 May 2020 22:25:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

2 Jun 2 Jun

9:17 a.m.

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

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency

We want to use the CACHE_ROM_* macros in linker scripts. Avoid `commonlib/helpers.h` as it contains an ALIGN() macro definition that conflicts with the ALIGN keyword in linker scripts.

Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/41785 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: HAOUAS Elyes ehaouas@noos.fr Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/cpu/x86/mtrr.h 1 file changed, 2 insertions(+), 3 deletions(-)

Approvals: build bot (Jenkins): Verified HAOUAS Elyes: Looks good to me, approved Angel Pons: Looks good to me, approved

-#include <commonlib/helpers.h> #ifndef __ASSEMBLER__ #include <cpu/x86/msr.h> #include <arch/cpu.h> @@ -187,8 +186,8 @@

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

9elements QA (Code Review)

9:55 a.m.

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

Change subject: cpu/x86/mtrr.h: Get rid of commonlib/helpers.h dependency ......................................................................

Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4520 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4519 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4518 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4517

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/41785 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3bf20733418ca4135f364a3f6489e74d45e4f466 Gerrit-Change-Number: 41785 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Julius Werner jwerner@chromium.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 02 Jun 2020 07:55:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1635

days inactive

1641

days old

coreboot-gerrit@coreboot.org

12 comments

7 participants

tags (0)

participants (7)

9elements QA (Code Review)
Aaron Durbin (Code Review)
Angel Pons (Code Review)
HAOUAS Elyes (Code Review)
Julius Werner (Code Review)
Nico Huber (Code Review)
Patrick Georgi (Code Review)