Re: [Qemu-ppc] [PATCH 1/4] target-ppc: Add hooks for handling tcg and kvm limitations

[Andreas Färber]

Am 04.04.2012 07:02, schrieb David Gibson:
> On target-ppc, our table of CPU types and features encodes the features as
> found on the hardware, regardless of whether these features are actually
> usable under TCG or KVM.  We already have cases where the information from
> the cpu table must be fixed up to account for limitations in the emulation
> method we're using.  e.g. TCG does not support the DFP and VSX instructions
> and KVM needs different numbering of the CPUs in order to tell it the
> correct thread to core mappings.
> 
> This patch cleans up these hacks to handle emulation limitations by
> consolidating them into a pair of functions specifically for the purpose.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---

I like the general idea. As usual I'm not entirely happy with some
details mostly due to the ongoing QOM'ification with which this collided...

>  target-ppc/helper.c         |    9 -------
>  target-ppc/kvm.c            |   14 +++++++++++
>  target-ppc/kvm_ppc.h        |    5 ++++
>  target-ppc/translate_init.c |   51 +++++++++++++++++++++++++++++-------------
>  4 files changed, 54 insertions(+), 25 deletions(-)
> 
> diff --git a/target-ppc/helper.c b/target-ppc/helper.c
> index 39dcc27..ef8fe28 100644
> --- a/target-ppc/helper.c
> +++ b/target-ppc/helper.c
> @@ -3198,15 +3198,6 @@ CPUPPCState *cpu_ppc_init (const char *cpu_model)
>      if (tcg_enabled()) {
>          ppc_translate_init();
>      }
> -    /* Adjust cpu index for SMT */
> -#if !defined(CONFIG_USER_ONLY)
> -    if (kvm_enabled()) {
> -        int smt = kvmppc_smt_threads();
> -
> -        env->cpu_index = (env->cpu_index / smp_threads)*smt
> -            + (env->cpu_index % smp_threads);
> -    }
> -#endif /* !CONFIG_USER_ONLY */
>      env->cpu_model_str = cpu_model;
>      cpu_ppc_register_internal(env, def);
>  
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 724f4c7..8b49761 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -27,6 +27,7 @@
>  #include "kvm.h"
>  #include "kvm_ppc.h"
>  #include "cpu.h"
> +#include "cpus.h"
>  #include "device_tree.h"
>  #include "hw/sysbus.h"
>  #include "hw/spapr.h"
> @@ -938,6 +939,19 @@ const ppc_def_t *kvmppc_host_cpu_def(void)
>      return spec;
>  }
>  
> +int kvm_fixup_ppc_env(CPUPPCState *env, const ppc_def_t *def)
> +{
> +    int smt;
> +
> +    /* Adjust cpu index for SMT */
> +    smt = kvmppc_smt_threads();
> +    env->cpu_index = (env->cpu_index / smp_threads)*smt

The code moved here needs spaces around operator.

> +        + (env->cpu_index % smp_threads);
> +
> +    return 0;
> +}

ppc_def_t def is unused, and ppc_def_t is going to be refactored away.
env is becoming cpu in my series, which can later access its class via
macros if need be.

I like this code being moved into a KVM file. Applying this first will
reduce the amount of code I move around when QOM'ifying CPU init.

> +
> +
>  bool kvm_arch_stop_on_emulation_error(CPUPPCState *env)
>  {
>      return true;
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 8f1267c..9940e39 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -29,6 +29,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t 
> window_size, int *pfd);
>  int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size);
>  #endif /* !CONFIG_USER_ONLY */
>  const ppc_def_t *kvmppc_host_cpu_def(void);
> +int kvm_fixup_ppc_env(CPUPPCState *env, const ppc_def_t *def);

All functions in kvm_ppc.h have a kvmppc_ prefix except this one.

>  
>  #else
>  
> @@ -95,6 +96,10 @@ static inline const ppc_def_t *kvmppc_host_cpu_def(void)
>      return NULL;
>  }
>  
> +static inline int kvm_fixup_ppc_env(CPUPPCState *env, const ppc_def_t *def)
> +{
> +    return -1;
> +}
>  #endif
>  
>  #ifndef CONFIG_KVM
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 367eefa..bbdf174 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9889,6 +9889,28 @@ static int gdb_set_spe_reg(CPUPPCState *env, uint8_t 
> *mem_buf, int n)
>      return 0;
>  }
>  
> +static int tcg_fixup_ppc_env(CPUPPCState *env, const ppc_def_t *def)
> +{
> +    /* TCG doesn't (yet) emulate some groups of instructions that
> +     * are implemented on some otherwise supported CPUs (e.g. VSX
> +     * and decimal floating point instructions on POWER7).  We
> +     * remove unsupported instruction groups from the cpu state's
> +     * instruction masks and hope the guest can cope.  For at
> +     * least the pseries machine, the unavailability of these
> +     * instructions can be advertise to the guest via the device

Grammar error in moved comment: advertised

> +     * tree. */
> +    if ((env->insns_flags & ~PPC_TCG_INSNS)
> +        || (env->insns_flags2 & ~PPC_TCG_INSNS2)) {
> +        fprintf(stderr, "Warning: Disabling some instructions which are not "
> +                "emulated by TCG (0x%" PRIx64 ", 0x%" PRIx64 ")\n",
> +                env->insns_flags & ~PPC_TCG_INSNS,
> +                env->insns_flags2 & ~PPC_TCG_INSNS2);
> +    }
> +    env->insns_flags &= PPC_TCG_INSNS;
> +    env->insns_flags2 &= PPC_TCG_INSNS2;
> +    return 0;
> +}

Don't spot ppc_def_t def being used here either.

> +
>  int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def)
>  {
>      env->msr_mask = def->msr_mask;
> @@ -9897,25 +9919,22 @@ int cpu_ppc_register_internal (CPUPPCState *env, 
> const ppc_def_t *def)
>      env->bus_model = def->bus_model;
>      env->insns_flags = def->insns_flags;
>      env->insns_flags2 = def->insns_flags2;
> -    if (!kvm_enabled()) {
> -        /* TCG doesn't (yet) emulate some groups of instructions that
> -         * are implemented on some otherwise supported CPUs (e.g. VSX
> -         * and decimal floating point instructions on POWER7).  We
> -         * remove unsupported instruction groups from the cpu state's
> -         * instruction masks and hope the guest can cope.  For at
> -         * least the pseries machine, the unavailability of these
> -         * instructions can be advertise to the guest via the device
> -         * tree.
> -         *
> -         * FIXME: we should have a similar masking for CPU features
> -         * not accessible under KVM, but so far, there aren't any of
> -         * those. */
> -        env->insns_flags &= PPC_TCG_INSNS;
> -        env->insns_flags2 &= PPC_TCG_INSNS2;
> -    }
>      env->flags = def->flags;
>      env->bfd_mach = def->bfd_mach;
>      env->check_pow = def->check_pow;
> +
> +    if (kvm_enabled()) {
> +        if (kvm_fixup_ppc_env(env, def) != 0) {
> +            fprintf(stderr, "Unable to virtualize selected cpu with KVM\n");
> +            exit(1);
> +        }
> +    } else {

I've wondered whether this should be conditional to tcg_enabled(), but
for one this matches the current logic and for another it shouldn't make
a difference for qtest.

> +        if (tcg_fixup_ppc_env(env, def) != 0) {
> +            fprintf(stderr, "Unable to emulated selected cpu with TCG\n");

"emulate", and let's uppercase CPU.

> +            exit(1);
> +        }
> +    }

While I can see the beauty in symmetry, the tcg_ prefix looks wrong
here, seems reserved for tcg/.

I'm going to repost our combined series, changing this to
kvmppc_fixup_cpu(env) and ppc_fixup_cpu(env) respectively. Either way we
are going to refactor this code again soon...

Andreas

> +
>      if (create_ppc_opcodes(env, def) < 0)
>          return -1;
>      init_ppc_proc(env, def);

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg