[v2] target/loongarch: Add TCG macro in structure CPUArchState

In structure CPUArchState some struct elements are only used in TCG
mode, and it is not used in KVM mode. Macro CONFIG_TCG is added to
make it simpiler in KVM mode, also there is the same modification
in c code when these struct elements are used.

When VM runs in KVM mode, TLB entries are not used and do not need
migrate. It is only useful when it runs in TCG mode.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Change-Id: Id30d663f5d7bc3436520638f606f99d93926eb2e
---
v1 --> v2:
- Add field needed in structure vmstate_tlb, dynamically judge whether
tlb should be migrated, since mostly qemu-system-loongarch64 is compiled
with both kvm and tcg accl enabled.
---
 target/loongarch/cpu.c        | 14 +++++++++++---
 target/loongarch/cpu.h        | 16 ++++++++++------
 target/loongarch/cpu_helper.c |  9 +++++++++
 target/loongarch/machine.c    | 34 +++++++++++++++++++++++++++++-----
 4 files changed, 59 insertions(+), 14 deletions(-)


base-commit: e1007b6bab5cf97705bf4f2aaec1f607787355b8

在 2024/3/4 10:18, Bibo Mao 写道:
> In structure CPUArchState some struct elements are only used in TCG
> mode, and it is not used in KVM mode. Macro CONFIG_TCG is added to
> make it simpiler in KVM mode, also there is the same modification
> in c code when these struct elements are used.
>
> When VM runs in KVM mode, TLB entries are not used and do not need
> migrate. It is only useful when it runs in TCG mode.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> Change-Id: Id30d663f5d7bc3436520638f606f99d93926eb2e
> ---
> v1 --> v2:
> - Add field needed in structure vmstate_tlb, dynamically judge whether
> tlb should be migrated, since mostly qemu-system-loongarch64 is compiled
> with both kvm and tcg accl enabled.
> ---
>   target/loongarch/cpu.c        | 14 +++++++++++---
>   target/loongarch/cpu.h        | 16 ++++++++++------
>   target/loongarch/cpu_helper.c |  9 +++++++++
>   target/loongarch/machine.c    | 34 +++++++++++++++++++++++++++++-----
>   4 files changed, 59 insertions(+), 14 deletions(-)
Reviewed-by: Song Gao <gaosong@loongson.cn>

