Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Boqun Feng]

On Fri, Oct 20, 2023 at 07:21:08PM +0200, Andrew Lunn wrote:
> > +``arm64``     Maintained        Little Endian only.
> 
> This question is just out of curiosity, not the patchset itself.
> 
> What is missing to make big endian work?
> 

FWIW, I tried the following:

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 8784284988e5..b697c2d7da68 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -227,7 +227,7 @@ config ARM64
        select HAVE_FUNCTION_ARG_ACCESS_API
        select MMU_GATHER_RCU_TABLE_FREE
        select HAVE_RSEQ
-       select HAVE_RUST if CPU_LITTLE_ENDIAN
+       select HAVE_RUST
        select HAVE_STACKPROTECTOR
        select HAVE_SYSCALL_TRACEPOINTS
        select HAVE_KPROBES
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 4562a8173e90..4621f1e00e06 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -41,7 +41,11 @@ KBUILD_CFLAGS        += -mgeneral-regs-only  \
 KBUILD_CFLAGS  += $(call cc-disable-warning, psabi)
 KBUILD_AFLAGS  += $(compat_vdso)

+ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
+KBUILD_RUSTFLAGS += --target aarch64_be-unknown-linux-gnu -C target-feature="-neon"
+else
 KBUILD_RUSTFLAGS += --target aarch64-unknown-none -C target-feature="-neon"
+endif

 KBUILD_CFLAGS  += $(call cc-option,-mabi=lp64)
 KBUILD_AFLAGS  += $(call cc-option,-mabi=lp64)

and ran the following kunit command (it will run a few tests in a qemu
emulated VM):

	./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch arm64 --kconfig_add CONFIG_RUST=y --kconfig_add CONFIG_CPU_BIG_ENDIAN=y

The kernel was built successfully, and all Rust related tests passed.

Of course this doesn't mean a lot, we still need people with deep
Rust compiler knowledge to confirm whether the support is completed or
not. But I think if people want to do experiments, the tool is there.

P.S. An unrelated topic: I found a few clk related tests (in
drivers/clk/clk-gate_test.c IIUC) don't pass (mostly due to that the
expected values don't handle big endian), a sample failure output:

[11:13:26]     # clk_gate_test_enable: EXPECTATION FAILED at drivers/clk/clk-gate_test.c:169
[11:13:26]     Expected enable_val == ctx->fake_reg, but
[11:13:26]         enable_val == 32 (0x20)
[11:13:26]         ctx->fake_reg == 536870912 (0x20000000)
[11:13:26] clk_unregister: unregistering prepared clock: test_gate
[11:13:26] clk_unregister: unregistering prepared clock: test_parent
[11:13:26] [FAILED] clk_gate_test_enable

(Cc clk folks)

Regards,
Boqun

> Network developers have expressed an interesting in testing Rust code
> on big endian systems, since the code should work on both endians. It
> might be easier to get an ARM board running big endian than get access
> to an S390x machine.
> 
>       Andrew

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Andrew Lunn]

> Of course this doesn't mean a lot, we still need people with deep
> Rust compiler knowledge to confirm whether the support is completed or
> not. But I think if people want to do experiments, the tool is there.

Thanks for testing this, its good to know.

       Andrew

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Alice Ryhl]

