[Qemu-devel] [PATCHv3 0/9] Cleanups to error reporting on ppc and spapr

[Qemu-devel] [PATCHv3 0/9] Cleanups to error reporting on ppc and spapr

[David Gibson]

Another spin of my patches to clean up a bunch of error reporting in
the pseries machine type and target-ppc code, to better use the error
API.

Once reviewed, I hope to merge this into ppc-for-2.6 shortly.

Changes in v3:
 * Adjusted a commit message for accuracy (suggest by Markus)
 * Dropped a patch which relied on a wrong guess about the behaviour
   of foreach_dynamic_sysbus_device().
Changes in v2:
 * Assorted minor tweaks based on review

David Gibson (9):
  ppc: Cleanup error handling in ppc_set_compat()
  pseries: Cleanup error handling of spapr_cpu_init()
  pseries: Clean up hash page table allocation error handling
  pseries: Clean up error handling in spapr_validate_node_memory()
  pseries: Cleanup error handling in spapr_vga_init()
  pseries: Clean up error handling in spapr_rtas_register()
  pseries: Clean up error handling in xics_system_init()
  pseries: Clean up error reporting in ppc_spapr_init()
  pseries: Clean up error reporting in htab migration functions

 hw/ppc/spapr.c              | 134 ++++++++++++++++++++++++++------------------
 hw/ppc/spapr_hcall.c        |  10 ++--
 hw/ppc/spapr_rtas.c         |  12 +---
 target-ppc/cpu.h            |   2 +-
 target-ppc/translate_init.c |  13 +++--
 5 files changed, 94 insertions(+), 77 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCHv3 1/9] ppc: Cleanup error handling in ppc_set_compat()

[David Gibson]

Current ppc_set_compat() returns -1 for errors, and also (unconditionally)
reports an error message.  The caller in h_client_architecture_support()
may then report it again using an outdated fprintf().

Clean this up by using the modern error reporting mechanisms.  Also add
strerror(errno) to the error message.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c              |  4 +---
 hw/ppc/spapr_hcall.c        | 10 +++++-----
 target-ppc/cpu.h            |  2 +-
 target-ppc/translate_init.c | 13 +++++++------
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 50e5a26..fa7a7f4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1635,9 +1635,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
     }
 
     if (cpu->max_compat) {
-        if (ppc_set_compat(cpu, cpu->max_compat) < 0) {
-            exit(1);
-        }
+        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
     }
 
     xics_cpu_setup(spapr->icp, cpu);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index cebceea..8b0fcb3 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -837,7 +837,7 @@ static target_ulong cas_get_option_vector(int vector, target_ulong table)
 typedef struct {
     PowerPCCPU *cpu;
     uint32_t cpu_version;
-    int ret;
+    Error *err;
 } SetCompatState;
 
 static void do_set_compat(void *arg)
@@ -845,7 +845,7 @@ static void do_set_compat(void *arg)
     SetCompatState *s = arg;
 
     cpu_synchronize_state(CPU(s->cpu));
-    s->ret = ppc_set_compat(s->cpu, s->cpu_version);
+    ppc_set_compat(s->cpu, s->cpu_version, &s->err);
 }
 
 #define get_compat_level(cpuver) ( \
@@ -929,13 +929,13 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
             SetCompatState s = {
                 .cpu = POWERPC_CPU(cs),
                 .cpu_version = cpu_version,
-                .ret = 0
+                .err = NULL,
             };
 
             run_on_cpu(cs, do_set_compat, &s);
 
-            if (s.ret < 0) {
-                fprintf(stderr, "Unable to set compatibility mode\n");
+            if (s.err) {
+                error_report_err(s.err);
                 return H_HARDWARE;
             }
         }
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 9706000..b3b89e6 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1210,7 +1210,7 @@ void ppc_store_msr (CPUPPCState *env, target_ulong value);
 
 void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
 int ppc_get_compat_smt_threads(PowerPCCPU *cpu);
-int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version);
+void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp);
 
 /* Time-base and decrementer management */
 #ifndef NO_CPU_IO_DEFS
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 4ab2d92..678957a 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9186,7 +9186,7 @@ int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
     return ret;
 }
 
-int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
+void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp)
 {
     int ret = 0;
     CPUPPCState *env = &cpu->env;
@@ -9208,12 +9208,13 @@ int ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
         break;
     }
 
-    if (kvm_enabled() && kvmppc_set_compat(cpu, cpu->cpu_version) < 0) {
-        error_report("Unable to set compatibility mode in KVM");
-        ret = -1;
+    if (kvm_enabled()) {
+        ret = kvmppc_set_compat(cpu, cpu->cpu_version);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Unable to set CPU compatibility mode in KVM");
+        }
     }
-
-    return ret;
 }
 
 static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
-- 
2.5.0

[Qemu-devel] [PATCHv3 2/9] pseries: Cleanup error handling of spapr_cpu_init()

[David Gibson]

Currently spapr_cpu_init() is hardcoded to handle any errors as fatal.
That works for now, since it's only called from initial setup where an
error here means we really can't proceed.

However, we'll want to handle this more flexibly for cpu hotplug in future
so generalize this using the error reporting infrastructure.  While we're
at it make a small cleanup in a related part of ppc_spapr_init() to use
error_report() instead of an old-style explicit fprintf().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fa7a7f4..b7fd09a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1617,7 +1617,8 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
     machine->boot_order = g_strdup(boot_device);
 }
 
