Re: [PATCH] x86, amd_ucode: Verify max allowed patch size before apply

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

On 4/28/2014 3:49 AM, Jan Beulich wrote:
>>>> On 25.04.14 at 22:30, <andrew.cooper3@citrix.com> 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