coreboot November 2015

coreboot@coreboot.org

38 participants
97 discussions

Re: [coreboot] [RFC] Time until patches should be submitted (was: Document for review: coreboot Gerrit Etiquette and Guidelines)
by Martin Roth 04 Nov '15

04 Nov '15

As others have stated, my concern is the tradeoff between long and short times. Too short a time, and people don't getting the chance to review patches. Too long a time and it delays development, frustrates developers, and requires rebasing the patch, maybe requiring MORE review. Alternate thoughts or proposals: - "Note that some contributors prefer to work on coreboot during business hours, so consider leaving patches up longer than 24 hours during weekends and holidays" This of course presents other issues of dealing with holidays in various countries, time zones, etc. - Trivial patches (Whitespace, spelling, and the like) can be submitted with no delay. Non-trivial commits touching fewer than 20 lines of code should be submitted no sooner than 24 hours after the last major change. Changes larger than that 20 lines should wait at least 48? 72? hours after the last major change. Paul also mentioned that a method of getting non-trivial patches in quickly should be addressed as well. This should not be a regular occurrence, but sometimes we need or desire to push a patch quickly. He suggested 2 +2s and a statement to the mailing list. To me, this defeats the purpose of quick. Yesterday tpearson requested on IRC that we get a patch in quickly. It was, in my opinion significant, although just a single line change, which made it easy to understand and review. We got 3 +2s on it in just a few minutes. I thought that was a good barrier and would like to propose that as the rule: If you can get 3 +2s, knowing that it's going to be submitted immediately, it can be submitted. If this gets abused, it can be reviewed and changed at some point in the future. Martin On Tue, Nov 3, 2015 at 2:48 PM, David Hendricks <dhendrix(a)google.com> wrote: > > > On Tue, Nov 3, 2015 at 1:36 PM, Paul Menzel > <paulepanter(a)users.sourceforge.net> wrote: >> >> Dear coreboot folks, >> >> >> Am Donnerstag, den 29.10.2015, 12:55 -0600 schrieb Martin Roth: >> > As the community has grown, so has the need to formalize some of the >> > guidelines that the community lives by. When the community was small, >> > it was easy to communicate these things just from one person to >> > another. >> >> first of, big thanks go to Martin for writing up the draft! >> >> […] >> >> > Expectations contributors should have: >> > -------------------------------------- >> > * Don't expect that people will review your patch unless you ask them >> > to. Adding other people as reviewers is the easiest way. Asking for >> > reviews for individual patches in the IRC channel, or by sending a >> > direct request to an individual through your favorite messenger is >> > usually the best way to get a patch reviewed quickly. >> > * Don't expect that your patch will be submitted immediately after >> > getting a +2. As stated previously, non-trivial patches should wait >> > at least 24 hours before being submitted. >> >> to get some context, at the coreboot conference in Bonn some people >> asked for longer review time to not wake up the next morning seeing >> something changed. >> >> Even then, especially for non-payed developers, I think it’s hard to >> stay up to date, so the question is, if the time is long enough. On IRC >> somebody even mentioned, that patches should stay up for review for *a >> week* before getting merged, so there is enough time people can notice >> this. >> >> To not complicate rules, it probably would be easiest to just ask >> around if people are alright with 24 hours. Especially, when people >> working on the code get added as reviewers, this should be alright. >> >> On the other hand, more complicated rules could be drafted. The >> following rules just deal with the time issue. I am assuming, that +2 >> has been given before and the appropriate announcements are made. >> >> 1. Commits just touching a board can be submitted after 24 hours. >> 2. Commits touch more boards, should stay up for review for a week. >> They can be submitted earlier if an announcement to the list about the >> urgency has been sent and at least two people have given +2. > > > There's a catch-22 here: The kinds of patches that could benefit from >24 > hours in limbo also tend to be the disruptive kinds that may require > additional rebasing or changes should they remain in code review for too > long. A lot can happen in a week and disruptive patches bitrot faster than > normal ones. > > 24 hours should be considered a minimum, but I'd say "preferably a few days > if possible." A >24hr grace period can be agreed upon by reviewers and the > author depending on the complexity to mitigate bitrot. > > -- > David Hendricks (dhendrix) > Systems Software Engineer, Google Inc. > > -- > coreboot mailing list: coreboot(a)coreboot.org > http://www.coreboot.org/mailman/listinfo/coreboot

1 0

Dell Dimension 8300 reboots when grub2 cbfs module is loaded
by Andrei Borzenkov 04 Nov '15

04 Nov '15

I was debugging problem reported by user on Dell Dimension 8300 - it rebooted when doing "ls -l". It turned out, the problem was triggered by loading cbfs which probed for header. System has 2GB memory, and attempt to read from address 0xffffffff caused instant reboot. 0xffffffff was returned by read from non-existing address 0xfffffffc. The proof of concept patch below avoids it, but I wonder what the proper fix is. diff --git a/grub-core/fs/cbfs.c b/grub-core/fs/cbfs.c index a34eb88..a5a2fde 100644 --- a/grub-core/fs/cbfs.c +++ b/grub-core/fs/cbfs.c @@ -344,8 +344,9 @@ init_cbfsdisk (void) ptr = *(grub_uint32_t *) 0xfffffffc; head = (struct cbfs_header *) (grub_addr_t) ptr; + grub_dprintf ("cbfs", "head=%p\n", head); - if (!validate_head (head)) + if (0xffffffff - ptr < sizeof (*head) || !validate_head (head)) return; cbfsdisk_size = ALIGN_UP (grub_be_to_cpu32 (head->romsize),

4 4

sgabios ans grub2 payload (without SeaBIOS)
by maxime de Roucy 04 Nov '15

04 Nov '15

Hello, I would like to know if it's possible to load and use sgabios with grub2 as coreboot payload (no SeaBIOS in the rom). I didn't test it yet but maybe you can tel me if it's possible or not. And even better : how to do it. :) -- Thank you in advance Maxime de Roucy

1 0

[RFC] Time until patches should be submitted (was: Document for review: coreboot Gerrit Etiquette and Guidelines)
by Paul Menzel 03 Nov '15

03 Nov '15

Dear coreboot folks, Am Donnerstag, den 29.10.2015, 12:55 -0600 schrieb Martin Roth: > As the community has grown, so has the need to formalize some of the > guidelines that the community lives by. When the community was small, > it was easy to communicate these things just from one person to > another. first of, big thanks go to Martin for writing up the draft! […] > Expectations contributors should have: > -------------------------------------- > * Don't expect that people will review your patch unless you ask them > to. Adding other people as reviewers is the easiest way. Asking for > reviews for individual patches in the IRC channel, or by sending a > direct request to an individual through your favorite messenger is > usually the best way to get a patch reviewed quickly. > * Don't expect that your patch will be submitted immediately after > getting a +2. As stated previously, non-trivial patches should wait > at least 24 hours before being submitted. to get some context, at the coreboot conference in Bonn some people asked for longer review time to not wake up the next morning seeing something changed. Even then, especially for non-payed developers, I think it’s hard to stay up to date, so the question is, if the time is long enough. On IRC somebody even mentioned, that patches should stay up for review for *a week* before getting merged, so there is enough time people can notice this. To not complicate rules, it probably would be easiest to just ask around if people are alright with 24 hours. Especially, when people working on the code get added as reviewers, this should be alright. On the other hand, more complicated rules could be drafted. The following rules just deal with the time issue. I am assuming, that +2 has been given before and the appropriate announcements are made. 1. Commits just touching a board can be submitted after 24 hours. 2. Commits touch more boards, should stay up for review for a week. They can be submitted earlier if an announcement to the list about the urgency has been sent and at least two people have given +2. […] Thanks, Paul >

3 2

[ANN] Sun Ultra 40 M2 board port pushed for review
by Paul Menzel 03 Nov '15

03 Nov '15

Dear coreboot folks, Jonathan Kollasch pushed the port of Sun Ultra 40 M2 for review. Help reviewing that AMD board is much appreciated. The “base“ change set is #12304 [1]. Thanks, Paul [1] http://review.coreboot.org/12304

1 0

Re: [coreboot] Dell Dimension 8300 reboots when grub2 cbfs module is loaded
by Vladimir 'phcoder' Serbinenko 03 Nov '15

03 Nov '15

Le 3 nov. 2015 6:46 PM, "Aaron Durbin" <adurbin(a)google.com> a écrit : > > On Tue, Nov 3, 2015 at 10:28 AM, Vladimir 'phcoder' Serbinenko > <phcoder(a)gmail.com> wrote: > > The code itself looks good but I'd like more details. Reading 0xffffffff > > shouldn't cause reboot. Why does it? > > It's probably implementation defined reading a multi-byte object from > 4GiB-1. Does it wrap? Blow up the machine? Machine check? Transaction > never gets terminated? > It would be interesting to find out. Since it's P4, it may be related to PAE but paging is disabled when GRUB is active However it shouldn't hold this patch. Andrei: go ahead, just please add reference to machine and cpu in the comment and the fact that we have little idea what's going on. > > > > Le 1 nov. 2015 3:53 PM, "Andrei Borzenkov" <arvidjaar(a)gmail.com> a écrit : > >> > >> I was debugging problem reported by user on Dell Dimension 8300 - it > >> rebooted when doing "ls -l". It turned out, the problem was triggered by > >> loading cbfs which probed for header. System has 2GB memory, and attempt to > >> read from address 0xffffffff caused instant reboot. 0xffffffff was returned > >> by read from non-existing address 0xfffffffc. > >> > >> The proof of concept patch below avoids it, but I wonder what the proper > >> fix is. > >> > >> diff --git a/grub-core/fs/cbfs.c b/grub-core/fs/cbfs.c > >> index a34eb88..a5a2fde 100644 > >> --- a/grub-core/fs/cbfs.c > >> +++ b/grub-core/fs/cbfs.c > >> @@ -344,8 +344,9 @@ init_cbfsdisk (void) > >> > >> ptr = *(grub_uint32_t *) 0xfffffffc; > >> head = (struct cbfs_header *) (grub_addr_t) ptr; > >> + grub_dprintf ("cbfs", "head=%p\n", head); > >> > >> - if (!validate_head (head)) > >> + if (0xffffffff - ptr < sizeof (*head) || !validate_head (head)) > >> return; > >> > >> cbfsdisk_size = ALIGN_UP (grub_be_to_cpu32 (head->romsize), > >> > >> > >> _______________________________________________ > >> Grub-devel mailing list > >> Grub-devel(a)gnu.org > >> https://lists.gnu.org/mailman/listinfo/grub-devel > > > > > > -- > > coreboot mailing list: coreboot(a)coreboot.org > > http://www.coreboot.org/mailman/listinfo/coreboot

1 0

Re: [coreboot] Dell Dimension 8300 reboots when grub2 cbfs module is loaded
by Aaron Durbin 03 Nov '15

03 Nov '15

On Tue, Nov 3, 2015 at 10:28 AM, Vladimir 'phcoder' Serbinenko <phcoder(a)gmail.com> wrote: > The code itself looks good but I'd like more details. Reading 0xffffffff > shouldn't cause reboot. Why does it? It's probably implementation defined reading a multi-byte object from 4GiB-1. Does it wrap? Blow up the machine? Machine check? Transaction never gets terminated? > > Le 1 nov. 2015 3:53 PM, "Andrei Borzenkov" <arvidjaar(a)gmail.com> a écrit : >> >> I was debugging problem reported by user on Dell Dimension 8300 - it >> rebooted when doing "ls -l". It turned out, the problem was triggered by >> loading cbfs which probed for header. System has 2GB memory, and attempt to >> read from address 0xffffffff caused instant reboot. 0xffffffff was returned >> by read from non-existing address 0xfffffffc. >> >> The proof of concept patch below avoids it, but I wonder what the proper >> fix is. >> >> diff --git a/grub-core/fs/cbfs.c b/grub-core/fs/cbfs.c >> index a34eb88..a5a2fde 100644 >> --- a/grub-core/fs/cbfs.c >> +++ b/grub-core/fs/cbfs.c >> @@ -344,8 +344,9 @@ init_cbfsdisk (void) >> >> ptr = *(grub_uint32_t *) 0xfffffffc; >> head = (struct cbfs_header *) (grub_addr_t) ptr; >> + grub_dprintf ("cbfs", "head=%p\n", head); >> >> - if (!validate_head (head)) >> + if (0xffffffff - ptr < sizeof (*head) || !validate_head (head)) >> return; >> >> cbfsdisk_size = ALIGN_UP (grub_be_to_cpu32 (head->romsize), >> >> >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel(a)gnu.org >> https://lists.gnu.org/mailman/listinfo/grub-devel > > > -- > coreboot mailing list: coreboot(a)coreboot.org > http://www.coreboot.org/mailman/listinfo/coreboot

1 0

Re: [coreboot] Dell Dimension 8300 reboots when grub2 cbfs module is loaded
by Andrei Borzenkov 03 Nov '15

03 Nov '15

03.11.2015 19:28, Vladimir 'phcoder' Serbinenko пишет: > The code itself looks good but I'd like more details. Reading 0xffffffff > shouldn't cause reboot. Why does it? That I do not know nor do I have access to system in question myself. I sent user patch that modified validate_header to do each comparison as individual statement and did line by line debug print (fortunately it was possible to connect serial port and capture output) and the last line printed was immediately before the very first head->magic == grub_cpu_to_be32_compile_time (CBFS_HEADER_MAGIC I suppose reading *one* byte from 0xffffffff should not cause issues but here we are reading 4 bytes which are beyond 0xffffffff. Who knows what memory controller in this system does in this case. > Le 1 nov. 2015 3:53 PM, "Andrei Borzenkov" <arvidjaar(a)gmail.com> a écrit : > >> I was debugging problem reported by user on Dell Dimension 8300 - it >> rebooted when doing "ls -l". It turned out, the problem was triggered by >> loading cbfs which probed for header. System has 2GB memory, and attempt to >> read from address 0xffffffff caused instant reboot. 0xffffffff was returned >> by read from non-existing address 0xfffffffc. >> >> The proof of concept patch below avoids it, but I wonder what the proper >> fix is. >> >> diff --git a/grub-core/fs/cbfs.c b/grub-core/fs/cbfs.c >> index a34eb88..a5a2fde 100644 >> --- a/grub-core/fs/cbfs.c >> +++ b/grub-core/fs/cbfs.c >> @@ -344,8 +344,9 @@ init_cbfsdisk (void) >> >> ptr = *(grub_uint32_t *) 0xfffffffc; >> head = (struct cbfs_header *) (grub_addr_t) ptr; >> + grub_dprintf ("cbfs", "head=%p\n", head); >> >> - if (!validate_head (head)) >> + if (0xffffffff - ptr < sizeof (*head) || !validate_head (head)) >> return; >> >> cbfsdisk_size = ALIGN_UP (grub_be_to_cpu32 (head->romsize), >> >> >> _______________________________________________ >> Grub-devel mailing list >> Grub-devel(a)gnu.org >> https://lists.gnu.org/mailman/listinfo/grub-devel >> > > > > _______________________________________________ > Grub-devel mailing list > Grub-devel(a)gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >

1 0

Re: [coreboot] A case for branching AGESA
by Alex G. 03 Nov '15

03 Nov '15

On 11/01/2015 12:49 AM, Patrick Georgi wrote: > Just a note: the only reason why current Intel fares better I didn't say Intel fares better. FSP is on my (rather long) hitlist, but that's not within the scope of this discussion. >> 7 min vs 20 min on empty ccache, and 2 min vs 6 min on primed ccache. >> Those are speedups of 2x to 3x. > Thank you for doing the measurements. Thank you for creating the 'what-jenkins-does' make target. >> Patch trains from google and other contributions to non-AGESA code gain a 2x to 3x >> speedup in server time, while users of AGESA can continue to contribute >> and work on the codebase. > ... and diverge... And that's expected. Convergence is a dream. AGESA boards use BuildOpts for configuration, and not much Kconfig/devicetree.cb, versus most other boards in the tree. Except for VX900 and sandybridge, every chipset implements its own SPD parsing routines. I can go on and on. Even within one branch, significant divergence exists, so non-divergence is a moot point. Alex

6 5

Re: [coreboot] Coreboot hackaton 2016 (proposal in Paris)
by Stefan Reinauer 03 Nov '15

03 Nov '15

* David Hendricks <dhendrix(a)google.com> [151102 21:00]: > Piggybacking on other conferences can certainly help overseas travellers. Are > there other major conferences later in the year? > > Hosting after FOSDEM 2016 actually seems ideal and it will be in Brussels (<2 > hours away by train, or <1 hour by flight). It is a bit soon after the Bonn > conference, though but maybe that won't matter? This isn't intended to be a big > formal event anyway. If you get enough community members in the Doodle poll I > think you should go for it :-) I also want to double stress the importance of getting an official invitation out early is crucial. People from overseas might need to go through a visa application process to attend, which might take several months (and is not seldomly tied to an official invitation) Stefan

2 1

← Newer
1
2
3
4
5
6
7
8
9
10
Older →

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

2004

2003

2002

coreboot November 2015