Re: [PATCH 2/4] target/ppc: added solutions for building with disable-tcg

[David Gibson]

On Fri, Apr 09, 2021 at 12:19:14PM -0300, Bruno Larsen (billionai) wrote:
> this commit presents 2 possible solutions for substituting TCG emulation
> with KVM calls. One - used in machine.c and arch_dump.c - explicitly
> adds the KVM function and has the possibility of adding the TCG one
> for more generic compilation, prioritizing te KVM option. The second
> option, implemented in kvm_ppc.h, transparently changes the helper
> into the KVM call, if TCG is not enabled. I believe the first solution
> is better, but it is less readable, so I wanted to have some feedback
> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/arch_dump.c | 17 +++++++++++++++++
>  target/ppc/kvm.c       | 30 ++++++++++++++++++++++++++++++
>  target/ppc/kvm_ppc.h   | 11 +++++++++++
>  target/ppc/machine.c   | 33 ++++++++++++++++++++++++++++++++-
>  4 files changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> index 9ab04b2c38..c53e01011a 100644
> --- a/target/ppc/arch_dump.c
> +++ b/target/ppc/arch_dump.c
> @@ -17,7 +17,10 @@
>  #include "elf.h"
>  #include "sysemu/dump.h"
>  #include "sysemu/kvm.h"
> +#include "kvm_ppc.h"
> +#if defined(CONFIG_TCG)
>  #include "exec/helper-proto.h"
> +#endif /* CONFIG_TCG */
>  
>  #ifdef TARGET_PPC64
>  #define ELFCLASS ELFCLASS64
> @@ -176,7 +179,21 @@ static void ppc_write_elf_vmxregset(NoteFuncArg *arg, 
> PowerPCCPU *cpu)
>              vmxregset->avr[i].u64[1] = avr->u64[1];
>          }
>      }
> +    /* This is the first solution implemented. My personal favorite as it
> +     * allows for explicit error handling, however it is much less readable 
> */
> +#if defined(CONFIG_KVM)
> +    if(kvm_enabled()){
> +        vmxregset->vscr.u32[3] = cpu_to_dump32(s, kvmppc_mfvscr(cpu));
> +    }else
> +#endif
> +
> +#if defined(CONFIG_TCG)
>      vmxregset->vscr.u32[3] = cpu_to_dump32(s, helper_mfvscr(&cpu->env));
> +#else
> +    {
> +        /* TODO: add proper error handling, even tough this should never be 
> reached */
> +    }
> +#endif

I think this is more complex than you need.  AFAICT, the logic in
helper_mfcsvr() is still valid even without TCG (we still have a copy
of the state in 'env' with KVM).

You could move helper_mfvscr() to a file that isn't going to get
excluded for !TCG builds.

Not directly related to what you're trying to accomplish here, but the
whole vscr_sat thing looks really weird.  I have no idea why we're
splitting out the storage of VSCR[SAT] for the TCG case at all.  If
you wanted to clean that up as a preliminary, I'd be grateful.

