| Message ID | 1615175897-23509-3-git-send-email-vincent.chen@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: introduce alternative mechanism to apply errata patches | expand |
On 2021/03/08 12:59, Vincent Chen wrote: > Issue 3 SBI calls to get the vendor ID, architecture ID and implementation > ID early in boot so we only need to take the SBI call overhead once. > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > --- > arch/riscv/include/asm/csr.h | 3 +++ > arch/riscv/include/asm/hwcap.h | 6 ++++++ > arch/riscv/include/asm/processor.h | 2 ++ > arch/riscv/include/asm/soc.h | 1 + > arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ > arch/riscv/kernel/setup.c | 2 ++ > arch/riscv/kernel/soc.c | 1 + > 7 files changed, 32 insertions(+) > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h > index caadfc1d7487..87ac65696871 100644 > --- a/arch/riscv/include/asm/csr.h > +++ b/arch/riscv/include/asm/csr.h > @@ -115,6 +115,9 @@ > #define CSR_MIP 0x344 > #define CSR_PMPCFG0 0x3a0 > #define CSR_PMPADDR0 0x3b0 > +#define CSR_MVENDORID 0xf11 > +#define CSR_MARCHID 0xf12 > +#define CSR_MIMPID 0xf13 > #define CSR_MHARTID 0xf14 > > #ifdef CONFIG_RISCV_M_MODE > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > index 5ce50468aff1..b7409487c9d2 100644 > --- a/arch/riscv/include/asm/hwcap.h > +++ b/arch/riscv/include/asm/hwcap.h > @@ -44,6 +44,12 @@ 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) > > +struct cpu_manufacturer_info_t { > + unsigned long vendor_id; > + unsigned long arch_id; > + unsigned long imp_id; > +}; > + > #endif > > #endif /* _ASM_RISCV_HWCAP_H */ > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > index 3a240037bde2..4e11a9621d14 100644 > --- a/arch/riscv/include/asm/processor.h > +++ b/arch/riscv/include/asm/processor.h > @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); > > extern void riscv_fill_hwcap(void); > > +void riscv_fill_cpu_manufacturer_info(void); > + > #endif /* __ASSEMBLY__ */ > > #endif /* _ASM_RISCV_PROCESSOR_H */ > diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h > index f494066051a2..03dee6db404c 100644 > --- a/arch/riscv/include/asm/soc.h > +++ b/arch/riscv/include/asm/soc.h > @@ -10,6 +10,7 @@ > #include <linux/of.h> > #include <linux/linkage.h> > #include <linux/types.h> > +#include <asm/hwcap.h> > > #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ > static const struct of_device_id __soc_early_init__##name \ > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index ac202f44a670..389162ee19ea 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -12,6 +12,8 @@ > #include <asm/hwcap.h> > #include <asm/smp.h> > #include <asm/switch_to.h> > +#include <asm/sbi.h> > +#include <asm/csr.h> > > unsigned long elf_hwcap __read_mostly; > > @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > bool has_fpu __read_mostly; > #endif > > +struct cpu_manufacturer_info_t cpu_mfr_info; > + > /** > * riscv_isa_extension_base() - Get base extension word > * > @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) > has_fpu = true; > #endif > } > + > +void riscv_fill_cpu_manufacturer_info(void) > +{ > +#ifndef CONFIG_RISCV_M_MODE > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); > + cpu_mfr_info.arch_id = sbi_get_archid(); > + cpu_mfr_info.imp_id = sbi_get_impid(); > +#else > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); > +#endif Why ? reading the registers will work with M-Mode too. It was there before when we temporarily had the builtin DTB lookup based on vendor/arch/imp (see defunct soc_lookup_builtin_dtb() in 5.11). > +} > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index e85bacff1b50..03621ce9092c 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) > #endif > > riscv_fill_hwcap(); > + Nit: I do not think the white libe is really necessary here. > + riscv_fill_cpu_manufacturer_info(); > } > > static int __init topology_init(void) > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c > index a0516172a33c..58f6fd91743a 100644 > --- a/arch/riscv/kernel/soc.c > +++ b/arch/riscv/kernel/soc.c > @@ -6,6 +6,7 @@ > #include <linux/libfdt.h> > #include <linux/pgtable.h> > #include <asm/soc.h> > +#include <asm/hwcap.h> Why is this necessary ? > > /* > * This is called extremly early, before parse_dtb(), to allow initializing >
On 3/8/21 6:30 PM, Damien Le Moal wrote: > On 2021/03/08 12:59, Vincent Chen wrote: >> Issue 3 SBI calls to get the vendor ID, architecture ID and implementation >> ID early in boot so we only need to take the SBI call overhead once. >> >> Signed-off-by: Vincent Chen <vincent.chen@sifive.com> >> --- >> arch/riscv/include/asm/csr.h | 3 +++ >> arch/riscv/include/asm/hwcap.h | 6 ++++++ >> arch/riscv/include/asm/processor.h | 2 ++ >> arch/riscv/include/asm/soc.h | 1 + >> arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ >> arch/riscv/kernel/setup.c | 2 ++ >> arch/riscv/kernel/soc.c | 1 + >> 7 files changed, 32 insertions(+) >> >> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h >> index caadfc1d7487..87ac65696871 100644 >> --- a/arch/riscv/include/asm/csr.h >> +++ b/arch/riscv/include/asm/csr.h >> @@ -115,6 +115,9 @@ >> #define CSR_MIP 0x344 >> #define CSR_PMPCFG0 0x3a0 >> #define CSR_PMPADDR0 0x3b0 >> +#define CSR_MVENDORID 0xf11 >> +#define CSR_MARCHID 0xf12 >> +#define CSR_MIMPID 0xf13 >> #define CSR_MHARTID 0xf14 >> >> #ifdef CONFIG_RISCV_M_MODE >> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h >> index 5ce50468aff1..b7409487c9d2 100644 >> --- a/arch/riscv/include/asm/hwcap.h >> +++ b/arch/riscv/include/asm/hwcap.h >> @@ -44,6 +44,12 @@ 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) >> >> +struct cpu_manufacturer_info_t { >> + unsigned long vendor_id; >> + unsigned long arch_id; >> + unsigned long imp_id; >> +}; >> + >> #endif >> >> #endif /* _ASM_RISCV_HWCAP_H */ >> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h >> index 3a240037bde2..4e11a9621d14 100644 >> --- a/arch/riscv/include/asm/processor.h >> +++ b/arch/riscv/include/asm/processor.h >> @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); >> >> extern void riscv_fill_hwcap(void); >> >> +void riscv_fill_cpu_manufacturer_info(void); >> + >> #endif /* __ASSEMBLY__ */ >> >> #endif /* _ASM_RISCV_PROCESSOR_H */ >> diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h >> index f494066051a2..03dee6db404c 100644 >> --- a/arch/riscv/include/asm/soc.h >> +++ b/arch/riscv/include/asm/soc.h >> @@ -10,6 +10,7 @@ >> #include <linux/of.h> >> #include <linux/linkage.h> >> #include <linux/types.h> >> +#include <asm/hwcap.h> >> >> #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ >> static const struct of_device_id __soc_early_init__##name \ >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> index ac202f44a670..389162ee19ea 100644 >> --- a/arch/riscv/kernel/cpufeature.c >> +++ b/arch/riscv/kernel/cpufeature.c >> @@ -12,6 +12,8 @@ >> #include <asm/hwcap.h> >> #include <asm/smp.h> >> #include <asm/switch_to.h> >> +#include <asm/sbi.h> >> +#include <asm/csr.h> >> >> unsigned long elf_hwcap __read_mostly; >> >> @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; >> bool has_fpu __read_mostly; >> #endif >> >> +struct cpu_manufacturer_info_t cpu_mfr_info; >> + >> /** >> * riscv_isa_extension_base() - Get base extension word >> * >> @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) >> has_fpu = true; >> #endif >> } >> + >> +void riscv_fill_cpu_manufacturer_info(void) >> +{ >> +#ifndef CONFIG_RISCV_M_MODE >> + cpu_mfr_info.vendor_id = sbi_get_vendorid(); >> + cpu_mfr_info.arch_id = sbi_get_archid(); >> + cpu_mfr_info.imp_id = sbi_get_impid(); >> +#else >> + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); >> + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); >> + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); >> +#endif > > Why ? reading the registers will work with M-Mode too. It was there before when > we temporarily had the builtin DTB lookup based on vendor/arch/imp (see defunct > soc_lookup_builtin_dtb() in 5.11). I don't understand what the objection is here. S-mode does not have these CSRs, so it uses SBI. M-mode does, so it reads the CSRs. This is the same logic as soc_lookup_builtin_dtb, except it works with S-mode as well. --Sean > >> +} >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >> index e85bacff1b50..03621ce9092c 100644 >> --- a/arch/riscv/kernel/setup.c >> +++ b/arch/riscv/kernel/setup.c >> @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) >> #endif >> >> riscv_fill_hwcap(); >> + > > Nit: I do not think the white libe is really necessary here. > >> + riscv_fill_cpu_manufacturer_info(); >> } >> >> static int __init topology_init(void) >> diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c >> index a0516172a33c..58f6fd91743a 100644 >> --- a/arch/riscv/kernel/soc.c >> +++ b/arch/riscv/kernel/soc.c >> @@ -6,6 +6,7 @@ >> #include <linux/libfdt.h> >> #include <linux/pgtable.h> >> #include <asm/soc.h> >> +#include <asm/hwcap.h> > > Why is this necessary ? > >> >> /* >> * This is called extremly early, before parse_dtb(), to allow initializing >> > >
> > #endif /* _ASM_RISCV_PROCESSOR_H */ > > diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h > > index f494066051a2..03dee6db404c 100644 > > --- a/arch/riscv/include/asm/soc.h > > +++ b/arch/riscv/include/asm/soc.h > > @@ -10,6 +10,7 @@ > > #include <linux/of.h> > > #include <linux/linkage.h> > > #include <linux/types.h> > > +#include <asm/hwcap.h> > > > > #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ > > static const struct of_device_id __soc_early_init__##name \ > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index ac202f44a670..389162ee19ea 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -12,6 +12,8 @@ > > #include <asm/hwcap.h> > > #include <asm/smp.h> > > #include <asm/switch_to.h> > > +#include <asm/sbi.h> > > +#include <asm/csr.h> > > > > unsigned long elf_hwcap __read_mostly; > > > > @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > > bool has_fpu __read_mostly; > > #endif > > > > +struct cpu_manufacturer_info_t cpu_mfr_info; > > + > > /** > > * riscv_isa_extension_base() - Get base extension word > > * > > @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) > > has_fpu = true; > > #endif > > } > > + > > +void riscv_fill_cpu_manufacturer_info(void) > > +{ > > +#ifndef CONFIG_RISCV_M_MODE > > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); > > + cpu_mfr_info.arch_id = sbi_get_archid(); > > + cpu_mfr_info.imp_id = sbi_get_impid(); > > +#else > > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); > > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); > > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); > > +#endif > > Why ? reading the registers will work with M-Mode too. It was there before when > we temporarily had the builtin DTB lookup based on vendor/arch/imp (see defunct > soc_lookup_builtin_dtb() in 5.11). > Sorry, I cannot fully catch what you mean. I agree that reading these registers will work with M-Mode, so I used the macro csr_read() to read these three registers when the kernel run in the M-mode. The definition of csr_read() is here: #define csr_read(csr) \ ({ \ register unsigned long __v; \ __asm__ __volatile__ ("csrr %0, " __ASM_STR(csr) \ : "=r" (__v) : \ : "memory"); \ __v; \ }) I guess that #if"n"def CONFIG_RISCV_M_MODE may mislead you. #ifndef is really unfriendly to read. I will change #ifndef to #ifdef in my next version patch. > > +} > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index e85bacff1b50..03621ce9092c 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) > > #endif > > > > riscv_fill_hwcap(); > > + > > Nit: I do not think the white libe is really necessary here. > OK, I will remove it. > > + riscv_fill_cpu_manufacturer_info(); > > } > > > > static int __init topology_init(void) > > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c > > index a0516172a33c..58f6fd91743a 100644 > > --- a/arch/riscv/kernel/soc.c > > +++ b/arch/riscv/kernel/soc.c > > @@ -6,6 +6,7 @@ > > #include <linux/libfdt.h> > > #include <linux/pgtable.h> > > #include <asm/soc.h> > > +#include <asm/hwcap.h> > > Why is this necessary ? I forgot to remove it when doing code cleanup. Thank you for the reminder. I will remove it
Hi Vincent, On Mon, Mar 8, 2021 at 11:58 AM Vincent Chen <vincent.chen@sifive.com> wrote: > > Issue 3 SBI calls to get the vendor ID, architecture ID and implementation > ID early in boot so we only need to take the SBI call overhead once. > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > --- > arch/riscv/include/asm/csr.h | 3 +++ > arch/riscv/include/asm/hwcap.h | 6 ++++++ > arch/riscv/include/asm/processor.h | 2 ++ > arch/riscv/include/asm/soc.h | 1 + > arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ > arch/riscv/kernel/setup.c | 2 ++ > arch/riscv/kernel/soc.c | 1 + > 7 files changed, 32 insertions(+) > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h > index caadfc1d7487..87ac65696871 100644 > --- a/arch/riscv/include/asm/csr.h > +++ b/arch/riscv/include/asm/csr.h > @@ -115,6 +115,9 @@ > #define CSR_MIP 0x344 > #define CSR_PMPCFG0 0x3a0 > #define CSR_PMPADDR0 0x3b0 > +#define CSR_MVENDORID 0xf11 > +#define CSR_MARCHID 0xf12 > +#define CSR_MIMPID 0xf13 > #define CSR_MHARTID 0xf14 > > #ifdef CONFIG_RISCV_M_MODE > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > index 5ce50468aff1..b7409487c9d2 100644 > --- a/arch/riscv/include/asm/hwcap.h > +++ b/arch/riscv/include/asm/hwcap.h > @@ -44,6 +44,12 @@ 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) > > +struct cpu_manufacturer_info_t { > + unsigned long vendor_id; > + unsigned long arch_id; > + unsigned long imp_id; > +}; > + > #endif > > #endif /* _ASM_RISCV_HWCAP_H */ > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > index 3a240037bde2..4e11a9621d14 100644 > --- a/arch/riscv/include/asm/processor.h > +++ b/arch/riscv/include/asm/processor.h > @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); > > extern void riscv_fill_hwcap(void); > > +void riscv_fill_cpu_manufacturer_info(void); > + > #endif /* __ASSEMBLY__ */ > > #endif /* _ASM_RISCV_PROCESSOR_H */ > diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h > index f494066051a2..03dee6db404c 100644 > --- a/arch/riscv/include/asm/soc.h > +++ b/arch/riscv/include/asm/soc.h > @@ -10,6 +10,7 @@ > #include <linux/of.h> > #include <linux/linkage.h> > #include <linux/types.h> > +#include <asm/hwcap.h> > > #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ > static const struct of_device_id __soc_early_init__##name \ > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index ac202f44a670..389162ee19ea 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -12,6 +12,8 @@ > #include <asm/hwcap.h> > #include <asm/smp.h> > #include <asm/switch_to.h> > +#include <asm/sbi.h> > +#include <asm/csr.h> > > unsigned long elf_hwcap __read_mostly; > > @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > bool has_fpu __read_mostly; > #endif > > +struct cpu_manufacturer_info_t cpu_mfr_info; > + > /** > * riscv_isa_extension_base() - Get base extension word > * > @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) > has_fpu = true; > #endif > } > + > +void riscv_fill_cpu_manufacturer_info(void) > +{ > +#ifndef CONFIG_RISCV_M_MODE > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); > + cpu_mfr_info.arch_id = sbi_get_archid(); > + cpu_mfr_info.imp_id = sbi_get_impid(); > +#else > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); > +#endif How about let opensbi emulate csr_read(CSR_MXXX) for S mode, then we needn't to define new sbi call. > +} > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index e85bacff1b50..03621ce9092c 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) > #endif > > riscv_fill_hwcap(); > + > + riscv_fill_cpu_manufacturer_info(); > } > > static int __init topology_init(void) > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c > index a0516172a33c..58f6fd91743a 100644 > --- a/arch/riscv/kernel/soc.c > +++ b/arch/riscv/kernel/soc.c > @@ -6,6 +6,7 @@ > #include <linux/libfdt.h> > #include <linux/pgtable.h> > #include <asm/soc.h> > +#include <asm/hwcap.h> > > /* > * This is called extremly early, before parse_dtb(), to allow initializing > -- > 2.7.4 > How
> > +void riscv_fill_cpu_manufacturer_info(void) > > +{ > > +#ifndef CONFIG_RISCV_M_MODE > > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); > > + cpu_mfr_info.arch_id = sbi_get_archid(); > > + cpu_mfr_info.imp_id = sbi_get_impid(); > > +#else > > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); > > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); > > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); > > +#endif > How about let opensbi emulate csr_read(CSR_MXXX) for S mode, then we > needn't to define new sbi call. > Hi Guo Ren, The SBI spec has defined 3 SBI calls for S-mode to get vendorid, archid, and impid, so I do not define new SBI calls here. The 3 functions ( sbi_get_vendorid(), sbi_get_archid(), and sbi_get_impid()) are the new kernel API to issue these 3 SBI calls. > > +} > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index e85bacff1b50..03621ce9092c 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) > > #endif > > > > riscv_fill_hwcap(); > > + > > + riscv_fill_cpu_manufacturer_info(); > > } > > > > static int __init topology_init(void) > > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c > > index a0516172a33c..58f6fd91743a 100644 > > --- a/arch/riscv/kernel/soc.c > > +++ b/arch/riscv/kernel/soc.c > > @@ -6,6 +6,7 @@ > > #include <linux/libfdt.h> > > #include <linux/pgtable.h> > > #include <asm/soc.h> > > +#include <asm/hwcap.h> > > > > /* > > * This is called extremly early, before parse_dtb(), to allow initializing > > -- > > 2.7.4 > > > How > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/
On 2021/03/09 10:25, Vincent Chen wrote: >>> #endif /* _ASM_RISCV_PROCESSOR_H */ >>> diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h >>> index f494066051a2..03dee6db404c 100644 >>> --- a/arch/riscv/include/asm/soc.h >>> +++ b/arch/riscv/include/asm/soc.h >>> @@ -10,6 +10,7 @@ >>> #include <linux/of.h> >>> #include <linux/linkage.h> >>> #include <linux/types.h> >>> +#include <asm/hwcap.h> >>> >>> #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ >>> static const struct of_device_id __soc_early_init__##name \ >>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >>> index ac202f44a670..389162ee19ea 100644 >>> --- a/arch/riscv/kernel/cpufeature.c >>> +++ b/arch/riscv/kernel/cpufeature.c >>> @@ -12,6 +12,8 @@ >>> #include <asm/hwcap.h> >>> #include <asm/smp.h> >>> #include <asm/switch_to.h> >>> +#include <asm/sbi.h> >>> +#include <asm/csr.h> >>> >>> unsigned long elf_hwcap __read_mostly; >>> >>> @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; >>> bool has_fpu __read_mostly; >>> #endif >>> >>> +struct cpu_manufacturer_info_t cpu_mfr_info; >>> + >>> /** >>> * riscv_isa_extension_base() - Get base extension word >>> * >>> @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) >>> has_fpu = true; >>> #endif >>> } >>> + >>> +void riscv_fill_cpu_manufacturer_info(void) >>> +{ >>> +#ifndef CONFIG_RISCV_M_MODE >>> + cpu_mfr_info.vendor_id = sbi_get_vendorid(); >>> + cpu_mfr_info.arch_id = sbi_get_archid(); >>> + cpu_mfr_info.imp_id = sbi_get_impid(); >>> +#else >>> + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); >>> + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); >>> + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); >>> +#endif >> >> Why ? reading the registers will work with M-Mode too. It was there before when >> we temporarily had the builtin DTB lookup based on vendor/arch/imp (see defunct >> soc_lookup_builtin_dtb() in 5.11). >> > Sorry, I cannot fully catch what you mean. > I agree that reading these registers will work with M-Mode, so I used > the macro csr_read() to read these three registers when the kernel run > in the M-mode. > The definition of csr_read() is here: > #define csr_read(csr) \ > ({ \ > register unsigned long __v; \ > __asm__ __volatile__ ("csrr %0, " __ASM_STR(csr) \ > : "=r" (__v) : \ > : "memory"); \ > __v; \ > }) > I guess that #if"n"def CONFIG_RISCV_M_MODE may mislead you. #ifndef is > really unfriendly to read. I will change #ifndef to #ifdef in my next > version patch. You are right. I totally misread this :) It was my morning and I needed more coffee ! Apologies for the noise. This looks fine. > > >>> +} >>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >>> index e85bacff1b50..03621ce9092c 100644 >>> --- a/arch/riscv/kernel/setup.c >>> +++ b/arch/riscv/kernel/setup.c >>> @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) >>> #endif >>> >>> riscv_fill_hwcap(); >>> + >> >> Nit: I do not think the white libe is really necessary here. >> > OK, I will remove it. >>> + riscv_fill_cpu_manufacturer_info(); >>> } >>> >>> static int __init topology_init(void) >>> diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c >>> index a0516172a33c..58f6fd91743a 100644 >>> --- a/arch/riscv/kernel/soc.c >>> +++ b/arch/riscv/kernel/soc.c >>> @@ -6,6 +6,7 @@ >>> #include <linux/libfdt.h> >>> #include <linux/pgtable.h> >>> #include <asm/soc.h> >>> +#include <asm/hwcap.h> >> >> Why is this necessary ? > > I forgot to remove it when doing code cleanup. Thank you for the > reminder. I will remove it >
On Tue, Mar 9, 2021 at 6:58 AM Guo Ren <guoren@kernel.org> wrote: > > Hi Vincent, > > On Mon, Mar 8, 2021 at 11:58 AM Vincent Chen <vincent.chen@sifive.com> wrote: > > > > Issue 3 SBI calls to get the vendor ID, architecture ID and implementation > > ID early in boot so we only need to take the SBI call overhead once. > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > --- > > arch/riscv/include/asm/csr.h | 3 +++ > > arch/riscv/include/asm/hwcap.h | 6 ++++++ > > arch/riscv/include/asm/processor.h | 2 ++ > > arch/riscv/include/asm/soc.h | 1 + > > arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ > > arch/riscv/kernel/setup.c | 2 ++ > > arch/riscv/kernel/soc.c | 1 + > > 7 files changed, 32 insertions(+) > > > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h > > index caadfc1d7487..87ac65696871 100644 > > --- a/arch/riscv/include/asm/csr.h > > +++ b/arch/riscv/include/asm/csr.h > > @@ -115,6 +115,9 @@ > > #define CSR_MIP 0x344 > > #define CSR_PMPCFG0 0x3a0 > > #define CSR_PMPADDR0 0x3b0 > > +#define CSR_MVENDORID 0xf11 > > +#define CSR_MARCHID 0xf12 > > +#define CSR_MIMPID 0xf13 > > #define CSR_MHARTID 0xf14 > > > > #ifdef CONFIG_RISCV_M_MODE > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > index 5ce50468aff1..b7409487c9d2 100644 > > --- a/arch/riscv/include/asm/hwcap.h > > +++ b/arch/riscv/include/asm/hwcap.h > > @@ -44,6 +44,12 @@ 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) > > > > +struct cpu_manufacturer_info_t { > > + unsigned long vendor_id; > > + unsigned long arch_id; > > + unsigned long imp_id; > > +}; > > + > > #endif > > > > #endif /* _ASM_RISCV_HWCAP_H */ > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > index 3a240037bde2..4e11a9621d14 100644 > > --- a/arch/riscv/include/asm/processor.h > > +++ b/arch/riscv/include/asm/processor.h > > @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); > > > > extern void riscv_fill_hwcap(void); > > > > +void riscv_fill_cpu_manufacturer_info(void); > > + > > #endif /* __ASSEMBLY__ */ > > > > #endif /* _ASM_RISCV_PROCESSOR_H */ > > diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h > > index f494066051a2..03dee6db404c 100644 > > --- a/arch/riscv/include/asm/soc.h > > +++ b/arch/riscv/include/asm/soc.h > > @@ -10,6 +10,7 @@ > > #include <linux/of.h> > > #include <linux/linkage.h> > > #include <linux/types.h> > > +#include <asm/hwcap.h> > > > > #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ > > static const struct of_device_id __soc_early_init__##name \ > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > index ac202f44a670..389162ee19ea 100644 > > --- a/arch/riscv/kernel/cpufeature.c > > +++ b/arch/riscv/kernel/cpufeature.c > > @@ -12,6 +12,8 @@ > > #include <asm/hwcap.h> > > #include <asm/smp.h> > > #include <asm/switch_to.h> > > +#include <asm/sbi.h> > > +#include <asm/csr.h> > > > > unsigned long elf_hwcap __read_mostly; > > > > @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > > bool has_fpu __read_mostly; > > #endif > > > > +struct cpu_manufacturer_info_t cpu_mfr_info; > > + > > /** > > * riscv_isa_extension_base() - Get base extension word > > * > > @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) > > has_fpu = true; > > #endif > > } > > + > > +void riscv_fill_cpu_manufacturer_info(void) > > +{ > > +#ifndef CONFIG_RISCV_M_MODE > > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); > > + cpu_mfr_info.arch_id = sbi_get_archid(); > > + cpu_mfr_info.imp_id = sbi_get_impid(); > > +#else > > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); > > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); > > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); > > +#endif > How about let opensbi emulate csr_read(CSR_MXXX) for S mode, then we > needn't to define new sbi call. Accessing M-mode CSRs from the S-mode kernel will only make things complicated for hypervisors because now hypervisors will also end-up emulating M-mode CSRs. Best would be to only access S-mode CSRs and SBI calls from S-mode kernel. > > > +} > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index e85bacff1b50..03621ce9092c 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) > > #endif > > > > riscv_fill_hwcap(); > > + > > + riscv_fill_cpu_manufacturer_info(); > > } > > > > static int __init topology_init(void) > > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c > > index a0516172a33c..58f6fd91743a 100644 > > --- a/arch/riscv/kernel/soc.c > > +++ b/arch/riscv/kernel/soc.c > > @@ -6,6 +6,7 @@ > > #include <linux/libfdt.h> > > #include <linux/pgtable.h> > > #include <asm/soc.h> > > +#include <asm/hwcap.h> > > > > /* > > * This is called extremly early, before parse_dtb(), to allow initializing > > -- > > 2.7.4 > > > How > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Got it. Thx On Tue, Mar 9, 2021 at 1:11 PM Anup Patel <anup@brainfault.org> wrote: > > On Tue, Mar 9, 2021 at 6:58 AM Guo Ren <guoren@kernel.org> wrote: > > > > Hi Vincent, > > > > On Mon, Mar 8, 2021 at 11:58 AM Vincent Chen <vincent.chen@sifive.com> wrote: > > > > > > Issue 3 SBI calls to get the vendor ID, architecture ID and implementation > > > ID early in boot so we only need to take the SBI call overhead once. > > > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > > > --- > > > arch/riscv/include/asm/csr.h | 3 +++ > > > arch/riscv/include/asm/hwcap.h | 6 ++++++ > > > arch/riscv/include/asm/processor.h | 2 ++ > > > arch/riscv/include/asm/soc.h | 1 + > > > arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ > > > arch/riscv/kernel/setup.c | 2 ++ > > > arch/riscv/kernel/soc.c | 1 + > > > 7 files changed, 32 insertions(+) > > > > > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h > > > index caadfc1d7487..87ac65696871 100644 > > > --- a/arch/riscv/include/asm/csr.h > > > +++ b/arch/riscv/include/asm/csr.h > > > @@ -115,6 +115,9 @@ > > > #define CSR_MIP 0x344 > > > #define CSR_PMPCFG0 0x3a0 > > > #define CSR_PMPADDR0 0x3b0 > > > +#define CSR_MVENDORID 0xf11 > > > +#define CSR_MARCHID 0xf12 > > > +#define CSR_MIMPID 0xf13 > > > #define CSR_MHARTID 0xf14 > > > > > > #ifdef CONFIG_RISCV_M_MODE > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > > > index 5ce50468aff1..b7409487c9d2 100644 > > > --- a/arch/riscv/include/asm/hwcap.h > > > +++ b/arch/riscv/include/asm/hwcap.h > > > @@ -44,6 +44,12 @@ 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) > > > > > > +struct cpu_manufacturer_info_t { > > > + unsigned long vendor_id; > > > + unsigned long arch_id; > > > + unsigned long imp_id; > > > +}; > > > + > > > #endif > > > > > > #endif /* _ASM_RISCV_HWCAP_H */ > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > index 3a240037bde2..4e11a9621d14 100644 > > > --- a/arch/riscv/include/asm/processor.h > > > +++ b/arch/riscv/include/asm/processor.h > > > @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); > > > > > > extern void riscv_fill_hwcap(void); > > > > > > +void riscv_fill_cpu_manufacturer_info(void); > > > + > > > #endif /* __ASSEMBLY__ */ > > > > > > #endif /* _ASM_RISCV_PROCESSOR_H */ > > > diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h > > > index f494066051a2..03dee6db404c 100644 > > > --- a/arch/riscv/include/asm/soc.h > > > +++ b/arch/riscv/include/asm/soc.h > > > @@ -10,6 +10,7 @@ > > > #include <linux/of.h> > > > #include <linux/linkage.h> > > > #include <linux/types.h> > > > +#include <asm/hwcap.h> > > > > > > #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ > > > static const struct of_device_id __soc_early_init__##name \ > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > > > index ac202f44a670..389162ee19ea 100644 > > > --- a/arch/riscv/kernel/cpufeature.c > > > +++ b/arch/riscv/kernel/cpufeature.c > > > @@ -12,6 +12,8 @@ > > > #include <asm/hwcap.h> > > > #include <asm/smp.h> > > > #include <asm/switch_to.h> > > > +#include <asm/sbi.h> > > > +#include <asm/csr.h> > > > > > > unsigned long elf_hwcap __read_mostly; > > > > > > @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > > > bool has_fpu __read_mostly; > > > #endif > > > > > > +struct cpu_manufacturer_info_t cpu_mfr_info; > > > + > > > /** > > > * riscv_isa_extension_base() - Get base extension word > > > * > > > @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) > > > has_fpu = true; > > > #endif > > > } > > > + > > > +void riscv_fill_cpu_manufacturer_info(void) > > > +{ > > > +#ifndef CONFIG_RISCV_M_MODE > > > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); > > > + cpu_mfr_info.arch_id = sbi_get_archid(); > > > + cpu_mfr_info.imp_id = sbi_get_impid(); > > > +#else > > > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); > > > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); > > > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); > > > +#endif > > How about let opensbi emulate csr_read(CSR_MXXX) for S mode, then we > > needn't to define new sbi call. > > Accessing M-mode CSRs from the S-mode kernel will only make things > complicated for hypervisors because now hypervisors will also end-up > emulating M-mode CSRs. > > Best would be to only access S-mode CSRs and SBI calls from > S-mode kernel. > > > > > > +} > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > > index e85bacff1b50..03621ce9092c 100644 > > > --- a/arch/riscv/kernel/setup.c > > > +++ b/arch/riscv/kernel/setup.c > > > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) > > > #endif > > > > > > riscv_fill_hwcap(); > > > + > > > + riscv_fill_cpu_manufacturer_info(); > > > } > > > > > > static int __init topology_init(void) > > > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c > > > index a0516172a33c..58f6fd91743a 100644 > > > --- a/arch/riscv/kernel/soc.c > > > +++ b/arch/riscv/kernel/soc.c > > > @@ -6,6 +6,7 @@ > > > #include <linux/libfdt.h> > > > #include <linux/pgtable.h> > > > #include <asm/soc.h> > > > +#include <asm/hwcap.h> > > > > > > /* > > > * This is called extremly early, before parse_dtb(), to allow initializing > > > -- > > > 2.7.4 > > > > > How > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/ > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, 08 Mar 2021 21:11:04 PST (-0800), anup@brainfault.org wrote: > On Tue, Mar 9, 2021 at 6:58 AM Guo Ren <guoren@kernel.org> wrote: >> >> Hi Vincent, >> >> On Mon, Mar 8, 2021 at 11:58 AM Vincent Chen <vincent.chen@sifive.com> wrote: >> > >> > Issue 3 SBI calls to get the vendor ID, architecture ID and implementation >> > ID early in boot so we only need to take the SBI call overhead once. >> > >> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> >> > --- >> > arch/riscv/include/asm/csr.h | 3 +++ >> > arch/riscv/include/asm/hwcap.h | 6 ++++++ >> > arch/riscv/include/asm/processor.h | 2 ++ >> > arch/riscv/include/asm/soc.h | 1 + >> > arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ >> > arch/riscv/kernel/setup.c | 2 ++ >> > arch/riscv/kernel/soc.c | 1 + >> > 7 files changed, 32 insertions(+) >> > >> > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h >> > index caadfc1d7487..87ac65696871 100644 >> > --- a/arch/riscv/include/asm/csr.h >> > +++ b/arch/riscv/include/asm/csr.h >> > @@ -115,6 +115,9 @@ >> > #define CSR_MIP 0x344 >> > #define CSR_PMPCFG0 0x3a0 >> > #define CSR_PMPADDR0 0x3b0 >> > +#define CSR_MVENDORID 0xf11 >> > +#define CSR_MARCHID 0xf12 >> > +#define CSR_MIMPID 0xf13 >> > #define CSR_MHARTID 0xf14 >> > >> > #ifdef CONFIG_RISCV_M_MODE >> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h >> > index 5ce50468aff1..b7409487c9d2 100644 >> > --- a/arch/riscv/include/asm/hwcap.h >> > +++ b/arch/riscv/include/asm/hwcap.h >> > @@ -44,6 +44,12 @@ 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) >> > >> > +struct cpu_manufacturer_info_t { >> > + unsigned long vendor_id; >> > + unsigned long arch_id; >> > + unsigned long imp_id; >> > +}; >> > + >> > #endif >> > >> > #endif /* _ASM_RISCV_HWCAP_H */ >> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h >> > index 3a240037bde2..4e11a9621d14 100644 >> > --- a/arch/riscv/include/asm/processor.h >> > +++ b/arch/riscv/include/asm/processor.h >> > @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); >> > >> > extern void riscv_fill_hwcap(void); >> > >> > +void riscv_fill_cpu_manufacturer_info(void); >> > + >> > #endif /* __ASSEMBLY__ */ >> > >> > #endif /* _ASM_RISCV_PROCESSOR_H */ >> > diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h >> > index f494066051a2..03dee6db404c 100644 >> > --- a/arch/riscv/include/asm/soc.h >> > +++ b/arch/riscv/include/asm/soc.h >> > @@ -10,6 +10,7 @@ >> > #include <linux/of.h> >> > #include <linux/linkage.h> >> > #include <linux/types.h> >> > +#include <asm/hwcap.h> >> > >> > #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ >> > static const struct of_device_id __soc_early_init__##name \ >> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c >> > index ac202f44a670..389162ee19ea 100644 >> > --- a/arch/riscv/kernel/cpufeature.c >> > +++ b/arch/riscv/kernel/cpufeature.c >> > @@ -12,6 +12,8 @@ >> > #include <asm/hwcap.h> >> > #include <asm/smp.h> >> > #include <asm/switch_to.h> >> > +#include <asm/sbi.h> >> > +#include <asm/csr.h> >> > >> > unsigned long elf_hwcap __read_mostly; >> > >> > @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; >> > bool has_fpu __read_mostly; >> > #endif >> > >> > +struct cpu_manufacturer_info_t cpu_mfr_info; >> > + >> > /** >> > * riscv_isa_extension_base() - Get base extension word >> > * >> > @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) >> > has_fpu = true; >> > #endif >> > } >> > + >> > +void riscv_fill_cpu_manufacturer_info(void) >> > +{ >> > +#ifndef CONFIG_RISCV_M_MODE >> > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); >> > + cpu_mfr_info.arch_id = sbi_get_archid(); >> > + cpu_mfr_info.imp_id = sbi_get_impid(); >> > +#else >> > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); >> > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); >> > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); >> > +#endif >> How about let opensbi emulate csr_read(CSR_MXXX) for S mode, then we >> needn't to define new sbi call. > > Accessing M-mode CSRs from the S-mode kernel will only make things > complicated for hypervisors because now hypervisors will also end-up > emulating M-mode CSRs. IMO that's not really the problem: hypervisors are going to have to emulate CSRs anyway, so adding more isn't a big deal. The problem is having S-mode software depend on M-mode. More explicitly: * The RISC-V specs are nominally layered, which means S-mode doesn't include any of the M-mode bits. M-mode CSR accesses are simply illegal S-mode instructions. I know the CSRs are a bit special in that they're chunked out by privilege mode, but there's nothing preventing the ISA from later reallocating these M-mode CSR bits in S-mode (aside from then having a modal ISA, which is a design anti-goal). * All behavior in M-mode is implementation defined, including these CSR accesses. While they're obviously allocated and a behavior is specified by the ISA, there's nothing that says M-mode has to actually implement these in any sane fashion (ie, it can return 0 or shut down or whatever). So is essence, adding these M-mode CSR accesses into S-mode kernels introduces an entirely new ISA extension that we're just making up on the spot and assuming will be implemented by firmware. At a bare minimum we'd need to have that defined by a specification, but even then I don't see it as the right way to go. > Best would be to only access S-mode CSRs and SBI calls from > S-mode kernel. Agreed. I'm not opposed to expanding the scope of the M-mode kernels to boot on more systems, but users who want S-mode shouldn't end up with M-mode dependencies sneaking in. So in this case, I'm very much in favor of the SBI call over accessing an M-mode CSR. > >> >> > +} >> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c >> > index e85bacff1b50..03621ce9092c 100644 >> > --- a/arch/riscv/kernel/setup.c >> > +++ b/arch/riscv/kernel/setup.c >> > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) >> > #endif >> > >> > riscv_fill_hwcap(); >> > + >> > + riscv_fill_cpu_manufacturer_info(); >> > } >> > >> > static int __init topology_init(void) >> > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c >> > index a0516172a33c..58f6fd91743a 100644 >> > --- a/arch/riscv/kernel/soc.c >> > +++ b/arch/riscv/kernel/soc.c >> > @@ -6,6 +6,7 @@ >> > #include <linux/libfdt.h> >> > #include <linux/pgtable.h> >> > #include <asm/soc.h> >> > +#include <asm/hwcap.h> >> > >> > /* >> > * This is called extremly early, before parse_dtb(), to allow initializing >> > -- >> > 2.7.4 >> > >> How >> >> -- >> Best Regards >> Guo Ren >> >> ML: https://lore.kernel.org/linux-csky/ >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
Got it. Thx for more clarification. On Wed, Mar 10, 2021 at 11:40 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Mon, 08 Mar 2021 21:11:04 PST (-0800), anup@brainfault.org wrote: > > On Tue, Mar 9, 2021 at 6:58 AM Guo Ren <guoren@kernel.org> wrote: > >> > >> Hi Vincent, > >> > >> On Mon, Mar 8, 2021 at 11:58 AM Vincent Chen <vincent.chen@sifive.com> wrote: > >> > > >> > Issue 3 SBI calls to get the vendor ID, architecture ID and implementation > >> > ID early in boot so we only need to take the SBI call overhead once. > >> > > >> > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > >> > --- > >> > arch/riscv/include/asm/csr.h | 3 +++ > >> > arch/riscv/include/asm/hwcap.h | 6 ++++++ > >> > arch/riscv/include/asm/processor.h | 2 ++ > >> > arch/riscv/include/asm/soc.h | 1 + > >> > arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ > >> > arch/riscv/kernel/setup.c | 2 ++ > >> > arch/riscv/kernel/soc.c | 1 + > >> > 7 files changed, 32 insertions(+) > >> > > >> > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h > >> > index caadfc1d7487..87ac65696871 100644 > >> > --- a/arch/riscv/include/asm/csr.h > >> > +++ b/arch/riscv/include/asm/csr.h > >> > @@ -115,6 +115,9 @@ > >> > #define CSR_MIP 0x344 > >> > #define CSR_PMPCFG0 0x3a0 > >> > #define CSR_PMPADDR0 0x3b0 > >> > +#define CSR_MVENDORID 0xf11 > >> > +#define CSR_MARCHID 0xf12 > >> > +#define CSR_MIMPID 0xf13 > >> > #define CSR_MHARTID 0xf14 > >> > > >> > #ifdef CONFIG_RISCV_M_MODE > >> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > >> > index 5ce50468aff1..b7409487c9d2 100644 > >> > --- a/arch/riscv/include/asm/hwcap.h > >> > +++ b/arch/riscv/include/asm/hwcap.h > >> > @@ -44,6 +44,12 @@ 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) > >> > > >> > +struct cpu_manufacturer_info_t { > >> > + unsigned long vendor_id; > >> > + unsigned long arch_id; > >> > + unsigned long imp_id; > >> > +}; > >> > + > >> > #endif > >> > > >> > #endif /* _ASM_RISCV_HWCAP_H */ > >> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > >> > index 3a240037bde2..4e11a9621d14 100644 > >> > --- a/arch/riscv/include/asm/processor.h > >> > +++ b/arch/riscv/include/asm/processor.h > >> > @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); > >> > > >> > extern void riscv_fill_hwcap(void); > >> > > >> > +void riscv_fill_cpu_manufacturer_info(void); > >> > + > >> > #endif /* __ASSEMBLY__ */ > >> > > >> > #endif /* _ASM_RISCV_PROCESSOR_H */ > >> > diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h > >> > index f494066051a2..03dee6db404c 100644 > >> > --- a/arch/riscv/include/asm/soc.h > >> > +++ b/arch/riscv/include/asm/soc.h > >> > @@ -10,6 +10,7 @@ > >> > #include <linux/of.h> > >> > #include <linux/linkage.h> > >> > #include <linux/types.h> > >> > +#include <asm/hwcap.h> > >> > > >> > #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ > >> > static const struct of_device_id __soc_early_init__##name \ > >> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > >> > index ac202f44a670..389162ee19ea 100644 > >> > --- a/arch/riscv/kernel/cpufeature.c > >> > +++ b/arch/riscv/kernel/cpufeature.c > >> > @@ -12,6 +12,8 @@ > >> > #include <asm/hwcap.h> > >> > #include <asm/smp.h> > >> > #include <asm/switch_to.h> > >> > +#include <asm/sbi.h> > >> > +#include <asm/csr.h> > >> > > >> > unsigned long elf_hwcap __read_mostly; > >> > > >> > @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > >> > bool has_fpu __read_mostly; > >> > #endif > >> > > >> > +struct cpu_manufacturer_info_t cpu_mfr_info; > >> > + > >> > /** > >> > * riscv_isa_extension_base() - Get base extension word > >> > * > >> > @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) > >> > has_fpu = true; > >> > #endif > >> > } > >> > + > >> > +void riscv_fill_cpu_manufacturer_info(void) > >> > +{ > >> > +#ifndef CONFIG_RISCV_M_MODE > >> > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); > >> > + cpu_mfr_info.arch_id = sbi_get_archid(); > >> > + cpu_mfr_info.imp_id = sbi_get_impid(); > >> > +#else > >> > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); > >> > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); > >> > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); > >> > +#endif > >> How about let opensbi emulate csr_read(CSR_MXXX) for S mode, then we > >> needn't to define new sbi call. > > > > Accessing M-mode CSRs from the S-mode kernel will only make things > > complicated for hypervisors because now hypervisors will also end-up > > emulating M-mode CSRs. > > IMO that's not really the problem: hypervisors are going to have to emulate > CSRs anyway, so adding more isn't a big deal. The problem is having S-mode > software depend on M-mode. More explicitly: > > * The RISC-V specs are nominally layered, which means S-mode doesn't include > any of the M-mode bits. M-mode CSR accesses are simply illegal S-mode > instructions. I know the CSRs are a bit special in that they're chunked out > by privilege mode, but there's nothing preventing the ISA from later > reallocating these M-mode CSR bits in S-mode (aside from then having a modal > ISA, which is a design anti-goal). > * All behavior in M-mode is implementation defined, including these CSR > accesses. While they're obviously allocated and a behavior is specified by > the ISA, there's nothing that says M-mode has to actually implement these in > any sane fashion (ie, it can return 0 or shut down or whatever). > > So is essence, adding these M-mode CSR accesses into S-mode kernels introduces > an entirely new ISA extension that we're just making up on the spot and > assuming will be implemented by firmware. At a bare minimum we'd need to have > that defined by a specification, but even then I don't see it as the right way > to go. > > > Best would be to only access S-mode CSRs and SBI calls from > > S-mode kernel. > > Agreed. I'm not opposed to expanding the scope of the M-mode kernels to boot > on more systems, but users who want S-mode shouldn't end up with M-mode > dependencies sneaking in. So in this case, I'm very much in favor of the SBI > call over accessing an M-mode CSR. > > > > >> > >> > +} > >> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > >> > index e85bacff1b50..03621ce9092c 100644 > >> > --- a/arch/riscv/kernel/setup.c > >> > +++ b/arch/riscv/kernel/setup.c > >> > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) > >> > #endif > >> > > >> > riscv_fill_hwcap(); > >> > + > >> > + riscv_fill_cpu_manufacturer_info(); > >> > } > >> > > >> > static int __init topology_init(void) > >> > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c > >> > index a0516172a33c..58f6fd91743a 100644 > >> > --- a/arch/riscv/kernel/soc.c > >> > +++ b/arch/riscv/kernel/soc.c > >> > @@ -6,6 +6,7 @@ > >> > #include <linux/libfdt.h> > >> > #include <linux/pgtable.h> > >> > #include <asm/soc.h> > >> > +#include <asm/hwcap.h> > >> > > >> > /* > >> > * This is called extremly early, before parse_dtb(), to allow initializing > >> > -- > >> > 2.7.4 > >> > > >> How > >> > >> -- > >> Best Regards > >> Guo Ren > >> > >> ML: https://lore.kernel.org/linux-csky/ > >> > >> _______________________________________________ > >> linux-riscv mailing list > >> linux-riscv@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Sun, 07 Mar 2021 19:58:15 PST (-0800), vincent.chen@sifive.com wrote: > Issue 3 SBI calls to get the vendor ID, architecture ID and implementation > ID early in boot so we only need to take the SBI call overhead once. > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com> > --- > arch/riscv/include/asm/csr.h | 3 +++ > arch/riscv/include/asm/hwcap.h | 6 ++++++ > arch/riscv/include/asm/processor.h | 2 ++ > arch/riscv/include/asm/soc.h | 1 + > arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ > arch/riscv/kernel/setup.c | 2 ++ > arch/riscv/kernel/soc.c | 1 + > 7 files changed, 32 insertions(+) > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h > index caadfc1d7487..87ac65696871 100644 > --- a/arch/riscv/include/asm/csr.h > +++ b/arch/riscv/include/asm/csr.h > @@ -115,6 +115,9 @@ > #define CSR_MIP 0x344 > #define CSR_PMPCFG0 0x3a0 > #define CSR_PMPADDR0 0x3b0 > +#define CSR_MVENDORID 0xf11 > +#define CSR_MARCHID 0xf12 > +#define CSR_MIMPID 0xf13 > #define CSR_MHARTID 0xf14 > > #ifdef CONFIG_RISCV_M_MODE > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h > index 5ce50468aff1..b7409487c9d2 100644 > --- a/arch/riscv/include/asm/hwcap.h > +++ b/arch/riscv/include/asm/hwcap.h > @@ -44,6 +44,12 @@ 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) > > +struct cpu_manufacturer_info_t { > + unsigned long vendor_id; > + unsigned long arch_id; > + unsigned long imp_id; > +}; > + > #endif > > #endif /* _ASM_RISCV_HWCAP_H */ > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > index 3a240037bde2..4e11a9621d14 100644 > --- a/arch/riscv/include/asm/processor.h > +++ b/arch/riscv/include/asm/processor.h > @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); > > extern void riscv_fill_hwcap(void); > > +void riscv_fill_cpu_manufacturer_info(void); > + > #endif /* __ASSEMBLY__ */ > > #endif /* _ASM_RISCV_PROCESSOR_H */ > diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h > index f494066051a2..03dee6db404c 100644 > --- a/arch/riscv/include/asm/soc.h > +++ b/arch/riscv/include/asm/soc.h > @@ -10,6 +10,7 @@ > #include <linux/of.h> > #include <linux/linkage.h> > #include <linux/types.h> > +#include <asm/hwcap.h> > > #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ > static const struct of_device_id __soc_early_init__##name \ > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c > index ac202f44a670..389162ee19ea 100644 > --- a/arch/riscv/kernel/cpufeature.c > +++ b/arch/riscv/kernel/cpufeature.c > @@ -12,6 +12,8 @@ > #include <asm/hwcap.h> > #include <asm/smp.h> > #include <asm/switch_to.h> > +#include <asm/sbi.h> > +#include <asm/csr.h> > > unsigned long elf_hwcap __read_mostly; > > @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; > bool has_fpu __read_mostly; > #endif > > +struct cpu_manufacturer_info_t cpu_mfr_info; > + > /** > * riscv_isa_extension_base() - Get base extension word > * > @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) > has_fpu = true; > #endif > } > + > +void riscv_fill_cpu_manufacturer_info(void) > +{ > +#ifndef CONFIG_RISCV_M_MODE > + cpu_mfr_info.vendor_id = sbi_get_vendorid(); > + cpu_mfr_info.arch_id = sbi_get_archid(); > + cpu_mfr_info.imp_id = sbi_get_impid(); > +#else > + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); > + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); > + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); > +#endif > +} > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index e85bacff1b50..03621ce9092c 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) > #endif > > riscv_fill_hwcap(); > + > + riscv_fill_cpu_manufacturer_info(); > } > > static int __init topology_init(void) > diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c > index a0516172a33c..58f6fd91743a 100644 > --- a/arch/riscv/kernel/soc.c > +++ b/arch/riscv/kernel/soc.c > @@ -6,6 +6,7 @@ > #include <linux/libfdt.h> > #include <linux/pgtable.h> > #include <asm/soc.h> > +#include <asm/hwcap.h> > > /* > * This is called extremly early, before parse_dtb(), to allow initializing Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h index caadfc1d7487..87ac65696871 100644 --- a/arch/riscv/include/asm/csr.h +++ b/arch/riscv/include/asm/csr.h @@ -115,6 +115,9 @@ #define CSR_MIP 0x344 #define CSR_PMPCFG0 0x3a0 #define CSR_PMPADDR0 0x3b0 +#define CSR_MVENDORID 0xf11 +#define CSR_MARCHID 0xf12 +#define CSR_MIMPID 0xf13 #define CSR_MHARTID 0xf14 #ifdef CONFIG_RISCV_M_MODE diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h index 5ce50468aff1..b7409487c9d2 100644 --- a/arch/riscv/include/asm/hwcap.h +++ b/arch/riscv/include/asm/hwcap.h @@ -44,6 +44,12 @@ 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) +struct cpu_manufacturer_info_t { + unsigned long vendor_id; + unsigned long arch_id; + unsigned long imp_id; +}; + #endif #endif /* _ASM_RISCV_HWCAP_H */ diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h index 3a240037bde2..4e11a9621d14 100644 --- a/arch/riscv/include/asm/processor.h +++ b/arch/riscv/include/asm/processor.h @@ -72,6 +72,8 @@ int riscv_of_parent_hartid(struct device_node *node); extern void riscv_fill_hwcap(void); +void riscv_fill_cpu_manufacturer_info(void); + #endif /* __ASSEMBLY__ */ #endif /* _ASM_RISCV_PROCESSOR_H */ diff --git a/arch/riscv/include/asm/soc.h b/arch/riscv/include/asm/soc.h index f494066051a2..03dee6db404c 100644 --- a/arch/riscv/include/asm/soc.h +++ b/arch/riscv/include/asm/soc.h @@ -10,6 +10,7 @@ #include <linux/of.h> #include <linux/linkage.h> #include <linux/types.h> +#include <asm/hwcap.h> #define SOC_EARLY_INIT_DECLARE(name, compat, fn) \ static const struct of_device_id __soc_early_init__##name \ diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index ac202f44a670..389162ee19ea 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -12,6 +12,8 @@ #include <asm/hwcap.h> #include <asm/smp.h> #include <asm/switch_to.h> +#include <asm/sbi.h> +#include <asm/csr.h> unsigned long elf_hwcap __read_mostly; @@ -22,6 +24,8 @@ static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly; bool has_fpu __read_mostly; #endif +struct cpu_manufacturer_info_t cpu_mfr_info; + /** * riscv_isa_extension_base() - Get base extension word * @@ -149,3 +153,16 @@ void riscv_fill_hwcap(void) has_fpu = true; #endif } + +void riscv_fill_cpu_manufacturer_info(void) +{ +#ifndef CONFIG_RISCV_M_MODE + cpu_mfr_info.vendor_id = sbi_get_vendorid(); + cpu_mfr_info.arch_id = sbi_get_archid(); + cpu_mfr_info.imp_id = sbi_get_impid(); +#else + cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID); + cpu_mfr_info.arch_id = csr_read(CSR_MARCHID); + cpu_mfr_info.imp_id = csr_read(CSR_MIMPID); +#endif +} diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index e85bacff1b50..03621ce9092c 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -278,6 +278,8 @@ void __init setup_arch(char **cmdline_p) #endif riscv_fill_hwcap(); + + riscv_fill_cpu_manufacturer_info(); } static int __init topology_init(void) diff --git a/arch/riscv/kernel/soc.c b/arch/riscv/kernel/soc.c index a0516172a33c..58f6fd91743a 100644 --- a/arch/riscv/kernel/soc.c +++ b/arch/riscv/kernel/soc.c @@ -6,6 +6,7 @@ #include <linux/libfdt.h> #include <linux/pgtable.h> #include <asm/soc.h> +#include <asm/hwcap.h> /* * This is called extremly early, before parse_dtb(), to allow initializing
Issue 3 SBI calls to get the vendor ID, architecture ID and implementation ID early in boot so we only need to take the SBI call overhead once. Signed-off-by: Vincent Chen <vincent.chen@sifive.com> --- arch/riscv/include/asm/csr.h | 3 +++ arch/riscv/include/asm/hwcap.h | 6 ++++++ arch/riscv/include/asm/processor.h | 2 ++ arch/riscv/include/asm/soc.h | 1 + arch/riscv/kernel/cpufeature.c | 17 +++++++++++++++++ arch/riscv/kernel/setup.c | 2 ++ arch/riscv/kernel/soc.c | 1 + 7 files changed, 32 insertions(+)