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 <ajo...@ventanamicro.com>
> > > Signed-off-by: Daniel Henrique Barboza <dbarb...@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
