[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 4/28/2014 3:49 AM, Jan Beulich wrote:
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.


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

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.