linux-kernel - Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 Aug 2022 12:31:11 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Juergen Gross <jgross@...e.com>
Cc:     xen-devel@...ts.xenproject.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function

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...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette