Re: [Qemu-devel] [kvm-unit-tests PATCH 3/4] arm/arm64: GICv2: add GICD_ITARGETSR testing

[Andrew Jones]

On Thu, Nov 17, 2016 at 05:57:51PM +0000, Andre Przywara wrote:
> Some tests for the ITARGETS registers.
> Bits corresponding to non-existent CPUs must be RAZ/WI.
> These registers must be byte-accessible, also check that accesses beyond
> the implemented IRQ limit are actually read-as-zero/write-ignore.
> 
> Signed-off-by: Andre Przywara <address@hidden>
> ---
>  arm/gic.c         | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/arm/asm/gic.h |  1 +
>  2 files changed, 55 insertions(+)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index a27da2c..02b1be1 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -397,6 +397,57 @@ static bool test_priorities(int nr_irqs, void *priptr)
>       return true;
>  }
>  
> +static bool test_targets(int nr_irqs)
> +{
> +     void *targetsptr = gicv2_dist_base() + GICD_ITARGETSR;
> +     u32 orig_targets;
> +     u32 cpu_mask;
> +     u32 pattern, reg;
> +
> +     orig_targets = readl(targetsptr + 32);
> +     report_prefix_push("ITARGETSR");
> +
> +     cpu_mask = (1 << nr_cpus) - 1;

Shouldn't this be 1 << (nr_cpus - 1) ?

Is this test always going to be gicv2-only? We should probably comment it,
if so. We don't want to risk this being run when nr_cpus can be larger
than 8.

> +     cpu_mask |= cpu_mask << 8;
> +     cpu_mask |= cpu_mask << 16;
> +
> +     /* Check that bits for non implemented CPUs are RAZ/WI. */
> +     if (nr_cpus < 8) {
> +             writel(0xffffffff, targetsptr + 32);
> +             report("bits for %d non-existent CPUs masked",
> +                    !(readl(targetsptr + 32) & ~cpu_mask), 8 - nr_cpus);
> +     } else {
> +             report_skip("CPU masking (all CPUs implemented)");
> +     }
> +
> +     report("accesses beyond limit RAZ/WI",
> +            test_readonly_32(targetsptr + nr_irqs, true));
> +
> +     pattern = 0x0103020f;
> +     writel(pattern, targetsptr + 32);
> +     reg = readl(targetsptr + 32);
> +     report("register content preserved (%08x => %08x)",
> +            reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
> +
> +     /*
> +      * The TARGETS registers are byte accessible, do a byte-wide
> +      * read and write of known content to check for this.
> +      */
> +     reg = readb(targetsptr + 33);
> +     report("byte reads successful (0x%08x => 0x%02x)",
> +            reg == (BYTE(pattern, 1) & cpu_mask),
> +            pattern & cpu_mask, reg);
> +
> +     pattern = REPLACE_BYTE(pattern, 2, 0x04);
> +     writeb(BYTE(pattern, 2), targetsptr + 34);
> +     reg = readl(targetsptr + 32);
> +     report("byte writes successful (0x%02x => 0x%08x)",
> +            reg == (pattern & cpu_mask), BYTE(pattern, 2), reg);

Last patch also had a byte addressability test. Maybe we should make
a helper function?

> +
> +     writel(orig_targets, targetsptr + 32);
> +     return true;

Function can/should be void.

> +}
> +
>  static int gic_test_mmio(int gic_version)
>  {
>       u32 reg;
> @@ -436,6 +487,9 @@ static int gic_test_mmio(int gic_version)
>  
>       test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
>  
> +     if (gic_version == 2)
> +             test_targets(nr_irqs);
> +
>       return 0;
>  }
>  
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index cef748d..6f170cb 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -14,6 +14,7 @@
>  #define GICD_IGROUPR                 0x0080
>  #define GICD_ISENABLER                       0x0100
>  #define GICD_IPRIORITYR                      0x0400
> +#define GICD_ITARGETSR                       0x0800
>  #define GICD_SGIR                    0x0f00
>  #define GICD_ICPIDR2                 0x0fe8
>  
> -- 
> 2.9.0
> 
>

Thanks,
drew