Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30209 )
Change subject: soc/intel/common/block/lpc: create LPC_GET_DEV macro
......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
PS2, Line 29: #if !defined(__SIMPLE_DEVICE__)
: #include <device/device.h>
: #define LPC_GET_DEV pcidev_on_root(PCH_DEV_SLOT_LPC, 0x0)
: #else
: #define LPC_GET_DEV PCH_DEV_LPC
: #endif
> would this be inherently taken care by: […]
right now we have a
#define __SIMPLE_DEVICE__ guard at line 18
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block…
this tells all PCH_DEV_LPC reference to go to simple device check in entire file.
problem is this file only complies to simple_device as we have guard above. after deleting tha guard, ramstage has started giving error as below (because of fetching code from !__SIMPLE_DEVICE__ in pch_devs.h)
CC ramstage/soc/intel/common/block/lpc/lpc_lib.o
In file included from
src/soc/intel/common/block/lpc/lpc_lib.c:27:
src/soc/intel/common/block/lpc/lpc_lib.c: In function 'lpc_set_bios_control_reg':
src/soc/intel/cannonlake/include/soc/pci_devs.h:26:30: error: initialization of 'pci_devfn_t' {aka 'unsigned in '} from 'struct device *' makes integer from pointer without a cast [-Werror=int-conversion]
#define _PCH_DEV(slot, func) pcidev_path_on_root_debug(_PCH_DEVFN(slot, func), __func__)
^~~~~~~~~~~~~~~~~~~~~~~~~
src/soc/intel/cannonlake/include/soc/pci_devs.h:187:23: note: in expansion of macro '_PCH_DEV'
#define PCH_DEV_LPC _PCH_DEV(LPC, 0)
^~~~~~~~
src/soc/intel/common/block/lpc/lpc_lib.c:179:20: note: in expansion of macro 'PCH_DEV_LPC'
pci_devfn_t dev = PCH_DEV_LPC;
^~~~~~~~~~~
src/soc/intel/common/block/lpc/lpc_lib.c:183:29: error: passing argument 1 of 'pci_read_config8' makes pointer from integer without a cast [-Werror=int-conversion]
bc_cntl = pci_read_config8(dev, LPC_BIOS_CNTL);
^~~
--
To view, visit https://review.coreboot.org/c/coreboot/+/30209
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I84a6102bf3849e9d4fe28e4c6a11bc7badcf5114
Gerrit-Change-Number: 30209
Gerrit-PatchSet: 2
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 08:32:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-MessageType: comment
Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38779 )
Change subject: vboot: push clear recovery mode switch until later
......................................................................
vboot: push clear recovery mode switch until later
On some platforms, FSP initialization may cause a reboot. Push
clearing the recovery mode switch until after FSP code runs, so
that a manual recovery request (three-finger salute) will
function correctly under this condition.
BUG=b:124141368, b:35576380
TEST=make clean && make test-abuild
BRANCH=none
Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad
Signed-off-by: Joel Kitching <kitching(a)google.com>
---
M src/security/vboot/bootmode.c
M src/security/vboot/vboot_logic.c
2 files changed, 19 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/38779/1
diff --git a/src/security/vboot/bootmode.c b/src/security/vboot/bootmode.c
index 83baa81..61b3d76 100644
--- a/src/security/vboot/bootmode.c
+++ b/src/security/vboot/bootmode.c
@@ -24,6 +24,25 @@
#include <security/vboot/vbnv.h>
#include <security/vboot/vboot_common.h>
+static void vboot_log_and_clear_recovery_mode_switch(void *unused)
+{
+ /* Log the recovery mode switches if required, before clearing them. */
+ log_recovery_mode_switch();
+
+ /*
+ * The recovery mode switch is cleared (typically backed by EC) here
+ * to allow multiple queries to get_recovery_mode_switch() and have
+ * them return consistent results during the verified boot path as well
+ * as dram initialization. x86 systems ignore the saved dram settings
+ * in the recovery path in order to start from a clean slate. Therefore
+ * clear the state here since this function is called when memory
+ * is known to be up.
+ */
+ clear_recovery_mode_switch();
+}
+BOOT_STATE_INIT_ENTRY(BS_DEV_INIT, BS_ON_EXIT,
+ vboot_log_and_clear_recovery_mode_switch, NULL);
+
static int vboot_get_recovery_reason_shared_data(void)
{
struct vb2_shared_data *sd = vb2_get_sd(vboot_get_context());
diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c
index 1d17a17..eabf8f7 100644
--- a/src/security/vboot/vboot_logic.c
+++ b/src/security/vboot/vboot_logic.c
@@ -269,26 +269,6 @@
vboot_extend_pcr(ctx, 1, HWID_DIGEST_PCR);
}
-static void vboot_log_and_clear_recovery_mode_switch(int unused)
-{
- /* Log the recovery mode switches if required, before clearing them. */
- log_recovery_mode_switch();
-
- /*
- * The recovery mode switch is cleared (typically backed by EC) here
- * to allow multiple queries to get_recovery_mode_switch() and have
- * them return consistent results during the verified boot path as well
- * as dram initialization. x86 systems ignore the saved dram settings
- * in the recovery path in order to start from a clean slate. Therefore
- * clear the state here since this function is called when memory
- * is known to be up.
- */
- clear_recovery_mode_switch();
-}
-#if !CONFIG(VBOOT_STARTS_IN_ROMSTAGE)
-ROMSTAGE_CBMEM_INIT_HOOK(vboot_log_and_clear_recovery_mode_switch)
-#endif
-
/**
* Verify and select the firmware in the RW image
*
@@ -449,11 +429,6 @@
vboot_is_firmware_slot_a(ctx) ? 'A' : 'B');
verstage_main_exit:
- /* If CBMEM is not up yet, let the ROMSTAGE_CBMEM_INIT_HOOK take care
- of running this function. */
- if (ENV_ROMSTAGE && CONFIG(VBOOT_STARTS_IN_ROMSTAGE))
- vboot_log_and_clear_recovery_mode_switch(0);
-
/* Save recovery reason in case of unexpected reboots on x86. */
vboot_save_recovery_reason_vbnv();
--
To view, visit https://review.coreboot.org/c/coreboot/+/38779
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I30c02787c620b937e5a50a5ed94ac906e3112dad
Gerrit-Change-Number: 38779
Gerrit-PatchSet: 1
Gerrit-Owner: Joel Kitching <kitching(a)google.com>
Gerrit-MessageType: newchange
Usha P has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38824 )
Change subject: soc/intel/apollolake: Display apollolake platform information
......................................................................
Patch Set 3:
> Patch Set 3:
>
> > Patch Set 3:
> >
> > > Patch Set 3:
> > >
> > > > Patch Set 3:
> > > >
> > > > Is there a reason why this needs to be part of bootblock?
> > > > We have size constrains in bootblock anyway and on the other hand have a plenty of space in the ramstage? Wouldn't it be as useful in ramstage?
> > >
> > > good point, right now its more over parity with other soc design we have where bootblock does report platform early.
> > >
> > > But fare point to move those in romstage may be as these are sorts of information to have, nothing as such mandatory to have inside BB itself. Agree ?
> >
> > Sure, romstage is OK as well OFC.
> > I just stumbled across this issue as the bootblock size was increased and I do not see a good reason for it in this context.
>
> yeah make sense.
>
> @Usha, possible for you to test this CL moving report platform into romstage ?
>
> Also how about 1 dedicated CL for rest of IA-SoC to move report platform into romstage.
>
> Once sorted we can create common report_platform.c for all SOC and use it from romstage?
Yes Subrata, this can be done. I will test this CL by moving it into romstage and update.
--
To view, visit https://review.coreboot.org/c/coreboot/+/38824
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id4edfeae7faee9f5f80698cf34b31fdcb066a813
Gerrit-Change-Number: 38824
Gerrit-PatchSet: 3
Gerrit-Owner: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 07:53:00 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38824 )
Change subject: soc/intel/apollolake: Display apollolake platform information
......................................................................
Patch Set 3:
> Patch Set 3:
>
> > Patch Set 3:
> >
> > > Patch Set 3:
> > >
> > > Is there a reason why this needs to be part of bootblock?
> > > We have size constrains in bootblock anyway and on the other hand have a plenty of space in the ramstage? Wouldn't it be as useful in ramstage?
> >
> > good point, right now its more over parity with other soc design we have where bootblock does report platform early.
> >
> > But fare point to move those in romstage may be as these are sorts of information to have, nothing as such mandatory to have inside BB itself. Agree ?
>
> Sure, romstage is OK as well OFC.
> I just stumbled across this issue as the bootblock size was increased and I do not see a good reason for it in this context.
yeah make sense.
@Usha, possible for you to test this CL moving report platform into romstage ?
Also how about 1 dedicated CL for rest of IA-SoC to move report platform into romstage.
Once sorted we can create common report_platform.c for all SOC and use it from romstage?
--
To view, visit https://review.coreboot.org/c/coreboot/+/38824
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id4edfeae7faee9f5f80698cf34b31fdcb066a813
Gerrit-Change-Number: 38824
Gerrit-PatchSet: 3
Gerrit-Owner: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 07:48:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38824 )
Change subject: soc/intel/apollolake: Display apollolake platform information
......................................................................
Patch Set 3:
> Patch Set 3:
>
> > Patch Set 3:
> >
> > Is there a reason why this needs to be part of bootblock?
> > We have size constrains in bootblock anyway and on the other hand have a plenty of space in the ramstage? Wouldn't it be as useful in ramstage?
>
> good point, right now its more over parity with other soc design we have where bootblock does report platform early.
>
> But fare point to move those in romstage may be as these are sorts of information to have, nothing as such mandatory to have inside BB itself. Agree ?
Sure, romstage is OK as well OFC.
I just stumbled across this issue as the bootblock size was increased and I do not see a good reason for it in this context.
--
To view, visit https://review.coreboot.org/c/coreboot/+/38824
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id4edfeae7faee9f5f80698cf34b31fdcb066a813
Gerrit-Change-Number: 38824
Gerrit-PatchSet: 3
Gerrit-Owner: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 07:45:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38824 )
Change subject: soc/intel/apollolake: Display apollolake platform information
......................................................................
Patch Set 3:
> Patch Set 3:
>
> Is there a reason why this needs to be part of bootblock?
> We have size constrains in bootblock anyway and on the other hand have a plenty of space in the ramstage? Wouldn't it be as useful in ramstage?
good point, right now its more over parity with other soc design we have where bootblock does report platform early.
But fare point to move those in romstage may be as these are sorts of information to have, nothing as such mandatory to have inside BB itself. Agree ?
--
To view, visit https://review.coreboot.org/c/coreboot/+/38824
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id4edfeae7faee9f5f80698cf34b31fdcb066a813
Gerrit-Change-Number: 38824
Gerrit-PatchSet: 3
Gerrit-Owner: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 07:40:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38640 )
Change subject: cpu: Add microcode at pre-defined address
......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38640/6//COMMIT_MSG
Commit Message:
https://review.coreboot.org/c/coreboot/+/38640/6//COMMIT_MSG@7
PS6, Line 7: c
> the assembly code around FSP-T invocation actually finds microcode location and size. […]
I'm aware that it's not possible to give the microcode location to FSP-T during runtime.
Coreboot has assembly code using only registers (for pre-CAR use) to find the location of the microcode cbfs file and then find the proper update and do the update itself. So using that and completely skipping FSP-T doing the update would be preferable.
--
To view, visit https://review.coreboot.org/c/coreboot/+/38640
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I6c02601a7ac64078e556e2032baeccaf27f77da2
Gerrit-Change-Number: 38640
Gerrit-PatchSet: 7
Gerrit-Owner: Andrey Petrov <anpetrov(a)fb.com>
Gerrit-Reviewer: Andrey Petrov <anpetrov(a)fb.com>
Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Reviewer: Lijian Zhao <lijian.zhao(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Mon, 17 Feb 2020 07:25:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: Andrey Petrov <anpetrov(a)fb.com>
Gerrit-MessageType: comment
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38824 )
Change subject: soc/intel/apollolake: Display apollolake platform information
......................................................................
Patch Set 3:
Is there a reason why this needs to be part of bootblock?
We have size constrains in bootblock anyway and on the other hand have a plenty of space in the ramstage? Wouldn't it be as useful in ramstage?
--
To view, visit https://review.coreboot.org/c/coreboot/+/38824
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id4edfeae7faee9f5f80698cf34b31fdcb066a813
Gerrit-Change-Number: 38824
Gerrit-PatchSet: 3
Gerrit-Owner: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Usha P <usha.p(a)intel.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Mon, 17 Feb 2020 06:46:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment