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

[Christoffer Dall <christoffer.dall@linaro.org>]

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 <peter.maydell@linaro.org>
> ---
>  hw/arm/virt.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> 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.
>       */

Is this really necessary?

I don't think the KVM GIC implementation ever listened to the guest in
terms of how to configure PPIs, but instead ignores writes to the config
registers for these interrupts (which I think the GIC architecture
allows).

So this would only be a matter of how the guest kernel between v4.8-rc1
and v4.8-rc6 expects the behavior to be.  Does the arch timer driver
really do something different in how it deals with interrupts based on
this DT value?

Of course, I suppose we could also be running other guests (UEFI?) but
again, if the KVM GIC doesn't care about how the guest tries to program
it, can it make a difference?

>      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)
>  
I don't understand this virt machine class version stuff.  In which case
is the claim_edge_triggered_timers set to true?  (ok, appears to be when
a 2.8 machine is created, but does that happen automatically or does the
user specifically have to ask for it?)

Thanks,
-Christoffer