Date:   Wed, 1 Feb 2023 13:47:58 -0800
From:   Ashok Raj <ashok.raj@intel.com>
To:     Borislav Petkov <bp@alien8.de>
CC:     Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Tony Luck <tony.luck@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Reinette Chatre <reinette.chatre@intel.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        Stefan Talpalaru <stefantalpalaru@yahoo.com>,
        David Woodhouse <dwmw2@infradead.org>,
        "Benjamin Herrenschmidt" <benh@kernel.crashing.org>,
        Jonathan Corbet <corbet@lwn.net>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Peter Zilstra <peterz@infradead.org>,
        Andy Lutomirski <luto@kernel.org>,
        Andrew Cooper <Andrew.Cooper3@citrix.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Martin Pohlack <mpohlack@amazon.de>,
        "Li, Aubrey" <aubrey.li@intel.com>, Ashok Raj <ashok.raj@intel.com>
Subject: Re: [Patch v3 Part2 1/9] x86/microcode: Taint kernel only if
 microcode loading was successful
Message-ID: <Y9reDitxHgzcrsOY@a4bf019067fa.jf.intel.com>
References: <20230130213955.6046-1-ashok.raj@intel.com>
 <20230130213955.6046-2-ashok.raj@intel.com>
 <Y9kAlKFfdek2rq6g@zn.tnic>
 <Y9lHDWjjnqdletL3@a4bf019067fa.jf.intel.com>
 <Y9l4FdvytLqMc3jm@zn.tnic>
 <Y9mcCxYK/VPB0ATM@a4bf019067fa.jf.intel.com>
 <Y9perwfaheZqAJWl@zn.tnic>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y9perwfaheZqAJWl@zn.tnic>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?BKMsckp8HH9CtMZCOpN4Rl15IeI9RSyvWj23L7+Z7xkllq+4LbFPUY8SA8O4?=
 =?us-ascii?Q?TILluiu5JesZKToM/Me2+War2Pk//qrd9ecUeH7ADZ1c9kzJYbBsgwwo4V3F?=
 =?us-ascii?Q?4eGY8MMsfIgMXbjpwFxuBH7aM2MdPBoWX/MkrWselv3gJkxEaF1VKK6OiGTR?=
 =?us-ascii?Q?EO7IhEjenl2rv9p5VnSProgWLjkqn10AJdxBMf/nSlCwVXVURCqofpy/ZZ26?=
 =?us-ascii?Q?xqQv7omD09/RjNIBkGKr21UM9HE596/oC2/tiZaFqjCOYlv1ZZMauS+MeLpL?=
 =?us-ascii?Q?8/6/CNmLgMsGh/2Od7iO38lzvHjsTz6q4Gz03+ewP+tqNRyREwWNoUVIXq46?=
 =?us-ascii?Q?QtUati2bbcGAl74d60talJWbX9YUhaM2ZIcVgQCVQ5UaeFyjvZVWWuP+uh2W?=
 =?us-ascii?Q?SXG72YBluvsHanohU/Sp+zC8yIbtwQJsVdyc+WRiggB2iI5poGPfOzwx5wNV?=
 =?us-ascii?Q?TCUR/rvenERvMg880nbVrcWPov/h4r/PkIWzijXWaaZnw9GsA5cxwsAkbhbz?=
 =?us-ascii?Q?TZ3r4fCSG+vKb/o72sTYfyNmTRcerZu28rCP5iBwZMJ/Clcbt/TTGxQ07NF+?=
 =?us-ascii?Q?kpI9II8T6/DqERfncy7giX7J6kp4RBzAsF5CowiuvGOl4QIKTQqd1ZFHIoEH?=
 =?us-ascii?Q?ptDWQqYFtnfyav7S0BkjehSOPGj+bZDlQzW15dt4gZ9TrKtfEtkb3MzpjE1j?=
 =?us-ascii?Q?VWgw2VeL6TcI3OWDzXUVRxq35JCJX3BjUTQF+obwJPEgAKipiStQUhvsiFZm?=
 =?us-ascii?Q?fH4WyTWI/mk6tfsNNIoSMBShnuL9Rq/syV1HL6Qq8VBRKWVhRn4F8AMIOyCd?=
 =?us-ascii?Q?yWskv1cQd5bJozjV49FY+GM7kbIL3VxrfMvuDRiXEvXRPtcJzznQN3NuVqKh?=
 =?us-ascii?Q?65H+Tqt2iT/xaQY2+aZs3GvfMXBSxKuJgtMsZU9CwIags2ZxNLjQ87SjMVjM?=
 =?us-ascii?Q?tfnc3YLdCtgN/AO2BG9cVyyZyhM9G8s0EgSi4Esl6coGV8X4JleTrB+l0Fbi?=
 =?us-ascii?Q?3ZB9N6vvJE8WDUtE/udB6VkTqmhF05Ge/8Ubh/0AkjcPHASfs5b6TLY7+i03?=
 =?us-ascii?Q?1CZVmONYMDM0fOE/M23ja24u2X8dISG/6HJ3jYAKBnVmLqbrqWSprDTL0BRB?=
 =?us-ascii?Q?DEa4Jzra+R/4hQ8TshhEHhC5ymwR+4+fOhoUmxWK6OZBDQdmHB5HEH4Dbi9u?=
 =?us-ascii?Q?SuOs1eXbkmdzMmURLWpO2IPN2b/sinlvx1aD/XGI9UHDj6T3641/VlqPcGP5?=
 =?us-ascii?Q?70rhKgvy5ej8QRFf+837+x9V3G09UeYzsidJWamkduoRKHwevtNJNZp/Vhgi?=
 =?us-ascii?Q?TSQguGNw8Q3blkhpyME1+0/WhEkpvDogIE9y5V1xRLVWksisdLLWJA385V9s?=
 =?us-ascii?Q?4T3Szbbvf2d7Z7TOzeXJjQClHH1hRr6SsJ5xzNWQ7ktwBIwsxISae4g14Kpe?=
 =?us-ascii?Q?3nhOrw1vknUalI+U68Gy//vM20HfLca6PYooZHSxeod63VzvJPZmrpMv4Baj?=
 =?us-ascii?Q?mQLnyVodl9xUoFbr+2zp9VjzzyzKrUvat+Ubu1SOxuMdt2N4xKktzJMuI9nC?=
 =?us-ascii?Q?x9RI/dzhQ0BGTvPMsPNZXkInHJPlauXgGHzDXwUg9aV7yPDf/O5We0rIPC7S?=
 =?us-ascii?Q?uw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 35bc16aa-95c8-49a0-7caf-08db049dfd80
X-MS-Exchange-CrossTenant-AuthSource: SJ1PR11MB6201.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2023 21:48:01.5596
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vkNoVbxkGd7W8w8YIgN3ooK+PCnZdrlQ5VdXhxFC0/rz06uLzrX8Yk7arAke7oLlJOEjo5bd6QcKo0mlRD8MNw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6118
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Boris,

While reworking I thought while at this, there is a chance to fix other
things.


On Wed, Feb 01, 2023 at 01:44:31PM +0100, Borislav Petkov wrote:
> On Tue, Jan 31, 2023 at 02:54:03PM -0800, Ashok Raj wrote:
> > It's not an error, only when request_microcode() returns UCODE_ERROR, should
> > it return -EINVAL,
> 
> So looking at ->request_microcode_fw(), it looks like we return
> UCODE_ERROR when something with parsing the blob has gone wrong. So I
> guess we can return something more fitting here to state that we failed
> while parsing the microcode blob from userspace: it is corrupted,
> truncated, what not.
> 
> Looking at the error codes, this:
> 
> #define ELIBBAD         80      /* Accessing a corrupted shared library */
> 
> seems fitting as it has "corrupted" blob in the definition. EBADF sounds
> fitting too. In any case, it should be a distinct error value which
> hints at what goes wrong.

Along the same lines, when check_online_cpu() should we return -EBUSY which
would distinguish between -EINVAL vs something different?

And we have a pr_err() to indicate not all CPUs are online. I suppose this
can be dropped in the same patch to fix return code? Since the error code
should be indicative of the problem?

pr_err("Not all CPUs online, aborting microcode update.\n");

> 
> > This shouldn't be noisy, but if you think this isn't needed, it can go
> > away.
> 
> I think all this preemptive development - it might make sense so let's
> do it - needs to stop. If there's an *actual* real use and need for it
> sure, but let's issue a printk just because is not one of them.
> 
> > When it fails due to current_rev < min_rev, Isn't it good to add indication
> > to user space that it didn't succeed? Thomas wanted these return codes, so
> > someone scripting can get a status after an attempt to load.
> 
> Return codes: yes. Random, flaky, potentially overwritten in the dmesg
> ring buffer error strings - nope. Soon someone will come along and say,
> "hey, don't touch those printk formats - my tool parses them and it'll
> break if you do." Yeah, right.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette