LinuxLists.cc - [RFC PATCH 0/7] Linux on Apple Silicon

[permalink] [raw]

Subject: [RFC PATCH 2/7] arm64: kernel: Add a WFI hook.

From: Stan Skowronek <[email protected]>

WFI drops register state on Apple Silicon for SMP systems.

This hook will be used for a hardware workaround in the
Apple CPU start driver.

Signed-off-by: Stan Skowronek <[email protected]>
Signed-off-by: Mohamed Mediouni <[email protected]>
---
arch/arm64/include/asm/cpu_ops.h | 2 ++
arch/arm64/kernel/cpu_ops.c | 6 ++++++
arch/arm64/kernel/process.c | 11 +++++++++--
3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index e95c4df83911..4be0fc5bcaf9 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -23,6 +23,7 @@
* @cpu_boot: Boots a cpu into the kernel.
* @cpu_postboot: Optionally, perform any post-boot cleanup or necessary
* synchronisation. Called from the cpu being booted.
+ * @cpu_wfi: Optionally, replace calls to WFI in default idle with this.
* @cpu_can_disable: Determines whether a CPU can be disabled based on
* mechanism-specific information.
* @cpu_disable: Prepares a cpu to die. May fail for some mechanism-specific
@@ -43,6 +44,7 @@ struct cpu_operations {
int (*cpu_prepare)(unsigned int);
int (*cpu_boot)(unsigned int);
void (*cpu_postboot)(void);
+ void (*cpu_wfi)(void);
#ifdef CONFIG_HOTPLUG_CPU
bool (*cpu_can_disable)(unsigned int cpu);
int (*cpu_disable)(unsigned int cpu);
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index e133011f64b5..6979fc4490b2 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -19,12 +19,18 @@ extern const struct cpu_operations smp_spin_table_ops;
extern const struct cpu_operations acpi_parking_protocol_ops;
#endif
extern const struct cpu_operations cpu_psci_ops;
+#ifdef CONFIG_ARCH_APPLE
+extern const struct cpu_operations cpu_apple_start_ops;
+#endif

static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;

static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
&smp_spin_table_ops,
&cpu_psci_ops,
+#ifdef CONFIG_ARCH_APPLE
+ &cpu_apple_start_ops,
+#endif
NULL,
};

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 34ec400288d0..611c639e20be 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -57,6 +57,7 @@
#include <asm/processor.h>
#include <asm/pointer_auth.h>
#include <asm/stacktrace.h>
+#include <asm/cpu_ops.h>

#if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
#include <linux/stackprotector.h>
@@ -74,8 +75,14 @@ void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);

static void noinstr __cpu_do_idle(void)
{
- dsb(sy);
- wfi();
+ const struct cpu_operations *ops = get_cpu_ops(task_cpu(current));
+
+ if (ops->cpu_wfi) {
+ ops->cpu_wfi();
+ } else {
+ dsb(sy);
+ wfi();
+ }
}

static void noinstr __cpu_do_idle_irqprio(void)
--
2.29.2

2021-01-20 14:00:08

[permalink] [raw]

Subject: [RFC PATCH 6/7] arm64: kernel: Apple CPU start driver

From: Stan Skowronek <[email protected]>

This driver is needed to spawn CPUs for SMP
on Apple Silicon platforms.

Signed-off-by: Stan Skowronek <[email protected]>
Signed-off-by: Mohamed Mediouni <[email protected]>
---
.../devicetree/bindings/arm/cpus.yaml | 1 +
arch/arm64/kernel/Makefile | 1 +
arch/arm64/kernel/apple_cpustart.c | 153 ++++++++++++++++++
3 files changed, 155 insertions(+)
create mode 100644 arch/arm64/kernel/apple_cpustart.c

diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
index 14cd727d3c4b..a6ff8cb3db1e 100644
--- a/Documentation/devicetree/bindings/arm/cpus.yaml
+++ b/Documentation/devicetree/bindings/arm/cpus.yaml
@@ -176,6 +176,7 @@ properties:
oneOf:
# On ARM v8 64-bit this property is required
- enum:
+ - apple
- psci
- spin-table
# On ARM 32-bit systems this property is optional
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 86364ab6f13f..497f43ca7f0f 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o
arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
obj-$(CONFIG_CRASH_CORE) += crash_core.o
+obj-$(CONFIG_ARCH_APPLE) += apple_cpustart.o
obj-$(CONFIG_ARM_SDE_INTERFACE) += sdei.o
obj-$(CONFIG_ARM64_PTR_AUTH) += pointer_auth.o
obj-$(CONFIG_ARM64_MTE) += mte.o
diff --git a/arch/arm64/kernel/apple_cpustart.c b/arch/arm64/kernel/apple_cpustart.c
new file mode 100644
index 000000000000..41d049eaaec7
--- /dev/null
+++ b/arch/arm64/kernel/apple_cpustart.c
@@ -0,0 +1,153 @@
+/* SPDX-License-Identifier: (GPL-2.0 or BSD-3-Clause) */
+/*
+ * Copyright (C) 2020 Corellium LLC
+ */
+
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/smp.h>
+#include <linux/delay.h>
+#include <linux/mm.h>
+
+#include <asm/cpu_ops.h>
+#include <asm/errno.h>
+#include <asm/smp_plat.h>
+#include <asm/io.h>
+
+#define MAGIC_UNLOCK 0xc5acce55
+
+struct cpu_apple_start_info {
+ void __iomem *pmgr_start;
+ u64 pmgr_start_size;
+ void __iomem *cputrc_rvbar;
+ void __iomem *dbg_unlock;
+};
+
+extern void apple_aic_cpu_prepare(unsigned int cpu);
+
+static int cpu_apple_start0_unlocked = 0;
+static DEFINE_PER_CPU(struct cpu_apple_start_info, cpu_apple_start_info);
+
+static int __init cpu_apple_start_init(unsigned int cpu)
+{
+ return 0;
+}
+
+static int cpu_apple_start_prepare(unsigned int cpu)
+{
+ struct device_node *node;
+ struct cpu_apple_start_info *info;
+
+ info = per_cpu_ptr(&cpu_apple_start_info, cpu);
+
+ if (info->pmgr_start && info->cputrc_rvbar && info->dbg_unlock)
+ return 0;
+
+ node = of_find_compatible_node(NULL, NULL, "apple,startcpu");
+ if (!node) {
+ pr_err("%s: missing startcpu node in device tree.\n", __func__);
+ return -EINVAL;
+ }
+
+ if (!info->pmgr_start) {
+ info->pmgr_start = of_iomap(node, cpu * 3);
+ if (!info->pmgr_start) {
+ pr_err("%s: failed to map start register for CPU %d.\n",
+ __func__, cpu);
+ return -EINVAL;
+ }
+ if (!of_get_address(node, cpu * 3, &info->pmgr_start_size,
+ NULL))
+ info->pmgr_start_size = 8;
+ }
+
+ if (!info->cputrc_rvbar) {
+ info->cputrc_rvbar = of_iomap(node, cpu * 3 + 1);
+ if (!info->cputrc_rvbar) {
+ pr_err("%s: failed to map reset address register for CPU %d.\n",
+ __func__, cpu);
+ return -EINVAL;
+ }
+ }
+
+ if (!info->dbg_unlock) {
+ info->dbg_unlock = of_iomap(node, cpu * 3 + 2);
+ if (!info->dbg_unlock) {
+ pr_err("%s: failed to map unlock register for CPU %d.\n",
+ __func__, cpu);
+ return -EINVAL;
+ }
+ }
+
+ if (cpu)
+ apple_aic_cpu_prepare(cpu);
+
+ return 0;
+}
+
+static int cpu_apple_start_boot(unsigned int cpu)
+{
+ struct cpu_apple_start_info *info;
+ unsigned long addr;
+
+ if (!cpu_apple_start0_unlocked) {
+ if (!cpu_apple_start_prepare(0)) {
+ info = per_cpu_ptr(&cpu_apple_start_info, 0);
+ writel(MAGIC_UNLOCK, info->dbg_unlock);
+ cpu_apple_start0_unlocked = 1;
+ } else
+ pr_err("%s: failed to unlock boot CPU\n", __func__);
+ }
+
+ info = per_cpu_ptr(&cpu_apple_start_info, cpu);
+
+ if (!info->pmgr_start || !info->cputrc_rvbar || !info->dbg_unlock)
+ return -EINVAL;
+
+ writeq(__pa_symbol(secondary_entry) | 1, info->cputrc_rvbar);
+ readq(info->cputrc_rvbar);
+ writeq(__pa_symbol(secondary_entry) | 1, info->cputrc_rvbar);
+ addr = readq(info->cputrc_rvbar) & 0xFFFFFFFFFul;
+ dsb(sy);
+
+ if (addr != (__pa_symbol(secondary_entry) | 1))
+ pr_err("%s: CPU%d reset address: 0x%lx, failed to set to 0x%lx.\n",
+ __func__, cpu, addr,
+ (unsigned long)(__pa_symbol(secondary_entry) | 1));
+
+ writel(MAGIC_UNLOCK, info->dbg_unlock);
+
+ writel(1 << cpu, info->pmgr_start);
+ if (info->pmgr_start_size >= 12) {
+ if (cpu < 4) {
+ writel(1 << cpu, info->pmgr_start + 4);
+ writel(0, info->pmgr_start + 8);
+ } else {
+ writel(0, info->pmgr_start + 4);
+ writel(1 << (cpu - 4), info->pmgr_start + 8);
+ }
+ } else
+ writel(1 << cpu, info->pmgr_start + 4);
+
+ dsb(sy);
+ sev();
+
+ return 0;
+}
+
+static void cpu_apple_wfi(void)
+{
+ /* can't do a proper WFI, because the CPU tends to lose state; will need
+ a proper wrapper sequence */
+ dsb(sy);
+ wfe();
+}
+
+const struct cpu_operations cpu_apple_start_ops = {
+ .name = "apple",
+ .cpu_init = cpu_apple_start_init,
+ .cpu_prepare = cpu_apple_start_prepare,
+ .cpu_boot = cpu_apple_start_boot,
+ .cpu_wfi = cpu_apple_wfi,
+};
--
2.29.2

