diff mbox series

RISC-V: Don't include Zicsr or Zifencei in I from ACPI

Message ID 20230711224600.10879-1-palmer@rivosinc.com (mailing list archive)
State Accepted
Commit ab2dbc7accedc2e98eb7d8b8878d337e3b36c95d
Headers show
Series RISC-V: Don't include Zicsr or Zifencei in I from ACPI | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be fixes at HEAD 06c2afb862f9
conchuod/fixes_present success Fixes tag present in non-next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 4 and now 4
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 11 this patch: 11
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 23 this patch: 23
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 3 this patch: 3
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 21 lines checked
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success Fixes tag looks correct
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Palmer Dabbelt July 11, 2023, 10:46 p.m. UTC
ACPI ISA strings are based on a specification after Zicsr and Zifencei
were split out of I, so we shouldn't be treating them as part of I.  We
haven't release an ACPI-based kernel yet, so we don't need to worry
about compatibility with the old ISA strings.

Fixes: 396c018332a1 ("RISC-V: cpufeature: Add ACPI support in riscv_fill_hwcap()")
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
 arch/riscv/kernel/cpufeature.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Conor Dooley July 11, 2023, 10:52 p.m. UTC | #1
On Tue, Jul 11, 2023 at 03:46:00PM -0700, Palmer Dabbelt wrote:
> ACPI ISA strings are based on a specification after Zicsr and Zifencei
> were split out of I, so we shouldn't be treating them as part of I.  We
> haven't release an ACPI-based kernel yet, so we don't need to worry
> about compatibility with the old ISA strings.
> 
> Fixes: 396c018332a1 ("RISC-V: cpufeature: Add ACPI support in riscv_fill_hwcap()")

I think, if anything, this is actually:
Fixes: 07edc32779e3 ("RISC-V: always report presence of extensions formerly part of the base ISA")

Although my rationale was that if we get as far as here, then Zicsr and
Zifencei are going to be enabled anyway so there is no harm in setting
it for both. I probably should have been less of a cute hoor though.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index bdcf460ea53d..a8f66c015229 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -317,19 +317,14 @@ void __init riscv_fill_hwcap(void)
>  #undef SET_ISA_EXT_MAP
>  		}
>  
> -		/*
> -		 * Linux requires the following extensions, so we may as well
> -		 * always set them.
> -		 */
> -		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> -		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> -
>  		/*
>  		 * These ones were as they were part of the base ISA when the
>  		 * port & dt-bindings were upstreamed, and so can be set
>  		 * unconditionally where `i` is in riscv,isa on DT systems.
>  		 */
>  		if (acpi_disabled) {
> +			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> +			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
>  			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
>  			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>  		}
> -- 
> 2.40.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Sunil V L July 12, 2023, 6:42 a.m. UTC | #2
On Tue, Jul 11, 2023 at 03:46:00PM -0700, Palmer Dabbelt wrote:
> ACPI ISA strings are based on a specification after Zicsr and Zifencei
> were split out of I, so we shouldn't be treating them as part of I.  We
> haven't release an ACPI-based kernel yet, so we don't need to worry
> about compatibility with the old ISA strings.
> 
> Fixes: 396c018332a1 ("RISC-V: cpufeature: Add ACPI support in riscv_fill_hwcap()")
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
LGTM. I agree with Conor that Fixes tag should point to 07edc32779e3.

Reviewed-by: Sunil V L <sunilvl@ventanamicro.com>

Thanks,
Sunil
>  arch/riscv/kernel/cpufeature.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index bdcf460ea53d..a8f66c015229 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -317,19 +317,14 @@ void __init riscv_fill_hwcap(void)
>  #undef SET_ISA_EXT_MAP
>  		}
>  
> -		/*
> -		 * Linux requires the following extensions, so we may as well
> -		 * always set them.
> -		 */
> -		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> -		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> -
>  		/*
>  		 * These ones were as they were part of the base ISA when the
>  		 * port & dt-bindings were upstreamed, and so can be set
>  		 * unconditionally where `i` is in riscv,isa on DT systems.
>  		 */
>  		if (acpi_disabled) {
> +			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> +			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
>  			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
>  			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>  		}
> -- 
> 2.40.1
>
Palmer Dabbelt July 12, 2023, 5:05 p.m. UTC | #3
On Tue, 11 Jul 2023 15:52:56 PDT (-0700), Conor Dooley wrote:
> On Tue, Jul 11, 2023 at 03:46:00PM -0700, Palmer Dabbelt wrote:
>> ACPI ISA strings are based on a specification after Zicsr and Zifencei
>> were split out of I, so we shouldn't be treating them as part of I.  We
>> haven't release an ACPI-based kernel yet, so we don't need to worry
>> about compatibility with the old ISA strings.
>> 
>> Fixes: 396c018332a1 ("RISC-V: cpufeature: Add ACPI support in riscv_fill_hwcap()")
>
> I think, if anything, this is actually:
> Fixes: 07edc32779e3 ("RISC-V: always report presence of extensions formerly part of the base ISA")
>
> Although my rationale was that if we get as far as here, then Zicsr and
> Zifencei are going to be enabled anyway so there is no harm in setting
> it for both. I probably should have been less of a cute hoor though.
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

I think there's no way to get here without the extensions, not 100% sure 
though.  I'm mainly trying to avoid going down the same rabbit hole of 
ISA string compatibility hacks in ACPI that we have for DT, though -- 
I'm sure we'll end up with a mess as soon as we release, but might as 
well catch as much as we can.

>
> Cheers,
> Conor.
>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> ---
>>  arch/riscv/kernel/cpufeature.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index bdcf460ea53d..a8f66c015229 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -317,19 +317,14 @@ void __init riscv_fill_hwcap(void)
>>  #undef SET_ISA_EXT_MAP
>>  		}
>>  
>> -		/*
>> -		 * Linux requires the following extensions, so we may as well
>> -		 * always set them.
>> -		 */
>> -		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
>> -		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
>> -
>>  		/*
>>  		 * These ones were as they were part of the base ISA when the
>>  		 * port & dt-bindings were upstreamed, and so can be set
>>  		 * unconditionally where `i` is in riscv,isa on DT systems.
>>  		 */
>>  		if (acpi_disabled) {
>> +			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
>> +			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
>>  			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
>>  			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
>>  		}
>> -- 
>> 2.40.1
>> 
>> 
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
Conor Dooley July 12, 2023, 5:13 p.m. UTC | #4
On Wed, Jul 12, 2023 at 10:05:39AM -0700, Palmer Dabbelt wrote:
> On Tue, 11 Jul 2023 15:52:56 PDT (-0700), Conor Dooley wrote:

> > Although my rationale was that if we get as far as here, then Zicsr and
> > Zifencei are going to be enabled anyway so there is no harm in setting
> > it for both. I probably should have been less of a cute hoor though.
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> I think there's no way to get here without the extensions, not 100% sure
> though.  I'm mainly trying to avoid going down the same rabbit hole of ISA
> string compatibility hacks in ACPI that we have for DT, though -- I'm sure
> we'll end up with a mess as soon as we release, but might as well catch as
> much as we can.

Seems reasonable to me chief :+1:
patchwork-bot+linux-riscv@kernel.org July 13, 2023, 11:50 p.m. UTC | #5
Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 11 Jul 2023 15:46:00 -0700 you wrote:
> ACPI ISA strings are based on a specification after Zicsr and Zifencei
> were split out of I, so we shouldn't be treating them as part of I.  We
> haven't release an ACPI-based kernel yet, so we don't need to worry
> about compatibility with the old ISA strings.
> 
> Fixes: 396c018332a1 ("RISC-V: cpufeature: Add ACPI support in riscv_fill_hwcap()")
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> [...]

Here is the summary with links:
  - RISC-V: Don't include Zicsr or Zifencei in I from ACPI
    https://git.kernel.org/riscv/c/ab2dbc7acced

You are awesome, thank you!
diff mbox series

Patch

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index bdcf460ea53d..a8f66c015229 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -317,19 +317,14 @@  void __init riscv_fill_hwcap(void)
 #undef SET_ISA_EXT_MAP
 		}
 
-		/*
-		 * Linux requires the following extensions, so we may as well
-		 * always set them.
-		 */
-		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
-		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
-
 		/*
 		 * These ones were as they were part of the base ISA when the
 		 * port & dt-bindings were upstreamed, and so can be set
 		 * unconditionally where `i` is in riscv,isa on DT systems.
 		 */
 		if (acpi_disabled) {
+			set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
+			set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
 			set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
 			set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
 		}