Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756865AbZDWSZn (ORCPT ); Thu, 23 Apr 2009 14:25:43 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752423AbZDWSZd (ORCPT ); Thu, 23 Apr 2009 14:25:33 -0400 Received: from extu-mxob-2.symantec.com ([216.10.194.135]:37928 "EHLO extu-mxob-2.symantec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751212AbZDWSZd (ORCPT ); Thu, 23 Apr 2009 14:25:33 -0400 Date: Thu, 23 Apr 2009 19:03:32 +0100 (BST) From: Hugh Dickins X-X-Sender: hugh@blonde.anvils To: Dmitry Adamushko cc: Ingo Molnar , Andrew Morton , Rusty Russell , Andreas Herrmann , Peter Oruba , Arjan van de Ven , linux-kernel@vger.kernel.org Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic In-Reply-To: Message-ID: References: <1240258569.6195.8.camel@earth> <1240344440.5861.10.camel@earth> <1240439073.12721.23.camel@earth> <20090423082704.GD599@elte.hu> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3275 Lines: 85 On Thu, 23 Apr 2009, Dmitry Adamushko wrote: > 2009/4/23 Ingo Molnar : > > * Dmitry Adamushko wrote: > > > >> Version 3 > >> > >> diff with v.2 > >> > >> - use smp_call_function_single() instead of work_on_cpu() as suggested by Ingo; > >> - add 'enum ucode_state' as return value of request_microcode_{fw,user} > >> - minor cleanups > > > > This version looks really nice structurally. Yes, it looks the right way to go to me too. > > What type of testing have you done on the patch, on what CPU type? > > Quite limited testing. It looks like there is no newer ucode availble > for my dual-core Intel Core2 Duo (Thinkpad R60) so what I did is as > follows: > > - change update_match_revision() in microcode_intel.c in a way that > allows loading of a ucode with a revision == the current revision > (rev. 0x57 in my case). From the POV of the microcode module it > nevertheless looks like an update; My P4 Xeon and Core2s and Atom all want newer microcode from the latest microcode.dat, and your version seems to work fine on them. > > - update via /dev/microcode (CONFIG_MICROCODE_OLD_INTERFACE) with a .dat file; > > - suspend -> resume (reload of ucode via cpu-callbacks) > > These tests worked for me. Yes, and I've also tried #if 0'ing the MICROCODE_OLD_INTERFACE code, converting the microcode.dat to binary, and placing that in /lib/firmware/intel-ucode/06-0f-0a or wherever: the firmware interface seems to be working too. > > Obviously, it'd be nice if more people could give it a try (e.g. > update with a firmware image and also on AMD - although the changes in > microcode_amd.c are quite straightforward). But like you I don't have an AMD to test. And just like last time I came here, I find there are more alternative ways through all this than I'm familiar with, and have time to test: device versus firmware, Intel versus AMD, startup versus resume versus online, builtin versus modular. I get easily confused without more time to spend on it. That SYSTEM_RUNNING test you've flagged with "--dimm. Review this case": yes, that's still necessary for when CONFIG_MICROCODE=y (and no initrd?), as I have - trying to get the firmware at that point will hang. I did try changing module_init(microcode_init) to late_initcall(microcode_init), but that didn't solve it. And skipping out at that point does leave CPU0 not updated. But you've not made anything worse there, and most people will have CONFIG_MICROCODE=m. > > p.s. argh... just noticed that the following line is redundant in > microcode_core.c (forgot to remove it) > > +enum { UCODE_UPDATE_ERR, UCODE_UPDATE_OK, UCODE_UPDATE_NAVAIL }; A couple of things that worried me. I guess your mutex Synchronization works out, but are interrupts still disabled around the critical wrmsr()s, wherever they're getting called from? And you've a habit of returning -1 in error cases, which later gets muddled in with errnos, so that it would amount to -EPERM, which is probably not what you want. Hugh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/