On Wed, Jan 31, 2018 at 09:46:52AM -0800, Mike Larkin 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
> >
>
> 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.
That would be nice!
> 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?
I can start collecting OKs I guess. :) I have no other changes pending
for this, so as long as there is no negative feedback I intend to commit
this at some point. Before doing that, I want to collect a bit of feed-
back and test results.
> Thanks!
No, thank you and Philip for the hard work during the last few weeks!
Patrick
> -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;
> >
>
