Message ID | 1332896731-25957-1-git-send-email-david@gibson.dropbear.id.au |
---|---|
State | New |
Headers | show |
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. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Patch overall looks good to me, except for some minor things. > > Conflicts: > > 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(-) > > 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); > } > > +static void rtas_system_reboot(sPAPREnvironment *spapr, > + uint32_t token, uint32_t nargs, > + target_ulong args, > + uint32_t nret, target_ulong rets) > +{ > + if (nargs != 0 || nret != 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 nargs, > 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, sPAPREnvironment *spapr, > > /* Check if device supports CRQs */ > if (!dev->crq.SendFunc) { > + hcall_dprintf("h_reg_crq, device does not support CRQ\n"); > return H_NOT_FOUND; > } > > - 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 = queue_addr; > @@ -453,6 +454,17 @@ static target_ulong h_reg_crq(CPUPPCState *env, sPAPREnvironment *spapr, > return H_SUCCESS; > } > > +static target_ulong free_crq(VIOsPAPRDevice *dev) > +{ > + dev->crq.qladdr = 0; > + dev->crq.qsize = 0; > + dev->crq.qnext = 0; > + > + dprintf("CRQ for dev 0x%" PRIx32 " freed\n", dev->reg); > + > + return H_SUCCESS; > +} > + > static target_ulong h_free_crq(CPUPPCState *env, sPAPREnvironment *spapr, > target_ulong opcode, target_ulong *args) > { > @@ -465,13 +477,7 @@ static target_ulong h_free_crq(CPUPPCState *env, sPAPREnvironment *spapr, > return H_PARAMETER; > } > > - dev->crq.qladdr = 0; > - dev->crq.qsize = 0; > - dev->crq.qnext = 0; > - > - dprintf("CRQ for dev 0x" TARGET_FMT_lx " freed\n", reg); > - > - return H_SUCCESS; > + return free_crq(dev); > } > > static target_ulong h_send_crq(CPUPPCState *env, sPAPREnvironment *spapr, Here freeing crq is moved to its own static function... > @@ -649,6 +655,15 @@ static int spapr_vio_check_reg(VIOsPAPRDevice *sdev) > return 0; > } > > +static void spapr_vio_busdev_reset(void *opaque) > +{ > + VIOsPAPRDevice *dev = (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 = (VIOsPAPRDevice *)qdev; > @@ -677,6 +692,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev) > > rtce_init(dev); > > + qemu_register_reset(spapr_vio_busdev_reset, dev); > + > return pc->init(dev); > } > ... 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
On Wed, Mar 28, 2012 at 06:15:18PM +0200, Andreas Färber wrote: > 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. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > Patch overall looks good to me, except for some minor things. Ok. Comments folded in and resent.
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); } +static void rtas_system_reboot(sPAPREnvironment *spapr, + uint32_t token, uint32_t nargs, + target_ulong args, + uint32_t nret, target_ulong rets) +{ + if (nargs != 0 || nret != 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 nargs, 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); 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, sPAPREnvironment *spapr, /* Check if device supports CRQs */ if (!dev->crq.SendFunc) { + hcall_dprintf("h_reg_crq, device does not support CRQ\n"); return H_NOT_FOUND; } - /* Already a queue ? */ if (dev->crq.qsize) { + hcall_dprintf("h_reg_crq, CRQ already registered\n"); return H_RESOURCE; } dev->crq.qladdr = queue_addr; @@ -453,6 +454,17 @@ static target_ulong h_reg_crq(CPUPPCState *env, sPAPREnvironment *spapr, return H_SUCCESS; } +static target_ulong free_crq(VIOsPAPRDevice *dev) +{ + dev->crq.qladdr = 0; + dev->crq.qsize = 0; + dev->crq.qnext = 0; + + dprintf("CRQ for dev 0x%" PRIx32 " freed\n", dev->reg); + + return H_SUCCESS; +} + static target_ulong h_free_crq(CPUPPCState *env, sPAPREnvironment *spapr, target_ulong opcode, target_ulong *args) { @@ -465,13 +477,7 @@ static target_ulong h_free_crq(CPUPPCState *env, sPAPREnvironment *spapr, return H_PARAMETER; } - dev->crq.qladdr = 0; - dev->crq.qsize = 0; - dev->crq.qnext = 0; - - dprintf("CRQ for dev 0x" TARGET_FMT_lx " freed\n", reg); - - return H_SUCCESS; + return free_crq(dev); } static target_ulong h_send_crq(CPUPPCState *env, sPAPREnvironment *spapr, @@ -649,6 +655,15 @@ static int spapr_vio_check_reg(VIOsPAPRDevice *sdev) return 0; } +static void spapr_vio_busdev_reset(void *opaque) +{ + VIOsPAPRDevice *dev = (VIOsPAPRDevice *)opaque; + + if (dev->crq.qsize) { + free_crq(dev); + } +} + static int spapr_vio_busdev_init(DeviceState *qdev) { VIOsPAPRDevice *dev = (VIOsPAPRDevice *)qdev; @@ -677,6 +692,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev) rtce_init(dev); + qemu_register_reset(spapr_vio_busdev_reset, dev); + return pc->init(dev); }
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. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Conflicts: hw/spapr_vio.c --- hw/spapr_rtas.c | 14 ++++++++++++++ hw/spapr_vio.c | 33 +++++++++++++++++++++++++-------- 2 files changed, 39 insertions(+), 8 deletions(-)