On Mon, Mar 06, 2017 at 07:16:09AM -0800, Christoffer Dall wrote:
> From: Christoffer Dall <[email protected]>
> 
> We found a deadlock when changing the active state of an interrupt while
> the interrupt is queued on the LR of the running VCPU.
> 
> Defend KVM against this bug in the future now when we've introduced a
> fix.
> 
> Signed-off-by: Christoffer Dall <[email protected]>
> ---
> Sending with the right subject prefix this time.
> 
>  arm/gic.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg | 14 +++++++++++++-
>  lib/arm/asm/gic.h |  2 ++
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 3054d45..82f6632 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -254,6 +254,47 @@ static struct gic gicv3 = {
>       },
>  };
>  
> +static void ipi_clear_active_handler(struct pt_regs *regs __unused)
> +{
> +     u32 irqstat = gic_read_iar();
> +     u32 irqnr = gic_iar_irqnr(irqstat);
> +
> +     if (irqnr != GICC_INT_SPURIOUS) {
> +             void *base;
> +             u32 val = 1 << IPI_IRQ;
> +
> +             if (gic_version() == 2)
> +                     base = gicv2_dist_base();
> +             else
> +                     base = gicv3_redist_base();

Using the redistributor interface is correct with the current
gic_enable_defaults(), because it enables affinity routing. I
wonder if we shouldn't confirm it's enabled first though.

> +
> +             writel(val, base + GICD_ICACTIVER);

For gicv3, with affinity routing enabled, the offset is
technically named GICR_ICACTIVER0, but that's also 0x380, so
it doesn't matter.

Not sure we need the 'val' variable.

> +
> +             smp_rmb(); /* pairs with wmb in stats_reset */
> +             ++acked[smp_processor_id()];
> +             check_irqnr(irqnr);
> +             smp_wmb(); /* pairs with rmb in check_acked */
> +     } else {
> +             ++spurious[smp_processor_id()];
> +             smp_wmb();
> +     }
> +}
> +
> +static void run_active_clear_test(void)
> +{
> +     report_prefix_push("active");
> +     gic_enable_defaults();
> +#ifdef __arm__
> +     install_exception_handler(EXCPTN_IRQ, ipi_clear_active_handler);
> +#else
> +     install_irq_handler(EL1H_IRQ, ipi_clear_active_handler);
> +#endif
> +     local_irq_enable();
> +
> +     ipi_test_self();
> +     report_prefix_pop();
> +}
> +
>  int main(int argc, char **argv)
>  {
>       char pfx[8];
> @@ -290,6 +331,8 @@ int main(int argc, char **argv)
>                               cpu == IPI_SENDER ? ipi_send : ipi_recv);
>               }
>               ipi_recv();
> +     } else if (strcmp(argv[1], "active") == 0) {
> +             run_active_clear_test();

test_active_clear() ?

>       } else {
>               report_abort("Unknown subtest '%s'", argv[1]);
>       }
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index c98658f..32d9858 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -26,7 +26,7 @@
>  
> ##############################################################################
>  
>  #
> -# Test that the configured number of processors (smp = <num>), and
> +/# Test that the configured number of processors (smp = <num>), and

stray character here

>  # that the configured amount of memory (-m <MB>) are correctly setup
>  # by the framework.
>  #
> @@ -92,6 +92,18 @@ smp = $MAX_SMP
>  extra_params = -machine gic-version=3 -append 'ipi'
>  groups = gic
>  
> +[gicv2-active]
> +file = gic.flat
> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> +extra_params = -machine gic-version=2 -append 'active'
> +groups = gic
> +
> +[gicv3-active]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'active'
> +groups = gic
> +
>  # Test PSCI emulation
>  [psci]
>  file = psci.flat
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index c8186f2..c688ccc 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -12,6 +12,8 @@
>  #define GICD_TYPER                   0x0004
>  #define GICD_IGROUPR                 0x0080
>  #define GICD_ISENABLER                       0x0100
> +#define GICD_ISACTIVER                       0x0300
> +#define GICD_ICACTIVER                       0x0380
>  #define GICD_IPRIORITYR                      0x0400
>  #define GICD_SGIR                    0x0f00
>  
> -- 
> 2.5.0
> 

Everything besides the stray character in unittests.cfg is on
nit level, and the stray character can probably be cleaned up
on commit, so

Reviewed-by: Andrew Jones <[email protected]>

Thanks,
drew
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reply via email to