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; >