Attention is currently required from: Anil Kumar K, Bora Guvendik, Jamie Ryu, Jeremy Compostella, Subrata Banik, Wonkyu Kim.
Pratikkumar V Prajapati has posted comments on this change by Wonkyu Kim. ( https://review.coreboot.org/c/coreboot/+/84228?usp=email )
Change subject: src/device: Add more condition to check valid PCI device id
......................................................................
Patch Set 2:
(1 comment)
File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/84228/comment/f6a5406f_1faf5ecb?us… :
PS2, Line 1225: dummy.vendor = id & 0xffff;
: dummy.device = (id >> 16) & 0xffff;
: if ((dummy.vendor == 0x0000) || (dummy.vendor == 0xffff) ||
: (dummy.device == 0x0000) || (dummy.device == 0xffff)) {
: printk(BIOS_SPEW, "device %s [0x%04x/0x%04x] is not found.\n",
: dev_path(&dummy), dummy.vendor, dummy.device);
:
why don't u make an inline function, as this test is used at multiple places?
--
To view, visit https://review.coreboot.org/c/coreboot/+/84228?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iffabc9037a8af1b2a4ffebdf30199c4f6eae9540
Gerrit-Change-Number: 84228
Gerrit-PatchSet: 2
Gerrit-Owner: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Reviewer: Jeremy Compostella <jeremy.compostella(a)gmail.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Jeremy Compostella <jeremy.compostella(a)gmail.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Comment-Date: Sat, 07 Sep 2024 04:52:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Nicholas Chin has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/84239?usp=email )
Change subject: Doc/rmodules.md: Change header levels
......................................................................
Doc/rmodules.md: Change header levels
Increase the header levels of headers following the initial "Relocatable
Modules (rmodules)" so that there is only one title for the page with
the other headings as subheadings. Also fix header capitalization while
we're here.
Change-Id: I72ae99ba10bf5b2386da2cc702efaf25328d6811
Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com>
---
M Documentation/rmodules.md
1 file changed, 5 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/84239/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/84239?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I72ae99ba10bf5b2386da2cc702efaf25328d6811
Gerrit-Change-Number: 84239
Gerrit-PatchSet: 2
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Nicholas Chin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/84240?usp=email )
Change subject: Doc/rmodules.md: Move to technotes
......................................................................
Doc/rmodules.md: Move to technotes
The content of this page seems to fit the technote category the best, so
move it. This also fixes a "document isn't included in any toctree"
warning from Sphinx since it is now added to the technotes/index.md
toctree.
Change-Id: I86545f4c1a7e1b3ccefa4f6085e764536f33f29c
Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com>
---
M Documentation/technotes/index.md
R Documentation/technotes/rmodules.md
2 files changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/84240/1
diff --git a/Documentation/technotes/index.md b/Documentation/technotes/index.md
index e57519f..a787584 100644
--- a/Documentation/technotes/index.md
+++ b/Documentation/technotes/index.md
@@ -9,4 +9,5 @@
Unit Test Code Coverage <2021-05-code-coverage.md>
Address Sanitizer <asan.md>
coreboot Consoles <console.md>
+Relocatable Modules <rmodules.md>
```
diff --git a/Documentation/rmodules.md b/Documentation/technotes/rmodules.md
similarity index 100%
rename from Documentation/rmodules.md
rename to Documentation/technotes/rmodules.md
--
To view, visit https://review.coreboot.org/c/coreboot/+/84240?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I86545f4c1a7e1b3ccefa4f6085e764536f33f29c
Gerrit-Change-Number: 84240
Gerrit-PatchSet: 1
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Nicholas Chin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/84239?usp=email )
Change subject: Doc/rmodules.md: Fix header levels
......................................................................
Doc/rmodules.md: Fix header levels
Increase the header levels of headers following "Relocatable Modules
(rmodules)" so that there is only one title and the other headings are
subheadings of it. Also fix header capitalization while we're here.
Change-Id: I72ae99ba10bf5b2386da2cc702efaf25328d6811
Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com>
---
M Documentation/rmodules.md
1 file changed, 5 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/84239/1
diff --git a/Documentation/rmodules.md b/Documentation/rmodules.md
index 520d184..39c72b6 100644
--- a/Documentation/rmodules.md
+++ b/Documentation/rmodules.md
@@ -15,9 +15,9 @@
based on the location of the binary (which was returned by CBMEM
at runtime).
-# Implementation Details
+## Implementation Details
-## build time
+### Build Time
At build time the rmodtool (util/cbfstool/rmodtool.c) is used to
create relocatable modules. The rmodtool basically takes an ELF
@@ -44,7 +44,7 @@
2. program (.program)
3. relocation entries (.relocs)
-## runtime
+### Runtime
Either rmodule\_load (lib/rmodule.c) is used directly or through the
rmodule\_stage\_load (lib/rmodule.c) wrapper. It is used to load the
@@ -59,7 +59,7 @@
itself is just a simple addition, that adds an offset from where the
image was "supposed" to be at link time, to where it is now relocated.
-## module\_parameters
+### Module\_parameters
module\_parameters is a section inside the rmodule ELF file. Its
basically a way to pass runtime information to an rmodule
@@ -73,7 +73,7 @@
struct smm_runtime smm_runtime;
```
-# x86 why rmodules
+## x86 why rmodules
//TODO
x86: postcar and ramstage cannot conflict with payload regarding
memory placement. Therefore payload location is usually fixed and
--
To view, visit https://review.coreboot.org/c/coreboot/+/84239?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I72ae99ba10bf5b2386da2cc702efaf25328d6811
Gerrit-Change-Number: 84239
Gerrit-PatchSet: 1
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Attention is currently required from: Ashish Kumar Mishra, Jérémy Compostella, Paul Menzel, Sowmya Aralguppe, Wonkyu Kim.
Subrata Banik has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/83946?usp=email )
Change subject: soc/intel/common/block/cpu: Add Kconfig for effective way size for NEM+
......................................................................
Patch Set 13:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83946/comment/22b65de6_10186f47?us… :
PS13, Line 23: The issue addressed by this commit can be observed with the following
: experiment: using a 18 MB LLC SKU, set `DCACHE_RAM_SIZE` to
: 0x400000 (4 MB).
> > But my question is where is the doc to refer to understand the formula to calculate the eff_way_size (Therefore, we instead compute the effective way size as the biggest of power of two of the way size which works across all three platforms.) I don't see any document that explains the logic mentioned by you in the commit section? that we should only keep the most significant bit position and mask rest to determine the effective way size.
>
> On Alder Lake, the External Design Specification #627270 "3.5.2 No-Eviction Mode (NEM) Sizes" provides a way to compute the effective cache size (and effective way size) by reading the number of CBO using MSR 0x396. I would not qualify this section of the documentation to be crystal clear but the idea is there.
>
> Since Meteor Lake does not have this MSR, the recommendation is to compute the effective cache size following CNDA 1433518 page 15 and determine the NEM size based on the power of 2 of bank number. This CL is implementing the same principle (also aligned with MTL HAS): 1- consider the effective way size for NEM size computation instead of the way size. 2- Compute the effective way size as the biggest power of two of the way size.
i hope you are referring to the section "As an illustration, if a particular SKU features 6 banks totaling 18MB, the NEM would be configured to use 4
banks, which equates to 12MB."
1. it talks about specific SoC sku and i assume this is SoC cache internal logic and nothing requires to configure by FW/SW for lowing the NEM size. For example: with b/306677879, my learning is that, if the NEM is not power of two, we can still get the benefit read into entire NEM range but the range that is > power_of_2 won't be map into DRAM while we are tearing down. By following your logic, we would be limiting the NEM size to smaller window compare to what max we could still use for read benefit (assuming CAR range is configured as WP). This is what we did using https://review.coreboot.org/c/coreboot/+/81269
2. As per above example from Intel doc, it says that the number of NEM bank will be check again the power_of_2 and not the LLC size. for example: NEM bank 6 is not power 2 hence, limit it to 4 and each bank has 3MB so, the LLC size would be 12MB
3. Looking at your logic in same example of 18MB of LLC (which is not power_of_2), you are ending up reserving the most significant 1 and masking rest would mean, the LLC size will be 16MB. Now trying to fit that into NEM bank (3MB/port). You won't be able to do it. 16MB/3MB=~5.33 (which is not power of 2)
4. The better logic would be to find the NEM bank count and then check if it's power_of_2 or not.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83946?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5cb66da0aa977eecb64a0021268a6827747c521c
Gerrit-Change-Number: 83946
Gerrit-PatchSet: 13
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Anil Kumar K <anil.kumar.k(a)intel.com>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Sowmya Aralguppe <sowmya.aralguppe(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sowmya Aralguppe <sowmya.aralguppe(a)intel.corp-partner.google.com>
Gerrit-Attention: Ashish Kumar Mishra <ashish.k.mishra(a)intel.com>
Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Sat, 07 Sep 2024 03:40:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>
Attention is currently required from: Eric Lai, Jérémy Compostella, Kapil Porwal, Pranava Y N.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/84236?usp=email )
Change subject: soc/intel/cmn/block/cpu: Simplify calculation of non-eviction ways
......................................................................
Patch Set 2:
(1 comment)
File src/soc/intel/common/block/cpu/car/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/84236/comment/6c1b2b3b_66807089?us… :
PS1, Line 519: add %edx, %eax
> I find this a bit suspicous, let's take an example: with `CONFIG_DCACHE_RAM_SIZE` at 0x200000 and %ecx (way size) at 0x180000, %edx should be at 0x80000. If you add the remainder to the quotient, you get way count = %eax = 0x80001 isn't ?
for sure, I missed adding the `testl` logic to check if EDX is zero or not before incrementing the quotient.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84236?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I7cf5ff19ec440d049edc3bf52c660dea96b1f08a
Gerrit-Change-Number: 84236
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Pranava Y N <pranavayn(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Eric Lai <ericllai(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Pranava Y N <pranavayn(a)google.com>
Gerrit-Comment-Date: Sat, 07 Sep 2024 03:17:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jérémy Compostella <jeremy.compostella(a)intel.com>
Attention is currently required from: Felix Held, Marshall Dawson, Matt DeVillier.
Hello Felix Held, Marshall Dawson, Matt DeVillier, Zheng Bao, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/84233?usp=email
to look at the new patch set (#5).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: amdfwtool: Add combo new layout for new family
......................................................................
amdfwtool: Add combo new layout for new family
The new layout definition has a new way to support combo.
It packs multiple ISH entries into PSP L1 directory.
TEST=Identical test on all AMD platform
Change-Id: If573cdeaeb56e95d2fed235c9337fab82d622757
Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com>
---
M util/amdfwtool/amdfwtool.c
M util/amdfwtool/amdfwtool.h
M util/amdfwtool/data_parse.c
3 files changed, 30 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/84233/5
--
To view, visit https://review.coreboot.org/c/coreboot/+/84233?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: If573cdeaeb56e95d2fed235c9337fab82d622757
Gerrit-Change-Number: 84233
Gerrit-PatchSet: 5
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>