-static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
+static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
+                           Error **errp)
 {
     CPUPPCState *env = &cpu->env;
 
@@ -1635,7 +1636,13 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu)
     }
 
     if (cpu->max_compat) {
-        ppc_set_compat(cpu, cpu->max_compat, &error_fatal);
+        Error *local_err = NULL;
+
+        ppc_set_compat(cpu, cpu->max_compat, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
 
     xics_cpu_setup(spapr->icp, cpu);
@@ -1804,10 +1811,10 @@ static void ppc_spapr_init(MachineState *machine)
     for (i = 0; i < smp_cpus; i++) {
         cpu = cpu_ppc_init(machine->cpu_model);
         if (cpu == NULL) {
-            fprintf(stderr, "Unable to find PowerPC CPU definition\n");
+            error_report("Unable to find PowerPC CPU definition");
             exit(1);
         }
-        spapr_cpu_init(spapr, cpu);
+        spapr_cpu_init(spapr, cpu, &error_fatal);
     }
 
     if (kvm_enabled()) {
-- 
2.5.0

[Qemu-devel] [PATCHv3 3/9] pseries: Clean up hash page table allocation error handling

[David Gibson]

The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
all errors with error_setg(&error_abort, ...).

But really, the callers are really better placed to decide on the error
handling.  So, instead make the functions use the error propagation
infrastructure.

In the callers we change to &error_fatal instead of &error_abort, since
this can be triggered by a bad configuration or kernel error rather than
indicating a programming error in qemu.

While we're at it improve the messages themselves a bit, and clean up the
indentation a little.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b7fd09a..d28e349 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1016,7 +1016,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
 #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
 #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
 
-static void spapr_alloc_htab(sPAPRMachineState *spapr)
+static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp)
 {
     long shift;
     int index;
@@ -1031,7 +1031,8 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
          * For HV KVM, host kernel will return -ENOMEM when requested
          * HTAB size can't be allocated.
          */
-        error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
+        error_setg_errno(errp, -shift,
+                         "Error allocating KVM hash page table, try smaller maxmem");
     } else if (shift > 0) {
         /*
          * Kernel handles htab, we don't need to allocate one
@@ -1040,7 +1041,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
          * but we don't allow booting of such guests.
          */
         if (shift != spapr->htab_shift) {
-            error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
+            error_setg(errp,
+                "Small allocation for KVM hash page table (%ld < %"
+                PRIu32 "), try smaller maxmem",
+                shift, spapr->htab_shift);
         }
 
         spapr->htab_shift = shift;
@@ -1064,17 +1068,21 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
  * If host kernel has allocated HTAB, KVM_PPC_ALLOCATE_HTAB ioctl is
  * used to clear HTAB. Otherwise QEMU-allocated HTAB is cleared manually.
  */
-static void spapr_reset_htab(sPAPRMachineState *spapr)
+static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
 {
     long shift;
     int index;
 
     shift = kvmppc_reset_htab(spapr->htab_shift);
     if (shift < 0) {
-        error_setg(&error_abort, "Failed to reset HTAB");
+        error_setg_errno(errp, -shift,
+                   "Error resetting KVM hash page table, try smaller maxmem");
     } else if (shift > 0) {
         if (shift != spapr->htab_shift) {
-            error_setg(&error_abort, "Requested HTAB allocation failed during reset");
+            error_setg(errp,
+                "Reduced size on reset of KVM hash page table (%ld < %"
+                PRIu32 "), try smaller maxmem",
+                shift, spapr->htab_shift);
         }
 
         /* Tell readers to update their file descriptor */
@@ -1145,7 +1153,7 @@ static void ppc_spapr_reset(void)
     foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
 
     /* Reset the hash table & recalc the RMA */
-    spapr_reset_htab(spapr);
+    spapr_reset_htab(spapr, &error_fatal);
 
     qemu_devices_reset();
 
@@ -1792,7 +1800,7 @@ static void ppc_spapr_init(MachineState *machine)
         }
         spapr->htab_shift++;
     }
-    spapr_alloc_htab(spapr);
+    spapr_alloc_htab(spapr, &error_fatal);
 
     /* Set up Interrupt Controller before we create the VCPUs */
     spapr->icp = xics_system_init(machine,
-- 
2.5.0

[Qemu-devel] [PATCHv3 4/9] pseries: Clean up error handling in spapr_validate_node_memory()

[David Gibson]

Use error_setg() and return an error, rather than using an explicit exit().

Also improve messages, and be more explicit about which constraint failed.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d28e349..87097bc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1699,27 +1699,34 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
  * to SPAPR_MEMORY_BLOCK_SIZE(256MB), then refuse to start the guest
  * since we can't support such unaligned sizes with DRCONF_MEMORY.
  */
-static void spapr_validate_node_memory(MachineState *machine)
+static void spapr_validate_node_memory(MachineState *machine, Error **errp)
 {
     int i;
 
-    if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE ||
-        machine->ram_size % SPAPR_MEMORY_BLOCK_SIZE) {
-        error_report("Can't support memory configuration where RAM size "
-                     "0x" RAM_ADDR_FMT " or maxmem size "
-                     "0x" RAM_ADDR_FMT " isn't aligned to %llu MB",
-                     machine->ram_size, machine->maxram_size,
-                     SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
-        exit(EXIT_FAILURE);
+    if (machine->ram_size % SPAPR_MEMORY_BLOCK_SIZE) {
+        error_setg(errp, "Memory size 0x" RAM_ADDR_FMT
+                   " is not aligned to %llu MiB",
+                   machine->ram_size,
+                   SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
+        return;
+    }
+
+    if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE) {
+        error_setg(errp, "Maximum memory size 0x" RAM_ADDR_FMT
+                   " is not aligned to %llu MiB",
+                   machine->ram_size,
+                   SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
+        return;
     }
 
     for (i = 0; i < nb_numa_nodes; i++) {
         if (numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) {
-            error_report("Can't support memory configuration where memory size"
-                         " %" PRIx64 " of node %d isn't aligned to %llu MB",
-                         numa_info[i].node_mem, i,
-                         SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
-            exit(EXIT_FAILURE);
+            error_setg(errp,
+                       "Node %d memory size 0x" RAM_ADDR_FMT
+                       " is not aligned to %llu MiB",
+                       i, numa_info[i].node_mem,
+                       SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
+            return;
         }
     }
 }
@@ -1809,7 +1816,7 @@ static void ppc_spapr_init(MachineState *machine)
                                   XICS_IRQS);
 
     if (smc->dr_lmb_enabled) {
-        spapr_validate_node_memory(machine);
+        spapr_validate_node_memory(machine, &error_fatal);
     }
 
     /* init CPUs */
-- 
2.5.0

[Qemu-devel] [PATCHv3 5/9] pseries: Cleanup error handling in spapr_vga_init()

[David Gibson]

Use error_setg() to return an error rather than an explicit exit().
Previously it was an exit(0) instead of a non-zero exit code, which was
simply a bug.  Also improve the error message.

While we're at it change the type of spapr_vga_init() to bool since that's
how we're using it anyway.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 87097bc..bb5eaa5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1246,7 +1246,7 @@ static void spapr_rtc_create(sPAPRMachineState *spapr)
 }
 
 /* Returns whether we want to use VGA or not */
-static int spapr_vga_init(PCIBus *pci_bus)
+static bool spapr_vga_init(PCIBus *pci_bus, Error **errp)
 {
     switch (vga_interface_type) {
     case VGA_NONE:
@@ -1257,9 +1257,9 @@ static int spapr_vga_init(PCIBus *pci_bus)
     case VGA_VIRTIO:
         return pci_vga_init(pci_bus) != NULL;
     default:
-        fprintf(stderr, "This vga model is not supported,"
-                "currently it only supports -vga std\n");
-        exit(0);
+        error_setg(errp,
+                   "Unsupported VGA mode, only -vga std or -vga virtio is supported");
+        return false;
     }
 }
 
@@ -1934,7 +1934,7 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     /* Graphics */
-    if (spapr_vga_init(phb->bus)) {
+    if (spapr_vga_init(phb->bus, &error_fatal)) {
         spapr->has_graphics = true;
         machine->usb |= defaults_enabled() && !machine->usb_disabled;
     }
-- 
2.5.0

[Qemu-devel] [PATCHv3 6/9] pseries: Clean up error handling in spapr_rtas_register()

[David Gibson]

The errors detected in this function necessarily indicate bugs in the rest
of the qemu code, rather than an external or configuration problem.

So, a simple assert() is more appropriate than any more complex error
reporting.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_rtas.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 34b12a3..0be52ae 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -648,17 +648,11 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 
 void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
 {
-    if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) {
-        fprintf(stderr, "RTAS invalid token 0x%x\n", token);
-        exit(1);
-    }
+    assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX));
 
     token -= RTAS_TOKEN_BASE;
-    if (rtas_table[token].name) {
-        fprintf(stderr, "RTAS call \"%s\" is registered already as 0x%x\n",
-                rtas_table[token].name, token);
-        exit(1);
-    }
+
+    assert(!rtas_table[token].name);
 
     rtas_table[token].name = name;
     rtas_table[token].fn = fn;
-- 
2.5.0

[Qemu-devel] [PATCHv3 7/9] pseries: Clean up error handling in xics_system_init()

[David Gibson]

Use the error handling infrastructure to pass an error out from
try_create_xics() instead of assuming &error_abort - the caller is in a
better position to decide on error handling policy.

Also change the error handling from an &error_abort to &error_fatal, since
this occurs during the initial machine construction and could be triggered
by bad configuration rather than a program error.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bb5eaa5..148ca5a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -111,7 +111,7 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
 }
 
 static XICSState *xics_system_init(MachineState *machine,
-                                   int nr_servers, int nr_irqs)
+                                   int nr_servers, int nr_irqs, Error **errp)
 {
     XICSState *icp = NULL;
 
@@ -130,7 +130,7 @@ static XICSState *xics_system_init(MachineState *machine,
     }
 
     if (!icp) {
-        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, &error_abort);
+        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, errp);
     }
 
     return icp;
@@ -1813,7 +1813,7 @@ static void ppc_spapr_init(MachineState *machine)
     spapr->icp = xics_system_init(machine,
                                   DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
                                                smp_threads),
-                                  XICS_IRQS);
+                                  XICS_IRQS, &error_fatal);
 
     if (smc->dr_lmb_enabled) {
         spapr_validate_node_memory(machine, &error_fatal);
-- 
2.5.0

[Qemu-devel] [PATCHv3 8/9] pseries: Clean up error reporting in ppc_spapr_init()

[David Gibson]

This function includes a number of explicit fprintf()s for errors.
Change these to use error_report() instead.

Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
the latter is the more usual idiom in qemu by a large margin.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 148ca5a..58f26cd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1789,8 +1789,8 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     if (spapr->rma_size > node0_size) {
-        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
-                spapr->rma_size);
+        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
+                     spapr->rma_size);
         exit(1);
     }
 
