Re: [Xen-devel] [PATCH v2 4/4] x86/microcode: Support builtin CPU microcode

[Eslam Elnikety <elnikety@xxxxxxxxxx>]

On 20.12.19 11:12, Jan Beulich wrote:
> On 19.12.2019 23:11, Eslam Elnikety wrote:
> > On 18.12.19 13:42, Jan Beulich wrote:
> > > On 18.12.2019 02:32, Eslam Elnikety wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/x86/microcode/Makefile
> > > > @@ -0,0 +1,46 @@
> > > > +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates.
> > > > +# Author: Eslam Elnikety <elnikety@xxxxxxxxxx>
> > > > +#
> > > > +# This program is free software; you can redistribute it and/or modify
> > > > +# it under the terms of the GNU General Public License as published by
> > > > +# the Free Software Foundation; either version 2 of the License, or
> > > > +# (at your option) any later version.
> > > > +#
> > > > +# This program is distributed in the hope that it will be useful,
> > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > +# GNU General Public License for more details.
> > > > +
> > > > +# Remove quotes and excess spaces from configuration strings
> > > > +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
> > > > +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
> > > > +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
> > > > +
> > > > +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
> > > > +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
> > > > +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
> > > > +
> > > > +ifneq ($(amd-blobs),)
> > > > +obj-y += ucode_amd.o
> > > > +endif
> > > > +
> > > > +ifneq ($(intel-blobs),)
> > > > +obj-y += ucode_intel.o
> > > > +endif
> > > > +
> > > > +ifeq ($(amd-blobs)$(intel-blobs),)
> > > > +obj-y += ucode_dummy.o
> > > > +endif
> > > > +
> > > > +ucode_amd.o: Makefile $(amd-blobs)
> > > > +       cat $(amd-blobs) > $@.bin
> > > > +       $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section 
> > > > .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@
> > > > +       rm -f $@.bin
> > > > +
> > > > +ucode_intel.o: Makefile $(intel-blobs)
> > > > +       cat $(intel-blobs) > $@.bin
> > > > +       $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section 
> > > > .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@
> > > > +       rm -f $@.bin
> > > This can be had with a pattern rule (with the vendor being the stem)
> > > and hence without duplication, I think.
> > >
> > > Also - is simply concatenating the blobs reliable enough? There's no
> > > build time diagnostic that the result would actually be understood
> > > at runtime.
> > Concatenation is reliable (as long as the individual microcode blobs are
> > not malformed, and in that case the builtin is not making matters worse
> > compared to presenting the malformed update via <integer> | scan).
> A malformed update found the other way is a bug in the tools
> constructing the respective images. A malformed built-in
> update is a bug in the Xen build system. The put the question
> differently: Is it specified somewhere that the blobs all have
> to have certain properties, which the straight concatenation
> relies upon?
Refer to ( https://www.kernel.org/doc/Documentation/x86/microcode.txt ). 
AuthenticAMD.bin and GenuineIntel.bin are both concatenations of 
individual microcode blobs.
> > > > +ucode_dummy.o: Makefile
> > > > +       $(CC) $(CFLAGS) -c -x c /dev/null -o $@;
> > > Since the commit message doesn't explain why this is needed, I
> > > have to ask (I guess we somewhere have a dependency on $(obj-y)
> > > not being empty).
> > Your guess is correct. All sub-directories of xen/arch/x86 are expected
> > to produce built_in.o. If there are not amd nor intel microcode blobs,
> > there will be no build dependencies and the build fails preparing the
> > built_in.o
> That's rather poor, but it's of course not your task to get this
> fixed (it shouldn't be very difficult to create an empty
> built_in.o for an empty $(obj-y)).
>
> > > _If_ it is needed, I don't see why you need
> > > ifeq() around its use. In fact you could have
> > >
> > > obj-y := ucode-dummy.o
> > >
> > > right at the top of the file.
> > >
> > > Furthermore I don't really understand why you need this in the
> > > first place. While cat won't do what you want with an empty
> > > argument list, can't you simply prepend / append /dev/null?
> > To make sure we are on the same page. You are suggesting using
> > "/dev/null" in case there are no amd/intel ucode to generate the
> > ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as
> > input (complains about empty binary).
> That's again rather poor, this time of the utility - it should be
> easy enough to produce an object with an empty .data (or whatever
> it is) section. As above - I'm fine with you keeping the logic
> then as is, provided you say in the description why it can't be
> simplified.
Ack. Will justify the logic in comments.

-- Eslam

> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel