From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34913) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aL5Tu-0001vp-UN for qemu-devel@nongnu.org; Mon, 18 Jan 2016 03:48:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aL5Tr-00043H-NF for qemu-devel@nongnu.org; Mon, 18 Jan 2016 03:48:10 -0500 References: <1453091083-13931-1-git-send-email-david@gibson.dropbear.id.au> <1453091083-13931-4-git-send-email-david@gibson.dropbear.id.au> From: Thomas Huth Message-ID: <569CA6BF.9060606@redhat.com> Date: Mon, 18 Jan 2016 09:47:59 +0100 MIME-Version: 1.0 In-Reply-To: <1453091083-13931-4-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv3 3/9] pseries: Clean up hash page table allocation error handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson , aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, armbru@redhat.com Cc: lvivier@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 > --- > 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