Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2804 bytes --]

On 25.08.22 12:31, Borislav Petkov wrote:
> On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote:
>> The Cyrix cpu specific MTRR function cyrix_set_all() will never be
>> called, as the struct mtrr_ops set_all() callback will only be called
>> in the use_intel() case, which would require the use_intel_if member
>> of struct mtrr_ops to be set, which isn't the case for Cyrix.
> 
> Doing some git archeology:
> 
> So the commit which added mtrr_aps_delayed_init is
> 
>    d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init")
> 
> from 2009.
> 
> The IPI callback before it, looked like this:
> 
> static void ipi_handler(void *info)
> {
> #ifdef CONFIG_SMP
> 	struct set_mtrr_data *data = info;
> 	unsigned long flags;
> 
> 	local_irq_save(flags);
> 
> 	atomic_dec(&data->count);
> 	while (!atomic_read(&data->gate))
> 		cpu_relax();
> 
> 	/*  The master has cleared me to execute  */
> 	if (data->smp_reg != ~0U) {
> 		mtrr_if->set(data->smp_reg, data->smp_base,
> 			     data->smp_size, data->smp_type);
> 	} else {
> 		mtrr_if->set_all();
> 		^^^^^^^^^
> 
> and that else branch would call ->set_all() on Cyrix too.
> 
> Suresh's patch changed it to do:
> 
> -	} else {
> +	} else if (mtrr_aps_delayed_init) {
> +		/*
> +		 * Initialize the MTRRs inaddition to the synchronisation.
> +		 */
>   		mtrr_if->set_all();
> 
> BUT below in the set_mtrr() call, it did:
> 
>          /*
>           * HACK!
>           * We use this same function to initialize the mtrrs on boot.
>           * The state of the boot cpu's mtrrs has been saved, and we want
>           * to replicate across all the APs.
>           * If we're doing that @reg is set to something special...
>           */
>          if (reg != ~0U)
>                  mtrr_if->set(reg, base, size, type);
>          else if (!mtrr_aps_delayed_init)
>                  mtrr_if->set_all();
> 		^^^
> 
> and that would be the Cyrix case.
> 
> But then
> 
>    192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous")
> 
> came and "cleaned" all up by removing the "HACK" and doing ->set_all()
> only in the rendezvous handler:
> 
> +       } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
>                  mtrr_if->set_all();
>          }
> 
> Which begs the question: why doesn't the second part of the else test
> match on Cyrix? The "|| !cpu_online(smp_processor_id())" case.
> 
> If only we had a Cyrix machine somewhere...
> 

Maybe the alternative reasoning is much faster to understand: if the
Cyrix set_all() could be called, the AMD and Centaur ones would be callable,
too. Those being called would result in a NULL deref, so why should we keep
the Cyrix one?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]