Re: [RFC PATCH 2/4] riscv: Cleanup ERRATA_THEAD_PBMT for rv32 svpbmt compile

From: "Heiko Stübner" <heiko@sntech.de>
To: palmer@rivosinc.com, linux-riscv@lists.infradead.org
Cc: linux-riscv@lists.infradead.org,
	Guo Ren <guoren@linux.alibaba.com>, Guo Ren <guoren@kernel.org>,
	guoren@kernel.org
Subject: Re: [RFC PATCH 2/4] riscv: Cleanup ERRATA_THEAD_PBMT for rv32 svpbmt compile
Date: Fri, 08 Jul 2022 10:50:53 +0200	[thread overview]
Message-ID: <3498257.R56niFO833@diego> (raw)
In-Reply-To: <20220705100523.1204595-3-guoren@kernel.org>

Hi Guo,

Am Dienstag, 5. Juli 2022, 12:05:21 CEST schrieb guoren@kernel.org:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> Make compile cleaner and don't reference the THEAD_PBMT data struct when
> CONFIG_ERRATA_THEAD_PBMT=y. Next, we could cleanly make svpbmt to
> support rv32.
> 
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
>  arch/riscv/include/asm/errata_list.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> index 416ead0f9a65..47175d91773d 100644
> --- a/arch/riscv/include/asm/errata_list.h
> +++ b/arch/riscv/include/asm/errata_list.h
> @@ -47,6 +47,8 @@ asm(ALTERNATIVE("sfence.vma %0", "sfence.vma", SIFIVE_VENDOR_ID,	\
>   * in the default case.
>   */
>  #define ALT_SVPBMT_SHIFT 61
> +
> +#ifdef CONFIG_ERRATA_THEAD_PBMT

I don't really think that is necessary and actually makes the code
more complex than needed.

Each alternative-entry already has the dependency on
CONFIG_* ... i.e. CONFIG_RISCV_ISA_SVPBMT and CONFIG_ERRATA_THEAD_PBMT

When you look at alternative-macros.h you'll see this translating to the
enable argument in ALT_NEW_CONTENT.

So only when that is active is the alternative section added to the build.
I.e. that translates to:

	.if IS_ENABLED(CONFIG_ERRATA_THEAD_PBMT)
	.pushsection .alternative, "a"
	...

So when CONFIG_ERRATA_THEAD_PBMT is disabled the whole alternative
part never gets added already, so there shouldn't be any need to make the
source more complicated.

Heiko

>  #define ALT_THEAD_PBMT_SHIFT 59
>  #define ALT_SVPBMT(_val, prot)						\
>  asm(ALTERNATIVE_2("li %0, 0\t\nnop",					\
> @@ -60,7 +62,6 @@ asm(ALTERNATIVE_2("li %0, 0\t\nnop",					\
>  		  "I"(ALT_SVPBMT_SHIFT),				\
>  		  "I"(ALT_THEAD_PBMT_SHIFT))
>  
> -#ifdef CONFIG_ERRATA_THEAD_PBMT
>  /*
>   * IO/NOCACHE memory types are handled together with svpbmt,
>   * so on T-Head chips, check if no other memory type is set,
> @@ -90,6 +91,14 @@ asm volatile(ALTERNATIVE(						\
>  	  "I"(ALT_THEAD_PBMT_SHIFT)					\
>  	: "t3")
>  #else
> +#define ALT_SVPBMT(_val, prot)						\
> +asm(ALTERNATIVE("li %0, 0\t\nnop",					\
> +		 "li %0, %1\t\nslli %0,%0,%2", 0,			\
> +			CPUFEATURE_SVPBMT, CONFIG_RISCV_ISA_SVPBMT)	\
> +		: "=r"(_val)						\
> +		: "I"(prot##_SVPBMT >> ALT_SVPBMT_SHIFT),		\
> +		  "I"(ALT_SVPBMT_SHIFT))
> +
>  #define ALT_THEAD_PMA(_val)
>  #endif
>  
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv