Message-ID: <5408854B.9010703@linaro.org>
Date:	Thu, 04 Sep 2014 23:29:15 +0800
From:	Hanjun Guo <hanjun.guo@...aro.org>
To:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
CC:	Catalin Marinas <Catalin.Marinas@....com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Mark Rutland <Mark.Rutland@....com>,
	Olof Johansson <olof@...om.net>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"graeme.gregory@...aro.org" <graeme.gregory@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	Sudeep Holla <Sudeep.Holla@....com>,
	Will Deacon <Will.Deacon@....com>,
	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <Marc.Zyngier@....com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
	Robert Richter <rric@...nel.org>,
	Lv Zheng <lv.zheng@...el.com>,
	Robert Moore <robert.moore@...el.com>,
	Liviu Dudau <Liviu.Dudau@....com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Charles Garcia-Tobin <Charles.Garcia-Tobin@....com>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linaro-acpi@...ts.linaro.org" <linaro-acpi@...ts.linaro.org>,
	Tomasz Nowicki <tomasz.nowicki@...aro.org>
Subject: Re: [PATCH v3 09/17] ARM64 / ACPI: Parse MADT for SMP initialization

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.

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

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

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

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

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists