On Thu, Sep 04, 2014 at 04:29:15PM +0100, Hanjun Guo wrote:

Hi Lorenzo,

On 2014?09?04? 01:21, Lorenzo Pieralisi wrote:

quoted

On Mon, Sep 01, 2014 at 03:57:47PM +0100, Hanjun Guo wrote:

quoted

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.

[...]

quoted

quoted

 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.

quoted

quoted

+
 /*
  * __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).

quoted

quoted

+	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/

quoted

+		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.

quoted

quoted

+	}
+
+	/* 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.

quoted

quoted

+	/*
+	 * 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.

quoted

quoted

+
+	/* 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

quoted

quoted

+	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.

quoted

quoted

+
+	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