Subject: Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
To: Borislav Petkov <bp@suse.de>
References: <1500688839-30247-1-git-send-email-suravee.suthikulpanit@amd.com>
 <1500688839-30247-2-git-send-email-suravee.suthikulpanit@amd.com>
 <20170722161215.GA13329@nazgul.tnic>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, tglx@linutronix.de,
        mingo@redhat.com, hpa@zytor.com, peterz@infradead.org,
        Yazen.Ghannam@amd.com
From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Message-ID: <4948259a-7328-3fb8-a57e-dcc3598380e7@amd.com>
Date: Mon, 24 Jul 2017 10:28:34 +0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0)
 Gecko/20100101 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <20170722161215.GA13329@nazgul.tnic>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Jul 2017 03:28:27.1024 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1744
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2135
Lines: 60

Boris,

On 7/22/17 23:12, Borislav Petkov wrote:
> On Fri, Jul 21, 2017 at 09:00:38PM -0500, Suravee Suthikulpanit wrote:
>> Refactoring in preparation for subsequent changes.
>> There is no functional change.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>  arch/x86/kernel/cpu/amd.c | 79 ++++++++++++++++++++++++++---------------------
>>  1 file changed, 44 insertions(+), 35 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>> index bb5abe8..74d8d7c 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -297,54 +297,63 @@ static int nearby_node(int apicid)
>>  #endif
>>
>>  /*
>> - * Fixup core topology information for
>> - * (1) AMD multi-node processors
>> - *     Assumption: Number of cores in each internal node is the same.
>> - * (2) AMD processors supporting compute units
>> + * Get topology information via X86_FEATURE_TOPOEXT
>>   */
>> -#ifdef CONFIG_SMP
>> -static void amd_get_topology(struct cpuinfo_x86 *c)
>> +static void __get_topoext(struct cpuinfo_x86 *c)
>>  {
>> -	u8 node_id;
>> +	u32 eax, ebx, ecx, edx;
>>  	int cpu = smp_processor_id();
>>
>> -	/* get information required for multi-node processors */
>> -	if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
>> -		u32 eax, ebx, ecx, edx;
>> +	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>>
>> -		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>> +	smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
>>
>> -		node_id  = ecx & 0xff;
>
> When reviewers ask you about a preparatory cleanup patch, you don't
> sneak in changes in it - you *only* *move* the code so that the change
> is *absolutely* comprehensible. Ontop you do changes. Don't tell me you
> didn't know that!

I know that we should not sneak in change. I might have missed something here.

Are you referring to the part that I moved the "node_id = ecx & 0xff" from the 
top level of the function to inside the "if/else" logic where it is the only 
place that used within this new refactored __get_topoext() and there is nothing 
changed functionally? If that's really the case, I'll fix it.

Thanks,
Suravee