On 12.11.23 19:10, Borislav Petkov wrote:
> On Sun, Nov 12, 2023 at 04:03:32PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>> That's because dracut until the recent commit
>> https://github.com/dracutdevs/dracut/commit/6c80408c8644a0add1907b0593eb83f90d6247b1
>> looked for CONFIG_MICROCODE_AMD and CONFIG_MICROCODE_INTEL in the config
>> file to decide what to include or not.
>
> They've been told a bunch of times already that grepping .config for
> specific symbols is not how one should check whether one should add
> microcode blobs to the initrd or not because Kconfig symbols are not an
> ABI.
Maybe, but you know how Linus sees things like this: what's considered
an ABI/API or not is nearly[1] irrelevant - if a change breaks something
that used to work then it needs to be fixed.
[1] unless you fiddle with things obviously internal; not sure if this
case would qualify for him, but somehow I doubt it -- but I might be
wrong there.
> And looking at that commit, now they're grepping for CONFIG_MICROCODE.
> And that'll break again if one day we decide to make the microcode
> loader built in unconditionally.
>
> How to fix this reliably and properly?
>
> Honestly, I don't have a good idea. If we do something like this:
> grep microcode_init System.map
>
> then that makes "microode_init" ABI and we won't be able to change it
> eva. I'd need to do some digging here...
Any progress on this?
BTW: I see that this could help preventing problems like the current one
to happen in the far future. But how would that help the current
situation (e.g. users that have an old dracut and updated the kernel
without updating dracut)?
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
#regzbot poke
On Wed, Nov 22, 2023 at 10:15:42AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> [1] unless you fiddle with things obviously internal; not sure if this
> case would qualify for him, but somehow I doubt it -- but I might be
> wrong there.
Well, think about it - by that logic, if CONFIG_* items are an ABI, we
will never ever be able to change any of them. Now that would be awful.
> Any progress on this?
We're thinking...
We might need an official scheme of stating what any given kernel image
supports for use by external tools which need it.
> BTW: I see that this could help preventing problems like the current one
> to happen in the far future. But how would that help the current
> situation (e.g. users that have an old dracut and updated the kernel
> without updating dracut)?
Update dracut too?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Nov 22, 2023 at 10:15 AM Linux regression tracking (Thorsten
Leemhuis) <[email protected]> wrote:
>
> On 12.11.23 19:10, Borislav Petkov wrote:
> > On Sun, Nov 12, 2023 at 04:03:32PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> >> That's because dracut until the recent commit
> >> https://github.com/dracutdevs/dracut/commit/6c80408c8644a0add1907b0593eb83f90d6247b1
> >> looked for CONFIG_MICROCODE_AMD and CONFIG_MICROCODE_INTEL in the config
> >> file to decide what to include or not.
> >
> > They've been told a bunch of times already that grepping .config for
> > specific symbols is not how one should check whether one should add
> > microcode blobs to the initrd or not because Kconfig symbols are not an
> > ABI.
>
> Maybe, but you know how Linus sees things like this: what's considered
> an ABI/API or not is nearly[1] irrelevant - if a change breaks something
> that used to work then it needs to be fixed.
>
> [1] unless you fiddle with things obviously internal; not sure if this
> case would qualify for him, but somehow I doubt it -- but I might be
> wrong there.
>
Thorsten, I think you are wrong here in this case. We are talking
about the kernel build configuration options and their names and these
are certainly not stable and never have been.
Some indication to show that the rate of change we are generally
talking about without anyone considering this stable ABI/API:
You can run "find . -name Kconfig* | xargs grep -h -E '^(menu)config '
| sort | uniq" on each kernel release. Then diff those lists of
configs with increasing kernel config versions.
If this would be stable, then only config options should appear and
little should disappear, but that is not the case. And if something
disappears, it should relate to a driver/feature that was removed, but
that is also not always the case.
Here are some quick numbers:
v6.0 to v6.1: 43 removals
v6.1 to v6.2: 40 removals
v6.2 to v6.3: 350 removals
v6.3 to v6.4: 86 removals
v6.4 to v6.5: 73 removals
v6.5 to v6.6: 61 removals
* Removals can also be potentially a renaming.
So, these config names are certainly not stable ABI/API. We can
investigate a bit deeper on which changes are due to driver removals,
which due to config removal but making a feature default and which are
simply a config renaming, but in the past, hardly any kernel developer
has considered this interface to be a special stable ABI/API.
Further, to my knowledge looking at kernel discussions and the
repository, there are currently no tools out there that would assist
in updating a kernel build configuration from one version to the next.
So, we are talking about roughly more than 50 removals to kernel
config options every release, and now there was this one special case
in one release, where a tool incorrectly relies on this one config
option to be stable. That is not a regression of a stable ABI/API,
that is a misuse of an internal non-stable ABI/API.
Lukas
Preface: considered CCing Linus here, as it's quite possible that I'm
wrong, as every situation is somewhat different. If anybody disagrees
with what I bring up below to hopefully clarify things thus please do me
a favor an CC Linus so he can clarify things.
Ohh, and sorry for being a PITA. I hate that, but when it comes to
regressions disagreements often happen, as all those discussions linked
at the end of https://docs.kernel.org/process/handling-regressions.html
illustrate.
On 22.11.23 12:58, Borislav Petkov wrote:
> On Wed, Nov 22, 2023 at 10:15:42AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>> [1] unless you fiddle with things obviously internal; not sure if this
>> case would qualify for him, but somehow I doubt it -- but I might be
>> wrong there.
>
> Well, think about it - by that logic, if CONFIG_* items are an ABI, we
> will never ever be able to change any of them. [...]
Can't follow your logic (or the one from Lukas in the other reply), as
what's an ABI (or an API) is afaik not the important factor when it
comes to the "no regressions" rule: you can change things (including
ABIs or APIs) all you want, as long as nothing breaks. To quote Linus from
https://lore.kernel.org/all/CAHk-=wiVi7mSrsMP=fLXQrXK_UimybW=ziLOwSzFTtoXUacWVQ@mail.gmail.com/
```
The rules about regressions have never been about any kind of
documented behavior, or where the code lives.
The rules about regressions are always about "breaks user workflow".
The other side of the coin is that people who talk about "API
stability" are entirely wrong. API's don't matter either. You can make
any changes to an API you like - as long as nobody notices.
Again, the regression rule is not about documentation, not about
API's, and not about the phase of the moon.
It's entirely about "we caused problems for user space that used to work".
```
>> BTW: I see that this could help preventing problems like the current one
>> to happen in the far future. But how would that help the current
>> situation (e.g. users that have an old dracut and updated the kernel
>> without updating dracut)?
> Update dracut too?
To quote Linus again, this time from
https://lore.kernel.org/lkml/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/
```
People should basically always feel like they can update their kernel
and simply not have to worry about it.
I refuse to introduce "you can only update the kernel if you also
update that other program" kind of limitations. If the kernel used to
work for you, the rule is that it continues to work for you.
There have been exceptions, but they are few and far between,
[...]
But if something actually breaks, then the change must get fixed or
reverted. And it gets fixed in the *kernel*. Not by saying "well, fix
your user space then".
```
Are those quotes fitting to the situation at hand? Not totally sure.
Initramfs generators might be special and we have done exceptions for
them in the past if no other solution could be found to prevent a
regression[1]. We'd need Linus to clarify.
Ciao, Thorsten
[1] maybe it's a naive idea, but can't we just avoid the problem at hand
by adding CONFIG_MICROCODE_AMD and CONFIG_MICROCODE_INTEL back as a
hidden config stub and remove those in ~3 years? Yeah, ugly, but we have
done things way more ugly than that to prevent regressions.
Sure,
lemme do that.
Hi Linus,
we have a disagreement on what is a userspace regression and what is
not.
The whole thread starts here:
https://lore.kernel.org/r/[email protected]
and I'm leaving Thorsten's arguments fully quoted below for more
context.
Basically, dracut has been grepping the kernel's .config to figure out
whether to add microcode blobs to the intird or not.
Now, we changed a CONFIG and it broke. Again. It wasn't the first time.
It went and fixed it this way:
https://github.com/dracutdevs/dracut/commit/6c80408c8644a0add1907b0593eb83f90d6247b1
which will break next time we change stuff.
IMO, yes, we should not break userspace but dracut is special. And it
parses willy nilly kernel internals which are not ABI to begin with.
Looking at that dracut function check_kernel_config(), it does:
# no kernel config file, so return true
[[ $_config_file ]] || return 0
if it can't find a kernel .config at the two places it looks for which
is just silly: if it can't find a .config just return true and include
those microcode blobs. Might as well hide the config as a fix. :-)
What it should do, is parse the .notes section of vmlinux for which
I have a proper fix:
https://lore.kernel.org/r/20231122132419.GBZV4BA399sG2JRFAJ@fat_crate.local
So IMNSVHO, CONFIG symbols are not an ABI.
If there's some other userspace tool which goes and greps the kernel
sources and looks for a particular function or symbol which is not even
exported, does that mean that we won't be able to change that function
name or symbol anymore just because some random tool touches it.
Yes, I know, we should not break userspace but there has to be some
sensible limit somewhere as to what constitutes a userspace breakage.
In the end of the day, that's your call.
If we consider this a userspace breakage, I would add back those
CONFIG_MICROCODE_INTEL and CONFIG_MICROCODE_AMD Kconfig symbols and
everytime I add a new CONFIG symbol, I should probably write a big fat
note above it that userspace should not rely on it existing forever...
Thx.
On Wed, Nov 22, 2023 at 04:34:03PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> Preface: considered CCing Linus here, as it's quite possible that I'm
> wrong, as every situation is somewhat different. If anybody disagrees
> with what I bring up below to hopefully clarify things thus please do me
> a favor an CC Linus so he can clarify things.
>
> Ohh, and sorry for being a PITA. I hate that, but when it comes to
> regressions disagreements often happen, as all those discussions linked
> at the end of https://docs.kernel.org/process/handling-regressions.html
> illustrate.
>
> On 22.11.23 12:58, Borislav Petkov wrote:
> > On Wed, Nov 22, 2023 at 10:15:42AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> >> [1] unless you fiddle with things obviously internal; not sure if this
> >> case would qualify for him, but somehow I doubt it -- but I might be
> >> wrong there.
> >
> > Well, think about it - by that logic, if CONFIG_* items are an ABI, we
> > will never ever be able to change any of them. [...]
>
> Can't follow your logic (or the one from Lukas in the other reply), as
> what's an ABI (or an API) is afaik not the important factor when it
> comes to the "no regressions" rule: you can change things (including
> ABIs or APIs) all you want, as long as nothing breaks. To quote Linus from
> https://lore.kernel.org/all/CAHk-=wiVi7mSrsMP=fLXQrXK_UimybW=ziLOwSzFTtoXUacWVQ@mail.gmail.com/
>
> ```
> The rules about regressions have never been about any kind of
> documented behavior, or where the code lives.
>
> The rules about regressions are always about "breaks user workflow".
>
> The other side of the coin is that people who talk about "API
> stability" are entirely wrong. API's don't matter either. You can make
> any changes to an API you like - as long as nobody notices.
>
> Again, the regression rule is not about documentation, not about
> API's, and not about the phase of the moon.
>
> It's entirely about "we caused problems for user space that used to work".
> ```
>
> >> BTW: I see that this could help preventing problems like the current one
> >> to happen in the far future. But how would that help the current
> >> situation (e.g. users that have an old dracut and updated the kernel
> >> without updating dracut)?
> > Update dracut too?
>
> To quote Linus again, this time from
> https://lore.kernel.org/lkml/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/
>
> ```
> People should basically always feel like they can update their kernel
> and simply not have to worry about it.
>
> I refuse to introduce "you can only update the kernel if you also
> update that other program" kind of limitations. If the kernel used to
> work for you, the rule is that it continues to work for you.
>
> There have been exceptions, but they are few and far between,
> [...]
> But if something actually breaks, then the change must get fixed or
> reverted. And it gets fixed in the *kernel*. Not by saying "well, fix
> your user space then".
> ```
>
> Are those quotes fitting to the situation at hand? Not totally sure.
> Initramfs generators might be special and we have done exceptions for
> them in the past if no other solution could be found to prevent a
> regression[1]. We'd need Linus to clarify.
>
> Ciao, Thorsten
>
> [1] maybe it's a naive idea, but can't we just avoid the problem at hand
> by adding CONFIG_MICROCODE_AMD and CONFIG_MICROCODE_INTEL back as a
> hidden config stub and remove those in ~3 years? Yeah, ugly, but we have
> done things way more ugly than that to prevent regressions.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Nov 22, 2023 at 12:58:26PM +0100, Borislav Petkov wrote:
> On Wed, Nov 22, 2023 at 10:15:42AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> > [1] unless you fiddle with things obviously internal; not sure if this
> > case would qualify for him, but somehow I doubt it -- but I might be
> > wrong there.
>
> Well, think about it - by that logic, if CONFIG_* items are an ABI, we
> will never ever be able to change any of them. Now that would be awful.
>
> > Any progress on this?
>
> We're thinking...
Turns out this is easier than I think and people have solved this
problem already - all I need to do is use it. Wonderful.
Lemme Cc [email protected] as an FYI and see whether dracut
folks would have any comments about this.
---
From: "Borislav Petkov (AMD)" <[email protected]>
Date: Wed, 22 Nov 2023 13:59:40 +0100
Add a "x86" ELF note namespace to put ELF note structures with which to
communicate in a ABI-compliant manner what a kernel image supports.
Also, add the first note type of this - X86_ELFNOTE_MICROCODE - which
denotes that microcode support is built into the kernel image and thus
initrd-generating tools like dracut can parse the ELF .notes section for
this.
$ readelf -n vmlinux
Displaying notes found in: .notes
Owner Data size Description
...
x86 0x00000004 Unknown note type: (0x00000000)
description data: 01 00 00 00
^^^^^^^^^^^^^^
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
---
arch/x86/include/uapi/elfnote.h | 18 ++++++++++++++++++
arch/x86/kernel/cpu/microcode/core.c | 5 +++++
2 files changed, 23 insertions(+)
create mode 100644 arch/x86/include/uapi/elfnote.h
diff --git a/arch/x86/include/uapi/elfnote.h b/arch/x86/include/uapi/elfnote.h
new file mode 100644
index 000000000000..bef26c4944e8
--- /dev/null
+++ b/arch/x86/include/uapi/elfnote.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _X86_UAPI_ELFNOTE_H_
+#define _X86_UAPI_ELFNOTE_H_
+
+/*
+ * "x86" namespaced ELF note structures to communicate features
+ * supported by the kernel binary to external utilities which need that
+ * info in order to do additional preparatory work based on the target
+ * kernel image.
+ */
+
+/*
+ * Used by the microcode loader to communicate support to external
+ * initrd generators like dracut.
+ */
+#define X86_ELFNOTE_MICROCODE 0
+
+#endif /* _X86_UAPI_ELFNOTE_H_ */
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 232026a239a6..f35444bafbbc 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -24,6 +24,7 @@
#include <linux/capability.h>
#include <linux/firmware.h>
#include <linux/cpumask.h>
+#include <linux/elfnote.h>
#include <linux/kernel.h>
#include <linux/delay.h>
#include <linux/mutex.h>
@@ -32,6 +33,8 @@
#include <linux/fs.h>
#include <linux/mm.h>
+#include <uapi/elfnote.h>
+
#include <asm/apic.h>
#include <asm/cpu_device_id.h>
#include <asm/perf_event.h>
@@ -859,3 +862,5 @@ static int __init microcode_init(void)
}
late_initcall(microcode_init);
+
+ELFNOTE32("x86", X86_ELFNOTE_MICROCODE, CONFIG_MICROCODE);
--
2.42.0.rc0.25.ga82fb66fed25
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, 22 Nov 2023 at 07:58, Borislav Petkov <[email protected]> wrote:
>
> IMO, yes, we should not break userspace but dracut is special. And it
> parses willy nilly kernel internals which are not ABI to begin with.
I don't think the "dracut is special" is the thing that matters.
The real issue is that hey, if dracut in its incompetence doesn't
include the microcode on the initrd, that doesn't really matter much.
It's fairly easily fixable, and at worst it will mean that we end up
having CPU mitigations that aren't optimal. Since most of those are BS
anyway, it really doesn't seem critical.
Sure, it's a "regression" in that you don't get the microcode update
included, but from a user perspective things should still continue to
work.
End result: this seems to be pretty solidly a distro issue.
IOW, the whole "users are the only thing that matters" pretty much
means that it's a non-issue. Things continued to work, to the point
that I'm actually surprised anybody even noticed.
That said, I don't think some ELF note is the fix either. I think we
might as well leave it at CONFIG_MICROCODE. Maybe add a note in the
kernel Kconfig that this thing matters for dracut.
Dracut also checks for CONFIG_ACPI_INITRD_TABLE_OVERRIDE. It's a
similar "normal users don't care".
Linus
On Wed, Nov 22, 2023 at 12:35:54PM -0800, Linus Torvalds wrote:
> IOW, the whole "users are the only thing that matters" pretty much
> means that it's a non-issue. Things continued to work, to the point
> that I'm actually surprised anybody even noticed.
Right, the patch which did the changes is in 6.6:
e6bcfdd75d53 ("x86/microcode: Hide the config knob")
I'm still waiting for the other shoe to drop when 6.6 gets used more but
we'll see...
> That said, I don't think some ELF note is the fix either. I think we
> might as well leave it at CONFIG_MICROCODE. Maybe add a note in the
> kernel Kconfig that this thing matters for dracut.
>
> Dracut also checks for CONFIG_ACPI_INITRD_TABLE_OVERRIDE. It's a
> similar "normal users don't care".
Ok.
My only worry here is that we're making a precedent and basically saying
that it is ok for tools to grep .config to figure out what is supported
by the kernel. And then other tools might follow.
I have no clue how many tools are actually interested in stuff enabled
in the kernel .config though. If only dracut then sure, don't care, but
what if it starts proliferating...
I'm just talking with my devil's advocate hat on.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, 22 Nov 2023 at 12:51, Borislav Petkov <[email protected]> wrote:
>
> My only worry here is that we're making a precedent and basically saying
> that it is ok for tools to grep .config to figure out what is supported
> by the kernel. And then other tools might follow.
Yes, I agree that it's not optimal, but I would hate to have some odd
"let's add another ELF note" churn too, for (presumably) increasingly
obscure reasons.
It looks like dracut has been doing this forever, and in fact back in
2015 apparently had the exact same issue (that never made it to kernel
developers, or at least not to me), when the kernel
CONFIG_MICROCODE_xyz_EARLY config went away, and became just
CONFIG_MICROCODE_xyz.
The whole "check kernel config" in dracut seems to go back to 2014, so
it's been that way for almost a decade by now.
Honestly, I think the right approach may be to just remove the check
again from dracut entirely - the intent seems to be to make the initrd
smaller when people don't support microcode updates, but does that
ever actually *happen*?
There are dracut command lines, like "--early-microcode" and
"--no-early-microcode", so people who really want to save space could
just force it that way. Doing the CONFIG_xyz check seems broken.
But that's for the dracut people to worry about.
I guess we on the kernel side could help with "make install" etc, but
we've (intentionally) tried to insulate us from distros having
distro-specific installkernel scripts, so we don't really haev a good
way to pass information down to the installkernel side.
It *would* make sense if we just had some actual arguments we might
pass down. Right now we just do
exec "${file}" "${KERNELRELEASE}" "${KBUILD_IMAGE}" System.map
"${INSTALL_PATH}"
so basically the only argument we pass down is that INSTALL_PATH
(which is just "/boot" by default).
Linus
Lemme add [email protected] to Cc again. I hope that's the
correct ML dracut folks use.
On Wed, Nov 22, 2023 at 01:08:41PM -0800, Linus Torvalds wrote:
> Yes, I agree that it's not optimal, but I would hate to have some odd
> "let's add another ELF note" churn too, for (presumably) increasingly
> obscure reasons.
Right, my angle with the ELF note is that it is at least something well
establshed and other things use it too (Xen, BUILD_SALT, other arches
too).
> It looks like dracut has been doing this forever, and in fact back in
> 2015 apparently had the exact same issue (that never made it to kernel
> developers, or at least not to me), when the kernel
> CONFIG_MICROCODE_xyz_EARLY config went away, and became just
> CONFIG_MICROCODE_xyz.
Yap, that was me. I merged the early loader because it didn't make any
sense to have a separate thing.
> The whole "check kernel config" in dracut seems to go back to 2014, so
> it's been that way for almost a decade by now.
>
> Honestly, I think the right approach may be to just remove the check
> again from dracut entirely - the intent seems to be to make the initrd
> smaller when people don't support microcode updates, but does that
> ever actually *happen*?
That thought also crossed my mind. With the mitigations sh*te, you
basically must build in microcode. Lemme cook up a dracut patch for this
tomorrow and see what happens.
> There are dracut command lines, like "--early-microcode" and
> "--no-early-microcode", so people who really want to save space could
> just force it that way. Doing the CONFIG_xyz check seems broken.
Yap, exactly.
> I guess we on the kernel side could help with "make install" etc, but
> we've (intentionally) tried to insulate us from distros having
> distro-specific installkernel scripts, so we don't really haev a good
> way to pass information down to the installkernel side.
>
> It *would* make sense if we just had some actual arguments we might
> pass down. Right now we just do
>
> exec "${file}" "${KERNELRELEASE}" "${KBUILD_IMAGE}" System.map
> "${INSTALL_PATH}"
>
> so basically the only argument we pass down is that INSTALL_PATH
> (which is just "/boot" by default).
Right, and on debian they run initramfs-tools as part of
a post-installation step at the end of /sbin/installkernel which could
then pass in more configuration info.
Yap, that could be one way to do it. We could document it in
scripts/install.sh or somewhere more prominent so that tools can look it
up.
Yap, all better ideas than parsing .config.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Adding Antonio who did that last fix to dracut:
6c80408c8644 ("fix(dracut.sh): remove microcode check based on CONFIG_MICROCODE_[AMD|INTEL]")
On Wed, Nov 22, 2023 at 01:08:41PM -0800, Linus Torvalds wrote:
> There are dracut command lines, like "--early-microcode" and
> "--no-early-microcode", so people who really want to save space could
> just force it that way. Doing the CONFIG_xyz check seems broken.
>
> But that's for the dracut people to worry about.
Yeah, I guess something like this below.
Antonio, how about something like the totally untested thing below?
dracut would simply always build in microcode - this is the majority of
the setups anyway - and people who want to save space, do:
--no-early-microcode
?
---
diff --git a/dracut.sh b/dracut.sh
index 3b292910f324..c0a88b083f8e 100755
--- a/dracut.sh
+++ b/dracut.sh
@@ -1561,20 +1561,16 @@ fi
if [[ $early_microcode == yes ]]; then
if [[ $hostonly ]]; then
- if [[ $(get_cpu_vendor) == "AMD" || $(get_cpu_vendor) == "Intel" ]]; then
- check_kernel_config CONFIG_MICROCODE || unset early_microcode
- else
+ if [[ $(get_cpu_vendor) != "AMD" && $(get_cpu_vendor) != "Intel" ]]; then
unset early_microcode
fi
- else
- ! check_kernel_config CONFIG_MICROCODE \
- && unset early_microcode
fi
+
# Do not complain on non-x86 architectures as it makes no sense
case "${DRACUT_ARCH:-$(uname -m)}" in
x86_64 | i?86)
[[ $early_microcode != yes ]] \
- && dwarn "Disabling early microcode, because kernel does not support it. CONFIG_MICROCODE!=y"
+ && dwarn "Disabling early microcode, unsupported configuration"
;;
*) ;;
esac
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
As a side note, complaints about this issue reached the kernel because most
distros out there didn't do their homework, as this patch has been merged
upstream since 6.6-rc1 was released. Fortunately, this problem does not break
the system boot.
As Linus said, the `check_kernel_config` stuff was implemented in 2014 and this
is not the only kernel config option that it's being checked by dracut
(CONFIG_ACPI_TABLE_UPGRADE, CONFIG_ACPI_INITRD_TABLE_OVERRIDE, CONFIG_RD_ZSTD),
although I agree that it's fragile if something changes. But adding in CC the
initramfs list (like you did), would be enough to prepare a simple fix in time.
On 23/11/2023 12.20, Borislav Petkov wrote:
> Adding Antonio who did that last fix to dracut:
>
> 6c80408c8644 ("fix(dracut.sh): remove microcode check based on CONFIG_MICROCODE_[AMD|INTEL]")
>
> On Wed, Nov 22, 2023 at 01:08:41PM -0800, Linus Torvalds wrote:
>> There are dracut command lines, like "--early-microcode" and
>> "--no-early-microcode", so people who really want to save space could
>> just force it that way. Doing the CONFIG_xyz check seems broken.
>>
>> But that's for the dracut people to worry about.
>
> Yeah, I guess something like this below.
>
> Antonio, how about something like the totally untested thing below?
>
> dracut would simply always build in microcode - this is the majority of
> the setups anyway - and people who want to save space, do:
>
> --no-early-microcode
>
> ?
The only problem I see in your patch is that we should also remove the
`--early-microcode` option, and dracut will fail if someone pass an option
available since 2013 (5f2c30d9bcd614d546d5c55c6897e33f88b9ab90) that would not
be recognized now (and by failing, I mean it will not build an initramfs if an
unrecognized option is passed).
Please, submit it to https://github.com/dracutdevs/dracut, so more people can
see it and discuss it. Thank you.
> ---
> diff --git a/dracut.sh b/dracut.sh
> index 3b292910f324..c0a88b083f8e 100755
> --- a/dracut.sh
> +++ b/dracut.sh
> @@ -1561,20 +1561,16 @@ fi
>
> if [[ $early_microcode == yes ]]; then
> if [[ $hostonly ]]; then
> - if [[ $(get_cpu_vendor) == "AMD" || $(get_cpu_vendor) == "Intel" ]]; then
> - check_kernel_config CONFIG_MICROCODE || unset early_microcode
> - else
> + if [[ $(get_cpu_vendor) != "AMD" && $(get_cpu_vendor) != "Intel" ]]; then
> unset early_microcode
> fi
> - else
> - ! check_kernel_config CONFIG_MICROCODE \
> - && unset early_microcode
> fi
> +
> # Do not complain on non-x86 architectures as it makes no sense
> case "${DRACUT_ARCH:-$(uname -m)}" in
> x86_64 | i?86)
> [[ $early_microcode != yes ]] \
> - && dwarn "Disabling early microcode, because kernel does not support it. CONFIG_MICROCODE!=y"
> + && dwarn "Disabling early microcode, unsupported configuration"
> ;;
> *) ;;
> esac
>
> Thx.
>
Best regards,
--
Antonio Álvarez Feijoo
System Boot and Init
SUSE
On Fri, Nov 24, 2023 at 09:49:27AM +0100, Antonio Feijoo wrote:
> As Linus said, the `check_kernel_config` stuff was implemented in 2014 and this
> is not the only kernel config option that it's being checked by dracut
> (CONFIG_ACPI_TABLE_UPGRADE, CONFIG_ACPI_INITRD_TABLE_OVERRIDE, CONFIG_RD_ZSTD),
> although I agree that it's fragile if something changes. But adding in CC the
> initramfs list (like you did), would be enough to prepare a simple fix in time.
Right, how about we give you a more reliable way to check functionality
built into the kernel instead of grepping the .config file?
Like the ELF note thing, for example:
https://lore.kernel.org/r/20231122132419.GBZV4BA399sG2JRFAJ@fat_crate.local
The current thing is not ABI and will break everytime we change
something and even if you fix it on time, older dracuts will still be
broken.
> The only problem I see in your patch is that we should also remove the
> `--early-microcode` option, and dracut will fail if someone pass an option
> available since 2013 (5f2c30d9bcd614d546d5c55c6897e33f88b9ab90) that would not
> be recognized now (and by failing, I mean it will not build an initramfs if an
> unrecognized option is passed).
Ah ok, --early-microcode becomes a no-op with my change. Sure.
> Please, submit it to https://github.com/dracutdevs/dracut, so more people can
> see it and discuss it. Thank you.
I presume I should read this first:
https://github.com/dracutdevs/dracut/blob/master/docs/HACKING.md
and send a github pull request?
Anything else I need to pay attention to when sending dracut patches?
Or is there also an old school mailing list where I can send the patch
to?
:-)
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 24/11/2023 13.15, Borislav Petkov wrote:
> On Fri, Nov 24, 2023 at 09:49:27AM +0100, Antonio Feijoo wrote:
>> As Linus said, the `check_kernel_config` stuff was implemented in 2014 and this
>> is not the only kernel config option that it's being checked by dracut
>> (CONFIG_ACPI_TABLE_UPGRADE, CONFIG_ACPI_INITRD_TABLE_OVERRIDE, CONFIG_RD_ZSTD),
>> although I agree that it's fragile if something changes. But adding in CC the
>> initramfs list (like you did), would be enough to prepare a simple fix in time.
>
> Right, how about we give you a more reliable way to check functionality
> built into the kernel instead of grepping the .config file?
>
> Like the ELF note thing, for example:
>
> https://lore.kernel.org/r/20231122132419.GBZV4BA399sG2JRFAJ@fat_crate.local
>
> The current thing is not ABI and will break everytime we change
> something and even if you fix it on time, older dracuts will still be
> broken.
The problem I see is having to add a new patch with a new note every time a
user space application requires new information to query. And also new dracuts
will be broken with older kernels that do not contain this info.
But (from a user space application point of view) if you (the kernel devs) are
ok with this approach, I don't see why we can at least get some info from there.
>
>> The only problem I see in your patch is that we should also remove the
>> `--early-microcode` option, and dracut will fail if someone pass an option
>> available since 2013 (5f2c30d9bcd614d546d5c55c6897e33f88b9ab90) that would not
>> be recognized now (and by failing, I mean it will not build an initramfs if an
>> unrecognized option is passed).
>
> Ah ok, --early-microcode becomes a no-op with my change. Sure.
>
>> Please, submit it to https://github.com/dracutdevs/dracut, so more people can
>> see it and discuss it. Thank you.
>
> I presume I should read this first:
>
> https://github.com/dracutdevs/dracut/blob/master/docs/HACKING.md
>
> and send a github pull request?
Yes, that would be enough.
> Anything else I need to pay attention to when sending dracut patches?
Just follow the Conventional Commit style for the commit messages, but that's
also specified in the HACKING.md doc.
> Or is there also an old school mailing list where I can send the patch
> to?
>
> :-)
Unfortunately no. All the development process was moved to github.
>
> Thx.
>
Thank you,
Best regards.
--
Antonio Álvarez Feijoo
System Boot and Init
SUSE
On Fri, Nov 24, 2023 at 02:15:02PM +0100, Antonio Feijoo wrote:
> The problem I see is having to add a new patch with a new note every
> time a user space application requires new information to query.
Yeah, this is how it should work. The mere addition of the note would
document that userspace uses a certain aspect of the kernel machinery
and then we will know not to break it.
> And also new dracuts will be broken with older kernels that do not
> contain this info.
Yes, the meaning of the absence of the note would be: the kernel doesn't
provide that functionality. If you want it, upgrade your kernel.
> But (from a user space application point of view) if you (the kernel
> devs) are ok with this approach, I don't see why we can at least get
> some info from there.
Yes, my goal is to have some machinery which both sides agree upon and
have that proper contract in place vs you guys using things which are
internal and us breaking them unknowingly.
But Linus isn't that crazy about the note thing so I'd prefer if he
comments on what we should do instead.
> Just follow the Conventional Commit style for the commit messages, but
> that's also specified in the HACKING.md doc.
Thx, lemme get cracking on fancy github workflows. :-P
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, Nov 24, 2023 at 02:33:03PM +0100, Borislav Petkov wrote:
> Thx, lemme get cracking on fancy github workflows. :-P
Uff:
https://github.com/dracutdevs/dracut/pull/2572
What a pain it is to do a pull request with github. :-\
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette