Message ID | 20160905140944.5379-1-alex.bennee@linaro.org |
---|---|
State | New |
Headers | show |
Hi, Your series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [RFC PATCH] hw/intc/arm_gic: handle Set-Active/Clear-Active registers Type: series Message-id: 20160905140944.5379-1-alex.bennee@linaro.org === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 # Useful git options git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' c424725 hw/intc/arm_gic: handle Set-Active/Clear-Active registers === OUTPUT BEGIN === Checking PATCH 1/1: hw/intc/arm_gic: handle Set-Active/Clear-Active registers... ERROR: braces {} are necessary for all arms of this statement #29: FILE: hw/intc/arm_gic.c:978: + if (irq >= s->num_irq || s->revision < 2) [...] ERROR: braces {} are necessary for all arms of this statement #47: FILE: hw/intc/arm_gic.c:994: + if (irq >= s->num_irq) [...] total: 2 errors, 0 warnings, 40 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
On 5 September 2016 at 15:09, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org> > --- > 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. > + > + 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. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 5 September 2016 at 15:09, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org> >> --- >> 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
On 5 September 2016 at 16:45, Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 5 September 2016 at 15:09, Alex Bennée <alex.bennee@linaro.org> wrote: >>> } 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 :-/ The GICv2 spec does actually say these registers don't exist in v1: in the Table 4-1 Distributor register map, the entry for GICD_ICACTIVERn is marked as having footnote 'e' applying, which says "GICv2 only". In the GICv1 spec range 0x380-0x3FC is marked Reserved (ie there is no clear-active register set at all.) Ditto in the 11MPCore TRM and the v7M ARM ARM. (The GICv1 register summary also appears in the A9 MPCore TRM, though that's a bit of an obscure place to know to look for it.) As a rule of thumb, if unsure whether something exists in all configurations, default to leaving it locked down/unimplemented. We can always widen it later if guest software turns out to use it, but it's much harder to later know we can remove something safely once we've let a QEMU into the wild that implemented it. thanks -- PMM
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) + 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); + } + } } 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; + + 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;
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. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- hw/intc/arm_gic.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)