Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759066AbcKDUnK (ORCPT ); Fri, 4 Nov 2016 16:43:10 -0400 Received: from mx0b-000f0801.pphosted.com ([67.231.152.113]:39022 "EHLO mx0a-000f0801.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756839AbcKDUnI (ORCPT ); Fri, 4 Nov 2016 16:43:08 -0400 Subject: Re: [RFC PATCH] perf/x86/intel/rapl: avoid access unallocate memory To: Sebastian Andrzej Siewior References: <20161102122557.qs4rl6mb7n7l7j7p@linutronix.de> <24e69019-60d0-29e7-e31f-c6f00f9ed98a@brocade.com> <20161103174753.o5ynquul2rjuiq77@linutronix.de> <20161104180313.wyaheuajevkrf6o7@linutronix.de> From: "Charles (Chas) Williams" CC: , , "M. Vefa Bicakci" Message-ID: Date: Fri, 4 Nov 2016 16:42:33 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.3.0 MIME-Version: 1.0 In-Reply-To: <20161104180313.wyaheuajevkrf6o7@linutronix.de> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: hq1wp-excas13.corp.brocade.com (10.70.36.103) To BRMWP-EXMB12.corp.brocade.com (172.16.59.130) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-11-04_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1611040373 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4826 Lines: 112 On 11/04/2016 02:03 PM, Sebastian Andrzej Siewior wrote: > On 2016-11-04 08:20:37 [-0400], Charles (Chas) Williams wrote: >> The initial CPU boots and is identified: >> >> [ 0.009018] identify_boot_cpu >> [ 0.009174] generic_identify: phys_proc_id is now 0 >> ... >> [ 0.009427] identify_cpu: before c ffffffff81ae2680 logical_proc_id 0 c->phys_proc_id 0 >> [ 0.009506] identify_cpu: after c ffffffff81ae2680 logical_proc_id 65535 c->phys_proc_id 0 >> >> So, this is fine because the APIC hasn't been scanned yet. APIC >> now gets scanned: >> >> [ 0.015789] smpboot: APIC(0) Converting physical 0 to logical package 0, cpu 0 (ffff88023fc0a040) >> [ 0.015794] smpboot: APIC(1) Converting physical 1 to logical package 1, cpu 1 (ffff88023fd0a040) >> [ 0.015797] smpboot: Max logical packages: 2 > > where is the APICID here is comming from? This comes from here: unsigned int apicid = apic->cpu_present_to_apicid(cpu); if (apicid == BAD_APICID || !apic->apic_id_valid(apicid)) continue; if (!topology_update_package_map(apicid, cpu)) And I think this is the part that is "wrong". The apicid appears to be a logical CPU id. I believe that in most cases this mapping comes from x86_bios_cpu_apicid (or x86_cpu_to_apicid) which is generated in generic_processor_info() which maps apicid's to logical cpu indexes. Note that apic->cpu_present_to_apicid() is using just the cpu_index. for_each_present_cpu(cpu) { unsigned int apicid = apic->cpu_present_to_apicid(cpu); if (apicid == BAD_APICID || !apic->apic_id_valid(apicid)) continue; if (!topology_update_package_map(apicid, cpu)) continue; pr_warn("CPU %u APICId %x disabled\n", cpu, apicid); per_cpu(x86_bios_cpu_apicid, cpu) = BAD_APICID; set_cpu_possible(cpu, false); set_cpu_present(cpu, false); } >> So, at this point, I think everything is correct. But now the secondary >> CPU's "boot": >> >> [ 0.236569] identify_secondary_cpu >> [ 0.236620] generic_identify: phys_proc_id is now 2 > > so here is where fun starts. Xen has also > arch/x86/xen/smp.c::cpu_bringup() where the phys_proc_id is changed. But > isn't done for vmware but it might a place where they duct tape things. > > How is this APIC id different from the earlier? I guess based on your > output that generic_identify() changes the content of phys_proc_id. > >> [ 0.236745] identify_cpu: before c ffff88023fd0a040 logical_proc_id 65535 c->phys_proc_id 2 >> [ 0.236747] identify_cpu: after c ffff88023fd0a040 logical_proc_id 65535 c->phys_proc_id 2 >> >> So, APIC discovered I have a cpu 0 and 1 but generic_identify() is called >> my second CPU, 2. This is >= max_physical_pkg_id, so it is going to get >> set to -1. > > Now. max_physical_pkg_id is huge. The physical_to_logical_pkg array is > set to -1 on init so slot two has the value -1. That is what you see - > not the -1 because of ">= max_physical_pkg_id". > >> The comment at the end of identfy_cpu() says: >> >> /* The boot/hotplug time assigment got cleared, restore it */ >> >> So, logical_proc_id being wrong here before restoration doesn't bother >> me since I assume something in booting the secondary CPU's clears any >> existing cpu data. >> >> I know detect_extended_topology() is likely being called for both CPU's >> and getting the right values (checking this now). I don't know why >> generic_identify() is resetting this value. > > I don't know either. But it is clearly reading the apic id twice and > second approach is different from the first which leads to different > results. So if you figure out how the first APICID for the second CPU is > retrieved and then you see how it happens for the second time. There > must be a difference. The phys core id from generic_identify() comes from the CPU's EBX register so we _know_ this is right. if (c->cpuid_level >= 0x00000001) { c->initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF; #ifdef CONFIG_X86_32 # ifdef CONFIG_SMP c->apicid = apic->phys_pkg_id(c->initial_apicid, 0); # else c->apicid = c->initial_apicid; # endif #endif c->phys_proc_id = c->initial_apicid; } The intel docs http://x86.renejeschke.de/html/file_module_x86_id_45.html claims this is the Local APIC ID. So it seems likely this is correct value. It's not clear it matter if this is the right value or not though. Even if this is the correct apicid, nothing knows about it. An argument could be made that instead of checking the cpuid level, we could just use the apicid based on the cpu index just like the other code. It would be consistent at least then.