Messages in this thread | | | Date | Mon, 22 Aug 2022 07:17:40 +0200 | Subject | Re: [PATCH v2 01/10] x86/mtrr: fix MTRR fixup on APs | From | Juergen Gross <> |
| |
On 21.08.22 23:41, Borislav Petkov wrote: > On Sun, Aug 21, 2022 at 02:25:59PM +0200, Borislav Petkov wrote: >>> Fix that by using percpu variables for saving the MSR contents. >>> >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> I thought adding a "Fixes:" tag for the kernel's initial git commit >>> would maybe be entertaining, but without being really helpful. >>> The percpu variables were preferred over on-stack ones in order to >>> avoid more code churn in followup patches decoupling PAT from MTRR >>> support. >> >> So if that thing has been broken for so long and no one noticed, we >> could just as well not backport to stable at all... > > Yeah, you can't do that. > > The whole day today I kept thinking that something's wrong with this > here. As in, why hasn't it been reported until now. > > You say above: > > "... for all cpus is racy in case the MSR contents differ across cpus." > > But they don't differ: > > "7.7.5 MTRRs in Multi-Processing Environments > > In multi-processing environments, the MTRRs located in all processors > must characterize memory in the same way. Generally, this means that > identical values are written to the MTRRs used by the processors. This > also means that values CR0.CD and the PAT must be consistent across > processors. Failure to do so may result in coherency violations or loss > of atomicity. Processor implementations do not check the MTRR settings > in other processors to ensure consistency. It is the responsibility of > system software to initialize and maintain MTRR consistency across all > processors." > > And you can't have different fixed MTRR type on each CPU - that would > lead to all kinds of nasty bugs. > > And here's from a big fat box: > > $ rdmsr -a 0x2ff | uniq -c > 256 c00 > > All 256 CPUs have the def type set to the same thing. > > Now, if all CPUs go write that same deftype_lo variable in the > rendezvous handler, the only issue that could happen is if a read > sees a partial write. BUT, AFAIK, x86 doesn't tear 32-bit writes so I > *think* all CPUs see the same value being corrected by using mtrr_state > previously saved on the BSP. > > As I said, we should've seen this exploding left and right otherwise...
And then there is mtrr_state_warn() in arch/x86/kernel/cpu/mtrr/generic.c which has a comment saying:
/* Some BIOS's are messed up and don't set all MTRRs the same! */
Yes, the chances are slim to hit such a box, but your reasoning suggests I should remove the related code?
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |