Re: [PATCH v6 11/20] target/riscv/cpu: add misa_ext_info_arr[]

[Andrew Jones]

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