2021-01-20 14:05:49

[permalink] [raw]

Subject: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious hardware quirk.

On Apple processors, writes using the nGnRE device memory type get dropped in flight,
getting to nowhere.

Signed-off-by: Stan Skowronek <[email protected]>
Signed-off-by: Mohamed Mediouni <[email protected]>
---
arch/arm64/mm/proc.S | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 1f7ee8c8b7b8..06436916f137 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -51,6 +51,25 @@
#define TCR_KASAN_HW_FLAGS 0
#endif

+#ifdef CONFIG_ARCH_APPLE
+
+/*
+ * Apple cores appear to black-hole writes done with nGnRE.
+ * We settled on a work-around that uses MAIR vs changing every single user of
+ * nGnRE across the arm64 code.
+ */
+
+#define MAIR_EL1_SET_APPLE \
+ (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) | \
+ MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRE) | \
+ MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) | \
+ MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) | \
+ MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) | \
+ MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) | \
+ MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
+
+#endif
+
/*
* Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
* changed during __cpu_setup to Normal Tagged if the system supports MTE.
@@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
* Memory region attributes
*/
mov_q x5, MAIR_EL1_SET
+#ifdef CONFIG_ARCH_APPLE
+ mrs x0, MIDR_EL1
+ lsr w0, w0, #24
+ mov_q x1, MAIR_EL1_SET_APPLE
+ cmp x0, #0x61 // 0x61 = Implementer: Apple
+ csel x5, x1, x5, eq
+#endif
#ifdef CONFIG_ARM64_MTE
mte_tcr .req x20

--
2.29.2

2021-01-20 16:53:04

by Alexander Graf

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On 20.01.21 14:27, Mohamed Mediouni wrote:
> Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious hardware quirk.
>
> On Apple processors, writes using the nGnRE device memory type get dropped in flight,
> getting to nowhere.
>
> Signed-off-by: Stan Skowronek <[email protected]>
> Signed-off-by: Mohamed Mediouni <[email protected]>
> ---
> arch/arm64/mm/proc.S | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 1f7ee8c8b7b8..06436916f137 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -51,6 +51,25 @@
> #define TCR_KASAN_HW_FLAGS 0
> #endif
>
> +#ifdef CONFIG_ARCH_APPLE

Is there any particular reason for this #ifdef?

Alex

> +
> +/*
> + * Apple cores appear to black-hole writes done with nGnRE.
> + * We settled on a work-around that uses MAIR vs changing every single user of
> + * nGnRE across the arm64 code.
> + */
> +
> +#define MAIR_EL1_SET_APPLE \
> + (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) | \
> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRE) | \
> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) | \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) | \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) | \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) | \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> +
> +#endif
> +
> /*
> * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> @@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
> * Memory region attributes
> */
> mov_q x5, MAIR_EL1_SET
> +#ifdef CONFIG_ARCH_APPLE
> + mrs x0, MIDR_EL1
> + lsr w0, w0, #24
> + mov_q x1, MAIR_EL1_SET_APPLE
> + cmp x0, #0x61 // 0x61 = Implementer: Apple
> + csel x5, x1, x5, eq
> +#endif
> #ifdef CONFIG_ARM64_MTE
> mte_tcr .req x20
>
> --
> 2.29.2
>

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

2021-01-20 16:56:24

by Alexander Graf

[permalink] [raw]

Subject: Re: [RFC PATCH 2/7] arm64: kernel: Add a WFI hook.

On 20.01.21 14:27, Mohamed Mediouni wrote:
> From: Stan Skowronek <[email protected]>
>
> WFI drops register state on Apple Silicon for SMP systems.

It probably drops the register because it loses power on WFI, right?

Have you tried to set the ARM64_REG_CYC_OVRD_ok2pwrdn_force_up bit in
ARM64_REG_CYC_OVRD yet? XNU has a call for that[1]. Maybe that's enough
to convince the core to preserve its register state for now.

For real power savings, we will probably want much more sophisticated
deep sleep capabilities later that would reach beyond just register
saving on WFI (different wakeup mechanisms, IRQ balancing, etc).

Alex

[1]
https://github.com/opensource-apple/xnu/blob/master/osfmk/arm64/machine_routines_asm.s#L797

>
> This hook will be used for a hardware workaround in the
> Apple CPU start driver.
>
> Signed-off-by: Stan Skowronek <[email protected]>
> Signed-off-by: Mohamed Mediouni <[email protected]>
> ---
> arch/arm64/include/asm/cpu_ops.h | 2 ++
> arch/arm64/kernel/cpu_ops.c | 6 ++++++
> arch/arm64/kernel/process.c | 11 +++++++++--
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
> index e95c4df83911..4be0fc5bcaf9 100644
> --- a/arch/arm64/include/asm/cpu_ops.h
> +++ b/arch/arm64/include/asm/cpu_ops.h
> @@ -23,6 +23,7 @@
> * @cpu_boot: Boots a cpu into the kernel.
> * @cpu_postboot: Optionally, perform any post-boot cleanup or necessary
> * synchronisation. Called from the cpu being booted.
> + * @cpu_wfi: Optionally, replace calls to WFI in default idle with this.
> * @cpu_can_disable: Determines whether a CPU can be disabled based on
> * mechanism-specific information.
> * @cpu_disable: Prepares a cpu to die. May fail for some mechanism-specific
> @@ -43,6 +44,7 @@ struct cpu_operations {
> int (*cpu_prepare)(unsigned int);
> int (*cpu_boot)(unsigned int);
> void (*cpu_postboot)(void);
> + void (*cpu_wfi)(void);
> #ifdef CONFIG_HOTPLUG_CPU
> bool (*cpu_can_disable)(unsigned int cpu);
> int (*cpu_disable)(unsigned int cpu);
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index e133011f64b5..6979fc4490b2 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -19,12 +19,18 @@ extern const struct cpu_operations smp_spin_table_ops;
> extern const struct cpu_operations acpi_parking_protocol_ops;
> #endif
> extern const struct cpu_operations cpu_psci_ops;
> +#ifdef CONFIG_ARCH_APPLE
> +extern const struct cpu_operations cpu_apple_start_ops;
> +#endif
>
> static const struct cpu_operations *cpu_ops[NR_CPUS] __ro_after_init;
>
> static const struct cpu_operations *const dt_supported_cpu_ops[] __initconst = {
> &smp_spin_table_ops,
> &cpu_psci_ops,
> +#ifdef CONFIG_ARCH_APPLE
> + &cpu_apple_start_ops,
> +#endif
> NULL,
> };
>
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 34ec400288d0..611c639e20be 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -57,6 +57,7 @@
> #include <asm/processor.h>
> #include <asm/pointer_auth.h>
> #include <asm/stacktrace.h>
> +#include <asm/cpu_ops.h>
>
> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> #include <linux/stackprotector.h>
> @@ -74,8 +75,14 @@ void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>
> static void noinstr __cpu_do_idle(void)
> {
> - dsb(sy);
> - wfi();
> + const struct cpu_operations *ops = get_cpu_ops(task_cpu(current));
> +
> + if (ops->cpu_wfi) {
> + ops->cpu_wfi();
> + } else {
> + dsb(sy);
> + wfi();
> + }
> }
>
> static void noinstr __cpu_do_idle_irqprio(void)
> --
> 2.29.2
>

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

2021-01-20 18:19:14

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

> On 20 Jan 2021, at 17:47, Alexander Graf <[email protected]> wrote:
>
> On 20.01.21 14:27, Mohamed Mediouni wrote:
>> Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious hardware quirk.
>> On Apple processors, writes using the nGnRE device memory type get dropped in flight,
>> getting to nowhere.
>> Signed-off-by: Stan Skowronek <[email protected]>
>> Signed-off-by: Mohamed Mediouni <[email protected]>
>> ---
>> arch/arm64/mm/proc.S | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 1f7ee8c8b7b8..06436916f137 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -51,6 +51,25 @@
>> #define TCR_KASAN_HW_FLAGS 0
>> #endif
>> +#ifdef CONFIG_ARCH_APPLE
>
> Is there any particular reason for this #ifdef?
>
>
> Alex
>
Not a particular reason, as we explicitly check for the implementer ID. However,
without CONFIG_ARCH_APPLE, other parts of the support for Apple CPUs
will not be available anyway.
>> +
>> +/*
>> + * Apple cores appear to black-hole writes done with nGnRE.
>> + * We settled on a work-around that uses MAIR vs changing every single user of
>> + * nGnRE across the arm64 code.
>> + */
>> +
>> +#define MAIR_EL1_SET_APPLE \
>> + (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRE) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
>> +
>> +#endif
>> +
>> /*
>> * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
>> * changed during __cpu_setup to Normal Tagged if the system supports MTE.
>> @@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
>> * Memory region attributes
>> */
>> mov_q x5, MAIR_EL1_SET
>> +#ifdef CONFIG_ARCH_APPLE
>> + mrs x0, MIDR_EL1
>> + lsr w0, w0, #24
>> + mov_q x1, MAIR_EL1_SET_APPLE
>> + cmp x0, #0x61 // 0x61 = Implementer: Apple
>> + csel x5, x1, x5, eq
>> +#endif
>> #ifdef CONFIG_ARM64_MTE
>> mte_tcr .req x20
>> --
>> 2.29.2
>
>
>
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879

2021-01-20 18:53:04

by Alexander Graf

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On 20.01.21 19:06, Mohamed Mediouni wrote:
>
>> On 20 Jan 2021, at 17:47, Alexander Graf <[email protected]> wrote:
>>
>> On 20.01.21 14:27, Mohamed Mediouni wrote:
>>> Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious hardware quirk.
>>> On Apple processors, writes using the nGnRE device memory type get dropped in flight,
>>> getting to nowhere.
>>> Signed-off-by: Stan Skowronek <[email protected]>
>>> Signed-off-by: Mohamed Mediouni <[email protected]>
>>> ---
>>> arch/arm64/mm/proc.S | 26 ++++++++++++++++++++++++++
>>> 1 file changed, 26 insertions(+)
>>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>>> index 1f7ee8c8b7b8..06436916f137 100644
>>> --- a/arch/arm64/mm/proc.S
>>> +++ b/arch/arm64/mm/proc.S
>>> @@ -51,6 +51,25 @@
>>> #define TCR_KASAN_HW_FLAGS 0
>>> #endif
>>> +#ifdef CONFIG_ARCH_APPLE
>>
>> Is there any particular reason for this #ifdef?
>>
>>
>> Alex
>>
> Not a particular reason, as we explicitly check for the implementer ID. However,
> without CONFIG_ARCH_APPLE, other parts of the support for Apple CPUs
> will not be available anyway.

The ifdef below for code looks ok to me, I'm explicitly wondering why
you're guarding the #define :)

Alex

>>> +
>>> +/*
>>> + * Apple cores appear to black-hole writes done with nGnRE.
>>> + * We settled on a work-around that uses MAIR vs changing every single user of
>>> + * nGnRE across the arm64 code.
>>> + */
>>> +
>>> +#define MAIR_EL1_SET_APPLE \
>>> + (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) | \
>>> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRE) | \
>>> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) | \
>>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) | \
>>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) | \
>>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) | \
>>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
>>> +
>>> +#endif
>>> +
>>> /*
>>> * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
>>> * changed during __cpu_setup to Normal Tagged if the system supports MTE.
>>> @@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
>>> * Memory region attributes
>>> */
>>> mov_q x5, MAIR_EL1_SET
>>> +#ifdef CONFIG_ARCH_APPLE
>>> + mrs x0, MIDR_EL1
>>> + lsr w0, w0, #24
>>> + mov_q x1, MAIR_EL1_SET_APPLE
>>> + cmp x0, #0x61 // 0x61 = Implementer: Apple
>>> + csel x5, x1, x5, eq
>>> +#endif
>>> #ifdef CONFIG_ARM64_MTE
>>> mte_tcr .req x20
>>> --
>>> 2.29.2

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

2021-01-20 20:27:26

[permalink] [raw]

Subject: [RFC PATCH 4/7] irqchip/apple-aic: Add support for Apple AIC

From: Stan Skowronek <[email protected]>

Apple SoCs use the Apple AIC interrupt controller.
The Arm architectural timers is wired over FIQ on that hardware.

Signed-off-by: Stan Skowronek <[email protected]>
Signed-off-by: Mohamed Mediouni <[email protected]>
---
.../interrupt-controller/apple,aic.yaml | 49 ++++
MAINTAINERS | 6 +
drivers/irqchip/Kconfig | 6 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-apple-aic.c | 211 ++++++++++++++++++
5 files changed, 273 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
create mode 100644 drivers/irqchip/irq-apple-aic.c

diff --git a/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
new file mode 100644
index 000000000000..e615eaaca869
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/apple,aic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Advanced Interrupt Controller (AIC)
+
+description:
+ Interrupt controller present on Apple processors. AIC
+ is used by Apple on their AArch64 SoCs since the Apple A7.
+
+maintainers:
+ - Stan Skowronek <[email protected]>
+
+properties:
+ compatible:
+ items:
+ - const: apple,aic
+
+ reg:
+ maxItems: 1
+
+ '#interrupt-cells':
+ const: 3
+
+ interrupt-controller: true
+
+ fast-ipi:
+ description:
+ Fast IPI support.
+
+required:
+ - compatible
+ - '#interrupt-cells'
+ - interrupt-controller
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ aic: interrupt-controller@23b100000 {
+ compatible = "apple,aic";
+ #interrupt-cells = <3>;
+ interrupt-controller;
+ reg = <0x2 0x3b100000 0x0 0x8000>;
+ fast-ipi;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 00836f6452f0..e609ede99dd4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1218,6 +1218,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparmor
F: Documentation/admin-guide/LSM/apparmor.rst
F: security/apparmor/

+APPLE ADVANCED INTERRUPT CONTROLLER DRIVER
+M: Stan Skowronek <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/irqchip/irq-apple-aic.c
+
APPLE BCM5974 MULTITOUCH DRIVER
M: Henrik Rydberg <[email protected]>
L: [email protected]
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 94920a51c628..3aa9e711324b 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -56,6 +56,12 @@ config ARM_GIC_V3_ITS_FSL_MC
depends on FSL_MC_BUS
default ARM_GIC_V3_ITS

+config APPLE_AIC
+ bool
+ select IRQ_DOMAIN_HIERARCHY
+ select GENERIC_IRQ_MULTI_HANDLER
+ select GENERIC_IRQ_EFFECTIVE_AFF_MASK
+
config ARM_NVIC
bool
select IRQ_DOMAIN_HIERARCHY
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 0ac93bfaec61..2f5a9a0cf40f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_ARM_GIC_V3) += irq-gic-v3.o irq-gic-v3-mbi.o irq-gic-common.o
obj-$(CONFIG_ARM_GIC_V3_ITS) += irq-gic-v3-its.o irq-gic-v3-its-platform-msi.o irq-gic-v4.o
obj-$(CONFIG_ARM_GIC_V3_ITS_PCI) += irq-gic-v3-its-pci-msi.o
obj-$(CONFIG_ARM_GIC_V3_ITS_FSL_MC) += irq-gic-v3-its-fsl-mc-msi.o
+obj-$(CONFIG_APPLE_AIC) += irq-apple-aic.o
obj-$(CONFIG_PARTITION_PERCPU) += irq-partition-percpu.o
obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
new file mode 100644
index 000000000000..c601bc4b501a
--- /dev/null
+++ b/drivers/irqchip/irq-apple-aic.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple chip interrupt controller
+ *
+ * Copyright (C) 2020 Corellium LLC
+ * Copyright (C) 1992, 1998 Linus Torvalds, Ingo Molnar
+ *
+ */
+
+#include <linux/interrupt.h>
+#include <linux/err.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include <asm/exception.h>
+#include <asm/irq.h>
+
+#define REG_ID_REVISION 0x0000
+#define REG_ID_CONFIG 0x0004
+#define REG_GLOBAL_CFG 0x0010
+#define REG_TIME_LO 0x0020
+#define REG_TIME_HI 0x0028
+#define REG_ID_CPUID 0x2000
+#define REG_IRQ_ACK 0x2004
+#define REG_IRQ_ACK_TYPE_MASK (15 << 16)
+#define REG_IRQ_ACK_TYPE_NONE (0 << 16)
+#define REG_IRQ_ACK_TYPE_IRQ (1 << 16)
+#define REG_IRQ_ACK_TYPE_IPI (4 << 16)
+#define REG_IRQ_ACK_IPI_OTHER 0x40001
+#define REG_IRQ_ACK_IPI_SELF 0x40002
+#define REG_IRQ_ACK_NUM_MASK (4095)
+#define REG_IPI_SET 0x2008
+#define REG_IPI_FLAG_SELF (1 << 31)
+#define REG_IPI_FLAG_OTHER (1 << 0)
+#define REG_IPI_CLEAR 0x200C
+#define REG_IPI_DISABLE 0x2024
+#define REG_IPI_ENABLE 0x2028
+#define REG_IPI_DEFER_SET 0x202C
+#define REG_IPI_DEFER_CLEAR 0x2030
+#define REG_TSTAMP_CTRL 0x2040
+#define REG_TSTAMP_LO 0x2048
+#define REG_TSTAMP_HI 0x204C
+#define REG_IRQ_AFFINITY(i) (0x3000 + ((i) << 2))
+#define REG_IRQ_DISABLE(i) (0x4100 + (((i) >> 5) << 2))
+#define REG_IRQ_xABLE_MASK(i) (1 << ((i)&31))
+#define REG_IRQ_ENABLE(i) (0x4180 + (((i) >> 5) << 2))
+#define REG_CPU_REGION 0x5000
+#define REG_CPU_LOCAL 0x2000
+#define REG_CPU_SHIFT 7
+#define REG_PERCPU(r, c) \
+ ((r) + REG_CPU_REGION - REG_CPU_LOCAL + ((c) << REG_CPU_SHIFT))
+
+static struct aic_chip_data {
+ void __iomem *base;
+ struct irq_domain *domain;
+ unsigned int num_irqs;
+} aic;
+
+static void apple_aic_irq_mask(struct irq_data *d)
+{
+ writel(REG_IRQ_xABLE_MASK(d->hwirq),
+ aic.base + REG_IRQ_DISABLE(d->hwirq));
+}
+
+static void apple_aic_irq_unmask(struct irq_data *d)
+{
+ writel(REG_IRQ_xABLE_MASK(d->hwirq),
+ aic.base + REG_IRQ_ENABLE(d->hwirq));
+}
+
+static struct irq_chip apple_aic_irq_chip = {
+ .name = "apple_aic",
+ .irq_mask = apple_aic_irq_mask,
+ .irq_mask_ack = apple_aic_irq_mask,
+ .irq_unmask = apple_aic_irq_unmask,
+};
+
+static void apple_aic_fiq_mask(struct irq_data *d)
+{
+}
+
+static void apple_aic_fiq_unmask(struct irq_data *d)
+{
+}
+
+static struct irq_chip apple_aic_irq_chip_fiq = {
+ .name = "apple_aic_fiq",
+ .irq_mask = apple_aic_fiq_mask,
+ .irq_unmask = apple_aic_fiq_unmask,
+};
+
+static int apple_aic_irq_domain_xlate(struct irq_domain *d,
+ struct device_node *ctrlr,
+ const u32 *intspec, unsigned int intsize,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ if (intspec[0]) { /* FIQ */
+ if (intspec[1] >= 2)
+ return -EINVAL;
+ if (out_hwirq)
+ *out_hwirq = aic.num_irqs + intspec[1];
+ } else {
+ if (intspec[1] >= aic.num_irqs)
+ return -EINVAL;
+ if (out_hwirq)
+ *out_hwirq = intspec[1];
+ }
+
+ if (out_type)
+ *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+}
+
+static int apple_aic_irq_domain_map(struct irq_domain *d, unsigned int virq,
+ irq_hw_number_t hw)
+{
+ if (hw >= aic.num_irqs) {
+ irq_set_percpu_devid(virq);
+ irq_domain_set_info(d, virq, hw, &apple_aic_irq_chip_fiq,
+ d->host_data, handle_percpu_devid_irq, NULL,
+ NULL);
+ irq_set_status_flags(virq, IRQ_NOAUTOEN);
+ } else {
+ irq_domain_set_info(d, virq, hw, &apple_aic_irq_chip,
+ d->host_data, handle_level_irq, NULL, NULL);
+ irq_set_probe(virq);
+ irqd_set_single_target(
+ irq_desc_get_irq_data(irq_to_desc(virq)));
+ }
+ return 0;
+}
+
+static const struct irq_domain_ops apple_aic_irq_domain_ops = {
+ .xlate = apple_aic_irq_domain_xlate,
+ .map = apple_aic_irq_domain_map,
+};
+
+static void __exception_irq_entry apple_aic_handle_irq(struct pt_regs *regs)
+{
+ uint32_t ack;
+ unsigned done = 0;
+
+ while (1) {
+ ack = readl(aic.base + REG_IRQ_ACK);
+ switch (ack & REG_IRQ_ACK_TYPE_MASK) {
+ case REG_IRQ_ACK_TYPE_NONE:
+ done = 1;
+ break;
+ case REG_IRQ_ACK_TYPE_IRQ:
+ handle_domain_irq(aic.domain,
+ ack & REG_IRQ_ACK_NUM_MASK, regs);
+ break;
+ }
+ if (done)
+ break;
+ }
+}
+
+static void __exception_irq_entry apple_aic_handle_fiq(struct pt_regs *regs)
+{
+ handle_domain_irq(aic.domain, aic.num_irqs, regs);
+}
+
+void apple_aic_cpu_prepare(unsigned int cpu)
+{
+ unsigned i;
+
+ for (i = 0; i < aic.num_irqs; i++)
+ writel(readl(aic.base + REG_IRQ_AFFINITY(i)) | (1u << cpu),
+ aic.base + REG_IRQ_AFFINITY(i));
+}
+
+static int __init apple_aic_init(struct device_node *node,
+ struct device_node *interrupt_parent)
+{
+ unsigned i;
+
+ if (!node)
+ return -ENODEV;
+
+ aic.base = of_iomap(node, 0);
+ if (WARN(!aic.base, "unable to map aic registers\n"))
+ return -EINVAL;
+
+ aic.num_irqs = readl(aic.base + REG_ID_CONFIG) & 0xFFFF;
+ pr_info("Apple AIC: %d IRQs + 1 FIQ + 1 dummy\n", aic.num_irqs);
+
+ for (i = 0; i < aic.num_irqs; i++)
+ writel(1, aic.base + REG_IRQ_AFFINITY(i));
+ for (i = 0; i < aic.num_irqs; i += 32)
+ writel(-1u, aic.base + REG_IRQ_DISABLE(i));
+ writel((readl(aic.base + REG_GLOBAL_CFG) & ~0xF00000) | 0x700000,
+ aic.base + REG_GLOBAL_CFG);
+
+ set_handle_irq(apple_aic_handle_irq);
+ set_handle_fiq(apple_aic_handle_fiq);
+
+ apple_aic_cpu_prepare(0);
+
+ aic.domain = irq_domain_add_linear(node, aic.num_irqs + 2,
+ &apple_aic_irq_domain_ops,
+ &apple_aic_irq_chip);
+ irq_set_default_host(aic.domain);
+ return 0;
+}
+
+IRQCHIP_DECLARE(apple_aic_irq_chip, "apple,aic", apple_aic_init);
--
2.29.2

2021-01-20 20:28:40

[permalink] [raw]

Subject: [RFC PATCH 5/7] arm64/Kconfig: Add Apple Silicon SoC platform

From: Stan Skowronek <[email protected]>

Signed-off-by: Stan Skowronek <[email protected]>
Signed-off-by: Mohamed Mediouni <[email protected]>
---
arch/arm64/Kconfig.platforms | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 6eecdef538bd..cc52519d4f67 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -328,4 +328,11 @@ config ARCH_ZYNQMP
help
This enables support for Xilinx ZynqMP Family

+config ARCH_APPLE
+ bool "Apple Silicon SoC Family"
+ select APPLE_AIC
+ help
+ This enables support for Apple processors present
+ on Mac computers.
+
endmenu
--
2.29.2

2021-01-21 01:26:04

by Stan Skowronek

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] irqchip/apple-aic: Add support for Apple AIC

Acked-by: Stan Skowronek <[email protected]>

On 1/20/21 8:27 AM, Mohamed Mediouni wrote:
> From: Stan Skowronek <[email protected]>
>
> Apple SoCs use the Apple AIC interrupt controller.
> The Arm architectural timers is wired over FIQ on that hardware.
>
> Signed-off-by: Stan Skowronek <[email protected]>
> Signed-off-by: Mohamed Mediouni <[email protected]>
> ---
> .../interrupt-controller/apple,aic.yaml | 49 ++++
> MAINTAINERS | 6 +
> drivers/irqchip/Kconfig | 6 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-apple-aic.c | 211 ++++++++++++++++++
> 5 files changed, 273 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
> create mode 100644 drivers/irqchip/irq-apple-aic.c

2021-01-21 10:08:26

by Linus Walleij

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] irqchip/apple-aic: Add support for Apple AIC

Hi Mohamed,

thanks for your patch!

On Wed, Jan 20, 2021 at 2:31 PM Mohamed Mediouni
<[email protected]> wrote:

> +properties:
> + compatible:
> + items:
> + - const: apple,aic

However weird it may seem, Apple is not in the file
Documentation/devicetree/bindings/vendor-prefixes.yaml

(I think it's weird because I remember clearly that they have been
using device tree for PPC since ages.)

Could you add this 2-liner to that file and send it directly to
DT binding maintainers as a single patch as a preparation?

Yours,
Linus Walleij

2021-01-21 10:54:12

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] irqchip/apple-aic: Add support for Apple AIC

On Thu, Jan 21, 2021 at 10:48 AM Linus Walleij <[email protected]> wrote:
>
> Hi Mohamed,
>
> thanks for your patch!
>
> On Wed, Jan 20, 2021 at 2:31 PM Mohamed Mediouni
> <[email protected]> wrote:
>
> > +properties:
> > + compatible:
> > + items:
> > + - const: apple,aic
>
> However weird it may seem, Apple is not in the file
> Documentation/devicetree/bindings/vendor-prefixes.yaml
>
> (I think it's weird because I remember clearly that they have been
> using device tree for PPC since ages.)
>
> Could you add this 2-liner to that file and send it directly to
> DT binding maintainers as a single patch as a preparation?

Choosing the vendor prefix here is going to be a little tricky
and non-obvious.

Background:

Traditionally, it should have been the stock ticker symbol of the
company (clearly only publicly traded companies would be able
to produce a Unix capable computer, right?), but there were
already inconsistent: IBM used "ibm" (in small letters), Sun
used "SUNW" (in capitals) but in 2007 changed the stock ticker
symbol to "JAVA", obviously without changing the firmware bindings.

Apple traditionally used "AAPL" (also in caps) in the device tree,
and there is one remnant of that in the M1 device tree, in the form
of the "AAPL,phandle" property in each node, which corresponds
to our "linux,phandle". (phandles were introduced as properties in
both of the flattened DT formats, .dtb and apple's own format).
There are also "AAPL,unit-string properties and some device_type
strings (AAPL,display-crossbar, AAPL,atc-dpxbar, ...) in the M1 DT,
and the CPU nodes (and only those) use "apple" in small letters
as in "apple,icestorm","ARM,v8". The other nodes tend to not have
a vendor prefix, but a lot use a subsystem name as the prefix, such
as compatible="gpio,t8101" or compatible="tempsensor,t8020".

Since Apple are already using both the "AAPL" and the "apple"
prefix themselves, I have a bad feeling about reusing either of
them for defining the devicetree.org bindings that we add to
linux/Documentation/devicetree/bindings. The question is: if
not "apple", what else should we use here?

Arnd

2021-01-21 10:58:02

[permalink] [raw]

Subject: Re: [RFC PATCH 2/7] arm64: kernel: Add a WFI hook.

On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
<[email protected]> wrote:
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c

> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> #include <linux/stackprotector.h>
> @@ -74,8 +75,14 @@ void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>
> static void noinstr __cpu_do_idle(void)
> {
> - dsb(sy);
> - wfi();
> + const struct cpu_operations *ops = get_cpu_ops(task_cpu(current));
> +
> + if (ops->cpu_wfi) {
> + ops->cpu_wfi();
> + } else {
> + dsb(sy);
> + wfi();
> + }
> }

I think the correct place to put this would be a platform specific driver
in drivers/cpuidle/ instead of an added low-level callback in the
default idle function and a custom cpu_operations structure.

Arnd

2021-01-21 11:06:56

[permalink] [raw]

Subject: Re: [RFC PATCH 2/7] arm64: kernel: Add a WFI hook.

> On 21 Jan 2021, at 11:52, Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
> <[email protected]> wrote:
>> --- a/arch/arm64/kernel/cpu_ops.c
>> +++ b/arch/arm64/kernel/cpu_ops.c
>
>> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
>> #include <linux/stackprotector.h>
>> @@ -74,8 +75,14 @@ void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
>>
>> static void noinstr __cpu_do_idle(void)
>> {
>> - dsb(sy);
>> - wfi();
>> + const struct cpu_operations *ops = get_cpu_ops(task_cpu(current));
>> +
>> + if (ops->cpu_wfi) {
>> + ops->cpu_wfi();
>> + } else {
>> + dsb(sy);
>> + wfi();
>> + }
>> }
>
> I think the correct place to put this would be a platform specific driver
> in drivers/cpuidle/ instead of an added low-level callback in the
> default idle function and a custom cpu_operations structure.
Can we make sure that wfi never gets called even on early
boot when using a cpuidle driver?

Thank you,
>
> Arnd

2021-01-21 11:21:01

[permalink] [raw]

Subject: Re: [RFC PATCH 6/7] arm64: kernel: Apple CPU start driver

On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
<[email protected]> wrote:
> diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
> index 14cd727d3c4b..a6ff8cb3db1e 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.yaml
> +++ b/Documentation/devicetree/bindings/arm/cpus.yaml
> @@ -176,6 +176,7 @@ properties:
> oneOf:
> # On ARM v8 64-bit this property is required
> - enum:
> + - apple
> - psci
> - spin-table
> # On ARM 32-bit systems this property is optional

This uses a very generic identifier for doing something that may
be very specific to a particular SoC generation. It's going to be hard
to decide what the right abstraction will be for long-term maintenance,
so I'd recommend starting with a boot loader that implements
spin-table for secondary startup, and getting back to this after more
of the basic stuff works.

> +static int cpu_apple_start_prepare(unsigned int cpu)
> +{
> + struct device_node *node;
> + struct cpu_apple_start_info *info;
> +
> + info = per_cpu_ptr(&cpu_apple_start_info, cpu);
> +
> + if (info->pmgr_start && info->cputrc_rvbar && info->dbg_unlock)
> + return 0;
> +
> + node = of_find_compatible_node(NULL, NULL, "apple,startcpu");
> + if (!node) {
> + pr_err("%s: missing startcpu node in device tree.\n", __func__);
> + return -EINVAL;
> + }

Where is the binding documentation for this? The way you do a separate
of_iomap() for each CPU suggests that this is not a great binding to
start with. Are these perhaps just individual registers within a larger IP
block in the end?

Arnd

2021-01-21 11:33:01

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On Wed, Jan 20, 2021 at 02:27:13PM +0100, Mohamed Mediouni wrote:
> Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious hardware quirk.
>
> On Apple processors, writes using the nGnRE device memory type get dropped in flight,
> getting to nowhere.
>
> Signed-off-by: Stan Skowronek <[email protected]>
> Signed-off-by: Mohamed Mediouni <[email protected]>
> ---
> arch/arm64/mm/proc.S | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 1f7ee8c8b7b8..06436916f137 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -51,6 +51,25 @@
> #define TCR_KASAN_HW_FLAGS 0
> #endif
>
> +#ifdef CONFIG_ARCH_APPLE
> +
> +/*
> + * Apple cores appear to black-hole writes done with nGnRE.
> + * We settled on a work-around that uses MAIR vs changing every single user of
> + * nGnRE across the arm64 code.
> + */
> +
> +#define MAIR_EL1_SET_APPLE \
> + (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) | \
> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRE) | \
> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) | \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) | \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) | \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) | \
> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> +
> +#endif
> +
> /*
> * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> @@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
> * Memory region attributes
> */
> mov_q x5, MAIR_EL1_SET
> +#ifdef CONFIG_ARCH_APPLE
> + mrs x0, MIDR_EL1
> + lsr w0, w0, #24
> + mov_q x1, MAIR_EL1_SET_APPLE
> + cmp x0, #0x61 // 0x61 = Implementer: Apple
> + csel x5, x1, x5, eq

Why does this need to be done so early? It would be a lot cleaner if we
could detect this in a similar fashion to other errata and update the MAIR
appropriately. If that's not possible because of early IO mappings (which
ones?), then we could instead initialise to nGnRnE unconditionally, but
relax it to nGnRE if we detect that we _don't_ have the erratum.

Will

2021-01-21 11:40:16

[permalink] [raw]

Subject: Re: [RFC PATCH 2/7] arm64: kernel: Add a WFI hook.

On Thu, Jan 21, 2021 at 12:01 PM Mohamed Mediouni
<[email protected]> wrote:
> > On 21 Jan 2021, at 11:52, Arnd Bergmann <[email protected]> wrote:
> >
> > On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
> > <[email protected]> wrote:
> >> --- a/arch/arm64/kernel/cpu_ops.c
> >> +++ b/arch/arm64/kernel/cpu_ops.c
> >
> >> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_STACKPROTECTOR_PER_TASK)
> >> #include <linux/stackprotector.h>
> >> @@ -74,8 +75,14 @@ void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
> >>
> >> static void noinstr __cpu_do_idle(void)
> >> {
> >> - dsb(sy);
> >> - wfi();
> >> + const struct cpu_operations *ops = get_cpu_ops(task_cpu(current));
> >> +
> >> + if (ops->cpu_wfi) {
> >> + ops->cpu_wfi();
> >> + } else {
> >> + dsb(sy);
> >> + wfi();
> >> + }
> >> }
> >
> > I think the correct place to put this would be a platform specific driver
> > in drivers/cpuidle/ instead of an added low-level callback in the
> > default idle function and a custom cpu_operations structure.
>
> Can we make sure that wfi never gets called even on early
> boot when using a cpuidle driver?

Good question, I don't know what all the possible call sites are
for this, but if there is nothing else works (such as what Alex suggested),
it may be possible to just patch out the wfi instruction here and
do a busy loop until the cpuidle driver has come up.

The main issue here is the existence of the custom cpu_operations
in the first place: I don't think we want or need the custom start_secondary
at the moment (as commented in the other patch), but then there is
no obvious place to put the custom wfi.

Note that there are a few other uses of the wfi instruction besides the
one in __cpu_do_idle(), so whatever you do here may also apply to the
others.

arch/arm64/include/asm/smp.h: wfi();
arch/arm64/kernel/head.S: wfi
arch/arm64/kernel/head.S: wfi
arch/arm64/kernel/head.S: wfi
arch/arm64/kernel/process.c: wfi();
arch/arm64/kvm/hyp/nvhe/hyp-init.S: wfi

