Hi everybody,
coreboot is shipping AMD's open sourced AGESA for a few generations as part of its tree.
Some people advocate dropping the code due to its quality and lack of maintenance while others are happy with using the code.
So: to help keep this code alive, we'd need maintainers - people willing to work through issues, improve the code quality and generally act as a point of contact if any questions arise.
One item to start with could be to work through Coverity issues, where the largest proportion is now AGESA based after Jacob cleaned up most of the rest of the tree. See https://scan.coverity.com/projects/coreboot
Drivers needs support to not get in the way of later development, and AGESA is sorely lacking in that department. If you see value in that code, please step up now, not only when we're looking into removing that code for good.
Regards, Patrick
On Thu, Sep 12, 2019 at 5:43 PM Patrick Georgi via coreboot coreboot@coreboot.org wrote:
Hi everybody,
coreboot is shipping AMD's open sourced AGESA for a few generations as part of its tree.
Some people advocate dropping the code due to its quality and lack of maintenance while others are happy with using the code.
Would "some people" or these "advocates" be willing to elaborate? Pretty much none of the style guide rules have ever been applied to vendorcode, if this is about clang-format or source code style in vendorcode/.
So: to help keep this code alive, we'd need maintainers - people willing to work through issues, improve the code quality and generally act as a point of contact if any questions arise.
I would say >50% of our MAINTAINERS file is just bogus when it comes to working through issues. We can equally make such quality assurance arguments about FSP2.0, once commercial vendor gets something merged, they don't really care how much it gets in the way of overall architecture or subsystems evolution. As an active topic, I don't see anybody at Intel willing to discuss the topic of how hiding PCI devices may brake PCI compliance and generally several assumptions in coreboot PCI subsystem.
Previous decisions by coreboot leadership and/or community meeting minutes were to let platform obsolition be determined by a) release requirements, typically announced at least 12 months beforehand and b) lack of board-status submissions. Do you want to extend this with c) coverity scan over vendorcode/ ?
One item to start with could be to work through Coverity issues, where the largest proportion is now AGESA based after Jacob cleaned up most of the rest of the tree. See https://scan.coverity.com/projects/coreboot
Since coreboot seems to accept blobs with ease nowadays, the solution to keep these platforms can be such that we move AGESA vendorcode to submodule. We already have infrastructure to embed AGESA as a blob into CBFS/FMAP. Method has been approved for amd/stoneyridge, so there should be little to complain about it. Neither coverity or clang-format rules need to extend to 3rdparty repositories.
Drivers needs support to not get in the way of later development, and AGESA is sorely lacking in that department. If you see value in that code, please step up now, not only when we're looking into removing that code for good.
Please give detailed sample where AGESA or binaryPI (inside coreboot proper, not vendorcode) is "sorely lacking or getting in the way" of current developments. Reference to some already existing gerrit comments or mailing list posts would be nice. I agree there are things like SMM_ASEG=y, PARALLEL_MP=n. Do you have something completely different in mind? I am not surprised if you wanted to deprecate util/romcc rather sooner than later. If that is the driving force, it is already covered with requirements for next release.
AGESA may reach C_ENVIRONMENT_BOOTBLOCK by the time of the next release and is already at CAR_GLOBAL_MIGRATION=n. Situation with binaryPI is actually worse, selected boards may meet requirements though. The timeframe for dropping entire platforms has previously been couple releases or couple years, you are making it sound like you want to drop AGESA vendorcode like tomorrow.
Regards, Kyösti Mälkki
On Thu, Sep 12, 2019 at 07:20:49PM +0300, Kyösti Mälkki wrote:
Would "some people" or these "advocates" be willing to elaborate?
I CC'd Nico and Martin because I seem to remember that we talked about AGESA (and its quality and/or life cycle). Nico, for example, seems to advocate scrapping AGESA to replace it with a rewrite ;-)
I would say >50% of our MAINTAINERS file is just bogus when it comes to working through issues.
Which is something we're trying to improve.
As an active topic, I don't see anybody at Intel willing to discuss the topic of how hiding PCI devices may brake PCI compliance and generally several assumptions in coreboot PCI subsystem.
It's unlikely that they'll find your request hidden in this thread, which is about something very much outside their domain, so please start a discussion separately and maybe Cc the people closest to being maintainers for that part of the code (e.g. affected SoC)?
Do you want to extend this with c) coverity scan over vendorcode/ ?
Sorry if I wasn't as clear on this as I wanted to be: I'm not saying that "it's ugly in Coverity means we'll drop the code", but we have a couple of folks complaining about AGESA code quality like all. the. time (e.g. Nico?), and during Jacob's GSoC there was some talk about how it's unclear how long that code will remain in the tree anyway (I think Martin can chime in on this).
As I'm working through Coverity, that was brought back to my attention and I thought about thinking about that. Also, if somebody wants to step up as maintainer, Coverity provides a pretty good set of bite-sized tasks that walk you through the code area of interest indiscriminately.
We said to leave vendorcode alone, but that was under the assumption that it's maintained from some upstream source. Our AGESA sources quite obviously aren't, so why not open them up to development (including clean up) some more?
Reference to some already existing gerrit comments or mailing list posts would be nice.
Mostly chatter on IRC, to be honest. Part of the intent of this mail was to surface this more officially.
AGESA may reach C_ENVIRONMENT_BOOTBLOCK by the time of the next release and is already at CAR_GLOBAL_MIGRATION=n.
Well, that's good news!
The timeframe for dropping entire platforms has previously been couple releases or couple years,
No change intended here. I mostly wanted to avoid bringing this up formally just days before the intended removal (from feedback on the removals in 4.10, people don't seem to notice out various deprecation notices but only when stuff is gone).
This was about surfacing issues like these earlier, to reduce the amount of surprise. Having your status update on the C bootblock and CAR migration implementation is useful for that, too!
you are making it sound like you want to drop AGESA vendorcode like tomorrow.
I'm sorry to have made that impression. That's definitely not what I was proposing.
Patrick
My proposal is to drop platforms that aren't being tested, aren't being maintained, or are causing serious problems with general coreboot development.
For example - ASUS KGPE-d16 is still being used and tested, so I wouldn't suggest dropping that code, even though it apparently doesn't support S3, so it was suggested that we drop it. S3 isn't used heavily on servers, so personally I don't think it matters.
- The ONLY platform that supports AGESA f12h is torpedo, and it's never been tested, so I'd like to suggest again that it get dropped.
- Family 14h is very well tested, with regular testing happening. Do not drop. -- pcengines/apu1/4.9-1870-gd44d4f0f4e/2019-06-03T10_14_06Z -- asrock/e350m1/4.9-2228-gce4d39d2d7/2019-06-29T00_31_14Z
- The last time a family 15tn board was tested was in 2019. Do not drop. -- lenovo/g505s/4.9-8-g42d1660/2018-12-21T00_01_02Z
- The last time Family16KB was tested was in mid 2018. If it doesn't get tested again by mid 2020, I think we should consider dropping it then. -- asus/am1i-a/4.7-988-ge93634caa0/2018-05-03T08_29_12Z
The initial reason we wanted to leave AMD vendorcode alone (8 years ago) was because we hoped that AMD would update it. That hasn't happened, so I've already told several people that they're welcome to make whatever changes they feel are needed.
Martin
On Thu, Sep 12, 2019 at 10:42 AM Patrick Georgi pgeorgi@google.com wrote:
On Thu, Sep 12, 2019 at 07:20:49PM +0300, Kyösti Mälkki wrote:
Would "some people" or these "advocates" be willing to elaborate?
I CC'd Nico and Martin because I seem to remember that we talked about AGESA (and its quality and/or life cycle). Nico, for example, seems to advocate scrapping AGESA to replace it with a rewrite ;-)
I would say >50% of our MAINTAINERS file is just bogus when it comes to working through issues.
Which is something we're trying to improve.
As an active topic, I don't see anybody at Intel willing to discuss the topic of how hiding PCI devices may brake PCI compliance and generally several assumptions in coreboot PCI subsystem.
It's unlikely that they'll find your request hidden in this thread, which is about something very much outside their domain, so please start a discussion separately and maybe Cc the people closest to being maintainers for that part of the code (e.g. affected SoC)?
Do you want to extend this with c) coverity scan over vendorcode/ ?
Sorry if I wasn't as clear on this as I wanted to be: I'm not saying that "it's ugly in Coverity means we'll drop the code", but we have a couple of folks complaining about AGESA code quality like all. the. time (e.g. Nico?), and during Jacob's GSoC there was some talk about how it's unclear how long that code will remain in the tree anyway (I think Martin can chime in on this).
As I'm working through Coverity, that was brought back to my attention and I thought about thinking about that. Also, if somebody wants to step up as maintainer, Coverity provides a pretty good set of bite-sized tasks that walk you through the code area of interest indiscriminately.
We said to leave vendorcode alone, but that was under the assumption that it's maintained from some upstream source. Our AGESA sources quite obviously aren't, so why not open them up to development (including clean up) some more?
Reference to some already existing gerrit comments or mailing list posts would be nice.
Mostly chatter on IRC, to be honest. Part of the intent of this mail was to surface this more officially.
AGESA may reach C_ENVIRONMENT_BOOTBLOCK by the time of the next release and is already at CAR_GLOBAL_MIGRATION=n.
Well, that's good news!
The timeframe for dropping entire platforms has previously been couple releases or couple years,
No change intended here. I mostly wanted to avoid bringing this up formally just days before the intended removal (from feedback on the removals in 4.10, people don't seem to notice out various deprecation notices but only when stuff is gone).
This was about surfacing issues like these earlier, to reduce the amount of surprise. Having your status update on the C bootblock and CAR migration implementation is useful for that, too!
you are making it sound like you want to drop AGESA vendorcode like tomorrow.
I'm sorry to have made that impression. That's definitely not what I was proposing.
Patrick
Google Germany GmbH, ABC-Str. 19, 20354 Hamburg Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
On Thu, Sep 19, 2019 at 1:05 AM Martin Roth martinroth@google.com wrote:
My proposal is to drop platforms that aren't being tested, aren't being maintained, or are causing serious problems with general coreboot development.
For example
- ASUS KGPE-d16 is still being used and tested, so I wouldn't suggest
dropping that code, even though it apparently doesn't support S3, so it was suggested that we drop it. S3 isn't used heavily on servers, so personally I don't think it matters.
Nothing to do with S3 for asus/kgpe-d16 deprecation.
Platform code (non-AGESA) fam10-15 does not meet 3 of the 3 announced requirements for next release. Should someone want to maintain kgpe-d16 on master branch, the decisions on those release requirements will need to be officially withdrawn. AFAICS, that platform codebase even suffers from cache coherency issues while executing from cache-as-ram; there has been indications that increased spinlock usage in romstage causes boot failures and/or reset loops.
Implementation of HyperTransport requires maintaining some pretty strange (or poor-quality) code for both static devicetree and PCI subsystem.
Regards, Kyösti Mälkki
Kyösti Mälkki said:
AFAICS, that platform codebase even suffers from cache coherency issues while executing from cache-as-ram; there has been indications that increased spinlock usage in romstage causes boot failures and/or reset loops.
Where do you see this? Has it been reported?
Implementation of HyperTransport requires maintaining some pretty strange
(or poor-quality) code for both static devicetree and PCI subsystem.
Which code? How can it be improved?
Sincerely, -Matthew Bradley
On Wed, Sep 18, 2019 at 6:25 PM Kyösti Mälkki kyosti.malkki@gmail.com wrote:
On Thu, Sep 19, 2019 at 1:05 AM Martin Roth martinroth@google.com wrote:
My proposal is to drop platforms that aren't being tested, aren't being maintained, or are causing serious problems with general coreboot development.
For example
- ASUS KGPE-d16 is still being used and tested, so I wouldn't suggest
dropping that code, even though it apparently doesn't support S3, so it was suggested that we drop it. S3 isn't used heavily on servers, so personally I don't think it matters.
Nothing to do with S3 for asus/kgpe-d16 deprecation.
Platform code (non-AGESA) fam10-15 does not meet 3 of the 3 announced requirements for next release. Should someone want to maintain kgpe-d16 on master branch, the decisions on those release requirements will need to be officially withdrawn. AFAICS, that platform codebase even suffers from cache coherency issues while executing from cache-as-ram; there has been indications that increased spinlock usage in romstage causes boot failures and/or reset loops.
Implementation of HyperTransport requires maintaining some pretty strange (or poor-quality) code for both static devicetree and PCI subsystem.
Regards, Kyösti Mälkki _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Thu, Sep 19, 2019 at 3:20 AM Matt B matthewwbradley6@gmail.com wrote:
Kyösti Mälkki said:
AFAICS, that platform codebase even suffers from cache coherency issues while executing from cache-as-ram; there has been indications that increased spinlock usage in romstage causes boot failures and/or reset loops.
Where do you see this? Has it been reported?
It was the plausible reason for the revert: https://review.coreboot.org/c/coreboot/+/30830 where seemingly legit code change trigger bugs.
Like with many RAM related problems on same platform(s) running with upstream coreboot, those with hardware have not bisected or debugged this further to have definite answers. I can tell AGESA fam14 has issues with AP's accessing CAR.
Implementation of HyperTransport requires maintaining some pretty strange (or poor-quality) code for both static devicetree and PCI subsystem.
Which code? How can it be improved?
Unless platform meets release requirements, there will be a commit to remove offending/bad code with explanations. As noted earlier, pretty much none of the promises from last five years to debug/bisect/improve, by various people on the mailing list, have not been fulfilled. My goodwill with fam10-15 is long gone.
Regards, Kyösti Mälkki
Martin Roth via coreboot:
- The ONLY platform that supports AGESA f12h is torpedo, and it's
never been tested, so I'd like to suggest again that it get dropped.
I am seeing some unique Coverity issues in f12h as well. Is there an established procedure for dropping platforms, or is it as simple as rm-rf src/vendorcode/amd/agesa/f12 (plus the specific torpedo mainboard)?
On 12.09.19 18:42, Patrick Georgi wrote:
On Thu, Sep 12, 2019 at 07:20:49PM +0300, Kyösti Mälkki wrote:
Would "some people" or these "advocates" be willing to elaborate?
I CC'd Nico and Martin because I seem to remember that we talked about AGESA (and its quality and/or life cycle). Nico, for example, seems to advocate scrapping AGESA to replace it with a rewrite ;-)
Ah, yes. I might have said something. When talking about AGESA ports, I most probably meant the hook-up in coreboot, not the vendorcode/. I usually don't look at the latter.
I would love to see a clean rewrite and assume that I proposed this when somebody asked what could/should be done. However, I don't see it as a requirement. Also, we have much more worrisome code in the tree (e.g. KGPE-D16 and surrounding code, suffering from undefined behavior, #including of .c files etc.).
Nico
On Thu, Sep 19, 2019 at 11:12 AM Nico Huber nico.h@gmx.de wrote:
On 12.09.19 18:42, Patrick Georgi wrote:
On Thu, Sep 12, 2019 at 07:20:49PM +0300, Kyösti Mälkki wrote:
Would "some people" or these "advocates" be willing to elaborate?
I CC'd Nico and Martin because I seem to remember that we talked about AGESA (and its quality and/or life cycle). Nico, for example, seems to advocate scrapping AGESA to replace it with a rewrite ;-)
Ah, yes. I might have said something. When talking about AGESA ports, I most probably meant the hook-up in coreboot, not the vendorcode/. I usually don't look at the latter.
I would love to see a clean rewrite and assume that I proposed this when somebody asked what could/should be done. However, I don't see it as a requirement. Also, we have much more worrisome code in the tree (e.g. KGPE-D16 and surrounding code, suffering from undefined behavior, #including of .c files etc.).
Nico
Interesting. In terms of lines of code, probably 75% of AGESA glue logic in ports has already been removed. But I agree, aside from release requirements, there is lots left that could be done. There is essentially no interest for new board ports on AGESA/binaryPI, these platforms have mostly survived in the tree due to commercial support to maintain them.
Kyösti Mälkki
There is essentially no interest for new board ports on AGESA/binaryPI, these platforms have mostly survived in the tree due to commercial support to maintain them.
This seems to be untrue when boards like the Asus AM1I-A were ported as recently as last year. [1] It's a AMD family 16h board that looks like it calls into binaryPI based AGESA a number of times, judging by the boot logs on the wiki. [2]
I plan on using this board myself for a small NAS system.
-Matt
[1] https://www.phoronix.com/scan.php?page=news_item&px=ASUS-AM1I-A-Coreboot https://source.puri.sm/coreboot/coreboot/commit/3dce9f09d9e26b147153ad0cda49... https://github.com/coreboot/coreboot/commits/master/src/mainboard/asus/am1i-... [2] https://www.coreboot.org/Board:asus/am1i-a
On Fri, Sep 20, 2019 at 4:00 AM Kyösti Mälkki kyosti.malkki@gmail.com wrote:
On Thu, Sep 19, 2019 at 11:12 AM Nico Huber nico.h@gmx.de wrote:
On 12.09.19 18:42, Patrick Georgi wrote:
On Thu, Sep 12, 2019 at 07:20:49PM +0300, Kyösti Mälkki wrote:
Would "some people" or these "advocates" be willing to elaborate?
I CC'd Nico and Martin because I seem to remember that we talked about AGESA (and its quality and/or life cycle). Nico, for example, seems to advocate scrapping AGESA to replace it with a rewrite ;-)
Ah, yes. I might have said something. When talking about AGESA ports, I most probably meant the hook-up in coreboot, not the vendorcode/. I usually don't look at the latter.
I would love to see a clean rewrite and assume that I proposed this when somebody asked what could/should be done. However, I don't see it as a requirement. Also, we have much more worrisome code in the tree (e.g. KGPE-D16 and surrounding code, suffering from undefined behavior, #including of .c files etc.).
Nico
Interesting. In terms of lines of code, probably 75% of AGESA glue logic in ports has already been removed. But I agree, aside from release requirements, there is lots left that could be done. There is essentially no interest for new board ports on AGESA/binaryPI, these platforms have mostly survived in the tree due to commercial support to maintain them.
Kyösti Mälkki _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Sat, Sep 21, 2019 at 5:18 AM Matt B matthewwbradley6@gmail.com wrote:
There is essentially no interest for new board ports on AGESA/binaryPI, these platforms have mostly survived in the tree due to commercial support to maintain them.
This seems to be untrue when boards like the Asus AM1I-A were ported as recently as last year. [1] It's a AMD family 16h board that looks like it calls into binaryPI based AGESA a number of times, judging by the boot logs on the wiki. [2]
Like Martin noted above, board-status for asus/am1i-a is from mid-2018. Now it's not uncommon to lose interest after initial port is merged, aspecially if that board turned into a NAS production system, but the next big board obsoletion criteria might be the lack of active testing. Those who actively do testing and development on lenovo/g505s and asus/f2a85-m, thank you.
I could rephrase what I said before but it will just look worse or offending to some: Those who have contributed to coreboot in form of new boards ports, using scrubbed AGESA vendorcode or binaryPI blobs, have never shown much interest to evolve the platform code (in coreboot proper). This very much applies to original authors AMD AES (R.I.P.) and SAGE (R.I.P.), AGESA and binaryPI only survived over LATE_CBMEM_INIT deprecation thanks to PC Engines support.
Kyösti Mälkki
Patrick Georgi via coreboot:
Hi everybody,
coreboot is shipping AMD's open sourced AGESA for a few generations as part of its tree.
Some people advocate dropping the code due to its quality and lack of maintenance while others are happy with using the code.
So: to help keep this code alive, we'd need maintainers - people willing to work through issues, improve the code quality and generally act as a point of contact if any questions arise.
One item to start with could be to work through Coverity issues, where the largest proportion is now AGESA based after Jacob cleaned up most of the rest of the tree. See https://scan.coverity.com/projects/coreboot
I would like to help out. Is there someone experienced who can mentor me on setting up a streamlined, open-source development environment for Coreboot? I've been using grep and gedit for my hacking needs, but trying to maintain a 5 level deep state table of AGESA code dependencies in my head was a problem. How did Jacob get started and what IDE did he use, for example?
Drivers needs support to not get in the way of later development, and AGESA is sorely lacking in that department. If you see value in that code, please step up now, not only when we're looking into removing that code for good.
Which drivers and what support? I see Kyösti Mälkki replied with better questions. Where is the biggest pain point today, i.e. not already being worked on and would return the most value to Coreboot by my work?
Interesting discussion! It got me to wondering, having spent a lot of time in the V1, V2, and V3 trees the last few months.
Is this statement true? "There is no source so bad that a binary blob is better"
If we take that to be true, then what about this: "bad source should never be replaced by a blob, but only by better source"
From there: "we must never remove source that is in use if the only replacement is a blob"
Would that help guide the decision on AGESA? Or is it just more bikeshedding :-)
I personally find the AgEsA cOdE quite UgLy, but ... it's code. I wonder if it can't be fixed with a few initial spatch passes. We kept it relatively the same when AMD was a player in coreboot, but they're long gone; time for major surgery?
I don't think we want to say "ugly code, nuke it" unless there is replacement that is code.
ron
On Thu, Sep 12, 2019 at 9:46 AM awokd via coreboot coreboot@coreboot.org wrote:
Patrick Georgi via coreboot:
Hi everybody,
coreboot is shipping AMD's open sourced AGESA for a few generations as part of its tree.
Some people advocate dropping the code due to its quality and lack of maintenance while others are happy with using the code.
So: to help keep this code alive, we'd need maintainers - people willing to work through issues, improve the code quality and generally act as a point of contact if any questions arise.
One item to start with could be to work through Coverity issues, where the largest proportion is now AGESA based after Jacob cleaned up most of the rest of the tree. See https://scan.coverity.com/projects/coreboot
I would like to help out. Is there someone experienced who can mentor me on setting up a streamlined, open-source development environment for Coreboot? I've been using grep and gedit for my hacking needs, but trying to maintain a 5 level deep state table of AGESA code dependencies in my head was a problem. How did Jacob get started and what IDE did he use, for example?
Drivers needs support to not get in the way of later development, and AGESA is sorely lacking in that department. If you see value in that code, please step up now, not only when we're looking into removing that code for good.
Which drivers and what support? I see Kyösti Mälkki replied with better questions. Where is the biggest pain point today, i.e. not already being worked on and would return the most value to Coreboot by my work? _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Greetings all,
Patrick gregori said:
Mostly chatter on IRC, to be honest. Part of the intent of this mail was to surface this more officially.
It would be helpful to carry over more details when porting discussions from IRC. It is always good to be specific how something is broken, not just that the garbage collector is coming.
Kyösti said:
Since coreboot seems to accept blobs with ease nowadays, the solution to keep these platforms can be such that we move AGESA vendorcode to submodule.
This is one way of handling it, but I don't think it does anything to address the quality of the code. Out of sight and out of mind, so to speak.
We can equally make such quality assurance arguments about FSP2.0, once
commercial vendor gets something merged, they don't really care how much it gets in the way of overall architecture or subsystems evolution.
As Ron noted:
We kept it relatively the same when AMD was a player in coreboot, but they're long gone; time for major surgery?
I assume that AMD probably doesn't have any plans to revisit 15h if/when they follow through on porting 17h? :-)
Patrik said:
This was about surfacing issues like these earlier, to reduce the amount of surprise. Having your status update on the C bootblock and CAR migration implementation is useful for that, too!
Is there a single point of reference for this kind of information, to avoid surprises?
Ron said:
Is this statement true? "There is no source so bad that a binary blob is better"
Unless this is the trivial case where the blob boots and the code doesn't (or is otherwise functionally inferior), I would argue that any code can be compiled into a blob. Thus, the code is automatically as-good-or-better. :-)
If we take that to be true, then what about this:
"bad source should never be replaced by a blob, but only by better source"
This is where things begin to come apart. The first part is true based on (1) but unless someone writes better source, eventually the code breaks and you enter the trivial case above.
"we must never remove source that is in use if the only replacement is a
blob"
If the code works (it is in use after all), then refer to (1). Otherwise, this is the trivial case.
Once you find yourself in the trivial case, you need to make the decision to either fix the broken code, or embrace the blob.
Sincerely, -Matt
On Thu, Sep 12, 2019 at 1:36 PM ron minnich rminnich@gmail.com wrote:
Interesting discussion! It got me to wondering, having spent a lot of time in the V1, V2, and V3 trees the last few months.
Is this statement true? "There is no source so bad that a binary blob is better"
If we take that to be true, then what about this: "bad source should never be replaced by a blob, but only by better source"
From there: "we must never remove source that is in use if the only replacement is a blob"
Would that help guide the decision on AGESA? Or is it just more bikeshedding :-)
I personally find the AgEsA cOdE quite UgLy, but ... it's code. I wonder if it can't be fixed with a few initial spatch passes. We kept it relatively the same when AMD was a player in coreboot, but they're long gone; time for major surgery?
I don't think we want to say "ugly code, nuke it" unless there is replacement that is code.
ron
On Thu, Sep 12, 2019 at 9:46 AM awokd via coreboot coreboot@coreboot.org wrote:
Patrick Georgi via coreboot:
Hi everybody,
coreboot is shipping AMD's open sourced AGESA for a few generations as part of its tree.
Some people advocate dropping the code due to its quality and lack of maintenance while others are happy with using the code.
So: to help keep this code alive, we'd need maintainers - people willing to work through issues, improve the code quality and generally act as a point of contact if any questions arise.
One item to start with could be to work through Coverity issues, where the largest proportion is now AGESA based after Jacob cleaned up most of the rest of the tree. See https://scan.coverity.com/projects/coreboot
I would like to help out. Is there someone experienced who can mentor me on setting up a streamlined, open-source development environment for Coreboot? I've been using grep and gedit for my hacking needs, but trying to maintain a 5 level deep state table of AGESA code dependencies in my head was a problem. How did Jacob get started and what IDE did he use, for example?
Drivers needs support to not get in the way of later development, and AGESA is sorely lacking in that department. If you see value in that code, please step up now, not only when we're looking into removing that code for good.
Which drivers and what support? I see Kyösti Mälkki replied with better questions. Where is the biggest pain point today, i.e. not already being worked on and would return the most value to Coreboot by my work? _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
excellent points matt
"There is no working bad source so bad that a binary blob is better"
"working bad source should never be replaced by a blob, but only by improved working bad source"
"we must never remove working bad source that is in use if the only replacement is a blob"
On Thu, Sep 12, 2019 at 1:47 PM Matt B matthewwbradley6@gmail.com wrote:
Greetings all,
Patrick gregori said:
Mostly chatter on IRC, to be honest. Part of the intent of this mail was to surface this more officially.
It would be helpful to carry over more details when porting discussions from IRC. It is always good to be specific how something is broken, not just that the garbage collector is coming.
Kyösti said:
Since coreboot seems to accept blobs with ease nowadays, the solution to keep these platforms can be such that we move AGESA vendorcode to submodule.
This is one way of handling it, but I don't think it does anything to address the quality of the code. Out of sight and out of mind, so to speak.
We can equally make such quality assurance arguments about FSP2.0, once commercial vendor gets something merged, they don't really care how much it gets in the way of overall architecture or subsystems evolution.
As Ron noted:
We kept it relatively the same when AMD was a player in coreboot, but they're long gone; time for major surgery?
I assume that AMD probably doesn't have any plans to revisit 15h if/when they follow through on porting 17h? :-)
Patrik said:
This was about surfacing issues like these earlier, to reduce the amount of surprise. Having your status update on the C bootblock and CAR migration implementation is useful for that, too!
Is there a single point of reference for this kind of information, to avoid surprises?
Ron said:
Is this statement true? "There is no source so bad that a binary blob is better"
Unless this is the trivial case where the blob boots and the code doesn't (or is otherwise functionally inferior), I would argue that any code can be compiled into a blob. Thus, the code is automatically as-good-or-better. :-)
If we take that to be true, then what about this: "bad source should never be replaced by a blob, but only by better source"
This is where things begin to come apart. The first part is true based on (1) but unless someone writes better source, eventually the code breaks and you enter the trivial case above.
"we must never remove source that is in use if the only replacement is a blob"
If the code works (it is in use after all), then refer to (1). Otherwise, this is the trivial case.
Once you find yourself in the trivial case, you need to make the decision to either fix the broken code, or embrace the blob.
Sincerely, -Matt
On Thu, Sep 12, 2019 at 1:36 PM ron minnich rminnich@gmail.com wrote:
Interesting discussion! It got me to wondering, having spent a lot of time in the V1, V2, and V3 trees the last few months.
Is this statement true? "There is no source so bad that a binary blob is better"
If we take that to be true, then what about this: "bad source should never be replaced by a blob, but only by better source"
From there: "we must never remove source that is in use if the only replacement is a blob"
Would that help guide the decision on AGESA? Or is it just more bikeshedding :-)
I personally find the AgEsA cOdE quite UgLy, but ... it's code. I wonder if it can't be fixed with a few initial spatch passes. We kept it relatively the same when AMD was a player in coreboot, but they're long gone; time for major surgery?
I don't think we want to say "ugly code, nuke it" unless there is replacement that is code.
ron
On Thu, Sep 12, 2019 at 9:46 AM awokd via coreboot coreboot@coreboot.org wrote:
Patrick Georgi via coreboot:
Hi everybody,
coreboot is shipping AMD's open sourced AGESA for a few generations as part of its tree.
Some people advocate dropping the code due to its quality and lack of maintenance while others are happy with using the code.
So: to help keep this code alive, we'd need maintainers - people willing to work through issues, improve the code quality and generally act as a point of contact if any questions arise.
One item to start with could be to work through Coverity issues, where the largest proportion is now AGESA based after Jacob cleaned up most of the rest of the tree. See https://scan.coverity.com/projects/coreboot
I would like to help out. Is there someone experienced who can mentor me on setting up a streamlined, open-source development environment for Coreboot? I've been using grep and gedit for my hacking needs, but trying to maintain a 5 level deep state table of AGESA code dependencies in my head was a problem. How did Jacob get started and what IDE did he use, for example?
Drivers needs support to not get in the way of later development, and AGESA is sorely lacking in that department. If you see value in that code, please step up now, not only when we're looking into removing that code for good.
Which drivers and what support? I see Kyösti Mälkki replied with better questions. Where is the biggest pain point today, i.e. not already being worked on and would return the most value to Coreboot by my work? _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Thu, Sep 12, 2019 at 04:46:00PM +0000, awokd via coreboot wrote:
Patrick Georgi via coreboot:
Hi everybody,
coreboot is shipping AMD's open sourced AGESA for a few generations as part of its tree.
Some people advocate dropping the code due to its quality and lack of maintenance while others are happy with using the code.
So: to help keep this code alive, we'd need maintainers - people willing to work through issues, improve the code quality and generally act as a point of contact if any questions arise.
One item to start with could be to work through Coverity issues, where the largest proportion is now AGESA based after Jacob cleaned up most of the rest of the tree. See https://scan.coverity.com/projects/coreboot
I would like to help out. Is there someone experienced who can mentor me on setting up a streamlined, open-source development environment for Coreboot? I've been using grep and gedit for my hacking needs, but trying to maintain a 5 level deep state table of AGESA code dependencies in my head was a problem. How did Jacob get started and what IDE did he use, for example?
The Coverity issue tracker has several IDE-like features, such as a usage finder and go-to definitions. This was adequate for most of my needs, and anything else I tracked down using vim and judicious use of grep. There are probably more efficient ways to do this (eg. ctags?), but this was sufficient for my purposes, since most Coverity problems only need a local understanding of the codebase. If you're looking to start tackling Coverity issues then #1405310, #1241824, #1241913, and #1241847 (and other issues of those types) look like a good place to get started. I'd be happy to help you through the workflow and review patches for simple fixes like these, but don't have the knowledge to do the more complex ones.
Cheers, Jacob
Jacob Garber:
The Coverity issue tracker has several IDE-like features, such as a usage finder and go-to definitions. This was adequate for most of my needs, and anything else I tracked down using vim and judicious use of grep. There are probably more efficient ways to do this (eg. ctags?), but this was sufficient for my purposes, since most Coverity problems only need a local understanding of the codebase. If you're looking to start tackling Coverity issues then #1405310, #1241824, #1241913, and #1241847 (and other issues of those types) look like a good place to get started. I'd be happy to help you through the workflow and review patches for simple fixes like these, but don't have the knowledge to do the more complex ones.
Sounds like I should be able to tackle at least the "local understanding" ones with my existing tools. Thank you for the pointers, will see what I can do.
On Thu, Sep 12, 2019 at 04:46:00PM +0000, awokd via coreboot wrote:
Drivers needs support to not get in the way of later development, and AGESA is sorely lacking in that department. If you see value in that code, please step up now, not only when we're looking into removing that code for good.
Which drivers and what support? I see Kyösti Mälkki replied with better questions. Where is the biggest pain point today, i.e. not already being worked on and would return the most value to Coreboot by my work?
In general terms:
coreboot is really a loose bunch of related projects ("open source firmware") sharing common code, but the specific code shares relatively little activity: You don't need to know about AMD bring-up if you're dealing with Intel or even ARM. The idea is that if everybody takes care of the parts they're interested in, we get full coverage of everything we need to support.
You'd provide the most value for the project if you keep the parts of the code alive that are relevant to you. That way, we know that the actively used code is in a good shape.
Keeping it "alive" starts with providing board-status reports (so we know what boards are still in use: it doesn't make much sense to support 15 year old stuff that nobody uses and you can't even find on ebay anymore).
The next step would to be look which parts of the sources are specific to your board (or its family of boards) and are at risk getting neglected: People are in for the new shiny, so it's normal that the developer base for older stuff gets smaller - with a few notable exceptions, like the desire to have capital-F-free boards where there are few-to-no new ones.
Such parts of the code would benefit the most (you, and through you, the project) from some additional developer eyeballs.
Finally, once you feel comfortable enough, you could volunteer as a maintainer of a device, making you the official point of contact if anything needs to be done about a board.
The duties of a maintainer aren't well carved-out yet, but they definitely include being responsive to changes on our code review system that affect the board, being responsive to inquiries ("We want to redo X and it might require changes to your code") and to do regular tests, so you know when the part you're responsible for break (due to unrelated changes), and then fix it.
Being a maintainer doesn't require you to be some kind of rockstar coder: You should be able to handle basic changes and understand what the code does, but if there are any large redesigns across coreboot, there's typically somebody on the other end of the project (those doing these redesigns) able to help you out.
Even if you need some guidance for more intricate changes, the more active devs doing project-wide changes can't know all the details and can't have all the devices to test them. As a maintainer, that's where you come in: You either know what the code does that you maintain or you're willing to find out. You have the device and you're willing to test unstable code and you're able to recover from bad flashes.
Let's move away from the abstract and to the practical:
If you have a board that you want to ensure remains in a healthy state in coreboot, you maybe should start to get your feet wet.
Step 1: Figure out what code is involved in booting your board.
I'm using ASUS F2A85-M as an example because that's somewhat related to this thread:
$ grep "^[[:space:]]*<chip>" src/mainboard/asus/f2a85-m/devicetree_f2a85-m.cb |sed "s,\t, ,g" chip northbridge/amd/agesa/family15tn/root_complex chip cpu/amd/agesa/family15tn chip northbridge/amd/agesa/family15tn # CPU side of HT root complex chip northbridge/amd/agesa/family15tn # PCI side of HT root complex chip southbridge/amd/agesa/hudson # it is under NB/SB Link, but on the same pci bus chip drivers/generic/generic #dimm 0 chip drivers/generic/generic #dimm 1 chip superio/ite/it8728f
These "chips" point to drivers, each in their own directory (prepend src/ and you're there). In these directories, Makefile.inc is the main mechanism that decides which files go where.
If somebody breaks common code, we'll notice immediately. If somebody breaks a driver that is used on a lot of boards, we'll notice quickly (e.g. SuperIO drivers). At highest risk are drivers that are specific to a few boards, especially if they're all approximately the same age (and therefore popularity). These are the chipset drivers (northbridge, southbridge, cpu, or on newer boards "soc"), and that's what we need maintainers for the most.
Step 2: Find something to do with the code
We have a few contributors who do rather manual tasks, like sorting out if the included header files are all necessary or making sure the coding style is followed (although we plan to automate the latter part).
There are also usually a number of bugs or TODOs mentioned in the code that you could look out for and dig into.
Plus, I mentioned it already, there's stuff like Coverity Scan (and scan-build at https://coreboot.org/scan-build/) that point out "suspicious" issues in the code. These are nice because they're often well-scoped: Here's a small snippet of code, and this issue is worth looking into.
If you're stuck, you're always welcome to ask for help or guidance on the mailing list or on IRC (chat). It can take a while to get a response since we're all busy with all kinds of things, but you'll usually find somebody who is willing to help.
The only thing we can't do is make you motivated to dig into the minutiae of hardware init: if that's your thing, we'll help fill in the gaps. If that sounds boring to you, you might want to look for something else to do.
Step 3: When you did something to the code, expose it
I'm not sure how well-versed you are in open source development, but this step can be frightening: You changed the code, it didn't break anything (to your knowledge) and now people are looking at it in something that is called "code review". Scary!
But I encourage you to see it as opportunity: We love it when folks get started in our codebase, and we love giving advice (makes us feel better, too!).
With that I encourage you to accept feedback as "hey, here's a way to get even better", not as "aw, that's crap". The community encourages a positive tone as well (although, since we're people, it doesn't always work out as intended).
Step 4: Become a domain expert
It takes surprisingly little to become more knowledgable about some specific thing in coreboot than everybody else. If that thing is directly related to your interest, everybody wins.
Signing up as maintainer (see the MAINTAINERS file) makes your willingness to be such a domain expert and primary contact about that thing official. We'll stop folks in time if we think that they're not ready yet ;-)
Patrick
Patrick Georgi via coreboot:
[snip well-written description of contributor/maintainer duties]
Thank you for that. Wonder if it should be added to the project documentation somewhere- I'd seen bits and pieces of it before on this list, but it is a great overview.
Plus, I mentioned it already, there's stuff like Coverity Scan (and scan-build at https://coreboot.org/scan-build/) that point out "suspicious" issues in the code. These are nice because they're often well-scoped: Here's a small snippet of code, and this issue is worth looking into.
Think I'll get some exposure to the code this way first and see how far I get. Submitted a request to Coverity to be added as contributor.
awokd via coreboot wrote:
Submitted a request to Coverity to be added as contributor.
Thank you very much! That's great.
//Peter
-
awokd -
Jacob Garber -
Kyösti Mälkki -
Martin Roth -
Matt B -
Nico Huber -
Patrick Georgi -
Peter Stuge -
ron minnich