@@ -1856,10 +1856,10 @@ static void ppc_spapr_init(MachineState *machine)
         ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
 
         if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
-            error_report("Specified number of memory slots %" PRIu64
-                         " exceeds max supported %d",
-                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
-            exit(EXIT_FAILURE);
+            error_report("Specified number of memory slots %"
+                         PRIu64" exceeds max supported %d",
+                machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
+            exit(1);
         }
 
         spapr->hotplug_memory.base = ROUND_UP(machine->ram_size,
@@ -1955,8 +1955,9 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     if (spapr->rma_size < (MIN_RMA_SLOF << 20)) {
-        fprintf(stderr, "qemu: pSeries SLOF firmware requires >= "
-                "%ldM guest RMA (Real Mode Area memory)\n", MIN_RMA_SLOF);
+        error_report(
+            "pSeries SLOF firmware requires >= %ldM guest RMA (Real Mode Area memory)",
+            MIN_RMA_SLOF);
         exit(1);
     }
 
@@ -1972,8 +1973,8 @@ static void ppc_spapr_init(MachineState *machine)
             kernel_le = kernel_size > 0;
         }
         if (kernel_size < 0) {
-            fprintf(stderr, "qemu: error loading %s: %s\n",
-                    kernel_filename, load_elf_strerror(kernel_size));
+            error_report("error loading %s: %s",
+                         kernel_filename, load_elf_strerror(kernel_size));
             exit(1);
         }
 
@@ -1986,8 +1987,8 @@ static void ppc_spapr_init(MachineState *machine)
             initrd_size = load_image_targphys(initrd_filename, initrd_base,
                                               load_limit - initrd_base);
             if (initrd_size < 0) {
-                fprintf(stderr, "qemu: could not load initial ram disk '%s'\n",
-                        initrd_filename);
+                error_report("could not load initial ram disk '%s'",
+                             initrd_filename);
                 exit(1);
             }
         } else {
-- 
2.5.0

[Qemu-devel] [PATCHv3 9/9] pseries: Clean up error reporting in htab migration functions

[David Gibson]

The functions for migrating the hash page table on pseries machine type
(htab_save_setup() and htab_load()) can report some errors with an
explicit fprintf() before returning an appropriate error code.  Change these
to use error_report() instead.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 58f26cd..ba0bfdf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1317,8 +1317,9 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
         spapr->htab_fd = kvmppc_get_htab_fd(false);
         spapr->htab_fd_stale = false;
         if (spapr->htab_fd < 0) {
-            fprintf(stderr, "Unable to open fd for reading hash table from KVM: %s\n",
-                    strerror(errno));
+            error_report(
+                "Unable to open fd for reading hash table from KVM: %s",
+                strerror(errno));
             return -1;
         }
     }
