On Wed, Jan 31, 2018 at 05:12:03PM +0100, Patrick Wildt wrote:
> Hi,
>
> this diff allows us to load the Intel microcode much earlier. So far
> we load it after the CPUs have identified and then have to update the
> CPU flags afterwards. This is not good since we have to assume that
> those updates can remove and add instructions and other features. We
> need to load it earlier. The only other option is to have the boot-
> blocks load the ucode for us.
>
> One issue though is actually loading and passing the binaries that can
> range from 2k to 90k of binary size. As far as I know we don't use the
> lower 16M of memory on amd64, which the bootblocks use as heap. Thus
> allocating a buffer for using alloc() should be "fine" if we make sure
> that we read the ucode after we early enough. kettenis@ tells me that
> cpu_startup() is the earliest MD code which can use malloc(9), so that
> is where we can copy the ucode from the bootloader to kernel land.
>
> I have tested this with efiboot(8), more tests would be appreciated.
>
> Thanks,
> Patrick
>
I like the idea. We should probably make sure it works with sr crypto also,
I can check that later today unless someone beats me to it.
After that, I'll review the diff more carefully. Are you looking for oks yet,
or is this just being passed around for comments at this time?
Thanks!
-ml
> diff --git a/sys/arch/amd64/amd64/cpu.c b/sys/arch/amd64/amd64/cpu.c
> index 8a779aa65eb..cb1b50947ba 100644
> --- a/sys/arch/amd64/amd64/cpu.c
> +++ b/sys/arch/amd64/amd64/cpu.c
> @@ -406,21 +406,20 @@ cpu_attach(struct device *parent, struct device *self,
> void *aux)
> printf("(uniprocessor)\n");
> ci->ci_flags |= CPUF_PRESENT | CPUF_SP | CPUF_PRIMARY;
> cpu_intr_init(ci);
> + cpu_ucode_apply(ci);
> identifycpu(ci);
> #ifdef MTRR
> mem_range_attach();
> #endif /* MTRR */
> cpu_init(ci);
> cpu_init_mwait(sc);
> -#ifndef SMALL_KERNEL
> - config_mountroot(NULL, cpu_ucode_attachhook);
> -#endif
> break;
>
> case CPU_ROLE_BP:
> printf("apid %d (boot processor)\n", caa->cpu_apicid);
> ci->ci_flags |= CPUF_PRESENT | CPUF_BSP | CPUF_PRIMARY;
> cpu_intr_init(ci);
> + cpu_ucode_apply(ci);
> identifycpu(ci);
> #ifdef MTRR
> mem_range_attach();
> @@ -438,9 +437,6 @@ cpu_attach(struct device *parent, struct device *self,
> void *aux)
> ioapic_bsp_id = caa->cpu_apicid;
> #endif
> cpu_init_mwait(sc);
> -#ifndef SMALL_KERNEL
> - config_mountroot(NULL, cpu_ucode_attachhook);
> -#endif
> break;
>
> case CPU_ROLE_AP:
> @@ -698,6 +694,7 @@ cpu_hatch(void *v)
>
> lapic_enable();
> lapic_startclock();
> + cpu_ucode_apply(ci);
>
> if ((ci->ci_flags & CPUF_IDENTIFIED) == 0) {
> /*
> @@ -738,8 +735,6 @@ cpu_hatch(void *v)
> lldt(0);
>
> cpu_init(ci);
> - cpu_ucode_apply(ci);
> - cpu_flags_update(ci);
> #if NPVBUS > 0
> pvbus_init_cpu();
> #endif
> @@ -939,20 +934,3 @@ cpu_activate(struct device *self, int act)
>
> return (0);
> }
> -
> -void
> -cpu_flags_update(struct cpu_info *ci)
> -{
> - uint32_t dummy;
> -
> - /* XXX this update is much later than we want it to be */
> - if (cpuid_level >= 0x07) {
> - CPUID_LEAF(0x7, 0, dummy, dummy, dummy,
> - ci->ci_feature_sefflags_edx);
> - }
> - if (!strcmp(cpu_vendor, "AuthenticAMD") &&
> - ci->ci_pnfeatset >= 0x80000008) {
> - CPUID(0x80000008, dummy, ci->ci_feature_amdspec_ebx,
> - dummy, dummy);
> - }
> -}
> diff --git a/sys/arch/amd64/amd64/machdep.c b/sys/arch/amd64/amd64/machdep.c
> index 33d1d38395b..d3320282998 100644
> --- a/sys/arch/amd64/amd64/machdep.c
> +++ b/sys/arch/amd64/amd64/machdep.c
> @@ -241,6 +241,7 @@ bios_diskinfo_t *bios_diskinfo;
> bios_memmap_t *bios_memmap;
> u_int32_t bios_cksumlen;
> bios_efiinfo_t *bios_efiinfo;
> +bios_ucode_t *bios_ucode;
>
> /*
> * Size of memory segments, before any memory is stolen.
> @@ -308,6 +309,7 @@ cpu_startup(void)
>
> /* Safe for i/o port / memory space allocation to use malloc now. */
> x86_bus_space_mallocok();
> + cpu_ucode_setup();
> }
>
> /*
> @@ -1886,6 +1888,10 @@ getbootinfo(char *bootinfo, int bootinfo_size)
> docninit++;
> break;
>
> + case BOOTARG_UCODE:
> + bios_ucode = (bios_ucode_t *)q->ba_arg;
> + break;
> +
> default:
> #ifdef BOOTINFO_DEBUG
> printf(" unsupported arg (%d) %p", q->ba_type,
> diff --git a/sys/arch/amd64/amd64/ucode.c b/sys/arch/amd64/amd64/ucode.c
> index ceec53dafea..908e40f96ba 100644
> --- a/sys/arch/amd64/amd64/ucode.c
> +++ b/sys/arch/amd64/amd64/ucode.c
> @@ -19,10 +19,14 @@
> #include <sys/param.h>
> #include <sys/systm.h>
> #include <sys/mutex.h>
> +#include <sys/malloc.h>
> +
> +#include <uvm/uvm_extern.h>
>
> #include <machine/cpu.h>
> #include <machine/cpufunc.h>
> #include <machine/specialreg.h>
> +#include <machine/biosvar.h>
>
> /* #define UCODE_DEBUG */
> #ifdef UCODE_DEBUG
> @@ -65,7 +69,7 @@ struct intel_ucode_ext_sig {
> char * cpu_ucode_data;
> size_t cpu_ucode_size;
>
> -void cpu_ucode_setup(struct cpu_info *);
> +void cpu_ucode_setup(void);
> void cpu_ucode_apply(struct cpu_info *);
>
> /* Intel */
> @@ -81,32 +85,18 @@ struct intel_ucode_header *cpu_ucode_intel_applied;
> struct mutex cpu_ucode_intel_mtx =
> MUTEX_INITIALIZER(IPL_HIGH);
>
> void
> -cpu_ucode_attachhook(struct device *dv)
> +cpu_ucode_setup(void)
> {
> - struct cpu_info *ci = curcpu();
> -
> - cpu_ucode_setup(ci);
> - cpu_ucode_apply(ci);
> - cpu_flags_update(ci);
> -}
> -
> -void
> -cpu_ucode_setup(struct cpu_info *ci)
> -{
> - char name[128];
> - u_char *ucode;
> - size_t size;
> -
> - snprintf(name, sizeof(name), "intel/%02x-%02x-%02x", ci->ci_family,
> - ci->ci_model, (ci->ci_signature >> 0) & 0x0f);
> + if (bios_ucode == NULL)
> + return;
>
> - if (loadfirmware(name, &ucode, &size) != 0) {
> - DPRINTF(("%s: no microcode found: %s\n", __func__, name));
> + if (!bios_ucode->uc_addr || !bios_ucode->uc_size)
> return;
> - }
>
> - cpu_ucode_data = ucode;
> - cpu_ucode_size = size;
> + cpu_ucode_size = bios_ucode->uc_size;
> + cpu_ucode_data = malloc(cpu_ucode_size, M_DEVBUF, M_WAITOK);
> + memcpy(cpu_ucode_data, (void *)PMAP_DIRECT_MAP(bios_ucode->uc_addr),
> + cpu_ucode_size);
> }
>
> /*
> diff --git a/sys/arch/amd64/include/biosvar.h
> b/sys/arch/amd64/include/biosvar.h
> index dcb59d766ca..9c7f3b5e7b4 100644
> --- a/sys/arch/amd64/include/biosvar.h
> +++ b/sys/arch/amd64/include/biosvar.h
> @@ -206,6 +206,12 @@ typedef struct _bios_efiinfo {
> uint32_t fb_reserved_mask;
> } __packed bios_efiinfo_t;
>
> +#define BOOTARG_UCODE 12
> +typedef struct _bios_ucode {
> + uint64_t uc_addr;
> + uint64_t uc_size;
> +} __packed bios_ucode_t;
> +
> #if defined(_KERNEL) || defined (_STANDALONE)
>
> #ifdef _LOCORE
> @@ -254,6 +260,7 @@ bios_diskinfo_t *bios_getdiskinfo(dev_t);
> extern u_int bootapiver;
> extern bios_memmap_t *bios_memmap;
> extern bios_efiinfo_t *bios_efiinfo;
> +extern bios_ucode_t *bios_ucode;
>
> #endif /* _KERNEL */
> #endif /* _LOCORE */
> diff --git a/sys/arch/amd64/include/cpufunc.h
> b/sys/arch/amd64/include/cpufunc.h
> index a86ac711a80..67398c5f077 100644
> --- a/sys/arch/amd64/include/cpufunc.h
> +++ b/sys/arch/amd64/include/cpufunc.h
> @@ -314,9 +314,8 @@ breakpoint(void)
> }
>
> void amd64_errata(struct cpu_info *);
> -void cpu_flags_update(struct cpu_info *);
> +void cpu_ucode_setup(void);
> void cpu_ucode_apply(struct cpu_info *);
> -void cpu_ucode_attachhook(struct device *);
>
> #endif /* _KERNEL */
>
> diff --git a/sys/arch/amd64/stand/boot/conf.c
> b/sys/arch/amd64/stand/boot/conf.c
> index 6bb7271de00..b00fec88696 100644
> --- a/sys/arch/amd64/stand/boot/conf.c
> +++ b/sys/arch/amd64/stand/boot/conf.c
> @@ -40,7 +40,7 @@
> #include <biosdev.h>
> #include <dev/cons.h>
>
> -const char version[] = "3.33";
> +const char version[] = "3.34";
> int debug = 1;
>
>
> diff --git a/sys/arch/amd64/stand/cdboot/conf.c
> b/sys/arch/amd64/stand/cdboot/conf.c
> index 29088565aa8..acca472568a 100644
> --- a/sys/arch/amd64/stand/cdboot/conf.c
> +++ b/sys/arch/amd64/stand/cdboot/conf.c
> @@ -41,7 +41,7 @@
> #include <biosdev.h>
> #include <dev/cons.h>
>
> -const char version[] = "3.28";
> +const char version[] = "3.29";
> int debug = 1;
>
>
> diff --git a/sys/arch/amd64/stand/efiboot/conf.c
> b/sys/arch/amd64/stand/efiboot/conf.c
> index b91d7485dea..7c0e8361d6b 100644
> --- a/sys/arch/amd64/stand/efiboot/conf.c
> +++ b/sys/arch/amd64/stand/efiboot/conf.c
> @@ -39,7 +39,7 @@
> #include "efidev.h"
> #include "efipxe.h"
>
> -const char version[] = "3.37";
> +const char version[] = "3.38";
>
> #ifdef EFI_DEBUG
> int debug = 0;
> diff --git a/sys/arch/amd64/stand/libsa/exec_i386.c
> b/sys/arch/amd64/stand/libsa/exec_i386.c
> index eecefb2c2d8..335109aeab4 100644
> --- a/sys/arch/amd64/stand/libsa/exec_i386.c
> +++ b/sys/arch/amd64/stand/libsa/exec_i386.c
> @@ -33,8 +33,10 @@
> #include <dev/cons.h>
> #include <lib/libsa/loadfile.h>
> #include <machine/biosvar.h>
> +#include <machine/specialreg.h>
> #include <stand/boot/bootarg.h>
>
> +#include "cmd.h"
> #include "disk.h"
> #include "libsa.h"
>
> @@ -51,6 +53,9 @@
> typedef void (*startfuncp)(int, int, int, int, int, int, int, int)
> __attribute__ ((noreturn));
>
> +void ucode_load(void);
> +extern struct cmd_state cmd;
> +
> char *bootmac = NULL;
>
> void
> @@ -102,6 +107,8 @@ run_loadfile(u_long *marks, int howto)
> bcopy(bootdev_dip->disklabel.d_uid, &bootduid.duid, sizeof(bootduid));
> addbootarg(BOOTARG_BOOTDUID, sizeof(bootduid), &bootduid);
>
> + ucode_load();
> +
> #ifdef SOFTRAID
> if (bootdev_dip->sr_vol != NULL) {
> bv = bootdev_dip->sr_vol;
> @@ -160,3 +167,54 @@ run_loadfile(u_long *marks, int howto)
> #endif
> /* not reached */
> }
> +
> +void
> +ucode_load(void)
> +{
> + uint32_t model, family, stepping;
> + uint32_t dummy, signature;
> + uint32_t vendor[4];
> + bios_ucode_t uc;
> + struct stat sb;
> + char path[128];
> + size_t buflen;
> + char *buf;
> + int fd;
> +
> + CPUID(0, dummy, vendor[0], vendor[2], vendor[1]);
> + vendor[3] = 0; /* NULL-terminate */
> + if (strcmp((char *)vendor, "GenuineIntel") != 0)
> + return;
> +
> + CPUID(1, signature, dummy, dummy, dummy);
> + family = (signature >> 8) & 0x0f;
> + model = (signature >> 4) & 0x0f;
> + if (family == 0x6 || family == 0xf) {
> + family += (signature >> 20) & 0xff;
> + model += ((signature >> 16) & 0x0f) << 4;
> + }
> + stepping = (signature >> 0) & 0x0f;
> +
> + snprintf(path, sizeof(path), "%s:/etc/firmware/intel/%02x-%02x-%02x",
> + cmd.bootdev, family, model, stepping);
> +
> + fd = open(path, 0);
> + if (fd == -1)
> + return;
> +
> + if (fstat(fd, &sb) == -1)
> + return;
> +
> + buflen = sb.st_size;
> + if ((buf = alloc(buflen)) == NULL)
> + return;
> +
> + if (read(fd, buf, buflen) != buflen) {
> + free(buf, buflen);
> + return;
> + }
> +
> + uc.uc_addr = (uint64_t)buf;
> + uc.uc_size = (uint64_t)buflen;
> + addbootarg(BOOTARG_UCODE, sizeof(uc), &uc);
> +}
> diff --git a/sys/arch/amd64/stand/pxeboot/conf.c
> b/sys/arch/amd64/stand/pxeboot/conf.c
> index 4e2484374c3..0e23f9f52c9 100644
> --- a/sys/arch/amd64/stand/pxeboot/conf.c
> +++ b/sys/arch/amd64/stand/pxeboot/conf.c
> @@ -43,7 +43,7 @@
> #include "pxeboot.h"
> #include "pxe_net.h"
>
> -const char version[] = "3.28";
> +const char version[] = "3.29";
> int debug = 0;
>
> void (*sa_cleanup)(void) = pxe_shutdown;
>