>  }
>  
>  static void ppc_write_elf_vsxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb..8ed54d12d8 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -51,6 +51,7 @@
>  #include "elf.h"
>  #include "sysemu/kvm_int.h"
>  
> +
>  #define PROC_DEVTREE_CPU      "/proc/device-tree/cpus/"
>  
>  #define DEBUG_RETURN_GUEST 0
> @@ -2947,3 +2948,32 @@ bool kvm_arch_cpu_check_are_resettable(void)
>  {
>      return true;
>  }
> +
> +/* Functions added to replace helper_m(t|f)vscr from int_helper.c */
> +int kvmppc_mtvscr(PowerPCCPU *cpu, uint32_t val){
> +    CPUState *cs = CPU(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    int ret;
> +    reg.id = KVM_REG_PPC_VSCR;
> +    reg.addr = (uintptr_t) &env->vscr;
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if(ret < 0){
> +        fprintf(stderr, "Unable to set VSCR to KVM: %s", strerror(errno));
> +    }
> +    return ret;
> +}
> +
> +int kvmppc_mfvscr(PowerPCCPU *cpu){
> +    CPUState *cs = CPU(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    int ret;
> +    reg.id = KVM_REG_PPC_VSCR;
> +    reg.addr = (uintptr_t) &env->vscr;
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if(ret < 0){
> +        fprintf(stderr, "Unable to get VSCR to KVM: %s", strerror(errno));
> +    }
> +    return ret;
> +}
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 989f61ace0..f618cb28b1 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -86,6 +86,17 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t 
> tb_offset);
>  
>  int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run);
>  
> +int kvmppc_mtvscr(PowerPCCPU*, uint32_t);
> +int kvmppc_mfvscr(PowerPCCPU*);
> +
> +/* This is the second (quick and dirty) solution. Not my personal favorite
> + * as it hides what is actually happening from the user and doesn't allow
> + * for error checking. but it requires less change in other files */
> +#ifndef CONFIG_TCG
> +#define helper_mtvscr(env, val) kvmppc_mtvscr(env_archcpu(env),val)
> +#define helper_mfvscr(env) kvmppc_mfvscr(env_archcpu(env))
> +#endif
> +
>  #else
>  
>  static inline uint32_t kvmppc_get_tbfreq(void)
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 283db1d28a..d92bc18859 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -8,7 +8,9 @@
>  #include "qapi/error.h"
>  #include "qemu/main-loop.h"
>  #include "kvm_ppc.h"
> +#ifdef CONFIG_TCG
>  #include "exec/helper-proto.h"
> +#endif /*CONFIG_TCG*/
>  
>  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>  {
> @@ -95,7 +97,18 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int 
> version_id)
>          ppc_store_sdr1(env, sdr1);
>      }
>      qemu_get_be32s(f, &vscr);
> -    helper_mtvscr(env, vscr);

Likewise here, the existing helper_mtvscr() is fine (if perhaps more
complex than necessary), so you don't need separate paths here.

> +#if defined(CONFIG_KVM)
> +    if(kvm_enabled()){
> +        kvmppc_mtvscr(cpu, vscr);
> +    }else
> +#endif
> +#if defined(CONFIG_TCG)
> +        helper_mtvscr(env, vscr);
> +#else
> +    {
> +        /* TODO: Add correct error handling, even though this should never 
> be reached */
> +    }
> +#endif
>      qemu_get_be64s(f, &env->spe_acc);
>      qemu_get_be32s(f, &env->spe_fscr);
>      qemu_get_betls(f, &env->msr_mask);
> @@ -450,7 +463,16 @@ static int get_vscr(QEMUFile *f, void *opaque, size_t 
> size,
>                      const VMStateField *field)
>  {
>      PowerPCCPU *cpu = opaque;
> +#if defined(CONFIG_KVM)
> +    if(kvm_enabled()){
> +        kvmppc_mtvscr(cpu, qemu_get_be32(f));
> +        return 0;
> +    }
> +#endif /*CONFIG_KVM*/
> +
> +#if defined(CONFIG_TCG)
>      helper_mtvscr(&cpu->env, qemu_get_be32(f));
> +#endif /*CONFIG_TCG*/
>      return 0;
>  }
>  
> @@ -458,7 +480,16 @@ static int put_vscr(QEMUFile *f, void *opaque, size_t 
> size,
>                      const VMStateField *field, JSONWriter *vmdesc)
>  {
>      PowerPCCPU *cpu = opaque;
> +#if defined(CONFIG_KVM)
> +    if(kvm_enabled()){
> +        qemu_put_be32(f, kvmppc_mfvscr(cpu));
> +        return 0;
> +    }
> +#endif /*CONFIG_KVM*/
> +
> +#if defined(CONFIG_TCG)
>      qemu_put_be32(f, helper_mfvscr(&cpu->env));
> +#endif /*CONFIG_TCG*/
>      return 0;
>  }
>  

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature