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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

> +     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.

> 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).

> +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 ?

> +CPIO=$images/microcode.x86.$date.cpio

Also if you set a variable like CPIO you risk something executing
$CPIO rather than /usr/bin/cpio :-).

> +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.

> +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"

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

> +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 ?


Thanks,
Ian.

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