| Message ID | 20200226002356.86986-5-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Dereference CPU operations indirectly | expand |
On Wed, Feb 26, 2020 at 11:23:55AM +1100, Gavin Shan wrote: > One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB > memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too > much memory has been used for this. Also, all CPUs must use same CPU > operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi > pointed out. I suspect there might be some pre PSCIv0.2 platforms where the boot CPU doesn't have PSCI, but others do. On those platforms, this could be because CPU0 cannot be hotplugged out, and we must avoid doing so. Can you check the in-kernel DTs to see if any of those exist? Other than that, I agree that mandating uniformity is the best approach here. > int __init init_cpu_ops(int cpu) > { > - const char *enable_method = cpu_read_enable_method(cpu); > - > - if (!enable_method) > - return -ENODEV; > + const struct cpu_operations *ops = get_cpu_method(cpu); > > - cpu_ops[cpu] = cpu_get_ops(enable_method); > - if (!cpu_ops[cpu]) { > - pr_warn("Unsupported enable-method: %s\n", enable_method); > + if (!ops) > return -EOPNOTSUPP; > + > + /* Update global CPU operations if it's not initialized yet */ > + if (!cpu_ops) { > + cpu_ops = ops; > + return 0; > + } As above, I don't think this is quite right. If we're going to mandate uniformity, we should init the ops from the boot CPU, and then verify that every other CPU matches that. The initialization of the global ops should not be conditional. Thanks, Mark.
On 3/17/20 9:56 PM, Mark Rutland wrote: > On Wed, Feb 26, 2020 at 11:23:55AM +1100, Gavin Shan wrote: >> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB >> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too >> much memory has been used for this. Also, all CPUs must use same CPU >> operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi >> pointed out. > > I suspect there might be some pre PSCIv0.2 platforms where the boot CPU > doesn't have PSCI, but others do. On those platforms, this could be > because CPU0 cannot be hotplugged out, and we must avoid doing so. > > Can you check the in-kernel DTs to see if any of those exist? > > Other than that, I agree that mandating uniformity is the best approach > here. > Thanks for the comments, Mark. There are platforms where the CPU#0 and other CPUs have different "enable-method" specified. More specificly, CPU#0 doesn't have "enable-method" while other CPUs have "psci" specified: lg/lg1312.dtsi lg/lg1313.dtsi mediatek/mt2712e.dtsi In order to support two enable methods, I think we have two options here: (1) Revert the code to what we had in v2. Two bits consumed by one CPU, which is taken as index to a CPU operation array. The code can be found in link [1] (2) Two CPU operation pointers are maintained. One is used to track the CPU#0's operations and other one is for other cpus. [1] https://patchwork.kernel.org/patch/11363745/ Please let me know which one is better. >> int __init init_cpu_ops(int cpu) >> { >> - const char *enable_method = cpu_read_enable_method(cpu); >> - >> - if (!enable_method) >> - return -ENODEV; >> + const struct cpu_operations *ops = get_cpu_method(cpu); >> >> - cpu_ops[cpu] = cpu_get_ops(enable_method); >> - if (!cpu_ops[cpu]) { >> - pr_warn("Unsupported enable-method: %s\n", enable_method); >> + if (!ops) >> return -EOPNOTSUPP; >> + >> + /* Update global CPU operations if it's not initialized yet */ >> + if (!cpu_ops) { >> + cpu_ops = ops; >> + return 0; >> + } > > As above, I don't think this is quite right. If we're going to mandate > uniformity, we should init the ops from the boot CPU, and then verify > that every other CPU matches that. The initialization of the global ops > should not be conditional.> I think you're correct because CPU#0's "enable-method" can be unspecified. In this case, the CPU#0's operations will be set to same one as other CPUs, which isn't correct. I will see how to handle this comments when the above comments is resolved. This might become invalid after the above comments get resolved. Thanks, Gavin
On 3/18/20 2:53 PM, Gavin Shan wrote: > On 3/17/20 9:56 PM, Mark Rutland wrote: >> On Wed, Feb 26, 2020 at 11:23:55AM +1100, Gavin Shan wrote: >>> One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB >>> memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too >>> much memory has been used for this. Also, all CPUs must use same CPU >>> operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi >>> pointed out. >> >> I suspect there might be some pre PSCIv0.2 platforms where the boot CPU >> doesn't have PSCI, but others do. On those platforms, this could be >> because CPU0 cannot be hotplugged out, and we must avoid doing so. >> >> Can you check the in-kernel DTs to see if any of those exist? >> >> Other than that, I agree that mandating uniformity is the best approach >> here. >> > > Thanks for the comments, Mark. There are platforms where the CPU#0 and other > CPUs have different "enable-method" specified. More specificly, CPU#0 doesn't > have "enable-method" while other CPUs have "psci" specified: > > lg/lg1312.dtsi > lg/lg1313.dtsi > mediatek/mt2712e.dtsi > > In order to support two enable methods, I think we have two options here: > (1) Revert the code to what we had in v2. Two bits consumed by one CPU, > which is taken as index to a CPU operation array. The code can be found in > link [1] (2) Two CPU operation pointers are maintained. One is used to track > the CPU#0's operations and other one is for other cpus. > > [1] https://patchwork.kernel.org/patch/11363745/ > > Please let me know which one is better. > Mark, I plan to post a new revision to have option#2 after thinking about it: we will have two pointers to track the operations for boot cpu and the secondary CPUs separately. the consumed memory isn't scaled up to the configured CPU number. So I think it's better than option#1, which uses two-bits as index to CPU operation array. Thanks, Gavin >>> int __init init_cpu_ops(int cpu) >>> { >>> - const char *enable_method = cpu_read_enable_method(cpu); >>> - >>> - if (!enable_method) >>> - return -ENODEV; >>> + const struct cpu_operations *ops = get_cpu_method(cpu); >>> - cpu_ops[cpu] = cpu_get_ops(enable_method); >>> - if (!cpu_ops[cpu]) { >>> - pr_warn("Unsupported enable-method: %s\n", enable_method); >>> + if (!ops) >>> return -EOPNOTSUPP; >>> + >>> + /* Update global CPU operations if it's not initialized yet */ >>> + if (!cpu_ops) { >>> + cpu_ops = ops; >>> + return 0; >>> + } >> >> As above, I don't think this is quite right. If we're going to mandate >> uniformity, we should init the ops from the boot CPU, and then verify >> that every other CPU matches that. The initialization of the global ops >> should not be conditional.> > I think you're correct because CPU#0's "enable-method" can be unspecified. > In this case, the CPU#0's operations will be set to same one as other CPUs, > which isn't correct. I will see how to handle this comments when the above > comments is resolved. This might become invalid after the above comments get > resolved. > > Thanks, > Gavin >
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c index e133011f64b5..af73ca502b95 100644 --- a/arch/arm64/kernel/cpu_ops.c +++ b/arch/arm64/kernel/cpu_ops.c @@ -20,41 +20,19 @@ extern const struct cpu_operations acpi_parking_protocol_ops; #endif extern const struct cpu_operations cpu_psci_ops; -static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init; - -static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = { +static const struct cpu_operations *const available_cpu_ops[] __initconst = { &smp_spin_table_ops, - &cpu_psci_ops, - NULL, -}; - -static const struct cpu_operations *const acpi_supported_cpu_ops[] __initconst = { #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL &acpi_parking_protocol_ops, #endif &cpu_psci_ops, - NULL, }; +static const struct cpu_operations *cpu_ops __ro_after_init; -static const struct cpu_operations * __init cpu_get_ops(const char *name) -{ - const struct cpu_operations *const *ops; - - ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops; - - while (*ops) { - if (!strcmp(name, (*ops)->name)) - return *ops; - - ops++; - } - - return NULL; -} - -static const char *__init cpu_read_enable_method(int cpu) +static const struct cpu_operations * __init get_cpu_method(int cpu) { const char *enable_method; + int i; if (acpi_disabled) { struct device_node *dn = of_get_cpu_node(cpu, NULL); @@ -91,22 +69,38 @@ static const char *__init cpu_read_enable_method(int cpu) } } - return enable_method; + if (!enable_method) { + pr_warn("No enable-method found on CPU %d\n", cpu); + return NULL; + } + + /* Search in the array with method */ + for (i = 0; i < ARRAY_SIZE(available_cpu_ops); i++) { + if (!strcmp(available_cpu_ops[i]->name, enable_method)) + return available_cpu_ops[i]; + } + + return NULL; } -/* - * Read a cpu's enable method and record it in cpu_ops. - */ + int __init init_cpu_ops(int cpu) { - const char *enable_method = cpu_read_enable_method(cpu); - - if (!enable_method) - return -ENODEV; + const struct cpu_operations *ops = get_cpu_method(cpu); - cpu_ops[cpu] = cpu_get_ops(enable_method); - if (!cpu_ops[cpu]) { - pr_warn("Unsupported enable-method: %s\n", enable_method); + if (!ops) return -EOPNOTSUPP; + + /* Update global CPU operations if it's not initialized yet */ + if (!cpu_ops) { + cpu_ops = ops; + return 0; + } + + /* We should have unified CPU operations */ + if (ops != cpu_ops) { + pr_warn("Invalid CPU operations %s (%s) on CPU %d\n", + ops->name, cpu_ops->name, cpu); + return -EINVAL; } return 0; @@ -114,5 +108,5 @@ int __init init_cpu_ops(int cpu) const struct cpu_operations *get_cpu_ops(int cpu) { - return cpu_ops[cpu]; + return cpu_ops; }
One CPU operations is maintained through array @cpu_ops[NR_CPUS]. 2KB memory is consumed when CONFIG_NR_CPUS is set to 256. It seems too much memory has been used for this. Also, all CPUs must use same CPU operations and we shouldn't bring up the broken CPU, as Lorenzo Pieralisi pointed out. This introduces variable (@cpu_ops) to store the unfied CPU operations. Those CPUs using different operations, which is figured out from device tree or ACPI table, won't be brought up. With this, the CPU operations dereferencing array is removed and 2KB memory is saved. Note the logic of cpu_get_ops() is merged to get_cpu_method() since the logic is simple enough and no need to have a function for that. Link: https://lore.kernel.org/linux-arm-kernel/20200211114553.GA21093@e121166-lin.cambridge.arm.com Signed-off-by: Gavin Shan <gshan@redhat.com> --- arch/arm64/kernel/cpu_ops.c | 70 +++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 38 deletions(-)