Thanks.
Song Gao
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index bc2684179f..35db8e244d 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -517,7 +517,9 @@ static void loongarch_cpu_reset_hold(Object *obj)
>           lacc->parent_phases.hold(obj);
>       }
>   
> +#ifdef CONFIG_TCG
>       env->fcsr0_mask = FCSR0_M1 | FCSR0_M2 | FCSR0_M3;
> +#endif
>       env->fcsr0 = 0x0;
>   
>       int n;
> @@ -562,7 +564,9 @@ static void loongarch_cpu_reset_hold(Object *obj)
>   
>   #ifndef CONFIG_USER_ONLY
>       env->pc = 0x1c000000;
> +#ifdef CONFIG_TCG
>       memset(env->tlb, 0, sizeof(env->tlb));
> +#endif
>       if (kvm_enabled()) {
>           kvm_arch_reset_vcpu(env);
>       }
> @@ -696,11 +700,15 @@ void loongarch_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>   {
>       LoongArchCPU *cpu = LOONGARCH_CPU(cs);
>       CPULoongArchState *env = &cpu->env;
> -    int i;
> +    int i, fp_status;
>   
> +#ifdef CONFIG_TCG
> +    fp_status = get_float_exception_flags(&env->fp_status);
> +#else
> +    fp_status = 0;
> +#endif
>       qemu_fprintf(f, " PC=%016" PRIx64 " ", env->pc);
> -    qemu_fprintf(f, " FCSR0 0x%08x  fp_status 0x%02x\n", env->fcsr0,
> -                 get_float_exception_flags(&env->fp_status));
> +    qemu_fprintf(f, " FCSR0 0x%08x  fp_status 0x%02x\n", env->fcsr0, fp_status);
>   
>       /* gpr */
>       for (i = 0; i < 32; i++) {
> diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
> index ec37579fd6..c25ad112b1 100644
> --- a/target/loongarch/cpu.h
> +++ b/target/loongarch/cpu.h
> @@ -272,6 +272,7 @@ union fpr_t {
>       VReg  vreg;
>   };
>   
> +#ifdef CONFIG_TCG
>   struct LoongArchTLB {
>       uint64_t tlb_misc;
>       /* Fields corresponding to CSR_TLBELO0/1 */
> @@ -279,23 +280,18 @@ struct LoongArchTLB {
>       uint64_t tlb_entry1;
>   };
>   typedef struct LoongArchTLB LoongArchTLB;
> +#endif
>   
>   typedef struct CPUArchState {
>       uint64_t gpr[32];
>       uint64_t pc;
>   
>       fpr_t fpr[32];
> -    float_status fp_status;
>       bool cf[8];
> -
>       uint32_t fcsr0;
> -    uint32_t fcsr0_mask;
>   
>       uint32_t cpucfg[21];
>   
> -    uint64_t lladdr; /* LL virtual address compared against SC */
> -    uint64_t llval;
> -
>       /* LoongArch CSRs */
>       uint64_t CSR_CRMD;
>       uint64_t CSR_PRMD;
> @@ -352,8 +348,16 @@ typedef struct CPUArchState {
>       uint64_t CSR_DERA;
>       uint64_t CSR_DSAVE;
>   
> +#ifdef CONFIG_TCG
> +    float_status fp_status;
> +    uint32_t fcsr0_mask;
> +    uint64_t lladdr; /* LL virtual address compared against SC */
> +    uint64_t llval;
> +#endif
>   #ifndef CONFIG_USER_ONLY
> +#ifdef CONFIG_TCG
>       LoongArchTLB  tlb[LOONGARCH_TLB_MAX];
> +#endif
>   
>       AddressSpace *address_space_iocsr;
>       bool load_elf;
> diff --git a/target/loongarch/cpu_helper.c b/target/loongarch/cpu_helper.c
> index 45f821d086..d1cdbe30ba 100644
> --- a/target/loongarch/cpu_helper.c
> +++ b/target/loongarch/cpu_helper.c
> @@ -11,6 +11,7 @@
>   #include "internals.h"
>   #include "cpu-csr.h"
>   
> +#ifdef CONFIG_TCG
>   static int loongarch_map_tlb_entry(CPULoongArchState *env, hwaddr *physical,
>                                      int *prot, target_ulong address,
>                                      int access_type, int index, int mmu_idx)
> @@ -154,6 +155,14 @@ static int loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
>   
>       return TLBRET_NOMATCH;
>   }
> +#else
> +static int loongarch_map_address(CPULoongArchState *env, hwaddr *physical,
> +                                 int *prot, target_ulong address,
> +                                 MMUAccessType access_type, int mmu_idx)
> +{
> +    return TLBRET_NOMATCH;
> +}
> +#endif
>   
>   static hwaddr dmw_va2pa(CPULoongArchState *env, target_ulong va,
>                           target_ulong dmw)
> diff --git a/target/loongarch/machine.c b/target/loongarch/machine.c
> index c7029fb9b4..77890f07cc 100644
> --- a/target/loongarch/machine.c
> +++ b/target/loongarch/machine.c
> @@ -8,6 +8,7 @@
>   #include "qemu/osdep.h"
>   #include "cpu.h"
>   #include "migration/cpu.h"
> +#include "sysemu/kvm.h"
>   #include "vec.h"
>   
>   static const VMStateDescription vmstate_fpu_reg = {
> @@ -109,9 +110,19 @@ static const VMStateDescription vmstate_lasx = {
>       },
>   };
>   
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
> +static bool tlb_needed(void *opaque)
> +{
> +    if (kvm_enabled()) {
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>   /* TLB state */
> -const VMStateDescription vmstate_tlb = {
> -    .name = "cpu/tlb",
> +static const VMStateDescription vmstate_tlb_entry = {
> +    .name = "cpu/tlb_entry",
>       .version_id = 0,
>       .minimum_version_id = 0,
>       .fields = (const VMStateField[]) {
> @@ -122,6 +133,19 @@ const VMStateDescription vmstate_tlb = {
>       }
>   };
>   
> +static const VMStateDescription vmstate_tlb = {
> +    .name = "cpu/tlb",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .needed = tlb_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_STRUCT_ARRAY(env.tlb, LoongArchCPU, LOONGARCH_TLB_MAX,
> +                             0, vmstate_tlb_entry, LoongArchTLB),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +#endif
> +
>   /* LoongArch CPU state */
>   const VMStateDescription vmstate_loongarch_cpu = {
>       .name = "cpu",
> @@ -187,9 +211,6 @@ const VMStateDescription vmstate_loongarch_cpu = {
>           VMSTATE_UINT64(env.CSR_DBG, LoongArchCPU),
>           VMSTATE_UINT64(env.CSR_DERA, LoongArchCPU),
>           VMSTATE_UINT64(env.CSR_DSAVE, LoongArchCPU),
> -        /* TLB */
> -        VMSTATE_STRUCT_ARRAY(env.tlb, LoongArchCPU, LOONGARCH_TLB_MAX,
> -                             0, vmstate_tlb, LoongArchTLB),
>   
>           VMSTATE_END_OF_LIST()
>       },
> @@ -197,6 +218,9 @@ const VMStateDescription vmstate_loongarch_cpu = {
>           &vmstate_fpu,
>           &vmstate_lsx,
>           &vmstate_lasx,
> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
> +        &vmstate_tlb,
> +#endif
>           NULL
>       }
>   };
>
> base-commit: e1007b6bab5cf97705bf4f2aaec1f607787355b8

On 3/3/24 16:18, Bibo Mao wrote:
> @@ -696,11 +700,15 @@ void loongarch_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>   {
>       LoongArchCPU *cpu = LOONGARCH_CPU(cs);
>       CPULoongArchState *env = &cpu->env;
> -    int i;
> +    int i, fp_status;
>   
> +#ifdef CONFIG_TCG
> +    fp_status = get_float_exception_flags(&env->fp_status);
> +#else
> +    fp_status = 0;
> +#endif
>       qemu_fprintf(f, " PC=%016" PRIx64 " ", env->pc);
> -    qemu_fprintf(f, " FCSR0 0x%08x  fp_status 0x%02x\n", env->fcsr0,
> -                 get_float_exception_flags(&env->fp_status));
> +    qemu_fprintf(f, " FCSR0 0x%08x  fp_status 0x%02x\n", env->fcsr0, fp_status);

fp_status, I think, is unnecessary to print all of the time.

In update_fcsr0_mask, we ensure that fcsr0 is updated and fp_status.exception_flags is 0. 
So I would expect this field to be 0 all of the time -- anything else is a bug.

> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
> +static bool tlb_needed(void *opaque)
> +{
> +    if (kvm_enabled()) {
> +        return false;
> +    }
> +
> +    return true;
> +}

Better as return tcg_enabled();


r~

On 2024/3/5 上午12:53, Richard Henderson wrote:
> On 3/3/24 16:18, Bibo Mao wrote:
>> @@ -696,11 +700,15 @@ void loongarch_cpu_dump_state(CPUState *cs, FILE 
>> *f, int flags)
>>   {
>>       LoongArchCPU *cpu = LOONGARCH_CPU(cs);
>>       CPULoongArchState *env = &cpu->env;
>> -    int i;
>> +    int i, fp_status;
>> +#ifdef CONFIG_TCG
>> +    fp_status = get_float_exception_flags(&env->fp_status);
>> +#else
>> +    fp_status = 0;
>> +#endif
>>       qemu_fprintf(f, " PC=%016" PRIx64 " ", env->pc);
>> -    qemu_fprintf(f, " FCSR0 0x%08x  fp_status 0x%02x\n", env->fcsr0,
>> -                 get_float_exception_flags(&env->fp_status));
>> +    qemu_fprintf(f, " FCSR0 0x%08x  fp_status 0x%02x\n", env->fcsr0, 
>> fp_status);
> 
> fp_status, I think, is unnecessary to print all of the time.
> 
> In update_fcsr0_mask, we ensure that fcsr0 is updated and 
> fp_status.exception_flags is 0. So I would expect this field to be 0 all 
> of the time -- anything else is a bug.
> 
yes, fp_status is temporary status during float instruction translation, 
it will clear to 0 when fp instruction translation is done.

Will remove print sentence.

>> +#if defined(CONFIG_TCG) && !defined(CONFIG_USER_ONLY)
>> +static bool tlb_needed(void *opaque)
>> +{
>> +    if (kvm_enabled()) {
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
> 
> Better as return tcg_enabled();
Will return as tcg_enabled(), it is more simpler.

Regards
Bibo Mao
> 
> 
> r~