Re: [PATCH v2 1/2] x86/cpu: Disable CR pinning during CPU bringup

[Dave Hansen]

On 3/11/26 08:42, Nikunj A. Dadhania wrote:
> -void cr4_init(void)
> +void cr4_init(bool boot_cpu)

I think the "pass a bool to a function" pattern is really not great.

The problem isn't actually setting CR4 twice. The problem is having two
different code paths to set it. Doing the 'bool' doesn't eliminate the
code path, it just makes the whole thing more complicated to reason
about and further bifurcates the boot and secondary CPU bringup paths.

We want to unify those, not bifurcate them.

Why don't we just universally set X86_CR4_FSGSBASE in cr4_init()?

BTW, commit c7ad5ad297e tells some of the tales of woe around CR4 and
boot vs. secondary CPUs:

	cpu_init() is weird: it's called rather late (after early
	identification and after most MMU state is initialized) on the
	boot CPU but is called extremely early (before identification)
	on secondary CPUs.

This weirdness is still biting us today. CR4 pinning just made things
worse (or at least harder to understand).

I have the feeling we need to bite the bullet here and actually start
thinking about this holistically. I _think_ 'mmu_cr4_features' is
conceptually pretty close to what we need. It's just named wrong.

/*
 * Current system-wide configuration information for CR4 register.
 * All of the bits in these feature masks are supported by the current
 * running CPU.
 */
struct cr4_config {
	/*
	 * Features that are needed in early assembly like the
	 * trampoline. These should not have side effects or interact
	 * with other features.
	 *
	 * Only written by the boot CPU. Establishes bringup value on
	 * secondary CPUs.
	 */
	unsigned long early_features;

	/*
	 * Features that get enabled from C code: cr4_late_init().
	 * Anything with early side-effects that can't be in the
	 * early set.
	 */
	unsigned long late_features;

	/*
	 * Features that do not work in real-mode.
	 */
	unsigned long realmode_incompat_features;

	/*
	 * Points into the real mode trampoline header. Write to this
	 * every time 'early_features' is updated. (maybe??)
	 */
	unsigned long *trampoline_features_target;

	/*
	 * Features that are always on when userspace is running.
	 * CR4 manipulation functions will notice if these bits get
	 * cleared, restore them and WARN().
	 */
	unsigned long pinned_features;
};


Then instead of sprinkling code around like:

        /*
         * This function is called before exiting to real-mode and that
	 * will
         * fail with CR4.PCIDE still set.
         */
        if (boot_cpu_has(X86_FEATURE_PCID))
                cr4_clear_bits(X86_CR4_PCIDE);

We can do *ALL* the PCID setup in one place:

void boot_cpu_setup_pcid()
{
	if (!boot_cpu_has(X86_FEATURE_PCID))
		return;

	/* PCID is not needed in early bringup, only enable it late: */
	cr4_config.late_features |= X86_CR4_PCIDE;

	/*
	 * PCIDE needs CR0.PG=1, which is not true in real mode.
	 * Ensure it is disabled in real mode:
	 */
	cr4_config.realmode_incompat_features |= X86_CR4_PCIDE;

	__write_cr4(cr4);
        /* Initialize cr4 shadow for this CPU. */
        this_cpu_write(cpu_tlbstate.cr4, cr4);
}

and the read-mode entry becomes:

	cr4_clear_bits(cr4_config.realmode_incompat_features);

What do folks think? Can we expand the 'mmu_cr4_features' to more than
MMU features?