On Thu, Sep 04, 2014 at 04:29:15PM +0100, Hanjun Guo wrote:
Hi Lorenzo,
On 2014?09?04? 01:21, Lorenzo Pieralisi wrote:
On Mon, Sep 01, 2014 at 03:57:47PM +0100, Hanjun Guo wrote:
MADT contains the information for MPIDR which is essential for SMP initialization, parse the GIC cpu interface structures to get the MPIDR value and map it to cpu_logical_map(), and add enabled cpu with valid MPIDR into cpu_possible_map.
ACPI 5.1 only has two explicit methods to boot up SMP, PSCI and Parking protocol, but the Parking protocol is only specified for ARMv7 now, so make PSCI as the only way for the SMP boot protocol before some updates for the ACPI spec or the Parking protocol spec.
[...]
int acpi_noirq; /* skip ACPI IRQ initialization */ int acpi_disabled; EXPORT_SYMBOL(acpi_disabled); @@ -31,6 +35,8 @@ EXPORT_SYMBOL(acpi_disabled); int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ EXPORT_SYMBOL(acpi_pci_disabled); +static int enabled_cpus; /* Processors (GICC) with enabled flag in MADT */
Will this be ever different from (num_possible_cpus() - 1) ?
Yes, num_possible_cpus() will much more than enabled cpus in MADT, when ACPI based CPU hot plug is introduced, you can refer to the code in x86.
Ok, but in the context of this patch to me they represent the same value. I understand you need a counter, which you should probably use to enumerate the logical cpus instead of resorting to the first empty slot in cpu_possible_mask.
Anyway, it is a minor point, please be consistent that's all I am asking.
/*
- __acpi_map_table() will be called before page_init(), so early_ioremap()
- or early_memremap() should be called here to for ACPI table mapping.
@@ -51,6 +57,144 @@ void __init __acpi_unmap_table(char *map, unsigned long size) early_memunmap(map, size); } +/**
- acpi_map_gic_cpu_interface - generates a logical cpu number
- and map to MPIDR represented by GICC structure
- @mpidr: CPU's hardware id to register, MPIDR represented in MADT
- @enabled: this cpu is enabled or not
- Returns the logical cpu number which maps to MPIDR
- */
+static int acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) +{
- int cpu;
- if (mpidr == INVALID_HWID) {
pr_info("Skip invalid cpu hardware ID\n");
return -EINVAL;
- }
- total_cpus++;
What's this used for ?
It is for all the CPU entries in MADT table, it is used to let people know how many CPUs in MADT (enabled and disabled).
I think its usage is very limited at the moment, again it is not a major point, I was just asking, I certainly do not think it is essential at this stage (apart from debugging the parsing code).
- if (!enabled)
return -EINVAL;
- if (enabled_cpus >= NR_CPUS) {
pr_warn("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n",
NR_CPUS, total_cpus, mpidr);
return -EINVAL;
- }
- /* No need to check duplicate MPIDRs for the first CPU */
- if (enabled_cpus) {
/*
* Duplicate MPIDRs are a recipe for disaster. Scan
* all initialized entries and check for
* duplicates. If any is found just ignore the CPU.
*/
for_each_possible_cpu(cpu) {
if (cpu_logical_map(cpu) == mpidr) {
pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
mpidr);
return -EINVAL;
}
}
- } else {
/* Fist GICC entry must be BSP as ACPI spec said */
s/Fist/First/
if (cpu_logical_map(0) != mpidr) {
pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n",
mpidr);
return -EINVAL;
}
Interesting, this means that if I want to change the boot CPU I have to recompile the ACPI tables. Is that really true ?
No, you needn't. there is a logic problem here, we just need to print some message here and continue, OS will still ok with that.
I need to look at the specs here. I do not like fixed dependencies on the boot CPU, which risk being translated in dependencies on first/last CPU going-to/getting-out-of idle and that is a major concern, among others.
- }
- /* allocate a logical cpu id for the new comer */
- if (cpu_logical_map(0) == mpidr) {
/*
* boot_cpu_init() already hold bit 0 in cpu_present_mask
* for BSP, no need to allocate again.
*/
cpu = 0;
- } else {
cpu = cpumask_next_zero(-1, cpu_possible_mask);
- }
You may use a ternary operator, more compact and clearer.
BTW you seem to be contradicting yourself. On one hand you keep a counter for enabled_cpus, and then use cpu_possible_mask to allocate a logical cpu id. Make a decision, either you use a counter or you use cpu_possible_mask and its bitweight.
ok.
- /*
* ACPI 5.1 only has two explicit methods to boot up SMP,
* PSCI and Parking protocol, but the Parking protocol is
* only specified for ARMv7 now, so make PSCI as the only
* way for the SMP boot protocol before some updates for
* the ACPI spec or the Parking protocol spec.
*/
- if (!acpi_psci_present()) {
pr_warn("CPU %d has no PSCI support, will not boot\n", cpu);
return -EOPNOTSUPP;
- }
This check really does not belong here. You do not even start parsing the gic cpu interfaces if psci is missing or I am missing something myself. Anyway, this check must not be in this function.
I agree with you, i will update the patch.
- /* Get cpu_ops include the boot CPU */
- cpu_ops[cpu] = cpu_get_ops("psci");
- if (!cpu_ops[cpu])
return -EINVAL;
- /* CPU 0 was already initialized */
- if (cpu) {
if (cpu_ops[cpu]->cpu_init(NULL, cpu))
return -EOPNOTSUPP;
/* map the logical cpu id to cpu MPIDR */
cpu_logical_map(cpu) = mpidr;
set_cpu_possible(cpu, true);
- }
- enabled_cpus++;
See above to me enabled_cpus and (num_possible_cpus() - 1) are identical.
I think I need to remove all the CPU hotplug related code and make this function as simple as possible and introduce them when needed.
Yes that makes sense, even though a bit of foresight is always appreciated; I certainly do not want you to completely rewrite this code to support CPU hotplug to be 100% clear. "Disabled" CPUs is a concept that is not managed at the moment with DT (on ARM and ARM64), and we need to introduce it properly. Again, I was asking questions, to understand why you would need those variables.
Have a look at this discussion:
https://lkml.org/lkml/2013/6/6/470
- return cpu;
+}
+static int __init +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
const unsigned long end)
+{
- struct acpi_madt_generic_interrupt *processor;
- processor = (struct acpi_madt_generic_interrupt *)header;
- if (BAD_MADT_ENTRY(processor, end))
return -EINVAL;
- acpi_table_print_madt_entry(header);
- acpi_map_gic_cpu_interface(processor->arm_mpidr,
processor->flags & ACPI_MADT_ENABLED);
Ehm. You must check the return value here right (and return an error if that's an error, otherwise the count value below can be botched ?!).
Or you do not consider a parsing error as an error and want to keep parsing remaining GIC CPU IF entries ?
yes, this is my intension. we can skip the error ones and boot other CPUs which have no errors.
- return 0;
+}
+/* Parse GIC cpu interface entries in MADT for SMP init */ +void __init acpi_smp_init_cpus(void) +{
- int count;
- /*
* do a partial walk of MADT to determine how many CPUs
* we have including disabled CPUs, and get information
* we need for SMP init
*/
- count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
acpi_parse_gic_cpu_interface,
ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
- if (!count) {
pr_err("No GIC CPU interface entries present\n");
return;
- } else if (count < 0) {
pr_err("Error parsing GIC CPU interface entry\n");
return;
- }
What would you consider an error ? A single GIC CPU IF entry error ?
could you please explain it in detail? I can't catch up with you, my apologizes.
You explained to me above. A bogus entry does not stop you from parsing other CPUs, this is a design choice and that's what we do in ARM64 DT today, so I would say that's fine.
Lorenzo