On 10/20/23 20:33, Boqun Feng wrote:
> On Fri, Oct 20, 2023 at 07:21:08PM +0200, Andrew Lunn wrote:
>>> +``arm64``     Maintained        Little Endian only.
>>
>> This question is just out of curiosity, not the patchset itself.
>>
>> What is missing to make big endian work?
>>
> 
> FWIW, I tried the following:
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 8784284988e5..b697c2d7da68 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -227,7 +227,7 @@ config ARM64
>          select HAVE_FUNCTION_ARG_ACCESS_API
>          select MMU_GATHER_RCU_TABLE_FREE
>          select HAVE_RSEQ
> -       select HAVE_RUST if CPU_LITTLE_ENDIAN
> +       select HAVE_RUST
>          select HAVE_STACKPROTECTOR
>          select HAVE_SYSCALL_TRACEPOINTS
>          select HAVE_KPROBES
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 4562a8173e90..4621f1e00e06 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -41,7 +41,11 @@ KBUILD_CFLAGS        += -mgeneral-regs-only  \
>   KBUILD_CFLAGS  += $(call cc-disable-warning, psabi)
>   KBUILD_AFLAGS  += $(compat_vdso)
> 
> +ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
> +KBUILD_RUSTFLAGS += --target aarch64_be-unknown-linux-gnu -C target-feature="-neon"
> +else
>   KBUILD_RUSTFLAGS += --target aarch64-unknown-none -C target-feature="-neon"
> +endif
> 
>   KBUILD_CFLAGS  += $(call cc-option,-mabi=lp64)
>   KBUILD_AFLAGS  += $(call cc-option,-mabi=lp64)
> 
> and ran the following kunit command (it will run a few tests in a qemu
> emulated VM):
> 
> 	./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch arm64 --kconfig_add CONFIG_RUST=y --kconfig_add CONFIG_CPU_BIG_ENDIAN=y
> 
> The kernel was built successfully, and all Rust related tests passed.
> 
> Of course this doesn't mean a lot, we still need people with deep
> Rust compiler knowledge to confirm whether the support is completed or
> not. But I think if people want to do experiments, the tool is there.

For what it's worth, I have some experience with big endian Rust in 
userspace, and it generally works without issues. I would expect that it 
doesn't need any special support beyond setting the target architecture 
properly.

Alice

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Miguel Ojeda]

On Fri, Oct 20, 2023 at 8:33 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> But I think if people want to do experiments, the tool is there.

Yeah. If the Arm maintainers are OK adding it, I think we should also
put it in (next cycle perhaps) -- the sooner we have it in-tree, the
more testing it will hopefully get over time, and we can eventually
ask the CIs to add a run with it if they have the resources.

Cheers,
Miguel

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Andrew Lunn]

On Sat, Oct 21, 2023 at 03:41:54PM +0200, Miguel Ojeda wrote:
> On Fri, Oct 20, 2023 at 8:33 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > But I think if people want to do experiments, the tool is there.
> 
> Yeah. If the Arm maintainers are OK adding it, I think we should also
> put it in (next cycle perhaps) -- the sooner we have it in-tree, the
> more testing it will hopefully get over time, and we can eventually
> ask the CIs to add a run with it if they have the resources.

As i said elsewhere, endiannes is interesting for networking.

Maybe at the netdev conference next month, how to handle endiannes can
be part of the presentation? I'm guessing, but is it part of the type
system? So long as a type is marked as 'cpu endian', little endian, or
big endian, the compiler will take care of adding the needed swaps?

    Andrew

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Miguel Ojeda]

On Sat, Oct 21, 2023 at 6:03 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Maybe at the netdev conference next month, how to handle endiannes can
> be part of the presentation? I'm guessing, but is it part of the type
> system? So long as a type is marked as 'cpu endian', little endian, or
> big endian, the compiler will take care of adding the needed swaps?

Yeah, the type system can take care of ensuring that one does not
forget which type of quantity one is handling.

For instance, in this out-of-tree commit [1] from Wedson a `LE`
generic is added, which allows you to use e.g. `LE<u32>` in your
structs:

    struct Example {
        a: LE<u32>,
        b: LE<u32>,
    }

    fn sum(e: &Example) -> u32 {
        // `value` extracts the value in cpu representation.
        e.a.value() + e.b.value()
    }

Relatedly, the primitive integer types also provide "raw"
functionality for endianness, e.g. the `to_{le,be}` and
`to_{le,be,ne}_bytes` methods [2][3], and there are third-party crates
on this, e.g. [4].

[1] https://github.com/wedsonaf/linux/commit/ca4a93caff8b96a54a68fb052959801468bce01a
[2] https://doc.rust-lang.org/std/primitive.u32.html#method.to_le
[3] https://doc.rust-lang.org/std/primitive.u32.html#method.to_be_bytes
[4] https://docs.rs/byteorder/latest/byteorder/

Cheers,
Miguel

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Stephen Boyd]

Quoting Boqun Feng (2023-10-20 11:33:10)
> On Fri, Oct 20, 2023 at 07:21:08PM +0200, Andrew Lunn wrote:
> > > +``arm64``     Maintained        Little Endian only.
> > 
> > This question is just out of curiosity, not the patchset itself.
> > 
> > What is missing to make big endian work?
> > 
> 
> FWIW, I tried the following:
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 8784284988e5..b697c2d7da68 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -227,7 +227,7 @@ config ARM64
>         select HAVE_FUNCTION_ARG_ACCESS_API
>         select MMU_GATHER_RCU_TABLE_FREE
>         select HAVE_RSEQ
> -       select HAVE_RUST if CPU_LITTLE_ENDIAN
> +       select HAVE_RUST
>         select HAVE_STACKPROTECTOR
>         select HAVE_SYSCALL_TRACEPOINTS
>         select HAVE_KPROBES
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 4562a8173e90..4621f1e00e06 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -41,7 +41,11 @@ KBUILD_CFLAGS        += -mgeneral-regs-only  \
>  KBUILD_CFLAGS  += $(call cc-disable-warning, psabi)
>  KBUILD_AFLAGS  += $(compat_vdso)
> 
> +ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
> +KBUILD_RUSTFLAGS += --target aarch64_be-unknown-linux-gnu -C target-feature="-neon"
> +else
>  KBUILD_RUSTFLAGS += --target aarch64-unknown-none -C target-feature="-neon"
> +endif
> 
>  KBUILD_CFLAGS  += $(call cc-option,-mabi=lp64)
>  KBUILD_AFLAGS  += $(call cc-option,-mabi=lp64)
> 
> and ran the following kunit command (it will run a few tests in a qemu
> emulated VM):
> 
>         ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch arm64 --kconfig_add CONFIG_RUST=y --kconfig_add CONFIG_CPU_BIG_ENDIAN=y
> 
> The kernel was built successfully, and all Rust related tests passed.
> 
> Of course this doesn't mean a lot, we still need people with deep
> Rust compiler knowledge to confirm whether the support is completed or
> not. But I think if people want to do experiments, the tool is there.
> 
> P.S. An unrelated topic: I found a few clk related tests (in
> drivers/clk/clk-gate_test.c IIUC) don't pass (mostly due to that the
> expected values don't handle big endian), a sample failure output:
> 
> [11:13:26]     # clk_gate_test_enable: EXPECTATION FAILED at drivers/clk/clk-gate_test.c:169
> [11:13:26]     Expected enable_val == ctx->fake_reg, but
> [11:13:26]         enable_val == 32 (0x20)
> [11:13:26]         ctx->fake_reg == 536870912 (0x20000000)
> [11:13:26] clk_unregister: unregistering prepared clock: test_gate
> [11:13:26] clk_unregister: unregistering prepared clock: test_parent
> [11:13:26] [FAILED] clk_gate_test_enable
> 
> (Cc clk folks)
> 

Thanks for the report! We should treat it as an __le32 for now. I'll
have to add tests for the big endian flag as well. Does this fix it?

---8<----
diff --git a/drivers/clk/clk-gate_test.c b/drivers/clk/clk-gate_test.c
index e136aaad48bf..c96d93b19ddf 100644
--- a/drivers/clk/clk-gate_test.c
+++ b/drivers/clk/clk-gate_test.c
@@ -131,7 +131,7 @@ struct clk_gate_test_context {
 	void __iomem *fake_mem;
 	struct clk_hw *hw;
 	struct clk_hw *parent;
-	u32 fake_reg; /* Keep at end, KASAN can detect out of bounds */
+	__le32 fake_reg; /* Keep at end, KASAN can detect out of bounds */
 };
 
 static struct clk_gate_test_context *clk_gate_test_alloc_ctx(struct kunit *test)
@@ -166,7 +166,7 @@ static void clk_gate_test_enable(struct kunit *test)
 
 	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
 
-	KUNIT_EXPECT_EQ(test, enable_val, ctx->fake_reg);
+	KUNIT_EXPECT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(hw));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(hw));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(parent));
@@ -183,10 +183,10 @@ static void clk_gate_test_disable(struct kunit *test)
 	u32 disable_val = 0;
 
 	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
-	KUNIT_ASSERT_EQ(test, enable_val, ctx->fake_reg);
+	KUNIT_ASSERT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
 
 	clk_disable_unprepare(clk);
