This patch series enables the basic ACPI infrastructure for RISC-V.
Supporting external interrupt controllers is in progress and hence it is
tested using poll based HVC SBI console and RAM disk.
The first patch in this series is one of the patch from Jisheng's
series [1] which is not merged yet. This patch is required to support
ACPI since efi_init() which gets called before sbi_init() can enable
static branches and hits a panic.
Patch 2 and 3 are ACPICA patches which are merged now into acpica
but not yet pulled into the linux sources. They exist in this patch set
as reference. This series can be merged only after those ACPICA patches
are pulled into linux.
Below are two ECRs approved by ASWG.
RINTC - https://drive.google.com/file/d/1R6k4MshhN3WTT-hwqAquu5nX6xSEqK2l/view
RHCT - https://drive.google.com/file/d/1nP3nFiH4jkPMp6COOxP6123DCZKR-tia/view
Based-on: [email protected]
(https://lore.kernel.org/lkml/[email protected]/)
[1] https://lore.kernel.org/all/[email protected]/
Changes since V3:
1) Added two more driver patches to workaround allmodconfig build failure.
2) Separated removal of riscv_of_processor_hartid() to a different patch.
3) Addressed Conor's feedback.
4) Rebased to v6.3-rc5 and added latest tags
Changes since V2:
1) Dropped ACPI_PROCESSOR patch.
2) Added new patch to print debug info of RISC-V INTC in MADT
3) Addressed other comments from Drew.
4) Rebased and updated tags
Changes since V1:
1) Dropped PCI changes and instead added dummy interfaces just to enable
building ACPI core when CONFIG_PCI is enabled. Actual PCI changes will
be added in future along with external interrupt controller support
in ACPI.
2) Squashed couple of patches so that new code added gets built in each
commit.
3) Fixed the missing wake_cpu code in timer refactor patch as pointed by
Conor
4) Fixed an issue with SMP disabled.
5) Addressed other comments from Conor.
6) Updated documentation patch as per feedback from Sanjaya.
7) Fixed W=1 and checkpatch --strict issues.
8) Added ACK/RB tags
These changes are available at
https://github.com/vlsunil/linux/commits/acpi_b1_us_review_ipi17_V4
Testing:
1) Build latest Qemu
2) Build EDK2 as per instructions in
https://github.com/vlsunil/riscv-uefi-edk2-docs/wiki/RISC-V-Qemu-Virt-support
3) Build Linux after enabling SBI HVC and SBI earlycon
CONFIG_RISCV_SBI_V01=y
CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
CONFIG_HVC_RISCV_SBI=y
4) Build buildroot.
Run with below command.
qemu-system-riscv64 -nographic \
-drive file=Build/RiscVVirtQemu/RELEASE_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1 \
-machine virt -smp 16 -m 2G \
-kernel arch/riscv/boot/Image \
-initrd buildroot/output/images/rootfs.cpio \
-append "root=/dev/ram ro console=hvc0 earlycon=sbi"
Jisheng Zhang (1):
riscv: move sbi_init() earlier before jump_label_init()
Sunil V L (22):
ACPICA: MADT: Add RISC-V INTC interrupt controller
ACPICA: Add structure definitions for RISC-V RHCT
ACPI: tables: Print RINTC information when MADT is parsed
ACPI: OSL: Make should_use_kmap() 0 for RISC-V
RISC-V: Add support to build the ACPI core
ACPI: processor_core: RISC-V: Enable mapping processor to the hartid
RISC-V: ACPI: Cache and retrieve the RINTC structure
drivers/acpi: RISC-V: Add RHCT related code
RISC-V: smpboot: Create wrapper smp_setup()
RISC-V: smpboot: Add ACPI support in smp_setup()
RISC-V: cpufeature: Avoid calling riscv_of_processor_hartid()
RISC-V: cpufeature: Add ACPI support in riscv_fill_hwcap()
RISC-V: cpu: Enable cpuinfo for ACPI systems
irqchip/riscv-intc: Add ACPI support
clocksource/timer-riscv: Refactor riscv_timer_init_dt()
clocksource/timer-riscv: Add ACPI support
RISC-V: time.c: Add ACPI support for time_init()
RISC-V: Add ACPI initialization in setup_arch()
RISC-V: Enable ACPI in defconfig
MAINTAINERS: Add entry for drivers/acpi/riscv
platform/surface: Disable for RISC-V
crypto: hisilicon/qm: Workaround to enable build with RISC-V clang
.../admin-guide/kernel-parameters.txt | 8 +-
MAINTAINERS | 8 +
arch/riscv/Kconfig | 5 +
arch/riscv/configs/defconfig | 1 +
arch/riscv/include/asm/acenv.h | 11 +
arch/riscv/include/asm/acpi.h | 77 +++++
arch/riscv/include/asm/cpu.h | 8 +
arch/riscv/kernel/Makefile | 2 +
arch/riscv/kernel/acpi.c | 266 ++++++++++++++++++
arch/riscv/kernel/cpu.c | 30 +-
arch/riscv/kernel/cpufeature.c | 44 ++-
arch/riscv/kernel/setup.c | 27 +-
arch/riscv/kernel/smpboot.c | 77 ++++-
arch/riscv/kernel/time.c | 25 +-
drivers/acpi/Makefile | 2 +
drivers/acpi/osl.c | 2 +-
drivers/acpi/processor_core.c | 29 ++
drivers/acpi/riscv/Makefile | 2 +
drivers/acpi/riscv/rhct.c | 83 ++++++
drivers/acpi/tables.c | 10 +
drivers/clocksource/timer-riscv.c | 92 +++---
drivers/crypto/hisilicon/qm.c | 13 +-
drivers/irqchip/irq-riscv-intc.c | 74 ++++-
drivers/platform/surface/aggregator/Kconfig | 2 +-
include/acpi/actbl2.h | 69 ++++-
25 files changed, 867 insertions(+), 100 deletions(-)
create mode 100644 arch/riscv/include/asm/acenv.h
create mode 100644 arch/riscv/include/asm/acpi.h
create mode 100644 arch/riscv/include/asm/cpu.h
create mode 100644 arch/riscv/kernel/acpi.c
create mode 100644 drivers/acpi/riscv/Makefile
create mode 100644 drivers/acpi/riscv/rhct.c
--
2.34.1
ACPICA commit bd6d1ae1e13abe78e149c8b61b4bc7bc7feab015
The ECR to add RISC-V INTC interrupt controller is approved by
the UEFI forum and will be available in the next revision of
the ACPI specification.
Link: https://github.com/acpica/acpica/commit/bd6d1ae1
Reference: Mantis ID: 2348
Signed-off-by: Sunil V L <[email protected]>
---
include/acpi/actbl2.h | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index b2973dbe37ee..c432fd15db65 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -891,7 +891,8 @@ enum acpi_madt_type {
ACPI_MADT_TYPE_MSI_PIC = 21,
ACPI_MADT_TYPE_BIO_PIC = 22,
ACPI_MADT_TYPE_LPC_PIC = 23,
- ACPI_MADT_TYPE_RESERVED = 24, /* 24 to 0x7F are reserved */
+ ACPI_MADT_TYPE_RINTC = 24,
+ ACPI_MADT_TYPE_RESERVED = 25, /* 25 to 0x7F are reserved */
ACPI_MADT_TYPE_OEM_RESERVED = 0x80 /* 0x80 to 0xFF are reserved for OEM use */
};
@@ -1250,6 +1251,24 @@ enum acpi_madt_lpc_pic_version {
ACPI_MADT_LPC_PIC_VERSION_RESERVED = 2 /* 2 and greater are reserved */
};
+/* 24: RISC-V INTC */
+struct acpi_madt_rintc {
+ struct acpi_subtable_header header;
+ u8 version;
+ u8 reserved;
+ u32 flags;
+ u64 hart_id;
+ u32 uid; /* ACPI processor UID */
+};
+
+/* Values for RISC-V INTC Version field above */
+
+enum acpi_madt_rintc_version {
+ ACPI_MADT_RINTC_VERSION_NONE = 0,
+ ACPI_MADT_RINTC_VERSION_V1 = 1,
+ ACPI_MADT_RINTC_VERSION_RESERVED = 2 /* 2 and greater are reserved */
+};
+
/* 80: OEM data */
struct acpi_madt_oem_data {
--
2.34.1
When MADT is parsed, print RINTC information as below:
ACPI: RISC-V INTC (acpi_uid[0x0000] hart_id[0x0] enabled)
ACPI: RISC-V INTC (acpi_uid[0x0001] hart_id[0x1] enabled)
...
ACPI: RISC-V INTC (acpi_uid[0x000f] hart_id[0xf] enabled)
This debug information will be very helpful during bring up.
Signed-off-by: Sunil V L <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
---
drivers/acpi/tables.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 7b4680da57d7..8ab0a82b4da4 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -220,6 +220,16 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
}
break;
+ case ACPI_MADT_TYPE_RINTC:
+ {
+ struct acpi_madt_rintc *p = (struct acpi_madt_rintc *)header;
+
+ pr_debug("RISC-V INTC (acpi_uid[0x%04x] hart_id[0x%llx] %s)\n",
+ p->uid, p->hart_id,
+ (p->flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+ }
+ break;
+
default:
pr_warn("Found unsupported MADT entry (type = 0x%x)\n",
header->type);
--
2.34.1
Enable ACPI core for RISC-V after adding architecture-specific
interfaces and header files required to build the ACPI core.
1) Couple of header files are required unconditionally by the ACPI
core. Add empty acenv.h and cpu.h header files.
2) If CONFIG_PCI is enabled, a few PCI related interfaces need to
be provided by the architecture. Define dummy interfaces for now
so that build succeeds. Actual implementation will be added when
PCI support is added for ACPI along with external interrupt
controller support.
3) A few globals and memory mapping related functions specific
to the architecture need to be provided.
Signed-off-by: Sunil V L <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/Kconfig | 5 +++
arch/riscv/include/asm/acenv.h | 11 +++++
arch/riscv/include/asm/acpi.h | 61 ++++++++++++++++++++++++++
arch/riscv/include/asm/cpu.h | 8 ++++
arch/riscv/kernel/Makefile | 2 +
arch/riscv/kernel/acpi.c | 80 ++++++++++++++++++++++++++++++++++
6 files changed, 167 insertions(+)
create mode 100644 arch/riscv/include/asm/acenv.h
create mode 100644 arch/riscv/include/asm/acpi.h
create mode 100644 arch/riscv/include/asm/cpu.h
create mode 100644 arch/riscv/kernel/acpi.c
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 139055bcfcae..710037f7ca0a 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -12,6 +12,8 @@ config 32BIT
config RISCV
def_bool y
+ select ACPI_GENERIC_GSI if ACPI
+ select ACPI_REDUCED_HARDWARE_ONLY if ACPI
select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION
select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
@@ -639,6 +641,7 @@ config EFI
depends on OF && !XIP_KERNEL
depends on MMU
default y
+ select ARCH_SUPPORTS_ACPI if 64BIT
select EFI_GENERIC_STUB
select EFI_PARAMS_FROM_FDT
select EFI_RUNTIME_WRAPPERS
@@ -742,3 +745,5 @@ source "drivers/cpufreq/Kconfig"
endmenu # "CPU Power Management"
source "arch/riscv/kvm/Kconfig"
+
+source "drivers/acpi/Kconfig"
diff --git a/arch/riscv/include/asm/acenv.h b/arch/riscv/include/asm/acenv.h
new file mode 100644
index 000000000000..43ae2e32c779
--- /dev/null
+++ b/arch/riscv/include/asm/acenv.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * RISC-V specific ACPICA environments and implementation
+ */
+
+#ifndef _ASM_ACENV_H
+#define _ASM_ACENV_H
+
+/* This header is required unconditionally by the ACPI core */
+
+#endif /* _ASM_ACENV_H */
diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
new file mode 100644
index 000000000000..bcade255bd6e
--- /dev/null
+++ b/arch/riscv/include/asm/acpi.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2013-2014, Linaro Ltd.
+ * Author: Al Stone <[email protected]>
+ * Author: Graeme Gregory <[email protected]>
+ * Author: Hanjun Guo <[email protected]>
+ *
+ * Copyright (C) 2021-2023, Ventana Micro Systems Inc.
+ * Author: Sunil V L <[email protected]>
+ */
+
+#ifndef _ASM_ACPI_H
+#define _ASM_ACPI_H
+
+/* Basic configuration for ACPI */
+#ifdef CONFIG_ACPI
+
+/* ACPI table mapping after acpi_permanent_mmap is set */
+void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
+#define acpi_os_ioremap acpi_os_ioremap
+
+#define acpi_strict 1 /* No out-of-spec workarounds on RISC-V */
+extern int acpi_disabled;
+extern int acpi_noirq;
+extern int acpi_pci_disabled;
+
+static inline void disable_acpi(void)
+{
+ acpi_disabled = 1;
+ acpi_pci_disabled = 1;
+ acpi_noirq = 1;
+}
+
+static inline void enable_acpi(void)
+{
+ acpi_disabled = 0;
+ acpi_pci_disabled = 0;
+ acpi_noirq = 0;
+}
+
+/*
+ * The ACPI processor driver for ACPI core code needs this macro
+ * to find out whether this cpu was already mapped (mapping from CPU hardware
+ * ID to CPU logical ID) or not.
+ */
+#define cpu_physical_id(cpu) cpuid_to_hartid_map(cpu)
+
+/*
+ * Since MADT must provide at least one RINTC structure, the
+ * CPU will be always available in MADT on RISC-V.
+ */
+static inline bool acpi_has_cpu_in_madt(void)
+{
+ return true;
+}
+
+static inline void arch_fix_phys_package_id(int num, u32 slot) { }
+
+#endif /* CONFIG_ACPI */
+
+#endif /*_ASM_ACPI_H*/
diff --git a/arch/riscv/include/asm/cpu.h b/arch/riscv/include/asm/cpu.h
new file mode 100644
index 000000000000..28d45a6678ce
--- /dev/null
+++ b/arch/riscv/include/asm/cpu.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _ASM_CPU_H
+#define _ASM_CPU_H
+
+/* This header is required unconditionally by the ACPI core */
+
+#endif /* _ASM_CPU_H */
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index 67f542be1bea..8ce334f6932f 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -90,3 +90,5 @@ obj-$(CONFIG_EFI) += efi.o
obj-$(CONFIG_COMPAT) += compat_syscall_table.o
obj-$(CONFIG_COMPAT) += compat_signal.o
obj-$(CONFIG_COMPAT) += compat_vdso/
+
+obj-$(CONFIG_ACPI) += acpi.o
diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
new file mode 100644
index 000000000000..81d448c41714
--- /dev/null
+++ b/arch/riscv/kernel/acpi.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * RISC-V Specific Low-Level ACPI Boot Support
+ *
+ * Copyright (C) 2013-2014, Linaro Ltd.
+ * Author: Al Stone <[email protected]>
+ * Author: Graeme Gregory <[email protected]>
+ * Author: Hanjun Guo <[email protected]>
+ * Author: Tomasz Nowicki <[email protected]>
+ * Author: Naresh Bhat <[email protected]>
+ *
+ * Copyright (C) 2021-2023, Ventana Micro Systems Inc.
+ * Author: Sunil V L <[email protected]>
+ */
+
+#include <linux/acpi.h>
+#include <linux/io.h>
+#include <linux/pci.h>
+
+int acpi_noirq = 1; /* skip ACPI IRQ initialization */
+int acpi_disabled = 1;
+EXPORT_SYMBOL(acpi_disabled);
+
+int acpi_pci_disabled = 1; /* skip ACPI PCI scan and IRQ initialization */
+EXPORT_SYMBOL(acpi_pci_disabled);
+
+/*
+ * __acpi_map_table() will be called before paging_init(), so early_ioremap()
+ * or early_memremap() should be called here to for ACPI table mapping.
+ */
+void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size)
+{
+ if (!size)
+ return NULL;
+
+ return early_memremap(phys, size);
+}
+
+void __init __acpi_unmap_table(void __iomem *map, unsigned long size)
+{
+ if (!map || !size)
+ return;
+
+ early_memunmap(map, size);
+}
+
+void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
+{
+ return memremap(phys, size, MEMREMAP_WB);
+}
+
+#ifdef CONFIG_PCI
+
+/*
+ * These interfaces are defined just to enable building ACPI core.
+ * TODO: Update it with actual implementation when external interrupt
+ * controller support is added in RISC-V ACPI.
+ */
+int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
+ int reg, int len, u32 *val)
+{
+ return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
+int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
+ int reg, int len, u32 val)
+{
+ return PCIBIOS_DEVICE_NOT_FOUND;
+}
+
+int acpi_pci_bus_find_domain_nr(struct pci_bus *bus)
+{
+ return -1;
+}
+
+struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
+{
+ return NULL;
+}
+#endif /* CONFIG_PCI */
--
2.34.1
From: Jisheng Zhang <[email protected]>
We call jump_label_init() in setup_arch() is to use static key
mechanism earlier, but riscv jump label relies on the sbi functions,
If we enable static key before sbi_init(), the code path looks like:
static_branch_enable()
..
arch_jump_label_transform()
patch_text_nosync()
flush_icache_range()
flush_icache_all()
sbi_remote_fence_i() for CONFIG_RISCV_SBI case
__sbi_rfence()
Since sbi isn't initialized, so NULL deference! Here is a typical
panic log:
[ 0.000000] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 0.000000] Oops [#1]
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-rc7+ #79
[ 0.000000] Hardware name: riscv-virtio,qemu (DT)
[ 0.000000] epc : 0x0
[ 0.000000] ra : sbi_remote_fence_i+0x1e/0x26
[ 0.000000] epc : 0000000000000000 ra : ffffffff80005826 sp : ffffffff80c03d50
[ 0.000000] gp : ffffffff80ca6178 tp : ffffffff80c0ad80 t0 : 6200000000000000
[ 0.000000] t1 : 0000000000000000 t2 : 62203a6b746e6972 s0 : ffffffff80c03d60
[ 0.000000] s1 : ffffffff80001af6 a0 : 0000000000000000 a1 : 0000000000000000
[ 0.000000] a2 : 0000000000000000 a3 : 0000000000000000 a4 : 0000000000000000
[ 0.000000] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000080200
[ 0.000000] s2 : ffffffff808b3e48 s3 : ffffffff808bf698 s4 : ffffffff80cb2818
[ 0.000000] s5 : 0000000000000001 s6 : ffffffff80c9c345 s7 : ffffffff80895aa0
[ 0.000000] s8 : 0000000000000001 s9 : 000000000000007f s10: 0000000000000000
[ 0.000000] s11: 0000000000000000 t3 : ffffffff80824d08 t4 : 0000000000000022
[ 0.000000] t5 : 000000000000003d t6 : 0000000000000000
[ 0.000000] status: 0000000000000100 badaddr: 0000000000000000 cause: 000000000000000c
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[ 0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
Fix this issue by moving sbi_init() earlier before jump_label_init()
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Anup Patel <[email protected]>
Reviewed-by: Atish Patra <[email protected]>
---
arch/riscv/kernel/setup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 376d2827e736..2d45a416d283 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -270,6 +270,7 @@ void __init setup_arch(char **cmdline_p)
*cmdline_p = boot_command_line;
early_ioremap_setup();
+ sbi_init();
jump_label_init();
parse_early_param();
@@ -287,7 +288,6 @@ void __init setup_arch(char **cmdline_p)
misc_mem_init();
init_resources();
- sbi_init();
#ifdef CONFIG_KASAN
kasan_init();
--
2.34.1
Without this, if the tables are larger than 4K,
acpi_map() will fail.
Signed-off-by: Sunil V L <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/osl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3269a888fb7a..f725813d0cce 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -276,7 +276,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
return NULL;
}
-#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)
+#if defined(CONFIG_IA64) || defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
/* ioremap will take care of cache attributes */
#define should_use_kmap(pfn) 0
#else
--
2.34.1
processor_core needs arch-specific functions to map the ACPI ID
to the physical ID. In RISC-V platforms, hartid is the physical id
and RINTC structure in MADT provides this mapping. Add arch-specific
function to get this mapping from RINTC.
Signed-off-by: Sunil V L <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
---
arch/riscv/include/asm/acpi.h | 3 +++
drivers/acpi/processor_core.c | 29 +++++++++++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index bcade255bd6e..9be52b6ffae1 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -15,6 +15,9 @@
/* Basic configuration for ACPI */
#ifdef CONFIG_ACPI
+typedef u64 phys_cpuid_t;
+#define PHYS_CPUID_INVALID INVALID_HARTID
+
/* ACPI table mapping after acpi_permanent_mmap is set */
void *acpi_os_ioremap(acpi_physical_address phys, acpi_size size);
#define acpi_os_ioremap acpi_os_ioremap
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 2ac48cda5b20..d6606a9f2da6 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -106,6 +106,32 @@ static int map_gicc_mpidr(struct acpi_subtable_header *entry,
return -EINVAL;
}
+/*
+ * Retrieve the RISC-V hartid for the processor
+ */
+static int map_rintc_hartid(struct acpi_subtable_header *entry,
+ int device_declaration, u32 acpi_id,
+ phys_cpuid_t *hartid)
+{
+ struct acpi_madt_rintc *rintc =
+ container_of(entry, struct acpi_madt_rintc, header);
+
+ if (!(rintc->flags & ACPI_MADT_ENABLED))
+ return -ENODEV;
+
+ /* device_declaration means Device object in DSDT, in the
+ * RISC-V, logical processors are required to
+ * have a Processor Device object in the DSDT, so we should
+ * check device_declaration here
+ */
+ if (device_declaration && rintc->uid == acpi_id) {
+ *hartid = rintc->hart_id;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
static phys_cpuid_t map_madt_entry(struct acpi_table_madt *madt,
int type, u32 acpi_id)
{
@@ -136,6 +162,9 @@ static phys_cpuid_t map_madt_entry(struct acpi_table_madt *madt,
} else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {
if (!map_gicc_mpidr(header, type, acpi_id, &phys_id))
break;
+ } else if (header->type == ACPI_MADT_TYPE_RINTC) {
+ if (!map_rintc_hartid(header, type, acpi_id, &phys_id))
+ break;
}
entry += header->length;
}
--
2.34.1
Enable SMP boot on ACPI based platforms by using the RINTC
structures in the MADT table.
Signed-off-by: Sunil V L <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/include/asm/acpi.h | 2 +
arch/riscv/kernel/smpboot.c | 72 ++++++++++++++++++++++++++++++++++-
2 files changed, 73 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index 2b3e78d5a13b..b26ba911f0a9 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -63,6 +63,8 @@ struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu);
u32 get_acpi_id_for_cpu(int cpu);
int acpi_get_riscv_isa(struct acpi_table_header *table,
unsigned int cpu, const char **isa);
+
+static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
#else
static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
unsigned int cpu, const char **isa)
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 26214ddefaa4..6a854b200b72 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -8,6 +8,7 @@
* Copyright (C) 2017 SiFive
*/
+#include <linux/acpi.h>
#include <linux/arch_topology.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -70,6 +71,72 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
}
}
+#ifdef CONFIG_ACPI
+static unsigned int cpu_count = 1;
+
+static int __init acpi_parse_rintc(union acpi_subtable_headers *header, const unsigned long end)
+{
+ unsigned long hart;
+ static bool found_boot_cpu;
+ struct acpi_madt_rintc *processor = (struct acpi_madt_rintc *)header;
+
+ /*
+ * Each RINTC structure in MADT will have a flag. If ACPI_MADT_ENABLED
+ * bit in the flag is not enabled, it means OS should not try to enable
+ * the cpu to which RINTC belongs.
+ */
+ if (!(processor->flags & ACPI_MADT_ENABLED))
+ return 0;
+
+ if (BAD_MADT_ENTRY(processor, end))
+ return -EINVAL;
+
+ acpi_table_print_madt_entry(&header->common);
+
+ hart = processor->hart_id;
+ if (hart == INVALID_HARTID) {
+ pr_warn("Invalid hartid\n");
+ return 0;
+ }
+
+ if (hart == cpuid_to_hartid_map(0)) {
+ BUG_ON(found_boot_cpu);
+ found_boot_cpu = true;
+ early_map_cpu_to_node(0, acpi_numa_get_nid(cpu_count));
+ return 0;
+ }
+
+ if (cpu_count >= NR_CPUS) {
+ pr_warn("NR_CPUS is too small for the number of ACPI tables.\n");
+ return 0;
+ }
+
+ cpuid_to_hartid_map(cpu_count) = hart;
+ early_map_cpu_to_node(cpu_count, acpi_numa_get_nid(cpu_count));
+ cpu_count++;
+
+ return 0;
+}
+
+static void __init acpi_parse_and_init_cpus(void)
+{
+ int cpuid;
+
+ cpu_set_ops(0);
+
+ acpi_table_parse_madt(ACPI_MADT_TYPE_RINTC, acpi_parse_rintc, 0);
+
+ for (cpuid = 1; cpuid < nr_cpu_ids; cpuid++) {
+ if (cpuid_to_hartid_map(cpuid) != INVALID_HARTID) {
+ cpu_set_ops(cpuid);
+ set_cpu_possible(cpuid, true);
+ }
+ }
+}
+#else
+#define acpi_parse_and_init_cpus(...) do { } while (0)
+#endif
+
static void __init of_parse_and_init_cpus(void)
{
struct device_node *dn;
@@ -118,7 +185,10 @@ static void __init of_parse_and_init_cpus(void)
void __init setup_smp(void)
{
- of_parse_and_init_cpus();
+ if (acpi_disabled)
+ of_parse_and_init_cpus();
+ else
+ acpi_parse_and_init_cpus();
}
static int start_secondary_cpu(int cpu, struct task_struct *tidle)
--
2.34.1
On ACPI based platforms, few details like ISA need to be read
from the ACPI table. Enable cpuinfo on ACPI based systems.
ACPI has nothing similar to DT compatible property for each CPU.
SBI calls must be used to get vendor/arch/imp ID for any errata.
Signed-off-by: Sunil V L <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
---
arch/riscv/kernel/cpu.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 8400f0cc9704..ace4752516d8 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -3,10 +3,12 @@
* Copyright (C) 2012 Regents of the University of California
*/
+#include <linux/acpi.h>
#include <linux/cpu.h>
#include <linux/init.h>
#include <linux/seq_file.h>
#include <linux/of.h>
+#include <asm/acpi.h>
#include <asm/csr.h>
#include <asm/hwcap.h>
#include <asm/sbi.h>
@@ -283,23 +285,35 @@ static void c_stop(struct seq_file *m, void *v)
static int c_show(struct seq_file *m, void *v)
{
unsigned long cpu_id = (unsigned long)v - 1;
- struct device_node *node = of_get_cpu_node(cpu_id, NULL);
struct riscv_cpuinfo *ci = per_cpu_ptr(&riscv_cpuinfo, cpu_id);
+ struct device_node *node;
const char *compat, *isa;
seq_printf(m, "processor\t: %lu\n", cpu_id);
seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
- if (!of_property_read_string(node, "riscv,isa", &isa))
- print_isa(m, isa);
- print_mmu(m);
- if (!of_property_read_string(node, "compatible", &compat)
- && strcmp(compat, "riscv"))
- seq_printf(m, "uarch\t\t: %s\n", compat);
+
+ if (acpi_disabled) {
+ node = of_get_cpu_node(cpu_id, NULL);
+ if (!of_property_read_string(node, "riscv,isa", &isa))
+ print_isa(m, isa);
+
+ print_mmu(m);
+ if (!of_property_read_string(node, "compatible", &compat) &&
+ strcmp(compat, "riscv"))
+ seq_printf(m, "uarch\t\t: %s\n", compat);
+
+ of_node_put(node);
+ } else {
+ if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
+ print_isa(m, isa);
+
+ print_mmu(m);
+ }
+
seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid);
seq_printf(m, "marchid\t\t: 0x%lx\n", ci->marchid);
seq_printf(m, "mimpid\t\t: 0x%lx\n", ci->mimpid);
seq_puts(m, "\n");
- of_node_put(node);
return 0;
}
--
2.34.1
Add support to build ACPI subsystem in defconfig.
Signed-off-by: Sunil V L <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
---
arch/riscv/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
index d98d6e90b2b8..8822b49ddb59 100644
--- a/arch/riscv/configs/defconfig
+++ b/arch/riscv/configs/defconfig
@@ -238,3 +238,4 @@ CONFIG_RCU_EQS_DEBUG=y
# CONFIG_FTRACE is not set
# CONFIG_RUNTIME_TESTING_MENU is not set
CONFIG_MEMTEST=y
+CONFIG_ACPI=y
--
2.34.1
With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in
allmodconfig build. The gcc tool chain builds this driver removing the
inline arm64 assembly code. However, clang for RISC-V tries to build
the arm64 assembly and below error is seen.
drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint '+Q' in asm
"+Q" (*((char __iomem *)fun_base))
^
It appears that RISC-V clang is not smart enough to detect
IS_ENABLED(CONFIG_ARM64) and remove the dead code.
As a workaround, move this check to preprocessing stage which works
with the RISC-V clang tool chain.
Signed-off-by: Sunil V L <[email protected]>
---
drivers/crypto/hisilicon/qm.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index e4c84433a88a..a5f521529ab2 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -611,13 +611,9 @@ EXPORT_SYMBOL_GPL(hisi_qm_wait_mb_ready);
static void qm_mb_write(struct hisi_qm *qm, const void *src)
{
void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
- unsigned long tmp0 = 0, tmp1 = 0;
- if (!IS_ENABLED(CONFIG_ARM64)) {
- memcpy_toio(fun_base, src, 16);
- dma_wmb();
- return;
- }
+#if IS_ENABLED(CONFIG_ARM64)
+ unsigned long tmp0 = 0, tmp1 = 0;
asm volatile("ldp %0, %1, %3\n"
"stp %0, %1, %2\n"
@@ -627,6 +623,11 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
"+Q" (*((char __iomem *)fun_base))
: "Q" (*((char *)src))
: "memory");
+#else
+ memcpy_toio(fun_base, src, 16);
+ dma_wmb();
+#endif
+
}
static int qm_mb_nolock(struct hisi_qm *qm, struct qm_mailbox *mailbox)
--
2.34.1
With CONFIG_ACPI enabled for RISC-V, this driver gets enabled
in allmodconfig build. However, RISC-V doesn't support sub-word
atomics which is used by this driver. Due to this, the build fails
with below error.
In function ‘ssh_seq_next’,
inlined from ‘ssam_request_write_data’ at drivers/platform/surface/aggregator/controller.c:1483:8:
././include/linux/compiler_types.h:399:45: error: call to ‘__compiletime_assert_335’ declared with attribute error: BUILD_BUG failed
399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^
./include/linux/compiler.h:78:45: note: in definition of macro ‘unlikely’
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
././include/linux/compiler_types.h:387:9: note: in expansion of macro ‘__compiletime_assert’
387 | __compiletime_assert(condition, msg, prefix, suffix)
| ^~~~~~~~~~~~~~~~~~~~
././include/linux/compiler_types.h:399:9: note: in expansion of macro ‘_compiletime_assert’
399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:59:21: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
| ^~~~~~~~~~~~~~~~
./arch/riscv/include/asm/cmpxchg.h:335:17: note: in expansion of macro ‘BUILD_BUG’
335 | BUILD_BUG(); \
| ^~~~~~~~~
./arch/riscv/include/asm/cmpxchg.h:344:30: note: in expansion of macro ‘__cmpxchg’
344 | (__typeof__(*(ptr))) __cmpxchg((ptr), \
| ^~~~~~~~~
./include/linux/atomic/atomic-instrumented.h:1916:9: note: in expansion of macro ‘arch_cmpxchg’
1916 | arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
| ^~~~~~~~~~~~
drivers/platform/surface/aggregator/controller.c:61:32: note: in expansion of macro ‘cmpxchg’
61 | while (unlikely((ret = cmpxchg(&c->value, old, new)) != old)) {
| ^~~~~~~
So, disable this driver for RISC-V even when ACPI is enabled for now.
Signed-off-by: Sunil V L <[email protected]>
---
drivers/platform/surface/aggregator/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig
index c114f9dd5fe1..88afc38ffdc5 100644
--- a/drivers/platform/surface/aggregator/Kconfig
+++ b/drivers/platform/surface/aggregator/Kconfig
@@ -4,7 +4,7 @@
menuconfig SURFACE_AGGREGATOR
tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers"
depends on SERIAL_DEV_BUS
- depends on ACPI
+ depends on ACPI && !RISCV
select CRC_CCITT
help
The Surface System Aggregator Module (Surface SAM or SSAM) is an
--
2.34.1
On Tue, Apr 04, 2023 at 11:50:14PM +0530, Sunil V L wrote:
> Changes since V3:
> 1) Added two more driver patches to workaround allmodconfig build failure.
btw, you need to fix the issues *before* you enable ACPI, not after.
On Tue, Apr 04, 2023 at 11:50:28PM +0530, Sunil V L wrote:
> On ACPI based platforms, few details like ISA need to be read
> from the ACPI table. Enable cpuinfo on ACPI based systems.
>
> ACPI has nothing similar to DT compatible property for each CPU.
> SBI calls must be used to get vendor/arch/imp ID for any errata.
ecalls are used on DT systems for this too FYI, vendorid/archid/impid
are not contained in the DT.
Otherwise,
Reviewed-by: Conor Dooley <[email protected]>
Should probably have given that conditionally last time, sorry about
that.
Thanks,
Conor.
>
> Signed-off-by: Sunil V L <[email protected]>
> Acked-by: Rafael J. Wysocki <[email protected]>
> Reviewed-by: Andrew Jones <[email protected]>
> ---
> arch/riscv/kernel/cpu.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 8400f0cc9704..ace4752516d8 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -3,10 +3,12 @@
> * Copyright (C) 2012 Regents of the University of California
> */
>
> +#include <linux/acpi.h>
> #include <linux/cpu.h>
> #include <linux/init.h>
> #include <linux/seq_file.h>
> #include <linux/of.h>
> +#include <asm/acpi.h>
> #include <asm/csr.h>
> #include <asm/hwcap.h>
> #include <asm/sbi.h>
> @@ -283,23 +285,35 @@ static void c_stop(struct seq_file *m, void *v)
> static int c_show(struct seq_file *m, void *v)
> {
> unsigned long cpu_id = (unsigned long)v - 1;
> - struct device_node *node = of_get_cpu_node(cpu_id, NULL);
> struct riscv_cpuinfo *ci = per_cpu_ptr(&riscv_cpuinfo, cpu_id);
> + struct device_node *node;
> const char *compat, *isa;
>
> seq_printf(m, "processor\t: %lu\n", cpu_id);
> seq_printf(m, "hart\t\t: %lu\n", cpuid_to_hartid_map(cpu_id));
> - if (!of_property_read_string(node, "riscv,isa", &isa))
> - print_isa(m, isa);
> - print_mmu(m);
> - if (!of_property_read_string(node, "compatible", &compat)
> - && strcmp(compat, "riscv"))
> - seq_printf(m, "uarch\t\t: %s\n", compat);
> +
> + if (acpi_disabled) {
> + node = of_get_cpu_node(cpu_id, NULL);
> + if (!of_property_read_string(node, "riscv,isa", &isa))
> + print_isa(m, isa);
> +
> + print_mmu(m);
> + if (!of_property_read_string(node, "compatible", &compat) &&
> + strcmp(compat, "riscv"))
> + seq_printf(m, "uarch\t\t: %s\n", compat);
> +
> + of_node_put(node);
> + } else {
> + if (!acpi_get_riscv_isa(NULL, cpu_id, &isa))
> + print_isa(m, isa);
> +
> + print_mmu(m);
> + }
> +
> seq_printf(m, "mvendorid\t: 0x%lx\n", ci->mvendorid);
> seq_printf(m, "marchid\t\t: 0x%lx\n", ci->marchid);
> seq_printf(m, "mimpid\t\t: 0x%lx\n", ci->mimpid);
> seq_puts(m, "\n");
> - of_node_put(node);
>
> return 0;
> }
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Apr 04, 2023 at 11:50:34PM +0530, Sunil V L wrote:
> Add support to build ACPI subsystem in defconfig.
>
> Signed-off-by: Sunil V L <[email protected]>
> Acked-by: Rafael J. Wysocki <[email protected]>
> Reviewed-by: Andrew Jones <[email protected]>
Dropped another R-b?
https://lore.kernel.org/linux-riscv/91cf4ebd-f22c-4cf9-9fb4-fa6349ea00ab@spud/
That said...
> ---
> arch/riscv/configs/defconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> index d98d6e90b2b8..8822b49ddb59 100644
> --- a/arch/riscv/configs/defconfig
> +++ b/arch/riscv/configs/defconfig
> @@ -238,3 +238,4 @@ CONFIG_RCU_EQS_DEBUG=y
> # CONFIG_FTRACE is not set
> # CONFIG_RUNTIME_TESTING_MENU is not set
> CONFIG_MEMTEST=y
> +CONFIG_ACPI=y
...this is not where savedefconfig puts this for me.
Please move it there & then:
Reviewed-by: Conor Dooley <[email protected]>
Thanks,
Conor.
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On 4 Apr 2023, at 19:20, Sunil V L <[email protected]> wrote:
>
> With CONFIG_ACPI enabled for RISC-V, this driver gets enabled
> in allmodconfig build. However, RISC-V doesn't support sub-word
> atomics which is used by this driver.
Why not? Compilers and libatomic do, so surely the Linux kernel should
too.
> Due to this, the build fails
> with below error.
>
> In function ‘ssh_seq_next’,
> inlined from ‘ssam_request_write_data’ at drivers/platform/surface/aggregator/controller.c:1483:8:
> ././include/linux/compiler_types.h:399:45: error: call to ‘__compiletime_assert_335’ declared with attribute error: BUILD_BUG failed
> 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> ./include/linux/compiler.h:78:45: note: in definition of macro ‘unlikely’
> 78 | # define unlikely(x) __builtin_expect(!!(x), 0)
> | ^
> ././include/linux/compiler_types.h:387:9: note: in expansion of macro ‘__compiletime_assert’
> 387 | __compiletime_assert(condition, msg, prefix, suffix)
> | ^~~~~~~~~~~~~~~~~~~~
> ././include/linux/compiler_types.h:399:9: note: in expansion of macro ‘_compiletime_assert’
> 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^~~~~~~~~~~~~~~~~~~
> ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
> 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> | ^~~~~~~~~~~~~~~~~~
> ./include/linux/build_bug.h:59:21: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
> 59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
> | ^~~~~~~~~~~~~~~~
> ./arch/riscv/include/asm/cmpxchg.h:335:17: note: in expansion of macro ‘BUILD_BUG’
> 335 | BUILD_BUG(); \
> | ^~~~~~~~~
> ./arch/riscv/include/asm/cmpxchg.h:344:30: note: in expansion of macro ‘__cmpxchg’
> 344 | (__typeof__(*(ptr))) __cmpxchg((ptr), \
> | ^~~~~~~~~
> ./include/linux/atomic/atomic-instrumented.h:1916:9: note: in expansion of macro ‘arch_cmpxchg’
> 1916 | arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
> | ^~~~~~~~~~~~
> drivers/platform/surface/aggregator/controller.c:61:32: note: in expansion of macro ‘cmpxchg’
> 61 | while (unlikely((ret = cmpxchg(&c->value, old, new)) != old)) {
> | ^~~~~~~
>
> So, disable this driver for RISC-V even when ACPI is enabled for now.
>
> Signed-off-by: Sunil V L <[email protected]>
> ---
> drivers/platform/surface/aggregator/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig
> index c114f9dd5fe1..88afc38ffdc5 100644
> --- a/drivers/platform/surface/aggregator/Kconfig
> +++ b/drivers/platform/surface/aggregator/Kconfig
> @@ -4,7 +4,7 @@
> menuconfig SURFACE_AGGREGATOR
> tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers"
> depends on SERIAL_DEV_BUS
> - depends on ACPI
> + depends on ACPI && !RISCV
If you insist on doing this, at least make it some new config variable
that’s self-documenting and means this automatically gets re-enabled
when arch/riscv fixes this deficiency? Hard-coding arch lists like this
seems like a terrible anti-pattern.
Jess
> select CRC_CCITT
> help
> The Surface System Aggregator Module (Surface SAM or SSAM) is an
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Apr 4, 2023, at 20:20, Sunil V L wrote:
> With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in
> allmodconfig build. The gcc tool chain builds this driver removing the
> inline arm64 assembly code. However, clang for RISC-V tries to build
> the arm64 assembly and below error is seen.
>
> drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint
> '+Q' in asm
> "+Q" (*((char __iomem *)fun_base))
> ^
> It appears that RISC-V clang is not smart enough to detect
> IS_ENABLED(CONFIG_ARM64) and remove the dead code.
>
> As a workaround, move this check to preprocessing stage which works
> with the RISC-V clang tool chain.
>
> Signed-off-by: Sunil V L <[email protected]>
Your patch looks correct for this particular problem, but I
see that there are a couple of other issues in the same function:
> - }
> +#if IS_ENABLED(CONFIG_ARM64)
> + unsigned long tmp0 = 0, tmp1 = 0;
>
> asm volatile("ldp %0, %1, %3\n"
> "stp %0, %1, %2\n"
> @@ -627,6 +623,11 @@ static void qm_mb_write(struct hisi_qm *qm, const
> void *src)
> "+Q" (*((char __iomem *)fun_base))
> : "Q" (*((char *)src))
> : "memory");
For the arm64 version:
- the "dmb oshst" barrier needs to come before the stp, not after
it, otherwise there is no guarantee that data written to memory
is visible by the device when the mailbox gets triggered
- The input/output arguments need to be pointers to 128-bit types,
either a struct or a __uint128_t
- this lacks a byteswap on big-endian kernels
> +#else
> + memcpy_toio(fun_base, src, 16);
> + dma_wmb();
> +#endif
This version has the same problems, plus the write is not actually
atomic. I wonder if a pair of writeq() calls would just do the
right thing here for both arm64 and others, or possibly a
writeq() followed by a writeq_relaxed() to avoid the extra dmb()
in the middle.
Arnd
On 4/4/23 20:20, Sunil V L wrote:
> With CONFIG_ACPI enabled for RISC-V, this driver gets enabled
> in allmodconfig build. However, RISC-V doesn't support sub-word
> atomics which is used by this driver. Due to this, the build fails
> with below error.
>
> In function ‘ssh_seq_next’,
> inlined from ‘ssam_request_write_data’ at drivers/platform/surface/aggregator/controller.c:1483:8:
> ././include/linux/compiler_types.h:399:45: error: call to ‘__compiletime_assert_335’ declared with attribute error: BUILD_BUG failed
> 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^
> ./include/linux/compiler.h:78:45: note: in definition of macro ‘unlikely’
> 78 | # define unlikely(x) __builtin_expect(!!(x), 0)
> | ^
> ././include/linux/compiler_types.h:387:9: note: in expansion of macro ‘__compiletime_assert’
> 387 | __compiletime_assert(condition, msg, prefix, suffix)
> | ^~~~~~~~~~~~~~~~~~~~
> ././include/linux/compiler_types.h:399:9: note: in expansion of macro ‘_compiletime_assert’
> 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> | ^~~~~~~~~~~~~~~~~~~
> ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
> 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> | ^~~~~~~~~~~~~~~~~~
> ./include/linux/build_bug.h:59:21: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
> 59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
> | ^~~~~~~~~~~~~~~~
> ./arch/riscv/include/asm/cmpxchg.h:335:17: note: in expansion of macro ‘BUILD_BUG’
> 335 | BUILD_BUG(); \
> | ^~~~~~~~~
> ./arch/riscv/include/asm/cmpxchg.h:344:30: note: in expansion of macro ‘__cmpxchg’
> 344 | (__typeof__(*(ptr))) __cmpxchg((ptr), \
> | ^~~~~~~~~
> ./include/linux/atomic/atomic-instrumented.h:1916:9: note: in expansion of macro ‘arch_cmpxchg’
> 1916 | arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
> | ^~~~~~~~~~~~
> drivers/platform/surface/aggregator/controller.c:61:32: note: in expansion of macro ‘cmpxchg’
> 61 | while (unlikely((ret = cmpxchg(&c->value, old, new)) != old)) {
> | ^~~~~~~
>
> So, disable this driver for RISC-V even when ACPI is enabled for now.
CONFIG_SURFACE_PLATFORMS should be enabled for ARM64 || X86 || COMPILE_TEST only,
so I guess the issue only happens when compiling with the latter enabled?
I'm not aware of any current plans of MS to release RISC-V-based Surface
devices, so you could maybe also just explicitly disable CONFIG_SURFACE_PLATFORMS.
In any case, I don't see any issues with disabling the whole platform/surface
or only individual drivers for RISC-V, so for either solution:
Acked-by: Maximilian Luz <[email protected]>
> Signed-off-by: Sunil V L <[email protected]>
> ---
> drivers/platform/surface/aggregator/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig
> index c114f9dd5fe1..88afc38ffdc5 100644
> --- a/drivers/platform/surface/aggregator/Kconfig
> +++ b/drivers/platform/surface/aggregator/Kconfig
> @@ -4,7 +4,7 @@
> menuconfig SURFACE_AGGREGATOR
> tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers"
> depends on SERIAL_DEV_BUS
> - depends on ACPI
> + depends on ACPI && !RISCV
> select CRC_CCITT
> help
> The Surface System Aggregator Module (Surface SAM or SSAM) is an
Hi Conor,
On Tue, Apr 04, 2023 at 10:59:41PM +0100, Conor Dooley wrote:
> Hey Sunil,
>
> This one made me scratch my head for a bit..
>
> On Tue, Apr 04, 2023 at 11:50:37PM +0530, Sunil V L wrote:
> > With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in
> > allmodconfig build. The gcc tool chain builds this driver removing the
> > inline arm64 assembly code. However, clang for RISC-V tries to build
> > the arm64 assembly and below error is seen.
>
> There's actually nothing RISC-V specific about that behaviour, that's
> just how clang works. Quoting Nathan:
> "Clang performs semantic analysis (i.e., validates assembly) before
> dead code elimination, so IS_ENABLED() is not sufficient for avoiding
> that error."
>
Huh, It never occurred to me that this issue could be known already since I
always thought we are hitting this first time since ACPI is enabled only
now for RISC-V. Thank you very much!.
> > drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint '+Q' in asm
> > "+Q" (*((char __iomem *)fun_base))
> > ^
> > It appears that RISC-V clang is not smart enough to detect
> > IS_ENABLED(CONFIG_ARM64) and remove the dead code.
>
> So I think this statement is just not true, it can remove dead code, but
> only after it has done the semantic analysis.
>
Yes, with more details now, let me update the commit message.
> The reason that this has not been seen before, again quoting Nathan, is:
> "arm64 and x86_64 both support the Q constraint, we cannot build
> LoongArch yet (although it does not have support for Q either so same
> boat as RISC-V), and ia64 is dead/unsupported in LLVM. Those are the
> only architectures that support ACPI, so I guess that explains why we
> have seen no issues aside from RISC-V so far."
>
> > As a workaround, move this check to preprocessing stage which works
> > with the RISC-V clang tool chain.
>
> I don't think there's much else you can do!
> Reviewed-by: Conor Dooley <[email protected]>
>
> Perhaps it is also worth adding:
> Link: https://github.com/ClangBuiltLinux/linux/issues/999
>
Sure, Thank you very much for digging this!
Thanks,
Sunil
On Tue, Apr 04, 2023 at 10:43:02PM +0100, Conor Dooley wrote:
> On Tue, Apr 04, 2023 at 11:50:34PM +0530, Sunil V L wrote:
> > Add support to build ACPI subsystem in defconfig.
> >
> > Signed-off-by: Sunil V L <[email protected]>
> > Acked-by: Rafael J. Wysocki <[email protected]>
> > Reviewed-by: Andrew Jones <[email protected]>
>
> Dropped another R-b?
> https://lore.kernel.org/linux-riscv/91cf4ebd-f22c-4cf9-9fb4-fa6349ea00ab@spud/
>
Yeah, missed updating....
> That said...
>
> > ---
> > arch/riscv/configs/defconfig | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig
> > index d98d6e90b2b8..8822b49ddb59 100644
> > --- a/arch/riscv/configs/defconfig
> > +++ b/arch/riscv/configs/defconfig
> > @@ -238,3 +238,4 @@ CONFIG_RCU_EQS_DEBUG=y
> > # CONFIG_FTRACE is not set
> > # CONFIG_RUNTIME_TESTING_MENU is not set
> > CONFIG_MEMTEST=y
> > +CONFIG_ACPI=y
>
> ...this is not where savedefconfig puts this for me.
> Please move it there & then:
> Reviewed-by: Conor Dooley <[email protected]>
>
Okay. Will update.
Thanks!
Sunil
Hi Jess,
On Wed, Apr 05, 2023 at 05:19:35AM +0100, Jessica Clarke wrote:
> On 4 Apr 2023, at 19:20, Sunil V L <[email protected]> wrote:
> >
> > With CONFIG_ACPI enabled for RISC-V, this driver gets enabled
> > in allmodconfig build. However, RISC-V doesn't support sub-word
> > atomics which is used by this driver.
>
> Why not? Compilers and libatomic do, so surely the Linux kernel should
> too.
>
I think you are probably right. But I don't want to combine that
activity with this series. IMO, that should be separate activity.
> > Due to this, the build fails
> > with below error.
> >
> > In function ‘ssh_seq_next’,
> > inlined from ‘ssam_request_write_data’ at drivers/platform/surface/aggregator/controller.c:1483:8:
> > ././include/linux/compiler_types.h:399:45: error: call to ‘__compiletime_assert_335’ declared with attribute error: BUILD_BUG failed
> > 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^
> > ./include/linux/compiler.h:78:45: note: in definition of macro ‘unlikely’
> > 78 | # define unlikely(x) __builtin_expect(!!(x), 0)
> > | ^
> > ././include/linux/compiler_types.h:387:9: note: in expansion of macro ‘__compiletime_assert’
> > 387 | __compiletime_assert(condition, msg, prefix, suffix)
> > | ^~~~~~~~~~~~~~~~~~~~
> > ././include/linux/compiler_types.h:399:9: note: in expansion of macro ‘_compiletime_assert’
> > 399 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > | ^~~~~~~~~~~~~~~~~~~
> > ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
> > 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> > | ^~~~~~~~~~~~~~~~~~
> > ./include/linux/build_bug.h:59:21: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
> > 59 | #define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
> > | ^~~~~~~~~~~~~~~~
> > ./arch/riscv/include/asm/cmpxchg.h:335:17: note: in expansion of macro ‘BUILD_BUG’
> > 335 | BUILD_BUG(); \
> > | ^~~~~~~~~
> > ./arch/riscv/include/asm/cmpxchg.h:344:30: note: in expansion of macro ‘__cmpxchg’
> > 344 | (__typeof__(*(ptr))) __cmpxchg((ptr), \
> > | ^~~~~~~~~
> > ./include/linux/atomic/atomic-instrumented.h:1916:9: note: in expansion of macro ‘arch_cmpxchg’
> > 1916 | arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
> > | ^~~~~~~~~~~~
> > drivers/platform/surface/aggregator/controller.c:61:32: note: in expansion of macro ‘cmpxchg’
> > 61 | while (unlikely((ret = cmpxchg(&c->value, old, new)) != old)) {
> > | ^~~~~~~
> >
> > So, disable this driver for RISC-V even when ACPI is enabled for now.
> >
> > Signed-off-by: Sunil V L <[email protected]>
> > ---
> > drivers/platform/surface/aggregator/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/surface/aggregator/Kconfig b/drivers/platform/surface/aggregator/Kconfig
> > index c114f9dd5fe1..88afc38ffdc5 100644
> > --- a/drivers/platform/surface/aggregator/Kconfig
> > +++ b/drivers/platform/surface/aggregator/Kconfig
> > @@ -4,7 +4,7 @@
> > menuconfig SURFACE_AGGREGATOR
> > tristate "Microsoft Surface System Aggregator Module Subsystem and Drivers"
> > depends on SERIAL_DEV_BUS
> > - depends on ACPI
> > + depends on ACPI && !RISCV
>
> If you insist on doing this, at least make it some new config variable
> that’s self-documenting and means this automatically gets re-enabled
> when arch/riscv fixes this deficiency? Hard-coding arch lists like this
> seems like a terrible anti-pattern.
>
I understand your point. But given that this is currently only issue with
a single driver from Microsoft and that too only in COMPILE_TEST builds,
I think introducing a new config variable is overkill. If we support
sub-word atomics in kernel, the option may not be useful much anyway.
There are patterns to disable an architecture for COMPILE_TEST builds.
Thanks,
Sunil
On Tue, Apr 11, 2023, at 13:42, Weili Qian wrote:
> On 2023/4/5 16:16, Arnd Bergmann wrote:
>> On Tue, Apr 4, 2023, at 20:20, Sunil V L wrote:
>>> With CONFIG_ACPI enabled for RISC-V, this driver gets enabled in
>>> allmodconfig build. The gcc tool chain builds this driver removing the
>>> inline arm64 assembly code. However, clang for RISC-V tries to build
>>> the arm64 assembly and below error is seen.
>>>
>>> drivers/crypto/hisilicon/qm.c:627:10: error: invalid output constraint
>>> '+Q' in asm
>>> "+Q" (*((char __iomem *)fun_base))
>>> ^
>>> It appears that RISC-V clang is not smart enough to detect
>>> IS_ENABLED(CONFIG_ARM64) and remove the dead code.
>>>
>>> As a workaround, move this check to preprocessing stage which works
>>> with the RISC-V clang tool chain.
>>>
>>> Signed-off-by: Sunil V L <[email protected]>
>>
>> Your patch looks correct for this particular problem, but I
>> see that there are a couple of other issues in the same function:
>>
>>> - }
>>> +#if IS_ENABLED(CONFIG_ARM64)
>>> + unsigned long tmp0 = 0, tmp1 = 0;
>>>
>>> asm volatile("ldp %0, %1, %3\n"
>>> "stp %0, %1, %2\n"
>>> @@ -627,6 +623,11 @@ static void qm_mb_write(struct hisi_qm *qm, const
>>> void *src)
>>> "+Q" (*((char __iomem *)fun_base))
>>> : "Q" (*((char *)src))
>>> : "memory");
>>
>> For the arm64 version:
>>
>> - the "dmb oshst" barrier needs to come before the stp, not after
>> it, otherwise there is no guarantee that data written to memory
>> is visible by the device when the mailbox gets triggered
>> - The input/output arguments need to be pointers to 128-bit types,
>> either a struct or a __uint128_t
>> - this lacks a byteswap on big-endian kernels
> Sorry for the late reply.
>
> - the execution order relies on the data dependency between ldp and stp:
> load "src" to "tmp0" and "tmp1", then
> store "tmp0" and "tmp1" to "fun_base";
Not entirely sure how that data dependency would help serialize
the store into the DMA buffer against the device access. The problem
here is not the qm_mailbox structure but the data pointed to by the
'u64 base' (e.g. struct qm_eqc *eqc) which may still be in a store
buffer waiting to make it to physical memory at the time the mailbox
store triggers the DMA from the device.
> The "dmb oshst" is used to ensure that the stp instruction has been executed
> before CPU checking mailbox status. Whether the execution order
> cannot be guaranteed via data dependency?
There is no need to have barriers between MMIO operations, they
are implicitly serialized already. In this case specifically,
the read is even on the same address as the write. Note that the
"dmb oshst" does not actually guarantee that the store has made it
to the device, as (at least on PCIe semantics) it can be posted,
but the read from the same address does guarantee that the write
is completed first, and this may be required to ensure that it does
not complete after the mutex_unlock().
> - The input argument "src" is struct "struct qm_mailbox".
> - Before call this funcion, the data has been byteswapped.
>
> mailbox->w0 = cpu_to_le16((cmd) |
> ((op) ? 0x1 << QM_MB_OP_SHIFT : 0) |
> (0x1 << QM_MB_BUSY_SHIFT));
> mailbox->queue_num = cpu_to_le16(queue);
> mailbox->base_l = cpu_to_le32(lower_32_bits(base));
> mailbox->base_h = cpu_to_le32(upper_32_bits(base));
> mailbox->rsvd = 0;
Right, this bit does look correct.
Arnd
Sunil V L <[email protected]> writes:
> Without this, if the tables are larger than 4K,
> acpi_map() will fail.
>
> Signed-off-by: Sunil V L <[email protected]>
> Acked-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/osl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 3269a888fb7a..f725813d0cce 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -276,7 +276,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> return NULL;
> }
>
> -#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)
> +#if defined(CONFIG_IA64) || defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
> /* ioremap will take care of cache attributes */
> #define should_use_kmap(pfn) 0
An observation, which can be addressed later; The acpi_os_ioremap()
(called when the config above is enabled for RV), does not have an arch
specific implementation for RISC-V. The generic one calls
ioremap_cached(), which on RISC-V defaults to ioremap() -- caching
disabled/_PAGE_IO.
That is probably not what we want, but rather something similar that
arm64 does.
Björn
Hi Bjorn,
On Wed, Apr 26, 2023 at 06:47:20PM +0200, Björn Töpel wrote:
> Sunil V L <[email protected]> writes:
>
> > Without this, if the tables are larger than 4K,
> > acpi_map() will fail.
> >
> > Signed-off-by: Sunil V L <[email protected]>
> > Acked-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/acpi/osl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 3269a888fb7a..f725813d0cce 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -276,7 +276,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
> > return NULL;
> > }
> >
> > -#if defined(CONFIG_IA64) || defined(CONFIG_ARM64)
> > +#if defined(CONFIG_IA64) || defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
> > /* ioremap will take care of cache attributes */
> > #define should_use_kmap(pfn) 0
>
> An observation, which can be addressed later; The acpi_os_ioremap()
> (called when the config above is enabled for RV), does not have an arch
> specific implementation for RISC-V. The generic one calls
> ioremap_cached(), which on RISC-V defaults to ioremap() -- caching
> disabled/_PAGE_IO.
>
> That is probably not what we want, but rather something similar that
> arm64 does.
>
Actually, for RISC-V acpi_os_ioremap() is currently a wrapper around
memremap(). Sure, this can be enhanced in future if required.
Thanks,
Sunil
Sunil V L <[email protected]> writes:
>> An observation, which can be addressed later; The acpi_os_ioremap()
>> (called when the config above is enabled for RV), does not have an arch
>> specific implementation for RISC-V. The generic one calls
>> ioremap_cached(), which on RISC-V defaults to ioremap() -- caching
>> disabled/_PAGE_IO.
>>
>> That is probably not what we want, but rather something similar that
>> arm64 does.
>>
> Actually, for RISC-V acpi_os_ioremap() is currently a wrapper around
> memremap(). Sure, this can be enhanced in future if required.
Yeah, realized that when I continued thru the series!
Thanks for clearing it up!