diff mbox series

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

Message ID 20220705100523.1204595-3-guoren@kernel.org (mailing list archive)
State New, archived
Headers show
Series Proof of concept for rv32 svpbmt support | expand

Commit Message

Guo Ren July 5, 2022, 10:05 a.m. UTC 
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(-)

Comments

Heiko Stuebner July 8, 2022, 8:50 a.m. UTC | #1 
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
>  
>
Guo Ren July 9, 2022, 4:24 p.m. UTC | #2 
On Fri, Jul 8, 2022 at 4:51 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> 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.
No, we need this for rv32 compile.

➜  linux git:(rv32svpbmt_v2_tmp) make ARCH=riscv
CROSS_COMPILE=riscv32-buildroot-linux-gnu- EXTRA_CFLAGS+=-g
O=../build-rv32/ -skj all
In file included from
/home/guoren/source/kernel/linux/arch/riscv/include/asm/pgtable.h:15,
                 from
/home/guoren/source/kernel/linux/include/linux/pgtable.h:6,
                 from /home/guoren/source/kernel/linux/include/linux/mm.h:29,
                 from
/home/guoren/source/kernel/linux/arch/riscv/kernel/asm-offsets.c:10:
/home/guoren/source/kernel/linux/arch/riscv/include/asm/errata_list.h:50:
warning: "ALT_SVPBMT_SHIFT" redefined
   50 | #define ALT_SVPBMT_SHIFT (__riscv_xlen-3)
      |
/home/guoren/source/kernel/linux/arch/riscv/include/asm/errata_list.h:49:
note: this is the location of the previous definition
   49 | #define ALT_SVPBMT_SHIFT 61
      |
/home/guoren/source/kernel/linux/arch/riscv/include/asm/pgtable-bits.h:
In function ‘riscv_page_mtmask’:
/home/guoren/source/kernel/linux/arch/riscv/include/asm/pgtable-bits.h:43:18:
error: ‘_PAGE_MTMASK_THEAD’ undeclared (first use in this function)
   43 |  ALT_SVPBMT(val, _PAGE_MTMASK);
      |                  ^~~~~~~~~~~~
/home/guoren/source/kernel/linux/arch/riscv/include/asm/errata_list.h:61:9:
note: in definition of macro ‘ALT_SVPBMT’
   61 |     "I"(prot##_THEAD >> ALT_THEAD_PBMT_SHIFT),  \
      |         ^~~~
/home/guoren/source/kernel/linux/arch/riscv/include/asm/pgtable-bits.h:43:18:
note: each undeclared identifier is reported only once for each
function it appears in
   43 |  ALT_SVPBMT(val, _PAGE_MTMASK);
      |                  ^~~~~~~~~~~~
/home/guoren/source/kernel/linux/arch/riscv/include/asm/errata_list.h:61:9:
note: in definition of macro ‘ALT_SVPBMT’
   61 |     "I"(prot##_THEAD >> ALT_THEAD_PBMT_SHIFT),  \
      |         ^~~~
/home/guoren/source/kernel/linux/arch/riscv/include/asm/pgtable-bits.h:
In function ‘riscv_page_nocache’:
/home/guoren/source/kernel/linux/arch/riscv/include/asm/pgtable-bits.h:51:18:
error: ‘_PAGE_NOCACHE_THEAD’ undeclared (first use in this function)
   51 |  ALT_SVPBMT(val, _PAGE_NOCACHE);
      |                  ^~~~~~~~~~~~~
/home/guoren/source/kernel/linux/arch/riscv/include/asm/errata_list.h:61:9:
note: in definition of macro ‘ALT_SVPBMT’
   61 |     "I"(prot##_THEAD >> ALT_THEAD_PBMT_SHIFT),  \
      |         ^~~~
/home/guoren/source/kernel/linux/arch/riscv/include/asm/pgtable-bits.h:
In function ‘riscv_page_io’:
/home/guoren/source/kernel/linux/arch/riscv/include/asm/pgtable-bits.h:59:18:
error: ‘_PAGE_IO_THEAD’ undeclared (first use in this function)
   59 |  ALT_SVPBMT(val, _PAGE_IO);
      |                  ^~~~~~~~
/home/guoren/source/kernel/linux/arch/riscv/include/asm/errata_list.h:61:9:
note: in definition of macro ‘ALT_SVPBMT’
   61 |     "I"(prot##_THEAD >> ALT_THEAD_PBMT_SHIFT),  \
      |         ^~~~
make[2]: *** [/home/guoren/source/kernel/linux/scripts/Makefile.build:117:
arch/riscv/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [/home/guoren/source/kernel/linux/Makefile:1200: prepare0] Error 2
make[1]: Target 'all' not remade because of errors.
make: *** [Makefile:219: __sub-make] Error 2
make: Target 'all' not remade because of errors.


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


--
Best Regards

 Guo Ren
Guo Ren July 10, 2022, 2:25 a.m. UTC | #3 
On Fri, Jul 8, 2022 at 4:51 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> 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"
>         ...

Above can't affect the below:
                : "I"(prot##_SVPBMT >> ALT_SVPBMT_SHIFT),               \
                  "I"(prot##_THEAD >> ALT_THEAD_PBMT_SHIFT),            \
                  "I"(ALT_SVPBMT_SHIFT),                                \
                  "I"(ALT_THEAD_PBMT_SHIFT))

So CONFIG_ERRATA_THEAD_PBMT is not clean as you think, compiler still
process the  "I"(prot##_THEAD >> ALT_THEAD_PBMT_SHIFT)" &
"I"(ALT_THEAD_PBMT_SHIFT).

>
> 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
> >
> >
>
>
>
>
diff mbox series

Patch

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
 #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