Messages in this thread Patch in this message | | | Date | Sat, 12 Aug 2023 00:25:00 +0200 | From | Borislav Petkov <> | Subject | Re: [patch 08/30] x86/microcode/intel: Rip out mixed stepping support for Intel CPUs |
| |
On Thu, Aug 10, 2023 at 08:37:38PM +0200, Thomas Gleixner wrote: > @@ -319,6 +264,7 @@ scan_microcode(void *data, size_t size, > { > struct microcode_header_intel *mc_header; > struct microcode_intel *patch = NULL; > + u32 cur_rev = uci->cpu_sig.rev; > unsigned int mc_size; > > while (size) { > @@ -328,8 +274,7 @@ scan_microcode(void *data, size_t size, > mc_header = (struct microcode_header_intel *)data; > > mc_size = get_totalsize(mc_header); > - if (!mc_size || > - mc_size > size || > + if (!mc_size || mc_size > size || > intel_microcode_sanity_check(data, false, MC_HEADER_TYPE_MICROCODE) < 0) > break; > > @@ -341,31 +286,16 @@ scan_microcode(void *data, size_t size, > continue; > } > > - if (save) { > - save_microcode_patch(uci, data, mc_size); > + /* BSP scan: Check whether there is newer microcode */ > + if (save && cur_rev >= mc_header->rev) > goto next; > - } > - > > - if (!patch) { > - if (!has_newer_microcode(data, > - uci->cpu_sig.sig, > - uci->cpu_sig.pf, > - uci->cpu_sig.rev)) > - goto next; > - > - } else { > - struct microcode_header_intel *phdr = &patch->hdr; > - > - if (!has_newer_microcode(data, > - phdr->sig, > - phdr->pf, > - phdr->rev)) > - goto next; > - } > + /* Save scan: Check whether there is newer or matching microcode */ > + if (save && cur_rev != mc_header->rev) > + goto next;
I'm confused: when you look at those statements when this patch is applied, they look like this:
/* BSP scan: Check whether there is newer microcode */ if (save && cur_rev >= mc_header->rev) goto next;
/* Save scan: Check whether there is newer or matching microcode */ if (save && cur_rev != mc_header->rev) goto next;
You'd only hit the second one if
cur_rev < mc_header->rev
but then that implies
cur_rev != mc_header->rev
too. I *think* you wanna have the first test be only ">" as you're looking for newer microcode.
Besides, __load_ucode_intel() is calling this function with safe == false so those statements would never check anything. I guess that's still ok because the above intel_find_matching_signature() would match.
Hmmm?
Uff, this function is ugly and can be simplified. Perhaps that happens later.
> > - /* We have a newer patch, save it. */ > patch = data; > + cur_rev = mc_header->rev; > > next: > data += mc_size; > @@ -374,18 +304,22 @@ scan_microcode(void *data, size_t size, > if (size) > return NULL; > > + if (save && patch) > + save_microcode_patch(patch, mc_size); > + > return patch; > } > > static void show_saved_mc(void) > { > #ifdef DEBUG
Yeah, what Nikolay said - move the next one before this one and then the show_saved_mc() hunks are gone.
> - int i = 0, j; > unsigned int sig, pf, rev, total_size, data_size, date; > + struct extended_sigtable *ext_header; > + struct extended_signature *ext_sig; > struct ucode_cpu_info uci; > - struct ucode_patch *p; > + int j, ext_sigcount; > > - if (list_empty(µcode_cache)) { > + if (!intel_ucode_patch) { > pr_debug("no microcode data saved.\n"); > return; > }
...
> @@ -451,7 +374,7 @@ static void save_mc_for_early(struct uco > > mutex_lock(&x86_cpu_microcode_mutex); > > - save_microcode_patch(uci, mc, size); > + save_microcode_patch(mc, size); > show_saved_mc(); > > mutex_unlock(&x86_cpu_microcode_mutex); > @@ -675,26 +598,10 @@ void load_ucode_intel_ap(void) > apply_microcode_early(&uci, true); > } > > -static struct microcode_intel *find_patch(struct ucode_cpu_info *uci) > +/* Accessor for microcode pointer */ > +static struct microcode_intel *ucode_get_patch(void)
static function - "get_patch" only is fine.
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |