Messages in this thread | | | Date | Thu, 19 Jan 2023 14:34:59 -0800 | From | Ashok Raj <> | Subject | Re: [PATCH v1 Part2 2/5] x86/microcode/intel: Add minimum required revision to microcode header |
| |
On Thu, Jan 19, 2023 at 11:03:14PM +0100, Thomas Gleixner wrote: > On Fri, Jan 13 2023 at 09:29, Ashok Raj wrote: > > In general users don't have the necessary information to determine > > whether a late loading of a new microcode version has removed any feature > > (MSR, CPUID etc) between what is currently loaded and this new microcode. > > s/this new microcode/a newer microcode revision/
Yes.
> > > To address this issue, Intel has added a "minimum required version" field > > to a previously reserved field in the file header. Microcode updates > > s/file header/microcode header/ perhaps?
Yep! > > > should only be applied if the current microcode version is equal > > to, or greater than this minimum required version. > > > > Thomas made some suggestions[1] on how meta-data in the microcode file > > could provide Linux with information to decide if the new microcode is > > suitable candidate for late loading. But even the "simpler" option#1 > > requires a lot of metadata and corresponding kernel code to parse it. > > > > The proposal here is an even simpler option. > > IIRC this was also suggested by this Thomas dude, right?
Same dude.. might have been your twin :-)
I'll fix it.
> > > Simply "OS visible features" such as CPUID and MSRs are the only two > > examples. The microcode must not change these OS visible features > > because they cause problems after late loading. When microcode changes > > features, microcode will change the min_rev to prevent such microcodes > > from being late loaded. > > > > Pseudo code for late loading is as follows: > > > > if header.min_required_id == 0 > > This is old format microcode, block late loading > > else if current_ucode_version < header.min_required_id > > Current version is too old, block late loading of this microcode. > > else > > OK to proceed with late loading. > > > > Any microcode that modifies the interface to an OS-visible feature > > will set the min_version to itself. This will enforce this microcode is > > not suitable for late loading unless the currently loaded revision is > > greater or equal to the new microcode affecting the change. > > Up to this paragraph the changelog made sense. > > If the currently loaded revision is the same as the to be loaded > revision, then there is nothing to do. > > If the currently loaded revision is greater than the to be loaded > revision then it is not loaded as the kernel does not support > downgrading in the first place. > > Even if it would support downgrading then this would be outright wrong > for this case: > > Rev: 10 > Min-Rev: 10 > > Rev: 20 > Min-Rev: 20 > > If Rev 20 is loaded, then you absolutely cannot load Rev 10 because that > would have the reverse side effects due to which Rev 20 prevents late > loading. > > See?
Yes, that's accurate, and in sprit it works that way.
The current_rev > mc_hdr->rev is done in apply_microcode_intel() but I suppose we could do that check early.
I didn't touch those parts to make sure only minimal changes were done and we can do cleanup's later. I should certainly add a note to make sure we aren't breaking the rev is always greater than what's in the CPU for clarity.
I do have several cleanups lined up, but didn't want to hold the minrev and the nmi series.
Cheers, Ashok
| |