From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44910) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCvWz-0000lZ-5c for qemu-devel@nongnu.org; Wed, 28 Mar 2012 12:15:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SCvWs-0002FR-P4 for qemu-devel@nongnu.org; Wed, 28 Mar 2012 12:15:28 -0400 Message-ID: <4F733916.5010104@suse.de> Date: Wed, 28 Mar 2012 18:15:18 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1332896731-25957-1-git-send-email-david@gibson.dropbear.id.au> In-Reply-To: <1332896731-25957-1-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] pseries: Implement RTAS system-reboot call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: scottwood@freescale.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org Am 28.03.2012 03:05, schrieb David Gibson: > This patch adds the PAPR defined RTAS system-reboot call to the pseries > machine emulation, providing the guest with a way to trigger a reboot. > This exposes a bug in the pseries VIO code which means CRQs are not > properly reset on a system reset. This patch also fixes that bug by > adding a suitable reset handler. >=20 > Signed-off-by: David Gibson Patch overall looks good to me, except for some minor things. >=20 > Conflicts: >=20 > hw/spapr_vio.c Please drop conflicts from commit messages (but no need to resend). > --- > hw/spapr_rtas.c | 14 ++++++++++++++ > hw/spapr_vio.c | 33 +++++++++++++++++++++++++-------- > 2 files changed, 39 insertions(+), 8 deletions(-) >=20 > diff --git a/hw/spapr_rtas.c b/hw/spapr_rtas.c > index 0946585..480a4ae 100644 > --- a/hw/spapr_rtas.c > +++ b/hw/spapr_rtas.c > @@ -112,6 +112,19 @@ static void rtas_power_off(sPAPREnvironment *spapr= , > rtas_st(rets, 0, 0); > } > =20 > +static void rtas_system_reboot(sPAPREnvironment *spapr, > + uint32_t token, uint32_t nargs, > + target_ulong args, > + uint32_t nret, target_ulong rets) > +{ > + if (nargs !=3D 0 || nret !=3D 1) { > + rtas_st(rets, 0, -3); > + return; > + } > + qemu_system_reset_request(); > + rtas_st(rets, 0, 0); > +} > + > static void rtas_query_cpu_stopped_state(sPAPREnvironment *spapr, > uint32_t token, uint32_t narg= s, > target_ulong args, > @@ -294,6 +307,7 @@ static void core_rtas_register_types(void) > spapr_rtas_register("get-time-of-day", rtas_get_time_of_day); > spapr_rtas_register("set-time-of-day", rtas_set_time_of_day); > spapr_rtas_register("power-off", rtas_power_off); > + spapr_rtas_register("system-reboot", rtas_system_reboot); > spapr_rtas_register("query-cpu-stopped-state", > rtas_query_cpu_stopped_state); > spapr_rtas_register("start-cpu", rtas_start_cpu); This part looks good to go. > diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c > index dbf5a90..200f27c 100644 > --- a/hw/spapr_vio.c > +++ b/hw/spapr_vio.c > @@ -435,12 +435,13 @@ static target_ulong h_reg_crq(CPUPPCState *env, s= PAPREnvironment *spapr, > =20 > /* Check if device supports CRQs */ > if (!dev->crq.SendFunc) { > + hcall_dprintf("h_reg_crq, device does not support CRQ\n"); > return H_NOT_FOUND; > } > =20 > - Was this whitespace change intentional for consistency? > /* Already a queue ? */ > if (dev->crq.qsize) { > + hcall_dprintf("h_reg_crq, CRQ already registered\n"); Adding dprintfs for CRQs seems only indirectly related. I'd suggest a separate patch and using __func__ so that function name and comment don't start to differ. > return H_RESOURCE; > } > dev->crq.qladdr =3D queue_addr; > @@ -453,6 +454,17 @@ static target_ulong h_reg_crq(CPUPPCState *env, sP= APREnvironment *spapr, > return H_SUCCESS; > } > =20 > +static target_ulong free_crq(VIOsPAPRDevice *dev) > +{ > + dev->crq.qladdr =3D 0; > + dev->crq.qsize =3D 0; > + dev->crq.qnext =3D 0; > + > + dprintf("CRQ for dev 0x%" PRIx32 " freed\n", dev->reg); > + > + return H_SUCCESS; > +} > + > static target_ulong h_free_crq(CPUPPCState *env, sPAPREnvironment *spa= pr, > target_ulong opcode, target_ulong *args= ) > { > @@ -465,13 +477,7 @@ static target_ulong h_free_crq(CPUPPCState *env, s= PAPREnvironment *spapr, > return H_PARAMETER; > } > =20 > - dev->crq.qladdr =3D 0; > - dev->crq.qsize =3D 0; > - dev->crq.qnext =3D 0; > - > - dprintf("CRQ for dev 0x" TARGET_FMT_lx " freed\n", reg); > - > - return H_SUCCESS; > + return free_crq(dev); > } > =20 > static target_ulong h_send_crq(CPUPPCState *env, sPAPREnvironment *spa= pr, Here freeing crq is moved to its own static function... > @@ -649,6 +655,15 @@ static int spapr_vio_check_reg(VIOsPAPRDevice *sde= v) > return 0; > } > =20 > +static void spapr_vio_busdev_reset(void *opaque) > +{ > + VIOsPAPRDevice *dev =3D (VIOsPAPRDevice *)opaque; > + > + if (dev->crq.qsize) { > + free_crq(dev); > + } > +} > + ...so that it can be used... > static int spapr_vio_busdev_init(DeviceState *qdev) > { > VIOsPAPRDevice *dev =3D (VIOsPAPRDevice *)qdev; > @@ -677,6 +692,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev) > =20 > rtce_init(dev); > =20 > + qemu_register_reset(spapr_vio_busdev_reset, dev); > + > return pc->init(dev); > } > =20 ... for a new reset handler. Would you mind splitting this off into a preceding patch? Cleaning up / freeing resources on reset seems a good idea independent of the new hcall (thinking of reset from monitor interface). I'll look into rebasing ppc-next meanwhile. Andreas --=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