| Message ID | 20190904161245.111924-4-anup.patel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM RISC-V Support | expand |
Hi Paul, On Wed, Sep 4, 2019 at 9:43 PM Anup Patel <Anup.Patel@wdc.com> wrote: > > This patch adds riscv_isa bitmap which represents Host ISA features > common across all Host CPUs. The riscv_isa is not same as elf_hwcap > because elf_hwcap will only have ISA features relevant for user-space > apps whereas riscv_isa will have ISA features relevant to both kernel > and user-space apps. > > One of the use-case for riscv_isa bitmap is in KVM hypervisor where > we will use it to do following operations: > > 1. Check whether hypervisor extension is available > 2. Find ISA features that need to be virtualized (e.g. floating > point support, vector extension, etc.) I had addressed your previous comments on this patch by making riscv_isa as bitmap to cover Z-extensions. Please review it again. Regards, Anup > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > Signed-off-by: Atish Patra <atish.patra@wdc.com> > Reviewed-by: Alexander Graf <graf@amazon.com> > --- > arch/riscv/include/asm/hwcap.h | 26 +++++++++++ > arch/riscv/kernel/cpufeature.c | 79 ++++++++++++++++++++++++++++++++-- > 2 files changed, 102 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > index 7ecb7c6a57b1..9b657375aa51 100644 > --- a/arch/riscv/include/asm/hwcap.h > +++ b/arch/riscv/include/asm/hwcap.h > @@ -8,6 +8,7 @@ > #ifndef __ASM_HWCAP_H > #define __ASM_HWCAP_H > > +#include <linux/bits.h> > #include <uapi/asm/hwcap.h> > > #ifndef __ASSEMBLY__ > @@ -22,5 +23,30 @@ enum { > }; > > extern unsigned long elf_hwcap; > + > +#define RISCV_ISA_EXT_a ('a' - 'a') > +#define RISCV_ISA_EXT_c ('c' - 'a') > +#define RISCV_ISA_EXT_d ('d' - 'a') > +#define RISCV_ISA_EXT_f ('f' - 'a') > +#define RISCV_ISA_EXT_h ('h' - 'a') > +#define RISCV_ISA_EXT_i ('i' - 'a') > +#define RISCV_ISA_EXT_m ('m' - 'a') > +#define RISCV_ISA_EXT_s ('s' - 'a') > +#define RISCV_ISA_EXT_u ('u' - 'a') > +#define RISCV_ISA_EXT_zicsr (('z' - 'a') + 1) > +#define RISCV_ISA_EXT_zifencei (('z' - 'a') + 2) > +#define RISCV_ISA_EXT_zam (('z' - 'a') + 3) > +#define RISCV_ISA_EXT_ztso (('z' - 'a') + 4) > + > +#define RISCV_ISA_EXT_MAX 256 > + > +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > + > +#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext) > + > +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit); > +#define riscv_isa_extension_available(isa_bitmap, ext) \ > + __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) > + > #endif > #endif > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index b1ade9a49347..4ce71ce5e290 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -6,21 +6,64 @@ > * Copyright (C) 2017 SiFive > */ > > +#include <linux/bitmap.h> > #include <linux/of.h> > #include <asm/processor.h> > #include <asm/hwcap.h> > #include <asm/smp.h> > > unsigned long elf_hwcap __read_mostly; > + > +/* Host ISA bitmap */ > +static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > + > #ifdef CONFIG_FPU > bool has_fpu __read_mostly; > #endif > > +/** > + * riscv_isa_extension_base - Get base extension word > + * > + * @isa_bitmap ISA bitmap to use > + * @returns base extension word as unsigned long value > + * > + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. > + */ > +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap) > +{ > + if (!isa_bitmap) > + return riscv_isa[0]; > + return isa_bitmap[0]; > +} > +EXPORT_SYMBOL_GPL(riscv_isa_extension_base); > + > +/** > + * __riscv_isa_extension_available - Check whether given extension > + * is available or not > + * > + * @isa_bitmap ISA bitmap to use > + * @bit bit position of the desired extension > + * @returns true or false > + * > + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. > + */ > +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit) > +{ > + const unsigned long *bmap = (isa_bitmap) ? isa_bitmap : riscv_isa; > + > + if (bit >= RISCV_ISA_EXT_MAX) > + return false; > + > + return test_bit(bit, bmap) ? true : false; > +} > +EXPORT_SYMBOL_GPL(__riscv_isa_extension_available); > + > void riscv_fill_hwcap(void) > { > struct device_node *node; > const char *isa; > - size_t i; > + char print_str[BITS_PER_LONG+1]; > + size_t i, j, isa_len; > static unsigned long isa2hwcap[256] = {0}; > > isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I; > @@ -32,8 +75,11 @@ void riscv_fill_hwcap(void) > > elf_hwcap = 0; > > + bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > + > for_each_of_cpu_node(node) { > unsigned long this_hwcap = 0; > + unsigned long this_isa = 0; > > if (riscv_of_processor_hartid(node) < 0) > continue; > @@ -43,8 +89,20 @@ void riscv_fill_hwcap(void) > continue; > } > > - for (i = 0; i < strlen(isa); ++i) > + i = 0; > + isa_len = strlen(isa); > +#if defined(CONFIG_32BIT) > + if (!strncmp(isa, "rv32", 4)) > + i += 4; > +#elif defined(CONFIG_64BIT) > + if (!strncmp(isa, "rv64", 4)) > + i += 4; > +#endif > + for (; i < isa_len; ++i) { > this_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; > + if ('a' <= isa[i] && isa[i] <= 'z') > + this_isa |= (1UL << (isa[i] - 'a')); > + } > > /* > * All "okay" hart should have same isa. Set HWCAP based on > @@ -55,6 +113,11 @@ void riscv_fill_hwcap(void) > elf_hwcap &= this_hwcap; > else > elf_hwcap = this_hwcap; > + > + if (riscv_isa[0]) > + riscv_isa[0] &= this_isa; > + else > + riscv_isa[0] = this_isa; > } > > /* We don't support systems with F but without D, so mask those out > @@ -64,7 +127,17 @@ void riscv_fill_hwcap(void) > elf_hwcap &= ~COMPAT_HWCAP_ISA_F; > } > > - pr_info("elf_hwcap is 0x%lx\n", elf_hwcap); > + memset(print_str, 0, sizeof(print_str)); > + for (i = 0, j = 0; i < BITS_PER_LONG; i++) > + if (riscv_isa[0] & BIT_MASK(i)) > + print_str[j++] = (char)('a' + i); > + pr_info("riscv: ISA extensions %s\n", print_str); > + > + memset(print_str, 0, sizeof(print_str)); > + for (i = 0, j = 0; i < BITS_PER_LONG; i++) > + if (elf_hwcap & BIT_MASK(i)) > + print_str[j++] = (char)('a' + i); > + pr_info("riscv: ELF capabilities %s\n", print_str); > > #ifdef CONFIG_FPU > if (elf_hwcap & (COMPAT_HWCAP_ISA_F | COMPAT_HWCAP_ISA_D)) > -- > 2.17.1 >
Hi Anup, Thanks for changing this to use a bitmap. A few comments below - On Wed, 4 Sep 2019, Anup Patel wrote: > This patch adds riscv_isa bitmap which represents Host ISA features > common across all Host CPUs. The riscv_isa is not same as elf_hwcap > because elf_hwcap will only have ISA features relevant for user-space > apps whereas riscv_isa will have ISA features relevant to both kernel > and user-space apps. > > One of the use-case for riscv_isa bitmap is in KVM hypervisor where > we will use it to do following operations: > > 1. Check whether hypervisor extension is available > 2. Find ISA features that need to be virtualized (e.g. floating > point support, vector extension, etc.) > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > Signed-off-by: Atish Patra <atish.patra@wdc.com> > Reviewed-by: Alexander Graf <graf@amazon.com> > --- > arch/riscv/include/asm/hwcap.h | 26 +++++++++++ > arch/riscv/kernel/cpufeature.c | 79 ++++++++++++++++++++++++++++++++-- > 2 files changed, 102 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > index 7ecb7c6a57b1..9b657375aa51 100644 > --- a/arch/riscv/include/asm/hwcap.h > +++ b/arch/riscv/include/asm/hwcap.h > @@ -8,6 +8,7 @@ > #ifndef __ASM_HWCAP_H > #define __ASM_HWCAP_H > > +#include <linux/bits.h> > #include <uapi/asm/hwcap.h> > > #ifndef __ASSEMBLY__ > @@ -22,5 +23,30 @@ enum { > }; > > extern unsigned long elf_hwcap; > + > +#define RISCV_ISA_EXT_a ('a' - 'a') > +#define RISCV_ISA_EXT_c ('c' - 'a') > +#define RISCV_ISA_EXT_d ('d' - 'a') > +#define RISCV_ISA_EXT_f ('f' - 'a') > +#define RISCV_ISA_EXT_h ('h' - 'a') > +#define RISCV_ISA_EXT_i ('i' - 'a') > +#define RISCV_ISA_EXT_m ('m' - 'a') > +#define RISCV_ISA_EXT_s ('s' - 'a') > +#define RISCV_ISA_EXT_u ('u' - 'a') > +#define RISCV_ISA_EXT_zicsr (('z' - 'a') + 1) > +#define RISCV_ISA_EXT_zifencei (('z' - 'a') + 2) > +#define RISCV_ISA_EXT_zam (('z' - 'a') + 3) > +#define RISCV_ISA_EXT_ztso (('z' - 'a') + 4) If we add the Z extensions here, it's probably best if we drop Zam from this list. The rationale is, as maintainers, we're planning to hold off on merging any support for extensions or modules that aren't in the "frozen" or "ratified" states, and according to the RISC-V specs, Zicsr, Zifencei, and Ztso are all either frozen or ratified. However, see below - > + > +#define RISCV_ISA_EXT_MAX 256 > + > +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > + > +#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext) > + > +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit); > +#define riscv_isa_extension_available(isa_bitmap, ext) \ > + __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) > + > #endif > #endif > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index b1ade9a49347..4ce71ce5e290 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -6,21 +6,64 @@ > * Copyright (C) 2017 SiFive > */ > > +#include <linux/bitmap.h> > #include <linux/of.h> > #include <asm/processor.h> > #include <asm/hwcap.h> > #include <asm/smp.h> > > unsigned long elf_hwcap __read_mostly; > + > +/* Host ISA bitmap */ > +static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > + > #ifdef CONFIG_FPU > bool has_fpu __read_mostly; > #endif > > +/** > + * riscv_isa_extension_base - Get base extension word > + * > + * @isa_bitmap ISA bitmap to use > + * @returns base extension word as unsigned long value > + * > + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. > + */ Am happy to see comments that can be automatically parsed, but could you reformat them into kernel-doc format? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst > +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap) > +{ > + if (!isa_bitmap) > + return riscv_isa[0]; > + return isa_bitmap[0]; > +} > +EXPORT_SYMBOL_GPL(riscv_isa_extension_base); > + > +/** > + * __riscv_isa_extension_available - Check whether given extension > + * is available or not > + * > + * @isa_bitmap ISA bitmap to use > + * @bit bit position of the desired extension > + * @returns true or false > + * > + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. > + */ Same comment as above. > +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit) > +{ > + const unsigned long *bmap = (isa_bitmap) ? isa_bitmap : riscv_isa; > + > + if (bit >= RISCV_ISA_EXT_MAX) > + return false; > + > + return test_bit(bit, bmap) ? true : false; > +} > +EXPORT_SYMBOL_GPL(__riscv_isa_extension_available); > + > void riscv_fill_hwcap(void) > { > struct device_node *node; > const char *isa; > - size_t i; > + char print_str[BITS_PER_LONG+1]; > + size_t i, j, isa_len; > static unsigned long isa2hwcap[256] = {0}; > > isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I; > @@ -32,8 +75,11 @@ void riscv_fill_hwcap(void) > > elf_hwcap = 0; > > + bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > + > for_each_of_cpu_node(node) { > unsigned long this_hwcap = 0; > + unsigned long this_isa = 0; > > if (riscv_of_processor_hartid(node) < 0) > continue; > @@ -43,8 +89,20 @@ void riscv_fill_hwcap(void) > continue; > } > > - for (i = 0; i < strlen(isa); ++i) > + i = 0; > + isa_len = strlen(isa); > +#if defined(CONFIG_32BIT) > + if (!strncmp(isa, "rv32", 4)) > + i += 4; > +#elif defined(CONFIG_64BIT) > + if (!strncmp(isa, "rv64", 4)) > + i += 4; > +#endif > + for (; i < isa_len; ++i) { > this_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; > + if ('a' <= isa[i] && isa[i] <= 'z') > + this_isa |= (1UL << (isa[i] - 'a')); Continuing from the earlier comment, this code won't properly handle the X and Z prefix extensions. So maybe for the time being, we should just drop the lines mentioned earlier that imply that we can parse Z-prefix extensions, and change this line so it ignores X and Z letters? Then a subsequent patch can add support for more complicated extension string parsing. > + } > > /* > * All "okay" hart should have same isa. Set HWCAP based on > @@ -55,6 +113,11 @@ void riscv_fill_hwcap(void) > elf_hwcap &= this_hwcap; > else > elf_hwcap = this_hwcap; > + > + if (riscv_isa[0]) > + riscv_isa[0] &= this_isa; > + else > + riscv_isa[0] = this_isa; > } > > /* We don't support systems with F but without D, so mask those out > @@ -64,7 +127,17 @@ void riscv_fill_hwcap(void) > elf_hwcap &= ~COMPAT_HWCAP_ISA_F; > } > > - pr_info("elf_hwcap is 0x%lx\n", elf_hwcap); > + memset(print_str, 0, sizeof(print_str)); > + for (i = 0, j = 0; i < BITS_PER_LONG; i++) > + if (riscv_isa[0] & BIT_MASK(i)) > + print_str[j++] = (char)('a' + i); > + pr_info("riscv: ISA extensions %s\n", print_str); > + > + memset(print_str, 0, sizeof(print_str)); > + for (i = 0, j = 0; i < BITS_PER_LONG; i++) > + if (elf_hwcap & BIT_MASK(i)) > + print_str[j++] = (char)('a' + i); > + pr_info("riscv: ELF capabilities %s\n", print_str); > > #ifdef CONFIG_FPU > if (elf_hwcap & (COMPAT_HWCAP_ISA_F | COMPAT_HWCAP_ISA_D)) > -- > 2.17.1 > > - Paul
On Sat, Sep 21, 2019 at 3:31 PM Paul Walmsley <paul.walmsley@sifive.com> wrote: > > Hi Anup, > > Thanks for changing this to use a bitmap. A few comments below - > > On Wed, 4 Sep 2019, Anup Patel wrote: > > > This patch adds riscv_isa bitmap which represents Host ISA features > > common across all Host CPUs. The riscv_isa is not same as elf_hwcap > > because elf_hwcap will only have ISA features relevant for user-space > > apps whereas riscv_isa will have ISA features relevant to both kernel > > and user-space apps. > > > > One of the use-case for riscv_isa bitmap is in KVM hypervisor where > > we will use it to do following operations: > > > > 1. Check whether hypervisor extension is available > > 2. Find ISA features that need to be virtualized (e.g. floating > > point support, vector extension, etc.) > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > Reviewed-by: Alexander Graf <graf@amazon.com> > > --- > > arch/riscv/include/asm/hwcap.h | 26 +++++++++++ > > arch/riscv/kernel/cpufeature.c | 79 ++++++++++++++++++++++++++++++++-- > > 2 files changed, 102 insertions(+), 3 deletions(-) > > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > index 7ecb7c6a57b1..9b657375aa51 100644 > > --- a/arch/riscv/include/asm/hwcap.h > > +++ b/arch/riscv/include/asm/hwcap.h > > @@ -8,6 +8,7 @@ > > #ifndef __ASM_HWCAP_H > > #define __ASM_HWCAP_H > > > > +#include <linux/bits.h> > > #include <uapi/asm/hwcap.h> > > > > #ifndef __ASSEMBLY__ > > @@ -22,5 +23,30 @@ enum { > > }; > > > > extern unsigned long elf_hwcap; > > + > > +#define RISCV_ISA_EXT_a ('a' - 'a') > > +#define RISCV_ISA_EXT_c ('c' - 'a') > > +#define RISCV_ISA_EXT_d ('d' - 'a') > > +#define RISCV_ISA_EXT_f ('f' - 'a') > > +#define RISCV_ISA_EXT_h ('h' - 'a') > > +#define RISCV_ISA_EXT_i ('i' - 'a') > > +#define RISCV_ISA_EXT_m ('m' - 'a') > > +#define RISCV_ISA_EXT_s ('s' - 'a') > > +#define RISCV_ISA_EXT_u ('u' - 'a') > > +#define RISCV_ISA_EXT_zicsr (('z' - 'a') + 1) > > +#define RISCV_ISA_EXT_zifencei (('z' - 'a') + 2) > > +#define RISCV_ISA_EXT_zam (('z' - 'a') + 3) > > +#define RISCV_ISA_EXT_ztso (('z' - 'a') + 4) > > If we add the Z extensions here, it's probably best if we drop Zam from > this list. The rationale is, as maintainers, we're planning to hold off > on merging any support for extensions or modules that aren't in the > "frozen" or "ratified" states, and according to the RISC-V specs, Zicsr, > Zifencei, and Ztso are all either frozen or ratified. However, see > below - No problem, I will remove Zam define. Please add documentation under Documentation/riscv regarding the policy of not merging support for RISC-V extensions which aren't in "frozen" or "ratified" state. > > > + > > +#define RISCV_ISA_EXT_MAX 256 > > + > > +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); > > + > > +#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext) > > + > > +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit); > > +#define riscv_isa_extension_available(isa_bitmap, ext) \ > > + __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) > > + > > #endif > > #endif > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index b1ade9a49347..4ce71ce5e290 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -6,21 +6,64 @@ > > * Copyright (C) 2017 SiFive > > */ > > > > +#include <linux/bitmap.h> > > #include <linux/of.h> > > #include <asm/processor.h> > > #include <asm/hwcap.h> > > #include <asm/smp.h> > > > > unsigned long elf_hwcap __read_mostly; > > + > > +/* Host ISA bitmap */ > > +static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > > + > > #ifdef CONFIG_FPU > > bool has_fpu __read_mostly; > > #endif > > > > +/** > > + * riscv_isa_extension_base - Get base extension word > > + * > > + * @isa_bitmap ISA bitmap to use > > + * @returns base extension word as unsigned long value > > + * > > + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. > > + */ > > Am happy to see comments that can be automatically parsed, but could you > reformat them into kernel-doc format? > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst Sure, I will update comments as-per kernel-doc.rst. > > > +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap) > > +{ > > + if (!isa_bitmap) > > + return riscv_isa[0]; > > + return isa_bitmap[0]; > > +} > > +EXPORT_SYMBOL_GPL(riscv_isa_extension_base); > > + > > +/** > > + * __riscv_isa_extension_available - Check whether given extension > > + * is available or not > > + * > > + * @isa_bitmap ISA bitmap to use > > + * @bit bit position of the desired extension > > + * @returns true or false > > + * > > + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. > > + */ > > Same comment as above. Okay, I will update. > > > +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit) > > +{ > > + const unsigned long *bmap = (isa_bitmap) ? isa_bitmap : riscv_isa; > > + > > + if (bit >= RISCV_ISA_EXT_MAX) > > + return false; > > + > > + return test_bit(bit, bmap) ? true : false; > > +} > > +EXPORT_SYMBOL_GPL(__riscv_isa_extension_available); > > + > > void riscv_fill_hwcap(void) > > { > > struct device_node *node; > > const char *isa; > > - size_t i; > > + char print_str[BITS_PER_LONG+1]; > > + size_t i, j, isa_len; > > static unsigned long isa2hwcap[256] = {0}; > > > > isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I; > > @@ -32,8 +75,11 @@ void riscv_fill_hwcap(void) > > > > elf_hwcap = 0; > > > > + bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > > + > > for_each_of_cpu_node(node) { > > unsigned long this_hwcap = 0; > > + unsigned long this_isa = 0; > > > > if (riscv_of_processor_hartid(node) < 0) > > continue; > > @@ -43,8 +89,20 @@ void riscv_fill_hwcap(void) > > continue; > > } > > > > - for (i = 0; i < strlen(isa); ++i) > > + i = 0; > > + isa_len = strlen(isa); > > +#if defined(CONFIG_32BIT) > > + if (!strncmp(isa, "rv32", 4)) > > + i += 4; > > +#elif defined(CONFIG_64BIT) > > + if (!strncmp(isa, "rv64", 4)) > > + i += 4; > > +#endif > > + for (; i < isa_len; ++i) { > > this_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; > > + if ('a' <= isa[i] && isa[i] <= 'z') > > + this_isa |= (1UL << (isa[i] - 'a')); > > Continuing from the earlier comment, this code won't properly handle the X > and Z prefix extensions. So maybe for the time being, we should just drop > the lines mentioned earlier that imply that we can parse Z-prefix > extensions, and change this line so it ignores X and Z letters? > > Then a subsequent patch can add support for more complicated extension > string parsing. Okay, we sill not parse 'x' and 'z' bits (which can be added later for X and Z prefix extensions). > > > > + } > > > > /* > > * All "okay" hart should have same isa. Set HWCAP based on > > @@ -55,6 +113,11 @@ void riscv_fill_hwcap(void) > > elf_hwcap &= this_hwcap; > > else > > elf_hwcap = this_hwcap; > > + > > + if (riscv_isa[0]) > > + riscv_isa[0] &= this_isa; > > + else > > + riscv_isa[0] = this_isa; > > } > > > > /* We don't support systems with F but without D, so mask those out > > @@ -64,7 +127,17 @@ void riscv_fill_hwcap(void) > > elf_hwcap &= ~COMPAT_HWCAP_ISA_F; > > } > > > > - pr_info("elf_hwcap is 0x%lx\n", elf_hwcap); > > + memset(print_str, 0, sizeof(print_str)); > > + for (i = 0, j = 0; i < BITS_PER_LONG; i++) > > + if (riscv_isa[0] & BIT_MASK(i)) > > + print_str[j++] = (char)('a' + i); > > + pr_info("riscv: ISA extensions %s\n", print_str); > > + > > + memset(print_str, 0, sizeof(print_str)); > > + for (i = 0, j = 0; i < BITS_PER_LONG; i++) > > + if (elf_hwcap & BIT_MASK(i)) > > + print_str[j++] = (char)('a' + i); > > + pr_info("riscv: ELF capabilities %s\n", print_str); > > > > #ifdef CONFIG_FPU > > if (elf_hwcap & (COMPAT_HWCAP_ISA_F | COMPAT_HWCAP_ISA_D)) > > -- > > 2.17.1 > > > > > > > - Paul Regards, Anup
On Sat, 2019-09-21 at 03:01 -0700, Paul Walmsley wrote: > Hi Anup, > > Thanks for changing this to use a bitmap. A few comments below - > > On Wed, 4 Sep 2019, Anup Patel wrote: > > > This patch adds riscv_isa bitmap which represents Host ISA features > > common across all Host CPUs. The riscv_isa is not same as elf_hwcap > > because elf_hwcap will only have ISA features relevant for user- > > space > > apps whereas riscv_isa will have ISA features relevant to both > > kernel > > and user-space apps. > > > > One of the use-case for riscv_isa bitmap is in KVM hypervisor where > > we will use it to do following operations: > > > > 1. Check whether hypervisor extension is available > > 2. Find ISA features that need to be virtualized (e.g. floating > > point support, vector extension, etc.) > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > Reviewed-by: Alexander Graf <graf@amazon.com> > > --- > > arch/riscv/include/asm/hwcap.h | 26 +++++++++++ > > arch/riscv/kernel/cpufeature.c | 79 > > ++++++++++++++++++++++++++++++++-- > > 2 files changed, 102 insertions(+), 3 deletions(-) > > > > diff --git a/arch/riscv/include/asm/hwcap.h > > b/arch/riscv/include/asm/hwcap.h > > index 7ecb7c6a57b1..9b657375aa51 100644 > > --- a/arch/riscv/include/asm/hwcap.h > > +++ b/arch/riscv/include/asm/hwcap.h > > @@ -8,6 +8,7 @@ > > #ifndef __ASM_HWCAP_H > > #define __ASM_HWCAP_H > > > > +#include <linux/bits.h> > > #include <uapi/asm/hwcap.h> > > > > #ifndef __ASSEMBLY__ > > @@ -22,5 +23,30 @@ enum { > > }; > > > > extern unsigned long elf_hwcap; > > + > > +#define RISCV_ISA_EXT_a ('a' - 'a') > > +#define RISCV_ISA_EXT_c ('c' - 'a') > > +#define RISCV_ISA_EXT_d ('d' - 'a') > > +#define RISCV_ISA_EXT_f ('f' - 'a') > > +#define RISCV_ISA_EXT_h ('h' - 'a') > > +#define RISCV_ISA_EXT_i ('i' - 'a') > > +#define RISCV_ISA_EXT_m ('m' - 'a') > > +#define RISCV_ISA_EXT_s ('s' - 'a') > > +#define RISCV_ISA_EXT_u ('u' - 'a') > > +#define RISCV_ISA_EXT_zicsr (('z' - 'a') + 1) > > +#define RISCV_ISA_EXT_zifencei (('z' - 'a') + 2) > > +#define RISCV_ISA_EXT_zam (('z' - 'a') + 3) > > +#define RISCV_ISA_EXT_ztso (('z' - 'a') + 4) > > If we add the Z extensions here, it's probably best if we drop Zam > from > this list. The rationale is, as maintainers, we're planning to hold > off > on merging any support for extensions or modules that aren't in the > "frozen" or "ratified" states, and according to the RISC-V specs, > Zicsr, > Zifencei, and Ztso are all either frozen or ratified. However, see > below - Hey Paul, I think that this should be documented somewhere in the kernel tree. In QEMU land we have decieded that draft extensions will be accepted and there are currently two extension series on list (Hypervisor from WDC and Vector from CSKY). I suspect as RISC-V grows there are going to be more and more groups that are interested in some specific extension and want it upstream. In this case it makes sense to have it very clearly documeneted so that everyone knows what will/won't be accepted. If it's clearly documented then everyone will be on the same page as to what will/won't be accepted by individual projects. As a side note I'll probably look at adding something for QEMU as well :) Alistair > > > + > > +#define RISCV_ISA_EXT_MAX 256 > > + > > +unsigned long riscv_isa_extension_base(const unsigned long > > *isa_bitmap); > > + > > +#define riscv_isa_extension_mask(ext) > > BIT_MASK(RISCV_ISA_EXT_##ext) > > + > > +bool __riscv_isa_extension_available(const unsigned long > > *isa_bitmap, int bit); > > +#define riscv_isa_extension_available(isa_bitmap, ext) \ > > + __riscv_isa_extension_available(isa_bitmap, > > RISCV_ISA_EXT_##ext) > > + > > #endif > > #endif > > diff --git a/arch/riscv/kernel/cpufeature.c > > b/arch/riscv/kernel/cpufeature.c > > index b1ade9a49347..4ce71ce5e290 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -6,21 +6,64 @@ > > * Copyright (C) 2017 SiFive > > */ > > > > +#include <linux/bitmap.h> > > #include <linux/of.h> > > #include <asm/processor.h> > > #include <asm/hwcap.h> > > #include <asm/smp.h> > > > > unsigned long elf_hwcap __read_mostly; > > + > > +/* Host ISA bitmap */ > > +static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > > + > > #ifdef CONFIG_FPU > > bool has_fpu __read_mostly; > > #endif > > > > +/** > > + * riscv_isa_extension_base - Get base extension word > > + * > > + * @isa_bitmap ISA bitmap to use > > + * @returns base extension word as unsigned long value > > + * > > + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. > > + */ > > Am happy to see comments that can be automatically parsed, but could > you > reformat them into kernel-doc format? > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst > > > +unsigned long riscv_isa_extension_base(const unsigned long > > *isa_bitmap) > > +{ > > + if (!isa_bitmap) > > + return riscv_isa[0]; > > + return isa_bitmap[0]; > > +} > > +EXPORT_SYMBOL_GPL(riscv_isa_extension_base); > > + > > +/** > > + * __riscv_isa_extension_available - Check whether given extension > > + * is available or not > > + * > > + * @isa_bitmap ISA bitmap to use > > + * @bit bit position of the desired extension > > + * @returns true or false > > + * > > + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. > > + */ > > Same comment as above. > > > +bool __riscv_isa_extension_available(const unsigned long > > *isa_bitmap, int bit) > > +{ > > + const unsigned long *bmap = (isa_bitmap) ? isa_bitmap : > > riscv_isa; > > + > > + if (bit >= RISCV_ISA_EXT_MAX) > > + return false; > > + > > + return test_bit(bit, bmap) ? true : false; > > +} > > +EXPORT_SYMBOL_GPL(__riscv_isa_extension_available); > > + > > void riscv_fill_hwcap(void) > > { > > struct device_node *node; > > const char *isa; > > - size_t i; > > + char print_str[BITS_PER_LONG+1]; > > + size_t i, j, isa_len; > > static unsigned long isa2hwcap[256] = {0}; > > > > isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I; > > @@ -32,8 +75,11 @@ void riscv_fill_hwcap(void) > > > > elf_hwcap = 0; > > > > + bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); > > + > > for_each_of_cpu_node(node) { > > unsigned long this_hwcap = 0; > > + unsigned long this_isa = 0; > > > > if (riscv_of_processor_hartid(node) < 0) > > continue; > > @@ -43,8 +89,20 @@ void riscv_fill_hwcap(void) > > continue; > > } > > > > - for (i = 0; i < strlen(isa); ++i) > > + i = 0; > > + isa_len = strlen(isa); > > +#if defined(CONFIG_32BIT) > > + if (!strncmp(isa, "rv32", 4)) > > + i += 4; > > +#elif defined(CONFIG_64BIT) > > + if (!strncmp(isa, "rv64", 4)) > > + i += 4; > > +#endif > > + for (; i < isa_len; ++i) { > > this_hwcap |= isa2hwcap[(unsigned > > char)(isa[i])]; > > + if ('a' <= isa[i] && isa[i] <= 'z') > > + this_isa |= (1UL << (isa[i] - 'a')); > > Continuing from the earlier comment, this code won't properly handle > the X > and Z prefix extensions. So maybe for the time being, we should just > drop > the lines mentioned earlier that imply that we can parse Z-prefix > extensions, and change this line so it ignores X and Z letters? > > Then a subsequent patch can add support for more complicated > extension > string parsing. > > > > + } > > > > /* > > * All "okay" hart should have same isa. Set HWCAP > > based on > > @@ -55,6 +113,11 @@ void riscv_fill_hwcap(void) > > elf_hwcap &= this_hwcap; > > else > > elf_hwcap = this_hwcap; > > + > > + if (riscv_isa[0]) > > + riscv_isa[0] &= this_isa; > > + else > > + riscv_isa[0] = this_isa; > > } > > > > /* We don't support systems with F but without D, so mask those > > out > > @@ -64,7 +127,17 @@ void riscv_fill_hwcap(void) > > elf_hwcap &= ~COMPAT_HWCAP_ISA_F; > > } > > > > - pr_info("elf_hwcap is 0x%lx\n", elf_hwcap); > > + memset(print_str, 0, sizeof(print_str)); > > + for (i = 0, j = 0; i < BITS_PER_LONG; i++) > > + if (riscv_isa[0] & BIT_MASK(i)) > > + print_str[j++] = (char)('a' + i); > > + pr_info("riscv: ISA extensions %s\n", print_str); > > + > > + memset(print_str, 0, sizeof(print_str)); > > + for (i = 0, j = 0; i < BITS_PER_LONG; i++) > > + if (elf_hwcap & BIT_MASK(i)) > > + print_str[j++] = (char)('a' + i); > > + pr_info("riscv: ELF capabilities %s\n", print_str); > > > > #ifdef CONFIG_FPU > > if (elf_hwcap & (COMPAT_HWCAP_ISA_F | COMPAT_HWCAP_ISA_D)) > > -- > > 2.17.1 > > > > > > - Paul
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h index 7ecb7c6a57b1..9b657375aa51 100644 --- a/arch/riscv/include/asm/hwcap.h +++ b/arch/riscv/include/asm/hwcap.h @@ -8,6 +8,7 @@ #ifndef __ASM_HWCAP_H #define __ASM_HWCAP_H +#include <linux/bits.h> #include <uapi/asm/hwcap.h> #ifndef __ASSEMBLY__ @@ -22,5 +23,30 @@ enum { }; extern unsigned long elf_hwcap; + +#define RISCV_ISA_EXT_a ('a' - 'a') +#define RISCV_ISA_EXT_c ('c' - 'a') +#define RISCV_ISA_EXT_d ('d' - 'a') +#define RISCV_ISA_EXT_f ('f' - 'a') +#define RISCV_ISA_EXT_h ('h' - 'a') +#define RISCV_ISA_EXT_i ('i' - 'a') +#define RISCV_ISA_EXT_m ('m' - 'a') +#define RISCV_ISA_EXT_s ('s' - 'a') +#define RISCV_ISA_EXT_u ('u' - 'a') +#define RISCV_ISA_EXT_zicsr (('z' - 'a') + 1) +#define RISCV_ISA_EXT_zifencei (('z' - 'a') + 2) +#define RISCV_ISA_EXT_zam (('z' - 'a') + 3) +#define RISCV_ISA_EXT_ztso (('z' - 'a') + 4) + +#define RISCV_ISA_EXT_MAX 256 + +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); + +#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext) + +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit); +#define riscv_isa_extension_available(isa_bitmap, ext) \ + __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) + #endif #endif diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index b1ade9a49347..4ce71ce5e290 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -6,21 +6,64 @@ * Copyright (C) 2017 SiFive */ +#include <linux/bitmap.h> #include <linux/of.h> #include <asm/processor.h> #include <asm/hwcap.h> #include <asm/smp.h> unsigned long elf_hwcap __read_mostly; + +/* Host ISA bitmap */ +static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; + #ifdef CONFIG_FPU bool has_fpu __read_mostly; #endif +/** + * riscv_isa_extension_base - Get base extension word + * + * @isa_bitmap ISA bitmap to use + * @returns base extension word as unsigned long value + * + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. + */ +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap) +{ + if (!isa_bitmap) + return riscv_isa[0]; + return isa_bitmap[0]; +} +EXPORT_SYMBOL_GPL(riscv_isa_extension_base); + +/** + * __riscv_isa_extension_available - Check whether given extension + * is available or not + * + * @isa_bitmap ISA bitmap to use + * @bit bit position of the desired extension + * @returns true or false + * + * NOTE: If isa_bitmap is NULL then Host ISA bitmap will be used. + */ +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit) +{ + const unsigned long *bmap = (isa_bitmap) ? isa_bitmap : riscv_isa; + + if (bit >= RISCV_ISA_EXT_MAX) + return false; + + return test_bit(bit, bmap) ? true : false; +} +EXPORT_SYMBOL_GPL(__riscv_isa_extension_available); + void riscv_fill_hwcap(void) { struct device_node *node; const char *isa; - size_t i; + char print_str[BITS_PER_LONG+1]; + size_t i, j, isa_len; static unsigned long isa2hwcap[256] = {0}; isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I; @@ -32,8 +75,11 @@ void riscv_fill_hwcap(void) elf_hwcap = 0; + bitmap_zero(riscv_isa, RISCV_ISA_EXT_MAX); + for_each_of_cpu_node(node) { unsigned long this_hwcap = 0; + unsigned long this_isa = 0; if (riscv_of_processor_hartid(node) < 0) continue; @@ -43,8 +89,20 @@ void riscv_fill_hwcap(void) continue; } - for (i = 0; i < strlen(isa); ++i) + i = 0; + isa_len = strlen(isa); +#if defined(CONFIG_32BIT) + if (!strncmp(isa, "rv32", 4)) + i += 4; +#elif defined(CONFIG_64BIT) + if (!strncmp(isa, "rv64", 4)) + i += 4; +#endif + for (; i < isa_len; ++i) { this_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; + if ('a' <= isa[i] && isa[i] <= 'z') + this_isa |= (1UL << (isa[i] - 'a')); + } /* * All "okay" hart should have same isa. Set HWCAP based on @@ -55,6 +113,11 @@ void riscv_fill_hwcap(void) elf_hwcap &= this_hwcap; else elf_hwcap = this_hwcap; + + if (riscv_isa[0]) + riscv_isa[0] &= this_isa; + else + riscv_isa[0] = this_isa; } /* We don't support systems with F but without D, so mask those out @@ -64,7 +127,17 @@ void riscv_fill_hwcap(void) elf_hwcap &= ~COMPAT_HWCAP_ISA_F; } - pr_info("elf_hwcap is 0x%lx\n", elf_hwcap); + memset(print_str, 0, sizeof(print_str)); + for (i = 0, j = 0; i < BITS_PER_LONG; i++) + if (riscv_isa[0] & BIT_MASK(i)) + print_str[j++] = (char)('a' + i); + pr_info("riscv: ISA extensions %s\n", print_str); + + memset(print_str, 0, sizeof(print_str)); + for (i = 0, j = 0; i < BITS_PER_LONG; i++) + if (elf_hwcap & BIT_MASK(i)) + print_str[j++] = (char)('a' + i); + pr_info("riscv: ELF capabilities %s\n", print_str); #ifdef CONFIG_FPU if (elf_hwcap & (COMPAT_HWCAP_ISA_F | COMPAT_HWCAP_ISA_D))