coreboot-gerrit November 2019

coreboot-gerrit@coreboot.org

1 participants
2418 discussions

Change in coreboot[master]: soc/intel/skylake: add soc implementation for ETR address API
by Michael Niewöhner (Code Review) 03 Nov '19

03 Nov '19

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36569 ) Change subject: soc/intel/skylake: add soc implementation for ETR address API ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/36569/4/src/soc/intel/skylake/pmut… File src/soc/intel/skylake/pmutil.c: https://review.coreboot.org/c/coreboot/+/36569/4/src/soc/intel/skylake/pmut… PS4, Line 178: pcicfg(PCH_DEVFN_PMC << 12)->reg32[ETR / sizeof(uint32_t)]; : } > That should not be exposed outside of pci_mmio_cfg.h. […] this was requested by Aaron -- To view, visit https://review.coreboot.org/c/coreboot/+/36569 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iae54af09347d693620b631721576e4b916ea0f0f Gerrit-Change-Number: 36569 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner 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: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner 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: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 02 Nov 2019 23:44:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [WIP]sb/intel/common: Handle legacy save states properly
by Arthur Heymans (Code Review) 02 Nov '19

02 Nov '19

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35783 ) Change subject: [WIP]sb/intel/common: Handle legacy save states properly ...................................................................... [WIP]sb/intel/common: Handle legacy save states properly This moves southbridge_update_gnvs to a common place. Older 32bit only CPU's have a different SMM save state, requiring a different handling of those. Change-Id: I08391d4ee361ce75507c9de508cd75cc07249fd7 Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/cpu/intel/em64t101_save_state.h M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/common/pmutil.h M src/southbridge/intel/common/smihandler.c M src/southbridge/intel/i82801gx/smihandler.c M src/southbridge/intel/i82801ix/smihandler.c M src/southbridge/intel/i82801jx/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c 9 files changed, 140 insertions(+), 75 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/35783/1 diff --git a/src/cpu/intel/smm/gen1/smmrelocate.c b/src/cpu/intel/smm/gen1/smmrelocate.c index 3eb869a..5350d1c 100644 --- a/src/cpu/intel/smm/gen1/smmrelocate.c +++ b/src/cpu/intel/smm/gen1/smmrelocate.c @@ -168,6 +168,9 @@ if (smm_reloc_params.ied_size) setup_ied_area(&smm_reloc_params); + /* This may not be be correct for older CPU's supported by this code, + but given that em64t101_smm_state_save_area_t is larger than the + save_state of these CPU's it works. */ *smm_save_state_size = sizeof(em64t101_smm_state_save_area_t); } @@ -191,6 +194,8 @@ { msr_t mtrr_cap; struct smm_relocation_params *relo_params = &smm_reloc_params; + /* The em64t101 save state is sufficiently compatible with older + save states with regards of smbase, smm_revision. */ em64t101_smm_state_save_area_t *save_state; u32 smbase = staggered_smbase; u32 iedbase = relo_params->ied_base; @@ -208,7 +213,10 @@ save_state = (void *)(curr_smbase + SMM_DEFAULT_SIZE - sizeof(*save_state)); save_state->smbase = smbase; - save_state->iedbase = iedbase; + + printk(BIOS_SPEW, "SMM revision: 0x%08x\n", save_state->smm_revision); + if (save_state->smm_revision == 0x00030101) + save_state->iedbase = iedbase; /* Write EMRR and SMRR MSRs based on indicated support. */ mtrr_cap = rdmsr(MTRR_CAP_MSR); diff --git a/src/include/cpu/intel/em64t101_save_state.h b/src/include/cpu/intel/em64t101_save_state.h index b8bb2db..7493c85 100644 --- a/src/include/cpu/intel/em64t101_save_state.h +++ b/src/include/cpu/intel/em64t101_save_state.h @@ -20,6 +20,7 @@ /* Intel Revision 30101 SMM State-Save Area * The following processor architectures use this: + * - Nehalem * - SandyBridge * - IvyBridge * - Haswell diff --git a/src/southbridge/intel/bd82x6x/smihandler.c b/src/southbridge/intel/bd82x6x/smihandler.c index ceac598..fe4f80e 100644 --- a/src/southbridge/intel/bd82x6x/smihandler.c +++ b/src/southbridge/intel/bd82x6x/smihandler.c @@ -21,7 +21,6 @@ #include <cpu/x86/cache.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> -#include <cpu/intel/em64t101_save_state.h> #include <northbridge/intel/sandybridge/sandybridge.h> #include <southbridge/intel/bd82x6x/me.h> #include <southbridge/intel/common/gpio.h> @@ -38,6 +37,11 @@ return gnvs; } +void smm_set_gnvs(uintptr_t gnvs_ptr) +{ + gnvs = (global_nvs_t *)gnvs_ptr; +} + int southbridge_io_trap_handler(int smif) { switch (smif) { @@ -211,18 +215,6 @@ xhci_sleep(slp_type); } -void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) -{ - em64t101_smm_state_save_area_t *state = - smi_apmc_find_state_save(apm_cnt); - if (state) { - /* EBX in the state save contains the GNVS pointer */ - gnvs = (global_nvs_t *)((u32)state->rbx); - *smm_done = 1; - printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); - } -} - void southbridge_finalize_all(void) { intel_me_finalize_smm(); diff --git a/src/southbridge/intel/common/pmutil.h b/src/southbridge/intel/common/pmutil.h index cea5c82..b2f1dc4 100644 --- a/src/southbridge/intel/common/pmutil.h +++ b/src/southbridge/intel/common/pmutil.h @@ -18,7 +18,9 @@ #define INTEL_COMMON_PMUTIL_H #include <cpu/x86/smm.h> +#include <cpu/x86/legacy_save_state.h> #include <cpu/intel/em64t101_save_state.h> +#include <cpu/intel/em64t_save_state.h> #define D31F0_PMBASE 0x40 #define D31F0_GEN_PMCON_1 0xa0 @@ -142,10 +144,11 @@ void southbridge_smm_xhci_sleep(u8 slp_type); void gpi_route_interrupt(u8 gpi, u8 mode); void southbridge_gate_memory_reset(void); -void southbridge_update_gnvs(u8 apm_cnt, int *smm_done); +void smm_set_gnvs_ptr(uintptr_t gnvs); void southbridge_finalize_all(void); void southbridge_smi_monitor(void); -em64t101_smm_state_save_area_t *smi_apmc_find_state_save(u8 cmd); + +void *smi_apmc_find_state_save(u8 cmd); void pch_log_state(void); #endif /*INTEL_COMMON_PMUTIL_H */ diff --git a/src/southbridge/intel/common/smihandler.c b/src/southbridge/intel/common/smihandler.c index d61238c..731856a 100644 --- a/src/southbridge/intel/common/smihandler.c +++ b/src/southbridge/intel/common/smihandler.c @@ -210,37 +210,77 @@ } } +enum save_state_type { + EM64T, + EM64T101, + LEGACY +}; + +static enum save_state_type get_save_state_type(void) +{ + /* Is geting this on node 0 fine??? */ + void *save_state = smm_get_save_state(0); + u32 smm_revision = ((em64t101_smm_state_save_area_t *)state)->smm_revision; + switch (smm_revision) { + case 0x00030002: + case 0x00030007: + return LEGACY; + case 0x00030100: + return EM64T; + default: + printk(BIOS_WARNING, "Unknown smm_revision...\n"); + /* Fallthrough */ + case 0x00030101: /* Nahalem, SandyBridge, IvyBridge, and Haswell */ + return EM64T101; + } +} + /* * Look for Synchronous IO SMI and use save state from that * core in case we are not running on the same core that * initiated the IO transaction. */ -em64t101_smm_state_save_area_t *smi_apmc_find_state_save(u8 cmd) +void *smi_apmc_find_state_save(u8 cmd) { - em64t101_smm_state_save_area_t *state; int node; + enum save_state_type save_state_type = get_save_state_type(); - /* Check all nodes looking for the one that issued the IO */ - for (node = 0; node < CONFIG_MAX_CPUS; node++) { - state = smm_get_save_state(node); + if (save_state_type == EM64T || save_state_type == EM64T101) { + /* Check all nodes looking for the one that issued the IO. + TODO: Check against actual amount of CPUs!! */ + for (node = 0; node < CONFIG_MAX_CPUS; node++) { + em64t101_smm_state_save_area_t *state = + (em64t101_smm_state_save_area_t *)smm_get_save_state(node); - /* Check for Synchronous IO (bit0 == 1) */ - if (!(state->io_misc_info & (1 << 0))) - continue; + /* Check for Synchronous IO (bit0 == 1) */ + if (!(state->io_misc_info & (1 << 0))) + continue; - /* Make sure it was a write (bit4 == 0) */ - if (state->io_misc_info & (1 << 4)) - continue; + /* Make sure it was a write (bit4 == 0) */ + if (state->io_misc_info & (1 << 4)) + continue; - /* Check for APMC IO port */ - if (((state->io_misc_info >> 16) & 0xff) != APM_CNT) - continue; + /* Check for APMC IO port */ + if (((state->io_misc_info >> 16) & 0xff) != APM_CNT) + continue; - /* Check AX against the requested command */ - if ((state->rax & 0xff) != cmd) - continue; + /* Check AX against the requested command */ + if ((state->rax & 0xff) != cmd) + continue; - return state; + return state; + } + } + + if (save_state_type == LEGACY) { + for (node = 0; node < CONFIG_MAX_CPUS; node++) { + legacy_smm_state_save_area_t *state = + (legacy_smm_state_save_area_t *)smm_get_save_state(node); + + /* Check AX against the requested command */ + if ((state->eax & 0xff) == cmd) + return state; + } } return NULL; @@ -272,23 +312,62 @@ static void southbridge_smi_store(void) { u8 sub_command, ret; - em64t101_smm_state_save_area_t *io_smi = - smi_apmc_find_state_save(APM_CNT_SMMSTORE); + void *state = smi_apmc_find_state_save(APM_CNT_SMMSTORE); uint32_t reg_ebx; - if (!io_smi) + if (!state) return; /* Command and return value in EAX */ - sub_command = (io_smi->rax >> 8) & 0xff; + switch (get_save_state_type()) { + case EM64T: + case EM64T101: + sub_command = ((u32)((em64t101_smm_state_save_area_t *)state)->rax >> 8) & 0xff; - /* Parameter buffer in EBX */ - reg_ebx = io_smi->rbx; + reg_ebx = (u32)((em64t101_smm_state_save_area_t *)state)->rbx; + break; + case LEGACY: + sub_command = (((legacy_smm_state_save_area_t *)state)->eax >> 8) & 0xff; + reg_ebx = ((legacy_smm_state_save_area_t *)state)->ebx; + break; + } - /* drivers/smmstore/smi.c */ + /* drivers/smmstore/smi.c, parameter buffer in EBX */ ret = smmstore_exec(sub_command, (uintptr_t *)reg_ebx); - io_smi->rax = ret; + + switch (get_save_state_type()) { + case EM64T: + case EM64T101: + ((em64t101_smm_state_save_area_t *)state)->rax = ret; + break; + case LEGACY: + ((legacy_smm_state_save_area_t *)state)->eax = ret; + break; + } } +static void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) +{ + uintptr_t gnvs = 0; + void *state = smi_apmc_find_state_save(apm_cnt); + if (state) { + switch (get_save_state_type()) { + case EM64T: + case EM64T101: + /* EBX in the state save contains the GNVS pointer */ + gnvs = (u32)((em64t101_smm_state_save_area_t *)state)->rbx; + break; + case LEGACY: + gnvs = ((legacy_smm_state_save_area_t *)state)->ebx; + break; + } + *smm_done = 1; + printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); + } + if (gnvs) + smm_set_gnvs_ptr(gnvs); +} + + static int mainboard_finalized = 0; static void southbridge_smi_apmc(void) diff --git a/src/southbridge/intel/i82801gx/smihandler.c b/src/southbridge/intel/i82801gx/smihandler.c index 16ceb13..fccc67d 100644 --- a/src/southbridge/intel/i82801gx/smihandler.c +++ b/src/southbridge/intel/i82801gx/smihandler.c @@ -41,12 +41,11 @@ /* GNVS needs to be updated by an 0xEA PM Trap (B2) after it has been located * by coreboot. */ -global_nvs_t *gnvs = (global_nvs_t *)0x0; +static global_nvs_t *gnvs; -void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) +void smm_set_gnvs_ptr(uintptr_t gnvs_ptr) { - gnvs = *(global_nvs_t **)0x500; - *smm_done = 1; + gnvs = (global_nvs_t *)gnvs; } int southbridge_io_trap_handler(int smif) diff --git a/src/southbridge/intel/i82801ix/smihandler.c b/src/southbridge/intel/i82801ix/smihandler.c index 8090a09..7135425 100644 --- a/src/southbridge/intel/i82801ix/smihandler.c +++ b/src/southbridge/intel/i82801ix/smihandler.c @@ -28,9 +28,7 @@ /* GNVS needs to be updated by an 0xEA PM Trap (B2) after it has been located * by coreboot. */ -global_nvs_t *gnvs = (global_nvs_t *)0x0; -void *tcg = (void *)0x0; -void *smi1 = (void *)0x0; +static global_nvs_t *gnvs; int southbridge_io_trap_handler(int smif) { @@ -49,12 +47,9 @@ return 0; } -void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) +void smm_set_gnvs_ptr(uintptr_t gnvs_ptr) { - gnvs = *(global_nvs_t **)0x500; - tcg = *(void **)0x504; - smi1 = *(void **)0x508; - *smm_done = 1; + gnvs = (global_nvs_t *)gnvs; } void southbridge_smi_monitor(void) diff --git a/src/southbridge/intel/i82801jx/smihandler.c b/src/southbridge/intel/i82801jx/smihandler.c index 667a853..d9e944d 100644 --- a/src/southbridge/intel/i82801jx/smihandler.c +++ b/src/southbridge/intel/i82801jx/smihandler.c @@ -35,9 +35,7 @@ /* GNVS needs to be updated by an 0xEA PM Trap (B2) after it has been located * by coreboot. */ -global_nvs_t *gnvs = (global_nvs_t *)0x0; -void *tcg = (void *)0x0; -void *smi1 = (void *)0x0; +static global_nvs_t *gnvs; int southbridge_io_trap_handler(int smif) { @@ -56,12 +54,9 @@ return 0; } -void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) +void smm_set_gnvs_ptr(uintptr_t gnvs_ptr) { - gnvs = *(global_nvs_t **)0x500; - tcg = *(void **)0x504; - smi1 = *(void **)0x508; - *smm_done = 1; + gnvs = (global_nvs_t *)gnvs; } void southbridge_smi_monitor(void) diff --git a/src/southbridge/intel/ibexpeak/smihandler.c b/src/southbridge/intel/ibexpeak/smihandler.c index 3668842..cc32127 100644 --- a/src/southbridge/intel/ibexpeak/smihandler.c +++ b/src/southbridge/intel/ibexpeak/smihandler.c @@ -21,7 +21,6 @@ #include <cpu/x86/cache.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> -#include <cpu/intel/em64t101_save_state.h> #include <halt.h> #include <pc80/mc146818rtc.h> #include <cpu/intel/model_2065x/model_2065x.h> @@ -44,11 +43,17 @@ * by coreboot. */ static global_nvs_t *gnvs; + global_nvs_t *smm_get_gnvs(void) { return gnvs; } +void smm_set_gnvs_ptr(uintptr_t gnvs_ptr) +{ + gnvs = (global_nvs_t *)gnvs; +} + int southbridge_io_trap_handler(int smif) { switch (smif) { @@ -171,18 +176,6 @@ #undef IOTRAP } -void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) -{ - em64t101_smm_state_save_area_t *state = - smi_apmc_find_state_save(apm_cnt); - if (state) { - /* EBX in the state save contains the GNVS pointer */ - gnvs = (global_nvs_t *)((u32)state->rbx); - *smm_done = 1; - printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); - } -} - void southbridge_finalize_all(void) { intel_me_finalize_smm(); -- To view, visit https://review.coreboot.org/c/coreboot/+/35783 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I08391d4ee361ce75507c9de508cd75cc07249fd7 Gerrit-Change-Number: 35783 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: newchange

