Message-ID: <9b6bca9c-7189-a2d5-8c0a-f55c24f54b62@bytedance.com>
Date:   Thu, 9 Feb 2023 20:32:02 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [External] Re: [PATCH v8 8/9] x86/mtrr: Avoid repeated save of
 MTRRs on boot-time CPU bringup
Content-Language: en-US
To:     Thomas Gleixner <tglx@linutronix.de>, 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>
References: <20230209154156.266385-1-usama.arif@bytedance.com>
 <20230209154156.266385-9-usama.arif@bytedance.com> <87mt5m1yiz.ffs@tglx>
From:   Usama Arif <usama.arif@bytedance.com>
In-Reply-To: <87mt5m1yiz.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 09/02/2023 18:31, Thomas Gleixner wrote:
> On Thu, Feb 09 2023 at 15:41, Usama Arif wrote:
>>   void mtrr_save_state(void)
>>   {
>> +	static bool mtrr_saved;
>>   	int first_cpu;
>>   
>>   	if (!mtrr_enabled())
>>   		return;
>>   
>> +	if (system_state < SYSTEM_RUNNING) {
>> +		if (!mtrr_saved) {
>> +			mtrr_save_fixed_ranges(NULL);
>> +			mtrr_saved = true;
>> +		}
>> +		return;
>> +	}
>> +
>>   	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?
> 
> Thanks,
> 
>          tglx

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.

So mainly for hotplug, but will let David confirm.

Thanks,
Usama