coreboot October 2015

coreboot@coreboot.org

44 participants
137 discussions

Re: [coreboot] coreboot binary policy
by David Hendricks 30 Oct '15

30 Oct '15

On Fri, Oct 30, 2015 at 9:03 AM, Marc Jones <marcj303(a)gmail.com> wrote: > > On Fri, Oct 30, 2015 at 9:44 AM Alex G. <mr.nuke.me(a)gmail.com> wrote: > >> On 10/29/2015 09:48 AM, Marc Jones wrote: >> > Hello coreboot, >> >> Hi Marc >> >> > Please limit comments to specific items in this version. If you have >> > additions for the next version (if needed), the draft document is open >> > for comment. >> > >> > >> https://docs.google.com/document/d/1wMdDUAZR2Z9V7hcs3IhIOqw6sYQxb3vPEmbITTC… >> >> That looks pretty good. I think you've done a great job of clarifying >> the requirements of ISA vs non-ISA blobs compared to the last version. >> I've made some comments on it to ask for clarification about the >> versioning requirements. >> >> While not necessarily specific to this version, are we still considering >> forbidding "no-reverse engineering" and "no-modification" clauses for >> blobs? >> >> > Thanks, I think it is all open for discussion and could go in the next > version. It might be a good idea, but that might be too limiting and we > would have to remove all blobs and they would be hosted somewhere else, > which defeates the utility of the blobs dir. We would like intel to push to > blobs/ but I think that would be a huge blocker for them. > +1. It's tough enough for us to get rid of a few lines of GPL boilerplate. Getting companies to significantly change their boilerplate licensing for blobs will be a blocker. Just treat them as we always have. -- David Hendricks (dhendrix) Systems Software Engineer, Google Inc.

1 0

Re: [coreboot] coreboot binary policy
by ron minnich 30 Oct '15

30 Oct '15

On Fri, Oct 30, 2015 at 10:59 AM Alex Gagniuc <mr.nuke.me(a)gmail.com> wrote: > > I think historically, it has been assumed that everything in blobs is > open up for RE and modification. > History? What? Only if your timeline is really short. We first started doing the blobs support in 2001 for graphics. We NEVER held it that we had a right to RE and modify nvidia blobs. We certainly never RE'ed the firmware we were trying to replace. ron

1 0

Re: [coreboot] coreboot binary policy
by Alex Gagniuc 30 Oct '15

30 Oct '15

On Fri, Oct 30, 2015 at 9:03 AM, Marc Jones <marcj303(a)gmail.com> wrote: > It might be a good idea, but that might be too limiting I think historically, it has been assumed that everything in blobs is open up for RE and modification. There are plenty of examples of people reverse-engineering stuff in blobs, and also modifying the blob itself [1]. First and foremost, we should protect the project, and with that, our contributors. We can have a process where we might grant exceptions from these (proposed) rules to certain non-ISA blobs. For example, we might exempt microcode on the basis that (we believe) It's impractical to RE, and keeping that avenue open is not of any particular value. > and we would have to remove all blobs and they would be hosted somewhere else We can grandfather in existing blobs, or we can have a process where we keep them for a while (a year?) while we try to work out appropriate licensing terms with the power-that-be of said blob. [1] http://review.coreboot.org/4605 Alex

1 0

Re: [coreboot] PAT memory type issues and cbmem
by Ben Gardner 30 Oct '15

30 Oct '15

On Fri, Oct 30, 2015 at 11:52 AM, Aaron Durbin <adurbin(a)google.com> wrote: > On Fri, Oct 30, 2015 at 9:19 AM, Ben Gardner <gardner.ben(a)gmail.com> wrote: >> cbmem attempts to map 1 MB of memory to read the CB tables. >> This is failing on my board due to the PAT configuration under Linux. >> If I boot Linux with "nopat", the issue goes away. >> >> I am using the upstream coreboot and SeaBIOS with a custom board based >> on the bayleybay_fsp/fsp_baytrail and Linux 4.2. >> >> ~ # ./cbmem -V -l >> Looking for coreboot table at 0 1048576 bytes. >> Mapping 1MB of physical memory at 0x0 (requested 0x0). >> Found! >> coreboot table entry 0x11 >> Found forwarding entry. >> Unmapping 1MB of virtual memory at 0x7fee18224000. >> Looking for coreboot table at 7ad9f000 1048576 bytes. >> Mapping 1MB of physical memory at 0x7ad9f000 (requested 0x7ad9f000). >> ... failed. Mapping 1052671B of physical memory at 0x7ad9e000. >> Failed to mmap /dev/mem: Resource temporarily unavailable >> ~ # dmesg | tail -n 4 >> [ 221.700621] x86/PAT: cbmem.orig:1058 conflicting memory types >> 7ad9f000-7ae9f000 uncached-minus<->write-back 7adb8000-7adbc000 >> [ 221.705419] x86/PAT: reserve_memtype failed [mem >> 0x7ad9f000-0x7ae9efff], track write-back, req write-back >> [ 221.710435] x86/PAT: cbmem.orig:1058 conflicting memory types >> 7ad9e000-7ae9f000 uncached-minus<->write-back 7adb8000-7adbc000 >> [ 221.715501] x86/PAT: reserve_memtype failed [mem >> 0x7ad9e000-0x7ae9efff], track write-back, req write-back >> ~ # cat /sys/kernel/debug/x86/pat_memtype_list >> PAT memtype list: >> uncached-minus @ 0x7ada7000-0x7ada8000 >> write-back @ 0x7adb8000-0x7adbc000 >> write-back @ 0x7adbb000-0x7adbd000 >> write-back @ 0x7addd000-0x7adde000 >> uncached-minus @ 0x80600000-0x80601000 >> uncached-minus @ 0x80601000-0x80602000 >> uncached-minus @ 0x80602000-0x80603000 >> write-combining @ 0xc0000000-0xd0000000 >> ... snip ... >> >> From the Linux kernel log: >> MTRR variable ranges enabled: >> 0 base 0FF800000 mask FFF800000 write-protect >> 1 base 000000000 mask F80000000 write-back >> 2 base 07B000000 mask FFF000000 uncachable >> 3 base 07C000000 mask FFC000000 uncachable >> >> I'm not sure how to read MTRR ranges, but it looks like the cbtable >> falls in the MTRR 1 with type write-back. >> >> From the SeaBIOS log: >> e820 map has 15 items: >> 0: 0000000000000000 - 000000000009fc00 = 1 RAM >> 1: 000000000009fc00 - 00000000000a0000 = 2 RESERVED >> 2: 00000000000f0000 - 0000000000100000 = 2 RESERVED >> 3: 0000000000100000 - 000000007ad8e000 = 1 RAM >> 4: 000000007ad8e000 - 0000000080000000 = 2 RESERVED >> 5: 00000000e0000000 - 00000000f0000000 = 2 RESERVED >> ... snip ... >> >> The issue appears to be that the coreboot table memory is marked as >> uncached-minus and then Linux (?) is marking the ACPI tables as >> write-back. >> The 1 MB size is overlapping the ACPI tables. >> That makes mmap attempts of the whole coreboot table to fail due to >> the mismatched memory types. >> >> >> I have an older version of coreboot from Sage that does not have this issue. >> The difference is that the first uncached-minus entry is not present. >> >> On the Sage-based (4.0) coreboot image: >> ~ # cat /sys/kernel/debug/x86/pat_memtype_list >> PAT memtype list: >> write-back @ 0x7add1000-0x7add2000 >> write-back @ 0x7add4000-0x7add9000 >> write-back @ 0x7adda000-0x7addc000 >> uncached-minus @ 0x7ade2000-0x7ade3000 >> write-combining @ 0xc0000000-0xd0000000 >> ... snip ... >> >> So, I'm trying to figure out a few things: >> >> 1) Where does Linux get the PAT table from? The e820 map? > > It's generated a runtime based on the requests the kernel makes for > specific mapping. >> >> 2) What piece of code sets the coreboot table as uncached-minus? > > The kernel does it. e820 is sort of a poor indicator for memory type. > It merges the concept of address space usage and type but it doesn't > do a very good job at it. > >> >> 3) Why is that range set as uncached-minus? Would write-back work? > > Please see this thread: > http://www.coreboot.org/pipermail/coreboot/2015-September/080381.html > > The actual issue stems from > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drive… > which maintains a persistent mapping of smbios tables. It uses > dmi_remap() which is '#define dmi_remap ioremap' which is where the > uncached-minus PAT entry comes from. It should be using the same > mechanism as the ACPI table mappings which uses ioremap_cache(). > > Hope that helps. > > -Aaron It does help. I can confirm that changing that #define in arch/x86/include/asm/dmi.h fixes the issue: #define dmi_remap ioremap_cache ~ # cat /sys/kernel/debug/x86/pat_memtype_list PAT memtype list: write-back @ 0x7ada7000-0x7ada8000 write-back @ 0x7adb8000-0x7adbc000 write-back @ 0x7adbb000-0x7adbd000 write-back @ 0x7addc000-0x7addd000 write-combining @ 0xc0000000-0xd0000000 ... snip ... Thanks, Ben