3 9

Change in coreboot[master]: cpu/x86/smm: Add helper functions to deal with different save states
by build bot (Jenkins) (Code Review) 02 Nov '19

02 Nov '19

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36589 ) Change subject: cpu/x86/smm: Add helper functions to deal with different save states ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/coreboot/+/36589/1/src/cpu/x86/smm/smihelper.c File src/cpu/x86/smm/smihelper.c: https://review.coreboot.org/c/coreboot/+/36589/1/src/cpu/x86/smm/smihelper.… PS1, Line 33: return EM64T; code indent should use tabs where possible -- To view, visit https://review.coreboot.org/c/coreboot/+/36589 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I85325a25dc38d7cfc7eb0dfde621e5ebfeef0e15 Gerrit-Change-Number: 36589 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 02 Nov 2019 22:32:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common: pmclib: make use of the new ETR address API
by Arthur Heymans (Code Review) 02 Nov '19

02 Nov '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36570 ) Change subject: soc/intel/common: pmclib: make use of the new ETR address API ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block… File src/soc/intel/common/block/pmc/pmclib.c: https://review.coreboot.org/c/coreboot/+/36570/4/src/soc/intel/common/block… PS4, Line 432: void pmc_global_reset_lock(void) : { : uint32_t *etr = soc_read_pmc_etr_addr(); : uint32_t reg; : : reg = read32(etr); : reg = (reg & ~CF9_GLB_RST) | CF9_LOCK; : write32(etr, reg); : } Is it really worth it to have a function and callback for something that can be written down in 1 or 2 lines of code? -- To view, visit https://review.coreboot.org/c/coreboot/+/36570 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I49f59efb4a7c7d3d629ac54a7922bbcc8a87714d Gerrit-Change-Number: 36570 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner 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: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner 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: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 02 Nov 2019 20:42:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/skylake: add soc implementation for ETR address API
by Arthur Heymans (Code Review) 02 Nov '19

