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 <da...@gibson.dropbear.id.au>
> ---
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
