Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681158 > unrolled thread

RISC-V Linux Port v4

Back to article view | Back to linux.kernel

Contents

  RISC-V Linux Port v4 Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-04 22:00 +0200
    Re: [patches] Re: [PATCH 1/9] RISC-V: Init and Halt Code Karsten Merker <merker@debian.org> - 2017-07-04 23:20 +0200
      Re: [patches] Re: [PATCH 1/9] RISC-V: Init and Halt Code Thomas Gleixner <tglx@linutronix.de> - 2017-07-05 08:50 +0200
    Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code Jonathan Neuschäfer <j.neuschaefer@gmx.net> - 2017-07-05 00:00 +0200
      Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-07 00:40 +0200
        Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code Jonathan Neuschäfer <j.neuschaefer@gmx.net> - 2017-07-07 15:10 +0200
          Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-10 22:50 +0200
    Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Peter Zijlstra <peterz@infradead.org> - 2017-07-05 10:50 +0200
      Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Boqun Feng <boqun.feng@gmail.com> - 2017-07-06 06:20 +0200
        Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Peter Zijlstra <peterz@infradead.org> - 2017-07-06 09:30 +0200
      Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-07 03:10 +0200
        Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Boqun Feng <boqun.feng@gmail.com> - 2017-07-07 04:20 +0200
          Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-10 22:40 +0200
        Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Peter Zijlstra <peterz@infradead.org> - 2017-07-07 10:10 +0200
          Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-10 22:40 +0200
    Re: [PATCH 8/9] RISC-V: User-facing API James Hogan <james.hogan@imgtec.com> - 2017-07-05 12:30 +0200
    Re: [PATCH 8/9] RISC-V: User-facing API Christoph Hellwig <hch@infradead.org> - 2017-07-06 04:10 +0200
      Re: [PATCH 8/9] RISC-V: User-facing API Will Deacon <will.deacon@arm.com> - 2017-07-06 11:00 +0200
        Re: [PATCH 8/9] RISC-V: User-facing API Christoph Hellwig <hch@infradead.org> - 2017-07-06 17:40 +0200
          Re: [PATCH 8/9] RISC-V: User-facing API Will Deacon <will.deacon@arm.com> - 2017-07-06 17:50 +0200
            Re: [PATCH 8/9] RISC-V: User-facing API Will Deacon <will.deacon@arm.com> - 2017-07-11 15:30 +0200
              Re: [PATCH 8/9] RISC-V: User-facing API Christoph Hellwig <hch@infradead.org> - 2017-07-11 16:00 +0200
                Re: [PATCH 8/9] RISC-V: User-facing API Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-11 19:30 +0200
              Re: [PATCH 8/9] RISC-V: User-facing API Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-11 19:10 +0200
    Re: [PATCH 2/9] RISC-V: Atomic and Locking Code Boqun Feng <boqun.feng@gmail.com> - 2017-07-06 05:40 +0200
    Re: [PATCH 8/9] RISC-V: User-facing API Dave P Martin <Dave.Martin@arm.com> - 2017-07-06 17:40 +0200
    Re: [patches] [PATCH 2/9] RISC-V: Atomic and Locking Code Jonathan Neuschäfer <j.neuschaefer@gmx.net> - 2017-07-07 15:20 +0200
      Re: [patches] [PATCH 2/9] RISC-V: Atomic and Locking Code Palmer Dabbelt <palmer@dabbelt.com> - 2017-07-10 22:50 +0200

Page 1 of 2  [1] 2  Next page →

#1681158 — RISC-V Linux Port v4

Thanks to everyone who has participated in the review process so far.  There
have only been a few changes since the v3 patch set:

 * The cmpxchg64 syscall is no longer enabled on 32-bit systems.  It's not
   possible to provide this on SMP systems, and it's not necessary as glibc
   knows not to call it.

 * We provide a ELF_HWCAP so users can determine the ISA of the machine the
   kernel is running on.

 * The multi-line comments are in a better form.

 * There were a handful of headers that could be replaced with the asm-generic
   versions, and a few unnecessary definitions.

 * We no longer use printk, but instead use pr_*.

 * A few Kconfig and defconfig entries have been cleaned up.

Since things have really calmed down, I think it would be good to try and get
this into 4.13 (ie, during this merge window).  This might let us get into the
upcoming glibc release as well.  Does that seem possible?

Like last time, in case one gets eaten by the mailing list this is also
available as a git tree on our Git Hub

  https://github.com/riscv/riscv-linux/tree/riscv-for-submission-arch-v4

This patch set just contains the arch code, we have various drivers that are
required to build and boot a RISC-V system.  A tree that contains this patch
set merged with all our other patch sets lives at

  https://github.com/riscv/riscv-linux/tree/riscv-for-submission-v4

  a83d4bb70b76 pci: Add a generic, weakly-linked pcibios_fixup_bus
  a3445d3b4ff1 pci: Add a generic, weakly-linked pcibios_align_resource
  d97ac4f5bb1d Merge branch 'riscv-for-submission-arch-v4' into riscv-for-submission-v4
  0705a31372d4 Merge branch 'riscv-for-submission-clk-v2' into riscv-for-submission-v4
  a6c8eb9af199 Merge branch 'riscv-for-submission-irq-v2' into riscv-for-submission-v4
  3e5b46967f66 Merge branch 'riscv-for-submission-lib-v2' into riscv-for-submission-v4
  8ca3c2cdefed Merge branch 'riscv-for-submission-pci-v3' into riscv-for-submission-v4
  e1aa9abddfa8 Merge branch 'riscv-for-submission-tty-v2' into riscv-for-submission-v4

If you're going to try to build or boot the kernel, I'd recommend using that.

Thanks to everyone who has helped review our port!

[PATCH 1/9] RISC-V: Init and Halt Code
[PATCH 2/9] RISC-V: Atomic and Locking Code
[PATCH 3/9] RISC-V: Generic library routines and assembly
[PATCH 4/9] RISC-V: ELF and module implementation
[PATCH 5/9] RISC-V: Task implementation
[PATCH 6/9] RISC-V: Device, timer, IRQs, and the SBI
[PATCH 7/9] RISC-V: Paging and MMU
[PATCH 8/9] RISC-V: User-facing API
[PATCH 9/9] RISC-V: Build Infastructure

[toc] | [next] | [standalone]

#1681186 — Re: [patches] Re: [PATCH 1/9] RISC-V: Init and Halt Code

On Tue, Jul 04, 2017 at 10:37:37PM +0200, Thomas Gleixner wrote:
> On Tue, 4 Jul 2017, Palmer Dabbelt wrote:

> > +unsigned long riscv_timebase;
> > +
> > +static int next_event(unsigned long delta, struct clock_event_device *ce)
> > +{
> > +	BUG_ON(ce != timer_riscv_device(smp_processor_id()));
> > +	sbi_set_timer(get_cycles64() + delta);
> > +	return 0;
> > +}
> > +
> > +static unsigned long long rdtime(struct clocksource *cs)
> > +{
> > +	BUG_ON(cs != timer_riscv_source(smp_processor_id()));
> > +	return get_cycles64();
> > +}
> > +
> > +void riscv_timer_interrupt(void)
> > +{
> > +	int cpu = smp_processor_id();
> > +	struct clock_event_device *evdev = timer_riscv_device(cpu);
> > +
> > +	evdev->event_handler(evdev);
> > +}
> > +
> > +void __init time_init(void)
> > +{
> > +	struct device_node *cpu;
> > +	u32 prop;
> > +	int cpu_id = smp_processor_id();
> > +
> > +	cpu = of_find_node_by_path("/cpus");
> > +	if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
> > +		panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
> > +	riscv_timebase = prop;
> > +
> > +	lpj_fine = riscv_timebase / HZ;
> > +
> > +	timer_riscv_init(cpu_id, riscv_timebase, &rdtime, &next_event);
> 
> This function is nowhere.... Neither can I find the header file.

It is part of the RISC-V clock driver patchset, please cf.
https://github.com/riscv/riscv-linux/commit/0705a31372d4714e54c5104e02021441785c1fd5

https://github.com/riscv/riscv-linux/commits/riscv-for-submission-v4
has the various RISC-V-related driver patchsets merged in (in
addition to this arch patchset).

Regards,
Karsten
-- 
Gem. Par. 28 Abs. 4 Bundesdatenschutzgesetz widerspreche ich der Nutzung
sowie der Weitergabe meiner personenbezogenen Daten für Zwecke der
Werbung sowie der Markt- oder Meinungsforschung.

[toc] | [prev] | [next] | [standalone]

#1681311 — Re: [patches] Re: [PATCH 1/9] RISC-V: Init and Halt Code

On Tue, 4 Jul 2017, Karsten Merker wrote:
> On Tue, Jul 04, 2017 at 10:37:37PM +0200, Thomas Gleixner wrote:
> > > +	timer_riscv_init(cpu_id, riscv_timebase, &rdtime, &next_event);
> > 
> > This function is nowhere.... Neither can I find the header file.
> 
> It is part of the RISC-V clock driver patchset, please cf.
> https://github.com/riscv/riscv-linux/commit/0705a31372d4714e54c5104e02021441785c1fd5
> 
> https://github.com/riscv/riscv-linux/commits/riscv-for-submission-v4
> has the various RISC-V-related driver patchsets merged in (in
> addition to this arch patchset).

Well, then please move that timer stuff to the clock driver patchset, so
the combo can be reviewed. Having half functional patches with missing
references here and there makes review a PITA.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1681197 — Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code

[Multipart message — attachments visible in raw view] — view raw

Hi, below are some small comments.

On Tue, Jul 04, 2017 at 12:50:54PM -0700, Palmer Dabbelt wrote:
> This contains the various __init C functions, the initial assembly
> kernel entry point, and the code to reset the system.  When a file was
> init-related, it contains

It contains what?

> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[...]

> +#ifdef CONFIG_GENERIC_BUG
> +#define __BUG_INSN	_AC(0x00100073, UL) /* sbreak */

This should be ebreak, not sbreak, in Priv Spec 1.10, AFAICT.
I guess binutils still understands sbreak, but it's nicer to stick to
the spec, IMHO.

> +#define BUG()							\
> +do {								\
> +	__asm__ __volatile__ (					\
> +		"1:\n\t"					\
> +			"sbreak\n"				\

ebreak

> +			".pushsection __bug_table,\"a\"\n\t"	\
> +		"2:\n\t"					\
> +			__BUG_ENTRY "\n\t"			\
> +			".org 2b + %2\n\t"			\
> +			".popsection"				\
> +		:						\
> +		: "i" (__FILE__), "i" (__LINE__),		\
> +		  "i" (sizeof(struct bug_entry)));		\
> +	unreachable();						\
> +} while (0)
> +#endif /* !__ASSEMBLY__ */
> +#else /* CONFIG_GENERIC_BUG */
> +#ifndef __ASSEMBLY__
> +#define BUG()							\
> +do {								\
> +	__asm__ __volatile__ ("sbreak\n");			\

ebreak

> +	unreachable();						\
> +} while (0)
> +#endif /* !__ASSEMBLY__ */
> +#endif /* CONFIG_GENERIC_BUG */
[...]

> +#define DO_ERROR_INFO(name, signo, code, str)				\
> +asmlinkage void name(struct pt_regs *regs)				\
> +{									\
> +	do_trap_error(regs, signo, code, regs->sepc, "Oops - " str);	\
> +}
> +
> +DO_ERROR_INFO(do_trap_unknown,
> +	SIGILL, ILL_ILLTRP, "unknown exception");
> +DO_ERROR_INFO(do_trap_insn_misaligned,
> +	SIGBUS, BUS_ADRALN, "instruction address misaligned");
> +DO_ERROR_INFO(do_trap_insn_fault,
> +	SIGBUS, BUS_ADRALN, "instruction access fault");

For a general instruction access fault, BUS_ADRALN seems wrong. A
variant of SIGSEGV seems more appropriate, IMHO.


Jonathan Neuschäfer

[toc] | [prev] | [next] | [standalone]

#1682777 — Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code

On Tue, 04 Jul 2017 14:54:01 PDT (-0700), j.neuschaefer@gmx.net wrote:
> Hi, below are some small comments.
>
> On Tue, Jul 04, 2017 at 12:50:54PM -0700, Palmer Dabbelt wrote:
>> This contains the various __init C functions, the initial assembly
>> kernel entry point, and the code to reset the system.  When a file was
>> init-related, it contains
>
> It contains what?
>
>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> [...]
>
>> +#ifdef CONFIG_GENERIC_BUG
>> +#define __BUG_INSN	_AC(0x00100073, UL) /* sbreak */
>
> This should be ebreak, not sbreak, in Priv Spec 1.10, AFAICT.
> I guess binutils still understands sbreak, but it's nicer to stick to
> the spec, IMHO.

I agree.  IIRC they're the same instruction, we just alias sbreak->ebreak in
binutils (like scall->ecall).

>> +#define BUG()							\
>> +do {								\
>> +	__asm__ __volatile__ (					\
>> +		"1:\n\t"					\
>> +			"sbreak\n"				\
>
> ebreak
>
>> +			".pushsection __bug_table,\"a\"\n\t"	\
>> +		"2:\n\t"					\
>> +			__BUG_ENTRY "\n\t"			\
>> +			".org 2b + %2\n\t"			\
>> +			".popsection"				\
>> +		:						\
>> +		: "i" (__FILE__), "i" (__LINE__),		\
>> +		  "i" (sizeof(struct bug_entry)));		\
>> +	unreachable();						\
>> +} while (0)
>> +#endif /* !__ASSEMBLY__ */
>> +#else /* CONFIG_GENERIC_BUG */
>> +#ifndef __ASSEMBLY__
>> +#define BUG()							\
>> +do {								\
>> +	__asm__ __volatile__ ("sbreak\n");			\
>
> ebreak
>
>> +	unreachable();						\
>> +} while (0)
>> +#endif /* !__ASSEMBLY__ */
>> +#endif /* CONFIG_GENERIC_BUG */
> [...]
>
>> +#define DO_ERROR_INFO(name, signo, code, str)				\
>> +asmlinkage void name(struct pt_regs *regs)				\
>> +{									\
>> +	do_trap_error(regs, signo, code, regs->sepc, "Oops - " str);	\
>> +}
>> +
>> +DO_ERROR_INFO(do_trap_unknown,
>> +	SIGILL, ILL_ILLTRP, "unknown exception");
>> +DO_ERROR_INFO(do_trap_insn_misaligned,
>> +	SIGBUS, BUS_ADRALN, "instruction address misaligned");
>> +DO_ERROR_INFO(do_trap_insn_fault,
>> +	SIGBUS, BUS_ADRALN, "instruction access fault");
>
> For a general instruction access fault, BUS_ADRALN seems wrong. A
> variant of SIGSEGV seems more appropriate, IMHO.

How does this look?

  diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
  index 4c693b5b9980..3ce9ac6e736e 100644
  --- a/arch/riscv/kernel/traps.c
  +++ b/arch/riscv/kernel/traps.c
  @@ -112,7 +112,7 @@ DO_ERROR_INFO(do_trap_unknown,
   DO_ERROR_INFO(do_trap_insn_misaligned,
          SIGBUS, BUS_ADRALN, "instruction address misaligned");
   DO_ERROR_INFO(do_trap_insn_fault,
  -       SIGBUS, BUS_ADRALN, "instruction access fault");
  +       SIGBUS, SEGV_ACCERR, "instruction access fault");
   DO_ERROR_INFO(do_trap_insn_illegal,
          SIGILL, ILL_ILLOPC, "illegal instruction");
   DO_ERROR_INFO(do_trap_load_misaligned,

[toc] | [prev] | [next] | [standalone]

#1683174 — Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 06, 2017 at 03:34:39PM -0700, Palmer Dabbelt wrote:
> On Tue, 04 Jul 2017 14:54:01 PDT (-0700), j.neuschaefer@gmx.net wrote:
[...]
> >> +#define DO_ERROR_INFO(name, signo, code, str)				\
> >> +asmlinkage void name(struct pt_regs *regs)				\
> >> +{									\
> >> +	do_trap_error(regs, signo, code, regs->sepc, "Oops - " str);	\
> >> +}
> >> +
> >> +DO_ERROR_INFO(do_trap_unknown,
> >> +	SIGILL, ILL_ILLTRP, "unknown exception");
> >> +DO_ERROR_INFO(do_trap_insn_misaligned,
> >> +	SIGBUS, BUS_ADRALN, "instruction address misaligned");
> >> +DO_ERROR_INFO(do_trap_insn_fault,
> >> +	SIGBUS, BUS_ADRALN, "instruction access fault");
> >
> > For a general instruction access fault, BUS_ADRALN seems wrong. A
> > variant of SIGSEGV seems more appropriate, IMHO.
> 
> How does this look?
> 
>   diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>   index 4c693b5b9980..3ce9ac6e736e 100644
>   --- a/arch/riscv/kernel/traps.c
>   +++ b/arch/riscv/kernel/traps.c
>   @@ -112,7 +112,7 @@ DO_ERROR_INFO(do_trap_unknown,
>    DO_ERROR_INFO(do_trap_insn_misaligned,
>           SIGBUS, BUS_ADRALN, "instruction address misaligned");
>    DO_ERROR_INFO(do_trap_insn_fault,
>   -       SIGBUS, BUS_ADRALN, "instruction access fault");
>   +       SIGBUS, SEGV_ACCERR, "instruction access fault");
>    DO_ERROR_INFO(do_trap_insn_illegal,
>           SIGILL, ILL_ILLOPC, "illegal instruction");
>    DO_ERROR_INFO(do_trap_load_misaligned,

I'm not familiar with the trap handling infrastructure, but looking at
include/uapi/asm-generic/siginfo.h, SEGV_ACCERR would alias to
BUS_ADRERR (both are defined as (__SI_FAULT|2)). So if you use SEGV_*,
you need to use SIGSEGV, too.

With DO_ERROR_INFO(..., SIGSEGV, SEGV_ACCERR, ...); it looks good to me.


Jonathan Neuschäfer

[toc] | [prev] | [next] | [standalone]

#1684628 — Re: [patches] [PATCH 1/9] RISC-V: Init and Halt Code

On Fri, 07 Jul 2017 05:58:55 PDT (-0700), j.neuschaefer@gmx.net wrote:
> On Thu, Jul 06, 2017 at 03:34:39PM -0700, Palmer Dabbelt wrote:
>> On Tue, 04 Jul 2017 14:54:01 PDT (-0700), j.neuschaefer@gmx.net wrote:
> [...]
>> >> +#define DO_ERROR_INFO(name, signo, code, str)				\
>> >> +asmlinkage void name(struct pt_regs *regs)				\
>> >> +{									\
>> >> +	do_trap_error(regs, signo, code, regs->sepc, "Oops - " str);	\
>> >> +}
>> >> +
>> >> +DO_ERROR_INFO(do_trap_unknown,
>> >> +	SIGILL, ILL_ILLTRP, "unknown exception");
>> >> +DO_ERROR_INFO(do_trap_insn_misaligned,
>> >> +	SIGBUS, BUS_ADRALN, "instruction address misaligned");
>> >> +DO_ERROR_INFO(do_trap_insn_fault,
>> >> +	SIGBUS, BUS_ADRALN, "instruction access fault");
>> >
>> > For a general instruction access fault, BUS_ADRALN seems wrong. A
>> > variant of SIGSEGV seems more appropriate, IMHO.
>>
>> How does this look?
>>
>>   diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>>   index 4c693b5b9980..3ce9ac6e736e 100644
>>   --- a/arch/riscv/kernel/traps.c
>>   +++ b/arch/riscv/kernel/traps.c
>>   @@ -112,7 +112,7 @@ DO_ERROR_INFO(do_trap_unknown,
>>    DO_ERROR_INFO(do_trap_insn_misaligned,
>>           SIGBUS, BUS_ADRALN, "instruction address misaligned");
>>    DO_ERROR_INFO(do_trap_insn_fault,
>>   -       SIGBUS, BUS_ADRALN, "instruction access fault");
>>   +       SIGBUS, SEGV_ACCERR, "instruction access fault");
>>    DO_ERROR_INFO(do_trap_insn_illegal,
>>           SIGILL, ILL_ILLOPC, "illegal instruction");
>>    DO_ERROR_INFO(do_trap_load_misaligned,
>
> I'm not familiar with the trap handling infrastructure, but looking at
> include/uapi/asm-generic/siginfo.h, SEGV_ACCERR would alias to
> BUS_ADRERR (both are defined as (__SI_FAULT|2)). So if you use SEGV_*,
> you need to use SIGSEGV, too.
>
> With DO_ERROR_INFO(..., SIGSEGV, SEGV_ACCERR, ...); it looks good to me.

Oh, sorry, I wasn't paying attention.  It should be fixed now.

Thanks!

[toc] | [prev] | [next] | [standalone]

#1681378 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

On Tue, Jul 04, 2017 at 12:50:55PM -0700, Palmer Dabbelt wrote:
> +/*
> + * FIXME: I could only find documentation that atomic_{add,sub,inc,dec} are
> + * barrier-free.  I'm assuming that and/or/xor have the same constraints as the
> + * others.
> + */

Yes.. we have new documentation in the work to which I would post a link
but for some reason copy/paste stopped working again (Konsole does that
at times and is #$%#$%#4# annoying).

Ha, found it using google...

  https://marc.info/?l=linux-kernel&m=14972790112580


> +

> +/*
> + * 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.
> + */

cmpxchg triggers an extra rule; all conditional operations only need to
imply barriers on success. So a cmpxchg that fails, need not imply any
ordering what so ever.

> +/*
> + * Our atomic operations set the AQ and RL bits and therefor we don't need to
> + * fence around atomics.
> + */
> +#define __smb_mb__before_atomic()	barrier()
> +#define __smb_mb__after_atomic()	barrier()

Ah, not quite... you need full barriers here. Because your regular
atomic ops imply no ordering what so ever.

> +/*
> + * These barries are meant to prevent memory operations inside a spinlock from
> + * moving outside of that spinlock.  Since we set the AQ and RL bits when
> + * entering or leaving spinlocks, no additional fence needs to be performed.
> + */
> +#define smb_mb__before_spinlock()	barrier()
> +#define smb_mb__after_spinlock()	barrier()

Also probably not true. I _think_ you want a full barrier here, but
given the total lack of documentation on your end and the fact I've not
yet read the spinlock (which I suppose is below) I cannot yet state
more.


> +
> +/* FIXME: I don't think RISC-V is allowed to perform a speculative load. */
> +#define smp_acquire__after_ctrl_dep()	barrier()

That would be a very weird thing to disallow... speculative loads are
teh awesome ;-) Note you can get the very same effect from caches when
your stores are not globally atomic.

> +/*
> + * The RISC-V ISA doesn't support byte or half-word AMOs, so we fall back to a
> + * regular store and a fence here.  Otherwise we emit an AMO with an AQ or RL
> + * bit set and allow the microarchitecture to avoid the other half of the AMO.
> + */
> +#define __smp_store_release(p, v)					\
> +do {									\
> +	union { typeof(*p) __val; char __c[1]; } __u =			\
> +		{ .__val = (__force typeof(*p)) (v) };			\
> +	compiletime_assert_atomic_type(*p);				\
> +	switch (sizeof(*p)) {						\
> +	case 1:								\
> +	case 2:								\
> +		smb_mb();						\
> +		WRITE_ONCE(*p, __u.__val);				\
> +		break;							\
> +	case 4:								\
> +		__asm__ __volatile__ (					\
> +			"amoswap.w.rl zero, %1, %0"			\
> +			: "+A" (*p), "r" (__u.__val)			\
> +			: 						\
> +			: "memory");					\
> +		break;							\
> +	case 8:								\
> +		__asm__ __volatile__ (					\
> +			"amoswap.d.rl zero, %1, %0"			\
> +			: "+A" (*p), "r" (__u.__val)			\
> +			: 						\
> +			: "memory");					\
> +		break;							\
> +	}								\
> +} while (0)
> +
> +#define __smp_load_acquire(p)						\
> +do {									\
> +	union { typeof(*p) __val; char __c[1]; } __u =			\
> +		{ .__val = (__force typeof(*p)) (v) };			\
> +	compiletime_assert_atomic_type(*p);				\
> +	switch (sizeof(*p)) {						\
> +	case 1:								\
> +	case 2:								\
> +		__u.__val = READ_ONCE(*p);				\
> +		smb_mb();						\
> +		break;							\
> +	case 4:								\
> +		__asm__ __volatile__ (					\
> +			"amoor.w.aq %1, zero, %0"			\
> +			: "+A" (*p)					\
> +			: "=r" (__u.__val)				\
> +			: "memory");					\
> +		break;							\
> +	case 8:								\
> +		__asm__ __volatile__ (					\
> +			"amoor.d.aq %1, zero, %0"			\
> +			: "+A" (*p)					\
> +			: "=r" (__u.__val)				\
> +			: "memory");					\
> +		break;							\
> +	}								\
> +	__u.__val;							\
> +} while (0)

'creative' use of amoswap and amoor :-)

You should really look at a normal load with ordering instruction
though, that amoor.aq is a rmw and will promote the cacheline to
exclusive (and dirty it).

> +/*
> + * 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)
> +#define arch_spin_unlock_wait(x) \
> +		do { cpu_relax(); } while ((x)->lock)
> +
> +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;
> +	}
> +}

OK, so back to smp_mb__{before,after}_spinlock(), that wants to order
things like:

	wakeup:					block:

	COND = 1;				p->state = UNINTERRUPTIBLE;
						smp_mb();
	smp_mb__before_spinlock();
	spin_lock(&lock);			if (!COND)
						  schedule()
	if (p->state & state)
		goto out;


And here it is important that the COND store not happen _after_ the
p->state load.

Now, your spin_lock() only implies the AQ thing, which should only
constraint later load/stores but does nothing for the prior load/stores.
So our COND store can drop into the lock and even happen after the
p->state load.

So you very much want your smp_mb__{before,after}_spinlock thingies to
be full barriers.

[toc] | [prev] | [next] | [standalone]

#1682036 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jul 05, 2017 at 10:43:21AM +0200, Peter Zijlstra wrote:
> On Tue, Jul 04, 2017 at 12:50:55PM -0700, Palmer Dabbelt wrote:
> > +/*
> > + * FIXME: I could only find documentation that atomic_{add,sub,inc,dec} are
> > + * barrier-free.  I'm assuming that and/or/xor have the same constraints as the
> > + * others.
> > + */
> 
> Yes.. we have new documentation in the work to which I would post a link
> but for some reason copy/paste stopped working again (Konsole does that
> at times and is #$%#$%#4# annoying).
> 
> Ha, found it using google...
> 
>   https://marc.info/?l=linux-kernel&m=14972790112580
> 

The link is broken, you miss a tailing 1 ;-)

	https://marc.info/?l=linux-kernel&m=149727901125801

Regards,
Boqun

[toc] | [prev] | [next] | [standalone]

#1682117 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

On Thu, Jul 06, 2017 at 07:08:33PM +0800, Boqun Feng wrote:
> On Wed, Jul 05, 2017 at 10:43:21AM +0200, Peter Zijlstra wrote:
> > On Tue, Jul 04, 2017 at 12:50:55PM -0700, Palmer Dabbelt wrote:
> > > +/*
> > > + * FIXME: I could only find documentation that atomic_{add,sub,inc,dec} are
> > > + * barrier-free.  I'm assuming that and/or/xor have the same constraints as the
> > > + * others.
> > > + */
> > 
> > Yes.. we have new documentation in the work to which I would post a link
> > but for some reason copy/paste stopped working again (Konsole does that
> > at times and is #$%#$%#4# annoying).
> > 
> > Ha, found it using google...
> > 
> >   https://marc.info/?l=linux-kernel&m=14972790112580
> > 
> 
> The link is broken, you miss a tailing 1 ;-)
> 
> 	https://marc.info/?l=linux-kernel&m=149727901125801
> 

Argh... thanks for fixing that!

[toc] | [prev] | [next] | [standalone]

#1682847 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

On Wed, 05 Jul 2017 01:43:21 PDT (-0700), peterz@infradead.org wrote:
> On Tue, Jul 04, 2017 at 12:50:55PM -0700, Palmer Dabbelt wrote:
>> +/*
>> + * FIXME: I could only find documentation that atomic_{add,sub,inc,dec} are
>> + * barrier-free.  I'm assuming that and/or/xor have the same constraints as the
>> + * others.
>> + */
>
> Yes.. we have new documentation in the work to which I would post a link
> but for some reason copy/paste stopped working again (Konsole does that
> at times and is #$%#$%#4# annoying).
>
> Ha, found it using google...
>
>   https://marc.info/?l=linux-kernel&m=149727901125801

Thanks.

>> +/*
>> + * 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.
>> + */
>
> cmpxchg triggers an extra rule; all conditional operations only need to
> imply barriers on success. So a cmpxchg that fails, need not imply any
> ordering what so ever.
>
>> +/*
>> + * Our atomic operations set the AQ and RL bits and therefor we don't need to
>> + * fence around atomics.
>> + */
>> +#define __smb_mb__before_atomic()	barrier()
>> +#define __smb_mb__after_atomic()	barrier()
>
> Ah, not quite... you need full barriers here. Because your regular
> atomic ops imply no ordering what so ever.

The new documentation helps here, too.  Thanks!

  diff --git a/arch/riscv/include/asm/barrier.h b/arch/riscv/include/asm/barrier.h
  index 82a0092a86d0..a480c0fb85e5 100644
  --- a/arch/riscv/include/asm/barrier.h
  +++ b/arch/riscv/include/asm/barrier.h
  @@ -39,11 +39,19 @@
   #define smp_wmb()      RISCV_FENCE(w,w)

   /*
  - * Our atomic operations set the AQ and RL bits and therefor we don't need to
  - * fence around atomics.
  + * 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();
  + * "
  + * So we emit full fences on both sides.
    */
  -#define __smb_mb__before_atomic()      barrier()
  -#define __smb_mb__after_atomic()       barrier()
  +#define __smb_mb__before_atomic()      smp_mb()
  +#define __smb_mb__after_atomic()       smp_mb()

   /*
    * These barries are meant to prevent memory operations inside a spinlock from

>> +/*
>> + * These barries are meant to prevent memory operations inside a spinlock from
>> + * moving outside of that spinlock.  Since we set the AQ and RL bits when
>> + * entering or leaving spinlocks, no additional fence needs to be performed.
>> + */
>> +#define smb_mb__before_spinlock()	barrier()
>> +#define smb_mb__after_spinlock()	barrier()
>
> Also probably not true. I _think_ you want a full barrier here, but
> given the total lack of documentation on your end and the fact I've not
> yet read the spinlock (which I suppose is below) I cannot yet state
> more.

Ya, sorry about that -- we're waiting on a proper memory model spec.  Is there
any other documentation I should produce?

More below.

>> +
>> +/* FIXME: I don't think RISC-V is allowed to perform a speculative load. */
>> +#define smp_acquire__after_ctrl_dep()	barrier()
>
> That would be a very weird thing to disallow... speculative loads are
> teh awesome ;-) Note you can get the very same effect from caches when
> your stores are not globally atomic.

OK -- I guess generally the user ISA spec is written disregarding
microarchitecture, so I assumed this would be illegal.  We'll wait for the
memory model spec.

  diff --git a/arch/riscv/include/asm/barrier.h b/arch/riscv/include/asm/barrier.h
  index a4e54f4c17eb..c039333d4a5d 100644
  --- a/arch/riscv/include/asm/barrier.h
  +++ b/arch/riscv/include/asm/barrier.h
  @@ -61,8 +61,12 @@
   #define smb_mb__before_spinlock()      smp_mb()
   #define smb_mb__after_spinlock()       smp_mb()

  -/* FIXME: I don't think RISC-V is allowed to perform a speculative load. */
  -#define smp_acquire__after_ctrl_dep()  barrier()
  +/*
  + * TODO_RISCV_MEMORY_MODEL: I don't think RISC-V is allowed to perform a
  + * speculative load, but we're going to wait on a formal memory model in order
  + * to ensure this is safe to elide.
  + */
  +#define smp_acquire__after_ctrl_dep()  smp_mb()

   /*
    * The RISC-V ISA doesn't support byte or half-word AMOs, so we fall back to a
  @@ -137,24 +141,6 @@
          __u.__val;                                                      \
   })

  -/*
  - * The default implementation of this uses READ_ONCE and
  - * smp_acquire__after_ctrl_dep, but since we can directly do an ACQUIRE load we
  - * can avoid the extra barrier.
  - */
  -#define smp_cond_load_acquire(ptr, cond_expr) ({                       \
  -       typeof(ptr) __PTR = (ptr);                                      \
  -       typeof(*ptr) VAL;                                               \
  -       for (;;) {                                                      \
  -               VAL = __smp_load_acquire(__PTR);                        \
  -               if (cond_expr)                                          \
  -                       break;                                          \
  -               cpu_relax();                                            \
  -       }                                                               \
  -       smp_acquire__after_ctrl_dep();                                  \
  -       VAL;                                                            \
  -})
  -
   #include <asm-generic/barrier.h>

   #endif /* __ASSEMBLY__ */

>> +/*
>> + * The RISC-V ISA doesn't support byte or half-word AMOs, so we fall back to a
>> + * regular store and a fence here.  Otherwise we emit an AMO with an AQ or RL
>> + * bit set and allow the microarchitecture to avoid the other half of the AMO.
>> + */
>> +#define __smp_store_release(p, v)					\
>> +do {									\
>> +	union { typeof(*p) __val; char __c[1]; } __u =			\
>> +		{ .__val = (__force typeof(*p)) (v) };			\
>> +	compiletime_assert_atomic_type(*p);				\
>> +	switch (sizeof(*p)) {						\
>> +	case 1:								\
>> +	case 2:								\
>> +		smb_mb();						\
>> +		WRITE_ONCE(*p, __u.__val);				\
>> +		break;							\
>> +	case 4:								\
>> +		__asm__ __volatile__ (					\
>> +			"amoswap.w.rl zero, %1, %0"			\
>> +			: "+A" (*p), "r" (__u.__val)			\
>> +			: 						\
>> +			: "memory");					\
>> +		break;							\
>> +	case 8:								\
>> +		__asm__ __volatile__ (					\
>> +			"amoswap.d.rl zero, %1, %0"			\
>> +			: "+A" (*p), "r" (__u.__val)			\
>> +			: 						\
>> +			: "memory");					\
>> +		break;							\
>> +	}								\
>> +} while (0)
>> +
>> +#define __smp_load_acquire(p)						\
>> +do {									\
>> +	union { typeof(*p) __val; char __c[1]; } __u =			\
>> +		{ .__val = (__force typeof(*p)) (v) };			\
>> +	compiletime_assert_atomic_type(*p);				\
>> +	switch (sizeof(*p)) {						\
>> +	case 1:								\
>> +	case 2:								\
>> +		__u.__val = READ_ONCE(*p);				\
>> +		smb_mb();						\
>> +		break;							\
>> +	case 4:								\
>> +		__asm__ __volatile__ (					\
>> +			"amoor.w.aq %1, zero, %0"			\
>> +			: "+A" (*p)					\
>> +			: "=r" (__u.__val)				\
>> +			: "memory");					\
>> +		break;							\
>> +	case 8:								\
>> +		__asm__ __volatile__ (					\
>> +			"amoor.d.aq %1, zero, %0"			\
>> +			: "+A" (*p)					\
>> +			: "=r" (__u.__val)				\
>> +			: "memory");					\
>> +		break;							\
>> +	}								\
>> +	__u.__val;							\
>> +} while (0)
>
> 'creative' use of amoswap and amoor :-)
>
> You should really look at a normal load with ordering instruction
> though, that amoor.aq is a rmw and will promote the cacheline to
> exclusive (and dirty it).