02 Nov '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36569 ) Change subject: soc/intel/skylake: add soc implementation for ETR address API ...................................................................... Patch Set 4: Code-Review-1 (1 comment) https://review.coreboot.org/c/coreboot/+/36569/4/src/soc/intel/skylake/pmut… File src/soc/intel/skylake/pmutil.c: https://review.coreboot.org/c/coreboot/+/36569/4/src/soc/intel/skylake/pmut… PS4, Line 178: pcicfg(PCH_DEVFN_PMC << 12)->reg32[ETR / sizeof(uint32_t)]; : } That should not be exposed outside of pci_mmio_cfg.h. Why not use the regular pci ops? -- To view, visit https://review.coreboot.org/c/coreboot/+/36569 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iae54af09347d693620b631721576e4b916ea0f0f Gerrit-Change-Number: 36569 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner 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: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner 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: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 02 Nov 2019 20:32:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/x86: Add a prog_run hook to set up caching of XIP stages
by Arthur Heymans (Code Review) 02 Nov '19

02 Nov '19

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35993 ) Change subject: cpu/x86: Add a prog_run hook to set up caching of XIP stages ...................................................................... cpu/x86: Add a prog_run hook to set up caching of XIP stages Some platforms lack a non-eviction mode and therefore caching the whole ROM to speed up XIP stages can be dangerous as it could result in eviction if too much of the ROM is being accessed. The solution is to only cache a region, about the size of the stage that the bootblock is about to load: verstage and/or romstage. CR0.CD bits are not touched to do this, but this seems to work just fine on at least model_1067x and model_6ex. Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> --- M src/cpu/x86/Kconfig M src/cpu/x86/mtrr/Makefile.inc A src/cpu/x86/mtrr/xip_cache.c 3 files changed, 73 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/35993/1 diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index a8cf54d..b316c1f 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -78,6 +78,16 @@ depends on !NO_FIXED_XIP_ROM_SIZE default 0x10000 +config SETUP_XIP_CACHE + bool + depends on C_ENVIRONMENT_BOOTBLOCK + depends on !NO_XIP_EARLY_STAGES + help + Select this option to set up an MTRR to cache XIP stages loaded + from the bootblock. This is useful on platforms lacking a + non-eviction mode and therefore need to be careful to avoid + eviction. + config CPU_ADDR_BITS int default 36 diff --git a/src/cpu/x86/mtrr/Makefile.inc b/src/cpu/x86/mtrr/Makefile.inc index caa6e9c..dc914de 100644 --- a/src/cpu/x86/mtrr/Makefile.inc +++ b/src/cpu/x86/mtrr/Makefile.inc @@ -7,3 +7,5 @@ romstage-y += debug.c postcar-y += debug.c ramstage-y += debug.c + +bootblock-$(CONFIG_SETUP_XIP_CACHE) += xip_cache.c diff --git a/src/cpu/x86/mtrr/xip_cache.c b/src/cpu/x86/mtrr/xip_cache.c new file mode 100644 index 0000000..a45944b --- /dev/null +++ b/src/cpu/x86/mtrr/xip_cache.c @@ -0,0 +1,61 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/cpu.h> +#include <program_loading.h> +#include <commonlib/region.h> +#include <console/console.h> +#include <cpu/x86/mtrr.h> + +#define MAX_ROM_CACHE_SIZE (256 * KiB) + +void platform_prog_run(struct prog *prog) +{ + uint32_t base = region_device_offset(&prog->rdev); + uint32_t size = region_device_sz(&prog->rdev); + uint32_t end = base + size; + int mtrr_num = get_free_var_mtrr(); + uint32_t mtrr_mask_size = 4 * KiB; + struct cpuinfo_x86 cpu_info; + + get_fms(&cpu_info, cpuid_eax(1)); + if (cpu_info.x86 == 0xf) { + printk(BIOS_DEBUG, + "PROG_RUN: CPU does not support caching ROM\n" + "The next stage will run slowly\n"); + return; + } + + if (mtrr_num == -1) { + printk(BIOS_NOTICE, + "PROG_RUN: No MTRR available to cache ROM!\n" + "The next stage will run slowly!\n"); + return; + } + + while (ALIGN_DOWN(base, mtrr_mask_size) + mtrr_mask_size < end) + mtrr_mask_size *= 2; + base = ALIGN_DOWN(base, mtrr_mask_size); + if (mtrr_mask_size > MAX_ROM_CACHE_SIZE) { + printk(BIOS_WARNING, + "PROG_RUN: %dKiB XIP cache requested but limiting to %dKiB!", + mtrr_mask_size, MAX_ROM_CACHE_SIZE); + mtrr_mask_size = MAX_ROM_CACHE_SIZE; + } + + printk(BIOS_DEBUG, + "PROG_RUN: Setting MTRR to cache XIP stage. base: 0x%08x, size: 0x%08x\n", + base, mtrr_mask_size); + + set_var_mtrr(mtrr_num, base, mtrr_mask_size, MTRR_TYPE_WRPROT); +} -- To view, visit https://review.coreboot.org/c/coreboot/+/35993 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I94d5771a57ffd74d53db3e35fe169d77d7fbb8cd Gerrit-Change-Number: 35993 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-MessageType: newchange

