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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.04.14 at 22:30, <andrew.cooper3@xxxxxxxxxx> 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel