Subject: Re: [PATCH] x86: Unbreak early processor microcode loading

On Thu, 19 Mar 2015, Borislav Petkov wrote:
> On Thu, Mar 19, 2015 at 03:30:23PM +0800, Daniel J Blueman wrote:
> > Neat; I added the 'amd-ucode/' directory prefix to the string and adjusted
> > the buffer size, and the microcode is loaded,
>
> We shouldn't need to do that though for obvious reasons.
>
> IOW, you need to type in only the filenames of the microcode blobs in Kconfig
> option:
>
> CONFIG_EXTRA_FIRMWARE="microcode_amd.bin microcode_amd_fam15h.bin microcode_amd_fam16h.bin"
>
> and then add the absolute path to them with:
>
> CONFIG_EXTRA_FIRMWARE_DIR="/lib/firmware/amd-ucode"
>
> That worked here yesterday.

I apologise for the late reply. Please disregard it if it is outdated.

That would obviously work, but people often have other firmware
built-in, so the likehood of CONFIG_EXTRA_FIRMWARE_DIR pointing to the
root of a linux-firmware work tree or to "/lib/firmware" is not low at
all.

In fact, it is natural to expect that CONFIG_EXTRA_FIRMWARE should point
to something that will result in the same filenames as the kernel would
want to use for regular firmware loading. The current text of the
Kconfig help for CONFIG_EXTRA_FIRMWARE even says so.

So, FWIW, I do think we should always use the same path for builtin and
regular firmware requests, based on the least-surprise principle.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh


2015-04-29 18:44:04

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH] x86: Unbreak early processor microcode loading

On Wed, Apr 29, 2015 at 03:23:57PM -0300, Henrique de Moraes Holschuh wrote:
> That would obviously work, but people often have other firmware
> built-in, so the likehood of CONFIG_EXTRA_FIRMWARE_DIR pointing to the
> root of a linux-firmware work tree or to "/lib/firmware" is not low at
> all.
>
> In fact, it is natural to expect that CONFIG_EXTRA_FIRMWARE should point
> to something that will result in the same filenames as the kernel would
> want to use for regular firmware loading. The current text of the
> Kconfig help for CONFIG_EXTRA_FIRMWARE even says so.

You mean that:

"These files should exist under
the directory specified by the EXTRA_FIRMWARE_DIR option, which is
by default the firmware subdirectory of the kernel source tree.

For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
the usb8388.bin file into the firmware directory, and build the kernel.
Then any request_firmware("usb8388.bin") will be satisfied internally
without needing to call out to userspace."

So people with lotsa firmware should put it all under one directory and
all should just work.

> So, FWIW, I do think we should always use the same path for builtin and
> regular firmware requests, based on the least-surprise principle.

We don't hardcode the path - only the name. What I gave was an example
only.

The regular microcode updates, i.e. the late ones, use firmware class
which has a bunch of hardcoded paths:

static const char * const fw_path[] = {
fw_path_para,
"/lib/firmware/updates/" UTS_RELEASE,
"/lib/firmware/updates",
"/lib/firmware/" UTS_RELEASE,
"/lib/firmware"
};

and that's different from CONFIG_FIRMWARE_DIR.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Subject: Re: [PATCH] x86: Unbreak early processor microcode loading

On Wed, 29 Apr 2015, Borislav Petkov wrote:
> On Wed, Apr 29, 2015 at 03:23:57PM -0300, Henrique de Moraes Holschuh wrote:
> > That would obviously work, but people often have other firmware
> > built-in, so the likehood of CONFIG_EXTRA_FIRMWARE_DIR pointing to the
> > root of a linux-firmware work tree or to "/lib/firmware" is not low at
> > all.
> >
> > In fact, it is natural to expect that CONFIG_EXTRA_FIRMWARE should point
> > to something that will result in the same filenames as the kernel would
> > want to use for regular firmware loading. The current text of the
> > Kconfig help for CONFIG_EXTRA_FIRMWARE even says so.
>
> You mean that:
>
> "These files should exist under
> the directory specified by the EXTRA_FIRMWARE_DIR option, which is
> by default the firmware subdirectory of the kernel source tree.
>
> For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
> the usb8388.bin file into the firmware directory, and build the kernel.
> Then any request_firmware("usb8388.bin") will be satisfied internally
> without needing to call out to userspace."
>
> So people with lotsa firmware should put it all under one directory and
> all should just work.

That's one possible reading, yes. Only, it will break for some drivers.

The full Kconfig text of the two most-relevant kconfig defines are:

config EXTRA_FIRMWARE
This option allows firmware to be built into the kernel for the
case where the user either cannot or doesn't want to provide it
from userspace at runtime (for example, when the firmware in
question is required for accessing the boot device, and the user
doesn't want to use an initrd).

This option is a string and takes the (space-separated) names of
the firmware files -- the same names that appear in
MODULE_FIRMWARE() and request_firmware() in the source. These
files should exist under the directory specified by the
EXTRA_FIRMWARE_DIR option, which is by default the firmware
subdirectory of the kernel source tree.

For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin",
copy the usb8388.bin file into the firmware directory, and build
the kernel. Then any request_firmware("usb8388.bin") will be
satisfied internally without needing to call out to userspace.

WARNING: If you include additional firmware files into your
binary kernel image that are not available under the terms of
the GPL, then it may be a violation of the GPL to distribute the
resulting image since it combines both GPL and non-GPL work. You
should consult a lawyer of your own before distributing such an
image.

config EXTRA_FIRMWARE_DIR
This option controls the directory in which the kernel build
system looks for the firmware files listed in the EXTRA_FIRMWARE
option. The default is firmware/ in the kernel source tree, but
by changing this option you can point it elsewhere, such as
/lib/firmware/ or some other directory containing the firmware
files.

Note the explicit mention of /lib/firmware as a possibility for
CONFIG_EXTRA_FIRMWARE_DIR, as well as the second paragraph in the
CONFIG_EXTRA_FIRMWARE help text.

This help text needs some love...

Things only work for the drivers that do something like
MODULE_FIRMWARE("foo/bar.fw") when you consider that by "names of the
firmware files" the help text actually means "relative path to the
firmware files".

For the drivers do MODULE_FIRMWARE("foo/bar.fw"), should you place the
firmware file directly in the root of EXTRA_FIRMWARE_DIR (built-in case)
or in the root of the firmware search path for the non-builtin case
(i.e., if drop the "foo/"), it is not going to be found by
request_firmware().

That's why IMHO the least-suprise path would be for builtin microcode to
be searched with the same relative path as it would for late loading.
Because that's how regular drivers that do request_firmware() behave.

FWIW, my preference would be for the early microcode core to look for
builtin firmware using the same names used for the early initramfs
(including the kernel/x86/microcode/ prefix), since it is doing an early
update. But that would certainly require documentation.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

2015-04-29 21:45:29

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH] x86: Unbreak early processor microcode loading

On Wed, Apr 29, 2015 at 05:54:29PM -0300, Henrique de Moraes Holschuh wrote:
> That's why IMHO the least-suprise path would be for builtin microcode to
> be searched with the same relative path as it would for late loading.
> Because that's how regular drivers that do request_firmware() behave.

Ok, I'll add "amd-ucode/" and "intel-ucode/" as prefixes to the built-in
microcode loading path.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--