RISC-V Linux Port v9

This patch adds device tree bindings for RISC-V CPUs, patterned after the ARM device tree CPU bindings.

+- cpu node

Documentation/devicetree/bindings/riscv/cpus.txt new file +8 -0

+
+        Description: Describes a hart context
+
+        PROPERTIES
+
+        - device_type
+                Usage: required
+                Value type: <string>

Documentation/devicetree/bindings/riscv/cpus.txt new file +6 -0

+Example: Spike ISA Simulator with 1 Hart
+----------------------------------------
+
+This device tree matches the Spike ISA golden model as run with `spike -p1`.
+
+        cpus {

I would *strongly* recommend that from day one, you determine the SMP bringup mechanism via an enable-method property, and document the contract with FW/bootloader somewhere in the kernel tree.

Hi Palmer,

On Thu, Oct 05, 2017 at 11:16:33AM +0100, Mark Rutland wrote:
[...]

Quoted 3 lines

I would *strongly* recommend that from day one, you determine the SMP bringup mechanism via an enable-method property, and document the contract with FW/bootloader somewhere in the kernel tree.

Somewhat, but not quite related: Please consider making the availability of the Supervisor Binary Interface explicit in the devicetree. I understand that the general plan is to make the SBI a mandatory feature of every RISC-V system capable of running Linux, but I do want to explore the possibility of running without run-time resident firmware at some point in the future. Thus it would be nice if the devicetree would indicate the presence of the SBI from the start, to avoid having to invent a way to express its *absence* later on.

It could look something like this (modelled after qcom,scm):

/ {
	firmware {
		sbi {
			compatible = "riscv,sbi";
		};
	};
};

This topic may warrant some discussion, because other people may have different opinions, and there hasn't been a discussion about it, AFAICS.

arch/riscv/kernel/cacheinfo.c new file +5 -0

+static void ci_leaf_init(struct cacheinfo *this_leaf,
+			 struct device_node *node,
+			 enum cache_type type, unsigned int level)
+{
+	this_leaf->of_node = node;

arch/riscv/kernel/cpu.c new file +7 -0

+{
+	const char *isa, *status;
+	u32 hart;
+
+	if (!of_device_is_compatible(node, "riscv")) {
+		pr_warn("Found incompatible CPU\n");
+		return -(ENODEV);

arch/riscv/kernel/cpu.c new file +8 -0

+static int c_show(struct seq_file *m, void *v)
+{
+	unsigned long hart_id = (unsigned long)v - 1;
+	struct device_node *node = of_get_cpu_node(hart_id, NULL);
+	const char *compat, *isa, *mmu;
+
+	seq_printf(m, "hart\t: %lu\n", hart_id);
+	if (!of_property_read_string(node, "riscv,isa", &isa)

arch/riscv/kernel/head.S new file +12 -0

+ENTRY(_start)
+	/* Mask all interrupts */
+	csrw sie, zero
+
+	/* Load the global pointer */
+.option push
+.option norelax
+	la gp, __global_pointer$
+.option pop
+
+	/*
+	 * Disable FPU to detect illegal usage of

+/*
+ * Allow the user to manually add a memory region (in case DTS is broken);
+ * "mem_end=nn[KkMmGg]"
+ */

arch/riscv/kernel/smpboot.c new file +13 -0

+void __init smp_prepare_cpus(unsigned int max_cpus)
+{
+}
+
+void __init setup_smp(void)
+{
+	struct device_node *dn = NULL;
+	int hart, im_okay_therefore_i_am = 0;
+
+	while ((dn = of_find_node_by_type(dn, "cpu"))) {
+		hart = riscv_of_processor_hart(dn);
+		if (hart >= 0) {
+			set_cpu_possible(hart, true);

arch/riscv/kernel/smpboot.c new file +5 -0

+
+int __cpu_up(unsigned int cpu, struct task_struct *tidle)
+{
+	tidle->thread_info.cpu = cpu;
+

arch/riscv/include/asm/smp.h line 45

Quoted 7 lines · Palmer Dabbelt

arch/riscv/include/asm/smp.h new file +7 -0

+/*
+ * This is particularly ugly: it appears we can't actually get the definition
+ * of task_struct here, but we need access to the CPU this task is running on.
+ * Instead of using C we're using asm-offsets.h to get the current processor
+ * ID.
+ */
+#define raw_smp_processor_id() (*((int*)((char*)get_current() + TASK_TI_CPU)))

Are you using CONFIG_THREAD_INFO_IN_TASK?

I assume so, becauase 'TI' usually stands for thread_info.

So if you can include your <asm/thread_info.h> here, I think you can do something like:

#define get_ti() ((struct thread_info *)get_current())
#define raw_smp_processor_id() (get_ti()->cpu)

Quoted 10 lines

arch/riscv/kernel/cacheinfo.c new file +10 -0

+static void ci_leaf_init(struct cacheinfo *this_leaf,
+			 struct device_node *node,
+			 enum cache_type type, unsigned int level)
+{
+	this_leaf->of_node = node;
+	this_leaf->level = level;
+	this_leaf->type = type;
+	/* not a sector cache */
+	this_leaf->physical_line_partition = 1;
+	/* TODO: Add to DTS */

arch/riscv/kernel/cacheinfo.c line 33

Quoted 5 lines

arch/riscv/kernel/cacheinfo.c new file +5 -0

+	this_leaf->attributes =
+		CACHE_WRITE_BACK
+		| CACHE_READ_ALLOCATE
+		| CACHE_WRITE_ALLOCATE;
+}

In practice, it's very hard to remove implicit assumptions later down the line, so I'd recommend you add this to your DT from the outset.

arch/riscv/kernel/cpu.c line 19

Quoted 2 lines

arch/riscv/kernel/cpu.c new file +2 -0

+/* Return -1 if not a valid hart */
+int riscv_of_processor_hart(struct device_node *node)

Perhaps s/hart/hart_id/ for the function name? Otherwise, it's not immediately obvious what it is doing.

Quoted 7 lines

arch/riscv/kernel/cpu.c new file +7 -0

+{
+	const char *isa, *status;
+	u32 hart;
+
+	if (!of_device_is_compatible(node, "riscv")) {
+		pr_warn("Found incompatible CPU\n");
+		return -(ENODEV);

arch/riscv/kernel/cpu.c line 36

Quoted 10 lines

arch/riscv/kernel/cpu.c new file +10 -0

+	}
+
+	if (of_property_read_u32(node, "reg", &hart)) {
+		pr_warn("Found CPU without hart ID\n");
+		return -(ENODEV);
+	}
+	if (hart >= NR_CPUS) {
+		pr_info("Found hart ID %d, which is above NR_CPUs.  Disabling this hart\n", hart);
+		return -(ENODEV);
+	}

Here you assume that the hard ID is equalt to the Linux logical ID.

I'd recommend that (as other architectures do), you decouple the two.

That's going to be necessary for things like kdump to work, as the kdump kernel will run on (Linux logical) CPU0, but may have been executed from any secondary CPU.

arch/riscv/kernel/cpu.c line 45

Quoted 9 lines

arch/riscv/kernel/cpu.c new file +9 -0

+
+	if (of_property_read_string(node, "status", &status)) {
+		pr_warn("CPU with hartid=%d has no \"status\" property\n", hart);
+		return -(ENODEV);
+	}
+	if (strcmp(status, "okay")) {
+		pr_info("CPU with hartid=%d has a non-okay status of \"%s\"\n", hart, status);
+		return -(ENODEV);
+	}

Please use of_device_is_available().

arch/riscv/kernel/cpu.c line 54

Quoted 9 lines

arch/riscv/kernel/cpu.c new file +9 -0

+
+	if (of_property_read_string(node, "riscv,isa", &isa)) {
+		pr_warn("CPU with hartid=%d has no \"riscv,isa\" property\n", hart);
+		return -(ENODEV);
+	}
+	if (isa[0] != 'r' || isa[1] != 'v') {
+		pr_warn("CPU with hartid=%d has an invalid ISA of \"%s\"\n", hart, isa);
+		return -(ENODEV);
+	}

As mentioned on the binding patch, I'd recommend that you use a set of discrete flags.

[...]

Quoted 8 lines

arch/riscv/kernel/cpu.c new file +8 -0

+static int c_show(struct seq_file *m, void *v)
+{
+	unsigned long hart_id = (unsigned long)v - 1;
+	struct device_node *node = of_get_cpu_node(hart_id, NULL);
+	const char *compat, *isa, *mmu;
+
+	seq_printf(m, "hart\t: %lu\n", hart_id);
+	if (!of_property_read_string(node, "riscv,isa", &isa)

arch/riscv/kernel/cpu.c line 96

Quoted 10 lines

arch/riscv/kernel/cpu.c new file +10 -0

+	    && isa[0] == 'r'
+	    && isa[1] == 'v')
+		seq_printf(m, "isa\t: %s\n", isa);
+	if (!of_property_read_string(node, "mmu-type", &mmu)
+	    && !strncmp(mmu, "riscv,", 6))
+		seq_printf(m, "mmu\t: %s\n", mmu+6);
+	if (!of_property_read_string(node, "compatible", &compat)
+	    && strcmp(compat, "riscv"))
+		seq_printf(m, "uarch\t: %s\n", compat);
+	seq_puts(m, "\n");

I'd *very strongly* recommend that you don't pass the DT values straight through to userspace.

Otherwise, things like riscv,isa = "rvtypo'd\nsomething" get exposed, and inevitably, some subset of userspace ends up relying upon it, while others break based upon it.

Further, specifically for the ISA parts, this is a very bad idea. If some ISA extensions require kernel support (e.g. to context switch state, or disable some configurable traps), they cannot work without kernel support. Generally, the wayt to handle this is for the kernel to detect the features at boot time, and save these in hwcaps.

Later, the hwcaps can be used to generate /proc/cpunifo output, and are also exposed via auxv, which avoids applications and libraries having to parse /proc/cpuinfo just to determine whether they have a particular feature, which doesn't work in many situations (e.g. early boot where /proc may not be mounted).

Take a look at arm64's hwcaps for an example of how to use hwcaps.

[...]

I deleted the context from my draft, but I didn't spot a header sta the start of your head.S. I would very strongly recommend that you have a header with magic, image size (including unpopulated BSS), and other room for expansion, so that bootloaders can figure out how to load your kernel correctly.

[...]

Quoted 12 lines

arch/riscv/kernel/head.S new file +12 -0

+ENTRY(_start)
+	/* Mask all interrupts */
+	csrw sie, zero
+
+	/* Load the global pointer */
+.option push
+.option norelax
+	la gp, __global_pointer$
+.option pop
+
+	/*
+	 * Disable FPU to detect illegal usage of

arch/riscv/kernel/head.S line 45

Quoted 10 lines

arch/riscv/kernel/head.S new file +10 -0

+	 * floating point in kernel space
+	 */
+	li t0, SR_FS
+	csrc sstatus, t0
+
+	/* Pick one hart to run the main boot sequence */
+	la a3, hart_lottery
+	li a2, 1
+	amoadd.w a3, a2, (a3)
+	bnez a3, .Lsecondary_start

I would *very strongly* recommend that you do not throw all CPUs into the kernel at the start of time, and instead have a mechanism where you can bring CPUs into the kernel individually.

While it's simple, it comes with a number of long term problems (e.g. for kexec/kdump), some of which are unsolvable (e.g. as you cannot know how many CPUs are stuck in the kernel text, which haven't been brought online).

If you want a simple SMP bringup mechanism, ePAPR's spin-table (and arm64's different-but-identically-named spin-table) are good starting points. Just make sure you require each CPU is brought online individually. We didn't do that for arm64 from day one, and it still prevents us from making improvements that we'd like to, years later.

Doing that will also simplify decoupling the HART IDs from linux logical IDs, necessary for kdump/kexec, etc.

[...]

Quoted 4 lines

+/*
+ * Allow the user to manually add a memory region (in case DTS is broken);
+ * "mem_end=nn[KkMmGg]"
+ */

This doesn't have a start, so it can't work for discontiguous memory blocks (e.g. if FW has reserved/carved-out a chunk in the middle).

Other architectures have "mem=" which would allow you to specify a region. I'd generally recommend you provide an incentive to fix dts, however.

Quoted 5 lines

arch/riscv/kernel/setup.c new file +5 -0

+void __init setup_arch(char **cmdline_p)
+{
+#if defined(CONFIG_HVC_RISCV_SBI)
+	if (likely(early_console == NULL)) {
+		early_console = &riscv_sbi_early_console_dev;

arch/riscv/kernel/setup.c line 215

Quoted 3 lines

arch/riscv/kernel/setup.c new file +3 -0

+		register_console(early_console);
+	}
+#endif

This would be better driven by the DT or command line. Otherwise, this may come back to bite you in future, as you have to come up with a new mechanism to describe absence of the console, rather than using an exsiting mechanism to describe its presence.

[...]

Quoted 17 lines

arch/riscv/kernel/smpboot.c new file +17 -0

+void __init smp_prepare_cpus(unsigned int max_cpus)
+{
+}
+
+void __init setup_smp(void)
+{
+	struct device_node *dn = NULL;
+	int hart, im_okay_therefore_i_am = 0;
+
+	while ((dn = of_find_node_by_type(dn, "cpu"))) {
+		hart = riscv_of_processor_hart(dn);
+		if (hart >= 0) {
+			set_cpu_possible(hart, true);
+			set_cpu_present(hart, true);
+			if (hart == smp_processor_id()) {
+				BUG_ON(im_okay_therefore_i_am);
+				im_okay_therefore_i_am = 1;

arch/riscv/kernel/smpboot.c line 68

Quoted 6 lines

arch/riscv/kernel/smpboot.c new file +6 -0

+			}
+		}
+	}
+
+	BUG_ON(!im_okay_therefore_i_am);
+}

That name is somewhat opaque. Perhaps bool found_boot_cpu?

Quoted 5 lines

arch/riscv/kernel/smpboot.c new file +5 -0

+
+int __cpu_up(unsigned int cpu, struct task_struct *tidle)
+{
+	tidle->thread_info.cpu = cpu;
+

arch/riscv/kernel/smpboot.c line 83

Quoted 10 lines

arch/riscv/kernel/smpboot.c new file +10 -0

+	/*
+	 * On RISC-V systems, all harts boot on their own accord.  Our _start
+	 * selects the first hart to boot the kernel and causes the remainder
+	 * of the harts to spin in a loop waiting for their stack pointer to be
+	 * setup by that main hart.  Writing __cpu_up_stack_pointer signals to
+	 * the spinning harts that they can continue the boot process.
+	 */
+	smp_mb();
+	__cpu_up_stack_pointer[cpu] = task_stack_page(tidle) + THREAD_SIZE;
+	__cpu_up_task_pointer[cpu] = tidle;

Shouldn't these be WRITE_ONCE() (or some atomic) to avoid tearing?

How are the secondaries reading these? Is there any requirement on the order these become visible?

[...]

arch/riscv/kernel/smpboot.c line 103

Quoted 9 lines

arch/riscv/kernel/smpboot.c new file +9 -0

+/*
+ * C entry point for a secondary processor.
+ */
+asmlinkage void __init smp_callin(void)
+{
+	struct mm_struct *mm = &init_mm;
+
+	/* All kernel threads share the same mm context.  */
+	atomic_inc(&mm->mm_count);

I beleive this should be:

	mmgrab(mm);

See commit f1f1007644ffc805 ("mm: add new mmgrab() helper")

arch/riscv/kernel/smpboot.c line 110

Quoted 7 lines

arch/riscv/kernel/smpboot.c new file +7 -0

+	current->active_mm = mm;
+
+	trap_init();
+	init_clockevent();
+	notify_cpu_starting(smp_processor_id());
+	set_cpu_online(smp_processor_id(), 1);
+	local_flush_tlb_all();

Why do you need a TLB flush here?

arch/riscv/kernel/smpboot.c line 112

Quoted 2 lines

arch/riscv/kernel/smpboot.c new file +2 -0

+	local_irq_enable();
+	preempt_disable();

I believe these two should be the other way around, if the preempt_disable() is necessary.

Sorry it took me an absurd amount of time to get back around to this... I'm adding our mailing list, as we didn't even have one a year ago.

I've included a few patches in line here, but they're just drafts. I'll test them and submit them properly.

Quoted 10 lines

Quoted 10 lines

arch/riscv/kernel/cacheinfo.c new file +10 -0

+static void ci_leaf_init(struct cacheinfo *this_leaf,
+			 struct device_node *node,
+			 enum cache_type type, unsigned int level)
+{
+	this_leaf->of_node = node;
+	this_leaf->level = level;
+	this_leaf->type = type;
+	/* not a sector cache */
+	this_leaf->physical_line_partition = 1;
+	/* TODO: Add to DTS */

Quoted 8 lines

arch/riscv/kernel/cacheinfo.c line 33

Quoted 5 lines

arch/riscv/kernel/cacheinfo.c new file +5 -0

+	this_leaf->attributes =
+		CACHE_WRITE_BACK
+		| CACHE_READ_ALLOCATE
+		| CACHE_WRITE_ALLOCATE;
+}

In practice, it's very hard to remove implicit assumptions later down the line, so I'd recommend you add this to your DT from the outset.

Are these actually used by anything? I poked around the kernel and it appears that pretty much all they do is get printed in drivers/base/cacheinfo.c. It looks like we're only filling out half the info in there anyway so it feels like we'd be better off just deleting these and waiting until we have a proper scheme before setting anything.

Quoted 20 lines

Quoted 7 lines

arch/riscv/kernel/cpu.c new file +7 -0

+{
+	const char *isa, *status;
+	u32 hart;
+
+	if (!of_device_is_compatible(node, "riscv")) {
+		pr_warn("Found incompatible CPU\n");
+		return -(ENODEV);

arch/riscv/kernel/cpu.c line 36

Quoted 10 lines

arch/riscv/kernel/cpu.c new file +10 -0

+	}
+
+	if (of_property_read_u32(node, "reg", &hart)) {
+		pr_warn("Found CPU without hart ID\n");
+		return -(ENODEV);
+	}
+	if (hart >= NR_CPUS) {
+		pr_info("Found hart ID %d, which is above NR_CPUs.  Disabling this hart\n", hart);
+		return -(ENODEV);
+	}

Here you assume that the hard ID is equalt to the Linux logical ID.

Quoted 5 lines

I'd recommend that (as other architectures do), you decouple the two.

That's going to be necessary for things like kdump to work, as the kdump kernel will run on (Linux logical) CPU0, but may have been executed from any secondary CPU.

This is a good point. We currently have another headache with CPU numbering on some systems where the machine CPU ID of 0 actually isn't a Linux CPU (ie, it doesn't have virtual memory) and we're handling the remapping in the bootloader. That's a bit convoluted, so it might be easier to handle the remapping in Linux.

Quoted 12 lines

arch/riscv/kernel/cpu.c line 45

Quoted 9 lines

arch/riscv/kernel/cpu.c new file +9 -0

+
+	if (of_property_read_string(node, "status", &status)) {
+		pr_warn("CPU with hartid=%d has no \"status\" property\n", hart);
+		return -(ENODEV);
+	}
+	if (strcmp(status, "okay")) {
+		pr_info("CPU with hartid=%d has a non-okay status of \"%s\"\n", hart, status);
+		return -(ENODEV);
+	}

Please use of_device_is_available().

Quoted 12 lines

arch/riscv/kernel/cpu.c line 54

Quoted 9 lines

arch/riscv/kernel/cpu.c new file +9 -0

+
+	if (of_property_read_string(node, "riscv,isa", &isa)) {
+		pr_warn("CPU with hartid=%d has no \"riscv,isa\" property\n", hart);
+		return -(ENODEV);
+	}
+	if (isa[0] != 'r' || isa[1] != 'v') {
+		pr_warn("CPU with hartid=%d has an invalid ISA of \"%s\"\n", hart, isa);
+		return -(ENODEV);
+	}

As mentioned on the binding patch, I'd recommend that you use a set of discrete flags.

RISC-V defines what the ISA string means, and it's got all sorts of mechanism for extensibility already defined. I'm trying to avoid inventing any other encodings of this as they're just going to be incompatible in some subtle way.

Quoted 35 lines

[...]

Quoted 8 lines

arch/riscv/kernel/cpu.c new file +8 -0

+static int c_show(struct seq_file *m, void *v)
+{
+	unsigned long hart_id = (unsigned long)v - 1;
+	struct device_node *node = of_get_cpu_node(hart_id, NULL);
+	const char *compat, *isa, *mmu;
+
+	seq_printf(m, "hart\t: %lu\n", hart_id);
+	if (!of_property_read_string(node, "riscv,isa", &isa)

arch/riscv/kernel/cpu.c line 96

Quoted 10 lines

arch/riscv/kernel/cpu.c new file +10 -0

+	    && isa[0] == 'r'
+	    && isa[1] == 'v')
+		seq_printf(m, "isa\t: %s\n", isa);
+	if (!of_property_read_string(node, "mmu-type", &mmu)
+	    && !strncmp(mmu, "riscv,", 6))
+		seq_printf(m, "mmu\t: %s\n", mmu+6);
+	if (!of_property_read_string(node, "compatible", &compat)
+	    && strcmp(compat, "riscv"))
+		seq_printf(m, "uarch\t: %s\n", compat);
+	seq_puts(m, "\n");

I'd *very strongly* recommend that you don't pass the DT values straight through to userspace.

Otherwise, things like riscv,isa = "rvtypo'd\nsomething" get exposed, and inevitably, some subset of userspace ends up relying upon it, while others break based upon it.

Further, specifically for the ISA parts, this is a very bad idea. If some ISA extensions require kernel support (e.g. to context switch state, or disable some configurable traps), they cannot work without kernel support. Generally, the wayt to handle this is for the kernel to detect the features at boot time, and save these in hwcaps.

Quoted 7 lines

Later, the hwcaps can be used to generate /proc/cpunifo output, and are also exposed via auxv, which avoids applications and libraries having to parse /proc/cpuinfo just to determine whether they have a particular feature, which doesn't work in many situations (e.g. early boot where /proc may not be mounted).

Take a look at arm64's hwcaps for an example of how to use hwcaps.

Yes, that's a very good point. We actually have an implementation of hwcaps (that's exposed via the aux vector) that's cropped up in the mean time, and while the right thing to do is probably to generate cpuinfo from that I do at least have a simple patch that allows us to avoid passing garbage to userspace:

commit 6dbfebe5b2ddd873283abf625556b99f98cc85f2
gpg: Signature made Fri 27 Jul 2018 10:40:02 AM PDT
gpg: using RSA key 00CE76D1834960DFCE886DF8EF4CA1502CCBAB41
gpg: issuer "[email protected]"
gpg: Good signature from "Palmer Dabbelt <[email protected]>" [ultimate]
gpg: aka "Palmer Dabbelt <[email protected]>" [ultimate]
Author: Palmer Dabbelt <[email protected]>
Date: Fri Jul 27 09:57:29 2018 -0700

RISC-V: Filter ISA and MMU values in cpuinfo

We shouldn't be directly passing device tree values to userspace, both because there could be mistakes in device trees and because the kernel doesn't support arbitrary ISAs.

Signed-off-by Palmer Dabbelt <[email protected]>

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c index 19e98c1710dd..a18b4e3962a1 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -58,6 +58,57 @@ int riscv_of_processor_hartid(struct device_node *node)

#ifdef CONFIG_PROC_FS

+static void print_isa(struct seq_file *f, const char *orig_isa) +{
+ static const char *ext = "mafdc";
+ const char *isa = orig_isa;
+ const char *e;
+
+ /* Linux doesn't support rv32e or rv128i, and we only support booting
+ * kernels on harts with the same ISA that the kernel is compiled for. */
+#if defined(CONFIG_32BIT)
+ if (strncmp(isa, "rv32i", 5) != 0)
+ return;
+#elif defined(CONFIG_64BIT)
+ if (strncmp(isa, "rv64i", 5) != 0)
+ return;
+#endif
+
+ /* Print the base ISA, as we already know it's legal. */
+ seq_printf(f, "isa\t: ");
+ seq_write(f, isa, 5);
+ isa += 5;
+
+ /* Check the rest of the ISA string for valid extensions, printing those we
+ * find. RISC-V ISA strings define an order, so we only print the
+ * extension bits when they're in order. */
+ for (e = ext; *e != '\0'; ++e) {
+ if (isa[0] == e[0]) {
+ seq_write(f, isa, 1);

+ isa++;
+ }
+ }
+
+ /* If we were given an unsupported ISA in the device tree then print a bit
+ * of info describing what went wrong. */
+ if (isa[0] != '\0')
+ pr_info("unsupported ISA \"%s\" in device tree", orig_isa);

Quoted 32 lines

Quoted 12 lines

arch/riscv/kernel/head.S new file +12 -0

+ENTRY(_start)
+	/* Mask all interrupts */
+	csrw sie, zero
+
+	/* Load the global pointer */
+.option push
+.option norelax
+	la gp, __global_pointer$
+.option pop
+
+	/*
+	 * Disable FPU to detect illegal usage of

arch/riscv/kernel/head.S line 45

Quoted 10 lines

arch/riscv/kernel/head.S new file +10 -0

+	 * floating point in kernel space
+	 */
+	li t0, SR_FS
+	csrc sstatus, t0
+
+	/* Pick one hart to run the main boot sequence */
+	la a3, hart_lottery
+	li a2, 1
+	amoadd.w a3, a2, (a3)
+	bnez a3, .Lsecondary_start

I would *very strongly* recommend that you do not throw all CPUs into the kernel at the start of time, and instead have a mechanism where you can bring CPUs into the kernel individually.

While it's simple, it comes with a number of long term problems (e.g. for kexec/kdump), some of which are unsolvable (e.g. as you cannot know how many CPUs are stuck in the kernel text, which haven't been brought online).

Quoted 8 lines

If you want a simple SMP bringup mechanism, ePAPR's spin-table (and arm64's different-but-identically-named spin-table) are good starting points. Just make sure you require each CPU is brought online individually. We didn't do that for arm64 from day one, and it still prevents us from making improvements that we'd like to, years later.

Doing that will also simplify decoupling the HART IDs from linux logical IDs, necessary for kdump/kexec, etc.

Defining a proper platform specification is something we've been punting on for too long. It's nominally on my plate, but I'm a bit behind right now. We've been able to get away without one because we don't have real bootloaders yet, but it's really time to get started working on this so we don't end up in a bad situation where we have production hardware out there that is capable of running Linux but is stuck with pre-specification firmware.

Quoted 17 lines

Quoted 4 lines

arch/riscv/kernel/smpboot.c new file +4 -0

+int __cpu_up(unsigned int cpu, struct task_struct *tidle)
+{
+	tidle->thread_info.cpu = cpu;
+

arch/riscv/kernel/smpboot.c line 83

Quoted 10 lines

arch/riscv/kernel/smpboot.c new file +10 -0

+	/*
+	 * On RISC-V systems, all harts boot on their own accord.  Our _start
+	 * selects the first hart to boot the kernel and causes the remainder
+	 * of the harts to spin in a loop waiting for their stack pointer to be
+	 * setup by that main hart.  Writing __cpu_up_stack_pointer signals to
+	 * the spinning harts that they can continue the boot process.
+	 */
+	smp_mb();
+	__cpu_up_stack_pointer[cpu] = task_stack_page(tidle) + THREAD_SIZE;
+	__cpu_up_task_pointer[cpu] = tidle;

Shouldn't these be WRITE_ONCE() (or some atomic) to avoid tearing?

Quoted 2 lines

How are the secondaries reading these? Is there any requirement on the order these become visible?

They just need to be WRITE_ONCE, the secondary harts spin until both of these are non-zero so it's not necessary to have any ordering here. Thanks for pointing this out, though, as this would have been an afwul bug to track down :)

Quoted 13 lines

arch/riscv/kernel/smpboot.c line 103

Quoted 10 lines

arch/riscv/kernel/smpboot.c new file +9 -0

+/*
+ * C entry point for a secondary processor.
+ */
+asmlinkage void __init smp_callin(void)
+{
+	struct mm_struct *mm = &init_mm;
+
+	/* All kernel threads share the same mm context.  */
+	atomic_inc(&mm->mm_count);

I beleive this should be:

Quoted 3 lines

	mmgrab(mm);

See commit f1f1007644ffc805 ("mm: add new mmgrab() helper")

Thanks!

    commit 120b58060bbd67d893552d89b18ebbc7fea66ea9
    gpg: Signature made Fri 27 Jul 2018 03:19:16 PM PDT
    gpg:                using RSA key 00CE76D1834960DFCE886DF8EF4CA1502CCBAB41
    gpg:                issuer "[email protected]"
    gpg: Good signature from "Palmer Dabbelt <[email protected]>" [ultimate]
    gpg:                 aka "Palmer Dabbelt <[email protected]>" [ultimate]
    Author: Palmer Dabbelt <[email protected]>
    Date:   Fri Jul 27 15:16:35 2018 -0700

        RISC-V: Use mmgrab()

        f1f1007644ff ("mm: add new mmgrab() helper") added a helper that we
        missed out on.

        Signed-off-by: Palmer Dabbelt <[email protected]>

    diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
    index 4f18ca5050bc..57552fa3892b 100644
    --- a/arch/riscv/kernel/smpboot.c
    +++ b/arch/riscv/kernel/smpboot.c
    @@ -79,8 +79,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
     	 * the spinning harts that they can continue the boot process.
     	 */
     	smp_mb();
    -	__cpu_up_stack_pointer[cpu] = task_stack_page(tidle) + THREAD_SIZE;
    -	__cpu_up_task_pointer[cpu] = tidle;
    +	WRITE_ONCE(__cpu_up_stack_pointer[cpu], task_stack_page(tidle) + THREAD_SIZE);
    +	WRITE_ONCE(__cpu_up_task_pointer[cpu], tidle);

     	while (!cpu_online(cpu))
     		cpu_relax();
    @@ -100,7 +100,7 @@ asmlinkage void __init smp_callin(void)
     	struct mm_struct *mm = &init_mm;

     	/* All kernel threads share the same mm context.  */
    -	atomic_inc(&mm->mm_count);
    +   mm_grab(mm);
     	current->active_mm = mm;

     	trap_init();

Quoted 9 lines

arch/riscv/kernel/smpboot.c line 110

Quoted 7 lines

arch/riscv/kernel/smpboot.c new file +7 -0

+	current->active_mm = mm;
+
+	trap_init();
+	init_clockevent();
+	notify_cpu_starting(smp_processor_id());
+	set_cpu_online(smp_processor_id(), 1);
+	local_flush_tlb_all();

Why do you need a TLB flush here?

On RISC-V systems there is a mask that specifies which CPUs to target with remote TLB flushes. If the CPU is offline then it will not be included as part of the mask bits set for global TLB flushes, so they're essentially ignored. Thus we do a local flush here in case any other flushes were ignored.

This contains all the code that directly interfaces with the RISC-V memory model. While this code corforms to the current RISC-V ISA specifications (user 2.2 and priv 1.10), the memory model is somewhat underspecified in those documents. There is a working group that hopes to produce a formal memory model by the end of the year, but my understanding is that the basic definitions we're relying on here won't change significantly.

Reviewed-by Arnd Bergmann <[email protected]>

Signed-off-by Palmer Dabbelt <[email protected]>

---
 arch/riscv/include/asm/atomic.h         | 375 ++++++++++++++++++++++++++++++++
 arch/riscv/include/asm/barrier.h        |  68 ++++++
 arch/riscv/include/asm/bitops.h         | 218 +++++++++++++++++++
 arch/riscv/include/asm/cacheflush.h     |  39 ++++
 arch/riscv/include/asm/cmpxchg.h        | 134 ++++++++++++
 arch/riscv/include/asm/io.h             | 303 ++++++++++++++++++++++++++
 arch/riscv/include/asm/spinlock.h       | 165 ++++++++++++++
 arch/riscv/include/asm/spinlock_types.h |  33 +++
 arch/riscv/include/asm/tlb.h            |  24 ++
 arch/riscv/include/asm/tlbflush.h       |  64 ++++++
 10 files changed, 1423 insertions(+)
 create mode 100644 arch/riscv/include/asm/atomic.h
 create mode 100644 arch/riscv/include/asm/barrier.h
 create mode 100644 arch/riscv/include/asm/bitops.h
 create mode 100644 arch/riscv/include/asm/cacheflush.h
 create mode 100644 arch/riscv/include/asm/cmpxchg.h
 create mode 100644 arch/riscv/include/asm/io.h
 create mode 100644 arch/riscv/include/asm/spinlock.h
 create mode 100644 arch/riscv/include/asm/spinlock_types.h
 create mode 100644 arch/riscv/include/asm/tlb.h
 create mode 100644 arch/riscv/include/asm/tlbflush.h

arch/riscv/include/asm/atomic.h new file +66 -0

new file mode 100644
index 000000000000..e2e37c57cbeb
@@ -0,0 +1,375 @@
+/*
+ * Copyright (C) 2007 Red Hat, Inc. All Rights Reserved.
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2017 SiFive
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#ifndef _ASM_RISCV_ATOMIC_H
+#define _ASM_RISCV_ATOMIC_H
+
+#ifdef CONFIG_GENERIC_ATOMIC64
+# include <asm-generic/atomic64.h>
+#else
+# if (__riscv_xlen < 64)
+#  error "64-bit atomics require XLEN to be at least 64"
+# endif
+#endif
+
+#include <asm/cmpxchg.h>
+#include <asm/barrier.h>
+
+#define ATOMIC_INIT(i)	{ (i) }
+static __always_inline int atomic_read(const atomic_t *v)
+{
+	return READ_ONCE(v->counter);
+}
+static __always_inline void atomic_set(atomic_t *v, int i)
+{
+	WRITE_ONCE(v->counter, i);
+}
+
+#ifndef CONFIG_GENERIC_ATOMIC64
+#define ATOMIC64_INIT(i) { (i) }
+static __always_inline long atomic64_read(const atomic64_t *v)
+{
+	return READ_ONCE(v->counter);
+}
+static __always_inline void atomic64_set(atomic64_t *v, long i)
+{
+	WRITE_ONCE(v->counter, i);
+}
+#endif
+
+/*
+ * First, the atomic ops that have no ordering constraints and therefor don't
+ * have the AQ or RL bits set.  These don't return anything, so there's only
+ * one version to worry about.
+ */
+#define ATOMIC_OP(op, asm_op, c_op, I, asm_type, c_type, prefix)				\
+static __always_inline void atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)		\
+{												\
+	__asm__ __volatile__ (									\
+		"amo" #asm_op "." #asm_type " zero, %1, %0"					\
+		: "+A" (v->counter)								\
+		: "r" (I)									\
+		: "memory");									\
+}
+
+#ifdef CONFIG_GENERIC_ATOMIC64
+#define ATOMIC_OPS(op, asm_op, c_op, I)			\
+        ATOMIC_OP (op, asm_op, c_op, I, w,  int,   )
+#else

arch/riscv/include/asm/atomic.h new file +29 -0

+/*
+ * Atomic ops that have ordered, relaxed, acquire, and relese variants.
+ * There's two flavors of these: the arithmatic ops have both fetch and return
+ * versions, while the logical ops only have fetch versions.
+ */
+#define ATOMIC_FETCH_OP(op, asm_op, c_op, I, asm_or, c_or, asm_type, c_type, prefix)			\
+static __always_inline c_type atomic##prefix##_fetch_##op##c_or(c_type i, atomic##prefix##_t *v)	\
+{													\
+	register c_type ret;										\
+	__asm__ __volatile__ (										\
+		"amo" #asm_op "." #asm_type #asm_or " %1, %2, %0"					\
+		: "+A" (v->counter), "=r" (ret)								\
+		: "r" (I)										\
+		: "memory");										\
+	return ret;											\
+}
+
+#define ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_or, c_or, asm_type, c_type, prefix)			\
+static __always_inline c_type atomic##prefix##_##op##_return##c_or(c_type i, atomic##prefix##_t *v)	\
+{													\
+        return atomic##prefix##_fetch_##op##c_or(i, v) c_op I;						\
+}
+
+#ifdef CONFIG_GENERIC_ATOMIC64
+#define ATOMIC_OPS(op, asm_op, c_op, I, asm_or, c_or)				\
+        ATOMIC_FETCH_OP (op, asm_op, c_op, I, asm_or, c_or, w,  int,   )	\
+        ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_or, c_or, w,  int,   )
+#else
+#define ATOMIC_OPS(op, asm_op, c_op, I, asm_or, c_or)				\

arch/riscv/include/asm/atomic.h new file +70 -0

+#define ATOMIC_OP(c_t, prefix, c_or, size, asm_or)						\
+static __always_inline c_t atomic##prefix##_cmpxchg##c_or(atomic##prefix##_t *v, c_t o, c_t n) 	\
+{												\
+	return __cmpxchg(&(v->counter), o, n, size, asm_or, asm_or);				\
+}												\
+static __always_inline c_t atomic##prefix##_xchg##c_or(atomic##prefix##_t *v, c_t n) 		\
+{												\
+	return __xchg(n, &(v->counter), size, asm_or);						\
+}
+
+#ifdef CONFIG_GENERIC_ATOMIC64
+#define ATOMIC_OPS(c_or, asm_or)			\
+	ATOMIC_OP( int,   , c_or, 4, asm_or)
+#else
+#define ATOMIC_OPS(c_or, asm_or)			\
+	ATOMIC_OP( int,   , c_or, 4, asm_or)		\
+	ATOMIC_OP(long, 64, c_or, 8, asm_or)
+#endif
+
+ATOMIC_OPS(        , .aqrl)
+ATOMIC_OPS(_acquire,   .aq)
+ATOMIC_OPS(_release,   .rl)
+ATOMIC_OPS(_relaxed,      )
+
+#undef ATOMIC_OPS
+#undef ATOMIC_OP
+
+static __always_inline int atomic_sub_if_positive(atomic_t *v, int offset)
+{
+       int prev, rc;
+
+	__asm__ __volatile__ (
+		"0:\n\t"
+		"lr.w.aqrl  %[p],  %[c]\n\t"
+		"sub       %[rc],  %[p], %[o]\n\t"
+		"bltz      %[rc],    1f\n\t"
+		"sc.w.aqrl %[rc], %[rc], %[c]\n\t"
+		"bnez      %[rc],    0b\n\t"
+		"1:"
+		: [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter)
+		: [o]"r" (offset)
+		: "memory");
+	return prev - offset;
+}
+
+#define atomic_dec_if_positive(v)	atomic_sub_if_positive(v, 1)
+
+#ifndef CONFIG_GENERIC_ATOMIC64
+static __always_inline long atomic64_sub_if_positive(atomic64_t *v, int offset)
+{
+       long prev, rc;
+
+	__asm__ __volatile__ (
+		"0:\n\t"
+		"lr.d.aqrl  %[p],  %[c]\n\t"
+		"sub       %[rc],  %[p], %[o]\n\t"
+		"bltz      %[rc],    1f\n\t"
+		"sc.d.aqrl %[rc], %[rc], %[c]\n\t"
+		"bnez      %[rc],    0b\n\t"
+		"1:"
+		: [p]"=&r" (prev), [rc]"=&r" (rc), [c]"+A" (v->counter)
+		: [o]"r" (offset)
+		: "memory");
+	return prev - offset;
+}
+
+#define atomic64_dec_if_positive(v)	atomic64_sub_if_positive(v, 1)
+#endif
+
+#endif /* _ASM_RISCV_ATOMIC_H */

arch/riscv/include/asm/barrier.h new file +44 -0

new file mode 100644
index 000000000000..183534b7c39b
@@ -0,0 +1,68 @@
+/*
+ * Based on arch/arm/include/asm/barrier.h
+ *
+ * Copyright (C) 2012 ARM Ltd.
+ * Copyright (C) 2013 Regents of the University of California
+ * Copyright (C) 2017 SiFive
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ASM_RISCV_BARRIER_H
+#define _ASM_RISCV_BARRIER_H
+
+#ifndef __ASSEMBLY__
+
+#define nop()		__asm__ __volatile__ ("nop")
+
+#define RISCV_FENCE(p, s) \
+	__asm__ __volatile__ ("fence " #p "," #s : : : "memory")
+
+/* These barriers need to enforce ordering on both devices or memory. */
+#define mb()		RISCV_FENCE(iorw,iorw)
+#define rmb()		RISCV_FENCE(ir,ir)
+#define wmb()		RISCV_FENCE(ow,ow)
+
+/* These barriers do not need to enforce ordering on devices, just memory. */
+#define smp_mb()	RISCV_FENCE(rw,rw)
+#define smp_rmb()	RISCV_FENCE(r,r)
+#define smp_wmb()	RISCV_FENCE(w,w)
+
+/*
+ * These fences exist to enforce ordering around the relaxed AMOs.  The
+ * documentation defines that
+ * "

arch/riscv/include/asm/barrier.h new file +7 -0

+#define smb_mb__after_spinlock()	smp_mb()
+
+#include <asm-generic/barrier.h>
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_BARRIER_H */

arch/riscv/include/asm/bitops.h new file +50 -0

new file mode 100644
index 000000000000..7c281ef1d583
@@ -0,0 +1,218 @@
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ */
+
+#ifndef _ASM_RISCV_BITOPS_H
+#define _ASM_RISCV_BITOPS_H
+
+#ifndef _LINUX_BITOPS_H
+#error "Only <linux/bitops.h> can be included directly"
+#endif /* _LINUX_BITOPS_H */
+
+#include <linux/compiler.h>
+#include <linux/irqflags.h>
+#include <asm/barrier.h>
+#include <asm/bitsperlong.h>
+
+#ifndef smp_mb__before_clear_bit
+#define smp_mb__before_clear_bit()  smp_mb()
+#define smp_mb__after_clear_bit()   smp_mb()
+#endif /* smp_mb__before_clear_bit */
+
+#include <asm-generic/bitops/__ffs.h>
+#include <asm-generic/bitops/ffz.h>
+#include <asm-generic/bitops/fls.h>
+#include <asm-generic/bitops/__fls.h>
+#include <asm-generic/bitops/fls64.h>
+#include <asm-generic/bitops/find.h>
+#include <asm-generic/bitops/sched.h>
+#include <asm-generic/bitops/ffs.h>
+
+#include <asm-generic/bitops/hweight.h>
+
+#if (BITS_PER_LONG == 64)
+#define __AMO(op)	"amo" #op ".d"
+#elif (BITS_PER_LONG == 32)
+#define __AMO(op)	"amo" #op ".w"
+#else
+#error "Unexpected BITS_PER_LONG"
+#endif
+
+#define __test_and_op_bit_ord(op, mod, nr, addr, ord)		\

arch/riscv/include/asm/bitops.h new file +57 -0

+#ifndef _ASM_RISCV_CMPXCHG_H
+#define _ASM_RISCV_CMPXCHG_H
+
+#include <linux/bug.h>
+
+#include <asm/barrier.h>
+
+#define __xchg(new, ptr, size, asm_or)				\
+({								\
+	__typeof__(ptr) __ptr = (ptr);				\
+	__typeof__(new) __new = (new);				\
+	__typeof__(*(ptr)) __ret;				\
+	switch (size) {						\
+	case 4:							\
+		__asm__ __volatile__ (				\
+			"amoswap.w" #asm_or " %0, %2, %1"	\
+			: "=r" (__ret), "+A" (*__ptr)		\
+			: "r" (__new)				\
+			: "memory");				\
+		break;						\
+	case 8:							\
+		__asm__ __volatile__ (				\
+			"amoswap.d" #asm_or " %0, %2, %1"	\
+			: "=r" (__ret), "+A" (*__ptr)		\
+			: "r" (__new)				\
+			: "memory");				\
+		break;						\
+	default:						\
+		BUILD_BUG();					\
+	}							\
+	__ret;							\
+})
+
+#define xchg(ptr, x)    (__xchg((x), (ptr), sizeof(*(ptr)), .aqrl))
+
+#define xchg32(ptr, x)				\
+({						\
+	BUILD_BUG_ON(sizeof(*(ptr)) != 4);	\
+	xchg((ptr), (x));			\
+})
+
+#define xchg64(ptr, x)				\
+({						\
+	BUILD_BUG_ON(sizeof(*(ptr)) != 8);	\
+	xchg((ptr), (x));			\
+})
+
+/*
+ * Atomic compare and exchange.  Compare OLD with MEM, if identical,
+ * store NEW in MEM.  Return the initial value in MEM.  Success is
+ * indicated by comparing RETURN with OLD.
+ */
+#define __cmpxchg(ptr, old, new, size, lrb, scb)			\
+({									\
+	__typeof__(ptr) __ptr = (ptr);					\
+	__typeof__(*(ptr)) __old = (old);				\
+	__typeof__(*(ptr)) __new = (new);				\

arch/riscv/include/asm/bitops.h new file +22 -0

+		"1:"							\
+			: "=&r" (__ret), "=&r" (__rc), "+A" (*__ptr)	\
+			: "rJ" (__old), "rJ" (__new)			\
+			: "memory");					\
+		break;							\
+	case 8:								\
+		__asm__ __volatile__ (					\
+		"0:"							\
+			"lr.d" #scb " %0, %2\n"				\
+			"bne         %0, %z3, 1f\n"			\
+			"sc.d" #lrb " %1, %z4, %2\n"			\
+			"bnez        %1, 0b\n"				\
+		"1:"							\
+			: "=&r" (__ret), "=&r" (__rc), "+A" (*__ptr)	\
+			: "rJ" (__old), "rJ" (__new)			\
+			: "memory");					\
+		break;							\
+	default:							\
+		BUILD_BUG();						\
+	}								\
+	__ret;								\
+})

arch/riscv/include/asm/bitops.h new file +4 -0

+#ifndef _ASM_RISCV_SPINLOCK_H
+#define _ASM_RISCV_SPINLOCK_H
+
+#include <linux/kernel.h>

arch/riscv/include/asm/bitops.h new file +31 -0

+
+static inline void arch_spin_unlock(arch_spinlock_t *lock)
+{
+	__asm__ __volatile__ (
+		"amoswap.w.rl x0, x0, %0"
+		: "=A" (lock->lock)
+		:: "memory");
+}
+
+static inline int arch_spin_trylock(arch_spinlock_t *lock)
+{
+	int tmp = 1, busy;
+
+	__asm__ __volatile__ (
+		"amoswap.w.aq %0, %2, %1"
+		: "=r" (busy), "+A" (lock->lock)
+		: "r" (tmp)
+		: "memory");
+
+	return !busy;
+}
+
+static inline void arch_spin_lock(arch_spinlock_t *lock)
+{
+	while (1) {
+		if (arch_spin_is_locked(lock))
+			continue;
+
+		if (arch_spin_trylock(lock))
+			break;
+	}

arch/riscv/include/asm/bitops.h new file +29 -0

+/***********************************************************/
+
+static inline int arch_read_can_lock(arch_rwlock_t *lock)
+{
+	return lock->lock >= 0;
+}
+
+static inline int arch_write_can_lock(arch_rwlock_t *lock)
+{
+	return lock->lock == 0;
+}
+
+static inline void arch_read_lock(arch_rwlock_t *lock)
+{
+	int tmp;
+
+	__asm__ __volatile__(
+		"1:	lr.w	%1, %0\n"
+		"	bltz	%1, 1b\n"
+		"	addi	%1, %1, 1\n"
+		"	sc.w.aq	%1, %1, %0\n"
+		"	bnez	%1, 1b\n"
+		: "+A" (lock->lock), "=&r" (tmp)
+		:: "memory");
+}
+
+static inline void arch_write_lock(arch_rwlock_t *lock)
+{
+	int tmp;

arch/riscv/include/asm/bitops.h new file +6 -0

+#ifndef _ASM_RISCV_TLBFLUSH_H
+#define _ASM_RISCV_TLBFLUSH_H
+
+#ifdef CONFIG_MMU
+
+/* Flush entire local TLB */

Hi Palmer,

Some late comments on this which you might want to address as you get the chance.

Quoted 24 lines

This contains all the code that directly interfaces with the RISC-V memory model. While this code corforms to the current RISC-V ISA specifications (user 2.2 and priv 1.10), the memory model is somewhat underspecified in those documents. There is a working group that hopes to produce a formal memory model by the end of the year, but my understanding is that the basic definitions we're relying on here won't change significantly.

Reviewed-by Arnd Bergmann <[email protected]>

Signed-off-by Palmer Dabbelt <[email protected]>

---
 arch/riscv/include/asm/atomic.h         | 375 ++++++++++++++++++++++++++++++++
 arch/riscv/include/asm/barrier.h        |  68 ++++++
 arch/riscv/include/asm/bitops.h         | 218 +++++++++++++++++++
 arch/riscv/include/asm/cacheflush.h     |  39 ++++
 arch/riscv/include/asm/cmpxchg.h        | 134 ++++++++++++
 arch/riscv/include/asm/io.h             | 303 ++++++++++++++++++++++++++
 arch/riscv/include/asm/spinlock.h       | 165 ++++++++++++++
 arch/riscv/include/asm/spinlock_types.h |  33 +++
 arch/riscv/include/asm/tlb.h            |  24 ++
 arch/riscv/include/asm/tlbflush.h       |  64 ++++++
 10 files changed, 1423 insertions(+)
 create mode 100644 arch/riscv/include/asm/atomic.h
 create mode 100644 arch/riscv/include/asm/barrier.h

Quoted 8 lines

 create mode 100644 arch/riscv/include/asm/bitops.h
 create mode 100644 arch/riscv/include/asm/cacheflush.h
 create mode 100644 arch/riscv/include/asm/cmpxchg.h
 create mode 100644 arch/riscv/include/asm/io.h
 create mode 100644 arch/riscv/include/asm/spinlock.h
 create mode 100644 arch/riscv/include/asm/spinlock_types.h
 create mode 100644 arch/riscv/include/asm/tlb.h
 create mode 100644 arch/riscv/include/asm/tlbflush.h

Quoted 18 lines

(quoted diff) +18 -0

+#define ATOMIC_OP(op, asm_op, c_op, I, asm_type, c_type, prefix)				\
+static __always_inline void atomic##prefix##_##op(c_type i, atomic##prefix##_t *v)		\
+{												\
+	__asm__ __volatile__ (									\
+		"amo" #asm_op "." #asm_type " zero, %1, %0"					\
+		: "+A" (v->counter)								\
+		: "r" (I)									\
+		: "memory");									\
+}
+
+#ifdef CONFIG_GENERIC_ATOMIC64
+#define ATOMIC_OPS(op, asm_op, c_op, I)			\
+        ATOMIC_OP (op, asm_op, c_op, I, w,  int,   )
+#else
+#define ATOMIC_OPS(op, asm_op, c_op, I)			\
+        ATOMIC_OP (op, asm_op, c_op, I, w,  int,   )	\
+        ATOMIC_OP (op, asm_op, c_op, I, d, long, 64)
+#endif

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+ATOMIC_OPS(add, add, +,  i)
+ATOMIC_OPS(sub, add, +, -i)
+ATOMIC_OPS(and, and, &,  i)
+ATOMIC_OPS( or,  or, |,  i)
+ATOMIC_OPS(xor, xor, ^,  i)

What is the point in the c_op parameter to these things?

Quoted 30 lines

(quoted diff) +29 -0

+/*
+ * Atomic ops that have ordered, relaxed, acquire, and relese variants.
+ * There's two flavors of these: the arithmatic ops have both fetch and return
+ * versions, while the logical ops only have fetch versions.
+ */
+#define ATOMIC_FETCH_OP(op, asm_op, c_op, I, asm_or, c_or, asm_type, c_type, prefix)			\
+static __always_inline c_type atomic##prefix##_fetch_##op##c_or(c_type i, atomic##prefix##_t *v)	\
+{													\
+	register c_type ret;										\
+	__asm__ __volatile__ (										\
+		"amo" #asm_op "." #asm_type #asm_or " %1, %2, %0"					\
+		: "+A" (v->counter), "=r" (ret)								\
+		: "r" (I)										\
+		: "memory");										\
+	return ret;											\
+}
+
+#define ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_or, c_or, asm_type, c_type, prefix)			\
+static __always_inline c_type atomic##prefix##_##op##_return##c_or(c_type i, atomic##prefix##_t *v)	\
+{													\
+        return atomic##prefix##_fetch_##op##c_or(i, v) c_op I;						\
+}
+
+#ifdef CONFIG_GENERIC_ATOMIC64
+#define ATOMIC_OPS(op, asm_op, c_op, I, asm_or, c_or)				\
+        ATOMIC_FETCH_OP (op, asm_op, c_op, I, asm_or, c_or, w,  int,   )	\
+        ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_or, c_or, w,  int,   )
+#else
+#define ATOMIC_OPS(op, asm_op, c_op, I, asm_or, c_or)				\

Review

Quoted 10 lines

(quoted diff) +10 -0

+        ATOMIC_FETCH_OP (op, asm_op, c_op, I, asm_or, c_or, w,  int,   )	\
+        ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_or, c_or, w,  int,   )	\
+        ATOMIC_FETCH_OP (op, asm_op, c_op, I, asm_or, c_or, d, long, 64)	\
+        ATOMIC_OP_RETURN(op, asm_op, c_op, I, asm_or, c_or, d, long, 64)
+#endif
+
+ATOMIC_OPS(add, add, +,  i,      , _relaxed)
+ATOMIC_OPS(add, add, +,  i, .aq  , _acquire)
+ATOMIC_OPS(add, add, +,  i, .rl  , _release)
+ATOMIC_OPS(add, add, +,  i, .aqrl,         )

Have you checked that .aqrl is equivalent to "ordered", since there are interpretations where that isn't the case. Specifically:

// all variables zero at start of time
P0:
WRITE_ONCE(x) = 1;
atomic_add_return(y, 1);
WRITE_ONCE(z) = 1;

P1:
READ_ONCE(z) // reads 1
smp_rmb();
READ_ONCE(x) // must not read 0

Quoted 6 lines

+/*
+ * atomic_{cmp,}xchg is required to have exactly the same ordering semantics as
+ * {cmp,}xchg and the operations that return, so they need a barrier. We just
+ * use the other implementations directly.
+ */

We also have relaxed/acquire/release versions of cmpxchg and xchg, if you want to implement them.

Quoted 56 lines

arch/riscv/include/asm/barrier.h new file +50 -0

new file mode 100644
index 000000000000..183534b7c39b
@@ -0,0 +1,68 @@
+/*
+ * Based on arch/arm/include/asm/barrier.h
+ *
+ * Copyright (C) 2012 ARM Ltd.
+ * Copyright (C) 2013 Regents of the University of California
+ * Copyright (C) 2017 SiFive
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ASM_RISCV_BARRIER_H
+#define _ASM_RISCV_BARRIER_H
+
+#ifndef __ASSEMBLY__
+
+#define nop()		__asm__ __volatile__ ("nop")
+
+#define RISCV_FENCE(p, s) \
+	__asm__ __volatile__ ("fence " #p "," #s : : : "memory")
+
+/* These barriers need to enforce ordering on both devices or memory. */
+#define mb()		RISCV_FENCE(iorw,iorw)
+#define rmb()		RISCV_FENCE(ir,ir)
+#define wmb()		RISCV_FENCE(ow,ow)
+
+/* These barriers do not need to enforce ordering on devices, just memory. */
+#define smp_mb()	RISCV_FENCE(rw,rw)
+#define smp_rmb()	RISCV_FENCE(r,r)
+#define smp_wmb()	RISCV_FENCE(w,w)
+
+/*
+ * These fences exist to enforce ordering around the relaxed AMOs.  The
+ * documentation defines that
+ * "
+ *     atomic_fetch_add();
+ *   is equivalent to:
+ *     smp_mb__before_atomic();
+ *     atomic_fetch_add_relaxed();
+ *     smp_mb__after_atomic();
+ * "

arch/riscv/include/asm/barrier.h line 54

Quoted 4 lines

arch/riscv/include/asm/barrier.h new file +4 -0

+ * So we emit full fences on both sides.
+ */
+#define __smb_mb__before_atomic()	smp_mb()
+#define __smb_mb__after_atomic()	smp_mb()

Now I'm confused, because you're also spitting out .aqrl for those afaict. Do you really need full barriers *and* .aqrl, or am I misunderstanding something here?

arch/riscv/include/asm/barrier.h line 61

Quoted 7 lines

arch/riscv/include/asm/barrier.h new file +7 -0

+
+/*
+ * These barriers prevent accesses performed outside a spinlock from being moved
+ * inside a spinlock.  Since RISC-V sets the aq/rl bits on our spinlock only
+ * enforce release consistency, we need full fences here.
+ */
+#define smb_mb__before_spinlock()	smp_mb()

We killed this macro, so you don't need to define it.

Quoted 61 lines

arch/riscv/include/asm/bitops.h new file +55 -0

new file mode 100644
index 000000000000..7c281ef1d583
@@ -0,0 +1,218 @@
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ *
+ *   This program is free software; you can redistribute it and/or
+ *   modify it under the terms of the GNU General Public License
+ *   as published by the Free Software Foundation, version 2.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ */
+
+#ifndef _ASM_RISCV_BITOPS_H
+#define _ASM_RISCV_BITOPS_H
+
+#ifndef _LINUX_BITOPS_H
+#error "Only <linux/bitops.h> can be included directly"
+#endif /* _LINUX_BITOPS_H */
+
+#include <linux/compiler.h>
+#include <linux/irqflags.h>
+#include <asm/barrier.h>
+#include <asm/bitsperlong.h>
+
+#ifndef smp_mb__before_clear_bit
+#define smp_mb__before_clear_bit()  smp_mb()
+#define smp_mb__after_clear_bit()   smp_mb()
+#endif /* smp_mb__before_clear_bit */
+
+#include <asm-generic/bitops/__ffs.h>
+#include <asm-generic/bitops/ffz.h>
+#include <asm-generic/bitops/fls.h>
+#include <asm-generic/bitops/__fls.h>
+#include <asm-generic/bitops/fls64.h>
+#include <asm-generic/bitops/find.h>
+#include <asm-generic/bitops/sched.h>
+#include <asm-generic/bitops/ffs.h>
+
+#include <asm-generic/bitops/hweight.h>
+
+#if (BITS_PER_LONG == 64)
+#define __AMO(op)	"amo" #op ".d"
+#elif (BITS_PER_LONG == 32)
+#define __AMO(op)	"amo" #op ".w"
+#else
+#error "Unexpected BITS_PER_LONG"
+#endif
+
+#define __test_and_op_bit_ord(op, mod, nr, addr, ord)		\
+({								\
+	unsigned long __res, __mask;				\
+	__mask = BIT_MASK(nr);					\
+	__asm__ __volatile__ (					\
+		__AMO(op) #ord " %0, %2, %1"			\

arch/riscv/include/asm/bitops.h line 60

Quoted 5 lines

arch/riscv/include/asm/bitops.h new file +5 -0

+		: "=r" (__res), "+A" (addr[BIT_WORD(nr)])	\
+		: "r" (mod(__mask))				\
+		: "memory");					\
+	((__res & __mask) != 0);				\
+})

This looks broken to me -- the value-returning test bitops need to be fully ordered.

Quoted 14 lines

arch/riscv/include/asm/bitops.h new file +14 -0

+/*
+ * Atomic compare and exchange.  Compare OLD with MEM, if identical,
+ * store NEW in MEM.  Return the initial value in MEM.  Success is
+ * indicated by comparing RETURN with OLD.
+ */
+#define __cmpxchg(ptr, old, new, size, lrb, scb)			\
+({									\
+	__typeof__(ptr) __ptr = (ptr);					\
+	__typeof__(*(ptr)) __old = (old);				\
+	__typeof__(*(ptr)) __new = (new);				\
+	__typeof__(*(ptr)) __ret;					\
+	register unsigned int __rc;					\
+	switch (size) {							\
+	case 4:								\

arch/riscv/include/asm/bitops.h line 80

Quoted 6 lines

arch/riscv/include/asm/bitops.h new file +6 -0

+		__asm__ __volatile__ (					\
+		"0:"							\
+			"lr.w" #scb " %0, %2\n"				\
+			"bne         %0, %z3, 1f\n"			\
+			"sc.w" #lrb " %1, %z4, %2\n"			\
+			"bnez        %1, 0b\n"				\

You don't have an AMO for these?

Quoted 22 lines

arch/riscv/include/asm/bitops.h new file +22 -0

+		"1:"							\
+			: "=&r" (__ret), "=&r" (__rc), "+A" (*__ptr)	\
+			: "rJ" (__old), "rJ" (__new)			\
+			: "memory");					\
+		break;							\
+	case 8:								\
+		__asm__ __volatile__ (					\
+		"0:"							\
+			"lr.d" #scb " %0, %2\n"				\
+			"bne         %0, %z3, 1f\n"			\
+			"sc.d" #lrb " %1, %z4, %2\n"			\
+			"bnez        %1, 0b\n"				\
+		"1:"							\
+			: "=&r" (__ret), "=&r" (__rc), "+A" (*__ptr)	\
+			: "rJ" (__old), "rJ" (__new)			\
+			: "memory");					\
+		break;							\
+	default:							\
+		BUILD_BUG();						\
+	}								\
+	__ret;								\
+})

[...]

Quoted 4 lines

arch/riscv/include/asm/bitops.h new file +4 -0

+#ifndef _ASM_RISCV_SPINLOCK_H
+#define _ASM_RISCV_SPINLOCK_H
+
+#include <linux/kernel.h>

arch/riscv/include/asm/bitops.h

Quoted 10 lines

arch/riscv/include/asm/bitops.h new file +10 -0

+#include <asm/current.h>
+
+/*
+ * Simple spin lock operations.  These provide no fairness guarantees.
+ */
+
+/* FIXME: Replace this with a ticket lock, like MIPS. */
+
+#define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
+#define arch_spin_is_locked(x)	((x)->lock != 0)

Missing READ_ONCE.

arch/riscv/include/asm/bitops.h

Quoted 8 lines

arch/riscv/include/asm/bitops.h new file +8 -0

+static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
+{
+	smp_rmb();
+	do {
+		cpu_relax();
+	} while (arch_spin_is_locked(lock));
+	smp_acquire__after_ctrl_dep();
+}

We killed this one too, so please drop it.

Quoted 30 lines

arch/riscv/include/asm/bitops.h new file +29 -0

+/***********************************************************/
+
+static inline int arch_read_can_lock(arch_rwlock_t *lock)
+{
+	return lock->lock >= 0;
+}
+
+static inline int arch_write_can_lock(arch_rwlock_t *lock)
+{
+	return lock->lock == 0;
+}
+
+static inline void arch_read_lock(arch_rwlock_t *lock)
+{
+	int tmp;
+
+	__asm__ __volatile__(
+		"1:	lr.w	%1, %0\n"
+		"	bltz	%1, 1b\n"
+		"	addi	%1, %1, 1\n"
+		"	sc.w.aq	%1, %1, %0\n"
+		"	bnez	%1, 1b\n"
+		: "+A" (lock->lock), "=&r" (tmp)
+		:: "memory");
+}
+
+static inline void arch_write_lock(arch_rwlock_t *lock)
+{
+	int tmp;

arch/riscv/include/asm/bitops.h

Quoted 10 lines

arch/riscv/include/asm/bitops.h new file +10 -0

+
+	__asm__ __volatile__(
+		"1:	lr.w	%1, %0\n"
+		"	bnez	%1, 1b\n"
+		"	li	%1, -1\n"
+		"	sc.w.aq	%1, %1, %0\n"
+		"	bnez	%1, 1b\n"
+		: "+A" (lock->lock), "=&r" (tmp)
+		:: "memory");
+}

I think you have the same starvation issues as we have on arm64 here. I strongly recommend moving over to qrwlock :)

Quoted 11 lines

arch/riscv/include/asm/bitops.h new file +11 -0

+#ifndef _ASM_RISCV_TLBFLUSH_H
+#define _ASM_RISCV_TLBFLUSH_H
+
+#ifdef CONFIG_MMU
+
+/* Flush entire local TLB */
+static inline void local_flush_tlb_all(void)
+{
+	__asm__ __volatile__ ("sfence.vma" : : : "memory");
+}
+

arch/riscv/include/asm/bitops.h

Quoted 5 lines

arch/riscv/include/asm/bitops.h new file +5 -0

+/* Flush one page from local TLB */
+static inline void local_flush_tlb_page(unsigned long addr)
+{
+	__asm__ __volatile__ ("sfence.vma %0" : : "r" (addr) : "memory");
+}

Is this serialised against prior updates to the page table (set_pte) and also against subsequent instruction fetch?

Quoted 8 lines · On Tue, 24 Oct 2017 07:10:33 PDT (-0700), [email protected] wrote:

Review

Quoted 5 lines · Palmer Dabbelt

(quoted diff) +5 -0

+ATOMIC_OPS(add, add, +,  i)
+ATOMIC_OPS(sub, add, +, -i)
+ATOMIC_OPS(and, and, &,  i)
+ATOMIC_OPS( or,  or, |,  i)
+ATOMIC_OPS(xor, xor, ^,  i)

What is the point in the c_op parameter to these things?

I guess there isn't one, it just lingered from a handful of refactorings. It's used in some of the other functions if we need to do a C operation after the atomic. How does this look?

commit 5db229491a205ad0e1aa18287e3b342176c62d30 (HEAD -> staging-mm)

Author: Palmer Dabbelt <[email protected]>
Date:   Tue Nov 14 11:35:37 2017 -0800

RISC-V: Remove c_op from ATOMIC_OP

This was an unused macro parameter.

Signed-off-by Palmer Dabbelt <[email protected]>

Quoted 14 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+ATOMIC_OPS(add, add, +,  i,      , _relaxed)
+ATOMIC_OPS(add, add, +,  i, .aq  , _acquire)
+ATOMIC_OPS(add, add, +,  i, .rl  , _release)
+ATOMIC_OPS(add, add, +,  i, .aqrl,         )

Have you checked that .aqrl is equivalent to "ordered", since there are interpretations where that isn't the case. Specifically:

// all variables zero at start of time
P0:
WRITE_ONCE(x) = 1;
atomic_add_return(y, 1);
WRITE_ONCE(z) = 1;

Quoted 4 lines

P1:
READ_ONCE(z) // reads 1
smp_rmb();
READ_ONCE(x) // must not read 0

I haven't. We don't quite have a formal memory model specification yet. I've added Daniel Lustig, who is creating that model. He should have a better idea

Quoted 15 lines

Quoted 15 lines

(quoted diff) +15 -0

+/* These barriers do not need to enforce ordering on devices, just memory. */
+#define smp_mb()	RISCV_FENCE(rw,rw)
+#define smp_rmb()	RISCV_FENCE(r,r)
+#define smp_wmb()	RISCV_FENCE(w,w)
+
+/*
+ * These fences exist to enforce ordering around the relaxed AMOs.  The
+ * documentation defines that
+ * "
+ *     atomic_fetch_add();
+ *   is equivalent to:
+ *     smp_mb__before_atomic();
+ *     atomic_fetch_add_relaxed();
+ *     smp_mb__after_atomic();
+ * "

Quoted 8 lines

Review

Quoted 4 lines

+ * So we emit full fences on both sides.
+ */

(quoted diff) +2 -0

+#define __smb_mb__before_atomic()	smp_mb()
+#define __smb_mb__after_atomic()	smp_mb()

Now I'm confused, because you're also spitting out .aqrl for those afaict. Do you really need full barriers *and* .aqrl, or am I misunderstanding something here?

Here's the section of atomic_t.txt that I was reading

The barriers:

     smp_mb__{before,after}_atomic()
   only apply to the RMW ops and can be used to augment/upgrade the ordering
   inherent to the used atomic op. These barriers provide a full smp_mb().
   These helper barriers exist because architectures have varying implicit
   ordering on their SMP atomic primitives. For example our TSO architectures
   provide full ordered atomics and these barriers are no-ops.
   Thus:
     atomic_fetch_add();
   is equivalent to:
     smp_mb__before_atomic();
     atomic_fetch_add_relaxed();
     smp_mb__after_atomic();

However the atomic_fetch_add() might be implemented more efficiently.

so I think what we've got there is correct: the barriers go with atomic_fetch_add_relaxed(), not atomic_fetch_add(). asm-generic/barrier.h has

   #ifndef __smp_mb__before_atomic
   #define __smp_mb__before_atomic()       __smp_mb()
   #endif
   #ifndef __smp_mb__after_atomic
   #define __smp_mb__after_atomic()        __smp_mb()
   #endif

so I think we can just drop these entirely. How does this look?

commit da682f7ee5d2af4aae7026ef40b5b5fb8e103938

Author: Palmer Dabbelt <[email protected]>
Date:   Tue Nov 14 11:49:42 2017 -0800

RISC-V: Remove __smp_bp__{before,after}_atomic

These duplicate the asm-generic definitions are therefor aren't useful.

Signed-off-by Palmer Dabbelt <[email protected]>

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+
+/*
+ * These barriers prevent accesses performed outside a spinlock from being moved
+ * inside a spinlock.  Since RISC-V sets the aq/rl bits on our spinlock only
+ * enforce release consistency, we need full fences here.
+ */
+#define smb_mb__before_spinlock()	smp_mb()

We killed this macro, so you don't need to define it.

Thanks!

commit 382a1f8b33a04fc0f991e062f70f4c65ca888bca

Author: Palmer Dabbelt <[email protected]>
Date:   Tue Nov 14 11:50:37 2017 -0800

RISC-V: Remove smb_mb__{before,after}_spinlock()

These are obselete.

Signed-off-by Palmer Dabbelt <[email protected]>

arch/riscv/include/asm/barrier.h +0 -0

index 455ee16127fb..773c4e039cd7 100644

arch/riscv/include/asm/barrier.h +0 -0

 >>+#define __test_and_op_bit_ord(op, mod, nr, addr, ord)		\
 >>+({								\
 >>+	unsigned long __res, __mask;				\

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+	__mask = BIT_MASK(nr);					\
+	__asm__ __volatile__ (					\
+		__AMO(op) #ord " %0, %2, %1"			\
+		: "=r" (__res), "+A" (addr[BIT_WORD(nr)])	\
+		: "r" (mod(__mask))				\
+		: "memory");					\
+	((__res & __mask) != 0);				\
+})

This looks broken to me -- the value-returning test bitops need to be fully ordered.

Yep, you're right -- I just mis-read atomic_ops.rst (and re-misread it the first time when I went to check again). I think this should do it

commit 9951b6ed76bffb714517d81d9ffceb0eb1796229

Author: Palmer Dabbelt <[email protected]>
Date:   Tue Nov 14 12:06:06 2017 -0800

RISC-V: __test_and_op_bit_ord should be strongly ordered

I mis-read the documentation.

Signed-off-by Palmer Dabbelt <[email protected]>

arch/riscv/include/asm/bitops.h +0 -0

index 7c281ef1d583..f30daf26f08f 100644

arch/riscv/include/asm/bitops.h +0 -0

 >>+#define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 >>+#define arch_spin_is_locked(x)	((x)->lock != 0)

Quoted 2 lines

Missing READ_ONCE.

Thanks!

commit 64e80b0bf3898a88de09f4e12090b002b57efede

Author: Palmer Dabbelt <[email protected]>
Date:   Tue Nov 14 12:18:49 2017 -0800

RISC-V: Add READ_ONCE in arch_spin_is_locked()

Signed-off-by Palmer Dabbelt <[email protected]>

Quoted 22 lines

Quoted 22 lines

(quoted diff) +22 -0

+static inline void arch_read_lock(arch_rwlock_t *lock)
+{
+	int tmp;
+
+	__asm__ __volatile__(
+		"1:	lr.w	%1, %0\n"
+		"	bltz	%1, 1b\n"
+		"	addi	%1, %1, 1\n"
+		"	sc.w.aq	%1, %1, %0\n"
+		"	bnez	%1, 1b\n"
+		: "+A" (lock->lock), "=&r" (tmp)
+		:: "memory");
+}
+
+static inline void arch_write_lock(arch_rwlock_t *lock)
+{
+	int tmp;
+
+	__asm__ __volatile__(
+		"1:	lr.w	%1, %0\n"
+		"	bnez	%1, 1b\n"
+		"	li	%1, -1\n"

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		"	sc.w.aq	%1, %1, %0\n"
+		"	bnez	%1, 1b\n"
+		: "+A" (lock->lock), "=&r" (tmp)
+		:: "memory");
+}

I think you have the same starvation issues as we have on arm64 here. I strongly recommend moving over to qrwlock :)

Quoted 12 lines

Quoted 12 lines

(quoted diff) +11 -0

+#ifndef _ASM_RISCV_TLBFLUSH_H
+#define _ASM_RISCV_TLBFLUSH_H
+
+#ifdef CONFIG_MMU
+
+/* Flush entire local TLB */
+static inline void local_flush_tlb_all(void)
+{
+	__asm__ __volatile__ ("sfence.vma" : : : "memory");
+}
+

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+/* Flush one page from local TLB */
+static inline void local_flush_tlb_page(unsigned long addr)
+{
+	__asm__ __volatile__ ("sfence.vma %0" : : "r" (addr) : "memory");
+}

Is this serialised against prior updates to the page table (set_pte) and also against subsequent instruction fetch?

This is a store -> (load, store) fence. The ordering is between stores that touch paging data structures and the implicit loads that come from any memory access when paging is enabled. As far as I can tell, it does not enforce any instruction fetch ordering constraints.

Hi Palmer,

Quoted 18 lines · Palmer Dabbelt

Quoted 8 lines · On Tue, 24 Oct 2017 07:10:33 PDT (-0700), [email protected] wrote:

Review

Quoted 5 lines · Palmer Dabbelt

(quoted diff) +5 -0

+ATOMIC_OPS(add, add, +,  i)
+ATOMIC_OPS(sub, add, +, -i)
+ATOMIC_OPS(and, and, &,  i)
+ATOMIC_OPS( or,  or, |,  i)
+ATOMIC_OPS(xor, xor, ^,  i)

What is the point in the c_op parameter to these things?

I guess there isn't one, it just lingered from a handful of refactorings. It's used in some of the other functions if we need to do a C operation after the atomic. How does this look?

commit 5db229491a205ad0e1aa18287e3b342176c62d30 (HEAD -> staging-mm)

Author: Palmer Dabbelt <[email protected]>
Date:   Tue Nov 14 11:35:37 2017 -0800

Quoted 5 lines

RISC-V: Remove c_op from ATOMIC_OP

This was an unused macro parameter.

Signed-off-by Palmer Dabbelt <[email protected]>

You can do the same thing for FETCH_OP, I think.