[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.