From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40550) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sx09O-0000sQ-US for qemu-devel@nongnu.org; Thu, 02 Aug 2012 14:29:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sx09N-0005w1-5F for qemu-devel@nongnu.org; Thu, 02 Aug 2012 14:29:34 -0400 Received: from mail-qc0-f173.google.com ([209.85.216.173]:62885) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sx09N-0005vv-00 for qemu-devel@nongnu.org; Thu, 02 Aug 2012 14:29:33 -0400 Received: by qcab12 with SMTP id b12so5262692qca.4 for ; Thu, 02 Aug 2012 11:29:32 -0700 (PDT) From: Anthony Liguori In-Reply-To: <501AA071.3030406@suse.de> References: <1343873409-8571-1-git-send-email-david@gibson.dropbear.id.au> <1343873409-8571-3-git-send-email-david@gibson.dropbear.id.au> <501AA071.3030406@suse.de> Date: Thu, 02 Aug 2012 13:29:29 -0500 Message-ID: <87vch1i1va.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 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: Andreas =?utf-8?Q?F=C3=A4rber?= , David Gibson Cc: Igor Mammedov , qemu-devel@nongnu.org, agraf@suse.de Andreas F=C3=A4rber writes: > Am 02.08.2012 04:10, schrieb David Gibson: >> A number of things need to occur during reset of the PAPR paravirtualized >> platform in a specific order. For example, the hash table needs to be >> cleared before the CPUs are reset, so that they initialize their register >> state correctly, and the CPUs need to have their main reset called before >> 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 res= et >> hook to correct these problems, by replacing the several existing spapr >> reset hooks with one new machine hook which ensures that the various sta= ges >> 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 *spa= pr) >> } >> } >>=20=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=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. I already commented on the first patch because I had a feeling you'd post something like this ;-) Regarding reset: 1) Devices should implement DeviceState::reset() 2) If a device doesn't implement ::reset(), it should call qemu_register_reset() 3) Reset should propagate through the device model, starting with the top-level machine which is logically what's plugged into the wall and is the source of power in the first place. Regards, Anthony Liguori > > Cheers, > Andreas > >>=20=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=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=20 >> @@ -612,14 +605,35 @@ static void spapr_cpu_reset(void *opaque) >> kvmppc_update_sdr1(env); >> } >>=20=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=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=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=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=20 >> /* Set time-base frequency to 512 MHz */ >> cpu_ppc_tb_init(env, TIMEBASE_FREQ); >> - qemu_register_reset(spapr_cpu_reset, cpu); >>=20=20 >> env->hreset_vector =3D 0x60; >> env->hreset_excp_prefix =3D 0; >> - env->gpr[3] =3D env->cpu_index; >> } >>=20=20 >> /* allocate RAM */ >> @@ -883,11 +893,6 @@ static void ppc_spapr_init(ram_addr_t ram_size, >>=20=20 >> spapr->entry_point =3D 0x100; >>=20=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=C3=BCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3= =BCrnberg