| Message ID | 20230628213033.170315-12-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv, KVM: fixes and enhancements | expand |
On 28/6/23 23:30, Daniel Henrique Barboza wrote: > Next patch will add KVM specific user properties for both MISA and > multi-letter extensions. For MISA extensions we want to make use of what > is already available in misa_ext_cfgs[] to avoid code repetition. > > misa_ext_info_arr[] array will hold name and description for each MISA > extension that misa_ext_cfgs[] is declaring. We'll then use this new > array in KVM code to avoid duplicating strings. > > There's nothing holding us back from doing the same with multi-letter > extensions. For now doing just with MISA extensions is enough. > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++---------------- > target/riscv/cpu.h | 7 +++- > 2 files changed, 61 insertions(+), 29 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 2485e820f8..90dd2078ae 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name, > visit_type_bool(v, name, &value, errp); > } > > -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = { > - {.name = "a", .description = "Atomic instructions", > - .misa_bit = RVA, .enabled = true}, > - {.name = "c", .description = "Compressed instructions", > - .misa_bit = RVC, .enabled = true}, > - {.name = "d", .description = "Double-precision float point", > - .misa_bit = RVD, .enabled = true}, > - {.name = "f", .description = "Single-precision float point", > - .misa_bit = RVF, .enabled = true}, > - {.name = "i", .description = "Base integer instruction set", > - .misa_bit = RVI, .enabled = true}, > - {.name = "e", .description = "Base integer instruction set (embedded)", > - .misa_bit = RVE, .enabled = false}, > - {.name = "m", .description = "Integer multiplication and division", > - .misa_bit = RVM, .enabled = true}, > - {.name = "s", .description = "Supervisor-level instructions", > - .misa_bit = RVS, .enabled = true}, > - {.name = "u", .description = "User-level instructions", > - .misa_bit = RVU, .enabled = true}, > - {.name = "h", .description = "Hypervisor", > - .misa_bit = RVH, .enabled = true}, > - {.name = "x-j", .description = "Dynamic translated languages", > - .misa_bit = RVJ, .enabled = false}, > - {.name = "v", .description = "Vector operations", > - .misa_bit = RVV, .enabled = false}, > - {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)", > - .misa_bit = RVG, .enabled = false}, > +typedef struct misa_ext_info { > + const char *name; > + const char *description; > +} MISAExtInfo; > + > +#define MISA_EXT_INFO(_idx, _propname, _descr) \ > + [(_idx - 'A')] = {.name = _propname, .description = _descr} > + > +static const MISAExtInfo misa_ext_info_arr[] = { > + MISA_EXT_INFO('A', "a", "Atomic instructions"), > + MISA_EXT_INFO('C', "c", "Compressed instructions"), > + MISA_EXT_INFO('D', "d", "Double-precision float point"), > + MISA_EXT_INFO('F', "f", "Single-precision float point"), > + MISA_EXT_INFO('I', "i", "Base integer instruction set"), > + MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"), > + MISA_EXT_INFO('M', "m", "Integer multiplication and division"), > + MISA_EXT_INFO('S', "s", "Supervisor-level instructions"), > + MISA_EXT_INFO('U', "u", "User-level instructions"), > + MISA_EXT_INFO('H', "h", "Hypervisor"), > + MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"), > + MISA_EXT_INFO('V', "v", "Vector operations"), > + MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"), > +}; > + > +const char *riscv_get_misa_ext_name(uint32_t bit) > +{ Is that OK to return NULL, or should we assert for unimplemented bit/feature? > + return misa_ext_info_arr[ctz32(bit)].name; > +} > + > +const char *riscv_get_misa_ext_descr(uint32_t bit) > +{ > + return misa_ext_info_arr[ctz32(bit)].description; Ditto. > +} > + > +#define MISA_CFG(_bit, _enabled) \ > + {.misa_bit = _bit, .enabled = _enabled} > + > +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = { 'const' > + MISA_CFG(RVA, true), > + MISA_CFG(RVC, true), > + MISA_CFG(RVD, true), > + MISA_CFG(RVF, true), > + MISA_CFG(RVI, true), > + MISA_CFG(RVE, false), > + MISA_CFG(RVM, true), > + MISA_CFG(RVS, true), > + MISA_CFG(RVU, true), > + MISA_CFG(RVH, true), > + MISA_CFG(RVJ, false), > + MISA_CFG(RVV, false), > + MISA_CFG(RVG, false), > }; > > static void riscv_cpu_add_misa_properties(Object *cpu_obj) > @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj) > int i; > > for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) { > - const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; > + RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; const > + > + misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit); > + misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit); Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Wed, Jun 28, 2023 at 06:30:24PM -0300, Daniel Henrique Barboza wrote: > Next patch will add KVM specific user properties for both MISA and > multi-letter extensions. For MISA extensions we want to make use of what > is already available in misa_ext_cfgs[] to avoid code repetition. > > misa_ext_info_arr[] array will hold name and description for each MISA > extension that misa_ext_cfgs[] is declaring. We'll then use this new > array in KVM code to avoid duplicating strings. > > There's nothing holding us back from doing the same with multi-letter > extensions. For now doing just with MISA extensions is enough. > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++---------------- > target/riscv/cpu.h | 7 +++- > 2 files changed, 61 insertions(+), 29 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 2485e820f8..90dd2078ae 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name, > visit_type_bool(v, name, &value, errp); > } > > -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = { > - {.name = "a", .description = "Atomic instructions", > - .misa_bit = RVA, .enabled = true}, > - {.name = "c", .description = "Compressed instructions", > - .misa_bit = RVC, .enabled = true}, > - {.name = "d", .description = "Double-precision float point", > - .misa_bit = RVD, .enabled = true}, > - {.name = "f", .description = "Single-precision float point", > - .misa_bit = RVF, .enabled = true}, > - {.name = "i", .description = "Base integer instruction set", > - .misa_bit = RVI, .enabled = true}, > - {.name = "e", .description = "Base integer instruction set (embedded)", > - .misa_bit = RVE, .enabled = false}, > - {.name = "m", .description = "Integer multiplication and division", > - .misa_bit = RVM, .enabled = true}, > - {.name = "s", .description = "Supervisor-level instructions", > - .misa_bit = RVS, .enabled = true}, > - {.name = "u", .description = "User-level instructions", > - .misa_bit = RVU, .enabled = true}, > - {.name = "h", .description = "Hypervisor", > - .misa_bit = RVH, .enabled = true}, > - {.name = "x-j", .description = "Dynamic translated languages", > - .misa_bit = RVJ, .enabled = false}, > - {.name = "v", .description = "Vector operations", > - .misa_bit = RVV, .enabled = false}, > - {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)", > - .misa_bit = RVG, .enabled = false}, > +typedef struct misa_ext_info { > + const char *name; > + const char *description; > +} MISAExtInfo; > + > +#define MISA_EXT_INFO(_idx, _propname, _descr) \ > + [(_idx - 'A')] = {.name = _propname, .description = _descr} We don't have to give up on passing RV* to this macro. Directly using __builtin_ctz() with a constant should work, i.e. #define MISA_EXT_INFO(_bit, _propname, _descr) \ [__builtin_ctz(_bit)] = {.name = _propname, .description = _descr} and then MISA_EXT_INFO(RVA, "a", "Atomic instructions"), MISA_EXT_INFO(RVD, "d", "Double-precision float point"), ... (We don't need the ctz32() wrapper since we know we'll never input zero to __builtin_ctz().) > + > +static const MISAExtInfo misa_ext_info_arr[] = { > + MISA_EXT_INFO('A', "a", "Atomic instructions"), > + MISA_EXT_INFO('C', "c", "Compressed instructions"), > + MISA_EXT_INFO('D', "d", "Double-precision float point"), > + MISA_EXT_INFO('F', "f", "Single-precision float point"), > + MISA_EXT_INFO('I', "i", "Base integer instruction set"), > + MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"), > + MISA_EXT_INFO('M', "m", "Integer multiplication and division"), > + MISA_EXT_INFO('S', "s", "Supervisor-level instructions"), > + MISA_EXT_INFO('U', "u", "User-level instructions"), > + MISA_EXT_INFO('H', "h", "Hypervisor"), > + MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"), > + MISA_EXT_INFO('V', "v", "Vector operations"), > + MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"), > +}; > + > +const char *riscv_get_misa_ext_name(uint32_t bit) > +{ > + return misa_ext_info_arr[ctz32(bit)].name; > +} > + > +const char *riscv_get_misa_ext_descr(uint32_t bit) nit: I'd prefer riscv_get_misa_ext_description() for the name. > +{ > + return misa_ext_info_arr[ctz32(bit)].description; > +} > + > +#define MISA_CFG(_bit, _enabled) \ > + {.misa_bit = _bit, .enabled = _enabled} > + > +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = { > + MISA_CFG(RVA, true), > + MISA_CFG(RVC, true), > + MISA_CFG(RVD, true), > + MISA_CFG(RVF, true), > + MISA_CFG(RVI, true), > + MISA_CFG(RVE, false), > + MISA_CFG(RVM, true), > + MISA_CFG(RVS, true), > + MISA_CFG(RVU, true), > + MISA_CFG(RVH, true), > + MISA_CFG(RVJ, false), > + MISA_CFG(RVV, false), > + MISA_CFG(RVG, false), > }; > > static void riscv_cpu_add_misa_properties(Object *cpu_obj) > @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj) > int i; > > for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) { > - const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; > + RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; > + > + misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit); > + misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit); This might be necessary for the KVM side, but we should be able to avoid this runtime setting here where the compiler can be certain everything is a constant. We just need the old MISA_CFG() back, slightly tweaked to use __builtin_ctz(). > > object_property_add(cpu_obj, misa_cfg->name, "bool", > cpu_get_misa_ext_cfg, > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index cc20ee25a7..acae118b9b 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -41,7 +41,10 @@ > > #define RV(x) ((target_ulong)1 << (x - 'A')) > > -/* Consider updating misa_ext_cfgs[] when adding new MISA bits here */ > +/* > + * Consider updating misa_ext_info_arr[] and misa_ext_cfgs[] > + * when adding new MISA bits here. > + */ > #define RVI RV('I') > #define RVE RV('E') /* E and I are mutually exclusive */ > #define RVM RV('M') > @@ -56,6 +59,8 @@ > #define RVJ RV('J') > #define RVG RV('G') > > +const char *riscv_get_misa_ext_name(uint32_t bit); > +const char *riscv_get_misa_ext_descr(uint32_t bit); > > /* Privileged specification version */ > enum { > -- > 2.41.0 > Thanks, drew
On 6/29/23 04:26, Philippe Mathieu-Daudé wrote: > On 28/6/23 23:30, Daniel Henrique Barboza wrote: >> Next patch will add KVM specific user properties for both MISA and >> multi-letter extensions. For MISA extensions we want to make use of what >> is already available in misa_ext_cfgs[] to avoid code repetition. >> >> misa_ext_info_arr[] array will hold name and description for each MISA >> extension that misa_ext_cfgs[] is declaring. We'll then use this new >> array in KVM code to avoid duplicating strings. >> >> There's nothing holding us back from doing the same with multi-letter >> extensions. For now doing just with MISA extensions is enough. >> >> Suggested-by: Andrew Jones <ajones@ventanamicro.com> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++---------------- >> target/riscv/cpu.h | 7 +++- >> 2 files changed, 61 insertions(+), 29 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 2485e820f8..90dd2078ae 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name, >> visit_type_bool(v, name, &value, errp); >> } >> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = { >> - {.name = "a", .description = "Atomic instructions", >> - .misa_bit = RVA, .enabled = true}, >> - {.name = "c", .description = "Compressed instructions", >> - .misa_bit = RVC, .enabled = true}, >> - {.name = "d", .description = "Double-precision float point", >> - .misa_bit = RVD, .enabled = true}, >> - {.name = "f", .description = "Single-precision float point", >> - .misa_bit = RVF, .enabled = true}, >> - {.name = "i", .description = "Base integer instruction set", >> - .misa_bit = RVI, .enabled = true}, >> - {.name = "e", .description = "Base integer instruction set (embedded)", >> - .misa_bit = RVE, .enabled = false}, >> - {.name = "m", .description = "Integer multiplication and division", >> - .misa_bit = RVM, .enabled = true}, >> - {.name = "s", .description = "Supervisor-level instructions", >> - .misa_bit = RVS, .enabled = true}, >> - {.name = "u", .description = "User-level instructions", >> - .misa_bit = RVU, .enabled = true}, >> - {.name = "h", .description = "Hypervisor", >> - .misa_bit = RVH, .enabled = true}, >> - {.name = "x-j", .description = "Dynamic translated languages", >> - .misa_bit = RVJ, .enabled = false}, >> - {.name = "v", .description = "Vector operations", >> - .misa_bit = RVV, .enabled = false}, >> - {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)", >> - .misa_bit = RVG, .enabled = false}, >> +typedef struct misa_ext_info { >> + const char *name; >> + const char *description; >> +} MISAExtInfo; >> + >> +#define MISA_EXT_INFO(_idx, _propname, _descr) \ >> + [(_idx - 'A')] = {.name = _propname, .description = _descr} >> + >> +static const MISAExtInfo misa_ext_info_arr[] = { >> + MISA_EXT_INFO('A', "a", "Atomic instructions"), >> + MISA_EXT_INFO('C', "c", "Compressed instructions"), >> + MISA_EXT_INFO('D', "d", "Double-precision float point"), >> + MISA_EXT_INFO('F', "f", "Single-precision float point"), >> + MISA_EXT_INFO('I', "i", "Base integer instruction set"), >> + MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"), >> + MISA_EXT_INFO('M', "m", "Integer multiplication and division"), >> + MISA_EXT_INFO('S', "s", "Supervisor-level instructions"), >> + MISA_EXT_INFO('U', "u", "User-level instructions"), >> + MISA_EXT_INFO('H', "h", "Hypervisor"), >> + MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"), >> + MISA_EXT_INFO('V', "v", "Vector operations"), >> + MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"), >> +}; >> + >> +const char *riscv_get_misa_ext_name(uint32_t bit) >> +{ > > Is that OK to return NULL, or should we assert for > unimplemented bit/feature? > >> + return misa_ext_info_arr[ctz32(bit)].name; >> +} >> + >> +const char *riscv_get_misa_ext_descr(uint32_t bit) >> +{ >> + return misa_ext_info_arr[ctz32(bit)].description; > > Ditto. > >> +} >> + >> +#define MISA_CFG(_bit, _enabled) \ >> + {.misa_bit = _bit, .enabled = _enabled} >> + >> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = { > > 'const' Not sure why I got rid of 'const' here. I'll reintroduce it. Daniel > >> + MISA_CFG(RVA, true), >> + MISA_CFG(RVC, true), >> + MISA_CFG(RVD, true), >> + MISA_CFG(RVF, true), >> + MISA_CFG(RVI, true), >> + MISA_CFG(RVE, false), >> + MISA_CFG(RVM, true), >> + MISA_CFG(RVS, true), >> + MISA_CFG(RVU, true), >> + MISA_CFG(RVH, true), >> + MISA_CFG(RVJ, false), >> + MISA_CFG(RVV, false), >> + MISA_CFG(RVG, false), >> }; >> static void riscv_cpu_add_misa_properties(Object *cpu_obj) >> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj) >> int i; >> for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) { >> - const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; >> + RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; > > const > >> + >> + misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit); >> + misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit); > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
On 6/29/23 05:59, Andrew Jones wrote: > On Wed, Jun 28, 2023 at 06:30:24PM -0300, Daniel Henrique Barboza wrote: >> Next patch will add KVM specific user properties for both MISA and >> multi-letter extensions. For MISA extensions we want to make use of what >> is already available in misa_ext_cfgs[] to avoid code repetition. >> >> misa_ext_info_arr[] array will hold name and description for each MISA >> extension that misa_ext_cfgs[] is declaring. We'll then use this new >> array in KVM code to avoid duplicating strings. >> >> There's nothing holding us back from doing the same with multi-letter >> extensions. For now doing just with MISA extensions is enough. >> >> Suggested-by: Andrew Jones <ajones@ventanamicro.com> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++---------------- >> target/riscv/cpu.h | 7 +++- >> 2 files changed, 61 insertions(+), 29 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 2485e820f8..90dd2078ae 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name, >> visit_type_bool(v, name, &value, errp); >> } >> >> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = { >> - {.name = "a", .description = "Atomic instructions", >> - .misa_bit = RVA, .enabled = true}, >> - {.name = "c", .description = "Compressed instructions", >> - .misa_bit = RVC, .enabled = true}, >> - {.name = "d", .description = "Double-precision float point", >> - .misa_bit = RVD, .enabled = true}, >> - {.name = "f", .description = "Single-precision float point", >> - .misa_bit = RVF, .enabled = true}, >> - {.name = "i", .description = "Base integer instruction set", >> - .misa_bit = RVI, .enabled = true}, >> - {.name = "e", .description = "Base integer instruction set (embedded)", >> - .misa_bit = RVE, .enabled = false}, >> - {.name = "m", .description = "Integer multiplication and division", >> - .misa_bit = RVM, .enabled = true}, >> - {.name = "s", .description = "Supervisor-level instructions", >> - .misa_bit = RVS, .enabled = true}, >> - {.name = "u", .description = "User-level instructions", >> - .misa_bit = RVU, .enabled = true}, >> - {.name = "h", .description = "Hypervisor", >> - .misa_bit = RVH, .enabled = true}, >> - {.name = "x-j", .description = "Dynamic translated languages", >> - .misa_bit = RVJ, .enabled = false}, >> - {.name = "v", .description = "Vector operations", >> - .misa_bit = RVV, .enabled = false}, >> - {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)", >> - .misa_bit = RVG, .enabled = false}, >> +typedef struct misa_ext_info { >> + const char *name; >> + const char *description; >> +} MISAExtInfo; >> + >> +#define MISA_EXT_INFO(_idx, _propname, _descr) \ >> + [(_idx - 'A')] = {.name = _propname, .description = _descr} > > We don't have to give up on passing RV* to this macro. Directly > using __builtin_ctz() with a constant should work, i.e. > > #define MISA_EXT_INFO(_bit, _propname, _descr) \ > [__builtin_ctz(_bit)] = {.name = _propname, .description = _descr} > > and then > > MISA_EXT_INFO(RVA, "a", "Atomic instructions"), > MISA_EXT_INFO(RVD, "d", "Double-precision float point"), > ... > > (We don't need the ctz32() wrapper since we know we'll never input zero to > __builtin_ctz().) I'll give it a try. I'll spare us from having to assign name and descr during runtime (at least for this file). Daniel > >> + >> +static const MISAExtInfo misa_ext_info_arr[] = { >> + MISA_EXT_INFO('A', "a", "Atomic instructions"), >> + MISA_EXT_INFO('C', "c", "Compressed instructions"), >> + MISA_EXT_INFO('D', "d", "Double-precision float point"), >> + MISA_EXT_INFO('F', "f", "Single-precision float point"), >> + MISA_EXT_INFO('I', "i", "Base integer instruction set"), >> + MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"), >> + MISA_EXT_INFO('M', "m", "Integer multiplication and division"), >> + MISA_EXT_INFO('S', "s", "Supervisor-level instructions"), >> + MISA_EXT_INFO('U', "u", "User-level instructions"), >> + MISA_EXT_INFO('H', "h", "Hypervisor"), >> + MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"), >> + MISA_EXT_INFO('V', "v", "Vector operations"), >> + MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"), >> +}; >> + >> +const char *riscv_get_misa_ext_name(uint32_t bit) >> +{ >> + return misa_ext_info_arr[ctz32(bit)].name; >> +} >> + >> +const char *riscv_get_misa_ext_descr(uint32_t bit) > > nit: I'd prefer riscv_get_misa_ext_description() for the name. > >> +{ >> + return misa_ext_info_arr[ctz32(bit)].description; >> +} >> + >> +#define MISA_CFG(_bit, _enabled) \ >> + {.misa_bit = _bit, .enabled = _enabled} >> + >> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = { >> + MISA_CFG(RVA, true), >> + MISA_CFG(RVC, true), >> + MISA_CFG(RVD, true), >> + MISA_CFG(RVF, true), >> + MISA_CFG(RVI, true), >> + MISA_CFG(RVE, false), >> + MISA_CFG(RVM, true), >> + MISA_CFG(RVS, true), >> + MISA_CFG(RVU, true), >> + MISA_CFG(RVH, true), >> + MISA_CFG(RVJ, false), >> + MISA_CFG(RVV, false), >> + MISA_CFG(RVG, false), >> }; >> >> static void riscv_cpu_add_misa_properties(Object *cpu_obj) >> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj) >> int i; >> >> for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) { >> - const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; >> + RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; >> + >> + misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit); >> + misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit); > > This might be necessary for the KVM side, but we should be able to avoid > this runtime setting here where the compiler can be certain everything is > a constant. We just need the old MISA_CFG() back, slightly tweaked to use > __builtin_ctz(). > >> >> object_property_add(cpu_obj, misa_cfg->name, "bool", >> cpu_get_misa_ext_cfg, >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index cc20ee25a7..acae118b9b 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -41,7 +41,10 @@ >> >> #define RV(x) ((target_ulong)1 << (x - 'A')) >> >> -/* Consider updating misa_ext_cfgs[] when adding new MISA bits here */ >> +/* >> + * Consider updating misa_ext_info_arr[] and misa_ext_cfgs[] >> + * when adding new MISA bits here. >> + */ >> #define RVI RV('I') >> #define RVE RV('E') /* E and I are mutually exclusive */ >> #define RVM RV('M') >> @@ -56,6 +59,8 @@ >> #define RVJ RV('J') >> #define RVG RV('G') >> >> +const char *riscv_get_misa_ext_name(uint32_t bit); >> +const char *riscv_get_misa_ext_descr(uint32_t bit); >> >> /* Privileged specification version */ >> enum { >> -- >> 2.41.0 >> > > Thanks, > drew
On 6/29/23 08:36, Daniel Henrique Barboza wrote: > > > On 6/29/23 04:26, Philippe Mathieu-Daudé wrote: >> On 28/6/23 23:30, Daniel Henrique Barboza wrote: >>> Next patch will add KVM specific user properties for both MISA and >>> multi-letter extensions. For MISA extensions we want to make use of what >>> is already available in misa_ext_cfgs[] to avoid code repetition. >>> >>> misa_ext_info_arr[] array will hold name and description for each MISA >>> extension that misa_ext_cfgs[] is declaring. We'll then use this new >>> array in KVM code to avoid duplicating strings. >>> >>> There's nothing holding us back from doing the same with multi-letter >>> extensions. For now doing just with MISA extensions is enough. >>> >>> Suggested-by: Andrew Jones <ajones@ventanamicro.com> >>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>> --- >>> target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++---------------- >>> target/riscv/cpu.h | 7 +++- >>> 2 files changed, 61 insertions(+), 29 deletions(-) >>> >>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>> index 2485e820f8..90dd2078ae 100644 >>> --- a/target/riscv/cpu.c >>> +++ b/target/riscv/cpu.c >>> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name, >>> visit_type_bool(v, name, &value, errp); >>> } >>> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = { >>> - {.name = "a", .description = "Atomic instructions", >>> - .misa_bit = RVA, .enabled = true}, >>> - {.name = "c", .description = "Compressed instructions", >>> - .misa_bit = RVC, .enabled = true}, >>> - {.name = "d", .description = "Double-precision float point", >>> - .misa_bit = RVD, .enabled = true}, >>> - {.name = "f", .description = "Single-precision float point", >>> - .misa_bit = RVF, .enabled = true}, >>> - {.name = "i", .description = "Base integer instruction set", >>> - .misa_bit = RVI, .enabled = true}, >>> - {.name = "e", .description = "Base integer instruction set (embedded)", >>> - .misa_bit = RVE, .enabled = false}, >>> - {.name = "m", .description = "Integer multiplication and division", >>> - .misa_bit = RVM, .enabled = true}, >>> - {.name = "s", .description = "Supervisor-level instructions", >>> - .misa_bit = RVS, .enabled = true}, >>> - {.name = "u", .description = "User-level instructions", >>> - .misa_bit = RVU, .enabled = true}, >>> - {.name = "h", .description = "Hypervisor", >>> - .misa_bit = RVH, .enabled = true}, >>> - {.name = "x-j", .description = "Dynamic translated languages", >>> - .misa_bit = RVJ, .enabled = false}, >>> - {.name = "v", .description = "Vector operations", >>> - .misa_bit = RVV, .enabled = false}, >>> - {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)", >>> - .misa_bit = RVG, .enabled = false}, >>> +typedef struct misa_ext_info { >>> + const char *name; >>> + const char *description; >>> +} MISAExtInfo; >>> + >>> +#define MISA_EXT_INFO(_idx, _propname, _descr) \ >>> + [(_idx - 'A')] = {.name = _propname, .description = _descr} >>> + >>> +static const MISAExtInfo misa_ext_info_arr[] = { >>> + MISA_EXT_INFO('A', "a", "Atomic instructions"), >>> + MISA_EXT_INFO('C', "c", "Compressed instructions"), >>> + MISA_EXT_INFO('D', "d", "Double-precision float point"), >>> + MISA_EXT_INFO('F', "f", "Single-precision float point"), >>> + MISA_EXT_INFO('I', "i", "Base integer instruction set"), >>> + MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"), >>> + MISA_EXT_INFO('M', "m", "Integer multiplication and division"), >>> + MISA_EXT_INFO('S', "s", "Supervisor-level instructions"), >>> + MISA_EXT_INFO('U', "u", "User-level instructions"), >>> + MISA_EXT_INFO('H', "h", "Hypervisor"), >>> + MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"), >>> + MISA_EXT_INFO('V', "v", "Vector operations"), >>> + MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"), >>> +}; >>> + >>> +const char *riscv_get_misa_ext_name(uint32_t bit) >>> +{ >> >> Is that OK to return NULL, or should we assert for >> unimplemented bit/feature? It's easier to assert out if name or description is NULL (meaning that we don't implement the bit). >> >>> + return misa_ext_info_arr[ctz32(bit)].name; >>> +} >>> + >>> +const char *riscv_get_misa_ext_descr(uint32_t bit) >>> +{ >>> + return misa_ext_info_arr[ctz32(bit)].description; >> >> Ditto. >> >>> +} >>> + >>> +#define MISA_CFG(_bit, _enabled) \ >>> + {.misa_bit = _bit, .enabled = _enabled} >>> + >>> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = { >> >> 'const' > > Not sure why I got rid of 'const' here. I'll reintroduce it. Just remembered why. 'name' and 'description' are being initialized during runtime, so the array can't be 'const'. If we managed to init everything in the macro like Drew suggested we can keep it 'const'. Daniel > > > Daniel > >> >>> + MISA_CFG(RVA, true), >>> + MISA_CFG(RVC, true), >>> + MISA_CFG(RVD, true), >>> + MISA_CFG(RVF, true), >>> + MISA_CFG(RVI, true), >>> + MISA_CFG(RVE, false), >>> + MISA_CFG(RVM, true), >>> + MISA_CFG(RVS, true), >>> + MISA_CFG(RVU, true), >>> + MISA_CFG(RVH, true), >>> + MISA_CFG(RVJ, false), >>> + MISA_CFG(RVV, false), >>> + MISA_CFG(RVG, false), >>> }; >>> static void riscv_cpu_add_misa_properties(Object *cpu_obj) >>> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj) >>> int i; >>> for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) { >>> - const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; >>> + RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; >> >> const >> >>> + >>> + misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit); >>> + misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit); >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>
Drew, On 6/29/23 05:59, Andrew Jones wrote: > On Wed, Jun 28, 2023 at 06:30:24PM -0300, Daniel Henrique Barboza wrote: >> Next patch will add KVM specific user properties for both MISA and >> multi-letter extensions. For MISA extensions we want to make use of what >> is already available in misa_ext_cfgs[] to avoid code repetition. >> >> misa_ext_info_arr[] array will hold name and description for each MISA >> extension that misa_ext_cfgs[] is declaring. We'll then use this new >> array in KVM code to avoid duplicating strings. >> >> There's nothing holding us back from doing the same with multi-letter >> extensions. For now doing just with MISA extensions is enough. >> >> Suggested-by: Andrew Jones <ajones@ventanamicro.com> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++---------------- >> target/riscv/cpu.h | 7 +++- >> 2 files changed, 61 insertions(+), 29 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 2485e820f8..90dd2078ae 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name, >> visit_type_bool(v, name, &value, errp); >> } >> >> -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = { >> - {.name = "a", .description = "Atomic instructions", >> - .misa_bit = RVA, .enabled = true}, >> - {.name = "c", .description = "Compressed instructions", >> - .misa_bit = RVC, .enabled = true}, >> - {.name = "d", .description = "Double-precision float point", >> - .misa_bit = RVD, .enabled = true}, >> - {.name = "f", .description = "Single-precision float point", >> - .misa_bit = RVF, .enabled = true}, >> - {.name = "i", .description = "Base integer instruction set", >> - .misa_bit = RVI, .enabled = true}, >> - {.name = "e", .description = "Base integer instruction set (embedded)", >> - .misa_bit = RVE, .enabled = false}, >> - {.name = "m", .description = "Integer multiplication and division", >> - .misa_bit = RVM, .enabled = true}, >> - {.name = "s", .description = "Supervisor-level instructions", >> - .misa_bit = RVS, .enabled = true}, >> - {.name = "u", .description = "User-level instructions", >> - .misa_bit = RVU, .enabled = true}, >> - {.name = "h", .description = "Hypervisor", >> - .misa_bit = RVH, .enabled = true}, >> - {.name = "x-j", .description = "Dynamic translated languages", >> - .misa_bit = RVJ, .enabled = false}, >> - {.name = "v", .description = "Vector operations", >> - .misa_bit = RVV, .enabled = false}, >> - {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)", >> - .misa_bit = RVG, .enabled = false}, >> +typedef struct misa_ext_info { >> + const char *name; >> + const char *description; >> +} MISAExtInfo; >> + >> +#define MISA_EXT_INFO(_idx, _propname, _descr) \ >> + [(_idx - 'A')] = {.name = _propname, .description = _descr} > > We don't have to give up on passing RV* to this macro. Directly > using __builtin_ctz() with a constant should work, i.e. > > #define MISA_EXT_INFO(_bit, _propname, _descr) \ > [__builtin_ctz(_bit)] = {.name = _propname, .description = _descr} > > and then > > MISA_EXT_INFO(RVA, "a", "Atomic instructions"), > MISA_EXT_INFO(RVD, "d", "Double-precision float point"), > ... > > (We don't need the ctz32() wrapper since we know we'll never input zero to > __builtin_ctz().) I run the series through gitlab because I got worried about this change in different compilers and so on. And in fact I fear that we break 'clang-user' with it: https://gitlab.com/danielhb/qemu/-/jobs/4569265199 u.c.o -c ../target/riscv/cpu.c ../target/riscv/cpu.c:1624:5: error: initializer element is not a compile-time constant MISA_CFG(RVA, true), ^~~~~~~~~~~~~~~~~~~ ../target/riscv/cpu.c:1619:53: note: expanded from macro 'MISA_CFG' {.name = misa_ext_info_arr[MISA_INFO_IDX(_bit)].name, \ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~ 1 error generated. [1503/2619] Compiling C object libqemu-ppc64le-linux-user.fa.p/linux-user_syscall.c.o Which is a shame because gcc and everyone else is okay with it, but 'clang-user' and 'tsan-build' runners are complaining about it. Unless there's a directive to make clang accept this code (I didn't find any) we'll need to keep updating name and description during runtime, and we'll have to keep removing 'const' from misa_ext_cfgs[]. Thanks, Daniel > >> + >> +static const MISAExtInfo misa_ext_info_arr[] = { >> + MISA_EXT_INFO('A', "a", "Atomic instructions"), >> + MISA_EXT_INFO('C', "c", "Compressed instructions"), >> + MISA_EXT_INFO('D', "d", "Double-precision float point"), >> + MISA_EXT_INFO('F', "f", "Single-precision float point"), >> + MISA_EXT_INFO('I', "i", "Base integer instruction set"), >> + MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"), >> + MISA_EXT_INFO('M', "m", "Integer multiplication and division"), >> + MISA_EXT_INFO('S', "s", "Supervisor-level instructions"), >> + MISA_EXT_INFO('U', "u", "User-level instructions"), >> + MISA_EXT_INFO('H', "h", "Hypervisor"), >> + MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"), >> + MISA_EXT_INFO('V', "v", "Vector operations"), >> + MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"), >> +}; >> + >> +const char *riscv_get_misa_ext_name(uint32_t bit) >> +{ >> + return misa_ext_info_arr[ctz32(bit)].name; >> +} >> + >> +const char *riscv_get_misa_ext_descr(uint32_t bit) > > nit: I'd prefer riscv_get_misa_ext_description() for the name. > >> +{ >> + return misa_ext_info_arr[ctz32(bit)].description; >> +} >> + >> +#define MISA_CFG(_bit, _enabled) \ >> + {.misa_bit = _bit, .enabled = _enabled} >> + >> +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = { >> + MISA_CFG(RVA, true), >> + MISA_CFG(RVC, true), >> + MISA_CFG(RVD, true), >> + MISA_CFG(RVF, true), >> + MISA_CFG(RVI, true), >> + MISA_CFG(RVE, false), >> + MISA_CFG(RVM, true), >> + MISA_CFG(RVS, true), >> + MISA_CFG(RVU, true), >> + MISA_CFG(RVH, true), >> + MISA_CFG(RVJ, false), >> + MISA_CFG(RVV, false), >> + MISA_CFG(RVG, false), >> }; >> >> static void riscv_cpu_add_misa_properties(Object *cpu_obj) >> @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj) >> int i; >> >> for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) { >> - const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; >> + RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; >> + >> + misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit); >> + misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit); > > This might be necessary for the KVM side, but we should be able to avoid > this runtime setting here where the compiler can be certain everything is > a constant. We just need the old MISA_CFG() back, slightly tweaked to use > __builtin_ctz(). > >> >> object_property_add(cpu_obj, misa_cfg->name, "bool", >> cpu_get_misa_ext_cfg, >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index cc20ee25a7..acae118b9b 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -41,7 +41,10 @@ >> >> #define RV(x) ((target_ulong)1 << (x - 'A')) >> >> -/* Consider updating misa_ext_cfgs[] when adding new MISA bits here */ >> +/* >> + * Consider updating misa_ext_info_arr[] and misa_ext_cfgs[] >> + * when adding new MISA bits here. >> + */ >> #define RVI RV('I') >> #define RVE RV('E') /* E and I are mutually exclusive */ >> #define RVM RV('M') >> @@ -56,6 +59,8 @@ >> #define RVJ RV('J') >> #define RVG RV('G') >> >> +const char *riscv_get_misa_ext_name(uint32_t bit); >> +const char *riscv_get_misa_ext_descr(uint32_t bit); >> >> /* Privileged specification version */ >> enum { >> -- >> 2.41.0 >> > > Thanks, > drew
On Thu, Jun 29, 2023 at 07:04:28PM -0300, Daniel Henrique Barboza wrote: > Drew, > > On 6/29/23 05:59, Andrew Jones wrote: > > On Wed, Jun 28, 2023 at 06:30:24PM -0300, Daniel Henrique Barboza wrote: > > > Next patch will add KVM specific user properties for both MISA and > > > multi-letter extensions. For MISA extensions we want to make use of what > > > is already available in misa_ext_cfgs[] to avoid code repetition. > > > > > > misa_ext_info_arr[] array will hold name and description for each MISA > > > extension that misa_ext_cfgs[] is declaring. We'll then use this new > > > array in KVM code to avoid duplicating strings. > > > > > > There's nothing holding us back from doing the same with multi-letter > > > extensions. For now doing just with MISA extensions is enough. > > > > > > Suggested-by: Andrew Jones <ajones@ventanamicro.com> > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > > --- > > > target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++---------------- > > > target/riscv/cpu.h | 7 +++- > > > 2 files changed, 61 insertions(+), 29 deletions(-) > > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > > index 2485e820f8..90dd2078ae 100644 > > > --- a/target/riscv/cpu.c > > > +++ b/target/riscv/cpu.c > > > @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name, > > > visit_type_bool(v, name, &value, errp); > > > } > > > -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = { > > > - {.name = "a", .description = "Atomic instructions", > > > - .misa_bit = RVA, .enabled = true}, > > > - {.name = "c", .description = "Compressed instructions", > > > - .misa_bit = RVC, .enabled = true}, > > > - {.name = "d", .description = "Double-precision float point", > > > - .misa_bit = RVD, .enabled = true}, > > > - {.name = "f", .description = "Single-precision float point", > > > - .misa_bit = RVF, .enabled = true}, > > > - {.name = "i", .description = "Base integer instruction set", > > > - .misa_bit = RVI, .enabled = true}, > > > - {.name = "e", .description = "Base integer instruction set (embedded)", > > > - .misa_bit = RVE, .enabled = false}, > > > - {.name = "m", .description = "Integer multiplication and division", > > > - .misa_bit = RVM, .enabled = true}, > > > - {.name = "s", .description = "Supervisor-level instructions", > > > - .misa_bit = RVS, .enabled = true}, > > > - {.name = "u", .description = "User-level instructions", > > > - .misa_bit = RVU, .enabled = true}, > > > - {.name = "h", .description = "Hypervisor", > > > - .misa_bit = RVH, .enabled = true}, > > > - {.name = "x-j", .description = "Dynamic translated languages", > > > - .misa_bit = RVJ, .enabled = false}, > > > - {.name = "v", .description = "Vector operations", > > > - .misa_bit = RVV, .enabled = false}, > > > - {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)", > > > - .misa_bit = RVG, .enabled = false}, > > > +typedef struct misa_ext_info { > > > + const char *name; > > > + const char *description; > > > +} MISAExtInfo; > > > + > > > +#define MISA_EXT_INFO(_idx, _propname, _descr) \ > > > + [(_idx - 'A')] = {.name = _propname, .description = _descr} > > > > We don't have to give up on passing RV* to this macro. Directly > > using __builtin_ctz() with a constant should work, i.e. > > > > #define MISA_EXT_INFO(_bit, _propname, _descr) \ > > [__builtin_ctz(_bit)] = {.name = _propname, .description = _descr} > > > > and then > > > > MISA_EXT_INFO(RVA, "a", "Atomic instructions"), > > MISA_EXT_INFO(RVD, "d", "Double-precision float point"), > > ... > > > > (We don't need the ctz32() wrapper since we know we'll never input zero to > > __builtin_ctz().) > > I run the series through gitlab because I got worried about this change in different > compilers and so on. And in fact I fear that we break 'clang-user' with it: > > https://gitlab.com/danielhb/qemu/-/jobs/4569265199 > > u.c.o -c ../target/riscv/cpu.c > ../target/riscv/cpu.c:1624:5: error: initializer element is not a compile-time constant > MISA_CFG(RVA, true), > ^~~~~~~~~~~~~~~~~~~ > ../target/riscv/cpu.c:1619:53: note: expanded from macro 'MISA_CFG' > {.name = misa_ext_info_arr[MISA_INFO_IDX(_bit)].name, \ > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~ > 1 error generated. > [1503/2619] Compiling C object libqemu-ppc64le-linux-user.fa.p/linux-user_syscall.c.o > > > Which is a shame because gcc and everyone else is okay with it, but 'clang-user' and > 'tsan-build' runners are complaining about it. > > Unless there's a directive to make clang accept this code (I didn't find any) we'll > need to keep updating name and description during runtime, and we'll have to keep > removing 'const' from misa_ext_cfgs[]. > Yeah, that's a pity, and odd that any compiler wouldn't be able to identify that a constant input to a builtin linear function produces another constant... Oh well, we can only be as good as the tools we use... Thanks, drew
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 2485e820f8..90dd2078ae 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1558,33 +1558,57 @@ static void cpu_get_misa_ext_cfg(Object *obj, Visitor *v, const char *name, visit_type_bool(v, name, &value, errp); } -static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = { - {.name = "a", .description = "Atomic instructions", - .misa_bit = RVA, .enabled = true}, - {.name = "c", .description = "Compressed instructions", - .misa_bit = RVC, .enabled = true}, - {.name = "d", .description = "Double-precision float point", - .misa_bit = RVD, .enabled = true}, - {.name = "f", .description = "Single-precision float point", - .misa_bit = RVF, .enabled = true}, - {.name = "i", .description = "Base integer instruction set", - .misa_bit = RVI, .enabled = true}, - {.name = "e", .description = "Base integer instruction set (embedded)", - .misa_bit = RVE, .enabled = false}, - {.name = "m", .description = "Integer multiplication and division", - .misa_bit = RVM, .enabled = true}, - {.name = "s", .description = "Supervisor-level instructions", - .misa_bit = RVS, .enabled = true}, - {.name = "u", .description = "User-level instructions", - .misa_bit = RVU, .enabled = true}, - {.name = "h", .description = "Hypervisor", - .misa_bit = RVH, .enabled = true}, - {.name = "x-j", .description = "Dynamic translated languages", - .misa_bit = RVJ, .enabled = false}, - {.name = "v", .description = "Vector operations", - .misa_bit = RVV, .enabled = false}, - {.name = "g", .description = "General purpose (IMAFD_Zicsr_Zifencei)", - .misa_bit = RVG, .enabled = false}, +typedef struct misa_ext_info { + const char *name; + const char *description; +} MISAExtInfo; + +#define MISA_EXT_INFO(_idx, _propname, _descr) \ + [(_idx - 'A')] = {.name = _propname, .description = _descr} + +static const MISAExtInfo misa_ext_info_arr[] = { + MISA_EXT_INFO('A', "a", "Atomic instructions"), + MISA_EXT_INFO('C', "c", "Compressed instructions"), + MISA_EXT_INFO('D', "d", "Double-precision float point"), + MISA_EXT_INFO('F', "f", "Single-precision float point"), + MISA_EXT_INFO('I', "i", "Base integer instruction set"), + MISA_EXT_INFO('E', "e", "Base integer instruction set (embedded)"), + MISA_EXT_INFO('M', "m", "Integer multiplication and division"), + MISA_EXT_INFO('S', "s", "Supervisor-level instructions"), + MISA_EXT_INFO('U', "u", "User-level instructions"), + MISA_EXT_INFO('H', "h", "Hypervisor"), + MISA_EXT_INFO('J', "x-j", "Dynamic translated languages"), + MISA_EXT_INFO('V', "v", "Vector operations"), + MISA_EXT_INFO('G', "g", "General purpose (IMAFD_Zicsr_Zifencei)"), +}; + +const char *riscv_get_misa_ext_name(uint32_t bit) +{ + return misa_ext_info_arr[ctz32(bit)].name; +} + +const char *riscv_get_misa_ext_descr(uint32_t bit) +{ + return misa_ext_info_arr[ctz32(bit)].description; +} + +#define MISA_CFG(_bit, _enabled) \ + {.misa_bit = _bit, .enabled = _enabled} + +static RISCVCPUMisaExtConfig misa_ext_cfgs[] = { + MISA_CFG(RVA, true), + MISA_CFG(RVC, true), + MISA_CFG(RVD, true), + MISA_CFG(RVF, true), + MISA_CFG(RVI, true), + MISA_CFG(RVE, false), + MISA_CFG(RVM, true), + MISA_CFG(RVS, true), + MISA_CFG(RVU, true), + MISA_CFG(RVH, true), + MISA_CFG(RVJ, false), + MISA_CFG(RVV, false), + MISA_CFG(RVG, false), }; static void riscv_cpu_add_misa_properties(Object *cpu_obj) @@ -1592,7 +1616,10 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj) int i; for (i = 0; i < ARRAY_SIZE(misa_ext_cfgs); i++) { - const RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; + RISCVCPUMisaExtConfig *misa_cfg = &misa_ext_cfgs[i]; + + misa_cfg->name = riscv_get_misa_ext_name(misa_cfg->misa_bit); + misa_cfg->description = riscv_get_misa_ext_descr(misa_cfg->misa_bit); object_property_add(cpu_obj, misa_cfg->name, "bool", cpu_get_misa_ext_cfg, diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index cc20ee25a7..acae118b9b 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -41,7 +41,10 @@ #define RV(x) ((target_ulong)1 << (x - 'A')) -/* Consider updating misa_ext_cfgs[] when adding new MISA bits here */ +/* + * Consider updating misa_ext_info_arr[] and misa_ext_cfgs[] + * when adding new MISA bits here. + */ #define RVI RV('I') #define RVE RV('E') /* E and I are mutually exclusive */ #define RVM RV('M') @@ -56,6 +59,8 @@ #define RVJ RV('J') #define RVG RV('G') +const char *riscv_get_misa_ext_name(uint32_t bit); +const char *riscv_get_misa_ext_descr(uint32_t bit); /* Privileged specification version */ enum {
Next patch will add KVM specific user properties for both MISA and multi-letter extensions. For MISA extensions we want to make use of what is already available in misa_ext_cfgs[] to avoid code repetition. misa_ext_info_arr[] array will hold name and description for each MISA extension that misa_ext_cfgs[] is declaring. We'll then use this new array in KVM code to avoid duplicating strings. There's nothing holding us back from doing the same with multi-letter extensions. For now doing just with MISA extensions is enough. Suggested-by: Andrew Jones <ajones@ventanamicro.com> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.c | 83 ++++++++++++++++++++++++++++++---------------- target/riscv/cpu.h | 7 +++- 2 files changed, 61 insertions(+), 29 deletions(-)