[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Eslam Elnikety <elnikety@xxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 9 Dec 2019 15:19:27 +0000
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=andrew.cooper3@xxxxxxxxxx; spf=Pass smtp.mailfrom=Andrew.Cooper3@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx
  • Autocrypt: addr=andrew.cooper3@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFLhNn8BEADVhE+Hb8i0GV6mihnnr/uiQQdPF8kUoFzCOPXkf7jQ5sLYeJa0cQi6Penp VtiFYznTairnVsN5J+ujSTIb+OlMSJUWV4opS7WVNnxHbFTPYZVQ3erv7NKc2iVizCRZ2Kxn srM1oPXWRic8BIAdYOKOloF2300SL/bIpeD+x7h3w9B/qez7nOin5NzkxgFoaUeIal12pXSR Q354FKFoy6Vh96gc4VRqte3jw8mPuJQpfws+Pb+swvSf/i1q1+1I4jsRQQh2m6OTADHIqg2E ofTYAEh7R5HfPx0EXoEDMdRjOeKn8+vvkAwhviWXTHlG3R1QkbE5M/oywnZ83udJmi+lxjJ5 YhQ5IzomvJ16H0Bq+TLyVLO/VRksp1VR9HxCzItLNCS8PdpYYz5TC204ViycobYU65WMpzWe LFAGn8jSS25XIpqv0Y9k87dLbctKKA14Ifw2kq5OIVu2FuX+3i446JOa2vpCI9GcjCzi3oHV e00bzYiHMIl0FICrNJU0Kjho8pdo0m2uxkn6SYEpogAy9pnatUlO+erL4LqFUO7GXSdBRbw5 gNt25XTLdSFuZtMxkY3tq8MFss5QnjhehCVPEpE6y9ZjI4XB8ad1G4oBHVGK5LMsvg22PfMJ ISWFSHoF/B5+lHkCKWkFxZ0gZn33ju5n6/FOdEx4B8cMJt+cWwARAQABtClBbmRyZXcgQ29v cGVyIDxhbmRyZXcuY29vcGVyM0BjaXRyaXguY29tPokCOgQTAQgAJAIbAwULCQgHAwUVCgkI CwUWAgMBAAIeAQIXgAUCWKD95wIZAQAKCRBlw/kGpdefoHbdD/9AIoR3k6fKl+RFiFpyAhvO 59ttDFI7nIAnlYngev2XUR3acFElJATHSDO0ju+hqWqAb8kVijXLops0gOfqt3VPZq9cuHlh IMDquatGLzAadfFx2eQYIYT+FYuMoPZy/aTUazmJIDVxP7L383grjIkn+7tAv+qeDfE+txL4 SAm1UHNvmdfgL2/lcmL3xRh7sub3nJilM93RWX1Pe5LBSDXO45uzCGEdst6uSlzYR/MEr+5Z JQQ32JV64zwvf/aKaagSQSQMYNX9JFgfZ3TKWC1KJQbX5ssoX/5hNLqxMcZV3TN7kU8I3kjK mPec9+1nECOjjJSO/h4P0sBZyIUGfguwzhEeGf4sMCuSEM4xjCnwiBwftR17sr0spYcOpqET ZGcAmyYcNjy6CYadNCnfR40vhhWuCfNCBzWnUW0lFoo12wb0YnzoOLjvfD6OL3JjIUJNOmJy RCsJ5IA/Iz33RhSVRmROu+TztwuThClw63g7+hoyewv7BemKyuU6FTVhjjW+XUWmS/FzknSi dAG+insr0746cTPpSkGl3KAXeWDGJzve7/SBBfyznWCMGaf8E2P1oOdIZRxHgWj0zNr1+ooF /PzgLPiCI4OMUttTlEKChgbUTQ+5o0P080JojqfXwbPAyumbaYcQNiH1/xYbJdOFSiBv9rpt TQTBLzDKXok86LkCDQRS4TZ/ARAAkgqudHsp+hd82UVkvgnlqZjzz2vyrYfz7bkPtXaGb9H4 Rfo7mQsEQavEBdWWjbga6eMnDqtu+FC+qeTGYebToxEyp2lKDSoAsvt8w82tIlP/EbmRbDVn 7bhjBlfRcFjVYw8uVDPptT0TV47vpoCVkTwcyb6OltJrvg/QzV9f07DJswuda1JH3/qvYu0p vjPnYvCq4NsqY2XSdAJ02HrdYPFtNyPEntu1n1KK+gJrstjtw7KsZ4ygXYrsm/oCBiVW/OgU g/XIlGErkrxe4vQvJyVwg6YH653YTX5hLLUEL1NS4TCo47RP+wi6y+TnuAL36UtK/uFyEuPy wwrDVcC4cIFhYSfsO0BumEI65yu7a8aHbGfq2lW251UcoU48Z27ZUUZd2Dr6O/n8poQHbaTd 6bJJSjzGGHZVbRP9UQ3lkmkmc0+XCHmj5WhwNNYjgbbmML7y0fsJT5RgvefAIFfHBg7fTY/i kBEimoUsTEQz+N4hbKwo1hULfVxDJStE4sbPhjbsPCrlXf6W9CxSyQ0qmZ2bXsLQYRj2xqd1 bpA+1o1j2N4/au1R/uSiUFjewJdT/LX1EklKDcQwpk06Af/N7VZtSfEJeRV04unbsKVXWZAk uAJyDDKN99ziC0Wz5kcPyVD1HNf8bgaqGDzrv3TfYjwqayRFcMf7xJaL9xXedMcAEQEAAYkC HwQYAQgACQUCUuE2fwIbDAAKCRBlw/kGpdefoG4XEACD1Qf/er8EA7g23HMxYWd3FXHThrVQ HgiGdk5Yh632vjOm9L4sd/GCEACVQKjsu98e8o3ysitFlznEns5EAAXEbITrgKWXDDUWGYxd pnjj2u+GkVdsOAGk0kxczX6s+VRBhpbBI2PWnOsRJgU2n10PZ3mZD4Xu9kU2IXYmuW+e5KCA vTArRUdCrAtIa1k01sPipPPw6dfxx2e5asy21YOytzxuWFfJTGnVxZZSCyLUO83sh6OZhJkk b9rxL9wPmpN/t2IPaEKoAc0FTQZS36wAMOXkBh24PQ9gaLJvfPKpNzGD8XWR5HHF0NLIJhgg 4ZlEXQ2fVp3XrtocHqhu4UZR4koCijgB8sB7Tb0GCpwK+C4UePdFLfhKyRdSXuvY3AHJd4CP 4JzW0Bzq/WXY3XMOzUTYApGQpnUpdOmuQSfpV9MQO+/jo7r6yPbxT7CwRS5dcQPzUiuHLK9i nvjREdh84qycnx0/6dDroYhp0DFv4udxuAvt1h4wGwTPRQZerSm4xaYegEFusyhbZrI0U9tJ B8WrhBLXDiYlyJT6zOV2yZFuW47VrLsjYnHwn27hmxTC/7tvG3euCklmkn9Sl9IAKFu29RSo d5bD8kMSCYsTqtTfT6W4A3qHGvIDta3ptLYpIAOD2sY3GYq2nf3Bbzx81wZK14JdDDHUX2Rs 6+ahAA==
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, Paul Durrant <pdurrant@xxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, David Woodhouse <dwmw@xxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Mon, 09 Dec 2019 15:19:40 +0000
  • Ironport-sdr: BoDT2vNx72J/3cOnl9ZZFYLz/1orw0XIE73pb1iYYPJ4TnZMXMYsDdJq0qma5O1jwPuoLPZTVH PmbIpT0bppd6b9F7rCgJj0mQn6MBrgnVItaqaFaE5+rb7UOK45Igg/NxH27fW1sweYZ+gehg48 g1oCLky9yxYX7XF8RXgqgg8bRvdzehLwRLwdo+mwKQOgoknk2kApIFxjJfJt9RxLOy1ZdC0AbL 7EHNKwHiB2/PZ9sh40CRsDwbLJNosuCqjPAEvH+qLUfr3VFku3LXbdWw4nRVJqH2b7AAkY3eYa FHY=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 09/12/2019 08:41, Eslam Elnikety wrote:
> diff --git a/docs/misc/builtin-ucode.txt b/docs/misc/builtin-ucode.txt
> new file mode 100644
> index 0000000000..43bb60d3eb

Instead of introducing a new file, please extend
docs/admin-guide/microcode-loading.rst

I have an in-prep docs/hypervisor-guide/microcode-loading.rst as well,
which I'll see about posting.  It would be a more appropriate place for
the technical details.

> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index 6ced293d88..7afbe44286 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -97,6 +97,14 @@ static struct ucode_mod_blob __initdata ucode_blob;
>   */
>  static bool_t __initdata ucode_scan;
>  
> +#ifdef CONFIG_BUILTIN_UCODE
> +/* builtin is the default when BUILTIN_UCODE is set */
> +static bool_t __initdata ucode_builtin = 1;

bool, true.

> +
> +extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[];
> +extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[];
> +#endif
> +
>  /* By default, ucode loading is done in NMI handler */
>  static bool ucode_in_nmi = true;
>  
> @@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int idx)
>  }
>  
>  /*
> - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are
> - * optional. If the EFI has forced which of the multiboot payloads is to be
> - * used, only nmi=<bool> is parsed.
> + * The format is '[<integer>|scan=<bool>|builtin=<bool>, nmi=<bool>]'. All
> + * options are optional. If the EFI has forced which of the multiboot 
> payloads
> + * is to be used, only nmi=<bool> is parsed.
>   */

Please delete this, or I'll do a prereq patch to fix it and the command
line docs.  (Both are in a poor state.)

> @@ -237,6 +249,48 @@ void __init microcode_grab_module(
>  scan:
>      if ( ucode_scan )
>          microcode_scan_module(module_map, mbi);
> +
> +#ifdef CONFIG_BUILTIN_UCODE
> +    /*
> +     * Do not use the builtin microcode if:
> +     * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin)
> +     * (b) a microcode module has been specified or a scan is successful
> +     */
> +    if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size )
> +        return;
> +
> +    /* Set ucode_start/_end to the proper blob */
> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> +        ucode_blob.size = (size_t)(__builtin_amd_ucode_end
> +                                   - __builtin_amd_ucode_start);

No need to cast the result.  Also, preferred Xen style would be to have
- on the preceding line.

> +    else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> +        ucode_blob.size = (size_t)(__builtin_intel_ucode_end
> +                                   - __builtin_intel_ucode_start);
> +    else
> +        return;
> +
> +    if ( !ucode_blob.size )
> +    {
> +        printk("No builtin ucode! 'ucode=builtin' is nullified.\n");
> +        return;
> +    }
> +    else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE )
> +    {
> +        printk("Builtin microcode payload too big! (%ld, we can do %d)\n",
> +               ucode_blob.size, MAX_EARLY_CPIO_MICROCODE);
> +        ucode_blob.size = 0;
> +        return;
> +    }
> +
> +    ucode_blob.data = xmalloc_bytes(ucode_blob.size);
> +    if ( !ucode_blob.data )
> +        return;

Any chance we can reuse the "fits" logic to avoid holding every
inapplicable blob in memory as well?

> +
> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> +        memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size);
> +    else
> +        memcpy(ucode_blob.data, __builtin_intel_ucode_start, 
> ucode_blob.size);
> +#endif
>  }
>  
>  const struct microcode_ops *microcode_ops;
> diff --git a/xen/arch/x86/microcode/Makefile b/xen/arch/x86/microcode/Makefile
> new file mode 100644
> index 0000000000..6d585c5482
> --- /dev/null
> +++ b/xen/arch/x86/microcode/Makefile
> @@ -0,0 +1,40 @@
> +# 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.
> +
> +obj-y += builtin_ucode.o
> +
> +# Directory holding the microcode updates.
> +UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR))
> +amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*)
> +intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*)

This is a little dangerous.  I can see why you want to do it like this,
and I can't provide any obvious suggestions, but if this glob picks up
anything which isn't a microcode file, it will break the logic to search
for the right blob.

> +
> +builtin_ucode.o: Makefile $(amd-blobs) $(intel-blobs)
> +     # Create AMD microcode blob if there are AMD updates on the build system
> +     if [ ! -z "$(amd-blobs)" ]; then \
> +             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 $@.amd; \
> +             rm -f $@.bin; \
> +     fi
> +     # Create INTEL microcode blob if there are INTEL updates on the build 
> system
> +     if [ ! -z "$(intel-blobs)" ]; then \
> +             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 $@.intel; \
> +             rm -f $@.bin; \
> +     fi
> +     # Create fake builtin_ucode.o if no updates were present. Otherwise, 
> builtin_ucode.o carries the available updates
> +     if [ -z "$(amd-blobs)" -a -z "$(intel-blobs)" ]; then \
> +             $(CC) $(CFLAGS) -c -x c /dev/null -o $@; \
> +     else \
> +             $(LD) $(LDFLAGS) -r -o $@ $@.*; \
> +             rm -f $@.*; \
> +     fi

How about using weak symbols, rather than playing games like this?

~Andrew

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.