[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] hw/intc/arm_gic: handle Set-Active/Clear-Ac
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [RFC PATCH] hw/intc/arm_gic: handle Set-Active/Clear-Active registers |
Date: |
Mon, 5 Sep 2016 16:06:30 +0100 |
On 5 September 2016 at 15:09, Alex Bennée <address@hidden> 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 <address@hidden>
> ---
> 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