6 43

Change in coreboot[master]: soc/intel/skylake: lockdown: lock global reset
by Michael Niewöhner (Code Review) 02 Nov '19

02 Nov '19

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36573 ) Change subject: soc/intel/skylake: lockdown: lock global reset ...................................................................... Uploaded patch set 6: Commit message was updated. -- To view, visit https://review.coreboot.org/c/coreboot/+/36573 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If190c3c66889ede105d958b423b38ebdcb698332 Gerrit-Change-Number: 36573 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Niewöhner 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: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner 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: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 02 Nov 2019 17:30:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/skylake: lockdown: lock global reset
by Michael Niewöhner (Code Review) 02 Nov '19

02 Nov '19

Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Subrata Banik, Angel Pons, Arthur Heymans, build bot (Jenkins), Nico Huber, Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/36573 to look at the new patch set (#6). Change subject: soc/intel/skylake: lockdown: lock global reset ...................................................................... soc/intel/skylake: lockdown: lock global reset There are four chipsets selecting PMC_GLOBAL_RESET_ENABLE_LOCK but only one (apollolake) is actually calling the code. Add the missing call. Also fix the register offset in a comment in reset code. Tested successfully on X11SSM-F by reading ETR3. Change-Id: If190c3c66889ede105d958b423b38ebdcb698332 Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de> --- M src/soc/intel/skylake/lockdown.c M src/soc/intel/skylake/reset.c 2 files changed, 5 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/36573/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/36573 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If190c3c66889ede105d958b423b38ebdcb698332 Gerrit-Change-Number: 36573 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Niewöhner 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: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner 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: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/common: pmclib: add API to get ETR register address
by Michael Niewöhner (Code Review) 02 Nov '19

02 Nov '19

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36565 ) Change subject: soc/intel/common: pmclib: add API to get ETR register address ...................................................................... Patch Set 4: (1 comment) https://review.coreboot.org/c/coreboot/+/36565/3/src/soc/intel/common/block… File src/soc/intel/common/block/include/intelblocks/pmclib.h: https://review.coreboot.org/c/coreboot/+/36565/3/src/soc/intel/common/block… PS3, Line 173: mmconf > It's still only MMCONF on SKL. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/36565 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I706f3e220d639a6133625e3cb7267f7009006af2 Gerrit-Change-Number: 36565 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Niewöhner 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: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner 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: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 02 Nov 2019 17:29:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/skylake: lockdown: lock global reset
by Michael Niewöhner (Code Review) 02 Nov '19

02 Nov '19

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36573 ) Change subject: soc/intel/skylake: lockdown: lock global reset ...................................................................... Uploaded patch set 5: Patch Set 4 was rebased. -- To view, visit https://review.coreboot.org/c/coreboot/+/36573 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If190c3c66889ede105d958b423b38ebdcb698332 Gerrit-Change-Number: 36573 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Niewöhner 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: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner 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: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sat, 02 Nov 2019 17:29:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2019