[Qemu-devel] [PATCH 1/3] pseries: Clean up hcall_dprintf() debugging messages

[Qemu-devel] [PATCH 1/3] pseries: Clean up hcall_dprintf() debugging messages

[David Gibson]

The pseries machine code has a number of debug messages for debugging PAPR
hypercalls, dependent on DEBUG_SPAPR_HCALLS.  This patch cleans these
messages up a bit, by adding __func__ to the hcall_dprintf() macro and
simplifying up a number of the individual messages accordingly.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Conflicts:

	hw/spapr_vio.c
---
 hw/spapr.h      |    2 +-
 hw/spapr_llan.c |   12 +++++-------
 hw/spapr_vio.c  |   25 +++++++++----------------
 3 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/hw/spapr.h b/hw/spapr.h
index 11160b0..654a7a8 100644
--- a/hw/spapr.h
+++ b/hw/spapr.h
@@ -272,7 +272,7 @@ extern sPAPREnvironment *spapr;
 
 #ifdef DEBUG_SPAPR_HCALLS
 #define hcall_dprintf(fmt, ...) \
-    do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
+    do { fprintf(stderr, "%s: " fmt, __func__, ## __VA_ARGS__); } while (0)
 #else
 #define hcall_dprintf(fmt, ...) \
     do { } while (0)
diff --git a/hw/spapr_llan.c b/hw/spapr_llan.c
index cfc7778..32dce17 100644
--- a/hw/spapr_llan.c
+++ b/hw/spapr_llan.c
@@ -279,21 +279,19 @@ static target_ulong h_register_logical_lan(CPUPPCState *env,
 
     if (check_bd(dev, VLAN_VALID_BD(buf_list, SPAPR_VIO_TCE_PAGE_SIZE),
                  SPAPR_VIO_TCE_PAGE_SIZE) < 0) {
-        hcall_dprintf("Bad buf_list 0x" TARGET_FMT_lx " for "
-                      "H_REGISTER_LOGICAL_LAN\n", buf_list);
+        hcall_dprintf("Bad buf_list 0x" TARGET_FMT_lx "\n", buf_list);
         return H_PARAMETER;
     }
 
     filter_list_bd = VLAN_VALID_BD(filter_list, SPAPR_VIO_TCE_PAGE_SIZE);
     if (check_bd(dev, filter_list_bd, SPAPR_VIO_TCE_PAGE_SIZE) < 0) {
-        hcall_dprintf("Bad filter_list 0x" TARGET_FMT_lx " for "
-                      "H_REGISTER_LOGICAL_LAN\n", filter_list);
+        hcall_dprintf("Bad filter_list 0x" TARGET_FMT_lx "\n", filter_list);
         return H_PARAMETER;
     }
 
     if (!(rec_queue & VLAN_BD_VALID)
         || (check_bd(dev, rec_queue, VLAN_RQ_ALIGNMENT) < 0)) {
-        hcall_dprintf("Bad receive queue for H_REGISTER_LOGICAL_LAN\n");
+        hcall_dprintf("Bad receive queue\n");
         return H_PARAMETER;
     }
 
@@ -358,13 +356,13 @@ static target_ulong h_add_logical_lan_buffer(CPUPPCState *env,
             ", 0x" TARGET_FMT_lx ")\n", reg, buf);
 
     if (!sdev) {
-        hcall_dprintf("Wrong device in h_add_logical_lan_buffer\n");
+        hcall_dprintf("Bad device\n");
         return H_PARAMETER;
     }
 
     if ((check_bd(dev, buf, 4) < 0)
         || (VLAN_BD_LEN(buf) < 16)) {
-        hcall_dprintf("Bad buffer enqueued in h_add_logical_lan_buffer\n");
+        hcall_dprintf("Bad buffer enqueued\n");
         return H_PARAMETER;
     }
 
diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
index dbf5a90..1f67e64 100644
--- a/hw/spapr_vio.c
+++ b/hw/spapr_vio.c
@@ -204,8 +204,7 @@ static target_ulong h_put_tce(CPUPPCState *env, sPAPREnvironment *spapr,
     VIOsPAPR_RTCE *rtce;
 
     if (!dev) {
-        hcall_dprintf("spapr_vio_put_tce on non-existent LIOBN "
-                      TARGET_FMT_lx "\n", liobn);
+        hcall_dprintf("LIOBN 0x" TARGET_FMT_lx " does not exist\n", liobn);
         return H_PARAMETER;
     }
 
@@ -217,8 +216,7 @@ static target_ulong h_put_tce(CPUPPCState *env, sPAPREnvironment *spapr,
 #endif
 
     if (ioba >= dev->rtce_window_size) {
-        hcall_dprintf("spapr_vio_put_tce on out-of-boards IOBA 0x"
-                      TARGET_FMT_lx "\n", ioba);
+        hcall_dprintf("Out-of-bounds IOBA 0x" TARGET_FMT_lx "\n", ioba);
         return H_PARAMETER;
     }
 
@@ -414,22 +412,20 @@ static target_ulong h_reg_crq(CPUPPCState *env, sPAPREnvironment *spapr,
     VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
 
     if (!dev) {
-        hcall_dprintf("h_reg_crq on non-existent unit 0x"
-                      TARGET_FMT_lx "\n", reg);
+        hcall_dprintf("Unit 0x" TARGET_FMT_lx " does not exist\n", reg);
         return H_PARAMETER;
     }
 
     /* We can't grok a queue size bigger than 256M for now */
     if (queue_len < 0x1000 || queue_len > 0x10000000) {
-        hcall_dprintf("h_reg_crq, queue size too small or too big (0x%llx)\n",
-                      (unsigned long long)queue_len);
+        hcall_dprintf("Queue size too small or too big (0x" TARGET_FMT_lx
+                      ")\n", queue_len);
         return H_PARAMETER;
     }
 
     /* Check queue alignment */
     if (queue_addr & 0xfff) {
-        hcall_dprintf("h_reg_crq, queue not aligned (0x%llx)\n",
-                      (unsigned long long)queue_addr);
+        hcall_dprintf("Queue not aligned (0x" TARGET_FMT_lx ")\n", queue_addr);
         return H_PARAMETER;
     }
 
@@ -460,8 +456,7 @@ static target_ulong h_free_crq(CPUPPCState *env, sPAPREnvironment *spapr,
     VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
 
     if (!dev) {
-        hcall_dprintf("h_free_crq on non-existent unit 0x"
-                      TARGET_FMT_lx "\n", reg);
+        hcall_dprintf("Unit 0x" TARGET_FMT_lx " does not exist\n", reg);
         return H_PARAMETER;
     }
 
@@ -484,8 +479,7 @@ static target_ulong h_send_crq(CPUPPCState *env, sPAPREnvironment *spapr,
     uint64_t crq_mangle[2];
 
     if (!dev) {
-        hcall_dprintf("h_send_crq on non-existent unit 0x"
-                      TARGET_FMT_lx "\n", reg);
+        hcall_dprintf("Unit 0x" TARGET_FMT_lx " does not exist\n", reg);
         return H_PARAMETER;
     }
     crq_mangle[0] = cpu_to_be64(msg_hi);
@@ -505,8 +499,7 @@ static target_ulong h_enable_crq(CPUPPCState *env, sPAPREnvironment *spapr,
     VIOsPAPRDevice *dev = spapr_vio_find_by_reg(spapr->vio_bus, reg);
 
     if (!dev) {
-        hcall_dprintf("h_enable_crq on non-existent unit 0x"
-                      TARGET_FMT_lx "\n", reg);
+        hcall_dprintf("Unit 0x" TARGET_FMT_lx " does not exist\n", reg);
         return H_PARAMETER;
     }
 
-- 
1.7.9.1

[Qemu-devel] [PATCH 2/3] pseries: Fix bug with reset of VIO CRQs

[David Gibson]

PAPR specifies a Command Response Queue (CRQ) mechanism used for virtual
IO, which we implement.  However, we don't correctly clean up registered
CRQs when we reset the system.

This patch adds a reset handler to fix this bug.  While we're at it, add
in some of the extra debug messages that were used to track the problem
down.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/spapr_vio.c |   33 +++++++++++++++++++++++++--------
 1 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
index 1f67e64..97d029a 100644
--- a/hw/spapr_vio.c
+++ b/hw/spapr_vio.c
@@ -431,12 +431,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;
@@ -449,6 +450,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)
 {
@@ -460,13 +472,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,
@@ -642,6 +648,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;
@@ -670,6 +685,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
 
     rtce_init(dev);
 
+    qemu_register_reset(spapr_vio_busdev_reset, dev);
+
     return pc->init(dev);
 }
 
-- 
1.7.9.1

Re: [Qemu-devel] [PATCH 2/3] pseries: Fix bug with reset of VIO CRQs

[Andreas Färber]

Am 28.03.2012 23:39, schrieb David Gibson:
> PAPR specifies a Command Response Queue (CRQ) mechanism used for virtual
> IO, which we implement.  However, we don't correctly clean up registered
> CRQs when we reset the system.
> 
> This patch adds a reset handler to fix this bug.  While we're at it, add
> in some of the extra debug messages that were used to track the problem
> down.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

As discussed on IRC, I've applied the following diff on my local branch
to drop the h_reg_crq that my __func__ comment was about:

diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
index 0bf2c31..97d029a 100644
--- a/hw/spapr_vio.c
+++ b/hw/spapr_vio.c
@@ -431,13 +431,13 @@ static target_ulong h_reg_crq(CPUPPCState *env,
sPAPREnvironment *spapr,

     /* Check if device supports CRQs */
     if (!dev->crq.SendFunc) {
-        hcall_dprintf("Device does not support CRQ\n");
+        hcall_dprintf("h_reg_crq, device does not support CRQ\n");
         return H_NOT_FOUND;
     }

     /* Already a queue ? */
     if (dev->crq.qsize) {
-        hcall_dprintf("CRQ already registered\n");
+        hcall_dprintf("h_reg_crq, CRQ already registered\n");
         return H_RESOURCE;
     }
     dev->crq.qladdr = queue_addr;

However, I'm having trouble testing reset. Whether on vanilla master or
using this patch on top of ppc-next or this whole series on top of
ppc-next, using `ppc64-softmmu/qemu-system-ppc64 -M pseries -m 1G`:

a) 0 > reset-all
results in: "reboot not available Aborted"
Do you need to update SLOF to actually use the newly added RTAS call?

b) (qemu) system_reset
results in:
 exception 700
SRR0 = 0000000000000000  SRR1 = 800000008000000000080000
SPRG2 = 0000000000000000  SPRG3 = 000000003DCD1AD4

Could you please look into the two above issues? How did you test?

Thanks,
Andreas

>  hw/spapr_vio.c |   33 +++++++++++++++++++++++++--------
>  1 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
> index 1f67e64..97d029a 100644
> --- a/hw/spapr_vio.c
> +++ b/hw/spapr_vio.c
> @@ -431,12 +431,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;
> @@ -449,6 +450,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)
>  {
> @@ -460,13 +472,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,
> @@ -642,6 +648,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;
> @@ -670,6 +685,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
>  
>      rtce_init(dev);
>  
> +    qemu_register_reset(spapr_vio_busdev_reset, dev);
> +
>      return pc->init(dev);
>  }
>  

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] pseries: Fix bug with reset of VIO CRQs

[David Gibson]

On Wed, Apr 04, 2012 at 06:13:36PM +0200, Andreas Färber wrote:
> Am 28.03.2012 23:39, schrieb David Gibson:
> > PAPR specifies a Command Response Queue (CRQ) mechanism used for virtual
> > IO, which we implement.  However, we don't correctly clean up registered
> > CRQs when we reset the system.
> > 
> > This patch adds a reset handler to fix this bug.  While we're at it, add
> > in some of the extra debug messages that were used to track the problem
> > down.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> As discussed on IRC, I've applied the following diff on my local branch
> to drop the h_reg_crq that my __func__ comment was about:
> 
> diff --git a/hw/spapr_vio.c b/hw/spapr_vio.c
> index 0bf2c31..97d029a 100644
> --- a/hw/spapr_vio.c
> +++ b/hw/spapr_vio.c
> @@ -431,13 +431,13 @@ static target_ulong h_reg_crq(CPUPPCState *env,
> sPAPREnvironment *spapr,
> 
>      /* Check if device supports CRQs */
>      if (!dev->crq.SendFunc) {
> -        hcall_dprintf("Device does not support CRQ\n");
> +        hcall_dprintf("h_reg_crq, device does not support CRQ\n");
>          return H_NOT_FOUND;
>      }
> 
>      /* Already a queue ? */
>      if (dev->crq.qsize) {
> -        hcall_dprintf("CRQ already registered\n");
> +        hcall_dprintf("h_reg_crq, CRQ already registered\n");
>          return H_RESOURCE;
>      }
>      dev->crq.qladdr = queue_addr;
> 
> However, I'm having trouble testing reset. Whether on vanilla master or
> using this patch on top of ppc-next or this whole series on top of
> ppc-next, using `ppc64-softmmu/qemu-system-ppc64 -M pseries -m 1G`:
> 
> a) 0 > reset-all
> results in: "reboot not available Aborted"
> Do you need to update SLOF to actually use the newly added RTAS call?
> 
> b) (qemu) system_reset
> results in:
>  exception 700
> SRR0 = 0000000000000000  SRR1 = 800000008000000000080000
> SPRG2 = 0000000000000000  SPRG3 = 000000003DCD1AD4
> 
> Could you please look into the two above issues? How did you test?

Ah.  I used "reboot" from within the guest Linux.  I'll look at the
others, the first could just be a slof bug.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/3] pseries: Fix bug with reset of VIO CRQs

[David Gibson]

On Wed, Apr 04, 2012 at 06:13:36PM +0200, Andreas Färber wrote:
> Am 28.03.2012 23:39, schrieb David Gibson:
[snip]
> However, I'm having trouble testing reset. Whether on vanilla master or
> using this patch on top of ppc-next or this whole series on top of
> ppc-next, using `ppc64-softmmu/qemu-system-ppc64 -M pseries -m 1G`:
> 
> a) 0 > reset-all
> results in: "reboot not available Aborted"
> Do you need to update SLOF to actually use the newly added RTAS call?

Maybe, I'll have to check that.

> b) (qemu) system_reset
> results in:
>  exception 700
> SRR0 = 0000000000000000  SRR1 = 800000008000000000080000
> SPRG2 = 0000000000000000  SPRG3 = 000000003DCD1AD4
> 
> Could you please look into the two above issues? How did you test?

Hrm.  I don't get that, at least with a fully booted kernel, although
it does fail to boot completely after the reset, which I'll debug.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[Qemu-devel] [PATCH 3/3] pseries: Implement RTAS system-reboot call

[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>
---
 hw/spapr_rtas.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 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);
-- 
1.7.9.1

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] pseries: Clean up hcall_dprintf() debugging messages

[Andreas Färber]

Am 28.03.2012 23:39, schrieb David Gibson:
> The pseries machine code has a number of debug messages for debugging PAPR
> hypercalls, dependent on DEBUG_SPAPR_HCALLS.  This patch cleans these
> messages up a bit, by adding __func__ to the hcall_dprintf() macro and
> simplifying up a number of the individual messages accordingly.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Conflicts:
> 
> 	hw/spapr_vio.c
> ---

Thanks, that cleanup went beyond what I expected for what-is-now-2/3.

Applied to ppc-next:
http://repo.or.cz/w/qemu/agraf.git/shortlog/refs/heads/ppc-next

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg