From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56487) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TB4J7-0006hW-Ph for qemu-devel@nongnu.org; Mon, 10 Sep 2012 09:45:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TB4Iw-0004T7-Rz for qemu-devel@nongnu.org; Mon, 10 Sep 2012 09:45:45 -0400 Message-ID: <504DEEFB.3020203@suse.de> Date: Mon, 10 Sep 2012 15:45:31 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1347259132-31184-1-git-send-email-david@gibson.dropbear.id.au> <1347259132-31184-3-git-send-email-david@gibson.dropbear.id.au> In-Reply-To: <1347259132-31184-3-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/7] pseries: Fix and cleanup CPU initialization and reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-ppc@nongnu.org, agraf@suse.de, qemu-devel@nongnu.org Am 10.09.2012 08:38, schrieb David Gibson: > The current pseries machine init function iterates over the CPUs at sev= eral > points, doing various bits of initialization. This is messy; these can > and should be merged into a single iteration doing all the necessary pe= r > 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. >=20 > 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. >=20 > This exposes a bug in our start-cpu RTAS routine (called by the guest t= o > start up CPUs other than CPU0) under kvm. Previously, this function di= d > 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. >=20 > Nonetheless the start-cpu RTAS call's behaviour was not correct and cou= ld > easily have been broken by guest changes. This patch also fixes it. >=20 > Signed-off-by: David Gibson > --- > hw/spapr.c | 33 +++++++++++++++++++-------------- > hw/spapr_rtas.c | 5 +++++ > 2 files changed, 24 insertions(+), 14 deletions(-) >=20 > 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 =3D opaque; > + CPUPPCState *env =3D &cpu->env; > =20 > 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 =3D 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 !=3D 0) missin= g or is ->halted overwritten for CPU0 elsewhere? (then the comment should probably go there instead?) Andreas > + > + env->spr[SPR_HIOR] =3D 0; > } > =20 > /* Returns whether we want to use VGA or not */ > @@ -665,11 +672,16 @@ static void ppc_spapr_init(ram_addr_t ram_size, > =20 > /* Set time-base frequency to 512 MHz */ > cpu_ppc_tb_init(env, TIMEBASE_FREQ); > - qemu_register_reset(spapr_cpu_reset, cpu); > =20 > - env->hreset_vector =3D 0x60; > + /* PAPR always has exception vectors in RAM not ROM */ > env->hreset_excp_prefix =3D 0; > - env->gpr[3] =3D 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); > } > =20 > /* allocate RAM */ > @@ -685,7 +697,10 @@ static void ppc_spapr_init(ram_addr_t ram_size, > =20 > /* 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 =3D 1ULL << (pteg_shift + 7); > spapr->htab =3D qemu_memalign(spapr->htab_size, spapr->htab_size); > =20 > @@ -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] =3D (unsigned long)spapr->htab | > ((pteg_shift + 7) - 18); > - env->spr[SPR_HIOR] =3D 0; > - > - if (kvm_enabled()) { > - kvmppc_set_papr(env); > - } > } > =20 > filename =3D qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")= ; > @@ -827,11 +837,6 @@ static void ppc_spapr_init(ram_addr_t ram_size, > =20 > spapr->entry_point =3D 0x100; > =20 > - /* SLOF will startup the secondary CPUs using RTAS */ > - for (env =3D first_cpu; env !=3D NULL; env =3D env->next_cpu) { > - env->halted =3D 1; > - } > - > /* Prepare the device tree */ > spapr->fdt_skel =3D 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; > } > =20 > + /* 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 =3D (1ULL << MSR_SF) | (1ULL << MSR_ME); > env->nip =3D start; > env->gpr[3] =3D r3; >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg