From:   Thomas Gleixner <tglx@linutronix.de>
To:     Usama Arif <usama.arif@bytedance.com>, dwmw2@infradead.org,
        kim.phillips@amd.com
Cc:     arjan@linux.intel.com, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, hpa@zytor.com, x86@kernel.org,
        pbonzini@redhat.com, paulmck@kernel.org,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        rcu@vger.kernel.org, mimoja@mimoja.de, hewenliang4@huawei.com,
        thomas.lendacky@amd.com, seanjc@google.com, pmenzel@molgen.mpg.de,
        fam.zheng@bytedance.com, punit.agrawal@bytedance.com,
        simon.evans@bytedance.com, liangma@liangbit.com,
        David Woodhouse <dwmw@amazon.co.uk>
Subject: Re: [External] Re: [PATCH v8 8/9] x86/mtrr: Avoid repeated save of
 MTRRs on boot-time CPU bringup
In-Reply-To: <9b6bca9c-7189-a2d5-8c0a-f55c24f54b62@bytedance.com>
References: <20230209154156.266385-1-usama.arif@bytedance.com>
 <20230209154156.266385-9-usama.arif@bytedance.com> <87mt5m1yiz.ffs@tglx>
 <9b6bca9c-7189-a2d5-8c0a-f55c24f54b62@bytedance.com>
Date:   Fri, 10 Feb 2023 00:50:30 +0100
Message-ID: <878rh61jqx.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Thu, Feb 09 2023 at 20:32, Usama Arif wrote:
> On 09/02/2023 18:31, Thomas Gleixner wrote:
>>>   	first_cpu = cpumask_first(cpu_online_mask);
>>>   	smp_call_function_single(first_cpu, mtrr_save_fixed_ranges, NULL, 1);
>> 
>> So why is this relevant after the initial bringup? The BP MTRRs have
>> been saved already above, no?
>> 
>
> I will let David confirm if this is correct and why he did it, but this 
> is what I thought while reviewing before posting v4:
>
> - At initial boot (system_state < SYSTEM_RUNNING), when mtrr_save_state 
> is called in do_cpu_up at roughly the same time so MTRR is going to be 
> the same, we can just save it once and then reuse for other secondary 
> cores as it wouldn't have changed for the rest of the do_cpu_up calls.
>
> - When the system is running and you offline and then online a CPU, you 
> want to make sure that hotplugged CPU gets the current MTRR (which might 
> have changed since boot?), incase the MTRR has changed after the system 
> has been booted, you save the MTRR of the first online CPU. When the 
> hotplugged CPU runs its initialisation code, its fixed-range MTRRs will 
> be updated with the newly saved fixed-range MTRRs.

I knew that already :) But seriously:

If the MTRRs are changed post boot then the cached values want to be
updated too.

We are not making these changes just to satisfy some fast boot
challenge. They have to make sense in general.

And this does not amke sense at all.

Thanks,

        tglx