coreboot-gerrit October 2020

coreboot-gerrit@coreboot.org

1 participants
3474 discussions

Change in coreboot[master]: libpayload/x86: Try to discover invariant TSC rate
by Nico Huber (Code Review) 14 Oct '20

14 Oct '20

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34170 ) Change subject: libpayload/x86: Try to discover invariant TSC rate ...................................................................... Patch Set 4: (5 comments) > > This might need checking how it behaves with Depthcharge. I was > > actually wondering how CrOS devices handle it... is the PIT enabled? > > do you have a different timer? > > I can confirm in the volteer (TGL) next to me that these two patches allow me to call `get_cpu_speed()` without hanging 😊. Without these, it hangs on the call, presumably b/c we leave the PIT disabled. We haven't used PIT for timing since some old skylake boards I see. Current boards AFAICT just use the timestamp counter It's all about using the TSC, but you need to know it's rate. Still wonder how you avoid calling get_cpu_speed(). https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x… File payloads/libpayload/arch/x86/timer.c: https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x… PS4, Line 90: ecx > yeah I guess it wouldn't work b/c of the macro. Thanks Angel. Indeed, I just wanted to avoid the cast. Doesn't matter much though I guess, could change it? https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x… PS4, Line 106: switch (cpuid_model()) { : case 0x4e: /* SKL-U/Y */ : case 0x5e: /* SKL-S/H */ : case 0x8e: /* KBL-U/Y */ : case 0x9e: /* KBL-S/H */ : nominal = 24000000; : break; : case 0x5c: /* APL */ : nominal = 19200000; : break; : default: : return -1; : } : } : : return nominal * num / denom / 1000; : } : : /** : * @brief Returns three times the bus clock in kHz : * : * The result of calculations with the returned value shall be divided by 3. : * This helps to avoid rounding errors. : */ : static int get_bus_khz_x3(void) : { : if (cpuid_family() != 6) : return -1; : : switch (cpuid_model()) { : case 0x25: /* Nehalem */ : return 400 * 1000; /* 133 MHz */ : case 0x2a: /* SandyBridge */ : case 0x3a: /* IvyBridge */ : case 0x3c: /* Haswell */ : case 0x3d: /* Broadwell */ : case 0x45: /* Haswell-ULT */ : case 0x46: /* Haswell-GT3e */ : return 300 * 1000; /* 100 MHz */ : default: : return -1; : } : } > suggestion: an enum for the results of cpuid_model() would be more readable than comments I like that! https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x… PS4, Line 172: /* > newline above Is that mandatory? Currently, the whole function does either what is above the empty line or what is below... Adding another empty line would make me want to split the second part into another function. Could do that ofc. https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x… PS4, Line 176: _rdmsr(MSR_PLATFORM_INFO) >> 8 > well, maybe even both lol, I can definitely drop those below. But would really not like to add unneeded ones. Unneeded brackets => people don't learn C => people make mistakes. https://review.coreboot.org/c/coreboot/+/34170/4/payloads/libpayload/arch/x… PS4, Line 186: cpu_khz > hm, somehow I seem to miss what this global variable is useful for (yeah I know, it was there before […] The whole point of this file is to discover the TSC rate. So you can later do rdtsc()*x/cpu_khz to get a monotonic time in s/ms/us/ns. -- To view, visit https://review.coreboot.org/c/coreboot/+/34170 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic6607ee2a8b41c2be9dc1bb4f1e23e652bb33889 Gerrit-Change-Number: 34170 Gerrit-PatchSet: 4 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Wed, 14 Oct 2020 19:16:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/cnl: lock AES-NI feature if selected
by Nico Huber (Code Review) 14 Oct '20

14 Oct '20

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46273 ) Change subject: soc/intel/cnl: lock AES-NI feature if selected ...................................................................... Patch Set 6: Code-Review+2 (1 comment) https://review.coreboot.org/c/coreboot/+/46273/6/src/soc/intel/cannonlake/K… File src/soc/intel/cannonlake/Kconfig: https://review.coreboot.org/c/coreboot/+/46273/6/src/soc/intel/cannonlake/K… PS6, Line 81: select CPU_INTEL_COMMON_HYPERTHREADING Wondering why we have that, i.e. would there be any harm if we'd assume it's `y`? -- To view, visit https://review.coreboot.org/c/coreboot/+/46273 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79495bfbd3ebf3b712ce9ecf2040cecfd954178d Gerrit-Change-Number: 46273 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 14 Oct 2020 18:56:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/intel/common: add a Kconfig to control AES-NI locking
by Nico Huber (Code Review) 14 Oct '20

14 Oct '20

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46277 ) Change subject: cpu/intel/common: add a Kconfig to control AES-NI locking ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/46277 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4eaf8d7d187188ee6e78741b1ceb837c40c2c402 Gerrit-Change-Number: 46277 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 14 Oct 2020 18:55:20 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: cpu/intel/common: rework AES-NI locking
by Nico Huber (Code Review) 14 Oct '20

14 Oct '20

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46275 ) Change subject: cpu/intel/common: rework AES-NI locking ...................................................................... Patch Set 3: (1 comment) https://review.coreboot.org/c/coreboot/+/46275/3/src/cpu/intel/common/commo… File src/cpu/intel/common/common_init.c: https://review.coreboot.org/c/coreboot/+/46275/3/src/cpu/intel/common/commo… PS3, Line 279: msr_unset_and_set(MSR_FEATURE_CONFIG, 0, AESNI_LOCK); I'm not 100% sure about this. Sometimes registers that are already locked take a wrmsr() as a serious offense. So why drop the check? I'd have to ask to test it on all affected platforms. -- To view, visit https://review.coreboot.org/c/coreboot/+/46275 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib2cda433bbec0192277839c02a1862b8f41340cb Gerrit-Change-Number: 46275 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Wed, 14 Oct 2020 18:52:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: include/cpu/x86: introduce new helper for (un)setting MSRs
by Nico Huber (Code Review) 14 Oct '20

14 Oct '20

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46354 ) Change subject: include/cpu/x86: introduce new helper for (un)setting MSRs ...................................................................... Patch Set 3: Code-Review+2 (5 comments) Thanks! https://review.coreboot.org/c/coreboot/+/46354/3//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/46354/3//COMMIT_MSG@10 PS3, Line 10: MRS MSR https://review.coreboot.org/c/coreboot/+/46354/3//COMMIT_MSG@12 PS3, Line 12: CB:42134 nit, the commit hash is easier to look up and set in stone. I use `git log -1 --format='%h (%s)'` usually. If you say 'commit' before it, it's also a link in Gerrit iirc. https://review.coreboot.org/c/coreboot/+/46354/3/src/cpu/intel/haswell/fina… File src/cpu/intel/haswell/finalize.c: https://review.coreboot.org/c/coreboot/+/46354/3/src/cpu/intel/haswell/fina… PS3, Line 10: (1 << 0) nit, the comma has highest precedence anyway (no need for parantheses around arguments) https://review.coreboot.org/c/coreboot/+/46354/3/src/include/cpu/x86/msr.h File src/include/cpu/x86/msr.h: https://review.coreboot.org/c/coreboot/+/46354/3/src/include/cpu/x86/msr.h@… PS3, Line 319: } We could also add an msr_set(reg, set) shorthand for the `unset == 0` case. https://review.coreboot.org/c/coreboot/+/46354/3/src/soc/intel/common/block… File src/soc/intel/common/block/include/intelblocks/msr.h: https://review.coreboot.org/c/coreboot/+/46354/3/src/soc/intel/common/block… PS3, Line 59: _BIT nit, I'd drop the _BIT suffix -- To view, visit https://review.coreboot.org/c/coreboot/+/46354 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idfe9b66e7cfe78ec295a44a2a193f530349f7689 Gerrit-Change-Number: 46354 Gerrit-PatchSet: 3 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 14 Oct 2020 18:48:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: libpayload/storage/ahci: Read device class from pcidev struct
by Felix Singer (Code Review) 14 Oct '20

14 Oct '20

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46424 ) Change subject: libpayload/storage/ahci: Read device class from pcidev struct ...................................................................... libpayload/storage/ahci: Read device class from pcidev struct Read device class from pcidev struct Change-Id: I2d5673c631d978d8ebd0c4a90962ab9cccaf40a2 Signed-off-by: Felix Singer <felixsinger(a)posteo.net> --- M payloads/libpayload/drivers/storage/ahci.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/46424/1 diff --git a/payloads/libpayload/drivers/storage/ahci.c b/payloads/libpayload/drivers/storage/ahci.c index e54bfbf..be54686 100644 --- a/payloads/libpayload/drivers/storage/ahci.c +++ b/payloads/libpayload/drivers/storage/ahci.c @@ -232,9 +232,9 @@ { int i; - const u16 class = pci_read_config16(dev, 0xa); - if (class != 0x0106) + if (dev->device_class != PCI_CLASS_STORAGE_AHCI) return; + const u16 vendor = pci_read_config16(dev, 0x00); const u16 device = pci_read_config16(dev, 0x02); -- To view, visit https://review.coreboot.org/c/coreboot/+/46424 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d5673c631d978d8ebd0c4a90962ab9cccaf40a2 Gerrit-Change-Number: 46424 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: libpayload/libpci: Return pointer to allocated memory directly
by Felix Singer (Code Review) 14 Oct '20

14 Oct '20

Hello build bot (Jenkins), Angel Pons, Michael Niewöhner, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/46345 to look at the new patch set (#3). Change subject: libpayload/libpci: Return pointer to allocated memory directly ...................................................................... libpayload/libpci: Return pointer to allocated memory directly Change-Id: Ib2ee8dbfaabbf7a824b4fd75ad7c779393af2900 Signed-off-by: Felix Singer <felix.singer(a)secunet.com> --- M payloads/libpayload/libpci/libpci.c 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/46345/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/46345 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib2ee8dbfaabbf7a824b4fd75ad7c779393af2900 Gerrit-Change-Number: 46345 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: libpayload/storage/ahci: Read device class from pcidev struct
by Felix Singer (Code Review) 14 Oct '20

14 Oct '20

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46423 ) Change subject: libpayload/storage/ahci: Read device class from pcidev struct ...................................................................... libpayload/storage/ahci: Read device class from pcidev struct Read device class from pcidev struct Change-Id: I2d5673c631d978d8ebd0c4a90962ab9cccaf40a2 Signed-off-by: Felix Singer <felixsinger(a)posteo.net> --- M payloads/libpayload/drivers/storage/ahci.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/46423/1 diff --git a/payloads/libpayload/drivers/storage/ahci.c b/payloads/libpayload/drivers/storage/ahci.c index e54bfbf..be54686 100644 --- a/payloads/libpayload/drivers/storage/ahci.c +++ b/payloads/libpayload/drivers/storage/ahci.c @@ -232,9 +232,9 @@ { int i; - const u16 class = pci_read_config16(dev, 0xa); - if (class != 0x0106) + if (dev->device_class != PCI_CLASS_STORAGE_AHCI) return; + const u16 vendor = pci_read_config16(dev, 0x00); const u16 device = pci_read_config16(dev, 0x02); -- To view, visit https://review.coreboot.org/c/coreboot/+/46423 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2d5673c631d978d8ebd0c4a90962ab9cccaf40a2 Gerrit-Change-Number: 46423 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: libpayload/libpci: Return pointer allocated memory directly
by Felix Singer (Code Review) 14 Oct '20

14 Oct '20

Hello build bot (Jenkins), Angel Pons, Michael Niewöhner, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/46345 to look at the new patch set (#2). Change subject: libpayload/libpci: Return pointer allocated memory directly ...................................................................... libpayload/libpci: Return pointer allocated memory directly Change-Id: Ib2ee8dbfaabbf7a824b4fd75ad7c779393af2900 Signed-off-by: Felix Singer <felix.singer(a)secunet.com> --- M payloads/libpayload/libpci/libpci.c 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/46345/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/46345 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib2ee8dbfaabbf7a824b4fd75ad7c779393af2900 Gerrit-Change-Number: 46345 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: nb/intel/haswell: Account for DPR region in memory map
by Angel Pons (Code Review) 14 Oct '20

14 Oct '20

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45712 ) Change subject: nb/intel/haswell: Account for DPR region in memory map ...................................................................... Patch Set 12: (1 comment) https://review.coreboot.org/c/coreboot/+/45712/10/src/northbridge/intel/has… File src/northbridge/intel/haswell/northbridge.c: https://review.coreboot.org/c/coreboot/+/45712/10/src/northbridge/intel/has… PS10, Line 339: resource->base = (dpr.top - dpr.size) << 10; : resource->size = (dpr.size << 10); > this needs to be in bytes Done -- To view, visit https://review.coreboot.org/c/coreboot/+/45712 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11f23fd43188f987e35fd61f52587e567496cd78 Gerrit-Change-Number: 45712 Gerrit-PatchSet: 12 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 14 Oct 2020 18:11:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit October 2020