From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33696) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLLLi-0000OW-Dr for qemu-devel@nongnu.org; Mon, 18 Jan 2016 20:44:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLLLg-0007g9-9S for qemu-devel@nongnu.org; Mon, 18 Jan 2016 20:44:46 -0500 Date: Tue, 19 Jan 2016 12:12:10 +1100 From: David Gibson Message-ID: <20160119011210.GW9301@voom.fritz.box> References: <1453091083-13931-1-git-send-email-david@gibson.dropbear.id.au> <1453091083-13931-4-git-send-email-david@gibson.dropbear.id.au> <569CA6BF.9060606@redhat.com> <874mebqjcb.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="tpZe61tYkA9f+p/0" Content-Disposition: inline In-Reply-To: <874mebqjcb.fsf@blackfin.pond.sub.org> 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: Markus Armbruster Cc: lvivier@redhat.com, Thomas Huth , aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org --tpZe61tYkA9f+p/0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 18, 2016 at 11:21:08AM +0100, Markus Armbruster wrote: > Thomas Huth writes: >=20 > > On 18.01.2016 05:24, David Gibson wrote: > >> The spapr_alloc_htab() and spapr_reset_htab() functions currently hand= le > >> all errors with error_setg(&error_abort, ...). > >>=20 > >> But really, the callers are really better placed to decide on the error > >> handling. So, instead make the functions use the error propagation > >> infrastructure. > >>=20 > >> 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 th= an > >> indicating a programming error in qemu. > >>=20 > >> While we're at it improve the messages themselves a bit, and clean up = the > >> indentation a little. > >>=20 > >> Signed-off-by: David Gibson > >> --- > >> hw/ppc/spapr.c | 24 ++++++++++++++++-------- > >> 1 file changed, 16 insertions(+), 8 deletions(-) > >>=20 > >> 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)) &=3D tswap64(~HPTE= 64_V_HPTE_DIRTY)) > >> #define DIRTY_HPTE(_hpte) ((*(uint64_t *)(_hpte)) |=3D tswap64(HPTE6= 4_V_HPTE_DIRTY)) > >> =20 > >> -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 requeste= d size, try with smaller maxmem"); > >> + error_setg_errno(errp, -shift, > >> + "Error allocating KVM hash page table, try s= maller 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 !=3D spapr->htab_shift) { > >> - error_setg(&error_abort, "Failed to allocate HTAB of requ= ested 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 =3D true" in case of errors. > > (practically this does not happen because errp =3D error_fatal, but in > > case the caller gets changed, this might introduce subtle errors otherw= ise) >=20 > Good point. >=20 > With abort() / exit(), we don't have to worry about recovery. In > particular, we don't have to revert half-done changes. >=20 > 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. --=20 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 --tpZe61tYkA9f+p/0 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWnY1qAAoJEGw4ysog2bOSgNIP/if6wvuYhMBrzo6V/WbUT+gk zyCga8dWwSZZjOsKHMhtbyL82TVp1Ge4mQgr7Rn7f3oE6KzdGqn8Bgdn9pRWeKvv e92HxqzM1WPZgnueMkUjhN6n9R5kTZ1Sb9YLpbw3lcdlr+FJ3o1KkHhmZsNRLdL7 0PsMLGatONgonseCaOe5PvCufbQ0vCauFoKB9kzaqBK0JlDM6PfSv+IDfigpJAPN I/nDLyAFk6WH5JX64LDN3zEnEq7ZO+iFdwW3z4H29SzWyoNW3/RP9NJTb2DZwU3P A7pLTzdWJlt6949HPiVEIgzTl38BjiomofeaiVdoJfl0soSf8pm1BwckuIo76EBO XWqc+dhEC1qoKRdaA36cjiPXzK2t+SjvPl9nKvlWL5/y5ukUnDRSLd23idiculxx pe+IZG9PzgqoudP57dEY7PRWc913YAPvkcgaD5k3hmDTs5EJfdAvOZS+1nBiKmik 65ywJraV7QOdONej2mZQEJJ/SkXflmLVm6TVjyaJVxYgqy9vjI3tPQWnBUaTbCso Iglf5dFtTR8XYIurgjO9NzPgNyYs3t/bHia1Frv7Z27nyPznPWGDj5mB/y/XaGbL /IW2vXMoy5MUFG+jMqOk4sHO0l8+docIQMyN3PVZ5d95ZbIzleW9tiPcj/AueRYM WFtDIYvZfT5LIGx0idae =u+pu -----END PGP SIGNATURE----- --tpZe61tYkA9f+p/0--