Arnd

2021-01-21 11:43:16

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On Thu, Jan 21, 2021 at 12:32 PM Will Deacon <[email protected]> wrote:
> On Wed, Jan 20, 2021 at 02:27:13PM +0100, Mohamed Mediouni wrote:
> > Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious hardware quirk.
> > /*
> > * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
> > * changed during __cpu_setup to Normal Tagged if the system supports MTE.
> > @@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
> > * Memory region attributes
> > */
> > mov_q x5, MAIR_EL1_SET
> > +#ifdef CONFIG_ARCH_APPLE
> > + mrs x0, MIDR_EL1
> > + lsr w0, w0, #24
> > + mov_q x1, MAIR_EL1_SET_APPLE
> > + cmp x0, #0x61 // 0x61 = Implementer: Apple
> > + csel x5, x1, x5, eq
>
> Why does this need to be done so early? It would be a lot cleaner if we
> could detect this in a similar fashion to other errata and update the MAIR
> appropriately. If that's not possible because of early IO mappings (which
> ones?), then we could instead initialise to nGnRnE unconditionally, but
> relax it to nGnRE if we detect that we _don't_ have the erratum.

There is (at least) the custom SMP startup code that uses device
mappings. If that's the only thing that needs the modified MAIR
to be used early, I'd consider that one more reason against doing the
custom cpu_operations for now.

Arnd

2021-01-21 11:48:19

by Marc Zyngier

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On 2021-01-21 11:27, Will Deacon wrote:
> On Wed, Jan 20, 2021 at 02:27:13PM +0100, Mohamed Mediouni wrote:
>> Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious
>> hardware quirk.
>>
>> On Apple processors, writes using the nGnRE device memory type get
>> dropped in flight,
>> getting to nowhere.
>>
>> Signed-off-by: Stan Skowronek <[email protected]>
>> Signed-off-by: Mohamed Mediouni <[email protected]>
>> ---
>> arch/arm64/mm/proc.S | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>> index 1f7ee8c8b7b8..06436916f137 100644
>> --- a/arch/arm64/mm/proc.S
>> +++ b/arch/arm64/mm/proc.S
>> @@ -51,6 +51,25 @@
>> #define TCR_KASAN_HW_FLAGS 0
>> #endif
>>
>> +#ifdef CONFIG_ARCH_APPLE
>> +
>> +/*
>> + * Apple cores appear to black-hole writes done with nGnRE.
>> + * We settled on a work-around that uses MAIR vs changing every
>> single user of
>> + * nGnRE across the arm64 code.
>> + */
>> +
>> +#define MAIR_EL1_SET_APPLE \
>> + (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRE) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) | \
>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
>> +
>> +#endif
>> +
>> /*
>> * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal
>> memory and
>> * changed during __cpu_setup to Normal Tagged if the system supports
>> MTE.
>> @@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
>> * Memory region attributes
>> */
>> mov_q x5, MAIR_EL1_SET
>> +#ifdef CONFIG_ARCH_APPLE
>> + mrs x0, MIDR_EL1
>> + lsr w0, w0, #24
>> + mov_q x1, MAIR_EL1_SET_APPLE
>> + cmp x0, #0x61 // 0x61 = Implementer: Apple
>> + csel x5, x1, x5, eq
>
> Why does this need to be done so early? It would be a lot cleaner if we
> could detect this in a similar fashion to other errata and update the
> MAIR
> appropriately. If that's not possible because of early IO mappings
> (which
> ones?), then we could instead initialise to nGnRnE unconditionally, but
> relax it to nGnRE if we detect that we _don't_ have the erratum.

Would that imply another round-trip into the idmap, much like we do
when we switch to non-global mappings? Or do you expect that we can
change
the MAIR with live mappings?

M.
--
Jazz is not dead. It just smells funny...

2021-01-21 12:49:33

by Hector Martin

[permalink] [raw]

Subject: Re: [RFC PATCH 2/7] arm64: kernel: Add a WFI hook.

I already mentioned this privately, but for the benefit of the ML:

On 21/01/2021 09.48, Mohamed Mediouni wrote:
> If we explicitly use the hardware override registers for this, then
> we'll be unable to use the deep sleep support provided by wfi on
> Apple SoCs later on without touching Apple-specific MSRs.
>
> Our current policy is to avoid having those modified inside the kernel
> at all costs, considering it to be a job for the bootloader instead.

I don't think there is any particular reason to do this; the bootloader
should be responsible for setting up all the chicken bits that make the
CPU work properly, including doing so for all SMP cores before entering
the kernel, but that's not the same thing as power management bits.

It seems entirely reasonable to me to configure WFI as clockgate only
(so it keeps state), not touch this part of kernel code at all, and then
in the cpuidle driver (which can come later) just do:

- switch to powerdown mode
- save state
- wfi
- restore state
- switch to clockgate mode

--
Hector Martin "marcan" ([email protected])
Public Key: https://mrcn.st/pub

2021-01-21 12:53:31

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On Thu, Jan 21, 2021 at 11:44:23AM +0000, Marc Zyngier wrote:
> On 2021-01-21 11:27, Will Deacon wrote:
> > On Wed, Jan 20, 2021 at 02:27:13PM +0100, Mohamed Mediouni wrote:
> > > Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious
> > > hardware quirk.
> > >
> > > On Apple processors, writes using the nGnRE device memory type get
> > > dropped in flight,
> > > getting to nowhere.
> > >
> > > Signed-off-by: Stan Skowronek <[email protected]>
> > > Signed-off-by: Mohamed Mediouni <[email protected]>
> > > ---
> > > arch/arm64/mm/proc.S | 26 ++++++++++++++++++++++++++
> > > 1 file changed, 26 insertions(+)
> > >
> > > diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> > > index 1f7ee8c8b7b8..06436916f137 100644
> > > --- a/arch/arm64/mm/proc.S
> > > +++ b/arch/arm64/mm/proc.S
> > > @@ -51,6 +51,25 @@
> > > #define TCR_KASAN_HW_FLAGS 0
> > > #endif
> > >
> > > +#ifdef CONFIG_ARCH_APPLE
> > > +
> > > +/*
> > > + * Apple cores appear to black-hole writes done with nGnRE.
> > > + * We settled on a work-around that uses MAIR vs changing every
> > > single user of
> > > + * nGnRE across the arm64 code.
> > > + */
> > > +
> > > +#define MAIR_EL1_SET_APPLE \
> > > + (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) | \
> > > + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRE) | \
> > > + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) | \
> > > + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) | \
> > > + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) | \
> > > + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) | \
> > > + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
> > > +
> > > +#endif
> > > +
> > > /*
> > > * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal
> > > memory and
> > > * changed during __cpu_setup to Normal Tagged if the system
> > > supports MTE.
> > > @@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
> > > * Memory region attributes
> > > */
> > > mov_q x5, MAIR_EL1_SET
> > > +#ifdef CONFIG_ARCH_APPLE
> > > + mrs x0, MIDR_EL1
> > > + lsr w0, w0, #24
> > > + mov_q x1, MAIR_EL1_SET_APPLE
> > > + cmp x0, #0x61 // 0x61 = Implementer: Apple
> > > + csel x5, x1, x5, eq
> >
> > Why does this need to be done so early? It would be a lot cleaner if we
> > could detect this in a similar fashion to other errata and update the
> > MAIR
> > appropriately. If that's not possible because of early IO mappings
> > (which
> > ones?), then we could instead initialise to nGnRnE unconditionally, but
> > relax it to nGnRE if we detect that we _don't_ have the erratum.
>
> Would that imply another round-trip into the idmap, much like we do
> when we switch to non-global mappings? Or do you expect that we can change
> the MAIR with live mappings?

I think we should be able to change it live and then invalidate the TLB. At
least, my reading of the BBM requirements suggests that it isn't required
for changing between different types of device memory. I can seek
clarification from Arm if necessary.

Will

2021-01-21 15:16:58

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

> On 21 Jan 2021, at 13:47, Will Deacon <[email protected]> wrote:
>
> On Thu, Jan 21, 2021 at 11:44:23AM +0000, Marc Zyngier wrote:
>> On 2021-01-21 11:27, Will Deacon wrote:
>>> On Wed, Jan 20, 2021 at 02:27:13PM +0100, Mohamed Mediouni wrote:
>>>> Use nGnRnE instead of nGnRE on Apple SoCs to workaround a serious
>>>> hardware quirk.
>>>>
>>>> On Apple processors, writes using the nGnRE device memory type get
>>>> dropped in flight,
>>>> getting to nowhere.
>>>>
>>>> Signed-off-by: Stan Skowronek <[email protected]>
>>>> Signed-off-by: Mohamed Mediouni <[email protected]>
>>>> ---
>>>> arch/arm64/mm/proc.S | 26 ++++++++++++++++++++++++++
>>>> 1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>>>> index 1f7ee8c8b7b8..06436916f137 100644
>>>> --- a/arch/arm64/mm/proc.S
>>>> +++ b/arch/arm64/mm/proc.S
>>>> @@ -51,6 +51,25 @@
>>>> #define TCR_KASAN_HW_FLAGS 0
>>>> #endif
>>>>
>>>> +#ifdef CONFIG_ARCH_APPLE
>>>> +
>>>> +/*
>>>> + * Apple cores appear to black-hole writes done with nGnRE.
>>>> + * We settled on a work-around that uses MAIR vs changing every
>>>> single user of
>>>> + * nGnRE across the arm64 code.
>>>> + */
>>>> +
>>>> +#define MAIR_EL1_SET_APPLE \
>>>> + (MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRnE) | \
>>>> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_nGnRnE, MT_DEVICE_nGnRE) | \
>>>> + MAIR_ATTRIDX(MAIR_ATTR_DEVICE_GRE, MT_DEVICE_GRE) | \
>>>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_NC, MT_NORMAL_NC) | \
>>>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL) | \
>>>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL_WT, MT_NORMAL_WT) | \
>>>> + MAIR_ATTRIDX(MAIR_ATTR_NORMAL, MT_NORMAL_TAGGED))
>>>> +
>>>> +#endif
>>>> +
>>>> /*
>>>> * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal
>>>> memory and
>>>> * changed during __cpu_setup to Normal Tagged if the system
>>>> supports MTE.
>>>> @@ -432,6 +451,13 @@ SYM_FUNC_START(__cpu_setup)
>>>> * Memory region attributes
>>>> */
>>>> mov_q x5, MAIR_EL1_SET
>>>> +#ifdef CONFIG_ARCH_APPLE
>>>> + mrs x0, MIDR_EL1
>>>> + lsr w0, w0, #24
>>>> + mov_q x1, MAIR_EL1_SET_APPLE
>>>> + cmp x0, #0x61 // 0x61 = Implementer: Apple
>>>> + csel x5, x1, x5, eq
>>>
>>> Why does this need to be done so early? It would be a lot cleaner if we
>>> could detect this in a similar fashion to other errata and update the
>>> MAIR
>>> appropriately. If that's not possible because of early IO mappings
>>> (which
>>> ones?), then we could instead initialise to nGnRnE unconditionally, but
>>> relax it to nGnRE if we detect that we _don't_ have the erratum.
>>
>> Would that imply another round-trip into the idmap, much like we do
>> when we switch to non-global mappings? Or do you expect that we can change
>> the MAIR with live mappings?
>
> I think we should be able to change it live and then invalidate the TLB. At
> least, my reading of the BBM requirements suggests that it isn't required
> for changing between different types of device memory. I can seek
> clarification from Arm if necessary.
>
Please ignore that patch.

It turns out that the PCIe controller on Apple M1 expects posted writes and so the memory range for it ought to be set nGnRE.
So, we need to use nGnRnE for on-chip MMIO and nGnRE for PCIe BARs.

The MAIR approach isn’t adequate for such a thing, so we’ll have to look elsewhere.

Thank you,
> Will

2021-01-21 15:34:41

by Hector Martin

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] irqchip/apple-aic: Add support for Apple AIC

On 21/01/2021 19.37, Arnd Bergmann wrote:
> On Thu, Jan 21, 2021 at 10:48 AM Linus Walleij <[email protected]> wrote:
>>
>> However weird it may seem, Apple is not in the file
>> Documentation/devicetree/bindings/vendor-prefixes.yaml
>
> Since Apple are already using both the "AAPL" and the "apple"
> prefix themselves, I have a bad feeling about reusing either of
> them for defining the devicetree.org bindings that we add to
> linux/Documentation/devicetree/bindings. The question is: if
> not "apple", what else should we use here?

This ties into the larger question of how we should handle devicetrees
in general on these platforms.

The two extremes are:

1) Have the bootloader outright convert ADT to FDT and make Linux
support the entirety of Apple's devicetree structure, or

2) Maintain our own devicetrees and ignore Apple's entirely

My feeling is that 1) is a non-starter, because Linux ARM device trees
and Apple ARM device trees have seen independent evolution from the
PowerPC era, and many details are completely different. Plus conversion
is non-trivial, because the endianness is different and the format is
too ambiguous to do programmatically without complex logic.

On the other hand, cranking out devicetrees by hand for every device
variant that Apple puts out is a waste of time.

Obviously at the bare minimum the bootloader will need to move some
dynamic information from the ADT to the FDT, but that can be a very
specific set of properties (memory layout, MAC addresses, etc).

My current thinking is that we should write offline, automated tooling
to parse, diff, and automatically convert portions of Apple devicetrees
into Linux ones. Then we can more easily maintain our own, but still
ultimately have humans decide what goes into the Linux device trees.

It's worth noting that AIUI Apple does not consider their devicetree
layout to be stable, and it may change any time. On M1 devices, the
devicetree is provided as part of the iBoot2 firmware bundle, which
means it changes from one macOS version to the next (this is paired with
the Darwin kernel itself, and they are upgraded as a unit). It includes
placeholder values that iBoot2 then replaces with data from NOR before
handing control over to the kernel. My goal for our long-term project
[1] is to keep up with iBoot2 updates so that we do not have to instruct
users to dig up old macOS versions.

Quick TL;DR on how these things boot:
- Boot ROM boots
- iBoot1 (system firmware) in NOR flash which looks for a bootable OS in
internal storage (only!) in the form of an APFS container+volume and
then boots
- iBoot2 (OS loader) which loads a bunch of firmware blobs and the
devicetree off of storage, customizes it with system data from NOR, and
then loads a wrapped mach-o file containing
- A Darwin kernel, or in our case a Linux bootloader which then boots
- A standard arm64 Linux blob

The boot ROM is ROM. iBoot1 only ever rolls forward (downgrades
impossible). iBoot2 downgrades are possible but Apple already proved
they can break this willingly or not, at least in betas (macOS 11.2
Beta2 iBoot1 cannot boot Beta1 iBoot2). The secureboot chain goes all
the way up to the mach-o kernel load, that is the first point where we
can change boot policy to load anything we want (with user consent).

[1] https://asahilinux.org/

--
Hector Martin "marcan" ([email protected])
Public Key: https://mrcn.st/pub

2021-01-21 16:30:20

by Marc Zyngier

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On 2021-01-21 15:12, Mohamed Mediouni wrote:
> Please ignore that patch.
>
> It turns out that the PCIe controller on Apple M1 expects posted
> writes and so the memory range for it ought to be set nGnRE.
> So, we need to use nGnRnE for on-chip MMIO and nGnRE for PCIe BARs.
>
> The MAIR approach isn’t adequate for such a thing, so we’ll have to
> look elsewhere.

Well, there isn't many alternative to having a memory type defined
in MAIR if you want to access your PCIe devices with specific
semantics.

It probably means defining a memory type for PCI only, but:
- we only have a single free MT entry, and I'm not sure we can
afford to waste this on a specific platform (can we re-purpose
GRE instead?),
- we'd need to teach the PCI code to use this...

Thanks,

M.
--
Jazz is not dead. It just smells funny...

2021-01-21 16:55:43

by Hector Martin

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] irqchip/apple-aic: Add support for Apple AIC

On 20/01/2021 22.27, Mohamed Mediouni wrote:
> + irq_domain_set_info(d, virq, hw, &apple_aic_irq_chip,
> + d->host_data, handle_level_irq, NULL, NULL);

The AIC automatically masks IRQs on reason fetch, which means the
handle_level_irq flow is redundant. Using the fasteoi flow, as in [1],
should be more efficient.

[1] https://github.com/AsahiLinux/linux/commit/d4cb18c93

--
Hector Martin "marcan" ([email protected])
Public Key: https://mrcn.st/pub

2021-01-21 17:12:13

by Rob Herring

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] irqchip/apple-aic: Add support for Apple AIC

On Thu, Jan 21, 2021 at 9:31 AM Hector Martin 'marcan' <[email protected]> wrote:
>
> On 21/01/2021 19.37, Arnd Bergmann wrote:
> > On Thu, Jan 21, 2021 at 10:48 AM Linus Walleij <[email protected]> wrote:
> >>
> >> However weird it may seem, Apple is not in the file
> >> Documentation/devicetree/bindings/vendor-prefixes.yaml
> >
> > Since Apple are already using both the "AAPL" and the "apple"
> > prefix themselves, I have a bad feeling about reusing either of
> > them for defining the devicetree.org bindings that we add to
> > linux/Documentation/devicetree/bindings. The question is: if
> > not "apple", what else should we use here?
>
> This ties into the larger question of how we should handle devicetrees
> in general on these platforms.
>
> The two extremes are:
>
> 1) Have the bootloader outright convert ADT to FDT and make Linux
> support the entirety of Apple's devicetree structure, or
>
> 2) Maintain our own devicetrees and ignore Apple's entirely
>
> My feeling is that 1) is a non-starter, because Linux ARM device trees
> and Apple ARM device trees have seen independent evolution from the
> PowerPC era, and many details are completely different. Plus conversion
> is non-trivial, because the endianness is different and the format is
> too ambiguous to do programmatically without complex logic.

You are right it's a non-starter. Apple's DT even from PowerPC days
were weird and the hardware was much simpler then. Given we're still
maintaining that code I don't care to add what they've evolved on
their own over the last 15 years and support it for the next 20+ years
(given folks notice when we break 1998 era Macs).

