On Mon, Feb 05, 2018 at 11:02:27AM +1300, Patrick Wildt wrote:
> 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
>
> Updated diff. RAMDISK compiles and runs through make release without
> any further issues.
>
> This diff also enables the debug printfs, so people testing this diff
> should see something happening in dmesg. I will not commit that diff
> with the prints enabled.
>
> Please report back. Some more stuff should be seen when doing
>
> $ dmesg | grep ucode
>
> Please not that you have to build and install the bootloader in addtion
> to installing a new kernel.
>
> Thanks,
> Patrick
>
I did a single test with sr crypto and didn't see any issues, fwiw.
-ml
> diff --git a/sys/arch/amd64/amd64/cpu.c b/sys/arch/amd64/amd64/cpu.c
> index 8a779aa65eb..5ec7e7f6409 100644
> --- a/sys/arch/amd64/amd64/cpu.c
> +++ b/sys/arch/amd64/amd64/cpu.c
> @@ -406,21 +406,24 @@ 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);
> +#ifndef SMALL_KERNEL
> + cpu_ucode_apply(ci);
> +#endif
> 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);
> +#ifndef SMALL_KERNEL
> + cpu_ucode_apply(ci);
> +#endif
> identifycpu(ci);
> #ifdef MTRR
> mem_range_attach();
> @@ -438,9 +441,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 +698,7 @@ cpu_hatch(void *v)
>
> lapic_enable();
> lapic_startclock();
> + cpu_ucode_apply(ci);
>
> if ((ci->ci_flags & CPUF_IDENTIFIED) == 0) {
> /*
> @@ -738,8 +739,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 +938,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..940716c174c 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,10 @@ cpu_startup(void)
>
> /* Safe for i/o port / memory space allocation to use malloc now. */
> x86_bus_space_mallocok();
> +
> +#ifndef SMALL_KERNEL
> + cpu_ucode_setup();
> +#endif
> }
>
> /*
> @@ -1886,6 +1891,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..7b807125782 100644
> --- a/sys/arch/amd64/amd64/ucode.c
> +++ b/sys/arch/amd64/amd64/ucode.c
> @@ -19,12 +19,16 @@
> #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 */
> +#define UCODE_DEBUG
> #ifdef UCODE_DEBUG
> #define DPRINTF(x) do { if (cpu_ucode_debug > 0) printf x; } while (0)
> #define DPRINTFN(n, x) do { if (cpu_ucode_debug >= (n)) printf x; }
> while (0)
> @@ -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);
> }
>
> /*
> @@ -126,8 +116,10 @@ cpu_ucode_intel_apply(struct cpu_info *ci)
> uint32_t old_rev, new_rev;
> paddr_t data;
>
> - if (cpu_ucode_data == NULL || cpu_ucode_size == 0)
> + if (cpu_ucode_data == NULL || cpu_ucode_size == 0) {
> + DPRINTF(("%s: no microcode provided\n", __func__));
> return;
> + }
>
> /*
> * Grab a mutex, because we are not allowed to run updates
> @@ -140,10 +132,14 @@ cpu_ucode_intel_apply(struct cpu_info *ci)
> if (update == NULL)
> update = cpu_ucode_intel_find(cpu_ucode_data,
> cpu_ucode_size, old_rev);
> - if (update == NULL)
> + if (update == NULL) {
> + DPRINTF(("%s: no microcode update found\n", __func__));
> goto out;
> - if (update->update_revision == old_rev)
> + }
> + if (update->update_revision == old_rev) {
> + DPRINTF(("%s: microcode already up-to-date\n", __func__));
> goto out;
> + }
>
> /* Apply microcode. */
> data = (paddr_t)update;
> 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;
>