@@ -1534,7 +1535,7 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
     int fd = -1;
 
     if (version_id < 1 || version_id > 1) {
-        fprintf(stderr, "htab_load() bad version\n");
+        error_report("htab_load() bad version");
         return -EINVAL;
     }
 
@@ -1555,8 +1556,8 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
 
         fd = kvmppc_get_htab_fd(true);
         if (fd < 0) {
-            fprintf(stderr, "Unable to open fd to restore KVM hash table: %s\n",
-                    strerror(errno));
+            error_report("Unable to open fd to restore KVM hash table: %s",
+                         strerror(errno));
         }
     }
 
@@ -1576,9 +1577,9 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
         if ((index + n_valid + n_invalid) >
             (HTAB_SIZE(spapr) / HASH_PTE_SIZE_64)) {
             /* Bad index in stream */
-            fprintf(stderr, "htab_load() bad index %d (%hd+%hd entries) "
-                    "in htab stream (htab_shift=%d)\n", index, n_valid, n_invalid,
-                    spapr->htab_shift);
+            error_report(
+                "htab_load() bad index %d (%hd+%hd entries) in htab stream (htab_shift=%d)",
+                index, n_valid, n_invalid, spapr->htab_shift);
             return -EINVAL;
         }
 
-- 
2.5.0

Re: [Qemu-devel] [PATCHv3 3/9] pseries: Clean up hash page table allocation error handling

[Thomas Huth]

On 18.01.2016 05:24, David Gibson wrote:
> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> all errors with error_setg(&error_abort, ...).
> 
> But really, the callers are really better placed to decide on the error
> handling.  So, instead make the functions use the error propagation
> infrastructure.
> 
> In the callers we change to &error_fatal instead of &error_abort, since
> this can be triggered by a bad configuration or kernel error rather than
> indicating a programming error in qemu.
> 
> While we're at it improve the messages themselves a bit, and clean up the
> indentation a little.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7fd09a..d28e349 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1016,7 +1016,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
>  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
>  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>  
> -static void spapr_alloc_htab(sPAPRMachineState *spapr)
> +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp)
>  {
>      long shift;
>      int index;
> @@ -1031,7 +1031,8 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>           * For HV KVM, host kernel will return -ENOMEM when requested
>           * HTAB size can't be allocated.
>           */
> -        error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> +        error_setg_errno(errp, -shift,
> +                         "Error allocating KVM hash page table, try smaller maxmem");
>      } else if (shift > 0) {
>          /*
>           * Kernel handles htab, we don't need to allocate one
> @@ -1040,7 +1041,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>           * but we don't allow booting of such guests.
>           */
>          if (shift != spapr->htab_shift) {
> -            error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> +            error_setg(errp,
> +                "Small allocation for KVM hash page table (%ld < %"
> +                PRIu32 "), try smaller maxmem",
> +                shift, spapr->htab_shift);

Maybe you should add an "return" statement here - theoretically you do
not want to continue with "kvmppc_kern_htab = true" in case of errors.
(practically this does not happen because errp = error_fatal, but in
case the caller gets changed, this might introduce subtle errors otherwise)

>          }
>  
>          spapr->htab_shift = shift;
> @@ -1064,17 +1068,21 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>   * If host kernel has allocated HTAB, KVM_PPC_ALLOCATE_HTAB ioctl is
>   * used to clear HTAB. Otherwise QEMU-allocated HTAB is cleared manually.
>   */
> -static void spapr_reset_htab(sPAPRMachineState *spapr)
> +static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
>  {
>      long shift;
>      int index;
>  
>      shift = kvmppc_reset_htab(spapr->htab_shift);
>      if (shift < 0) {
> -        error_setg(&error_abort, "Failed to reset HTAB");
> +        error_setg_errno(errp, -shift,
> +                   "Error resetting KVM hash page table, try smaller maxmem");

dito, better do an "return" here...

>      } else if (shift > 0) {
>          if (shift != spapr->htab_shift) {
> -            error_setg(&error_abort, "Requested HTAB allocation failed during reset");
> +            error_setg(errp,
> +                "Reduced size on reset of KVM hash page table (%ld < %"
> +                PRIu32 "), try smaller maxmem",
> +                shift, spapr->htab_shift);

... and here.

>          }
>  
>          /* Tell readers to update their file descriptor */
> @@ -1145,7 +1153,7 @@ static void ppc_spapr_reset(void)
>      foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL);
>  
>      /* Reset the hash table & recalc the RMA */
> -    spapr_reset_htab(spapr);
> +    spapr_reset_htab(spapr, &error_fatal);
>  
>      qemu_devices_reset();
>  
> @@ -1792,7 +1800,7 @@ static void ppc_spapr_init(MachineState *machine)
>          }
>          spapr->htab_shift++;
>      }
> -    spapr_alloc_htab(spapr);
> +    spapr_alloc_htab(spapr, &error_fatal);
>  
>      /* Set up Interrupt Controller before we create the VCPUs */
>      spapr->icp = xics_system_init(machine,
> 

 Thomas

Re: [Qemu-devel] [PATCHv3 4/9] pseries: Clean up error handling in spapr_validate_node_memory()

[Thomas Huth]

On 18.01.2016 05:24, David Gibson wrote:
> Use error_setg() and return an error, rather than using an explicit exit().
> 
> Also improve messages, and be more explicit about which constraint failed.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c | 37 ++++++++++++++++++++++---------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d28e349..87097bc 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1699,27 +1699,34 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>   * to SPAPR_MEMORY_BLOCK_SIZE(256MB), then refuse to start the guest
>   * since we can't support such unaligned sizes with DRCONF_MEMORY.
>   */
> -static void spapr_validate_node_memory(MachineState *machine)
> +static void spapr_validate_node_memory(MachineState *machine, Error **errp)
>  {
>      int i;
>  
> -    if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE ||
> -        machine->ram_size % SPAPR_MEMORY_BLOCK_SIZE) {
> -        error_report("Can't support memory configuration where RAM size "
> -                     "0x" RAM_ADDR_FMT " or maxmem size "
> -                     "0x" RAM_ADDR_FMT " isn't aligned to %llu MB",
> -                     machine->ram_size, machine->maxram_size,
> -                     SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
> -        exit(EXIT_FAILURE);
> +    if (machine->ram_size % SPAPR_MEMORY_BLOCK_SIZE) {
> +        error_setg(errp, "Memory size 0x" RAM_ADDR_FMT
> +                   " is not aligned to %llu MiB",
> +                   machine->ram_size,
> +                   SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> +        return;
> +    }
> +
> +    if (machine->maxram_size % SPAPR_MEMORY_BLOCK_SIZE) {
> +        error_setg(errp, "Maximum memory size 0x" RAM_ADDR_FMT
> +                   " is not aligned to %llu MiB",
> +                   machine->ram_size,
> +                   SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> +        return;
>      }
>  
>      for (i = 0; i < nb_numa_nodes; i++) {
>          if (numa_info[i].node_mem % SPAPR_MEMORY_BLOCK_SIZE) {
> -            error_report("Can't support memory configuration where memory size"
> -                         " %" PRIx64 " of node %d isn't aligned to %llu MB",
> -                         numa_info[i].node_mem, i,
> -                         SPAPR_MEMORY_BLOCK_SIZE/M_BYTE);
> -            exit(EXIT_FAILURE);
> +            error_setg(errp,
> +                       "Node %d memory size 0x" RAM_ADDR_FMT
> +                       " is not aligned to %llu MiB",
> +                       i, numa_info[i].node_mem,
> +                       SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
> +            return;
>          }
>      }
>  }
> @@ -1809,7 +1816,7 @@ static void ppc_spapr_init(MachineState *machine)
>                                    XICS_IRQS);
>  
>      if (smc->dr_lmb_enabled) {
> -        spapr_validate_node_memory(machine);
> +        spapr_validate_node_memory(machine, &error_fatal);
>      }
>  
>      /* init CPUs */
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCHv3 5/9] pseries: Cleanup error handling in spapr_vga_init()

[Thomas Huth]

On 18.01.2016 05:24, David Gibson wrote:
> Use error_setg() to return an error rather than an explicit exit().
> Previously it was an exit(0) instead of a non-zero exit code, which was
> simply a bug.  Also improve the error message.
> 
> While we're at it change the type of spapr_vga_init() to bool since that's
> how we're using it anyway.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 87097bc..bb5eaa5 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1246,7 +1246,7 @@ static void spapr_rtc_create(sPAPRMachineState *spapr)
>  }
>  
>  /* Returns whether we want to use VGA or not */
> -static int spapr_vga_init(PCIBus *pci_bus)
> +static bool spapr_vga_init(PCIBus *pci_bus, Error **errp)
>  {
>      switch (vga_interface_type) {
>      case VGA_NONE:
> @@ -1257,9 +1257,9 @@ static int spapr_vga_init(PCIBus *pci_bus)
>      case VGA_VIRTIO:
>          return pci_vga_init(pci_bus) != NULL;
>      default:
> -        fprintf(stderr, "This vga model is not supported,"
> -                "currently it only supports -vga std\n");
> -        exit(0);
> +        error_setg(errp,
> +                   "Unsupported VGA mode, only -vga std or -vga virtio is supported");
> +        return false;
>      }
>  }
>  
> @@ -1934,7 +1934,7 @@ static void ppc_spapr_init(MachineState *machine)
>      }
>  
>      /* Graphics */
> -    if (spapr_vga_init(phb->bus)) {
> +    if (spapr_vga_init(phb->bus, &error_fatal)) {
>          spapr->has_graphics = true;
>          machine->usb |= defaults_enabled() && !machine->usb_disabled;
>      }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCHv3 6/9] pseries: Clean up error handling in spapr_rtas_register()

[Thomas Huth]

On 18.01.2016 05:24, David Gibson wrote:
> The errors detected in this function necessarily indicate bugs in the rest
> of the qemu code, rather than an external or configuration problem.
> 
> So, a simple assert() is more appropriate than any more complex error
> reporting.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_rtas.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 34b12a3..0be52ae 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -648,17 +648,11 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  
>  void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
>  {
> -    if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) {
> -        fprintf(stderr, "RTAS invalid token 0x%x\n", token);
> -        exit(1);
> -    }
> +    assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX));

While you're at it, you could also get rid of some superfluous
parentheses in that statement:

	assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);

>      token -= RTAS_TOKEN_BASE;
> -    if (rtas_table[token].name) {
> -        fprintf(stderr, "RTAS call \"%s\" is registered already as 0x%x\n",
> -                rtas_table[token].name, token);
> -        exit(1);
> -    }
> +
> +    assert(!rtas_table[token].name);
>  
>      rtas_table[token].name = name;
>      rtas_table[token].fn = fn;
> 

Anyway, patch sounds reasonable,
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCHv3 7/9] pseries: Clean up error handling in xics_system_init()

[Thomas Huth]

On 18.01.2016 05:24, David Gibson wrote:
> Use the error handling infrastructure to pass an error out from
> try_create_xics() instead of assuming &error_abort - the caller is in a
> better position to decide on error handling policy.
> 
> Also change the error handling from an &error_abort to &error_fatal, since
> this occurs during the initial machine construction and could be triggered
> by bad configuration rather than a program error.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bb5eaa5..148ca5a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -111,7 +111,7 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
>  }
>  
>  static XICSState *xics_system_init(MachineState *machine,
> -                                   int nr_servers, int nr_irqs)
> +                                   int nr_servers, int nr_irqs, Error **errp)
>  {
>      XICSState *icp = NULL;
>  
> @@ -130,7 +130,7 @@ static XICSState *xics_system_init(MachineState *machine,
>      }
>  
>      if (!icp) {
> -        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, &error_abort);
> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs, errp);
>      }
>  
>      return icp;
> @@ -1813,7 +1813,7 @@ static void ppc_spapr_init(MachineState *machine)
>      spapr->icp = xics_system_init(machine,
>                                    DIV_ROUND_UP(max_cpus * kvmppc_smt_threads(),
>                                                 smp_threads),
> -                                  XICS_IRQS);
> +                                  XICS_IRQS, &error_fatal);
>  
>      if (smc->dr_lmb_enabled) {
>          spapr_validate_node_memory(machine, &error_fatal);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCHv3 8/9] pseries: Clean up error reporting in ppc_spapr_init()

[Thomas Huth]

On 18.01.2016 05:24, David Gibson wrote:
> This function includes a number of explicit fprintf()s for errors.
> Change these to use error_report() instead.
> 
> Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
> the latter is the more usual idiom in qemu by a large margin.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 148ca5a..58f26cd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1789,8 +1789,8 @@ static void ppc_spapr_init(MachineState *machine)
>      }
>  
>      if (spapr->rma_size > node0_size) {
> -        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
> -                spapr->rma_size);
> +        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> +                     spapr->rma_size);
>          exit(1);
>      }
>  
> @@ -1856,10 +1856,10 @@ static void ppc_spapr_init(MachineState *machine)
>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
>  
>          if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> -            error_report("Specified number of memory slots %" PRIu64
> -                         " exceeds max supported %d",
> -                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
> -            exit(EXIT_FAILURE);
> +            error_report("Specified number of memory slots %"
> +                         PRIu64" exceeds max supported %d",
> +                machine->ram_slots, SPAPR_MAX_RAM_SLOTS);

Why did you change the indentation of the "machine->ram_slots, ..." line
here? The original looked better to me.

> +            exit(1);

EXIT_FAILURE still seems to be used quite often in the QEMU sources...
All in all, this hunk does not really change anything from a functional
point of view, so I'd like to suggest to omit this hunk completely
instead to avoid code churn here.

 Thomas

Re: [Qemu-devel] [PATCHv3 8/9] pseries: Clean up error reporting in ppc_spapr_init()

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 18.01.2016 05:24, David Gibson wrote:
>> This function includes a number of explicit fprintf()s for errors.
>> Change these to use error_report() instead.
>> 
>> Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
>> the latter is the more usual idiom in qemu by a large margin.
>> 
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  hw/ppc/spapr.c | 25 +++++++++++++------------
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 148ca5a..58f26cd 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1789,8 +1789,8 @@ static void ppc_spapr_init(MachineState *machine)
>>      }
>>  
>>      if (spapr->rma_size > node0_size) {
>> -        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
>> -                spapr->rma_size);
>> +        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
>> +                     spapr->rma_size);
>>          exit(1);
>>      }
>>  
>> @@ -1856,10 +1856,10 @@ static void ppc_spapr_init(MachineState *machine)
>>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
>>  
>>          if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
>> -            error_report("Specified number of memory slots %" PRIu64
>> -                         " exceeds max supported %d",
>> -                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
>> -            exit(EXIT_FAILURE);
>> +            error_report("Specified number of memory slots %"
>> +                         PRIu64" exceeds max supported %d",
>> +                machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
>
> Why did you change the indentation of the "machine->ram_slots, ..." line
> here? The original looked better to me.

Agreed.

>> +            exit(1);
>
> EXIT_FAILURE still seems to be used quite often in the QEMU sources...
> All in all, this hunk does not really change anything from a functional
> point of view, so I'd like to suggest to omit this hunk completely
> instead to avoid code churn here.

It makes the code locally consistent, so I'd keep it.

Re: [Qemu-devel] [PATCHv3 3/9] pseries: Clean up hash page table allocation error handling

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 18.01.2016 05:24, David Gibson wrote:
>> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
>> all errors with error_setg(&error_abort, ...).
>> 
>> But really, the callers are really better placed to decide on the error
>> handling.  So, instead make the functions use the error propagation
>> infrastructure.
>> 
>> In the callers we change to &error_fatal instead of &error_abort, since
>> this can be triggered by a bad configuration or kernel error rather than
>> indicating a programming error in qemu.
>> 
>> While we're at it improve the messages themselves a bit, and clean up the
>> indentation a little.
>> 
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  hw/ppc/spapr.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index b7fd09a..d28e349 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1016,7 +1016,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
>>  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
>>  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
>>  
>> -static void spapr_alloc_htab(sPAPRMachineState *spapr)
>> +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp)
>>  {
>>      long shift;
>>      int index;
>> @@ -1031,7 +1031,8 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>>           * For HV KVM, host kernel will return -ENOMEM when requested
>>           * HTAB size can't be allocated.
>>           */
>> -        error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
>> +        error_setg_errno(errp, -shift,
>> +                         "Error allocating KVM hash page table, try smaller maxmem");
>>      } else if (shift > 0) {
>>          /*
>>           * Kernel handles htab, we don't need to allocate one
>> @@ -1040,7 +1041,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
>>           * but we don't allow booting of such guests.
>>           */
>>          if (shift != spapr->htab_shift) {
>> -            error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
>> +            error_setg(errp,
>> +                "Small allocation for KVM hash page table (%ld < %"
>> +                PRIu32 "), try smaller maxmem",
>> +                shift, spapr->htab_shift);
>
> Maybe you should add an "return" statement here - theoretically you do
> not want to continue with "kvmppc_kern_htab = true" in case of errors.
> (practically this does not happen because errp = error_fatal, but in
> case the caller gets changed, this might introduce subtle errors otherwise)

Good point.

With abort() / exit(), we don't have to worry about recovery.  In
particular, we don't have to revert half-done changes.

Conversions away from abort() / exit() need to consider error recovery.
We have to make sure the function leaves things in a sane state on
error.  This normally means taking an early return, and often means
reverting some state changes.

[...]

Re: [Qemu-devel] [PATCHv3 3/9] pseries: Clean up hash page table allocation error handling

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4989 bytes --]

On Mon, Jan 18, 2016 at 09:47:59AM +0100, Thomas Huth wrote:
> On 18.01.2016 05:24, David Gibson wrote:
> > The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> > all errors with error_setg(&error_abort, ...).
> > 
> > But really, the callers are really better placed to decide on the error
> > handling.  So, instead make the functions use the error propagation
> > infrastructure.
> > 
> > In the callers we change to &error_fatal instead of &error_abort, since
> > this can be triggered by a bad configuration or kernel error rather than
> > indicating a programming error in qemu.
> > 
> > While we're at it improve the messages themselves a bit, and clean up the
> > indentation a little.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b7fd09a..d28e349 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1016,7 +1016,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
> >  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> >  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> >  
> > -static void spapr_alloc_htab(sPAPRMachineState *spapr)
> > +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp)
> >  {
> >      long shift;
> >      int index;
> > @@ -1031,7 +1031,8 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >           * For HV KVM, host kernel will return -ENOMEM when requested
> >           * HTAB size can't be allocated.
> >           */
> > -        error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> > +        error_setg_errno(errp, -shift,
> > +                         "Error allocating KVM hash page table, try smaller maxmem");
> >      } else if (shift > 0) {
> >          /*
> >           * Kernel handles htab, we don't need to allocate one
> > @@ -1040,7 +1041,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >           * but we don't allow booting of such guests.
> >           */
> >          if (shift != spapr->htab_shift) {
> > -            error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> > +            error_setg(errp,
> > +                "Small allocation for KVM hash page table (%ld < %"
> > +                PRIu32 "), try smaller maxmem",
> > +                shift, spapr->htab_shift);
> 
> Maybe you should add an "return" statement here - theoretically you do
> not want to continue with "kvmppc_kern_htab = true" in case of errors.
> (practically this does not happen because errp = error_fatal, but in
> case the caller gets changed, this might introduce subtle errors
> otherwise)

No, actually.  If the error is non-fatal, then we *must* set
kvmppc_kern_htab = true.  It is possible we can continue without the
size of hash table we wanted - we did so until pretty recently.  But
it *is* still a kernel provided hash table, and must be marked as such
to operate correctly.

> 
> >          }
> >  
> >          spapr->htab_shift = shift;
> > @@ -1064,17 +1068,21 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >   * If host kernel has allocated HTAB, KVM_PPC_ALLOCATE_HTAB ioctl is
> >   * used to clear HTAB. Otherwise QEMU-allocated HTAB is cleared manually.
> >   */
> > -static void spapr_reset_htab(sPAPRMachineState *spapr)
> > +static void spapr_reset_htab(sPAPRMachineState *spapr, Error **errp)
> >  {
> >      long shift;
> >      int index;
> >  
> >      shift = kvmppc_reset_htab(spapr->htab_shift);
> >      if (shift < 0) {
> > -        error_setg(&error_abort, "Failed to reset HTAB");
> > +        error_setg_errno(errp, -shift,
> > +                   "Error resetting KVM hash page table, try smaller maxmem");
> 
> dito, better do an "return" here...

No.  The remaining statement in the function could be relevant if
we're somehow able to keep going here.

> >      } else if (shift > 0) {
> >          if (shift != spapr->htab_shift) {
> > -            error_setg(&error_abort, "Requested HTAB allocation failed during reset");
> > +            error_setg(errp,
> > +                "Reduced size on reset of KVM hash page table (%ld < %"
> > +                PRIu32 "), try smaller maxmem",
> > +                shift, spapr->htab_shift);
> 
> ... and here.

Hrm.. here, yes we would be in trouble, but 'return' wouldn't help in
the slightest.  Instead we'd need to change spapr->htab_shift to have
any hope of continuing.

I'll make that change.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv3 6/9] pseries: Clean up error handling in spapr_rtas_register()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]

On Mon, Jan 18, 2016 at 10:20:24AM +0100, Thomas Huth wrote:
> On 18.01.2016 05:24, David Gibson wrote:
> > The errors detected in this function necessarily indicate bugs in the rest
> > of the qemu code, rather than an external or configuration problem.
> > 
> > So, a simple assert() is more appropriate than any more complex error
> > reporting.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_rtas.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 34b12a3..0be52ae 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -648,17 +648,11 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >  
> >  void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn)
> >  {
> > -    if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) {
> > -        fprintf(stderr, "RTAS invalid token 0x%x\n", token);
> > -        exit(1);
> > -    }
> > +    assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX));
> 
> While you're at it, you could also get rid of some superfluous
> parentheses in that statement:
> 
> 	assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);

I could, but I won't because I think it's clearer as it is.

> >      token -= RTAS_TOKEN_BASE;
> > -    if (rtas_table[token].name) {
> > -        fprintf(stderr, "RTAS call \"%s\" is registered already as 0x%x\n",
> > -                rtas_table[token].name, token);
> > -        exit(1);
> > -    }
> > +
> > +    assert(!rtas_table[token].name);
> >  
> >      rtas_table[token].name = name;
> >      rtas_table[token].fn = fn;
> > 
> 
> Anyway, patch sounds reasonable,
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv3 3/9] pseries: Clean up hash page table allocation error handling

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3954 bytes --]

