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 <da...@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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
