Re: [PATCH 14/16] target/riscv: adapt 'riscv_isa_string' for KVM

[Daniel Henrique Barboza]

On 6/13/23 07:29, Daniel Henrique Barboza wrote:
> On 6/7/23 09:21, Andrew Jones wrote:
> > On Tue, May 30, 2023 at 04:46:21PM -0300, Daniel Henrique Barboza wrote:
> > > KVM is not using the same attributes as TCG, i.e. it doesn't use
> > > isa_edata_arr[]. Add a new kvm_riscv_isa_string_ext() helper that does
> > > basically the same thing, but using KVM internals instead.
> > >
> > > The decision to add this helper target/riscv/kvm.c is to foster the
> > > separation between KVM and TCG logic, while still using
> > > riscv_isa_string_ext() from target/riscv/cpu.c to retrieve the string
> > > to not overcomplicate things.
> > >
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > ---
> > >   target/riscv/cpu.c       |  5 +++++
> > >   target/riscv/kvm.c       | 19 +++++++++++++++++++
> > >   target/riscv/kvm_riscv.h |  2 ++
> > >   3 files changed, 26 insertions(+)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 3c348049a3..ec1d0c621a 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1956,6 +1956,11 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char 
> > > **isa_str,
> > >       char *new = *isa_str;
> > >       int i;
> > > +    if (riscv_running_KVM()) {
> > > +        kvm_riscv_isa_string_ext(cpu, isa_str, max_str_len);
> > > +        return;
> > > +    }
> > > +
> > >       for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> > >           if (cpu->env.priv_ver >= isa_edata_arr[i].min_version &&
> > >               isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
> > > diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> > > index b4193a10d8..675e18df3b 100644
> > > --- a/target/riscv/kvm.c
> > > +++ b/target/riscv/kvm.c
> > > @@ -320,6 +320,25 @@ static void kvm_riscv_add_cpu_user_properties(Object 
> > > *cpu_obj)
> > >       }
> > >   }
> > > +void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
> > > +                              int max_str_len)
> > > +{
> > > +    char *old = *isa_str;
> > > +    char *new = *isa_str;
> > > +    int i;
> > > +
> > > +    for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
> > > +        RISCVCPUMultiExtConfig *multi_ext_cfg = &kvm_multi_ext_cfgs[i];
> > > +        if (kvm_cpu_cfg_get(cpu, multi_ext_cfg)) {
> > > +            new = g_strconcat(old, "_", multi_ext_cfg->name, NULL);
> > > +            g_free(old);
> > > +            old = new;
> > > +        }
> > > +    }
> > > +
> > > +    *isa_str = new;
> > > +}
> > > +
> > >   static int kvm_riscv_get_regs_core(CPUState *cs)
> > >   {
> > >       int ret = 0;
> > > diff --git a/target/riscv/kvm_riscv.h b/target/riscv/kvm_riscv.h
> > > index e3ba935808..1a12efa8db 100644
> > > --- a/target/riscv/kvm_riscv.h
> > > +++ b/target/riscv/kvm_riscv.h
> > > @@ -20,6 +20,8 @@
> > >   #define QEMU_KVM_RISCV_H
> > >   void kvm_riscv_init_user_properties(Object *cpu_obj);
> > > +void kvm_riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
> > > +                              int max_str_len);
> > >   void kvm_riscv_reset_vcpu(RISCVCPU *cpu);
> > >   void kvm_riscv_set_irq(RISCVCPU *cpu, int irq, int level);
> > > --
> > > 2.40.1
> >
> > Hmm, more duplication. I think we need an abstraction which can support
> > both TCG and KVM extension lists. Allowing functions like
> > riscv_isa_string_ext() to be shared for them.
>
> I tried to play around a bit and didn't manage to find a solution that covers
> both.
>
> The root cause is that the TCG only options are being ignored by KVM, but they
> are still around. I made an attempt of re-using the existing isa_string()
> function with KVM by changing all TCG-only extensions default to 'false'. This
> doesn't change the fact that, with KVM, I can do:
>
> sudo ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
> -cpu host,zhinx=true,zhinxmin=true (...)
>
> Note that zhinx and zhinxmin are TCG-only. And the ISA string showed these 2
> extensions:
>
> # cat /proc/device-tree/cpus/cpu@0/riscv,isa
> rv64imafdc_zicbom_zicboz_zbb_zhinx_zhinxmin_sstc
>
>
> Alternatives would be to change TCG code to allow for extra fields for KVM 
> (e.g. the
> 'supported' flag) to allow the isa_string() function to ignore the TCG-only 
> extensions.
> Bear in mind that TCG has 63 extensions, so we would do 63 ioctls for each CPU 
> in this
> extension discovery and KVM only 8 support extensions ATM.

... or we can add an extra flag in isa_edata_arr[] 'kvm_available' to indicate 
if a
given extension is also present in KVM, set it manually for each KVM-capable
entry of the array and check for that during riscv_isa_string_ext().

This will avoid the code duplication while not allowing TCG-only extensions
to appear in the riscv,isa when running KVM.

I made this change in v2. I'll send it up shortly. Thanks,


Daniel

> Another idea is to make the existing isa_string() compare isa_edata_arr[] with 
> the
> KVM counterpart kvm_multi_ext_cfgs[] and, if running KVM, check if the extension
> in isa_edata_arr[] is also in the KVM array. This also seems a bit inefficient 
> since
> we're adding a search loop for 55 extensions when creating the string.
>
> Another alternative is to exclude all TCG-only extensions from the command line 
> when
> running KVM. We would fork the API though, which is something that we're 
> wanting to
> avoid.
>
> Duplicating this code as we're doing here guarantees that the KVM isa string 
> won't
> have anything that KVM doesn't know about, regardless of the user input. I am 
> not a
> fan of duplication, but at this moment it seems plausible to keep it. At least 
> until
> we sort a way of unifying both TCG and KVM options in a satisfying manner.
>
> I mean, at least as far as a I can see. Suggestions always welcome.
>
>
> Thanks,
>
>
> Daniel
>
>
>
>
> > Thanks,
> > drew