On Wed, Jan 27, 2016 at 11:23:41AM +0100, Ard Biesheuvel wrote:
[...]
> > It is to report that this patch currently works on my AMD Seattle with
> > a v4.5-rc1 kernel whilst it was not working before owing to FW putting
> > the mailboxes in WB memory (that was part of linear mapping, I still have
> > to pinpoint the exact reason). I think it is related to recent changes in
> > EFI and memblock. The code in this patch is allowed to map mailboxes
> > through ioremap() (ie it was not before because IIUC pfn_valid was returning
> > true on the mailboxes addresses), which it looks like it changed with:
> >
> > 68709f45385a ("arm64: only consider memblocks with NOMAP cleared for
> > linear mapping")
> >
> > Basically we end up mapping a memory area with ioremap with attributes
> > that may mismatch EFI descriptors, on my AMD Seattle the mailboxes are
> > part of runtime services data that is WB (see below).
> >
>
> The '|WB|WT|WC|UC]' means it can be mapped in various ways, and since
> we dropped it from the linear mapping, it should tolerate being mapped
> as device memory, with the caveat that, since it is tagged as
> EFI_MEMORY_RUNTIME as well, it will be mapped cacheable during
> invocations to EFI runtime services.
>
> So the issue is not in the memory types it exposes, but in the fact
> that it is listed as a region that is subject to VA remapping.
Thanks for clarifying and yes, I agree it is a firmware bug, I just
flagged this up to check with you whether we want to do something
about it (ie it is not just related to this patch).
> > I think the only way I can prevent this is by looking up the
> > mailboxes addresses in the EFI memory map and bail out if the
> > memory descriptor is marked as WB (according to the ACPI parking
> > protocol the mailboxes must be Device-nGnRnE).
> >
>
> This is a firmware bug, and should be caught at the ACPI/UEFI
> validation level. I am not sure we have to special case this in the
> kernel.
I came to the same conclusion, I thought it was worth mentioning
it since with previous kernel versions this mismatch was caught in
the ioremap implementation and now it is not, if a warning has to be
put in place it has to be done separately, it is not an issue
specific to this patch only.
Thanks,
Lorenzo
>
> --
> Ard.
>
>
>
> > AMD Seattle snip boot log with this patch applied and some debug output
> > below (ie mailbox address falls within "Runtime Data").
> >
> > I will keep debugging, comments appreciated.
> >
> > Thanks,
> > Lorenzo
> >
> > [ 0.000000] Processing EFI memory map:
> >
> > <snip>
> >
> > [ 0.000000] 0x008028000000-0x0080280fffff [Runtime Data |RUN| | | | | | |WB|WT|WC|UC]*
> > [ 0.000000] acpi_parking_protocol_cpu_init: ACPI parked addr=80280c1000
> >
> >> - Moved Kconfig option under "Boot options"
> >>
> >> v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/369121.html
> >>
> >> v1->v2
> >>
> >> - Rebased against v4.2
> >> - Removed SMP dependency (it was removed from arm64)
> >> - Updated some comments
> >> - Clarified 64k page mailbox alignment and requested UEFI specs update
> >>
> >> v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/356750.html
> >>
> >> arch/arm64/Kconfig | 9 ++
> >> arch/arm64/include/asm/acpi.h | 19 +++-
> >> arch/arm64/include/asm/hardirq.h | 2 +-
> >> arch/arm64/include/asm/smp.h | 9 ++
> >> arch/arm64/kernel/Makefile | 1 +
> >> arch/arm64/kernel/acpi_parking_protocol.c | 153 ++++++++++++++++++++++++++++++
> >> arch/arm64/kernel/cpu_ops.c | 27 +++++-
> >> arch/arm64/kernel/smp.c | 22 +++++
> >> 8 files changed, 236 insertions(+), 6 deletions(-)
> >> create mode 100644 arch/arm64/kernel/acpi_parking_protocol.c
> >>
> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >> index 8cc6228..53e48a6 100644
> >> --- a/arch/arm64/Kconfig
> >> +++ b/arch/arm64/Kconfig
> >> @@ -756,6 +756,15 @@ endmenu
> >>
> >> menu "Boot options"
> >>
> >> +config ARM64_ACPI_PARKING_PROTOCOL
> >> + bool "Enable support for the ARM64 ACPI parking protocol"
> >> + depends on ACPI
> >> + help
> >> + Enable support for the ARM64 ACPI parking protocol. If disabled
> >> + the kernel will not allow booting through the ARM64 ACPI parking
> >> + protocol even if the corresponding data is present in the ACPI
> >> + MADT table.
> >> +
> >> config CMDLINE
> >> string "Default kernel command string"
> >> default ""
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index caafd63..aee323b 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -87,9 +87,26 @@ void __init acpi_init_cpus(void);
> >> static inline void acpi_init_cpus(void) { }
> >> #endif /* CONFIG_ACPI */
> >>
> >> +#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
> >> +bool acpi_parking_protocol_valid(int cpu);
> >> +void __init
> >> +acpi_set_mailbox_entry(int cpu, struct acpi_madt_generic_interrupt *processor);
> >> +#else
> >> +static inline bool acpi_parking_protocol_valid(int cpu) { return false; }
> >> +static inline void
> >> +acpi_set_mailbox_entry(int cpu, struct acpi_madt_generic_interrupt *processor)
> >> +{}
> >> +#endif
> >> +
> >> static inline const char *acpi_get_enable_method(int cpu)
> >> {
> >> - return acpi_psci_present() ? "psci" : NULL;
> >> + if (acpi_psci_present())
> >> + return "psci";
> >> +
> >> + if (acpi_parking_protocol_valid(cpu))
> >> + return "parking-protocol";
> >> +
> >> + return NULL;
> >> }
> >>
> >> #ifdef CONFIG_ACPI_APEI
> >> diff --git a/arch/arm64/include/asm/hardirq.h b/arch/arm64/include/asm/hardirq.h
> >> index a57601f..8740297 100644
> >> --- a/arch/arm64/include/asm/hardirq.h
> >> +++ b/arch/arm64/include/asm/hardirq.h
> >> @@ -20,7 +20,7 @@
> >> #include <linux/threads.h>
> >> #include <asm/irq.h>
> >>
> >> -#define NR_IPI 5
> >> +#define NR_IPI 6
> >>
> >> typedef struct {
> >> unsigned int __softirq_pending;
> >> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
> >> index d9c3d6a..2013a4d 100644
> >> --- a/arch/arm64/include/asm/smp.h
> >> +++ b/arch/arm64/include/asm/smp.h
> >> @@ -64,6 +64,15 @@ extern void secondary_entry(void);
> >> extern void arch_send_call_function_single_ipi(int cpu);
> >> extern void arch_send_call_function_ipi_mask(const struct cpumask *mask);
> >>
> >> +#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
> >> +extern void arch_send_wakeup_ipi_mask(const struct cpumask *mask);
> >> +#else
> >> +static inline void arch_send_wakeup_ipi_mask(const struct cpumask *mask)
> >> +{
> >> + BUILD_BUG();
> >> +}
> >> +#endif
> >> +
> >> extern int __cpu_disable(void);
> >>
> >> extern void __cpu_die(unsigned int cpu);
> >> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> >> index 83cd7e6..8a9c65c 100644
> >> --- a/arch/arm64/kernel/Makefile
> >> +++ b/arch/arm64/kernel/Makefile
> >> @@ -41,6 +41,7 @@ arm64-obj-$(CONFIG_EFI) += efi.o efi-entry.stub.o
> >> arm64-obj-$(CONFIG_PCI) += pci.o
> >> arm64-obj-$(CONFIG_ARMV8_DEPRECATED) += armv8_deprecated.o
> >> arm64-obj-$(CONFIG_ACPI) += acpi.o
> >> +arm64-obj-$(CONFIG_ARM64_ACPI_PARKING_PROTOCOL) += acpi_parking_protocol.o
> >> arm64-obj-$(CONFIG_PARAVIRT) += paravirt.o
> >>
> >> obj-y += $(arm64-obj-y) vdso/
> >> diff --git a/arch/arm64/kernel/acpi_parking_protocol.c b/arch/arm64/kernel/acpi_parking_protocol.c
> >> new file mode 100644
> >> index 0000000..531c3ad
> >> --- /dev/null
> >> +++ b/arch/arm64/kernel/acpi_parking_protocol.c
> >> @@ -0,0 +1,153 @@
> >> +/*
> >> + * ARM64 ACPI Parking Protocol implementation
> >> + *
> >> + * Authors: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> >> + * Mark Salter <msalter@xxxxxxxxxx>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +#include <linux/acpi.h>
> >> +#include <linux/types.h>
> >> +
> >> +#include <asm/cpu_ops.h>
> >> +
> >> +struct cpu_mailbox_entry {
> >> + phys_addr_t mailbox_addr;
> >> + u8 version;
> >> + u8 gic_cpu_id;
> >> +};
> >> +
> >> +static struct cpu_mailbox_entry cpu_mailbox_entries[NR_CPUS];
> >> +
> >> +void __init acpi_set_mailbox_entry(int cpu,
> >> + struct acpi_madt_generic_interrupt *p)
> >> +{
> >> + struct cpu_mailbox_entry *cpu_entry = &cpu_mailbox_entries[cpu];
> >> +
> >> + cpu_entry->mailbox_addr = p->parked_address;
> >> + cpu_entry->version = p->parking_version;
> >> + cpu_entry->gic_cpu_id = p->cpu_interface_number;
> >> +}
> >> +
> >> +bool __init acpi_parking_protocol_valid(int cpu)
> >> +{
> >> + struct cpu_mailbox_entry *cpu_entry = &cpu_mailbox_entries[cpu];
> >> +
> >> + return cpu_entry->mailbox_addr && cpu_entry->version;
> >> +}
> >> +
> >> +static int acpi_parking_protocol_cpu_init(unsigned int cpu)
> >> +{
> >> + pr_debug("%s: ACPI parked addr=%llx\n", __func__,
> >> + cpu_mailbox_entries[cpu].mailbox_addr);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int acpi_parking_protocol_cpu_prepare(unsigned int cpu)
> >> +{
> >> + return 0;
> >> +}
> >> +
> >> +struct parking_protocol_mailbox {
> >> + __le32 cpu_id;
> >> + __le32 reserved;
> >> + __le64 entry_point;
> >> +};
> >> +
> >> +static int acpi_parking_protocol_cpu_boot(unsigned int cpu)
> >> +{
> >> + struct cpu_mailbox_entry *cpu_entry = &cpu_mailbox_entries[cpu];
> >> + struct parking_protocol_mailbox __iomem *mailbox;
> >> + __le32 cpu_id;
> >> +
> >> + /*
> >> + * Map mailbox memory with attribute device nGnRE (ie ioremap -
> >> + * this deviates from the parking protocol specifications since
> >> + * the mailboxes are required to be mapped nGnRnE; the attribute
> >> + * discrepancy is harmless insofar as the protocol specification
> >> + * is concerned).
> >> + * If the mailbox is mistakenly allocated in the linear mapping
> >> + * by FW ioremap will fail since the mapping will be prevented
> >> + * by the kernel (it clashes with the linear mapping attributes
> >> + * specifications).
> >> + */
> >> + mailbox = ioremap(cpu_entry->mailbox_addr, sizeof(*mailbox));
> >> + if (!mailbox)
> >> + return -EIO;
> >> +
> >> + cpu_id = readl_relaxed(&mailbox->cpu_id);
> >> + /*
> >> + * Check if firmware has set-up the mailbox entry properly
> >> + * before kickstarting the respective cpu.
> >> + */
> >> + if (cpu_id != ~0U) {
> >> + iounmap(mailbox);
> >> + return -ENXIO;
> >> + }
> >> +
> >> + /*
> >> + * We write the entry point and cpu id as LE regardless of the
> >> + * native endianness of the kernel. Therefore, any boot-loaders
> >> + * that read this address need to convert this address to the
> >> + * Boot-Loader's endianness before jumping.
> >> + */
> >> + writeq_relaxed(__pa(secondary_entry), &mailbox->entry_point);
> >> + writel_relaxed(cpu_entry->gic_cpu_id, &mailbox->cpu_id);
> >> +
> >> + arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> >> +
> >> + iounmap(mailbox);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void acpi_parking_protocol_cpu_postboot(void)
> >> +{
> >> + int cpu = smp_processor_id();
> >> + struct cpu_mailbox_entry *cpu_entry = &cpu_mailbox_entries[cpu];
> >> + struct parking_protocol_mailbox __iomem *mailbox;
> >> + __le64 entry_point;
> >> +
> >> + /*
> >> + * Map mailbox memory with attribute device nGnRE (ie ioremap -
> >> + * this deviates from the parking protocol specifications since
> >> + * the mailboxes are required to be mapped nGnRnE; the attribute
> >> + * discrepancy is harmless insofar as the protocol specification
> >> + * is concerned).
> >> + * If the mailbox is mistakenly allocated in the linear mapping
> >> + * by FW ioremap will fail since the mapping will be prevented
> >> + * by the kernel (it clashes with the linear mapping attributes
> >> + * specifications).
> >> + */
> >> + mailbox = ioremap(cpu_entry->mailbox_addr, sizeof(*mailbox));
> >> + if (!mailbox)
> >> + return;
> >> +
> >> + entry_point = readl_relaxed(&mailbox->entry_point);
> >> + /*
> >> + * Check if firmware has cleared the entry_point as expected
> >> + * by the protocol specification.
> >> + */
> >> + WARN_ON(entry_point);
> >> +
> >> + iounmap(mailbox);
> >> +}
> >> +
> >> +const struct cpu_operations acpi_parking_protocol_ops = {
> >> + .name = "parking-protocol",
> >> + .cpu_init = acpi_parking_protocol_cpu_init,
> >> + .cpu_prepare = acpi_parking_protocol_cpu_prepare,
> >> + .cpu_boot = acpi_parking_protocol_cpu_boot,
> >> + .cpu_postboot = acpi_parking_protocol_cpu_postboot
> >> +};
> >> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> >> index b6bd7d4..c7cfb8f 100644
> >> --- a/arch/arm64/kernel/cpu_ops.c
> >> +++ b/arch/arm64/kernel/cpu_ops.c
> >> @@ -25,19 +25,30 @@
> >> #include <asm/smp_plat.h>
> >>
> >> extern const struct cpu_operations smp_spin_table_ops;
> >> +extern const struct cpu_operations acpi_parking_protocol_ops;
> >> extern const struct cpu_operations cpu_psci_ops;
> >>
> >> const struct cpu_operations *cpu_ops[NR_CPUS];
> >>
> >> -static const struct cpu_operations *supported_cpu_ops[] __initconst = {
> >> +static const struct cpu_operations *dt_supported_cpu_ops[] __initconst = {
> >> &smp_spin_table_ops,
> >> &cpu_psci_ops,
> >> NULL,
> >> };
> >>
> >> +static const struct cpu_operations *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 * __init cpu_get_ops(const char *name)
> >> {
> >> - const struct cpu_operations **ops = supported_cpu_ops;
> >> + const struct cpu_operations **ops;
> >> +
> >> + ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;
> >>
> >> while (*ops) {
> >> if (!strcmp(name, (*ops)->name))
> >> @@ -75,8 +86,16 @@ static const char *__init cpu_read_enable_method(int cpu)
> >> }
> >> } else {
> >> enable_method = acpi_get_enable_method(cpu);
> >> - if (!enable_method)
> >> - pr_err("Unsupported ACPI enable-method\n");
> >> + if (!enable_method) {
> >> + /*
> >> + * In ACPI systems the boot CPU does not require
> >> + * checking the enable method since for some
> >> + * boot protocol (ie parking protocol) it need not
> >> + * be initialized. Don't warn spuriously.
> >> + */
> >> + if (cpu != 0)
> >> + pr_err("Unsupported ACPI enable-method\n");
> >> + }
> >> }
> >>
> >> return enable_method;
> >> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> >> index b1adc51..6f608af 100644
> >> --- a/arch/arm64/kernel/smp.c
> >> +++ b/arch/arm64/kernel/smp.c
> >> @@ -70,6 +70,7 @@ enum ipi_msg_type {
> >> IPI_CPU_STOP,
> >> IPI_TIMER,
> >> IPI_IRQ_WORK,
> >> + IPI_WAKEUP
> >> };
> >>
> >> /*
> >> @@ -445,6 +446,17 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor)
> >> /* map the logical cpu id to cpu MPIDR */
> >> cpu_logical_map(cpu_count) = hwid;
> >>
> >> + /*
> >> + * Set-up the ACPI parking protocol cpu entries
> >> + * while initializing the cpu_logical_map to
> >> + * avoid parsing MADT entries multiple times for
> >> + * nothing (ie a valid cpu_logical_map entry should
> >> + * contain a valid parking protocol data set to
> >> + * initialize the cpu if the parking protocol is
> >> + * the only available enable method).
> >> + */
> >> + acpi_set_mailbox_entry(cpu_count, processor);
> >> +
> >> cpu_count++;
> >> }
> >>
> >> @@ -627,6 +639,7 @@ static const char *ipi_types[NR_IPI] __tracepoint_string = {
> >> S(IPI_CPU_STOP, "CPU stop interrupts"),
> >> S(IPI_TIMER, "Timer broadcast interrupts"),
> >> S(IPI_IRQ_WORK, "IRQ work interrupts"),
> >> + S(IPI_WAKEUP, "CPU wakeup interrupts"),
> >> };
> >>
> >> static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
> >> @@ -670,6 +683,13 @@ void arch_send_call_function_single_ipi(int cpu)
> >> smp_cross_call(cpumask_of(cpu), IPI_CALL_FUNC);
> >> }
> >>
> >> +#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
> >> +void arch_send_wakeup_ipi_mask(const struct cpumask *mask)
> >> +{
> >> + smp_cross_call(mask, IPI_WAKEUP);
> >> +}
> >> +#endif
> >> +
> >> #ifdef CONFIG_IRQ_WORK
> >> void arch_irq_work_raise(void)
> >> {
> >> @@ -746,6 +766,8 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
> >> irq_exit();
> >> break;
> >> #endif
> >> + case IPI_WAKEUP:
> >> + break;
> >>
> >> default:
> >> pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr);
> >> --
> >> 2.5.1
> >>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
