On Tue 22/02/11 23:58 , "Alex G." mr.nuke.me@gmail.com wrote:
On 02/22/2011 02:47 AM, Peter Stuge wrote:
Xavi Drudis Ferran wrote:
Does everyone prefer to have it not include update_microcode.c and change romstage.c in those boards that call update_microcode(...) ?
At least I like this better. It makes it clear what effect this option has for mainboard code.
If this option ever existed, I want it to stay out of my way entirely. I don't even want to know it exists without explicitly searching for it. I don't want to ever be in the situation where I find out that my board froze because the microcode update was disabled, and I don't want newcomers to waste their time asking about that pompous sounding
"Disable Microcode update" option in menuconfig.
Pompous ?
If you look at the patch the option name is +config UPDATE_CPU_MICROCODE + bool "Update cpu microcode" + default y
and it's "y" by default, so you won't get your microcode update disabled unless all these happen: - you have a FAM 10 cpu - you make menuconfig (or similar) - you go to the chipset menu and under title CPU unselect the preselected option "Update cpu microcode"
If that is too visible for you I don't know how to build an option that different users may enable or disable.
If you mean visible in the source code:
Option A: the patch I sent changes only one .c file and one Kconfig file. The function update_microcode() would still exist and it would do the update or do nothing depending on CONFIG_UPDATE_CPU_MICROCODE. No other files would need touching. This is what my software engineer intuition recommends, centralise changes in one place, change the behaviour of a procedure depending on configuration options, don't change every single place that procedure is called.
But people have considered this to be too little visible, or too little invasive for its heretic nature, or I don't quite understand what, and they've asked
Option B:
change src/cpu/amd/model_10xxx/Makefile.inc so that update_microcode.c does not get compiled in. when CONFIG_UPDATE_CPU_MICROCODE is false since in that case the function update_microcode(...) would be undefined, we would have to also change all files with a call to it, by placing an #if around the call. Those files are src/cpu/amd/model_10xxx/init-cpus.c src/mainboard/*/*/romstage.c (and by */* I mean all or some of the fam 10 boards, not all boards).
And I can even imagine another
Option C:
change update_microcode.c so that when CONFIG_AMD_UCODE_PATCH_FILE is defined to be an empty string or 0 or something, then mc_*.h does not get included into update_microcode.c, the update_microcode function does not find microcode to update and it ends up doing nothing.
I don't think this is desirable because then this would be a mainboard option, not a user option, and you might need different mainboard dirs for different CPU revisions or just for different user choices. So I won't send a patch for this. But someone might.
If I understand you I think you would prefer option A, so that the change is not so visible in source code, but maybe you want C because you don't want to see it in make menuconfig at all . I'd appreciate some more detail.
About newcomers complaining because they've disabled microcode update, it's not somethign that would happen without user action, and we already have newcomers (Ivaylo and I) complaining because the microcode update it giving us trouble. We can add a note in Kconfig help like :
"If you unselect this option and have some problem you want to ask about in the coreboot mailing list (or elsewhere) please report clearly you disable microcode update, since some developers strongly advise against this."
If I _really_ wanted to disable microcode updates, I would simply comment out a line of code.
That's what I did ! But I found out there were more people than just myself wanting this option, so I tried to code a patch that would change nothing for those who wanted the old behaviour but would allow to disable microcode update and keep testing new svn revisions, sending patches, etc. without the hurdle of a local modification, for those of us that need it.
If I had know how polemic it all would become I've probably hadn't sent the "pompous" patch at all.
As far as the licensing discusion gooes, I have thought of this intensely. I don't see microcode as violating my freedom. It is hardware. modifying microcode modifies the hardware. This is something that software cannot do.
Violating licenses is not the same as violating freedom, and freedom from lawsuits is also something one mike like. Also being kind to people you take code from and considering they might consider hardware as the part of an information system that you cannot change (excluding people) and software as the part that you can change, even if you think they're wrong and microcode is hardware, has some ethical value.
But I won't follow along this route, because we simply don't agree in enough categories to agree in this.
- We are firmware developers, not hardware engineers.
May I remind that the patch does not modifiy the microcode creatively ? It just leaves as it was the microcode that the "hardware" engineers installed in the factory ? Some people have found that version of the "hardware" to work better than the "modified hardware" after microcode update.
I'm not voting no on this, just so as long as it is completely invisible to me.
Please specify which of Option A, B, or C do you prefer, or propose something else, if you don't vote no. I can't code a completely invisible patch to a free software project that evolves in the open like coreboot. Whatever I do you will see it if you look at it.
Thank you.