From: hanjun.guo@linaro.org (Hanjun Guo)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v3 09/17] ARM64 / ACPI: Parse MADT for SMP initialization
Date: Thu, 04 Sep 2014 23:29:15 +0800 [thread overview]
Message-ID: <5408854B.9010703@linaro.org> (raw)
In-Reply-To: <20140903172138.GG1824@e102568-lin.cambridge.arm.com>
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
next prev parent reply other threads:[~2014-09-04 15:29 UTC|newest]
Thread overview: 117+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-01 14:57 [PATCH v3 00/17] Introduce ACPI for ARM64 based on ACPI 5.1 Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 01/17] ARM64: Move the init of cpu_logical_map(0) before unflatten_device_tree() Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 02/17] ARM64 / ACPI: Get RSDP and ACPI boot-time tables Hanjun Guo
2014-09-09 16:26 ` Catalin Marinas
2014-09-09 16:41 ` Jon Masters
2014-09-09 16:44 ` Jon Masters
2014-09-09 17:15 ` Mark Rutland
2014-09-09 17:33 ` Jon Masters
2014-09-09 17:50 ` Lorenzo Pieralisi
2014-09-09 18:05 ` Sudeep Holla
2014-09-09 19:06 ` Jon Masters
2014-09-10 11:13 ` Hanjun Guo
2014-09-10 12:33 ` Catalin Marinas
2014-09-10 21:51 ` Grant Likely
2014-09-11 11:01 ` Catalin Marinas
2014-09-14 15:40 ` Grant Likely
2014-09-14 21:59 ` Catalin Marinas
2014-09-15 3:53 ` Grant Likely
2014-09-16 5:29 ` Zheng, Lv
2014-09-10 21:41 ` Grant Likely
2014-09-09 16:54 ` Mark Rutland
2014-09-10 7:30 ` Hanjun Guo
2014-09-10 21:37 ` Grant Likely
2014-09-01 14:57 ` [PATCH v3 03/17] ARM64 / ACPI: Introduce lowlevel suspend function Hanjun Guo
2014-09-09 16:35 ` Catalin Marinas
2014-09-09 22:04 ` Graeme Gregory
2014-09-01 14:57 ` [PATCH v3 04/17] ARM64 / ACPI: Introduce early_param for "acpi" Hanjun Guo
2014-09-09 16:37 ` Catalin Marinas
2014-09-09 17:17 ` Bjorn Helgaas
2014-09-09 22:14 ` Jon Masters
2014-09-10 13:04 ` Will Deacon
2014-09-10 13:21 ` Bjorn Helgaas
2014-09-10 18:30 ` Will Deacon
2014-09-10 21:58 ` Grant Likely
2014-09-01 14:57 ` [PATCH v3 05/17] ARM64 / ACPI: If we chose to boot from acpi then disable FDT Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 06/17] ARM64 / ACPI: Make PCI optional for ACPI on ARM64 Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 07/17] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 08/17] ACPI / table: Print GIC information when MADT is parsed Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 09/17] ARM64 / ACPI: Parse MADT for SMP initialization Hanjun Guo
2014-09-03 17:21 ` Lorenzo Pieralisi
2014-09-04 15:29 ` Hanjun Guo [this message]
2014-09-09 4:29 ` Jon Masters
2014-09-09 5:11 ` Hanjun Guo
2014-09-09 5:34 ` Jon Masters
2014-09-09 16:52 ` Lorenzo Pieralisi
2014-09-09 17:00 ` Jon Masters
2014-09-09 17:02 ` Jon Masters
2014-09-09 4:23 ` Jon Masters
2014-09-09 4:57 ` Hanjun Guo
2014-09-09 5:44 ` Jon Masters
2014-09-09 16:00 ` Hanjun Guo
2014-09-09 16:04 ` Jon Masters
2014-09-09 16:14 ` Hanjun Guo
2014-09-11 14:15 ` Will Deacon
2014-09-12 21:30 ` Jon Masters
2014-09-11 10:24 ` Grant Likely
2014-09-01 14:57 ` [PATCH v3 10/17] ACPI / processor: Make it possible to get CPU hardware ID via GICC Hanjun Guo
2014-09-03 16:27 ` Lorenzo Pieralisi
2014-09-08 13:10 ` Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 11/17] ARM64 / ACPI: Introduce ACPI_IRQ_MODEL_GIC and register device's gsi Hanjun Guo
2014-09-11 11:08 ` Grant Likely
2014-09-11 11:34 ` Grant Likely
2014-09-12 9:42 ` Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 12/17] ACPI / table: Add new function to get table entries Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support Hanjun Guo
2014-09-01 17:35 ` Marc Zyngier
2014-09-02 8:28 ` [Linaro-acpi] " Alexander Spyridakis
2014-09-02 11:48 ` Tomasz Nowicki
2014-09-02 13:02 ` Marc Zyngier
2014-09-02 15:45 ` Hanjun Guo
2014-09-02 15:59 ` Marc Zyngier
2014-09-02 16:11 ` Sudeep Holla
2014-09-03 10:30 ` Marc Zyngier
2014-09-03 11:17 ` Hanjun Guo
2014-09-04 14:03 ` Hanjun Guo
2014-09-09 6:21 ` Jon Masters
2014-09-03 9:26 ` Tomasz Nowicki
2014-09-03 14:57 ` Arnd Bergmann
2014-09-05 8:52 ` Tomasz Nowicki
2014-09-05 9:47 ` Marc Zyngier
2014-09-05 10:13 ` [Linaro-acpi] " Arnd Bergmann
2014-09-05 10:36 ` Tomasz Nowicki
2014-09-05 10:39 ` Marc Zyngier
2014-09-05 10:49 ` Tomasz Nowicki
2014-09-09 6:27 ` Jon Masters
2014-09-11 13:43 ` Grant Likely
2014-09-02 16:34 ` Catalin Marinas
2014-09-11 11:48 ` Grant Likely
2014-09-11 12:01 ` Marc Zyngier
2014-09-09 6:14 ` Jon Masters
2014-09-03 18:42 ` Arnd Bergmann
2014-09-04 10:10 ` Tomasz Nowicki
2014-09-04 10:14 ` Arnd Bergmann
2014-09-04 10:39 ` Tomasz Nowicki
2014-09-09 6:35 ` Jon Masters
2014-09-01 14:57 ` [PATCH v3 14/17] ARM64 / ACPI: Parse GTDT to initialize arch timer Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 15/17] ARM64 / ACPI: Select ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled on ARM64 Hanjun Guo
2014-09-01 14:57 ` [PATCH v3 16/17] ARM64 / ACPI: Enable ARM64 in Kconfig Hanjun Guo
2014-09-11 15:18 ` Lorenzo Pieralisi
2014-09-01 14:57 ` [PATCH v3 17/17] Documentation: ACPI for ARM64 Hanjun Guo
2014-09-11 13:29 ` [PATCH v3 00/17] Introduce ACPI for ARM64 based on ACPI 5.1 Grant Likely
2014-09-11 13:49 ` Will Deacon
2014-09-12 21:38 ` Jon Masters
2014-09-12 21:43 ` Jon Masters
2014-09-15 4:21 ` Grant Likely
2014-09-11 14:23 ` Rafael J. Wysocki
2014-09-11 14:04 ` Grant Likely
2014-09-11 15:37 ` Catalin Marinas
2014-09-11 15:57 ` Sudeep Holla
2014-09-11 16:06 ` Graeme Gregory
2014-09-11 16:14 ` Sudeep Holla
2014-09-15 4:31 ` Grant Likely
2014-09-15 9:15 ` Catalin Marinas
2014-09-15 22:48 ` Grant Likely
2014-09-16 10:12 ` Catalin Marinas
2014-09-11 16:05 ` Olof Johansson
2014-09-15 4:37 ` Grant Likely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5408854B.9010703@linaro.org \
--to=hanjun.guo@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).