Re: [Qemu-arm] [PATCH 4/4] hw/arm/virt: Don't incorrectly claim architectural timer to be edge-triggered

[Andrew Jones]

On Fri, Dec 09, 2016 at 04:30:20PM +0000, Peter Maydell wrote:
> The architectural timers in ARM CPUs all have level triggered interrupts
> (unless you're using KVM on a host kernel before 4.4, which misimplemented
> them as edge-triggered).
> 
> We were incorrectly describing them in the device tree as edge triggered.
> This can cause problems for guest kernels in 4.8 before rc6:
>  * pre-4.8 kernels ignore the values in the DT
>  * 4.8 before rc6 write the DT values to the GIC config registers
>  * newer than rc6 ignore the DT and insist that the timer interrupts
>    are level triggered regardless
> 
> Fix the DT so we're describing reality. For backwards-compatibility
> purposes, only do this for the virt-2.9 machine onward.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  hw/arm/virt.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)

Reviewed-by: Andrew Jones <address@hidden>

I'll submit the ACPI counterpart, which would require we add
claim_edge_triggered_timers to VirtGuestInfo, but I have plans
for VirtGuestInfo - killing it, just like x86 killed PcGuestInfo.
So the need for this timer patch just bumped the kill VirtGuestInfo
patch up in the priority queue. I'll start working on it today.

Thanks,
drew

> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 54498ea..2ca9527 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -71,6 +71,7 @@ typedef struct {
>      bool disallow_affinity_adjustment;
>      bool no_its;
>      bool no_pmu;
> +    bool claim_edge_triggered_timers;
>  } VirtMachineClass;
>  
>  typedef struct {
> @@ -309,12 +310,31 @@ static void fdt_add_psci_node(const VirtMachineState 
> *vms)
>  
>  static void fdt_add_timer_nodes(const VirtMachineState *vms, int gictype)
>  {
> -    /* Note that on A15 h/w these interrupts are level-triggered,
> -     * but for the GIC implementation provided by both QEMU and KVM
> -     * they are edge-triggered.
> +    /* On real hardware these interrupts are level-triggered.
> +     * On KVM they were edge-triggered before host kernel version 4.4,
> +     * and level-triggered afterwards.
> +     * On emulated QEMU they are level-triggered.
> +     *
> +     * Getting the DTB info about them wrong is awkward for some
> +     * guest kernels:
> +     *  pre-4.8 ignore the DT and leave the interrupt configured
> +     *   with whatever the GIC reset value (or the bootloader) left it at
> +     *  4.8 before rc6 honour the incorrect data by programming it back
> +     *   into the GIC, causing problems
> +     *  4.8rc6 and later ignore the DT and always write "level triggered"
> +     *   into the GIC
> +     *
> +     * For backwards-compatibility, virt-2.8 and earlier will continue
> +     * to say these are edge-triggered, but later machines will report
> +     * the correct information.
>       */
>      ARMCPU *armcpu;
> -    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> +
> +    if (vmc->claim_edge_triggered_timers) {
> +        irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
> +    }
>  
>      if (gictype == 2) {
>          irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> @@ -1556,8 +1576,14 @@ static void virt_2_8_instance_init(Object *obj)
>  
>  static void virt_machine_2_8_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_2_9_options(mc);
>      SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_8);
> +    /* For 2.8 and earlier we falsely claimed in the DT that
> +     * our timers were edge-triggered, not level-triggered.
> +     */
> +    vmc->claim_edge_triggered_timers = true;
>  }
>  DEFINE_VIRT_MACHINE(2, 8)
>  
> -- 
> 2.7.4
>