On Fri, Feb 2, 2024 at 12:08 AM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: > > On Thu, Feb 1, 2024 at 7:29 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote: > > I really wanted get_cpu_address_sizes() to be the one and only spot > > where c->x86_phys_bits is established. That way, we don't get a bunch > > of code all of the place tweaking it and fighting for who "wins". > > We're not there yet, but the approach in this patch moves it back in the > > wrong direction because it permits the random tweaking of c->x86_phys_bits. > > I see your point; one of my earlier attempts added a > ->c_detect_mem_encrypt() callback that basically amounted to either > amd_detect_mem_encrypt() or detect_tme(). I bailed out of it mostly > because these functions do more than adjusting phys_bits, and it > seemed arbitrary to call them from get_cpu_address_sizes(). The two > approaches share the idea of centralizing the computation of > x86_phys_bits in get_cpu_address_sizes(). > > There is unfortunately an important hurdle for your patch, in that > currently the BSP and AP flows are completely different. For the BSP > the flow is ->c_early_init(), then get_cpu_address_sizes(), then again > ->c_early_init() called by ->c_init(), then ->c_init(). For APs it is > get_cpu_address_sizes(), then ->c_early_init() called by ->c_init(), > then the rest of ->c_init(). And let's not even look at > ->c_identify(). > > This difference is bad for your patch, because get_cpu_address_sizes() > is called too early to see enc_phys_bits on APs. But it was also > something that fbf6449f84bf didn't take into account, because it left > behind the tentative initialization of x86_*_bits in identify_cpu(), > while removing it from early_identify_cpu(). And > > TBH my first reaction after Kirill pointed me to fbf6449f84bf was to > revert it. It's not like the code before was much less of a dumpster > fire, but that commit made the BSP vs. AP mess even worse. But it > fixed a real-world bug and it did remove most of the "first set then > adjust" logic, at least for the BSP, so a revert wasn't on the table > and patch 1 was what came out of it. > > I know that in general in Linux we prefer to fix things for good. > Dancing one step behind and two ahead comes with the the risk that you > only do the step behind. But in the end something like this patch 1 > would have to be posted for stable branches (and Greg doesn't like > one-off patches), and I am not even sure it's a step behind because it > removes _some_ of the BSP vs. AP differences introduced by > fbf6449f84bf. > > In a nutshell: I don't dislike the idea behind your patch, but the > code is just not ready for it. This is the diffstat before your patch can be applied more or less as is: $ git diffstat origin/master arch/x86/include/asm/processor.h | 1 + arch/x86/kernel/cpu/amd.c | 12 ++-- arch/x86/kernel/cpu/centaur.c | 13 ++--- arch/x86/kernel/cpu/common.c | 103 +++++++++++++++++++---------------- arch/x86/kernel/cpu/hygon.c | 2 - arch/x86/kernel/cpu/intel.c | 4 +- arch/x86/kernel/cpu/transmeta.c | 2 - arch/x86/kernel/cpu/zhaoxin.c | 1 - 8 files changed, 69 insertions(+), 69 deletions(-) $ git log --oneline --reverse origin/master.. d639afed02aa x86/cpu/common: move code up to early get_cpu_address_sizes() to a new function 40d34260a4ba x86/cpu/common: pull get_cpu_*() calls common to BSPs and APs to a new function 67b9839f9c38 x86/cpu/common: put all setup_force/clear capabilities together ebeae7f91cbc x86/cpu/centaur: do everything before early_init_centaur() in early_init_centaur() d62fa7400885 x86/cpu/cyrix: call early init function from init function 0aa0916cd7e0 x86/cpu/common: call early_init function on APs from common code d656b651d217 (HEAD) dave I still haven't tested very well, but anyway, what do you think? Paolo