> On the other hand, cranking out devicetrees by hand for every device
> variant that Apple puts out is a waste of time.
>
> Obviously at the bare minimum the bootloader will need to move some
> dynamic information from the ADT to the FDT, but that can be a very
> specific set of properties (memory layout, MAC addresses, etc).
>
> My current thinking is that we should write offline, automated tooling
> to parse, diff, and automatically convert portions of Apple devicetrees
> into Linux ones. Then we can more easily maintain our own, but still
> ultimately have humans decide what goes into the Linux device trees.

Seems reasonable.

> It's worth noting that AIUI Apple does not consider their devicetree
> layout to be stable, and it may change any time.

Yeah, also not something we want to support.

> On M1 devices, the
> devicetree is provided as part of the iBoot2 firmware bundle, which
> means it changes from one macOS version to the next (this is paired with
> the Darwin kernel itself, and they are upgraded as a unit). It includes
> placeholder values that iBoot2 then replaces with data from NOR before
> handing control over to the kernel. My goal for our long-term project
> [1] is to keep up with iBoot2 updates so that we do not have to instruct
> users to dig up old macOS versions.
>
> Quick TL;DR on how these things boot:
> - Boot ROM boots
> - iBoot1 (system firmware) in NOR flash which looks for a bootable OS in
> internal storage (only!) in the form of an APFS container+volume and
> then boots
> - iBoot2 (OS loader) which loads a bunch of firmware blobs and the
> devicetree off of storage, customizes it with system data from NOR, and
> then loads a wrapped mach-o file containing
> - A Darwin kernel, or in our case a Linux bootloader which then boots
> - A standard arm64 Linux blob
>
> The boot ROM is ROM. iBoot1 only ever rolls forward (downgrades
> impossible). iBoot2 downgrades are possible but Apple already proved
> they can break this willingly or not, at least in betas (macOS 11.2
> Beta2 iBoot1 cannot boot Beta1 iBoot2). The secureboot chain goes all
> the way up to the mach-o kernel load, that is the first point where we
> can change boot policy to load anything we want (with user consent).
>
> [1] https://asahilinux.org/
>
> --
> Hector Martin "marcan" ([email protected])
> Public Key: https://mrcn.st/pub
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2021-01-21 17:51:50

by Rob Herring

[permalink] [raw]

Subject: Re: [RFC PATCH 4/7] irqchip/apple-aic: Add support for Apple AIC

On Thu, Jan 21, 2021 at 4:38 AM Arnd Bergmann <[email protected]> wrote:
>
> On Thu, Jan 21, 2021 at 10:48 AM Linus Walleij <[email protected]> wrote:
> >
> > Hi Mohamed,
> >
> > thanks for your patch!
> >
> > On Wed, Jan 20, 2021 at 2:31 PM Mohamed Mediouni
> > <[email protected]> wrote:
> >
> > > +properties:
> > > + compatible:
> > > + items:
> > > + - const: apple,aic
> >
> > However weird it may seem, Apple is not in the file
> > Documentation/devicetree/bindings/vendor-prefixes.yaml
> >
> > (I think it's weird because I remember clearly that they have been
> > using device tree for PPC since ages.)
> >
> > Could you add this 2-liner to that file and send it directly to
> > DT binding maintainers as a single patch as a preparation?
>
> Choosing the vendor prefix here is going to be a little tricky
> and non-obvious.
>
> Background:
>
> Traditionally, it should have been the stock ticker symbol of the
> company (clearly only publicly traded companies would be able
> to produce a Unix capable computer, right?), but there were
> already inconsistent: IBM used "ibm" (in small letters), Sun
> used "SUNW" (in capitals) but in 2007 changed the stock ticker
> symbol to "JAVA", obviously without changing the firmware bindings.
>
> Apple traditionally used "AAPL" (also in caps) in the device tree,
> and there is one remnant of that in the M1 device tree, in the form
> of the "AAPL,phandle" property in each node, which corresponds
> to our "linux,phandle". (phandles were introduced as properties in
> both of the flattened DT formats, .dtb and apple's own format).
> There are also "AAPL,unit-string properties and some device_type
> strings (AAPL,display-crossbar, AAPL,atc-dpxbar, ...) in the M1 DT,
> and the CPU nodes (and only those) use "apple" in small letters
> as in "apple,icestorm","ARM,v8". The other nodes tend to not have
> a vendor prefix, but a lot use a subsystem name as the prefix, such
> as compatible="gpio,t8101" or compatible="tempsensor,t8020".
>
> Since Apple are already using both the "AAPL" and the "apple"
> prefix themselves, I have a bad feeling about reusing either of
> them for defining the devicetree.org bindings that we add to
> linux/Documentation/devicetree/bindings. The question is: if
> not "apple", what else should we use here?

IMO, 'AAPL' as that is what is already in use in the kernel. I don't
see why it matters at all what Apple is using. It might if we used any
of the ADT as-is, but I don't see that happening from what I've seen.

Rob

2021-01-21 18:00:07

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On Thu, Jan 21, 2021 at 04:25:54PM +0000, Marc Zyngier wrote:
> On 2021-01-21 15:12, Mohamed Mediouni wrote:
> > Please ignore that patch.
> >
> > It turns out that the PCIe controller on Apple M1 expects posted
> > writes and so the memory range for it ought to be set nGnRE.
> > So, we need to use nGnRnE for on-chip MMIO and nGnRE for PCIe BARs.
> >
> > The MAIR approach isn’t adequate for such a thing, so we’ll have to
> > look elsewhere.
>
> Well, there isn't many alternative to having a memory type defined
> in MAIR if you want to access your PCIe devices with specific
> semantics.
>
> It probably means defining a memory type for PCI only, but:
> - we only have a single free MT entry, and I'm not sure we can
> afford to waste this on a specific platform (can we re-purpose
> GRE instead?),

We already have an nGnRnE MAIR for config space accesses.

Will

2021-01-21 18:30:10

by Marc Zyngier

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

On 2021-01-21 17:55, Will Deacon wrote:
> On Thu, Jan 21, 2021 at 04:25:54PM +0000, Marc Zyngier wrote:
>> On 2021-01-21 15:12, Mohamed Mediouni wrote:
>> > Please ignore that patch.
>> >
>> > It turns out that the PCIe controller on Apple M1 expects posted
>> > writes and so the memory range for it ought to be set nGnRE.
>> > So, we need to use nGnRnE for on-chip MMIO and nGnRE for PCIe BARs.
>> >
>> > The MAIR approach isn’t adequate for such a thing, so we’ll have to
>> > look elsewhere.
>>
>> Well, there isn't many alternative to having a memory type defined
>> in MAIR if you want to access your PCIe devices with specific
>> semantics.
>>
>> It probably means defining a memory type for PCI only, but:
>> - we only have a single free MT entry, and I'm not sure we can
>> afford to waste this on a specific platform (can we re-purpose
>> GRE instead?),
>
> We already have an nGnRnE MAIR for config space accesses.

I'm confused. If M1 needs nGnRE for PCI, and overrides nGnRE to nE
for its in-SoC accesses, where does nGnRE goes?

Or do you propose that it is the page tables that get a different
MT index?

M.
--
Jazz is not dead. It just smells funny...

2021-01-21 18:33:17

[permalink] [raw]

Subject: Re: [RFC PATCH 3/7] arm64: mm: use nGnRnE instead of nGnRE on Apple processors

> On 21 Jan 2021, at 19:15, Marc Zyngier <[email protected]> wrote:
>
> On 2021-01-21 17:55, Will Deacon wrote:
>> On Thu, Jan 21, 2021 at 04:25:54PM +0000, Marc Zyngier wrote:
>>> On 2021-01-21 15:12, Mohamed Mediouni wrote:
>>>> Please ignore that patch.
>>>>
>>>> It turns out that the PCIe controller on Apple M1 expects posted
>>>> writes and so the memory range for it ought to be set nGnRE.
>>>> So, we need to use nGnRnE for on-chip MMIO and nGnRE for PCIe BARs.
>>>>
>>>> The MAIR approach isn’t adequate for such a thing, so we’ll have to
>>>> look elsewhere.
>>> Well, there isn't many alternative to having a memory type defined
>>> in MAIR if you want to access your PCIe devices with specific
>>> semantics.
>>> It probably means defining a memory type for PCI only, but:
>>> - we only have a single free MT entry, and I'm not sure we can
>>> afford to waste this on a specific platform (can we re-purpose
>>> GRE instead?),
>> We already have an nGnRnE MAIR for config space accesses.
>
> I'm confused. If M1 needs nGnRE for PCI, and overrides nGnRE to nE
> for its in-SoC accesses, where does nGnRE goes?
>
> Or do you propose that it is the page tables that get a different
> MT index?
>

That MAIR patch that I added overrides nGnRE accesses to nGnRnE.

Linux tries to access to those SoC devices using nGnRE as the device
memory type without that workaround.

Maybe have a device tree property to override the used device memory type
for a given device on the SoC? Or that’s too big for what’s at the end just one
particular set of SoCs?

But what the hardware wants is accesses to in-SoC devices being nGnRnE
and access to the PCIe BARs being nGnRE.

So both have to be supported…

> M.
> --
> Jazz is not dead. It just smells funny...

2021-01-21 18:41:01