Re: [RFC PATCH v2 01/31] Documentation: kvx: Add basic documentation
On Wed, Jan 25, 2023 at 07:28:20PM +0100, Jules Maselbas wrote: > Hi Bagas, > > Thanks for taking your time and effort to improve the documentation. > We not only need to clean the documention syntax and wording but also > its content. I am tempted to apply all your proposed changes first and > then work on improving and correcting the documentation. > > However I am not very sure on how to integrate your changes and give > proper contribution attributions. Any insights on this would be greatly > appreciated. > Hi Jules, The reword diff can be squashed into the doc patch (here, [01/31]). For the attribution, since the reword is significant, Co-developed-by: Bagas Sanjaya Signed-off-by: Bagas Sanjaya Thanks. -- An old man doll... just what I always wanted! - Clara signature.asc Description: PGP signature -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 11/31] kvx: Add atomic/locking headers
On Thu, Jan 26, 2023 at 10:57:20AM +0100, Jules Maselbas wrote:
> Hi Mark,
...
> > > +static inline int arch_atomic_add_return(int i, atomic_t *v)
> > > +{
> > > + int new, old, ret;
> > > +
> > > + do {
> > > + old = v->counter;
> >
> > Likewise, arch_atomic64_read(v) here.
> ack, this will bt arch_atomic_read(v) here since this is not atomic64_t
> here.
I took a second look at this and I think we are not doing the right
thing, we do not need to defined arch_atomic_add_return at all since
we are including the generic atomic right after, which will define
the macro arch_atomic_add_return as generic_atomic_add_return
>
> Thanks
> -- Jules
>
>
>
>
>
>
>
>
>
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 01/31] Documentation: kvx: Add basic documentation
Hi Bagas, Thanks for taking your time and effort to improve the documentation. We not only need to clean the documention syntax and wording but also its content. I am tempted to apply all your proposed changes first and then work on improving and correcting the documentation. However I am not very sure on how to integrate your changes and give proper contribution attributions. Any insights on this would be greatly appreciated. Thanks -- Jules -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 12/31] kvx: Add other common headers
Hi Jason,
On Fri, Jan 20, 2023 at 03:29:14PM +0100, Jason A. Donenfeld wrote:
> Hi Yann,
>
> On Fri, Jan 20, 2023 at 03:09:43PM +0100, Yann Sionneau wrote:
> > +#include
> > +#include
> > +
> > +extern unsigned long __stack_chk_guard;
> > +
> > +/*
> > + * Initialize the stackprotector canary value.
> > + *
> > + * NOTE: this must only be called from functions that never return,
> > + * and it must always be inlined.
> > + */
> > +static __always_inline void boot_init_stack_canary(void)
> > +{
> > + unsigned long canary;
> > +
> > + /* Try to get a semi random initial value. */
> > + get_random_bytes(, sizeof(canary));
> > + canary ^= LINUX_VERSION_CODE;
> > + canary &= CANARY_MASK;
> > +
> > + current->stack_canary = canary;
> > + __stack_chk_guard = current->stack_canary;
> > +}
>
>
> You should rewrite this as:
>
> current->stack_canary = get_random_canary();
> __stack_chk_guard = current->stack_canary;
>
> which is what the other archs all now do. (They didn't used to, and this
> looks like it's simply based on older code.)
Thanks for the suggestion, this will be into v3
> > +#define get_cycles get_cycles
> > +
> > +#include
> > +#include
> > +
> > +static inline cycles_t get_cycles(void)
> > +{
> > + return kvx_sfr_get(PM0);
> > +}
>
> Glad to see this CPU has a cycle counter. Out of curiosity, what is
> its resolution?
This cpu has 4 performance monitor (PM), the first one is reserved to
count cycles, and it is cycle accurate.
> Also, related, does this CPU happen to have a "RDRAND"-like instruction?
I didn't knew about the RDRAND insruction, but no this CPU do not have
an instruction like that.
> (I don't know anything about kvx or even what it is.)
It's a VLIW core, a bit like Itanium, there are currently not publicly
available documentation. We have started a discussion internally at
Kalray to share more information regarding this CPU and its ABI.
A very crude instruction listing can be found in our fork of
gdb-binutils:
https://raw.githubusercontent.com/kalray/binutils/binutils-2_35_2/coolidge/opcodes/kv3-opc.c
Best regards,
-- Jules
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 11/31] kvx: Add atomic/locking headers
Hi Jules,
On Thu, Jan 26, 2023 at 10:57:20AM +0100, Jules Maselbas wrote:
> Hi Mark,
>
> On Fri, Jan 20, 2023 at 03:18:48PM +, Mark Rutland wrote:
> > On Fri, Jan 20, 2023 at 03:09:42PM +0100, Yann Sionneau wrote:
> > > +#define ATOMIC64_RETURN_OP(op, c_op)
> > > \
> > > +static inline long arch_atomic64_##op##_return(long i, atomic64_t *v)
> > > \
> > > +{
> > > \
> > > + long new, old, ret; \
> > > + \
> > > + do {\
> > > + old = v->counter; \
> >
> > This should be arch_atomic64_read(v), in order to avoid the potential for
> > the
> > compiler to replay the access and introduce ABA races and other such
> > problems.
> Thanks for the suggestion, this will be into v3.
>
> > For details, see:
> >
> > https://lore.kernel.org/lkml/Y70SWXHDmOc3RhMd@osiris/
> > https://lore.kernel.org/lkml/Y71LoCIl+IFdy9D8@FVFF77S0Q05N/
> >
> > I see that the generic 32-bit atomic code suffers from that issue, and we
> > should fix it.
> I took a look at the generic 32-bit atomic, but I am unsure if this
> needs to be done for both the SMP and non-SMP implementations. But I
> can send a first patch and we can discuss from there.
Sounds good to me; thanks!
[...]
> > > +static inline int arch_atomic_add_return(int i, atomic_t *v)
> > > +{
> > > + int new, old, ret;
> > > +
> > > + do {
> > > + old = v->counter;
> >
> > Likewise, arch_atomic64_read(v) here.
> ack, this will bt arch_atomic_read(v) here since this is not atomic64_t
> here.
Ah, yes, my bad!
Thanks,
Mark.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 02/31] Documentation: Add binding for kalray,kv3-1-core-intc
Hi Krzysztof,
On Sun, Jan 22, 2023 at 12:44:46PM +0100, Krzysztof Kozlowski wrote:
> On 20/01/2023 15:09, Yann Sionneau wrote:
> > From: Jules Maselbas
>
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
This will be fixed, sorry for the inconvenience.
>
> >
> > Add documentation for `kalray,kv3-1-core-intc` binding.
> >
> > Co-developed-by: Jules Maselbas
> > Signed-off-by: Jules Maselbas
> > Signed-off-by: Yann Sionneau
> > ---
> >
> > Notes:
> > V1 -> V2: new patch
> >
> > .../kalray,kv3-1-core-intc.yaml | 46 +++
> > 1 file changed, 46 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/interrupt-controller/kalray,kv3-1-core-intc.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/kalray,kv3-1-core-intc.yaml
> >
> > b/Documentation/devicetree/bindings/interrupt-controller/kalray,kv3-1-core-intc.yaml
> > new file mode 100644
> > index ..1e3d0593173a
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/interrupt-controller/kalray,kv3-1-core-intc.yaml
> > @@ -0,0 +1,46 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/interrupt-controller/kalray,kv3-1-core-intc#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Kalray kv3-1 Core Interrupt Controller
> > +
> > +description: |
> > + The Kalray Core Interrupt Controller is tightly integrated in each kv3
> > core
> > + present in the Coolidge SoC.
> > +
> > + It provides the following features:
> > + - 32 independent interrupt sources
> > + - 2-bit configurable priority level
> > + - 2-bit configurable ownership level
> > +
> > +allOf:
> > + - $ref: /schemas/interrupt-controller.yaml#
> > +
> > +properties:
> > + compatible:
> > +const: kalray,kv3-1-core-intc
>
> Blank line between each of these,
Ack
> > + "#interrupt-cells":
> > +const: 1
> > +description:
> > + The IRQ number.
> > + reg:
> > +maxItems: 0
>
> ??? No way... What's this?
This (per CPU) interrupt controller is not memory mapped at all, it is
controlled and configured through system registers.
I do not have found existing .yaml bindings for such devices, only the
file snps,archs-intc.txt has something similar.
I do not know what is the best way to represent such devices in the
device-tree. Any suggestions are welcome.
>
> > + "kalray,intc-nr-irqs":
>
> Drop quotes.
>
> > +description: Number of irqs handled by the controller.
>
> Why this is variable per board? Why do you need it ?
This property is not even used in our device-tree, this will be removed
from the documentation and from the driver as well.
> > +
> > +required:
> > + - compatible
> > + - "#interrupt-cells"
> > + - interrupt-controller
>
> missing additionalProperties: false
>
> This binding looks poor, like you started from something odd. Please
> don't. Take the newest reviewed binding or better example-schema and use
> it to build yours. This would solve several trivial mistakes and style
> issues.
I am starting over from the example-schema.
> > +
> > +examples:
> > + - |
> > +intc: interrupt-controller {
>
> What's the IO address space?
As said above, this is not a memory mapped device, but is accessed
through system registers.
Thanks,
-- Jules
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH v2 11/31] kvx: Add atomic/locking headers
Hi Mark,
On Fri, Jan 20, 2023 at 03:18:48PM +, Mark Rutland wrote:
> On Fri, Jan 20, 2023 at 03:09:42PM +0100, Yann Sionneau wrote:
> > Add common headers (atomic, bitops, barrier and locking) for basic
> > kvx support.
> >
> > Co-developed-by: Clement Leger
> > Signed-off-by: Clement Leger
> > Co-developed-by: Jules Maselbas
> > Signed-off-by: Jules Maselbas
> > Co-developed-by: Julian Vetter
> > Signed-off-by: Julian Vetter
> > Co-developed-by: Julien Villette
> > Signed-off-by: Julien Villette
> > Co-developed-by: Yann Sionneau
> > Signed-off-by: Yann Sionneau
> > ---
> >
> > Notes:
> > V1 -> V2:
> > - use {READ,WRITE}_ONCE for arch_atomic64_{read,set}
> > - use asm-generic/bitops/atomic.h instead of __test_and_*_bit
> > - removed duplicated includes
> > - rewrite xchg and cmpxchg in C using builtins for acswap insn
>
> Thanks for those changes. I see one issue below (instantiated a few times),
> but
> other than that this looks good to me.
>
> [...]
>
> > +#define ATOMIC64_RETURN_OP(op, c_op)
> > \
> > +static inline long arch_atomic64_##op##_return(long i, atomic64_t *v)
> > \
> > +{ \
> > + long new, old, ret; \
> > + \
> > + do {\
> > + old = v->counter; \
>
> This should be arch_atomic64_read(v), in order to avoid the potential for the
> compiler to replay the access and introduce ABA races and other such problems.
Thanks for the suggestion, this will be into v3.
> For details, see:
>
> https://lore.kernel.org/lkml/Y70SWXHDmOc3RhMd@osiris/
> https://lore.kernel.org/lkml/Y71LoCIl+IFdy9D8@FVFF77S0Q05N/
>
> I see that the generic 32-bit atomic code suffers from that issue, and we
> should fix it.
I took a look at the generic 32-bit atomic, but I am unsure if this
needs to be done for both the SMP and non-SMP implementations. But I
can send a first patch and we can discuss from there.
> > + new = old c_op i; \
> > + ret = arch_cmpxchg(>counter, old, new); \
> > + } while (ret != old); \
> > + \
> > + return new; \
> > +}
> > +
> > +#define ATOMIC64_OP(op, c_op)
> > \
> > +static inline void arch_atomic64_##op(long i, atomic64_t *v)
> > \
> > +{ \
> > + long new, old, ret; \
> > + \
> > + do {\
> > + old = v->counter; \
>
> Likewise, arch_atomic64_read(v) here.
ack
> > + new = old c_op i; \
> > + ret = arch_cmpxchg(>counter, old, new); \
> > + } while (ret != old); \
> > +}
> > +
> > +#define ATOMIC64_FETCH_OP(op, c_op)
> > \
> > +static inline long arch_atomic64_fetch_##op(long i, atomic64_t *v) \
> > +{ \
> > + long new, old, ret; \
> > + \
> > + do {\
> > + old = v->counter; \
>
> Likewise, arch_atomic64_read(v) here.
ack
> > + new = old c_op i; \
> > + ret = arch_cmpxchg(>counter, old, new); \
> > + } while (ret != old); \
> > + \
> > + return old; \
> > +}
> > +
> > +#define ATOMIC64_OPS(op, c_op)
> > \
> > + ATOMIC64_OP(op, c_op) \
> > + ATOMIC64_RETURN_OP(op, c_op)\
> > + ATOMIC64_FETCH_OP(op, c_op)
> > +
> > +ATOMIC64_OPS(and, &)
> > +ATOMIC64_OPS(or, |)
> > +ATOMIC64_OPS(xor, ^)
> > +ATOMIC64_OPS(add, +)
> > +ATOMIC64_OPS(sub, -)
> > +
> > +#undef ATOMIC64_OPS
> > +#undef ATOMIC64_FETCH_OP
> > +#undef ATOMIC64_OP
> > +
> > +static inline int arch_atomic_add_return(int i, atomic_t *v)
> > +{
> > + int new, old, ret;
> > +
> > + do {
> > +
