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

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(+)

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.

Thanks,
drew

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.

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

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