From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38987) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Swxa5-000436-Fk for qemu-devel@nongnu.org; Thu, 02 Aug 2012 11:45:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Swxa3-0003k3-CT for qemu-devel@nongnu.org; Thu, 02 Aug 2012 11:44:57 -0400 Received: from cantor2.suse.de ([195.135.220.15]:44305 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Swxa3-0003jz-1x for qemu-devel@nongnu.org; Thu, 02 Aug 2012 11:44:55 -0400 Message-ID: <501AA071.3030406@suse.de> Date: Thu, 02 Aug 2012 17:44:49 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1343873409-8571-1-git-send-email-david@gibson.dropbear.id.au> <1343873409-8571-3-git-send-email-david@gibson.dropbear.id.au> In-Reply-To: <1343873409-8571-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/2] pseries: Use new hook to correct reset sequence List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: Igor Mammedov , qemu-devel@nongnu.org, anthony@codemonkey.ws, agraf@suse.de Am 02.08.2012 04:10, schrieb David Gibson: > A number of things need to occur during reset of the PAPR paravirtualiz= ed > platform in a specific order. For example, the hash table needs to be > cleared before the CPUs are reset, so that they initialize their regist= er > state correctly, and the CPUs need to have their main reset called befo= re > we set up the entry point state on the boot cpu. We also need to have > the main qdev reset happen before the creation and installation of the > device tree for the new boot, because we need the state of the devices > settled to correctly construct the device tree. >=20 > Currently reset of pseries is broken in a number of ways, and in other > cases works largely by accident. This patch uses the new QEMUMachine re= set > hook to correct these problems, by replacing the several existing spapr > reset hooks with one new machine hook which ensures that the various st= ages > happen in the correct order. >=20 > Signed-off-by: David Gibson > --- > hw/spapr.c | 66 +++++++++++++++++++++++++++++++++-------------------= -------- > 1 file changed, 36 insertions(+), 30 deletions(-) >=20 > diff --git a/hw/spapr.c b/hw/spapr.c > index 2453bae..1e60ec1 100644 > --- a/hw/spapr.c > +++ b/hw/spapr.c > @@ -582,29 +582,22 @@ static void spapr_reset_htab(sPAPREnvironment *sp= apr) > } > } > =20 > -static void spapr_reset(void *opaque) > +static void spapr_reset_cpu(CPUPPCState *env) > { > - sPAPREnvironment *spapr =3D (sPAPREnvironment *)opaque; > - > - /* Reset the hash table & recalc the RMA */ > - spapr_reset_htab(spapr); > - > - /* Load the fdt */ > - spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr, > - spapr->rtas_size); > -} > - > -static void spapr_cpu_reset(void *opaque) > -{ > - PowerPCCPU *cpu =3D opaque; > - CPUPPCState *env =3D &cpu->env; > + PowerPCCPU *cpu =3D container_of(env, PowerPCCPU, env); NACK. Please don't undo the cleanups I have applied! Functions should take a QOM PowerPCCPU, not its internal CPUPPCState. Fields are gradually being moved from CPUxxxState into CPUState. > =20 > cpu_reset(CPU(cpu)); Also note the current discussion about CPU reset and ordering, e.g.: http://patchwork.ozlabs.org/patch/174602/ Anthony was favoring moving reset code out of machines and expressed dislike for looping through CPUs, which my above patch took into account. The ordering issue between CPU and devices is still unsolved the= re. Some on-list comments from Anthony would be nice, since we are moving into opposing directions here - having the sPAPR machine be more in control vs. moving code away from the PC machine into target-i386 CPU and/or common CPU code. Cheers, Andreas > =20 > env->external_htab =3D spapr->htab; > env->htab_base =3D -1; > env->htab_mask =3D HTAB_SIZE(spapr) - 1; > + /* CPUs need to start halted at reset, the platform reset code > + * will activate CPU0 then the rest are explicitly started by the > + * guest using RTAS */ > + env->halted =3D 1; > =20 > + /* Secondary CPUs get the CPU ID in r3 on entry */ > + env->gpr[3] =3D env->cpu_index; > env->spr[SPR_SDR1] =3D (unsigned long)spapr->htab | > (spapr->htab_shift - 18); > =20 > @@ -612,14 +605,35 @@ static void spapr_cpu_reset(void *opaque) > kvmppc_update_sdr1(env); > } > =20 > - /* Set up the entry state */ > - if (env =3D=3D first_cpu) { > - env->gpr[3] =3D spapr->fdt_addr; > - env->gpr[5] =3D 0; > - env->halted =3D 0; > - env->nip =3D spapr->entry_point; > + tb_flush(env); > +} > + > +static void spapr_reset(bool report) > +{ > + CPUPPCState *env =3D first_cpu; > + > + /* Reset the qdevs */ > + qemu_default_system_reset(report); > + > + /* Reset the hash table & recalc the RMA */ > + spapr_reset_htab(spapr); > + > + /* Reset the CPUs */ > + for (env =3D first_cpu; env !=3D NULL; env =3D env->next_cpu) { > + spapr_reset_cpu(env); > } > =20 > + /* Load the fdt */ > + spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr, > + spapr->rtas_size); > + > + /* Set up the entry state on CPU0 */ > + env =3D first_cpu; > + > + env->gpr[3] =3D spapr->fdt_addr; > + env->gpr[5] =3D 0; > + env->halted =3D 0; > + env->nip =3D spapr->entry_point; > tb_flush(env); > } > =20 > @@ -718,8 +732,6 @@ static void ppc_spapr_init(ram_addr_t ram_size, > /* FIXME: we should change this default based on RAM size */ > spapr->htab_shift =3D 24; > =20 > - qemu_register_reset(spapr_reset, spapr); > - > /* init CPUs */ > if (cpu_model =3D=3D NULL) { > cpu_model =3D kvm_enabled() ? "host" : "POWER7"; > @@ -734,11 +746,9 @@ 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; > env->hreset_excp_prefix =3D 0; > - env->gpr[3] =3D env->cpu_index; > } > =20 > /* allocate RAM */ > @@ -883,11 +893,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, > initrd_base, initrd_size, > @@ -900,6 +905,7 @@ static QEMUMachine spapr_machine =3D { > .name =3D "pseries", > .desc =3D "pSeries Logical Partition (PAPR compliant)", > .init =3D ppc_spapr_init, > + .reset =3D spapr_reset, > .max_cpus =3D MAX_CPUS, > .no_parallel =3D 1, > .use_scsi =3D 1, >=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