Re: [PATCH OSSTEST] Arrange to upgrade microcode on x86 test hosts.

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-06-11 at 16:39 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST] Arrange to upgrade microcode on x86 test hosts."):
> > Both Xen and Linux support extracting a microcode update from an
> > initramfs early during boot. This requires prepending a suitable
> > uncompressed cpio archive containing the necessary files to the
> > initrd.
> 
> Looking good...
> 
> > +sub preseed_microcode($$)
> > +{
> > +    my ($ho,$sfx) = @_;
> > +    my $prop = "MicrocodeUpdate".ucfirst($r{arch});
> > +    logm("ucode=$prop $c{$prop}");
> > +    return unless $c{$prop};
> 
> If $c{prop} is undef, the logm will produce a warning.  Maybe you want
> //'' somewhere.

I think that was a stray wip debug thing, I could fix, remove or move it
below the check of $c{$prop}. 

> 
> > +	sub {
> > +	    my $f = $c{Images}."/".$c{$prop};
> 
> This is slighly unidiomatic.  I would write "$c{Images}/$c{$prop}".
> But what you have isn't wrong.

I had that and then switched, for some reason. I'll go back.

> > diff --git a/mg-cpu-microcode-update b/mg-cpu-microcode-update
> > new file mode 100755
> > index 0000000..dd87838
> > --- /dev/null
> > +++ b/mg-cpu-microcode-update
> > @@ -0,0 +1,82 @@
> > +#!/bin/bash
> > +
> > +set -e
> > +
> > +. cri-getconfig
> > +
> > +images=`getconfig Images`
> > +date=`date +%Y-%m-%d`
> > +
> > +TMP=`mktemp -t -d mg-cpu-microcode-update.XXXXXX`
> 
> I think it would be better to use ./tmp (or maybe bits of the
> destination, the way mg-debian-installer-update does).

OK.

> > +CPIODIR=cpio.x86
> > +UCODEPATH=kernel/x86/microcode
> > +
> > +INTELBIN=GenuineIntel.bin
> > +AMDBIN=AuthenticAMD.bin
> 
> This use of caps lock for unexported shell variables is rather
> unidiomatic, don't you think ?

I'm not really in the habit of caring for short scripts, but I'll down
case them.

> 
> > +CPIO=$images/microcode.x86.$date.cpio
> 
> Also if you set a variable like CPIO you risk something executing
> $CPIO rather than /usr/bin/cpio :-).

True. I'll call it UCODECPIO^Wucodecpio or some such.

> > +LINUX_FW=https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git
> > +
> > +if [ -f $CPIO ] ;then
> > +    echo "error: $CPIO already exists." >&2
> > +    echo "Refusing to overwrite" >&2
> > +    exit 1
> > +fi
> 
> mg-debian-installer-update is idempotent.  Perhaps this script should
> be too ?  After all you can only overwrite something generated today.

True. I'll arrange that.

I'll see if I can also make it deterministic like with the installer
stuff, which will make spotting differences easier.

> > +tar -C intel-ucode -xaf intel-ucode/microcode.tgz microcode.dat
> > +
> > +echo >&2 "Converting Intel ucode"
> > +/usr/sbin/iucode-tool \
> 
> Put sbin on the PATH, rather than hardcoding:
> 
> PATH="/usr/local/sbin:$PATH:/sbin:/usr/sbin"

Ack.

> Also we should file a bug complaining that this tool should be in
> /usr/bin.

I'll do that.

> > +for BIN in $AMD_BINS ; do
> > +    curl -s $LINUX_FW/plain/$BIN > $BIN
> > +done
> 
> We're really fetching these via the gitweb/cgit/... ?  Isn't that a
> bit fragile ?

A bit, yes. The alternative is to clone the whole of linux-firmware.git.
I suppose with --depth=1 that might not be so bad. Would you prefer
that?

Ian.