Attention is currently required from: Darius Goad, Patrick Rudolph.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57089 )
Change subject: cpu/intel: Make doubly sure we don't write to IA32_FEATURE_CTRL if its lock bit is set on a reset.
......................................................................
Patch Set 1:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57089/comment/7eabab88_a8f84df7
PS1, Line 2: qeeg
Please use *Darius Goad*.
$ git commit --amend --author="Darius Goad <mszoopers(a)protonmail.com>"
$ git config --global user.name "Darius Goad <mszoopers(a)protonmail.com>"
https://review.coreboot.org/c/coreboot/+/57089/comment/6c3d2446_66525b8c
PS1, Line 8: lock bit is set on a reset.
Please shorten it, and remove the dot/period at the end. Maybe:
> cpu/intel/model_1067x: Do not write to IA32_FEATURE_CTRL with lock bit set on reset
https://review.coreboot.org/c/coreboot/+/57089/comment/b7cbfa2a_8f117167
PS1, Line 9:
Can you describe the problem first?
Patchset:
PS1:
Welcome to coreboot!
File src/cpu/intel/model_1067x/mp_init.c:
https://review.coreboot.org/c/coreboot/+/57089/comment/1fb8bf55_a4acd1d5
PS1, Line 63: return;
Why is this removed?
--
To view, visit https://review.coreboot.org/c/coreboot/+/57089
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib6ff08b6997e5662d32ff003a436c62ccc1c40ac
Gerrit-Change-Number: 57089
Gerrit-PatchSet: 1
Gerrit-Owner: Darius Goad
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Darius Goad
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Mon, 23 Aug 2021 07:21:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Zheng Bao.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57063 )
Change subject: amdfwtool: Detect the flag multilevel to decide the actual value
......................................................................
Patch Set 2:
(2 comments)
File util/amdfwtool/amdfwtool.c:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-126552):
https://review.coreboot.org/c/coreboot/+/57063/comment/4c016a88_c26fc7b2
PS2, Line 589: amd_cb_config *cb_config)
need consistent spacing around '*' (ctx:WxV)
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-126552):
https://review.coreboot.org/c/coreboot/+/57063/comment/8b8bdbca_f419587c
PS2, Line 770: amd_cb_config *cb_config)
need consistent spacing around '*' (ctx:WxV)
--
To view, visit https://review.coreboot.org/c/coreboot/+/57063
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ibe8cdd5c14225899352b02bb19aae6059d56d428
Gerrit-Change-Number: 57063
Gerrit-PatchSet: 2
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Zheng Bao
Gerrit-Comment-Date: Mon, 23 Aug 2021 07:01:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Zheng Bao.
Hello build bot (Jenkins), Zheng Bao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/57064
to look at the new patch set (#2).
Change subject: amdfwtool: Remove some duplicate binaries/entries in PSP table
......................................................................
amdfwtool: Remove some duplicate binaries/entries in PSP table
Based on test on Cezanne, these duplicated FWs can be removed from
level 1 entry.
The recovery bootloader and other recovery issue is goint to be
handled in later other patch series.
Please refer the previous change,
https://review.coreboot.org/c/coreboot/+/57063
which can make the Picasso platform, which only has one level, not
change.
BUG=b:195329409
Test=Majolica (Cezanne) & Mandolin (Picasso)
Change-Id: Ia81b8a85792d1565445803100bdb01c3436e5698
Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com>
---
M util/amdfwtool/amdfwtool.c
1 file changed, 15 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/57064/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/57064
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia81b8a85792d1565445803100bdb01c3436e5698
Gerrit-Change-Number: 57064
Gerrit-PatchSet: 2
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Zheng Bao
Gerrit-MessageType: newpatchset
Attention is currently required from: Zheng Bao.
Hello build bot (Jenkins), Zheng Bao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/57063
to look at the new patch set (#2).
Change subject: amdfwtool: Detect the flag multilevel to decide the actual value
......................................................................
amdfwtool: Detect the flag multilevel to decide the actual value
To save the space for FW, some of the FWs are going to be define as
LVL2 entries. To be compatible to "flattened" layout, we still drop
the LVL2 entry to level1 if there is only one level.
Change-Id: Ibe8cdd5c14225899352b02bb19aae6059d56d428
Signed-off-by: Zheng Bao <fishbaozi(a)gmail.com>
---
M util/amdfwtool/amdfwtool.c
1 file changed, 16 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/57063/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/57063
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ibe8cdd5c14225899352b02bb19aae6059d56d428
Gerrit-Change-Number: 57063
Gerrit-PatchSet: 2
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Zheng Bao
Gerrit-MessageType: newpatchset
Attention is currently required from: Sugnan Prabhu S, Tim Wawrzynczak, Paul Menzel.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56750 )
Change subject: vc/google/chromeos: Add support for new SAR tables revisions
......................................................................
Patch Set 17:
(1 comment)
File src/include/sar.h:
https://review.coreboot.org/c/coreboot/+/56750/comment/e76738b9_796635f7
PS17, Line 21: struct wifi_sar_delta_table {
: uint8_t version;
: union {
: struct {
: uint8_t power_max_2400mhz;
: uint8_t power_chain_a_2400mhz;
: uint8_t power_chain_b_2400mhz;
: uint8_t power_max_5200mhz;
: uint8_t power_chain_a_5200mhz;
: uint8_t power_chain_b_5200mhz;
: } __packed group_rev0[SAR_NUM_WGDS_GROUPS];
: struct {
: uint8_t power_max_2400mhz;
: uint8_t power_chain_a_2400mhz;
: uint8_t power_chain_b_2400mhz;
: uint8_t power_max_5200mhz;
: uint8_t power_chain_a_5200mhz;
: uint8_t power_chain_b_5200mhz;
: uint8_t power_max_6000mhz;
: uint8_t power_chain_a_6000mhz;
: uint8_t power_chain_b_6000mhz;
: } __packed group_rev1[SAR_NUM_WGDS_GROUPS];
: } __packed;
: } __packed;
> This think this will be better approach. […]
Thanks Sugnan. Before you go ahead with the changes, let's just wait one more day to see if Tim has any other comments as well. Don't want to make you go back and forth between different implementations :). Thanks!
--
To view, visit https://review.coreboot.org/c/coreboot/+/56750
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I08c3f321938eba04e8bcff4d87cb215422715bb2
Gerrit-Change-Number: 56750
Gerrit-PatchSet: 17
Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Dtrain Hsu <dtrain_hsu(a)compal.corp-partner.google.com>
Gerrit-CC: Jamie Chen <jamie.chen(a)intel.com>
Gerrit-CC: Janiex Tu <janiex.tu(a)intel.corp-partner.google.com>
Gerrit-CC: Matt Chen <matt.chen(a)intel.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-CC: Simon Yang <simon1.yang(a)intel.com>
Gerrit-Attention: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Mon, 23 Aug 2021 06:29:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com>
Gerrit-MessageType: comment
Name of user not set #1003839 has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/57088 )
Change subject: cpu/intel: Make doubly sure we don't write to IA32_FEATURE_CTRL if its lock bit is set on a reset.
......................................................................
Abandoned
Wrong fix.
--
To view, visit https://review.coreboot.org/c/coreboot/+/57088
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id21cc304b1e1e2871b684e287713af1c6a603efe
Gerrit-Change-Number: 57088
Gerrit-PatchSet: 2
Gerrit-Owner: Name of user not set #1003839
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: abandon
Name of user not set #1003839 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/57089 )
Change subject: cpu/intel: Make doubly sure we don't write to IA32_FEATURE_CTRL if its lock bit is set on a reset.
......................................................................
cpu/intel: Make doubly sure we don't write to IA32_FEATURE_CTRL if its
lock bit is set on a reset.
The lock bit in this MSR is preserved across resets.
Change-Id: Ib6ff08b6997e5662d32ff003a436c62ccc1c40ac
Signed-off-by: Darius Goad <mszoopers(a)protonmail.com>
---
M src/cpu/intel/model_1067x/mp_init.c
1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/57089/1
diff --git a/src/cpu/intel/model_1067x/mp_init.c b/src/cpu/intel/model_1067x/mp_init.c
index 7b26875..5df5365 100644
--- a/src/cpu/intel/model_1067x/mp_init.c
+++ b/src/cpu/intel/model_1067x/mp_init.c
@@ -48,7 +48,6 @@
{
msr_t mtrr_cap = rdmsr(MTRR_CAP_MSR);
if (cpu_has_alternative_smrr() && mtrr_cap.lo & SMRR_SUPPORTED) {
- set_feature_ctrl_vmx();
msr_t ia32_ft_ctrl = rdmsr(IA32_FEATURE_CONTROL);
/* We don't care if the lock is already setting
as our smm relocation handler is able to handle
@@ -59,12 +58,11 @@
printk(BIOS_DEBUG, "IA32_FEATURE_CONTROL already locked\n");
printk(BIOS_DEBUG, "SMRR status: %senabled\n",
ia32_ft_ctrl.lo & (1 << 3) ? "" : "not ");
- smm_relocate();
- return;
} else {
if (!CONFIG(SET_IA32_FC_LOCK_BIT))
printk(BIOS_INFO,
"Overriding CONFIG(SET_IA32_FC_LOCK_BIT) to enable SMRR\n");
+ set_feature_ctrl_vmx();
ia32_ft_ctrl.lo |= (1 << 3) | (1 << 0);
wrmsr(IA32_FEATURE_CONTROL, ia32_ft_ctrl);
}
--
To view, visit https://review.coreboot.org/c/coreboot/+/57089
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib6ff08b6997e5662d32ff003a436c62ccc1c40ac
Gerrit-Change-Number: 57089
Gerrit-PatchSet: 1
Gerrit-Owner: Name of user not set #1003839
Gerrit-MessageType: newchange