From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57161) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bgw5W-0004Rh-K0 for qemu-devel@nongnu.org; Mon, 05 Sep 2016 11:45:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bgw5L-0001ro-Jk for qemu-devel@nongnu.org; Mon, 05 Sep 2016 11:45:33 -0400 Received: from mail-wm0-x236.google.com ([2a00:1450:400c:c09::236]:36277) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bgw5L-0001rH-A8 for qemu-devel@nongnu.org; Mon, 05 Sep 2016 11:45:23 -0400 Received: by mail-wm0-x236.google.com with SMTP id b187so25377279wme.1 for ; Mon, 05 Sep 2016 08:45:23 -0700 (PDT) References: <20160905140944.5379-1-alex.bennee@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Mon, 05 Sep 2016 16:45:20 +0100 Message-ID: <87lgz6z6y7.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC PATCH] hw/intc/arm_gic: handle Set-Active/Clear-Active registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers , "open list:ARM cores" Peter Maydell writes: > On 5 September 2016 at 15:09, Alex Bennée wrote: >> I noticed while testing with modern kernels and -d guest_errors warnings >> about invalid writes to the GIC. For GICv2 these registers certainly >> should work so I've implemented both. As the code is common between all >> the various GICs writes to GICD_ISACTIVERn is checked to ensure it is >> not a RO register for v1 GICs. > > This is definitely a bug, and also the right way to fix it, so you > don't need to mark your patch 'RFC' :-) > > Some minor review issues below. > >> Signed-off-by: Alex Bennée >> --- >> hw/intc/arm_gic.c | 33 +++++++++++++++++++++++++++++++-- >> 1 file changed, 31 insertions(+), 2 deletions(-) >> >> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c >> index b30cc91..423a4ae 100644 >> --- a/hw/intc/arm_gic.c >> +++ b/hw/intc/arm_gic.c >> @@ -972,9 +972,38 @@ static void gic_dist_writeb(void *opaque, hwaddr offset, >> GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK); >> } >> } >> + } else if (offset < 0x380) { >> + /* Interrupt Set-Active */ >> + irq = (offset - 0x300) * 8 + GIC_BASE_IRQ; >> + if (irq >= s->num_irq || s->revision < 2) > > Better to check "s->revision != 2" -- we still have the NVIC > code tangled up with the GIC, and on the NVIC these are R/O. > >> + goto bad_reg; >> + >> + for (i = 0; i < 8; i++) { >> + if (s->security_extn && !attrs.secure && >> + !GIC_TEST_GROUP(irq + i, 1 << cpu)) { >> + continue; /* Ignore Non-secure access of Group0 IRQ */ >> + } >> + >> + if (value & (1 << i)) { >> + GIC_SET_ACTIVE(irq + i, 1 << cpu); > > The mask parameter to GIC_SET_ACTIVE/GIC_CLEAR_ACTIVE should be > calculated like > int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK; > > -- compare the set-enable, clear-enable, etc write code. > > I'm fairly sure that just setting the active bit (ie not also > trying to update the active-priority registers) is the correct > behaviour here, though the GIC spec is not clear to me on this point. > >> + } >> + } >> } else if (offset < 0x400) { >> - /* Interrupt Active. */ >> - goto bad_reg; >> + /* Interrupt Clear-Active */ >> + irq = (offset - 0x380) * 8 + GIC_BASE_IRQ; >> + if (irq >= s->num_irq) >> + goto bad_reg; > > This is missing the check against s->revision. This was deliberate as the GICv1 reference was only mentioned in the GICD_ISACTIVERn description. Unfortunately the only documentation for GICs on silver.arm.com are for v2 and v3 so I had to guess what v1 had :-/ > >> + >> + for (i = 0; i < 8; i++) { >> + if (s->security_extn && !attrs.secure && >> + !GIC_TEST_GROUP(irq + i, 1 << cpu)) { >> + continue; /* Ignore Non-secure access of Group0 IRQ */ >> + } >> + >> + if (value & (1 << i)) { >> + GIC_CLEAR_ACTIVE(irq + i, 1 << cpu); >> + } >> + } >> } else if (offset < 0x800) { >> /* Interrupt Priority. */ >> irq = (offset - 0x400) + GIC_BASE_IRQ; >> -- >> 2.9.3 > > It looks like we don't implement reads of clear-active correctly > either. I'll have a look at that. I'll see if Drew's kvm-unit-tests for the GIC exercise any of this code ;-) Thanks for the review. -- Alex Bennée