Re: [Xen-devel] [PATCH v2] microcode: Scan the initramfs payload for microcode blob.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.13 at 02:50, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> On Wed, Aug 07, 2013 at 03:09:43PM +0100, Jan Beulich wrote:
>> >>> On 18.07.13 at 18:36, Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx> wrote:
>> > The option to turn this scan on/off is gated by the 'ucode'
>> > parameter. The options are now:
>> >  'initrd'    Scan for the microcode in any multiboot payload.
>> >  <index>     Attempt to load microcode blob (not the cpio archive
>> >              format) from the multiboot payload number.
>> > 
>> > This option alters slightly the 'ucode' parameter by now allowing
>> > two parameters:
>> >   ucode=[<index>,][initrd]
>> 
>> I don't really like this - it should permit a number _or_ "initrd", not
>> both (as that's meaningless).
> 
> You could have have a microcode blob (with microcode v1), and there
> *might* have a blob in initramfs (with microcode v2 or without). Since
> the 'initrd' scans first - if it does not find it in the initramfs
> it will fallback to the microcode blob (which might have slightly older
> microcode blob data or perhaps is corrupted and does not have it).
> 
> Maybe my example is too contrived for reality.

As said elsewhere - which of the two is "better" can't be known.
Hence let's avoid the ambiguity (and possible surprise) by being
explicit in allowing either, but not both at the same time.

>> Also I think the term "initrd" usually refers to the old style variant
>> not using cpio format (see init/initramfs.c:populate_rootfs() in the
>> Linux sources), hence "initramfs", "cpio", or "initrd-cpio" might be
>> better to avoid confusion.
> 
> Perhaps then just 'scan' to have it all under one option?

Well, "scan" is a little too wide a term. Otoh you don't really look
at only the initrd, you indeed scan all modules, so perhaps "scan"
indeed is the right name. The only thing I'm (mildly) worried about
then is how we'd name an eventual extension to scan other than
cpio format modules.

>> > +static __initdata const char * const ucodes[] = 
>> > {"kernel/x86/microcode/GenuineIntel.bin",
>> 
>> Some gcc versions will cause errors mixing constant and non-constant
>> data placed in the same overridden section. Please drop the second
>> const.
>> 
>> > +                                                 
>> > "kernel/x86/microcode/AuthenticAMD.bin"};
>> 
>> I wonder anyway whether we wouldn't be better off using CPUID
>> function 0 output here to generate the name, and drop this pretty
>> contrived array approach (you don't use the array as such
>> anyway, i.e. you could as well inline the uses).
> 
> We can do that, however - we would have to use an snprintf and such - 
> which would make the code a bit lengthier (or maybe not, since the CPUID
> value is of fixed size).
> 
> How about for this patch we just do inline the uses and then
> in another fortcomming patch I can try the cpuid approach and see
> how that pans out?

Fine with me. The main request really was to get rid of the
array that's never really used as such.

>> >  static void __init _do_microcode_update(unsigned long data)
>> >  {
>> > -    microcode_update_cpu((void *)data, ucode_mod.mod_end);
>> > +    void *_data = (void *)data;
>> > +    size_t len = ucode_mod.mod_end;
>> > +
>> > +    if ( ucode_blob.size )
>> > +    {
>> > +        _data = (void *)ucode_blob.data;
>> > +        len = ucode_blob.size;
>> 
>> Here it becomes very visible why allowing both methods at the
>> same time is bad: What tells you that what you found in the
>> cpio is better than what was specified as an explicit module?
> 
> Perhaps then try both? And whichever one that the underlaying
> platform code would like to use -  that is the one we will stick
> with for the SMP case?

So what if the first try finds a usable ucode update (newer than
what got loaded by firmware), yet the second one would have
been even more recent? Of course you could fully run through
both update cycles, but again - I'd favor avoiding the ambiguity
and explicitly allow only for either method.

Jan

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