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 <da...@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?)
Andreas
> +
> + env->spr[SPR_HIOR] = 0;
> }
>
> /* Returns whether we want to use VGA or not */
> @@ -665,11 +672,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 +697,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 +712,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 +837,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;
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
