| 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: <CAAhV-H5jGZz2MeCSuLmJb5b-ugaaj3EECD7Z3mvtHW=OQrhLBw@mail.gmail.com>
Date: Wed, 30 Oct 2024 16:12:40 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: maobibo <maobibo@...ngson.cn>
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 Tue, Oct 29, 2024 at 7:49 PM maobibo <maobibo@...ngson.cn> wrote:
>
>
>
> 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.
> >
> > 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.
> I do not understand such requirement about logical cpu should be
> continuous. You can check logical cpu allocation method on other
> architectures, or what does the requirement about logical cpu continuous
> come from.
1, In an internal conference, it is said that non-continuous cpu
numbers make users think our processors have bugs.
2, See prefill_possible_map(), it assumes logical numbers continuous
cpu_possible_mask and cpu_present_mask, which make it convenient for
"nr_cpus=xxx".
3, Can you show me an example in a real machine that "processor" in
/proc/cpuinfo non-continues after boot and before soft hotplug?
Huacai
>
> Regards
> Bibo Mao
>
> >
> > 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