-	KUNIT_EXPECT_EQ(test, disable_val, ctx->fake_reg);
+	KUNIT_EXPECT_EQ(test, disable_val, le32_to_cpu(ctx->fake_reg));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(hw));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(hw));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(parent));
@@ -246,7 +246,7 @@ static void clk_gate_test_invert_enable(struct kunit *test)
 
 	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
 
-	KUNIT_EXPECT_EQ(test, enable_val, ctx->fake_reg);
+	KUNIT_EXPECT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(hw));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(hw));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(parent));
@@ -263,10 +263,10 @@ static void clk_gate_test_invert_disable(struct kunit *test)
 	u32 disable_val = BIT(15);
 
 	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
-	KUNIT_ASSERT_EQ(test, enable_val, ctx->fake_reg);
+	KUNIT_ASSERT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
 
 	clk_disable_unprepare(clk);
-	KUNIT_EXPECT_EQ(test, disable_val, ctx->fake_reg);
+	KUNIT_EXPECT_EQ(test, disable_val, le32_to_cpu(ctx->fake_reg));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(hw));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(hw));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(parent));
@@ -290,7 +290,7 @@ static int clk_gate_test_invert_init(struct kunit *test)
 					    2000000);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
 
-	ctx->fake_reg = BIT(15); /* Default to off */
+	ctx->fake_reg = cpu_to_le32(BIT(15)); /* Default to off */
 	hw = clk_hw_register_gate_parent_hw(NULL, "test_gate", parent, 0,
 					    ctx->fake_mem, 15,
 					    CLK_GATE_SET_TO_DISABLE, NULL);
@@ -319,7 +319,7 @@ static void clk_gate_test_hiword_enable(struct kunit *test)
 
 	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
 
-	KUNIT_EXPECT_EQ(test, enable_val, ctx->fake_reg);
+	KUNIT_EXPECT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(hw));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(hw));
 	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(parent));
@@ -336,10 +336,10 @@ static void clk_gate_test_hiword_disable(struct kunit *test)
 	u32 disable_val = BIT(9 + 16);
 
 	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
-	KUNIT_ASSERT_EQ(test, enable_val, ctx->fake_reg);
+	KUNIT_ASSERT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
 
 	clk_disable_unprepare(clk);
-	KUNIT_EXPECT_EQ(test, disable_val, ctx->fake_reg);
+	KUNIT_EXPECT_EQ(test, disable_val, le32_to_cpu(ctx->fake_reg));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(hw));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(hw));
 	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(parent));
@@ -387,7 +387,7 @@ static void clk_gate_test_is_enabled(struct kunit *test)
 	struct clk_gate_test_context *ctx;
 
 	ctx = clk_gate_test_alloc_ctx(test);
-	ctx->fake_reg = BIT(7);
+	ctx->fake_reg = cpu_to_le32(BIT(7));
 	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 7,
 				  0, NULL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
@@ -402,7 +402,7 @@ static void clk_gate_test_is_disabled(struct kunit *test)
 	struct clk_gate_test_context *ctx;
 
 	ctx = clk_gate_test_alloc_ctx(test);
-	ctx->fake_reg = BIT(4);
+	ctx->fake_reg = cpu_to_le32(BIT(4));
 	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 7,
 				  0, NULL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
@@ -417,7 +417,7 @@ static void clk_gate_test_is_enabled_inverted(struct kunit *test)
 	struct clk_gate_test_context *ctx;
 
 	ctx = clk_gate_test_alloc_ctx(test);
-	ctx->fake_reg = BIT(31);
+	ctx->fake_reg = cpu_to_le32(BIT(31));
 	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 2,
 				  CLK_GATE_SET_TO_DISABLE, NULL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
@@ -432,7 +432,7 @@ static void clk_gate_test_is_disabled_inverted(struct kunit *test)
 	struct clk_gate_test_context *ctx;
 
 	ctx = clk_gate_test_alloc_ctx(test);
-	ctx->fake_reg = BIT(29);
+	ctx->fake_reg = cpu_to_le32(BIT(29));
 	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 29,
 				  CLK_GATE_SET_TO_DISABLE, NULL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Boqun Feng]

On Mon, Oct 23, 2023 at 05:57:59PM -0700, Stephen Boyd wrote:
> Quoting Boqun Feng (2023-10-20 11:33:10)
> > On Fri, Oct 20, 2023 at 07:21:08PM +0200, Andrew Lunn wrote:
> > > > +``arm64``     Maintained        Little Endian only.
> > > 
> > > This question is just out of curiosity, not the patchset itself.
> > > 
> > > What is missing to make big endian work?
> > > 
> > 
> > FWIW, I tried the following:
> > 
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 8784284988e5..b697c2d7da68 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -227,7 +227,7 @@ config ARM64
> >         select HAVE_FUNCTION_ARG_ACCESS_API
> >         select MMU_GATHER_RCU_TABLE_FREE
> >         select HAVE_RSEQ
> > -       select HAVE_RUST if CPU_LITTLE_ENDIAN
> > +       select HAVE_RUST
> >         select HAVE_STACKPROTECTOR
> >         select HAVE_SYSCALL_TRACEPOINTS
> >         select HAVE_KPROBES
> > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> > index 4562a8173e90..4621f1e00e06 100644
> > --- a/arch/arm64/Makefile
> > +++ b/arch/arm64/Makefile
> > @@ -41,7 +41,11 @@ KBUILD_CFLAGS        += -mgeneral-regs-only  \
> >  KBUILD_CFLAGS  += $(call cc-disable-warning, psabi)
> >  KBUILD_AFLAGS  += $(compat_vdso)
> > 
> > +ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
> > +KBUILD_RUSTFLAGS += --target aarch64_be-unknown-linux-gnu -C target-feature="-neon"
> > +else
> >  KBUILD_RUSTFLAGS += --target aarch64-unknown-none -C target-feature="-neon"
> > +endif
> > 
> >  KBUILD_CFLAGS  += $(call cc-option,-mabi=lp64)
> >  KBUILD_AFLAGS  += $(call cc-option,-mabi=lp64)
> > 
> > and ran the following kunit command (it will run a few tests in a qemu
> > emulated VM):
> > 
> >         ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch arm64 --kconfig_add CONFIG_RUST=y --kconfig_add CONFIG_CPU_BIG_ENDIAN=y
> > 
> > The kernel was built successfully, and all Rust related tests passed.
> > 
> > Of course this doesn't mean a lot, we still need people with deep
> > Rust compiler knowledge to confirm whether the support is completed or
> > not. But I think if people want to do experiments, the tool is there.
> > 
> > P.S. An unrelated topic: I found a few clk related tests (in
> > drivers/clk/clk-gate_test.c IIUC) don't pass (mostly due to that the
> > expected values don't handle big endian), a sample failure output:
> > 
> > [11:13:26]     # clk_gate_test_enable: EXPECTATION FAILED at drivers/clk/clk-gate_test.c:169
> > [11:13:26]     Expected enable_val == ctx->fake_reg, but
> > [11:13:26]         enable_val == 32 (0x20)
> > [11:13:26]         ctx->fake_reg == 536870912 (0x20000000)
> > [11:13:26] clk_unregister: unregistering prepared clock: test_gate
> > [11:13:26] clk_unregister: unregistering prepared clock: test_parent
> > [11:13:26] [FAILED] clk_gate_test_enable
> > 
> > (Cc clk folks)
> > 
> 
> Thanks for the report! We should treat it as an __le32 for now. I'll
> have to add tests for the big endian flag as well. Does this fix it?

Yep! I just tested with kunit, and all the tests pass.

Feel free to add:

Tested-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> 
> ---8<----
> diff --git a/drivers/clk/clk-gate_test.c b/drivers/clk/clk-gate_test.c
> index e136aaad48bf..c96d93b19ddf 100644
> --- a/drivers/clk/clk-gate_test.c
> +++ b/drivers/clk/clk-gate_test.c
> @@ -131,7 +131,7 @@ struct clk_gate_test_context {
>  	void __iomem *fake_mem;
>  	struct clk_hw *hw;
>  	struct clk_hw *parent;
> -	u32 fake_reg; /* Keep at end, KASAN can detect out of bounds */
> +	__le32 fake_reg; /* Keep at end, KASAN can detect out of bounds */
>  };
>  
>  static struct clk_gate_test_context *clk_gate_test_alloc_ctx(struct kunit *test)
> @@ -166,7 +166,7 @@ static void clk_gate_test_enable(struct kunit *test)
>  
>  	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
>  
> -	KUNIT_EXPECT_EQ(test, enable_val, ctx->fake_reg);
> +	KUNIT_EXPECT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(hw));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(hw));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(parent));
> @@ -183,10 +183,10 @@ static void clk_gate_test_disable(struct kunit *test)
>  	u32 disable_val = 0;
>  
>  	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
> -	KUNIT_ASSERT_EQ(test, enable_val, ctx->fake_reg);
> +	KUNIT_ASSERT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
>  
>  	clk_disable_unprepare(clk);
> -	KUNIT_EXPECT_EQ(test, disable_val, ctx->fake_reg);
> +	KUNIT_EXPECT_EQ(test, disable_val, le32_to_cpu(ctx->fake_reg));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(hw));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(hw));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(parent));
> @@ -246,7 +246,7 @@ static void clk_gate_test_invert_enable(struct kunit *test)
>  
>  	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
>  
> -	KUNIT_EXPECT_EQ(test, enable_val, ctx->fake_reg);
> +	KUNIT_EXPECT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(hw));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(hw));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(parent));
> @@ -263,10 +263,10 @@ static void clk_gate_test_invert_disable(struct kunit *test)
>  	u32 disable_val = BIT(15);
>  
>  	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
> -	KUNIT_ASSERT_EQ(test, enable_val, ctx->fake_reg);
> +	KUNIT_ASSERT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
>  
>  	clk_disable_unprepare(clk);
> -	KUNIT_EXPECT_EQ(test, disable_val, ctx->fake_reg);
> +	KUNIT_EXPECT_EQ(test, disable_val, le32_to_cpu(ctx->fake_reg));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(hw));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(hw));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(parent));
> @@ -290,7 +290,7 @@ static int clk_gate_test_invert_init(struct kunit *test)
>  					    2000000);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
>  
> -	ctx->fake_reg = BIT(15); /* Default to off */
> +	ctx->fake_reg = cpu_to_le32(BIT(15)); /* Default to off */
>  	hw = clk_hw_register_gate_parent_hw(NULL, "test_gate", parent, 0,
>  					    ctx->fake_mem, 15,
>  					    CLK_GATE_SET_TO_DISABLE, NULL);
> @@ -319,7 +319,7 @@ static void clk_gate_test_hiword_enable(struct kunit *test)
>  
>  	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
>  
> -	KUNIT_EXPECT_EQ(test, enable_val, ctx->fake_reg);
> +	KUNIT_EXPECT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(hw));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_prepared(hw));
>  	KUNIT_EXPECT_TRUE(test, clk_hw_is_enabled(parent));
> @@ -336,10 +336,10 @@ static void clk_gate_test_hiword_disable(struct kunit *test)
>  	u32 disable_val = BIT(9 + 16);
>  
>  	KUNIT_ASSERT_EQ(test, clk_prepare_enable(clk), 0);
> -	KUNIT_ASSERT_EQ(test, enable_val, ctx->fake_reg);
> +	KUNIT_ASSERT_EQ(test, enable_val, le32_to_cpu(ctx->fake_reg));
>  
>  	clk_disable_unprepare(clk);
> -	KUNIT_EXPECT_EQ(test, disable_val, ctx->fake_reg);
> +	KUNIT_EXPECT_EQ(test, disable_val, le32_to_cpu(ctx->fake_reg));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(hw));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_prepared(hw));
>  	KUNIT_EXPECT_FALSE(test, clk_hw_is_enabled(parent));
> @@ -387,7 +387,7 @@ static void clk_gate_test_is_enabled(struct kunit *test)
>  	struct clk_gate_test_context *ctx;
>  
>  	ctx = clk_gate_test_alloc_ctx(test);
> -	ctx->fake_reg = BIT(7);
> +	ctx->fake_reg = cpu_to_le32(BIT(7));
>  	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 7,
>  				  0, NULL);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> @@ -402,7 +402,7 @@ static void clk_gate_test_is_disabled(struct kunit *test)
>  	struct clk_gate_test_context *ctx;
>  
>  	ctx = clk_gate_test_alloc_ctx(test);
> -	ctx->fake_reg = BIT(4);
> +	ctx->fake_reg = cpu_to_le32(BIT(4));
>  	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 7,
>  				  0, NULL);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> @@ -417,7 +417,7 @@ static void clk_gate_test_is_enabled_inverted(struct kunit *test)
>  	struct clk_gate_test_context *ctx;
>  
>  	ctx = clk_gate_test_alloc_ctx(test);
> -	ctx->fake_reg = BIT(31);
> +	ctx->fake_reg = cpu_to_le32(BIT(31));
>  	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 2,
>  				  CLK_GATE_SET_TO_DISABLE, NULL);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> @@ -432,7 +432,7 @@ static void clk_gate_test_is_disabled_inverted(struct kunit *test)
>  	struct clk_gate_test_context *ctx;
>  
>  	ctx = clk_gate_test_alloc_ctx(test);
> -	ctx->fake_reg = BIT(29);
> +	ctx->fake_reg = cpu_to_le32(BIT(29));
>  	hw = clk_hw_register_gate(NULL, "test_gate", NULL, 0, ctx->fake_mem, 29,
>  				  CLK_GATE_SET_TO_DISABLE, NULL);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw);
> 

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Linus Walleij]

