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

[Auger Eric]

Hi,

On 18/11/2016 15:20, Andrew Jones wrote:
> 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) ?
original looks correct to me.
> 
> 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;
Don't we miss the 4th byte mask?

Thanks

Eric
>> +
>> +    /* 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
>