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

From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
To: Andrew Jones <ajones@ventanamicro.com>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
	alistair.francis@wdc.com, bmeng@tinylab.org,
	liweiwei@iscas.ac.cn, zhiwei_liu@linux.alibaba.com,
	palmer@rivosinc.com
Subject: Re: [PATCH 14/16] target/riscv: adapt 'riscv_isa_string' for KVM
Date: Tue, 13 Jun 2023 15:19:20 -0300	[thread overview]
Message-ID: <784fd208-265e-e951-7355-7398e2eec270@ventanamicro.com> (raw)
In-Reply-To: <7a4217e2-163b-8e2d-e86b-97fb0733fef3@ventanamicro.com>

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