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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

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