Re: [PATCH 11/20] target/riscv: introduce KVM AccelCPUClass

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 8/28/23 13:38, Philippe Mathieu-Daudé wrote:
> On 25/8/23 15:08, Daniel Henrique Barboza wrote:
>> Add a KVM accelerator class like we did with TCG. The difference is
>> that, at least for now, we won't be using a realize() implementation for
>> this accelerator.
>>
>> We'll start by assiging kvm_riscv_cpu_add_kvm_properties(), renamed to
>> kvm_cpu_instance_init(), as a 'cpu_instance_init' implementation. Change
>> riscv_cpu_post_init() to invoke accel_cpu_instance_init(), which will go
>> through the 'cpu_instance_init' impl of the current acceleration (if
>> available) and execute it. The end result is that the KVM initial setup,
>> i.e. starting registers and adding its specific properties, will be done
>> via this hook.
>>
>> riscv_cpu_add_user_properties() is still being called via the common
>> post_init() function, thus we still need the "if kvm then return" logic
>> inside it for now. We'll deal with it when TCG accel class get its own
>> 'cpu_instance_init' implementation.
>>
>> riscv_add_satp_mode_properties() is now being exported from cpu.c since
>> it's a common helper between KVM and TCG.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c       |  8 ++---
>>   target/riscv/cpu.h       |  1 +
>>   target/riscv/kvm.c       | 64 +++++++++++++++++++++++++++-------------
>>   target/riscv/kvm_riscv.h |  1 -
>>   4 files changed, 49 insertions(+), 25 deletions(-)
> 
> 
>> -static void riscv_add_satp_mode_properties(Object *obj)
>> +void riscv_add_satp_mode_properties(Object *obj)
>>   {
>>       RISCVCPU *cpu = RISCV_CPU(obj);
>> @@ -1199,6 +1199,8 @@ static void riscv_cpu_post_init(Object *obj)
>>       RISCVCPU *cpu = RISCV_CPU(obj);
>>       RISCVCPUClass *rcc = RISCV_CPU_GET_CLASS(cpu);
>> +    accel_cpu_instance_init(CPU(obj));
>> +
>>       if (rcc->user_extension_properties) {
>>           riscv_cpu_add_user_properties(obj);
>>       }
>> @@ -1561,12 +1563,10 @@ static void riscv_cpu_add_multiext_prop_array(Object *obj,
>>   static void riscv_cpu_add_user_properties(Object *obj)
>>   {
>>   #ifndef CONFIG_USER_ONLY
>> -    riscv_add_satp_mode_properties(obj);
>> -
>>       if (kvm_enabled()) {
>> -        kvm_riscv_cpu_add_kvm_properties(obj);
>>           return;
>>       }
> 
> Maybe in a preliminary patch:
> 
> if (tcg_enabled()) {
> 
>> +    riscv_add_satp_mode_properties(obj);
> 
> }
> 
> then remove the 'if kvm_enabled' in this patch?

I'll add a preliminary patch to make kvm_riscv_cpu_add_kvm_properties() call
riscv_add_satp_mode_properties() and change the order they're being called here
(i.e. call kvm_riscv_cpu_add_kvm_properties() first). Then this patch can safely
remove the 'if kvm_enabled' block as you suggested.


Thanks,

Daniel


> 
>>   #endif
>