[Andrew wrote]

> > Network developers have expressed an interesting in testing Rust code
> > on big endian systems, since the code should work on both endians. It
> > might be easier to get an ARM board running big endian than get access
> > to an S390x machine.

If you mean ARM32 BE I have that on the IXP4xx systems up and running
mainline kernels and mainline OpenWrt now, but I haven't
seen rust enablement for ARM32 yet (but it can't be that hard).

Yours,
Linus Walleij

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Pratham Patel]

On Fri, 20 Oct 2023 11:33:10 -0700, Boqun Feng wrote:
> FWIW, I tried the following:
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 8784284988e5..b697c2d7da68 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -227,7 +227,7 @@ config ARM64
>         select HAVE_FUNCTION_ARG_ACCESS_API
>         select MMU_GATHER_RCU_TABLE_FREE
>         select HAVE_RSEQ
> -       select HAVE_RUST if CPU_LITTLE_ENDIAN
> +       select HAVE_RUST
>         select HAVE_STACKPROTECTOR
>         select HAVE_SYSCALL_TRACEPOINTS
>         select HAVE_KPROBES
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 4562a8173e90..4621f1e00e06 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -41,7 +41,11 @@ KBUILD_CFLAGS        += -mgeneral-regs-only  \
>  KBUILD_CFLAGS  += $(call cc-disable-warning, psabi)
>  KBUILD_AFLAGS  += $(compat_vdso)
>
> +ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
> +KBUILD_RUSTFLAGS += --target aarch64_be-unknown-linux-gnu -C target-feature="-neon"
> +else
>  KBUILD_RUSTFLAGS += --target aarch64-unknown-none -C target-feature="-neon"
> +endif
>
>  KBUILD_CFLAGS  += $(call cc-option,-mabi=lp64)
>  KBUILD_AFLAGS  += $(call cc-option,-mabi=lp64)
>

This comes from a nobody (quite literally my first email on the kernel
mailing list), but I would rather that the support for AArch64 BE be
not added at all. My reasoning for this is because the rustc target
`aarch64_be-unknown-linux-gnu` is in the tier 3 of support. More
details can be found in the official documentation [0], but a few,
**select** key points from that section:

> At this tier, the Rust project provides no official support for a target, so we place minimal requirements on the introduction of targets.

> Tier 3 targets should attempt to implement as much of the standard libraries as possible and appropriate (core for most targets, alloc for targets that can support dynamic memory allocation, std for targets with an operating system or equivalent layer of system-provided functionality), but may leave some code unimplemented (either unavailable or stubbed out as appropriate), whether because the target makes it impossible to implement or challenging to implement. The authors of pull requests are not obligated to avoid calling any portions of the standard library on the basis of a tier 3 target not implementing those portions.

I am not an owner of any AArch64 BE machines, but if I were, this
would be concerning to _me_. I would prefer waiting for the Rust side
to be a bit more stable, at least in tier 2 support, for adding
AArch64 BE support. Though from the user of the Linux kernel, it may
not be that much of an issue, but from a developer's and packager's
perspective, this will lead to a few wasted hours until the
metaphorical person figures out why said Rust code failed to build for
AArch64 BE.

[0]: https://doc.rust-lang.org/rustc/target-tier-policy.html#tier-3-target-policy

P.S. This is my first time sending an email to a mailing list, sorry
if I mucked something up.

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Miguel Ojeda]

On Tue, Oct 24, 2023 at 4:18 PM Pratham Patel <thefirst1322@gmail.com> wrote:
>
> This comes from a nobody (quite literally my first email on the kernel
> mailing list), but I would rather that the support for AArch64 BE be
> not added at all. My reasoning for this is because the rustc target
> `aarch64_be-unknown-linux-gnu` is in the tier 3 of support.

We are aware of the tier list support, but please note that e.g. for
x86, we are not even using one of the built-in targets (for the moment
at least), and that we use a range of other unstable features. It is
under discussion between the two projects how these details will be
properly supported.

> but from a developer's and packager's
> perspective, this will lead to a few wasted hours until the
> metaphorical person figures out why said Rust code failed to build for
> AArch64 BE.

Why would the code fail to build? i.e. as far as I understand, Boqun
successfully compiled and QEMU-booted a BE image.

If you mean that it could stop compiling at any point because it is
not officially supported, then note that we pin the compiler, and that
it also applies to the rest of the code due to other unstable features
too, which is why we need to get the Rust project to support the
kernel properly for the use cases we need (not just targets, but also
features).

Also please note that, even if everything else was formally supported,
it would still be a good idea to add this support early (possibly with
a warning or gate or similar if not intended to be used by end users
yet) so that we have the first big-endian target around.

> P.S. This is my first time sending an email to a mailing list, sorry
> if I mucked something up.

The content looks fine :)

However, I think the proper `In-Reply-To` header is not there so Lore
gets confused -- did you use the suggested parameters at the bottom of
e.g. [1]?

[1] https://lore.kernel.org/rust-for-linux/CAM+6MoE1DyOay-fMWQP0hsenz174hSuGJnKM7symh0C-y1mqZw@mail.gmail.com/

Cheers,
Miguel

Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64

[Pratham Patel]

On Tue, 24 Oct 2023 at 20:49, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> > but from a developer's and packager's
> > perspective, this will lead to a few wasted hours until the
> > metaphorical person figures out why said Rust code failed to build for
> > AArch64 BE.
>
> Why would the code fail to build? i.e. as far as I understand, Boqun
> successfully compiled and QEMU-booted a BE image.
>
> If you mean that it could stop compiling at any point because it is
> not officially supported, then note that we pin the compiler, and that
> it also applies to the rest of the code due to other unstable features
> too, which is why we need to get the Rust project to support the
> kernel properly for the use cases we need (not just targets, but also
> features).

Yes, apologies for not clarifying it. I meant exactly that: the code
would stop compiling if either the target was dropped for any reason,
or supporting AArch64 BE would hold back upgrading the compiler for
other architectures. Though, this is from a very high level overview.
If you feel like this won't be an issue, I don't have any problems
with this :)

> Also please note that, even if everything else was formally supported,
> it would still be a good idea to add this support early (possibly with
> a warning or gate or similar if not intended to be used by end users
> yet) so that we have the first big-endian target around.


> > P.S. This is my first time sending an email to a mailing list, sorry
> > if I mucked something up.
>
> The content looks fine :)
>
> However, I think the proper `In-Reply-To` header is not there so Lore
> gets confused -- did you use the suggested parameters at the bottom of
> e.g. [1]?
>
> [1] https://lore.kernel.org/rust-for-linux/CAM+6MoE1DyOay-fMWQP0hsenz174hSuGJnKM7symh0C-y1mqZw@mail.gmail.com/

Yes, I appear to have missed copying the `In-Reply-To` header in this
reply thread. Sorry about that.