The thought here was that implementations could elide the MW by pattern
matching the "zero" (x0, the architectural zero register) forms of AMOs where
it's interesting.  I talked to one of our microarchitecture guys, and while he
agrees that's easy he points out that eliding half the AMO may wreak havoc on
the consistency model.  Since we're not sure what the memory model is actually
going to look like, we thought it'd be best to just write the simplest code
here

  /*
   * TODO_RISCV_MEMORY_MODEL: While we could emit AMOs for the W and D sized
   * accesses here, it's questionable if that actually helps or not: the lack of
   * offsets in the AMOs means they're usually preceded by an addi, so they
   * probably won't save code space.  For now we'll just emit the fence.
   */
  #define __smp_store_release(p, v)                                       \
  ({                                                                      \
          compiletime_assert_atomic_type(*p);                             \
          smp_mb();                                                       \
          WRITE_ONCE(*p, v);                                              \
  })

  #define __smp_load_acquire(p)                                           \
  ({                                                                      \
          union{typeof(*p) __p; long __l;} __u;                           \
          compiletime_assert_atomic_type(*p);                             \
          __u.__l = READ_ONCE(*p);                                        \
          smp_mb();                                                       \
          __u.__p;                                                        \
  })

>> +/*
>> + * 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)
>> +#define arch_spin_unlock_wait(x) \
>> +		do { cpu_relax(); } while ((x)->lock)
>> +
>> +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;
>> +	}
>> +}
>
> OK, so back to smp_mb__{before,after}_spinlock(), that wants to order
> things like:
>
> 	wakeup:					block:
>
> 	COND = 1;				p->state = UNINTERRUPTIBLE;
> 						smp_mb();
> 	smp_mb__before_spinlock();
> 	spin_lock(&lock);			if (!COND)
> 						  schedule()
> 	if (p->state & state)
> 		goto out;
>
>
> And here it is important that the COND store not happen _after_ the
> p->state load.
>
> Now, your spin_lock() only implies the AQ thing, which should only
> constraint later load/stores but does nothing for the prior load/stores.
> So our COND store can drop into the lock and even happen after the
> p->state load.
>
> So you very much want your smp_mb__{before,after}_spinlock thingies to
> be full barriers.

OK, thanks!  I just had the movement direction backwards.  This makes much more
sense.

  diff --git a/arch/riscv/include/asm/barrier.h b/arch/riscv/include/asm/barrier.h
  index a480c0fb85e5..a4e54f4c17eb 100644
  --- a/arch/riscv/include/asm/barrier.h
  +++ b/arch/riscv/include/asm/barrier.h
  @@ -54,12 +54,12 @@
   #define __smb_mb__after_atomic()       smp_mb()

   /*
  - * These barries are meant to prevent memory operations inside a spinlock from
  - * moving outside of that spinlock.  Since we set the AQ and RL bits when
  - * entering or leaving spinlocks, no additional fence needs to be performed.
  + * These barries 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()      barrier()
  -#define smb_mb__after_spinlock()       barrier()
  +#define smb_mb__before_spinlock()      smp_mb()
  +#define smb_mb__after_spinlock()       smp_mb()

   /* FIXME: I don't think RISC-V is allowed to perform a speculative load. */
   #define smp_acquire__after_ctrl_dep()  barrier()

