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

From: Boqun Feng <boqun.feng@gmail.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Jamie Cunliffe <Jamie.Cunliffe@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	rust-for-linux@vger.kernel.org, Miguel Ojeda <ojeda@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	steve.capper@arm.com, Asahi Lina <lina@asahilina.net>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-clk@vger.kernel.org
Subject: Re: [PATCH v4 2/2] arm64: rust: Enable Rust support for AArch64
Date: Wed, 25 Oct 2023 16:55:23 -0700	[thread overview]
Message-ID: <ZTmq67IEI42_tPoY@boqun-archlinux> (raw)
In-Reply-To: <d9ba6b65f70ff25df92fda4070bb58f8.sboyd@kernel.org>

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);
> 