Fam10 FIDVID in SVI : disabling microcode update
Hello. I finally splitted my changes into some 25 small patches (some 8-10 quite trivial refactorings, but I hesitate to use "trivial" in signing off) . I compiled each for my board and tested it , and I hoped to find which of them fixed my fidvid issues. I have to add some commit comments and I hope to send them tonight or tomorrow evening. But after applying all of them it still didn't work. So I looked again and discovered in my previous tests I had commented the calls to update_microcode(...) . In the test of the smaller patches I had mistakenly commented only the call for the BSP but not for the APs. I tried with both calls commented and it works, then I thought, "ok, it must work with both uncommented, having diferent microcode versions in different cores is just weird". But with the calls uncommented it won't work (it hangs in ram initialization, like without most of the patches, or with microcode updated only in some cores, and then it depends, sometines it works). So in order for my board to pass fidvid and ram init I have to disable microcode updating. I haven't tried any new microcode in Agesa code submitted this last sunday, because, after all, it does not have source, so for all I care I'll be happier if it works without it. I guess the reason could be either of: - coreboot is applying the wrong microcode version, and some logic selecting the microcode can be fixed - the version of the microcode itself is faulty, and may be fixed in a later version (maybe already under /vendorcode/...) - the way to update the microcode does not work in my CPU But I don't feel like fixing any. Yet I don't think I should send a patch just commenting those calls to update_microcode, should I ? Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ? Should I just send the patches for fidvid and keep the update_microcode commented for myself only, even if this still does not fix the issues for my setup, so that someone else may investigate what's wrong with the microcode ? Are my patches moot because fam10 fidvid is going to be replaced with something from Agesa ? Finally, I've never used abuild, but taking into account I'm touching src/cpu/amd/model_10xxx/fidvid.c src/cpu/amd/model_10xxx/defaults.h src/cpu/amd/model_10xxx/init_cpus.c src/northbridge/amd/amdmct/wrappers/mcti_d.c src/northbridge/amd/amdmct/amddefs.h src/northbridge/amd/amdfam10/Kconfig src/northbridge/amd/amdfam10/raminit_amdmct.c src/northbridge/amd/amdht/AsPsDefs.h should I try to abuild before sending my patches ? thanks.
see patch
Am Donnerstag, den 17.02.2011, 07:35 +0100 schrieb xdrudis:
see patch Any opinion on these patches? Patch 1-8 seem to be refactorings only, and splitting functions into smaller logical units looks good to me, but I'd like to hear from someone deeper in the AMD code.
Patrick -- Patrick Georgi SINA-Development - High Security secunet Security Networks AG - Mergenthalerallee 77 - 65760 Eschborn, Germany Phone +49 201 54 54-3610 - Fax +49 201 54 54-1325 - www.secunet.com Sitz: Kronprinzenstraße 30, 45128 Essen / Amtsgericht Essen HRB 13615 Vorstand: Dr. Rainer Baumgart (Vors.), Thomas Koelzer, Thomas Pleines Aufsichtsratsvorsitzender: Dr. Karsten Ottenberg
On Thu, Feb 24, 2011 at 6:31 AM, Georgi, Patrick <Patrick.Georgi@secunet.com> wrote:
Am Donnerstag, den 17.02.2011, 07:35 +0100 schrieb xdrudis:
see patch Any opinion on these patches? Patch 1-8 seem to be refactorings only, and splitting functions into smaller logical units looks good to me, but I'd like to hear from someone deeper in the AMD code.
Patrick
Yes, These patches are improvements in the SVI code, and I think we need to pull them in. I just haven't had a chance to test them or review them yet. I think that all new Fam10 systems are SVI now. If anyone else has time to try it, please do. Marc -- http://se-eng.com
On Thu, Feb 24, 2011 at 02:31:29PM +0100, Georgi, Patrick wrote:
Am Donnerstag, den 17.02.2011, 07:35 +0100 schrieb xdrudis:
see patch Any opinion on these patches? Patch 1-8 seem to be refactorings only, and splitting functions into smaller logical units looks good to me, but I'd like to hear from someone deeper in the AMD code.
Yes, if these 8 are not refactorings, then it's a bug. I know it's a little work to review it all, but it does not have to be one person. You can review just one patch, maybe. Testing is maybe better to do with all of them, or all without negative reviews, or something. I've tested them one by one and it is a little a waste of time. And I haven't found a single one that fixes it for me. Must be a combination, possibly not all but not sure which ones. They're secuential although not each and every one needs all previous ones. By the way testing for both SVI and PVI is welcome (for AMD FAM 10). I don't intend to break PVI, but I can't test it. Some of the later ones may be a little paranoid or a matter of taste but I tried to split them in small pieces so they can be rejected or modified.
On Thu, Feb 24, 2011 at 2:27 PM, xdrudis <xdrudis@tinet.cat> wrote:
On Thu, Feb 24, 2011 at 02:31:29PM +0100, Georgi, Patrick wrote:
Am Donnerstag, den 17.02.2011, 07:35 +0100 schrieb xdrudis:
see patch Any opinion on these patches? Patch 1-8 seem to be refactorings only, and splitting functions into smaller logical units looks good to me, but I'd like to hear from someone deeper in the AMD code.
Yes, if these 8 are not refactorings, then it's a bug.
I know it's a little work to review it all, but it does not have to be one person. You can review just one patch, maybe.
Testing is maybe better to do with all of them, or all without negative reviews, or something. I've tested them one by one and it is a little a waste of time. And I haven't found a single one that fixes it for me. Must be a combination, possibly not all but not sure which ones. They're secuential although not each and every one needs all previous ones.
By the way testing for both SVI and PVI is welcome (for AMD FAM 10). I don't intend to break PVI, but I can't test it.
Some of the later ones may be a little paranoid or a matter of taste but I tried to split them in small pieces so they can be rejected or modified.
Acked-by: Marc Jones <marcj303@gmail.com> r6387 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:36 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6388 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:37 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6389 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:37 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by:Marc Jones <marcj303@gmail.com> r6390 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:37 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6391 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:38 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6392 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:38 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6393 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:39 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6394 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:39 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6395 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:39 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6396 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:40 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by:Marc Jones <marcj303@gmail.com> r6397 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:40 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by:Marc Jones <marcj303@gmail.com> r6398 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:40 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6399 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:42 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Minor tweak to remove codeDelay() function that was no longer used.. Acked-by: Marc Jones <marcj303@gmail.com> r6401 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:43 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6402 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:43 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6403 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:43 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6404 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:44 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6405 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:44 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6406 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:44 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6407 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:45 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
I moved coreDelay() to mcti_d where it was being used. Acked-by: Marc Jones <marcj303@gmail.com> r6408 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:45 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r6409 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:45 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Acked-by: Marc Jones <marcj303@gmail.com> r 6410 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:46 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
yay documentation! :) Acked-by: Marc Jones <marcj303@gmail.com> r 6411 -- http://se-eng.com
see patch
On Wed, Feb 16, 2011 at 11:46 PM, xdrudis <xdrudis@tinet.cat> wrote:
see patch
Oops no patch attached. Marc -- http://se-eng.com
Xavi Drudis Ferran writes:
Hello.
Hello.
Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ?
I have interest in that. I am working on ECS A740GM-M port and have asked that question in my mail, but for different reasons. [1] My patches are not in the repository yet. Still haven't split them. :( Right now I use a workaround to prevent loading of microcode. I've made a zero-filled microcode patch file that is kept in the board directory. The Kconfig option for the microcode file points to that file. The effect of this is same revision (0) in patch and CPU and the microcode is not loaded. This worked on Sempron 140 and r6725. Haven't tested it with recent changes and revisions. [1] http://www.coreboot.org/pipermail/coreboot/2011-January/063067.html
Hi Xavi, On Wed, Feb 16, 2011 at 02:45:02PM +0100, Xavi Drudis Ferran wrote:
Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ?
Yes, please. I would test and ack that. Thanks, Ward. -- Ward Vandewege <ward@fsf.org> Free Software Foundation - Senior Systems Administrator
On Fri, Feb 18, 2011 at 10:19:31AM -0500, Ward Vandewege wrote:
Hi Xavi,
On Wed, Feb 16, 2011 at 02:45:02PM +0100, Xavi Drudis Ferran wrote:
Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ?
Yes, please. I would test and ack that.
Thanks, Ward.
Here it is. It's limited to fam10. I don't know about other families nor can test. Sorry about having missed the request from Ivaylo, for a moment I thought I was the only one interested. It works in my board (but my board still does not boot, so tests in functional boards are welcome). Thanks.
On 2/19/11 12:45 PM, xdrudis wrote:
On Fri, Feb 18, 2011 at 10:19:31AM -0500, Ward Vandewege wrote:
Hi Xavi,
On Wed, Feb 16, 2011 at 02:45:02PM +0100, Xavi Drudis Ferran wrote:
Should I send a patch making a Kconfig option to not upgrade microcode for fam10? Is there any interest in that ? What's the particular rationale behind this? When disabling microcode updates, why don't you also disable the other erratas?
+ Select this to apply (propietary?) patches to the cpu I don't think the word proprietary (nor the question mark) applies very well here.
+ microcode provided by AMD to correct issues in the CPU after + production, and distributed with coreboot (not necessarily + the latest microcode version produced by AMD, but only + applied if newer than the version in your CPU). + + Unselect to let FAM10 CPUs run with factory microcode. If Here's some formatting problem. It's also unclear what you mean by "factory microcode" imho.
+ you unselect this, no binary microcode patches will be + included in the image, so it will help you get an image + which you have the entire source code for and may simplify + license compliance (IANAL). I don't see how this makes license compliance any easier. Also, please refrain from using terms like IANAL. If we make claims they should be either right to the best of our knowledge or we don't put them in the source code, anyways.
BTW, some corporations had legal teams analyze the microcode license and it was considered not problematic for inclusion in coreboot in the sense of the GPL.
--- src/cpu/amd/model_10xxx/update_microcode.c 2011-02-19 21:56:44.000000000 +0100 +++ src/cpu/amd/model_10xxx/update_microcode.c 2011-02-19 22:09:17.000000000 +0100 @@ -29,6 +29,7 @@ #include<cpu/amd/microcode.h> #endif
+#if CONFIG_UPDATE_CPU_MICROCODE static const u8 microcode_updates[] __attribute__ ((aligned(16))) = {
Please change the patch to not include update_microcode.c in the Makefile for the case that microcode selection is disabled. Stefan
participants (7)
-
Georgi, Patrick -
Ivaylo Valkov -
Marc Jones -
Stefan Reinauer -
Ward Vandewege -
Xavi Drudis Ferran -
xdrudis