Thanks for all the help!

[toc] | [prev] | [next] | [standalone]

#1682875 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 06, 2017 at 06:04:13PM -0700, Palmer Dabbelt wrote:
[...]
> >> +#define __smp_load_acquire(p)						\
> >> +do {									\
> >> +	union { typeof(*p) __val; char __c[1]; } __u =			\
> >> +		{ .__val = (__force typeof(*p)) (v) };			\
> >> +	compiletime_assert_atomic_type(*p);				\
> >> +	switch (sizeof(*p)) {						\
> >> +	case 1:								\
> >> +	case 2:								\
> >> +		__u.__val = READ_ONCE(*p);				\
> >> +		smb_mb();						\
> >> +		break;							\
> >> +	case 4:								\
> >> +		__asm__ __volatile__ (					\
> >> +			"amoor.w.aq %1, zero, %0"			\
> >> +			: "+A" (*p)					\
> >> +			: "=r" (__u.__val)				\
> >> +			: "memory");					\
> >> +		break;							\
> >> +	case 8:								\
> >> +		__asm__ __volatile__ (					\
> >> +			"amoor.d.aq %1, zero, %0"			\
> >> +			: "+A" (*p)					\
> >> +			: "=r" (__u.__val)				\
> >> +			: "memory");					\
> >> +		break;							\
> >> +	}								\
> >> +	__u.__val;							\
> >> +} while (0)
> >
> > 'creative' use of amoswap and amoor :-)
> >
> > You should really look at a normal load with ordering instruction
> > though, that amoor.aq is a rmw and will promote the cacheline to
> > exclusive (and dirty it).
> 
> The thought here was that implementations could elide the MW by pattern
> matching the "zero" (x0, the architectural zero register) forms of AMOs where
> it's interesting.  I talked to one of our microarchitecture guys, and while he
> agrees that's easy he points out that eliding half the AMO may wreak havoc on
> the consistency model.  Since we're not sure what the memory model is actually
> going to look like, we thought it'd be best to just write the simplest code
> here
> 
>   /*
>    * TODO_RISCV_MEMORY_MODEL: While we could emit AMOs for the W and D sized
>    * accesses here, it's questionable if that actually helps or not: the lack of
>    * offsets in the AMOs means they're usually preceded by an addi, so they
>    * probably won't save code space.  For now we'll just emit the fence.
>    */
>   #define __smp_store_release(p, v)                                       \
>   ({                                                                      \
>           compiletime_assert_atomic_type(*p);                             \
>           smp_mb();                                                       \
>           WRITE_ONCE(*p, v);                                              \
>   })
> 
>   #define __smp_load_acquire(p)                                           \
>   ({                                                                      \
>           union{typeof(*p) __p; long __l;} __u;                           \

AFAICT, there seems to be an endian issue if you do this. No?

Let us assume typeof(*p) is char and *p == 1, and on a big endian 32bit
platform:

>           compiletime_assert_atomic_type(*p);                             \
>           __u.__l = READ_ONCE(*p);                                        \

	READ_ONCE(*p) is 1 so
	__u.__l is 0x00 00 00 01 now

>           smp_mb();                                                       \
>           __u.__p;                                                        \

	__u.__p is then 0x00.

Am I missing something here?

Even so why not use the simple definition as in include/asm-generic/barrier.h?

Regards,
Boqun



>   })
> 
[...]

[toc] | [prev] | [next] | [standalone]

#1684624 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

On Thu, 06 Jul 2017 19:14:25 PDT (-0700), boqun.feng@gmail.com wrote:
> On Thu, Jul 06, 2017 at 06:04:13PM -0700, Palmer Dabbelt wrote:
> [...]
>> >> +#define __smp_load_acquire(p)						\
>> >> +do {									\
>> >> +	union { typeof(*p) __val; char __c[1]; } __u =			\
>> >> +		{ .__val = (__force typeof(*p)) (v) };			\
>> >> +	compiletime_assert_atomic_type(*p);				\
>> >> +	switch (sizeof(*p)) {						\
>> >> +	case 1:								\
>> >> +	case 2:								\
>> >> +		__u.__val = READ_ONCE(*p);				\
>> >> +		smb_mb();						\
>> >> +		break;							\
>> >> +	case 4:								\
>> >> +		__asm__ __volatile__ (					\
>> >> +			"amoor.w.aq %1, zero, %0"			\
>> >> +			: "+A" (*p)					\
>> >> +			: "=r" (__u.__val)				\
>> >> +			: "memory");					\
>> >> +		break;							\
>> >> +	case 8:								\
>> >> +		__asm__ __volatile__ (					\
>> >> +			"amoor.d.aq %1, zero, %0"			\
>> >> +			: "+A" (*p)					\
>> >> +			: "=r" (__u.__val)				\
>> >> +			: "memory");					\
>> >> +		break;							\
>> >> +	}								\
>> >> +	__u.__val;							\
>> >> +} while (0)
>> >
>> > 'creative' use of amoswap and amoor :-)
>> >
>> > You should really look at a normal load with ordering instruction
>> > though, that amoor.aq is a rmw and will promote the cacheline to
>> > exclusive (and dirty it).
>>
>> The thought here was that implementations could elide the MW by pattern
>> matching the "zero" (x0, the architectural zero register) forms of AMOs where
>> it's interesting.  I talked to one of our microarchitecture guys, and while he
>> agrees that's easy he points out that eliding half the AMO may wreak havoc on
>> the consistency model.  Since we're not sure what the memory model is actually
>> going to look like, we thought it'd be best to just write the simplest code
>> here
>>
>>   /*
>>    * TODO_RISCV_MEMORY_MODEL: While we could emit AMOs for the W and D sized
>>    * accesses here, it's questionable if that actually helps or not: the lack of
>>    * offsets in the AMOs means they're usually preceded by an addi, so they
>>    * probably won't save code space.  For now we'll just emit the fence.
>>    */
>>   #define __smp_store_release(p, v)                                       \
>>   ({                                                                      \
>>           compiletime_assert_atomic_type(*p);                             \
>>           smp_mb();                                                       \
>>           WRITE_ONCE(*p, v);                                              \
>>   })
>>
>>   #define __smp_load_acquire(p)                                           \
>>   ({                                                                      \
>>           union{typeof(*p) __p; long __l;} __u;                           \
>
> AFAICT, there seems to be an endian issue if you do this. No?
>
> Let us assume typeof(*p) is char and *p == 1, and on a big endian 32bit
> platform:
>
>>           compiletime_assert_atomic_type(*p);                             \
>>           __u.__l = READ_ONCE(*p);                                        \
>
> 	READ_ONCE(*p) is 1 so
> 	__u.__l is 0x00 00 00 01 now
>
>>           smp_mb();                                                       \
>>           __u.__p;                                                        \
>
> 	__u.__p is then 0x00.
>
> Am I missing something here?

We're little endian (though I might have still screwed it up).  I didn't really
bother looking because...

> Even so why not use the simple definition as in include/asm-generic/barrier.h?

...that's much better -- I forgot there were generic versions, as we used to
have a much more complicated one.

  https://github.com/riscv/riscv-linux/commit/910d2bf4c3c349b670a1d839462e32e122ac70a5

Thanks!

[toc] | [prev] | [next] | [standalone]

#1683003 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

On Thu, Jul 06, 2017 at 06:04:13PM -0700, Palmer Dabbelt wrote:
> > Also probably not true. I _think_ you want a full barrier here, but
> > given the total lack of documentation on your end and the fact I've not
> > yet read the spinlock (which I suppose is below) I cannot yet state
> > more.
> 
> Ya, sorry about that -- we're waiting on a proper memory model spec.  Is there
> any other documentation I should produce?

Nah, I'll wait for your shiny new document.


>   +/*
>   + * TODO_RISCV_MEMORY_MODEL: I don't think RISC-V is allowed to perform a
>   + * speculative load, but we're going to wait on a formal memory model in order
>   + * to ensure this is safe to elide.
>   + */
>   +#define smp_acquire__after_ctrl_dep()  smp_mb()

So typically a control dependency already provides read->write ordering,
by virtue of speculative writes being BAD.

So a control dependency only needs to provide read->read ordering in
addition to the existing read->write ordering and hence this barrier is
typically a smp_rmb().

See the definition in asm-generic/barrier.h.

Having to use a full barrier here would imply your architecture does not
respect control dependencies, which would be BAD because we actually
rely on them.

So either the normal definition is good and you don't need to do
anything, or you prohibit read speculation in which case you have a
special case like TILE does.



> >> +#define __smp_load_acquire(p)						\
> >> +do {									\
> >> +	union { typeof(*p) __val; char __c[1]; } __u =			\
> >> +		{ .__val = (__force typeof(*p)) (v) };			\
> >> +	compiletime_assert_atomic_type(*p);				\
> >> +	switch (sizeof(*p)) {						\
> >> +	case 1:								\
> >> +	case 2:								\
> >> +		__u.__val = READ_ONCE(*p);				\
> >> +		smb_mb();						\
> >> +		break;							\
> >> +	case 4:								\
> >> +		__asm__ __volatile__ (					\
> >> +			"amoor.w.aq %1, zero, %0"			\
> >> +			: "+A" (*p)					\
> >> +			: "=r" (__u.__val)				\
> >> +			: "memory");					\
> >> +		break;							\
> >> +	case 8:								\
> >> +		__asm__ __volatile__ (					\
> >> +			"amoor.d.aq %1, zero, %0"			\
> >> +			: "+A" (*p)					\
> >> +			: "=r" (__u.__val)				\
> >> +			: "memory");					\
> >> +		break;							\
> >> +	}								\
> >> +	__u.__val;							\
> >> +} while (0)
> >
> > 'creative' use of amoswap and amoor :-)
> >
> > You should really look at a normal load with ordering instruction
> > though, that amoor.aq is a rmw and will promote the cacheline to
> > exclusive (and dirty it).
> 
> The thought here was that implementations could elide the MW by pattern
> matching the "zero" (x0, the architectural zero register) forms of AMOs where
> it's interesting.  I talked to one of our microarchitecture guys, and while he
> agrees that's easy he points out that eliding half the AMO may wreak havoc on
> the consistency model.  Since we're not sure what the memory model is actually
> going to look like, we thought it'd be best to just write the simplest code
> here
> 
>   /*
>    * TODO_RISCV_MEMORY_MODEL: While we could emit AMOs for the W and D sized
>    * accesses here, it's questionable if that actually helps or not: the lack of
>    * offsets in the AMOs means they're usually preceded by an addi, so they
>    * probably won't save code space.  For now we'll just emit the fence.
>    */
>   #define __smp_store_release(p, v)                                       \
>   ({                                                                      \
>           compiletime_assert_atomic_type(*p);                             \
>           smp_mb();                                                       \
>           WRITE_ONCE(*p, v);                                              \
>   })
> 
>   #define __smp_load_acquire(p)                                           \
>   ({                                                                      \
>           union{typeof(*p) __p; long __l;} __u;                           \
>           compiletime_assert_atomic_type(*p);                             \
>           __u.__l = READ_ONCE(*p);                                        \
>           smp_mb();                                                       \
>           __u.__p;                                                        \
>   })

Fair enough, that works.

> > OK, so back to smp_mb__{before,after}_spinlock(), that wants to order
> > things like:
> >
> > 	wakeup:					block:
> >
> > 	COND = 1;				p->state = UNINTERRUPTIBLE;
> > 						smp_mb();
> > 	smp_mb__before_spinlock();
> > 	spin_lock(&lock);			if (!COND)
> > 						  schedule()
> > 	if (p->state & state)
> > 		goto out;
> >
> >
> > And here it is important that the COND store not happen _after_ the
> > p->state load.
> >
> > Now, your spin_lock() only implies the AQ thing, which should only
> > constraint later load/stores but does nothing for the prior load/stores.
> > So our COND store can drop into the lock and even happen after the
> > p->state load.
> >
> > So you very much want your smp_mb__{before,after}_spinlock thingies to
> > be full barriers.
> 
> OK, thanks!  I just had the movement direction backwards.  This makes much more
> sense.
> 
>   diff --git a/arch/riscv/include/asm/barrier.h b/arch/riscv/include/asm/barrier.h
>   index a480c0fb85e5..a4e54f4c17eb 100644
>   --- a/arch/riscv/include/asm/barrier.h
>   +++ b/arch/riscv/include/asm/barrier.h
>   @@ -54,12 +54,12 @@
>    #define __smb_mb__after_atomic()       smp_mb()
> 
>    /*
>   - * These barries are meant to prevent memory operations inside a spinlock from
>   - * moving outside of that spinlock.  Since we set the AQ and RL bits when
>   - * entering or leaving spinlocks, no additional fence needs to be performed.
>   + * These barries 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()      barrier()
>   -#define smb_mb__after_spinlock()       barrier()
>   +#define smb_mb__before_spinlock()      smp_mb()
>   +#define smb_mb__after_spinlock()       smp_mb()
> 

Most excellent. Thanks!

[toc] | [prev] | [next] | [standalone]

#1684623 — Re: [PATCH 2/9] RISC-V: Atomic and Locking Code

On Fri, 07 Jul 2017 01:08:19 PDT (-0700), peterz@infradead.org wrote:
> On Thu, Jul 06, 2017 at 06:04:13PM -0700, Palmer Dabbelt wrote:
>>   +/*
>>   + * TODO_RISCV_MEMORY_MODEL: I don't think RISC-V is allowed to perform a
>>   + * speculative load, but we're going to wait on a formal memory model in order
>>   + * to ensure this is safe to elide.
>>   + */
>>   +#define smp_acquire__after_ctrl_dep()  smp_mb()
>
> So typically a control dependency already provides read->write ordering,
> by virtue of speculative writes being BAD.
>
> So a control dependency only needs to provide read->read ordering in
> addition to the existing read->write ordering and hence this barrier is
> typically a smp_rmb().
>
> See the definition in asm-generic/barrier.h.
>
> Having to use a full barrier here would imply your architecture does not
> respect control dependencies, which would be BAD because we actually
> rely on them.
>
> So either the normal definition is good and you don't need to do
> anything, or you prohibit read speculation in which case you have a
> special case like TILE does.

I'd be very surprised (and very unhappy) if we ended up with speculative
writes, as that would be a huge mess.

Thanks!

[toc] | [prev] | [next] | [standalone]

#1681434 — Re: [PATCH 8/9] RISC-V: User-facing API

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jul 04, 2017 at 12:51:01PM -0700, Palmer Dabbelt wrote:
> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> new file mode 100644
> index 000000000000..2720d5e97354
> --- /dev/null
> +++ b/arch/riscv/kernel/ptrace.c
> @@ -0,0 +1,138 @@

> +/* Put registers back to task. */
> +static void putregs(struct task_struct *child, struct pt_regs *uregs)
> +{
> +	struct pt_regs *regs = task_pt_regs(child);
> +	*regs = *uregs;
> +}
> +
> +static int riscv_gpr_get(struct task_struct *target,
> +			 const struct user_regset *regset,
> +			 unsigned int pos, unsigned int count,
> +			 void *kbuf, void __user *ubuf)
> +{
> +	struct pt_regs *regs;
> +
> +	regs = task_pt_regs(target);
> +	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, regs, 0,
> +				   sizeof(*regs));

sizeof(struct pt_regs) > sizeof(struct user_regs_struct), which allows
supervisor registers to be copied too. I think you should be using
sizeof(struct user_regs_struct) instead.

> +}
> +
> +static int riscv_gpr_set(struct task_struct *target,
> +			 const struct user_regset *regset,
> +			 unsigned int pos, unsigned int count,
> +			 const void *kbuf, const void __user *ubuf)
> +{
> +	int ret;
> +	struct pt_regs regs;
> +
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &regs, 0,
> +				 sizeof(regs));

same

> +	if (ret)
> +		return ret;
> +
> +	putregs(target, &regs);

you're still copying via the stack without initialising the non-written
fields. If userland does a short PTRACE_SETREGSET the remaining fields
will be copied from the uninitialised kernel stack and accessible to
userland via PTRACE_GETREGSET.

Even if the user does a full sizeof(struct user_regs_struct) (not
pt_regs) PTRACE_SETREGSET the supervisor registers will be overwritten
with uninitialised stack content.

> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
> new file mode 100644
> index 000000000000..4419604ff46c
> --- /dev/null
> +++ b/arch/riscv/kernel/sys_riscv.c

> +SYSCALL_DEFINE3(sysriscv_cmpxchg32, u32 __user *, ptr, u32, new, u32, old)
> +{
> +	u32 prev;
> +	unsigned int err;
> +
> +	if (!access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)))
> +		return -EFAULT;
> +
> +#ifdef CONFIG_ISA_A
> +	err = 0;
> +	prev = cmpxchg32(ptr, old, new);

I think this needs a special version of cmpxchg (or for cmpxchg to be
modified) with fixup protection to return -EFAULT in case the page isn't
mapped or is paged out/read only.

> +#else
> +	preempt_disable();
> +	err = __get_user(prev, ptr);
> +	if (likely(!err && prev == old))
> +		err = __put_user(new, ptr);
> +	preempt_enable();
> +#endif
> +
> +	return unlikely(err) ? err : prev;
> +}
> +
> +SYSCALL_DEFINE3(sysriscv_cmpxchg64, u64 __user *, ptr, u64, new, u64, old)
> +{
> +#ifdef CONFIG_64BIT
> +	u64 prev;
> +	unsigned int err;
> +
> +	if (!access_ok(VERIFY_WRITE, ptr, sizeof(*ptr)))
> +		return -EFAULT;
> +
> +#ifdef CONFIG_ISA_A
> +	err = 0;
> +	prev = cmpxchg64(ptr, old, new);

Likewise

> +#else
> +	preempt_disable();
> +	err = __get_user(prev, ptr);
> +	if (likely(!err && prev == old))
> +		err = __put_user(new, ptr);
> +	preempt_enable();
> +#endif
> +	return unlikely(err) ? err : prev;
> +#else
> +	return -ENOTSUPP;

I think -ENOSYS is more standard for missing/unimplemented system calls.

A better way IMO would be to #ifdef the definitions in unistd.h, then
the __NR_* definitions could also be more accurately extracted from the
kernel headers, and you could just ifdef CONFIG_64BIT the whole syscall
implementation, i.e.:

> diff --git a/arch/riscv/include/uapi/asm/unistd.h b/arch/riscv/include/uapi/asm/unistd.h
> new file mode 100644
> index 000000000000..37a5429cc896
> --- /dev/null
> +++ b/arch/riscv/include/uapi/asm/unistd.h
> @@ -0,0 +1,23 @@

> +/*
> + * These system calls add support for AMOs on RISC-V systems without support
> + * for the A extension.
> + */
> +#define __NR_sysriscv_cmpxchg32		(__NR_arch_specific_syscall + 0)
> +__SYSCALL(__NR_sysriscv_cmpxchg32, sys_sysriscv_cmpxchg32)

+ifdef WHATEVER_BUILTIN_GCC_DEFINES_FOR_64BIT_RISCV_ABI

In case its helpful the following should list builtin preprocessor
defines for given CFLAGS:
${CROSS_COMPILE}gcc ${CFLAGS} -dM -E -</dev/null

> +#define __NR_sysriscv_cmpxchg64		(__NR_arch_specific_syscall + 1)
> +__SYSCALL(__NR_sysriscv_cmpxchg64, sys_sysriscv_cmpxchg64)

+endif

Cheers
James

[toc] | [prev] | [next] | [standalone]

#1682013 — Re: [PATCH 8/9] RISC-V: User-facing API

I'm a bit concerned about these cmpxchg syscalls, and I'd like to
understand if my concerns are justified.

For a new instruction set that starts out in the 201x years we really
should have cmpxchg as a mandatory instruction - if not in the CPU
it should be in the Linux ABI so that we don't have to deal with a mess
where programs will either need an indirection for CPUs that have
cmpxchg capabilities vs those that don't, and we don't need to worry
if given binaries work on all CPUs.

What keeps from from declaring that the RISV-A extension is mandatory
for Linux?

[toc] | [prev] | [next] | [standalone]

#1682205 — Re: [PATCH 8/9] RISC-V: User-facing API

On Wed, Jul 05, 2017 at 07:01:41PM -0700, Christoph Hellwig wrote:
> I'm a bit concerned about these cmpxchg syscalls, and I'd like to
> understand if my concerns are justified.
> 
> For a new instruction set that starts out in the 201x years we really
> should have cmpxchg as a mandatory instruction - if not in the CPU
> it should be in the Linux ABI so that we don't have to deal with a mess
> where programs will either need an indirection for CPUs that have
> cmpxchg capabilities vs those that don't, and we don't need to worry
> if given binaries work on all CPUs.

Agreed on the indirection; it feels like this is something that should be in
the vDSO, which could use the cmpxchg instruction if it's available, or
otherwise just uses plain loads and stores. In the latter case, the kernel
can then detect if you're preempted in the vdso critical region and restart
the cmpxchg (we do something similar for arch/arm/, but it's slightly
simpler with the vectors page (see entry-armv.S).

Alternatively, we could revisit the restartable sequences work from Mathieu,
but I think that's stalled pending real-world performance data.

> What keeps from from declaring that the RISV-A extension is mandatory
> for Linux?

Or that!

Will

[toc] | [prev] | [next] | [standalone]

#1682520 — Re: [PATCH 8/9] RISC-V: User-facing API

On Thu, Jul 06, 2017 at 09:55:03AM +0100, Will Deacon wrote:
> Agreed on the indirection; it feels like this is something that should be in
> the vDSO, which could use the cmpxchg instruction if it's available, or
> otherwise just uses plain loads and stores.

Even that seems like a lot of indirection for something that is in
the critical fast path for synchronization.  I really can't understand
how a new ISA / ABI could even come up with an idea as stupid as making
essential synchronization primitives optional. 

[toc] | [prev] | [next] | [standalone]

#1682526 — Re: [PATCH 8/9] RISC-V: User-facing API

On Thu, Jul 06, 2017 at 08:34:27AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 06, 2017 at 09:55:03AM +0100, Will Deacon wrote:
> > Agreed on the indirection; it feels like this is something that should be in
> > the vDSO, which could use the cmpxchg instruction if it's available, or
> > otherwise just uses plain loads and stores.
> 
> Even that seems like a lot of indirection for something that is in
> the critical fast path for synchronization.  I really can't understand
> how a new ISA / ABI could even come up with an idea as stupid as making
> essential synchronization primitives optional. 

No disagreement there!

Will

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web