| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4b1bc14b-d937-be80-8b9d-e1a904b26b18@loongson.cn>
Date: Tue, 29 Oct 2024 19:52:08 +0800
From: maobibo <maobibo@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>
Cc: Jianmin Lv <lvjianmin@...ngson.cn>, loongarch@...ts.linux.dev,
linux-kernel@...r.kernel.org, lixianglai@...ngson.cn,
WANG Xuerui <kernel@...0n.name>
Subject: Re: [PATCH v2] LoongArch: Fix cpu hotplug issue
On 2024/10/29 下午6:36, Huacai Chen wrote:
> On Mon, Oct 28, 2024 at 8:38 PM maobibo <maobibo@...ngson.cn> wrote:
>>
>> Hi Huacai,
>>
>> On 2024/10/22 上午9:31, Huacai Chen wrote:
>>> On Tue, Oct 22, 2024 at 9:17 AM maobibo <maobibo@...ngson.cn> wrote:
>>>>
>>>>
>>>>
>>>> On 2024/10/21 下午10:32, Huacai Chen wrote:
>>>>> Hi, Bibo,
>>>>>
>>>>> This version still doesn't touch the round-robin method, but it
>>>>> doesn't matter, I think I misunderstood something since V1...
>>>> I do not understand why round-robin method need be modified, SRAT may be
>>>> disabled with general function disable_srat(). Then round-robin method
>>>> is required.
>>> I don't mean round-robin should be modified, I mean I misunderstand round-robin.
>>>
>>>>
>>>>>
>>>>> Please correct me if I'm wrong: For cpus without ACPI_MADT_ENABLED, in
>>>>> smp_prepare_boot_cpu() the round-robin node ids only apply to
>>>>> cpu_to_node(), but __cpuid_to_node[] still record the right node ids.
>>>>> early_cpu_to_node() returns NUMA_NO_NODE not because
>>>>> __cpuid_to_node[] records NUMA_NO_NODE, but because cpu_logical_map()
>>>>> < 0.
>>>>>
>>>>> If the above is correct, we don't need so complicated, because the
>>>>> correct and simplest way is:
>>>>> https://lore.kernel.org/loongarch/6b2b3e89-5a46-2d20-3dfb-7aae33839f49@loongson.cn/T/#m950eead5250e5992cc703bbe69622348cecfa465
>>>>>
>>>> It works also. Only that LoongArch kernel parsing about SRAT/MADT is
>>>> badly. If you do not mind, I do not mind neither. It is not my duty for
>>>> kernel side.
>>> Yes, I don't mind, please use that simplest way.
>> There is another problem with the simple way. eiointc reports error when
>> cpu is online. The error message is:
>> Loongson-64bit Processor probed (LA464 Core)
>> CPU2 revision is: 0014c010 (Loongson-64bit)
>> FPU2 revision is: 00000001
>> eiointc: Error: invalid nodemap!
>> CPU 2 UP state irqchip/loongarch/eiointc:starting (100) failed (-1)
>>
>> The problem is that node_map of eiointc is problematic,
>>
>>
>> static int cpu_to_eio_node(int cpu)
>> {
>> return cpu_logical_map(cpu) / CORES_PER_EIO_NODE;
>> }
>>
>> static int __init eiointc_init(struct eiointc_priv *priv, int parent_irq,
>> u64 node_map)
>> {
>> int i;
>>
>> node_map = node_map ? node_map : -1ULL;
>> for_each_possible_cpu(i) {
>> if (node_map & (1ULL << (cpu_to_eio_node(i)))) {
>> node_set(cpu_to_eio_node(i), priv->node_map);
>> ...
>> The cause is that for possible not present cpu, *cpu_logical_map(cpu)*
>> is -1, cpu_to_eio_node(i) will be equal to -1, so node_map of eiointc is
>> problematic.
> The error message seems from eiointc_router_init(), but it is a little
> strange. Physical hot-add should be before logical hot-add. So
> acpi_map_cpu() is before cpu_up(). acpi_map_cpu() calls
> set_processor_mask() to setup logical-physical mapping, so in
> eiointc_router_init() which is called by cpu_up(), cpu_logical_map()
> should work well.
cpu hot-adding works in run-time stage, however eiointc_router_init()
runs early in booting stage. Setting cpu_logical_map() in runtime is too
late for function eiointc_router_init(), since eiointc_router_init() is
only called in early kernel boot stage.
Regards
Bibo Mao
>
> Maybe in your case a whole node is hot-added? I don't think the
> eiointc design can work with this case...
>
>>
>> So cpu_logical_map(cpu) should be set during MADT parsing even if it is
>> not enabled at beginning, it should not be set at hotplug runtime.
> This will cause the logical cpu number be not continuous after boot.
> Physical numbers have no requirement, but logical numbers should be
> continuous.
>
> Huacai
>
>>
>> Regards
>> Bibo Mao
>>
>>
>>>
>>> Huacai
>>>
>>>>
>>>> Bibo Mao
>>>>>
>>>>> Huacai
>>>>>
>>>>> On Mon, Oct 21, 2024 at 4:04 PM Bibo Mao <maobibo@...ngson.cn> wrote:
>>>>>>
>>>>>> On LoongArch system, there are two places to set cpu numa node. One
>>>>>> is in arch specified function smp_prepare_boot_cpu(), the other is
>>>>>> in generic function early_numa_node_init(). The latter will overwrite
>>>>>> the numa node information.
>>>>>>
>>>>>> With hot-added cpu without numa information, cpu_logical_map() fails
>>>>>> to its physical cpuid at beginning since it is not enabled in ACPI
>>>>>> MADT table. So function early_cpu_to_node() also fails to get its
>>>>>> numa node for hot-added cpu, and generic function
>>>>>> early_numa_node_init() will overwrite with incorrect numa node.
>>>>>>
>>>>>> APIs topo_get_cpu() and topo_add_cpu() is added here, like other
>>>>>> architectures logic cpu is allocated when parsing MADT table. When
>>>>>> parsing SRAT table or hot-add cpu, logic cpu is acquired by searching
>>>>>> all allocated logical cpu with matched physical id. It solves such
>>>>>> problems such as:
>>>>>> 1. Boot cpu is not the first entry in MADT table, the first entry
>>>>>> will be overwritten with later boot cpu.
>>>>>> 2. Physical cpu id not presented in MADT table is invalid, in later
>>>>>> SRAT/hot-add cpu parsing, invalid physical cpu detected is added
>>>>>> 3. For hot-add cpu, its logic cpu is allocated in MADT table parsing,
>>>>>> so early_cpu_to_node() can be used for hot-add cpu and cpu_to_node()
>>>>>> is correct for hot-add cpu.
>>>>>>
>>>>>> Signed-off-by: Bibo Mao <maobibo@...ngson.cn>
>>>>>> ---
>>>>>> v1 ... v2:
>>>>>> 1. Like other architectures, allocate logic cpu when parsing MADT table.
>>>>>> 2. Add invalid or duplicated physical cpuid parsing with SRAT table or
>>>>>> hot-add cpu DSDT information.
>>>>>> ---
>>>>>> arch/loongarch/include/asm/smp.h | 3 ++
>>>>>> arch/loongarch/kernel/acpi.c | 24 ++++++++++------
>>>>>> arch/loongarch/kernel/setup.c | 47 ++++++++++++++++++++++++++++++++
>>>>>> arch/loongarch/kernel/smp.c | 9 +++---
>>>>>> 4 files changed, 70 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/loongarch/include/asm/smp.h b/arch/loongarch/include/asm/smp.h
>>>>>> index 3383c9d24e94..c61b75937a77 100644
>>>>>> --- a/arch/loongarch/include/asm/smp.h
>>>>>> +++ b/arch/loongarch/include/asm/smp.h
>>>>>> @@ -119,4 +119,7 @@ static inline void __cpu_die(unsigned int cpu)
>>>>>> #define cpu_logical_map(cpu) 0
>>>>>> #endif /* CONFIG_SMP */
>>>>>>
>>>>>> +int topo_add_cpu(int physid);
>>>>>> +int topo_get_cpu(int physid);
>>>>>> +
>>>>>> #endif /* __ASM_SMP_H */
>>>>>> diff --git a/arch/loongarch/kernel/acpi.c b/arch/loongarch/kernel/acpi.c
>>>>>> index f1a74b80f22c..84d9812d5f38 100644
>>>>>> --- a/arch/loongarch/kernel/acpi.c
>>>>>> +++ b/arch/loongarch/kernel/acpi.c
>>>>>> @@ -78,10 +78,10 @@ static int set_processor_mask(u32 id, u32 flags)
>>>>>> return -ENODEV;
>>>>>>
>>>>>> }
>>>>>> - if (cpuid == loongson_sysconf.boot_cpu_id)
>>>>>> - cpu = 0;
>>>>>> - else
>>>>>> - cpu = find_first_zero_bit(cpumask_bits(cpu_present_mask), NR_CPUS);
>>>>>> +
>>>>>> + cpu = topo_add_cpu(cpuid);
>>>>>> + if (cpu < 0)
>>>>>> + return -EEXIST;
>>>>>>
>>>>>> if (!cpu_enumerated)
>>>>>> set_cpu_possible(cpu, true);
>>>>>> @@ -203,8 +203,6 @@ void __init acpi_boot_table_init(void)
>>>>>> goto fdt_earlycon;
>>>>>> }
>>>>>>
>>>>>> - loongson_sysconf.boot_cpu_id = read_csr_cpuid();
>>>>>> -
>>>>>> /*
>>>>>> * Process the Multiple APIC Description Table (MADT), if present
>>>>>> */
>>>>>> @@ -257,7 +255,7 @@ void __init numa_set_distance(int from, int to, int distance)
>>>>>> void __init
>>>>>> acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
>>>>>> {
>>>>>> - int pxm, node;
>>>>>> + int pxm, node, cpu;
>>>>>>
>>>>>> if (srat_disabled())
>>>>>> return;
>>>>>> @@ -286,6 +284,11 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> + cpu = topo_get_cpu(pa->apic_id);
>>>>>> + /* Check whether apic_id exists in MADT table */
>>>>>> + if (cpu < 0)
>>>>>> + return;
>>>>>> +
>>>>>> early_numa_add_cpu(pa->apic_id, node);
>>>>>>
>>>>>> set_cpuid_to_node(pa->apic_id, node);
>>>>>> @@ -324,12 +327,17 @@ int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id, int *pcpu
>>>>>> {
>>>>>> int cpu;
>>>>>>
>>>>>> - cpu = set_processor_mask(physid, ACPI_MADT_ENABLED);
>>>>>> + cpu = topo_get_cpu(physid);
>>>>>> + /* Check whether apic_id exists in MADT table */
>>>>>> if (cpu < 0) {
>>>>>> pr_info(PREFIX "Unable to map lapic to logical cpu number\n");
>>>>>> return cpu;
>>>>>> }
>>>>>>
>>>>>> + num_processors++;
>>>>>> + set_cpu_present(cpu, true);
>>>>>> + __cpu_number_map[physid] = cpu;
>>>>>> + __cpu_logical_map[cpu] = physid;
>>>>>> acpi_map_cpu2node(handle, cpu, physid);
>>>>>>
>>>>>> *pcpu = cpu;
>>>>>> diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/setup.c
>>>>>> index 00e307203ddb..649e98640076 100644
>>>>>> --- a/arch/loongarch/kernel/setup.c
>>>>>> +++ b/arch/loongarch/kernel/setup.c
>>>>>> @@ -65,6 +65,8 @@ EXPORT_SYMBOL(cpu_data);
>>>>>>
>>>>>> struct loongson_board_info b_info;
>>>>>> static const char dmi_empty_string[] = " ";
>>>>>> +static int possible_cpus;
>>>>>> +static bool bsp_added;
>>>>>>
>>>>>> /*
>>>>>> * Setup information
>>>>>> @@ -346,10 +348,55 @@ static void __init bootcmdline_init(char **cmdline_p)
>>>>>> *cmdline_p = boot_command_line;
>>>>>> }
>>>>>>
>>>>>> +int topo_get_cpu(int physid)
>>>>>> +{
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < possible_cpus; i++)
>>>>>> + if (cpu_logical_map(i) == physid)
>>>>>> + break;
>>>>>> +
>>>>>> + if (i == possible_cpus)
>>>>>> + return -ENOENT;
>>>>>> +
>>>>>> + return i;
>>>>>> +}
>>>>>> +
>>>>>> +int topo_add_cpu(int physid)
>>>>>> +{
>>>>>> + int cpu;
>>>>>> +
>>>>>> + if (!bsp_added && (physid == loongson_sysconf.boot_cpu_id)) {
>>>>>> + bsp_added = true;
>>>>>> + return 0;
>>>>>> + }
>>>>>> +
>>>>>> + cpu = topo_get_cpu(physid);
>>>>>> + if (cpu >= 0) {
>>>>>> + pr_warn("Adding duplicated physical cpuid 0x%x\n", physid);
>>>>>> + return -EEXIST;
>>>>>> + }
>>>>>> +
>>>>>> + if (possible_cpus >= nr_cpu_ids)
>>>>>> + return -ERANGE;
>>>>>> +
>>>>>> + __cpu_logical_map[possible_cpus] = physid;
>>>>>> + cpu = possible_cpus++;
>>>>>> + return cpu;
>>>>>> +}
>>>>>> +
>>>>>> +static void __init topo_init(void)
>>>>>> +{
>>>>>> + loongson_sysconf.boot_cpu_id = read_csr_cpuid();
>>>>>> + __cpu_logical_map[0] = loongson_sysconf.boot_cpu_id;
>>>>>> + possible_cpus++;
>>>>>> +}
>>>>>> +
>>>>>> void __init platform_init(void)
>>>>>> {
>>>>>> arch_reserve_vmcore();
>>>>>> arch_reserve_crashkernel();
>>>>>> + topo_init();
>>>>>>
>>>>>> #ifdef CONFIG_ACPI
>>>>>> acpi_table_upgrade();
>>>>>> diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
>>>>>> index 9afc2d8b3414..a3f466b89179 100644
>>>>>> --- a/arch/loongarch/kernel/smp.c
>>>>>> +++ b/arch/loongarch/kernel/smp.c
>>>>>> @@ -291,10 +291,9 @@ static void __init fdt_smp_setup(void)
>>>>>> if (cpuid >= nr_cpu_ids)
>>>>>> continue;
>>>>>>
>>>>>> - if (cpuid == loongson_sysconf.boot_cpu_id)
>>>>>> - cpu = 0;
>>>>>> - else
>>>>>> - cpu = find_first_zero_bit(cpumask_bits(cpu_present_mask), NR_CPUS);
>>>>>> + cpu = topo_add_cpu(cpuid);
>>>>>> + if (cpu < 0)
>>>>>> + continue;
>>>>>>
>>>>>> num_processors++;
>>>>>> set_cpu_possible(cpu, true);
>>>>>> @@ -302,7 +301,7 @@ static void __init fdt_smp_setup(void)
>>>>>> __cpu_number_map[cpuid] = cpu;
>>>>>> __cpu_logical_map[cpu] = cpuid;
>>>>>>
>>>>>> - early_numa_add_cpu(cpu, 0);
>>>>>> + early_numa_add_cpu(cpuid, 0);
>>>>>> set_cpuid_to_node(cpuid, 0);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
>>>>>> --
>>>>>> 2.39.3
>>>>>>
>>>>
>>>>
>>
Powered by blists - more mailing lists