Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/7] pseries: Fix and cleanup CPU initialization and reset

[David Gibson <david@gibson.dropbear.id.au>]

On Mon, Sep 10, 2012 at 03:45:31PM +0200, Andreas Färber wrote:
> Am 10.09.2012 08:38, schrieb David Gibson:
> > The current pseries machine init function iterates over the CPUs at several
> > points, doing various bits of initialization.  This is messy; these can
> > and should be merged into a single iteration doing all the necessary per
> > cpu initialization.  Worse, some of these initializations were setting up
> > state which should be set on every reset, not just at machine init time.
> > A few of the initializations simply weren't necessary at all.
> > 
> > This patch, therefore, moves those things that need to be to the
> > per-cpu reset handler, and combines the remainder into two loops over
> > the cpus (which also creates them).  The second loop is for setting up
> > hash table information, and will be removed in a subsequent patch also
> > making other fixes to the hash table setup.
> > 
> > This exposes a bug in our start-cpu RTAS routine (called by the guest to
> > start up CPUs other than CPU0) under kvm.  Previously, this function did
> > not make a call to ensure that it's changes to the new cpu's state were
> > pushed into KVM in-kernel state.  We sort-of got away with this because
> > some of the initializations had already placed the secondary CPUs into the
> > right starting state for the sorts of Linux guests we've been running.
> > 
> > Nonetheless the start-cpu RTAS call's behaviour was not correct and could
> > easily have been broken by guest changes.  This patch also fixes it.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/spapr.c      |   33 +++++++++++++++++++--------------
> >  hw/spapr_rtas.c |    5 +++++
> >  2 files changed, 24 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/spapr.c b/hw/spapr.c
> > index c34b767..be9092a 100644
> > --- a/hw/spapr.c
> > +++ b/hw/spapr.c
> > @@ -581,8 +581,15 @@ static void spapr_reset(void *opaque)
> >  static void spapr_cpu_reset(void *opaque)
> >  {
> >      PowerPCCPU *cpu = opaque;
> > +    CPUPPCState *env = &cpu->env;
> >  
> >      cpu_reset(CPU(cpu));
> > +
> > +    /* All CPUs start halted, except CPU0, the rest are explicitly
> > +     * started up by the guest using an RTAS call */
> > +    env->halted = 1;
> 
> That comment does not make sense to me. There is no differentiation
> between CPU0 and other CPUs here, is an if (env->cpu_index != 0) missing
> or is ->halted overwritten for CPU0 elsewhere? (then the comment should
> probably go there instead?)

Ah, I guess it's not entirely clear here.  Yes, ->halted is
overwritten for CPU0 elsewhere, from spapr_reset().  Below is a
revised version of the patch with a clearer comment.

>From c5b36b49f51bda02e6b012d6a004ef9862de856f Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Tue, 11 Sep 2012 09:44:17 +1000
Subject: [PATCH] pseries: Fix and cleanup CPU initialization and reset

The current pseries machine init function iterates over the CPUs at several
points, doing various bits of initialization.  This is messy; these can
and should be merged into a single iteration doing all the necessary per
cpu initialization.  Worse, some of these initializations were setting up
state which should be set on every reset, not just at machine init time.
A few of the initializations simply weren't necessary at all.

This patch, therefore, moves those things that need to be to the
per-cpu reset handler, and combines the remainder into two loops over
the cpus (which also creates them).  The second loop is for setting up
hash table information, and will be removed in a subsequent patch also
making other fixes to the hash table setup.

This exposes a bug in our start-cpu RTAS routine (called by the guest to
start up CPUs other than CPU0) under kvm.  Previously, this function did
not make a call to ensure that it's changes to the new cpu's state were
pushed into KVM in-kernel state.  We sort-of got away with this because
some of the initializations had already placed the secondary CPUs into the
right starting state for the sorts of Linux guests we've been running.

Nonetheless the start-cpu RTAS call's behaviour was not correct and could
easily have been broken by guest changes.  This patch also fixes it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr.c      |   34 ++++++++++++++++++++--------------
 hw/spapr_rtas.c |    5 +++++
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/hw/spapr.c b/hw/spapr.c
index c34b767..d88525a 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -581,8 +581,16 @@ static void spapr_reset(void *opaque)
 static void spapr_cpu_reset(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
+    CPUPPCState *env = &cpu->env;
 
     cpu_reset(CPU(cpu));
+
+    /* All CPUs start halted.  CPU0 is unhalted from the machine level
+     * reset code and the rest are explicitly started up by the guest
+     * using an RTAS call */
+    env->halted = 1;
+
+    env->spr[SPR_HIOR] = 0;
 }
 
 /* Returns whether we want to use VGA or not */
@@ -665,11 +673,16 @@ static void ppc_spapr_init(ram_addr_t ram_size,
 
         /* Set time-base frequency to 512 MHz */
         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
-        qemu_register_reset(spapr_cpu_reset, cpu);
 
-        env->hreset_vector = 0x60;
+        /* PAPR always has exception vectors in RAM not ROM */
         env->hreset_excp_prefix = 0;
-        env->gpr[3] = env->cpu_index;
+
+        /* Tell KVM that we're in PAPR mode */
+        if (kvm_enabled()) {
+            kvmppc_set_papr(env);
+        }
+
+        qemu_register_reset(spapr_cpu_reset, cpu);
     }
 
     /* allocate RAM */
@@ -685,7 +698,10 @@ static void ppc_spapr_init(ram_addr_t ram_size,
 
     /* allocate hash page table.  For now we always make this 16mb,
      * later we should probably make it scale to the size of guest
-     * RAM */
+     * RAM.  FIXME: setting the htab information in the CPU env really
+     * belongs at CPU reset time, but we can get away with it for now
+     * because the PAPR guest is not permitted to write SDR1 so in
+     * fact these settings will never change during the run */
     spapr->htab_size = 1ULL << (pteg_shift + 7);
     spapr->htab = qemu_memalign(spapr->htab_size, spapr->htab_size);
 
@@ -697,11 +713,6 @@ static void ppc_spapr_init(ram_addr_t ram_size,
         /* Tell KVM that we're in PAPR mode */
         env->spr[SPR_SDR1] = (unsigned long)spapr->htab |
                              ((pteg_shift + 7) - 18);
-        env->spr[SPR_HIOR] = 0;
-
-        if (kvm_enabled()) {
-            kvmppc_set_papr(env);
-        }
     }
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin");
@@ -827,11 +838,6 @@ static void ppc_spapr_init(ram_addr_t ram_size,
 
     spapr->entry_point = 0x100;
 
-    /* SLOF will startup the secondary CPUs using RTAS */
-    for (env = first_cpu; env != NULL; env = env->next_cpu) {
-        env->halted = 1;
-    }
-
     /* Prepare the device tree */
     spapr->fdt_skel = spapr_create_fdt_skel(cpu_model, rma_size,
                                             initrd_base, initrd_size,
diff --git a/hw/spapr_rtas.c b/hw/spapr_rtas.c
index ae18595..b808f80 100644
--- a/hw/spapr_rtas.c
+++ b/hw/spapr_rtas.c
@@ -184,6 +184,11 @@ static void rtas_start_cpu(sPAPREnvironment *spapr,
             return;
         }
 
+        /* This will make sure qemu state is up to date with kvm, and
+         * mark it dirty so our changes get flushed back before the
+         * new cpu enters */
+        kvm_cpu_synchronize_state(env);
+
         env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
         env->nip = start;
         env->gpr[3] = r3;
-- 
1.7.10.4



-- 
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