On Mon, Jan 18, 2016 at 11:21:08AM +0100, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
> > On 18.01.2016 05:24, David Gibson wrote:
> >> The spapr_alloc_htab() and spapr_reset_htab() functions currently handle
> >> all errors with error_setg(&error_abort, ...).
> >> 
> >> But really, the callers are really better placed to decide on the error
> >> handling.  So, instead make the functions use the error propagation
> >> infrastructure.
> >> 
> >> In the callers we change to &error_fatal instead of &error_abort, since
> >> this can be triggered by a bad configuration or kernel error rather than
> >> indicating a programming error in qemu.
> >> 
> >> While we're at it improve the messages themselves a bit, and clean up the
> >> indentation a little.
> >> 
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >>  hw/ppc/spapr.c | 24 ++++++++++++++++--------
> >>  1 file changed, 16 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index b7fd09a..d28e349 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1016,7 +1016,7 @@ static void emulate_spapr_hypercall(PowerPCCPU *cpu)
> >>  #define CLEAN_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) &= tswap64(~HPTE64_V_HPTE_DIRTY))
> >>  #define DIRTY_HPTE(_hpte)  ((*(uint64_t *)(_hpte)) |= tswap64(HPTE64_V_HPTE_DIRTY))
> >>  
> >> -static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >> +static void spapr_alloc_htab(sPAPRMachineState *spapr, Error **errp)
> >>  {
> >>      long shift;
> >>      int index;
> >> @@ -1031,7 +1031,8 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >>           * For HV KVM, host kernel will return -ENOMEM when requested
> >>           * HTAB size can't be allocated.
> >>           */
> >> -        error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> >> +        error_setg_errno(errp, -shift,
> >> +                         "Error allocating KVM hash page table, try smaller maxmem");
> >>      } else if (shift > 0) {
> >>          /*
> >>           * Kernel handles htab, we don't need to allocate one
> >> @@ -1040,7 +1041,10 @@ static void spapr_alloc_htab(sPAPRMachineState *spapr)
> >>           * but we don't allow booting of such guests.
> >>           */
> >>          if (shift != spapr->htab_shift) {
> >> -            error_setg(&error_abort, "Failed to allocate HTAB of requested size, try with smaller maxmem");
> >> +            error_setg(errp,
> >> +                "Small allocation for KVM hash page table (%ld < %"
> >> +                PRIu32 "), try smaller maxmem",
> >> +                shift, spapr->htab_shift);
> >
> > Maybe you should add an "return" statement here - theoretically you do
> > not want to continue with "kvmppc_kern_htab = true" in case of errors.
> > (practically this does not happen because errp = error_fatal, but in
> > case the caller gets changed, this might introduce subtle errors otherwise)
> 
> Good point.
> 
> With abort() / exit(), we don't have to worry about recovery.  In
> particular, we don't have to revert half-done changes.
> 
> Conversions away from abort() / exit() need to consider error recovery.
> We have to make sure the function leaves things in a sane state on
> error.  This normally means taking an early return, and often means
> reverting some state changes.

That's true, but Thomas is mistaken about what error recovery is
needed here.

However, I'm going to drop this patch from the series anyway - I've
realised I need to rework the htab allocation substantially for other
reasons, so it would be better to not have that conflict with this
series.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCHv3 8/9] pseries: Clean up error reporting in ppc_spapr_init()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2564 bytes --]

On Mon, Jan 18, 2016 at 10:31:42AM +0100, Thomas Huth wrote:
> On 18.01.2016 05:24, David Gibson wrote:
> > This function includes a number of explicit fprintf()s for errors.
> > Change these to use error_report() instead.
> > 
> > Also replace the single exit(EXIT_FAILURE) with an explicit exit(1), since
> > the latter is the more usual idiom in qemu by a large margin.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr.c | 25 +++++++++++++------------
> >  1 file changed, 13 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 148ca5a..58f26cd 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1789,8 +1789,8 @@ static void ppc_spapr_init(MachineState *machine)
> >      }
> >  
> >      if (spapr->rma_size > node0_size) {
> > -        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
> > -                spapr->rma_size);
> > +        error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")",
> > +                     spapr->rma_size);
> >          exit(1);
> >      }
> >  
> > @@ -1856,10 +1856,10 @@ static void ppc_spapr_init(MachineState *machine)
> >          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> >  
> >          if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> > -            error_report("Specified number of memory slots %" PRIu64
> > -                         " exceeds max supported %d",
> > -                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
> > -            exit(EXIT_FAILURE);
> > +            error_report("Specified number of memory slots %"
> > +                         PRIu64" exceeds max supported %d",
> > +                machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
> 
> Why did you change the indentation of the "machine->ram_slots, ..." line
> here? The original looked better to me.

I don't know.  Probably just a mistake on my part, I'll fix it.

> > +            exit(1);
> 
> EXIT_FAILURE still seems to be used quite often in the QEMU sources...
> All in all, this hunk does not really change anything from a functional
> point of view, so I'd like to suggest to omit this hunk completely
> instead to avoid code churn here.

This I'll keep, as Markus says for local consistency.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]