1 0

Re: [coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines
by ron minnich 30 Oct '15

30 Oct '15

Possibly an appendix defining coreboot leadership would help. On Fri, Oct 30, 2015 at 9:51 AM Martin Roth <gaumless(a)gmail.com> wrote: > Hi Alex, > Thanks for voicing your concerns. I do think that most of the > issues you bring up are handled, or aren't as big issues you make them > out to be. As always, I could be wrong, so I'd again invite others to > give their opinions. I've tried to address your issues below. > > > ... the "maintainer" is given almost absolute veto power > > in all matters relating to that code. > ... > > There are also no responsibilities defined for a "maintainer", despite > > such person being given near-inter-stellar of power. > I did try to define maintainers in the document: > * Try to coordinate with platform maintainers when making changes to > platforms. The platform maintainers are the users who initially pushed > the > code for that platform, as well as users who have made significant > changes > to a platform. To find out who maintains a piece of code, please use > util/scripts/maintainers.go or refer to the original author of the code > in > git log. > I'm not certain how "Try to coordinate" is being read as "almost > absolute veto power" or "near-inter-stellar of power". > > > Concerns from external reviewers often get > > ignored or brushed off as "don't care". > This is absolutely addressed by these three guidelines: > * Respond to anyone who has taken the time to review your patches. > * If there have been comments or discussion on a patch, verify that the > comments have been addressed before giving a +2. > * If there is still ongoing discussion to a patch, try to wait for a > conclusion to the discussion before submitting it to the tree. > > > I've seen plenty of cases where patches have been -2'd > > by W(a)g.com until other @g.com patches were merged, only for the -2 to be > > removed once it was on the contributor to rebase his work. > I haven't seen this, but if you see a situation where you feel like > someone is being treated unfairly, please bring it to the attention of > the coreboot leadership or post something to the mailing list. > > > This then creates another significant issue: the bar for contributions > > from such teams is very low, while anyone else experiences a much higher > > bar. This has many times resulted in sub-optimal code being merged, and > > great contributions getting delayed and abandoned. > Again, I feel like if you comment on these patches, the rules above > should handle these issues. And again, if you feel like something is > unfair, bring attention to it. > > > ... Raptor Eng. was asking around $15000 to upstream ... > > All of a sudden, we've made coreboot a very expensive > > project to support. > Additionally, I specifically reached out to tpearson with these > guidelines to get his feedback before they were posted. I have tried > to address the concerns he brought up. If you (or anyone else) feel > like there's something specific we can add here that will help him or > future developers get code submitted, please recommend the addition. > > > Then there's talk about a vague "coreboot leadership"... > Patrick addressed this in the review - here's Stefan talking about > coreboot leadership: > http://blogs.coreboot.org/blog/2015/05/11/on-coreboot-leadership/ > > -- > coreboot mailing list: coreboot(a)coreboot.org > http://www.coreboot.org/mailman/listinfo/coreboot >

1 0

Re: [coreboot] PAT memory type issues and cbmem
by Aaron Durbin 30 Oct '15

30 Oct '15

On Fri, Oct 30, 2015 at 9:19 AM, Ben Gardner <gardner.ben(a)gmail.com> wrote: > cbmem attempts to map 1 MB of memory to read the CB tables. > This is failing on my board due to the PAT configuration under Linux. > If I boot Linux with "nopat", the issue goes away. > > I am using the upstream coreboot and SeaBIOS with a custom board based > on the bayleybay_fsp/fsp_baytrail and Linux 4.2. > > ~ # ./cbmem -V -l > Looking for coreboot table at 0 1048576 bytes. > Mapping 1MB of physical memory at 0x0 (requested 0x0). > Found! > coreboot table entry 0x11 > Found forwarding entry. > Unmapping 1MB of virtual memory at 0x7fee18224000. > Looking for coreboot table at 7ad9f000 1048576 bytes. > Mapping 1MB of physical memory at 0x7ad9f000 (requested 0x7ad9f000). > ... failed. Mapping 1052671B of physical memory at 0x7ad9e000. > Failed to mmap /dev/mem: Resource temporarily unavailable > ~ # dmesg | tail -n 4 > [ 221.700621] x86/PAT: cbmem.orig:1058 conflicting memory types > 7ad9f000-7ae9f000 uncached-minus<->write-back 7adb8000-7adbc000 > [ 221.705419] x86/PAT: reserve_memtype failed [mem > 0x7ad9f000-0x7ae9efff], track write-back, req write-back > [ 221.710435] x86/PAT: cbmem.orig:1058 conflicting memory types > 7ad9e000-7ae9f000 uncached-minus<->write-back 7adb8000-7adbc000 > [ 221.715501] x86/PAT: reserve_memtype failed [mem > 0x7ad9e000-0x7ae9efff], track write-back, req write-back > ~ # cat /sys/kernel/debug/x86/pat_memtype_list > PAT memtype list: > uncached-minus @ 0x7ada7000-0x7ada8000 > write-back @ 0x7adb8000-0x7adbc000 > write-back @ 0x7adbb000-0x7adbd000 > write-back @ 0x7addd000-0x7adde000 > uncached-minus @ 0x80600000-0x80601000 > uncached-minus @ 0x80601000-0x80602000 > uncached-minus @ 0x80602000-0x80603000 > write-combining @ 0xc0000000-0xd0000000 > ... snip ... > > From the Linux kernel log: > MTRR variable ranges enabled: > 0 base 0FF800000 mask FFF800000 write-protect > 1 base 000000000 mask F80000000 write-back > 2 base 07B000000 mask FFF000000 uncachable > 3 base 07C000000 mask FFC000000 uncachable > > I'm not sure how to read MTRR ranges, but it looks like the cbtable > falls in the MTRR 1 with type write-back. > > From the SeaBIOS log: > e820 map has 15 items: > 0: 0000000000000000 - 000000000009fc00 = 1 RAM > 1: 000000000009fc00 - 00000000000a0000 = 2 RESERVED > 2: 00000000000f0000 - 0000000000100000 = 2 RESERVED > 3: 0000000000100000 - 000000007ad8e000 = 1 RAM > 4: 000000007ad8e000 - 0000000080000000 = 2 RESERVED > 5: 00000000e0000000 - 00000000f0000000 = 2 RESERVED > ... snip ... > > The issue appears to be that the coreboot table memory is marked as > uncached-minus and then Linux (?) is marking the ACPI tables as > write-back. > The 1 MB size is overlapping the ACPI tables. > That makes mmap attempts of the whole coreboot table to fail due to > the mismatched memory types. > > > I have an older version of coreboot from Sage that does not have this issue. > The difference is that the first uncached-minus entry is not present. > > On the Sage-based (4.0) coreboot image: > ~ # cat /sys/kernel/debug/x86/pat_memtype_list > PAT memtype list: > write-back @ 0x7add1000-0x7add2000 > write-back @ 0x7add4000-0x7add9000 > write-back @ 0x7adda000-0x7addc000 > uncached-minus @ 0x7ade2000-0x7ade3000 > write-combining @ 0xc0000000-0xd0000000 > ... snip ... > > So, I'm trying to figure out a few things: > > 1) Where does Linux get the PAT table from? The e820 map? It's generated a runtime based on the requests the kernel makes for specific mapping. > > 2) What piece of code sets the coreboot table as uncached-minus? The kernel does it. e820 is sort of a poor indicator for memory type. It merges the concept of address space usage and type but it doesn't do a very good job at it. > > 3) Why is that range set as uncached-minus? Would write-back work? Please see this thread: http://www.coreboot.org/pipermail/coreboot/2015-September/080381.html The actual issue stems from http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drive… which maintains a persistent mapping of smbios tables. It uses dmi_remap() which is '#define dmi_remap ioremap' which is where the uncached-minus PAT entry comes from. It should be using the same mechanism as the ACPI table mappings which uses ioremap_cache(). Hope that helps. -Aaron

1 0

Re: [coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines
by Alex G. 30 Oct '15

30 Oct '15

Hi Martin, It seems that the first part of your document ("Summary", and "More detail") is written in direct response to the removal of FSP 1.0 a month or so ago. As such, it's very situation-centric, and fails to address some more fundamental issues. The concept of a "maintainer" is central to a large number of the definitions and guidelines posted, but that concept is very ill-defined. According to your definition of "maintainer", someone who touched that code, even with a simple refactoring patch, can now be construed to be a "maintainer". Then, the "maintainer" is given almost absolute veto power in all matters relating to that code. There are also no responsibilities defined for a "maintainer", despite such person being given near-inter-stellar of power. This only exacerbates some of the long-standing problems we've had in coreboot. One such problem is code parking, whereas a contributor merges a piece of code, but then doesn't do his due diligence in taking care of that code. If the code causes issues, or prevents other refactorings from happening, the original contributor disappears, and it's up to the reacaftoring contributor to fix that code as well. To that, you now add the requirement that the refactoring contribution leaves the broken platform in a functioning state. That places the entire burden on the new contributor, while the original submitter can come in at any time and say "veto". This creates a counter-constructive disproportion of powers and responsibilities. You want to make it such that both original submitter and new contributor have balanced amounts of power and responsibility over the piece of code in question. We might want to learn from what other projects have done here, most notably linux distributions. Take a look at Fedora's nonresponsive maintainer policy [1] as an example. Give "maintainers" responsibilities, not just plain stupid absolute power. We're not solving any issues otherwise. Another issue that's not addressed here is the problem of people in the same team submitting and merging patches. This has been a recurring theme with google contributions. X(a)g.com submits the patch, Y(a)g.com approves and merges it. Concerns from external reviewers often get ignored or brushed off as "don't care". @g.com patches that create merge conflicts for other contributors are sometimes rushed so @g.com doesn't have to rebase. I've seen plenty of cases where patches have been -2'd by W(a)g.com until other @g.com patches were merged, only for the -2 to be removed once it was on the contributor to rebase his work. This then creates another significant issue: the bar for contributions from such teams is very low, while anyone else experiences a much higher bar. This has many times resulted in sub-optimal code being merged, and great contributions getting delayed and abandoned. This is actually a huge issue that we're dealing with. I don't remember the exact numbers, but Raptor Eng. was asking around $15000 to upstream their fam15 work. That's because it's painfully difficult for anyone outside those teams to merge their contributions. Considering that there's one guy paying Raptor for all this work, one guy who believes in, and wants to support coreboot. We've cost him $15000 dollars (of his own money!). All of a sudden, we've made coreboot a very expensive project to support. Then there's talk about a vague "coreboot leadership" who can make arbitrary decisions about X and Y. There's no concise guideline really, but it does allow a mysterious group to make any decisions about contributions and contributors, and leave too much open to the whim of someone's personal thoughts or the goodness or badness of his day. I don't think that's acceptable. This needs more work. Alex [1] https://fedoraproject.org/wiki/Policy_for_nonresponsive_package_maintainers On 10/29/2015 11:55 AM, Martin Roth wrote: > 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. > > Now, with more people joining the community every day, it seems that > it's time to write some of these things down, allowing people to > understand our policies immediately instead of making them learn our > practices as they make mistakes. > > As it says in the document: The following rules are the requirements > for behavior in the coreboot codebase in gerrit. These have mainly > been unwritten rules up to this point, and should be familiar to most > users who have been active in coreboot for a period of time. Following > these rules will help reduce friction in the community. > > This has been posted to gerrit for review: > http://review.coreboot.org/#/c/12256/ > > Please post your thoughts and comments either as a reply to this email > or in gerrit. > > Thanks. > Martin > > > > coreboot Gerrit Etiquette and Guidelines > ======================================== > > The following rules are the requirements for behavior in the coreboot > codebase in gerrit. These have mainly been unwritten rules up to this > point, and should be familiar to most users who have been active in > coreboot for a period of time. Following these rules will help reduce > friction in the community. > > > Summary: > -------- > These are the expectations for committing, reviewing, and submitting > code into coreboot git and gerrit. While breaking individual rules may > not have immediate consequences, the coreboot leadership may act on > repeated or flagrant violations with or without notice. > > * Don't violate the licenses. > * Let non-trivial patches sit in a review state for at least 24 hours > before submission. > * Try to coordinate with platform maintainers when making changes to platforms. > * If you give a patch a -2, you are responsible for giving concrete > recommendations for what could be changed to resolve the issue the > patch addresses. > * Don't modify other people's patches without their consent. > * Be respectful to others when commenting. > * Each patch should be kept to one logical change. > * Don’t submit patches that you know will break other platforms. > > > More detail: > ------------ > * Don't violate the licenses. If you're submitting code that you > didn't write yourself, make sure the license is compatible with the > license of the project you're submitting the changes to. If you’re > submitting code that you wrote that might be owned by your employer, > make sure that your employer is aware and you are authorized to submit > the code. See the Developer's Certificate of Origin in the > Signed-off-by policy. > * Let non-trivial patches sit in a review state for at least 24 hours > before submission. Remember that coreboot developers are in timezones > all over the world, and everyone should have a chance to contribute. > Trivial patches would be things like whitespace changes or spelling > fixes. In general, small changes that don’t impact the final binary > output. > * Do not +2 patches that you authored or own, even for something as > trivial as whitespace fixes. When working on your own patches, it’s > easy to overlook something like accidentally updating file permissions > or git submodule commit IDs. Let someone else review the patch. > * Try to coordinate with platform maintainers when making changes to > platforms. The platform maintainers are the users who initially pushed > the code for that platform, as well as users who have made significant > changes to a platform. To find out who maintains a piece of code, > please use util/scripts/maintainers.go or refer to the original author > of the code in git log. > * If you give a patch a -2, you are responsible for giving concrete > recommendations for what could be changed to resolve the issue the > patch addresses. If you feel strongly that a patch should NEVER be > merged, you are responsible for defending your position and listening > to other points of view. Giving a -2 and walking away is not > acceptable, and may cause your -2 to be removed by the coreboot > leadership after a period of time. > * Don't modify other people's patches unless you are specifically > requested to do so by the owner of that patch. Not only is this > considered rude, but your changes could be lost unintentionally when > the owner pushes an update without realizing that you’ve changed > something. > * Be respectful to others when commenting on their patch. Comments > should be kept to the code, and should be kept in a polite tone. If > you feel attacked, resist the urge to attack back - this only > escalates the issue. > * Each patch should be kept to one logical change, which should be > described in the title of the patch. Unrelated changes should be split > out into separate patches. Fixing whitespace on a line you’re editing > is reasonable. Fixing whitespace around the code you’re working on > should be a separate ‘cleanup’ patch. Larger patches that touch > several areas are fine, so long as they are one logical change. Adding > new chips and doing code cleanup over wide areas are two examples of > this. > * Don’t submit code that you know will break other platforms. If your > patch affects code that is used by other platforms, it should be > compatible with those platforms. While it would be nice to update any > other platforms, you must at least provide a path that will allow > other platforms to continue working. > > > Recommendations for gerrit activity: > ------------------------------------ > These guidelines are less strict than the ones listed above. These are > more of the “good idea” variety. You are requested to follow the below > guidelines, but there will probably be no actual consequences if > they’re not followed. That said, following the recommendations below > will speed up review of your patches, and make the members of the > community do less work. > > * Test your patches before submitting them to gerrit. It's also > appreciated if you add a line to the commit message describing how the > patch was tested. This prevents people from having to ask whether and > how the patch was tested. An example of this sort of comment would be > ‘TEST=Built platform’ or ‘TEST=Built and booted platform’ > * Take advantage of the lint tools to make sure your patches don’t > contain trivial mistakes. By running ‘make gitconfig’, the lint-stable > tools are automatically put in place and will test your patches before > they are committed. As a violation of these tools will cause the > jenkins build test to fail, it’s to your advantage to test this before > pushing to gerrit. > * Don't submit patch trains longer than around 20 patches. Long patch > trains become unmanageable and tie up the build servers for long > periods of time. Rebasing a patch train over and over as you fix > earlier patches in the train can hide comments, and make people review > the code multiple times to see if anything has changed between > revisions. > * Run 'make what-jenkins-does' locally on patch trains before > submitting. This helps verify that the patch train won’t tie up the > jenkins builders for no reason if there are failing patches in the > train. > * Use a topic when pushing a train of patches. This groups the commits > together so people can easily see the connection at the top level of > gerrit. Topics can be set for individual patches in gerrit by going > into the patch and clicking on the icon next to the topic line. Topics > can also be set when you push the patches into gerrit. For example, to > push a set of commits with the the i915-kernel-x60 set, use the > command: > git push origin HEAD:refs/for/master/i915-kernel-x60 > * If one of your patches isn't ready to be merged, make sure it's > obvious that you don't feel it's ready for merge yet. This can be > something like giving it a -1 or -2, or marking in the commit message > that it’s not ready until X. The commit message can be updated easily > when it’s ready to be pushed. Examples of this are "WIP: title" or > "[NEEDS_TEST]: title". These can also be pushed as drafts as shown in > the next guideline. > * When pushing patches that are not for submission, these should be > marked as such. This can be done in the title ‘[DONOTSUBMIT]’, or can > be pushed as draft commits, so that only explicitly added reviewers > will see them. These sorts of patches are frequently posted as ideas > or RFCs for the community to look at. To push a draft, use the > command: > git push origin HEAD:refs/drafts/master > * Respond to anyone who has taken the time to review your patches, > even if it's just to say that you disagree. While it may seem annoying > to address a request to fix spelling or 'trivial' issues, it’s > generally easy to handle in gerrit’s built in editor. It's also > acceptable to add fixes for these sorts of comments to another patch, > but it's recommended that that patch be pushed to gerrit before the > initial patch gets submitted. > * Consider breaking up large individual patches into smaller patches > grouped by areas. This makes the patches easier to review, but > increases the number of patches. The way you want to handle this is a > personal decision, as long as each patch is still one logical change. > * If you have an interest in a particular area or mainboard, set > yourself up as a ‘maintainer’ of that area by adding yourself to the > MAINTAINERS file in the coreboot root directory. Eventually, this > should automatically add you as a reviewer when an area that you’re > listed as a maintainer is changed. > * Submit mainboards that you’re working on to the board-status repo. > This helps others and shows that these mainboards are currently being > maintained. At some point, boards that are not up to date in the > board-status repo will probably end up getting deleted from the > coreboot tree. > * Abandon patches that are no longer useful, or that you don’t intend > to keep working on. > * Bring attention to patches that you would like reviewed. Add > reviewers, or even just rebase it against the current codebase to > bring it to the top of the gerrit list. If you’re not sure who would > be a good reviewer, look in the MAINTAINERS file or git history of the > files that you’ve changed, and add those people. > * Familiarize yourself with the commit message guidelines, and try to > follow them. This will help to keep annoying requests to fix your > commit message to a minimum. > * If there have been comments or discussion on a patch, verify that > the comments have been addressed before giving a +2. If you feel that > a comment is invalid, please respond to that comment instead of just > ignoring it. > * Be conscientious when reviewing patches. If you give a patch a +2 > and it breaks things, you should feel as responsible as the owner of > the patch for fixing things, and could be called on by the community > to help fix any fallout of the patch. This means you shouldn’t +2 a > patch just because you trust the author of a patch - Make sure you > understand what the implications of a patch might be, or leave the > review to others. Partial reviews, reviewing code style, for example, > can be given a +1 instead of a +2. This also applies if you think the > patch looks good, but may not have the experience to know if there may > be unintended consequences. > * If there is still ongoing discussion to a patch, try to wait for a > conclusion to the discussion before submitting it to the tree. If you > feel that someone is beating a dead horse, maybe just state that and > give a time that the patch will be submitted. If no new objections > have come up instead of submitting the patch immediately and ending > the discussion by force. > * When working with patch trains, for minor requests it’s acceptable > to create a fix addressing a comment in another patch at the end of > the patch train. This minimizes rebases of the patch train while still > addressing the request. For major problems where the change doesn’t > work as intended or breaks other platforms, the change really needs to > go into the original patch. > > > 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. That said, if you feel that > your patch or series of patches has been sitting longer than needed, > you can ask for it to be submitted on IRC, or comment that it's ready > for submission in the patch. This will move it to the top of the list > where it's more likely to be noticed and acted upon. > * Reviews are about the code. It's easy to take it personally when > someone is criticising your code, but the whole idea is to get better > code into our codebase. Again, this also applies in the other > direction: review code, criticize code, but don’t make it personal. > > > Requests for clarification and suggestions for updates to these > guidelines should be sent to the coreboot mailing list at > <coreboot(a)coreboot.org>. >

3 2

PAT memory type issues and cbmem
by Ben Gardner 30 Oct '15

30 Oct '15

cbmem attempts to map 1 MB of memory to read the CB tables. This is failing on my board due to the PAT configuration under Linux. If I boot Linux with "nopat", the issue goes away. I am using the upstream coreboot and SeaBIOS with a custom board based on the bayleybay_fsp/fsp_baytrail and Linux 4.2. ~ # ./cbmem -V -l Looking for coreboot table at 0 1048576 bytes. Mapping 1MB of physical memory at 0x0 (requested 0x0). Found! coreboot table entry 0x11 Found forwarding entry. Unmapping 1MB of virtual memory at 0x7fee18224000. Looking for coreboot table at 7ad9f000 1048576 bytes. Mapping 1MB of physical memory at 0x7ad9f000 (requested 0x7ad9f000). ... failed. Mapping 1052671B of physical memory at 0x7ad9e000. Failed to mmap /dev/mem: Resource temporarily unavailable ~ # dmesg | tail -n 4 [ 221.700621] x86/PAT: cbmem.orig:1058 conflicting memory types 7ad9f000-7ae9f000 uncached-minus<->write-back 7adb8000-7adbc000 [ 221.705419] x86/PAT: reserve_memtype failed [mem 0x7ad9f000-0x7ae9efff], track write-back, req write-back [ 221.710435] x86/PAT: cbmem.orig:1058 conflicting memory types 7ad9e000-7ae9f000 uncached-minus<->write-back 7adb8000-7adbc000 [ 221.715501] x86/PAT: reserve_memtype failed [mem 0x7ad9e000-0x7ae9efff], track write-back, req write-back ~ # cat /sys/kernel/debug/x86/pat_memtype_list PAT memtype list: uncached-minus @ 0x7ada7000-0x7ada8000 write-back @ 0x7adb8000-0x7adbc000 write-back @ 0x7adbb000-0x7adbd000 write-back @ 0x7addd000-0x7adde000 uncached-minus @ 0x80600000-0x80601000 uncached-minus @ 0x80601000-0x80602000 uncached-minus @ 0x80602000-0x80603000 write-combining @ 0xc0000000-0xd0000000 ... snip ... >From the Linux kernel log: MTRR variable ranges enabled: 0 base 0FF800000 mask FFF800000 write-protect 1 base 000000000 mask F80000000 write-back 2 base 07B000000 mask FFF000000 uncachable 3 base 07C000000 mask FFC000000 uncachable I'm not sure how to read MTRR ranges, but it looks like the cbtable falls in the MTRR 1 with type write-back. >From the SeaBIOS log: e820 map has 15 items: 0: 0000000000000000 - 000000000009fc00 = 1 RAM 1: 000000000009fc00 - 00000000000a0000 = 2 RESERVED 2: 00000000000f0000 - 0000000000100000 = 2 RESERVED 3: 0000000000100000 - 000000007ad8e000 = 1 RAM 4: 000000007ad8e000 - 0000000080000000 = 2 RESERVED 5: 00000000e0000000 - 00000000f0000000 = 2 RESERVED ... snip ... The issue appears to be that the coreboot table memory is marked as uncached-minus and then Linux (?) is marking the ACPI tables as write-back. The 1 MB size is overlapping the ACPI tables. That makes mmap attempts of the whole coreboot table to fail due to the mismatched memory types. I have an older version of coreboot from Sage that does not have this issue. The difference is that the first uncached-minus entry is not present. On the Sage-based (4.0) coreboot image: ~ # cat /sys/kernel/debug/x86/pat_memtype_list PAT memtype list: write-back @ 0x7add1000-0x7add2000 write-back @ 0x7add4000-0x7add9000 write-back @ 0x7adda000-0x7addc000 uncached-minus @ 0x7ade2000-0x7ade3000 write-combining @ 0xc0000000-0xd0000000 ... snip ... So, I'm trying to figure out a few things: 1) Where does Linux get the PAT table from? The e820 map? 2) What piece of code sets the coreboot table as uncached-minus? 3) Why is that range set as uncached-minus? Would write-back work? Thanks, Ben

1 0

Re: [coreboot] coreboot binary policy
by Alex G. 30 Oct '15

30 Oct '15

On 10/29/2015 09:48 AM, Marc Jones wrote: > Hello coreboot, Hi Marc > Please limit comments to specific items in this version. If you have > additions for the next version (if needed), the draft document is open > for comment. > > https://docs.google.com/document/d/1wMdDUAZR2Z9V7hcs3IhIOqw6sYQxb3vPEmbITTC… That looks pretty good. I think you've done a great job of clarifying the requirements of ISA vs non-ISA blobs compared to the last version. I've made some comments on it to ask for clarification about the versioning requirements. While not necessarily specific to this version, are we still considering forbidding "no-reverse engineering" and "no-modification" clauses for blobs? Alex

3 2

Re: [coreboot] Document for review: coreboot Gerrit Etiquette and Guidelines
by Alex G. 30 Oct '15

30 Oct '15

On 10/29/2015 09:28 PM, ron minnich wrote: > It's a CL. Ooh, I thought it was a coreboot policy change that attempted to solve long-standing issues within coreboot. My bad. > It would be very helpful if you want to propose specific changes. I did. Refresh the page :) Alex

2 1

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

2004

2003

2002

coreboot October 2015