On 28/10/2024 1:56 pm, Jan Beulich wrote:
> On 28.10.2024 10:18, Andrew Cooper wrote:
>> The work to add the `ucode=nmi` cmdline option left a subtle corner case.
>> Both scan and an explicit index could be selected, and we could really find a
>> CPIO archive and explicit microcode file.
>>
>> Worse, because the if/else chains for processing ucode_{blob,mod} are
>> opposite
>> ways around in early_microcode_load() and microcode_init_cache(), we can
>> genuinely perform early microcode loading from the CPIO archive, then cache
>> from the explicit file.
>>
>> Therefore, enforce that only one selection method can be active.
> Question is - is this really the best of all possible behaviors? One may want
> to use one approach as the fallback for the other, e.g. preferably use what
> the CPIO has, but fall back to something pre-installed on the boot or EFI
> partition.
It is unfortunate behaviour.
I've seen explicit complains about it on the archlinux forums; putting a
CPIO fragment in EFI's ucode= argument and getting confused as to why it
doesn't work.
However, it is (reasonably) necessary to dis-enagle this path, and we
currently document it as "undefined behaviour".
I think that we should re-evaluate the behaviour, after the rest of the
boot cleanup is done and we have an easier time reasoning about what is
what.
>> @@ -139,12 +148,16 @@ static int __init cf_check parse_ucode(const char *s)
>> else if ( !ucode_mod_forced ) /* Not forced by EFI */
>> {
>> if ( (val = parse_boolean("scan", s, ss)) >= 0 )
>> - ucode_scan = val;
>> + {
>> + opt_scan = val;
>> + opt_mod_idx = 0;
>> + }
>> else
>> {
>> const char *q;
>>
>> - ucode_mod_idx = simple_strtol(s, &q, 0);
>> + opt_scan = false;
>> + opt_mod_idx = simple_strtol(s, &q, 0);
>> if ( q != ss )
>> rc = -EINVAL;
>> }
> I think this latter part rather wants to be
>
> opt_mod_idx = simple_strtol(s, &q, 0);
> if ( q != ss )
> {
> opt_mod_idx = 0;
> rc = -EINVAL;
> }
> else
> opt_scan = false;
>
> to prevent a malformed ucode= to clobber an earlier wellformed ucode=scan.
> (There are limits to this of course, as an out-of-range value would still
> invalidate the "scan" request.)
Fine. I'm not overly fussed. We don't make any pretence that erroneous
cmdline settings are handled nicely.
>
>> @@ -817,17 +830,42 @@ static int __init early_microcode_load(struct
>> boot_info *bi)
>> const void *data = NULL;
>> size_t size;
>> struct microcode_patch *patch;
>> + int idx = opt_mod_idx;
>> +
>> + /*
>> + * Cmdline parsing ensures this invariant holds, so that we don't end up
>> + * trying to mix multiple ways of finding the microcode.
>> + */
>> + ASSERT(idx == 0 || !opt_scan);
>>
>> - if ( ucode_mod_idx < 0 )
>> - ucode_mod_idx += bi->nr_modules;
>> - if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
>> - !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
>> - goto scan;
>> - ucode_mod = *bi->mods[ucode_mod_idx].mod;
>> - scan:
> Oh, the goto and label are going away here anyway. Never mind the comment on
> the earlier patch then.
Thanks, and yes.
I did play with the order quite a lot. The first iteration did clean
this up as part of merging into early_microcode_load(), but it turned
into a mess.
This way around (straight fold in the previous patch, rework here given
the invariant) was the cleanest I came up with.
~Andrew
