From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply Date: Mon, 28 Apr 2014 10:48:12 -0500 Message-ID: <535E783C.50102@amd.com> References: <1398369287-2501-1-git-send-email-aravind.gopalakrishnan@amd.com> <5359738A.6050607@citrix.com> <535ABBF6.1060303@amd.com> <535AC5F7.404@citrix.com> <535E324E020000780000CB4D@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <535E324E020000780000CB4D@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Andrew Cooper Cc: Thomas.Lendacky@amd.com, keir@xen.org, Suravee.Suthikulpanit@amd.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 4/28/2014 3:49 AM, Jan Beulich wrote: >>>> On 25.04.14 at 22:30, wrote: >> On 25/04/14 20:48, Aravind Gopalakrishnan wrote: >>> Could someone please help me understand why there are two initcalls - >>> 'microcode_presmp_init' && 'microcode_init' >>> that perform the same tasks? >>> >>> It results in these functions - 'collect_cpu_info', >>> 'cpu_request_microcode' to run a second time through 'microcode_init' >>> when we have already accomplished the job of updating cpu with >>> microcode patches >>> through 'microcode_presmp_init' and 'microcode_resume_cpu' >>> >>> If there is particular reason for 'microcode_init''s existence, then >>> I'll modify the patch so that the error handling around 'microcode_fits' >>> is not altered. But if not, then I simply have to just remove the >>> 'break' statement. >> Good grief this looks mad. Ignoring for a moment what the code actually >> does, lets consider what it should do. >> >> The administrator has an option of providing a microcode blob in a >> multiboot module, and instructing Xen where to look. In this case if a >> valid blob is found, Xen will want to load the microcode ASAP and cache >> it for the other processors. This can be done as early as is suitable >> on the BSP. >> >> The administrator has an option of providing a microcode blob by >> hypercall (either automatically as part of their kernel microcode >> patching code, or perhaps from a userspace tool in dom0). For a valid >> blob, Xen will want to cache it and apply to all active processors. >> This can be done by whichever cpu processes the hypercall. >> >> At any point, a new cpu coming online (AP boot, S3 etc) should look at >> the cached microcode and try to load it. >> >> >> So it would occur to me to have something such as as presmp initcall >> which sets up a cpunotifier for any cpu coming online. It checks to see >> whether there are any blobs provided at boot, tries to load them and >> caches the best result. The hypercall tries to apply the blob from >> userspace and if successful, caches the new microcode and instructs all >> other cpus to load this new microcode. >> >> I dont see a need for two initcalls in the slightest, nor do I see a >> need for using tasklets on boot. If the earlier initcall hasn't found >> any microcode in a multiboot module then there will be nothing for the >> latter one to do. If the earlier did find microcode, then it should be >> applied automatically whenever a new cpu comes up, rather than when >> explicitly prodded by the later initcall. > The code indeed looks more complicated than one would think it > needs to be, but simplifying it would likely require adding another, > later CPU notification that is being issued _on_ the CPU being > brought up, or an IPI issued from e.g. the CPU_ONLINE callback > (but I'm rather uncertain about it being a good idea to do the ucode > update in an interrupt handler). The whole purpose of the later > initcall and the use of a tasklet there is to have a vehicle to get code > executed _on_ the new CPU, with CPU_STARTING being too early to > hook onto. > > Okay, So let me just modify the patch to make sure I don't change original behavior. and worry about simplifying this code at a later time.. Will send out a V2.. Thanks Jan, Andrew -Aravind