[PATCH] hw/intc/arm_gic: Allow reset of the running priority

[PATCH] hw/intc/arm_gic: Allow reset of the running priority

[Petr Pavlu]

When running Linux on a machine with GICv2, the kernel can crash while
processing an interrupt and can subsequently start a kdump kernel from
the active interrupt handler. In such a case, the crashed kernel might
not gracefully signal the end of interrupt to the GICv2 hardware. The
kdump kernel will however try to reset the GIC state on startup to get
the controller into a sane state, in particular the kernel writes ones
to GICD_ICACTIVERn and wipes out GICC_APRn to make sure that no
interrupt is active.

The patch makes sure that this reset works for the GICv2 emulation in
QEMU too and the kdump kernel starts receiving interrupts. It adds
a logic to recalculate the running priority when GICC_APRn/GICC_NSAPRn
is written and implements read of GICC_IIDR so the kernel can recognize
that the GICv2 with GICC_APRn is present.

The described scenario can be reproduced on an AArch64 QEMU virt machine
with a kdump-enabled Linux system by using the softdog module. The kdump
kernel will hang at some point because QEMU still thinks the running
priority is that of the timer interrupt and asserts no new interrupts to
the system:
$ modprobe softdog soft_margin=10 soft_panic=1
$ cat > /dev/watchdog
[Press Enter to start the watchdog, wait for its timeout and observe
that the kdump kernel hangs on startup.]

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 hw/intc/arm_gic.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index a994b1f024..2edbc4cb46 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1662,6 +1662,10 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         }
         break;
     }
+    case 0xfc:
+        /* GICv1/2, ARM implementation */
+        *data = (s->revision << 16) + 0x43b;
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_read: Bad offset %x\n", (int)offset);
@@ -1727,6 +1731,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
         } else {
             s->apr[regno][cpu] = value;
         }
+        s->running_priority[cpu] = gic_get_prio_from_apr_bits(s, cpu);
         break;
     }
     case 0xe0: case 0xe4: case 0xe8: case 0xec:
@@ -1743,6 +1748,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
             return MEMTX_OK;
         }
         s->nsapr[regno][cpu] = value;
+        s->running_priority[cpu] = gic_get_prio_from_apr_bits(s, cpu);
         break;
     }
     case 0x1000:
-- 
2.34.1

Re: [PATCH] hw/intc/arm_gic: Allow reset of the running priority

[Peter Maydell]

On Tue, 14 Dec 2021 at 17:28, Petr Pavlu <petr.pavlu@suse.com> wrote:
> When running Linux on a machine with GICv2, the kernel can crash while
> processing an interrupt and can subsequently start a kdump kernel from
> the active interrupt handler. In such a case, the crashed kernel might
> not gracefully signal the end of interrupt to the GICv2 hardware. The
> kdump kernel will however try to reset the GIC state on startup to get
> the controller into a sane state, in particular the kernel writes ones
> to GICD_ICACTIVERn and wipes out GICC_APRn to make sure that no
> interrupt is active.
>
> The patch makes sure that this reset works for the GICv2 emulation in
> QEMU too and the kdump kernel starts receiving interrupts. It adds
> a logic to recalculate the running priority when GICC_APRn/GICC_NSAPRn
> is written and implements read of GICC_IIDR so the kernel can recognize
> that the GICv2 with GICC_APRn is present.
>
> The described scenario can be reproduced on an AArch64 QEMU virt machine
> with a kdump-enabled Linux system by using the softdog module. The kdump
> kernel will hang at some point because QEMU still thinks the running
> priority is that of the timer interrupt and asserts no new interrupts to
> the system:
> $ modprobe softdog soft_margin=10 soft_panic=1
> $ cat > /dev/watchdog
> [Press Enter to start the watchdog, wait for its timeout and observe
> that the kdump kernel hangs on startup.]

Hi; thanks for this patch and sorry I haven't got to it earlier
(I've been on holiday). Both the mishandling of the cached
priority and the failure to implement GICC_IIDR are definitely bugs,
but I think they are distinct bugs. Could you split this into a two
patch series, please ? (If you don't have time to do the respin,
let me know and I can do it at this end.)

> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  hw/intc/arm_gic.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index a994b1f024..2edbc4cb46 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1662,6 +1662,10 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>          }
>          break;
>      }
> +    case 0xfc:
> +        /* GICv1/2, ARM implementation */
> +        *data = (s->revision << 16) + 0x43b;

This is correct for GICv1 and GICv2, but not for 11MPCore, whose
interrupt controller has no GICC_IIDR:

https://developer.arm.com/documentation/ddi0360/e/mpcore-distributed-interrupt-controller/cpu-interface-registers-definition?lang=en

So this should be

    if (s->revision == REV_11MPCORE) {
        /* Reserved on 11MPCore */
        *data = 0;
    } else {
        /* GICv1 or v2; Arm implementation */
        *data = (s->revision << 16) | 0x43b;
    }

(also using '|' rather than '+' since we're assembling a value
as a bunch of bit operations, not doing arithmetic on it. The
end result is the same but I think '|' is clearer stylistically.)

> +        break;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "gic_cpu_read: Bad offset %x\n", (int)offset);
> @@ -1727,6 +1731,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>          } else {
>              s->apr[regno][cpu] = value;
>          }
> +        s->running_priority[cpu] = gic_get_prio_from_apr_bits(s, cpu);
>          break;
>      }
>      case 0xe0: case 0xe4: case 0xe8: case 0xec:
> @@ -1743,6 +1748,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>              return MEMTX_OK;
>          }
>          s->nsapr[regno][cpu] = value;
> +        s->running_priority[cpu] = gic_get_prio_from_apr_bits(s, cpu);
>          break;
>      }
>      case 0x1000:

These parts are correct and just need to be in their own patch.

thanks
-- PMM