From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48544) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYPTn-000847-1K for qemu-devel@nongnu.org; Tue, 23 Feb 2016 21:47:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYPTl-0007eD-Ka for qemu-devel@nongnu.org; Tue, 23 Feb 2016 21:47:06 -0500 Date: Wed, 24 Feb 2016 11:36:49 +1100 From: David Gibson Message-ID: <20160224003649.GA2808@voom.fritz.box> References: <20160205084340.4178.52171.stgit@bahia.huguette.org> <20160208014519.GD3702@voom> <20160208093149.73248af5@bahia.huguette.org> <20160210104116.79fd4b93@bahia.huguette.org> <20160223184644.39a9df6f@bahia.huguette.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AX2clmtSw6ieHBMM" Content-Disposition: inline In-Reply-To: <20160223184644.39a9df6f@bahia.huguette.org> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH] hw/intc: fix failure return for xics_alloc_block() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org --AX2clmtSw6ieHBMM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 23, 2016 at 06:46:44PM +0100, Greg Kurz wrote: > On Wed, 10 Feb 2016 10:41:16 +0100 > Greg Kurz wrote: >=20 > > On Mon, 8 Feb 2016 09:31:49 +0100 > > Greg Kurz wrote: > >=20 > > > On Mon, 8 Feb 2016 11:45:19 +1000 > > > David Gibson wrote: > > > =20 > > > > On Fri, Feb 05, 2016 at 09:43:40AM +0100, Greg Kurz wrote: =20 > > > > > From: Brian W. Hart > > > > >=20 > > > > > xics_alloc_block() does not return a clear error code when it > > > > > fails to allocate a block of interrupts. Instead it returns the > > > > > base interrupt number minus 1. This change updates it to return a > > > > > clear -1 in case of failure (following the example of xics_alloc(= )). > > > > >=20 > > > > > The two callers of xics_alloc_block() are updated to check for > > > > > a negative return as an error. They had previously checked for > > > > > a 0 return as an error, which wrongly treated most failures as > > > > > successes. > > > > >=20 > > > > > Fixes: bee763dbfb8cfceea112131970da07f215f293a6 > > > > > Signed-off-by: Brian W. Hart > > > > > [only pass src and num to trace_xics_alloc_block_failed_no_left, > > > > > added trace_xics_alloc_block_failed_no_left definition to trace-= events] > > > > > Signed-off-by: Greg Kurz =20 > > > >=20 > > > > Hrm, it would probably be better to give xics_alloc_block() an Error > > > > ** argument so it can report errors using the new API. > > > > =20 > > >=20 > > > Sure. I can rework the patch to do so. > > > =20 > >=20 > > The trace_xics_alloc_block_failed_no_left trace is more a debugging thi= ng > > than an error to be reported to the user. Also, rtas_ibm_change_msi() > > already has a meaningful error message: > >=20 > > error_report("Cannot allocate MSIs for device %x", config_addr); > >=20 > > So in the end, I'm not sure about the benefit of passing an Error ** > > down to xics_alloc_block(). > >=20 >=20 > Hi David ! >=20 > Given the remarks above, do you still think we should pass Error ** ? I still think using the Error API would be preferable, but it doesn't make a huge difference. > > > > TBH the whole xics_alloc_block() interface is kind of dubious, or at > > > > least the ics_find_free_block() part of it. Dynamically allocating > > > > irqs to devices is basically awful for migration, so it's better to > > > > have fixed allocations of all interrupts at the machine level. > > > > =20 > > >=20 > > > I agree about the extra complexity, but isn't it the purpose of > > > the ibm,change-msi RTAS call ? I'm not sure to understand what you > > > are suggesting... > > > =20 > >=20 > > And anyway, even if the decision is made one day to have fixed > > allocations, shouldn't we consider fixing this bug first ? > >=20 >=20 > According to the following commit changelog, the dynamic allocation was > introduced to support PCI hot(un)plug. Maybe Alexey may explain why it > got coded this way. >=20 > commit bee763dbfb8cfceea112131970da07f215f293a6 > Author: Alexey Kardashevskiy > Date: Fri May 30 19:34:15 2014 +1000 >=20 > spapr: Move interrupt allocator to xics >=20 > I'm not sure of the amount of reflexion and work needed to address your > concern... Given the time frame, what about deferring xics rework to 2.7 > and fix the bug we currently have in 2.6 ? Yeah, sorry, I realised after I sent that the allocation does actually make sense in this case - these are guest triggered allocations that we really do need an allocator for, not host setup allocations which we have used in the past but turned out to be dubious. --=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 --AX2clmtSw6ieHBMM Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWzPshAAoJEGw4ysog2bOSkkgP/isMr/wOT/+SkI3k5zADt9xF 525LQHFcPzFXtyysyOUAfz56nTmEyN2+8wHAZhT6O9oGn4LVNa26v8cIIkRm2A3F cGSx1ji469QiyBDABopHaH243o1lpVuG9qm422yLXduCSJi/7rrKmUfR28Py+iXH VQsr3x1KCEdwwhYl0S07gc30YRr9iU7uAfzww2K2/I9eBXadgWEEqOpkkpBuCtHX tHLY8HJyJCMyHRvB2Wipa6kxqQi8CJVSNb4jefDYx6jHe+L1haGcGrNh/HfsfW8E izcol9vdopXNfmLsdOBPPn+PREDKEvjxO5ii7jpMCd1yG+rJhCmIBk5hyCLJE5PH sbsgVSFe2B4AlVt6Vr5VeNtr8MziGQVokIJ0RLYqNNmRaarHzob/oN1mboWzEAiJ y/ZSv+svRJ5WbXGCWCjdFWrl+WaNQ/6U7FWoem1QNBwrtHQl/WRfm0gy1b+gOBKx TNbHqalLdpUAhgKSLJxPUBMVQlFfBvqbUGoEMcqn5V72jpvyxP/qk/3IYEIi9eVQ E941LdTmMi4knAlqlf4pt1x03gV11FgTswC25lusY23PmhKNKzUJB0PZSW/TO7qe RN7CGrjSv9FJCvqJtN0hgzPF52dxbEUBL0v0fkZVSO374Dc/AMIP2AGoq886CS5i +YgejEk+vWKj3sAqgn7t =mF7Q -----END PGP SIGNATURE----- --AX2clmtSw6ieHBMM--