Re: [Qemu-devel] [kvm-unit-tests PATCH v8 2/3] arm: pmu: Check cycle count increases

[Wei Huang]

On 11/11/2016 01:43 AM, Andrew Jones wrote:
> On Tue, Nov 08, 2016 at 12:17:14PM -0600, Wei Huang wrote:
>> From: Christopher Covington 
>>
>> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
>> even for the smallest delta of two subsequent reads.
>>
>> Signed-off-by: Christopher Covington 
>> Signed-off-by: Wei Huang 
>> ---
>>  arm/pmu.c | 98 
>> +++
>>  1 file changed, 98 insertions(+)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index 0b29088..d5e3ac3 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -14,6 +14,7 @@
>>   */
>>  #include "libcflat.h"
>>  
>> +#define PMU_PMCR_E (1 << 0)
>>  #define PMU_PMCR_N_SHIFT   11
>>  #define PMU_PMCR_N_MASK0x1f
>>  #define PMU_PMCR_ID_SHIFT  16
>> @@ -21,6 +22,10 @@
>>  #define PMU_PMCR_IMP_SHIFT 24
>>  #define PMU_PMCR_IMP_MASK  0xff
>>  
>> +#define PMU_CYCLE_IDX  31
>> +
>> +#define NR_SAMPLES 10
>> +
>>  #if defined(__arm__)
>>  static inline uint32_t pmcr_read(void)
>>  {
>> @@ -29,6 +34,47 @@ static inline uint32_t pmcr_read(void)
>>  asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>>  return ret;
>>  }
>> +
>> +static inline void pmcr_write(uint32_t value)
>> +{
>> +asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (value));
>> +}
>> +
>> +static inline void pmselr_write(uint32_t value)
>> +{
>> +asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));
>> +}
>> +
>> +static inline void pmxevtyper_write(uint32_t value)
>> +{
>> +asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
>> +}
>> +
>> +/*
>> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, 
>> returning 64
>> + * bits doesn't seem worth the trouble when differential usage of the 
>> result is
>> + * expected (with differences that can easily fit in 32 bits). So just 
>> return
>> + * the lower 32 bits of the cycle count in AArch32.
> 
> Like I said in the last review, I'd rather we not do this. We should
> return the full value and then the test case should confirm the upper
> 32 bits are zero.
> 

Unless I miss something in ARM documentation, ARMv7 PMCCNTR is a 32-bit
register. We can force it to a more coarse-grained cycle counter with
PMCR.D bit=1 (see below). But it is still not a 64-bit register. ARMv8
PMCCNTR_EL0 is a 64-bit register.

"The PMCR.D bit configures whether PMCCNTR increments once every clock
cycle, or once every 64 clock cycles. "

So I think the comment above in the code is an overstatement, which
should be deleted or moved down to ARMv8 pmccntr_read() below.

>> + */
>> +static inline uint32_t pmccntr_read(void)
>> +{
>> +uint32_t cycles;
>> +
>> +asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
>> +return cycles;
>> +}
>> +
>> +static inline void pmcntenset_write(uint32_t value)
>> +{
>> +asm volatile("mcr p15, 0, %0, c9, c12, 1" : : "r" (value));
>> +}
>> +
>> +/* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */
>> +static inline void pmccfiltr_write(uint32_t value)
>> +{
>> +pmselr_write(PMU_CYCLE_IDX);
>> +pmxevtyper_write(value);
>> +}
>>  #elif defined(__aarch64__)
>>  static inline uint32_t pmcr_read(void)
>>  {
>> @@ -37,6 +83,29 @@ static inline uint32_t pmcr_read(void)
>>  asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>>  return ret;
>>  }
>> +
>> +static inline void pmcr_write(uint32_t value)
>> +{
>> +asm volatile("msr pmcr_el0, %0" : : "r" (value));
>> +}
>> +
>> +static inline uint32_t pmccntr_read(void)
>> +{
>> +uint32_t cycles;
>> +
>> +asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
>> +return cycles;
>> +}
>> +
>> +static inline void pmcntenset_write(uint32_t value)
>> +{
>> +asm volatile("msr pmcntenset_el0, %0" : : "r" (value));
>> +}
>> +
>> +static inline void pmccfiltr_write(uint32_t value)
>> +{
>> +asm volatile("msr pmccfiltr_el0, %0" : : "r" (value));
>> +}
>>  #endif
>>  
>>  /*
>> @@ -63,11 +132,40 @@ static bool check_pmcr(void)
>>  return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0;
>>  }
>>  
>> +/*
>> + * Ensure that the cycle counter progresses between back-to-back reads.
>> + */
>> +static bool check_cycles_increase(void)
>> +{
>> +pmcr_write(pmcr_read() | PMU_PMCR_E);
>> +
>> +for (int i = 0; i < NR_SAMPLES; i++) {
>> +unsigned long a, b;
>> +
>> +a = pmccntr_read();
>> +b = pmccntr_read();
>> +
>> +if (a >= b) {
>> +printf("Read %ld then %ld.\n", a, b);
>> +return false;
>> +}
>> +}
>> +
>> +pmcr_write(pmcr_read() & ~PMU_PMCR_E);
>> +
>> +return true;
>> +}
>> +
>>  int main(void)
>>  {
>>  report_prefix_push("pmu");
>>  
>> +/* init for PMU event access, right now only care about cycle count */
>> +pmcntenset_write(1 << PMU_CYCLE_IDX);
>> +pmccfiltr_write(0); /* count cycles in EL0, EL1, but not EL2 

Re: [kvm-unit-tests PATCH v5 09/11] arm/arm64: add initial gicv3 support

[Andre Przywara]

Hi,

On 10/11/16 17:21, Andrew Jones wrote:
> Signed-off-by: Andrew Jones 
> 
> ---
> v5: use modern register names [Andre]
> v4:
>  - only take defines from kernel we need now [Andre]
>  - simplify enable by not caring if we reinit the distributor [drew]
> v2:
>  - configure irqs as NS GRP1
> ---
>  lib/arm/asm/arch_gicv3.h   | 42 +
>  lib/arm/asm/gic-v3.h   | 94 
> ++
>  lib/arm/asm/gic.h  |  6 ++-
>  lib/arm/gic.c  | 65 
>  lib/arm64/asm/arch_gicv3.h | 44 ++
>  lib/arm64/asm/gic-v3.h |  1 +
>  lib/arm64/asm/sysreg.h | 44 ++
>  7 files changed, 294 insertions(+), 2 deletions(-)
>  create mode 100644 lib/arm/asm/arch_gicv3.h
>  create mode 100644 lib/arm/asm/gic-v3.h
>  create mode 100644 lib/arm64/asm/arch_gicv3.h
>  create mode 100644 lib/arm64/asm/gic-v3.h
>  create mode 100644 lib/arm64/asm/sysreg.h
> 
> diff --git a/lib/arm/asm/arch_gicv3.h b/lib/arm/asm/arch_gicv3.h
> new file mode 100644
> index ..81a1e5f6c29c
> --- /dev/null
> +++ b/lib/arm/asm/arch_gicv3.h
> @@ -0,0 +1,42 @@
> +/*
> + * All ripped off from arch/arm/include/asm/arch_gicv3.h
> + *
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_ARCH_GICV3_H_
> +#define _ASMARM_ARCH_GICV3_H_
> +
> +#ifndef __ASSEMBLY__
> +#include 
> +#include 
> +#include 
> +
> +#define __stringify xstr
> +
> +#define __ACCESS_CP15(CRn, Op1, CRm, Op2)p15, Op1, %0, CRn, CRm, Op2
> +
> +#define ICC_PMR  __ACCESS_CP15(c4, 0, c6, 0)
> +#define ICC_IGRPEN1  __ACCESS_CP15(c12, 0, c12, 7)
> +
> +static inline void gicv3_write_pmr(u32 val)
> +{
> + asm volatile("mcr " __stringify(ICC_PMR) : : "r" (val));
> +}
> +
> +static inline void gicv3_write_grpen1(u32 val)
> +{
> + asm volatile("mcr " __stringify(ICC_IGRPEN1) : : "r" (val));
> + isb();
> +}
> +
> +static inline u64 gicv3_read_typer(const volatile void __iomem *addr)

It may be worth to add that this is for GICR_TYPER (or GITS_TYPER),
because GICD_TYPER is 32-bit only.
Or to make the naming generic (because the code actually is), along the
lines of read_64bit_reg or the like?

> +{
> + u64 val = readl(addr);
> + val |= (u64)readl(addr + 4) << 32;
> + return val;
> +}
> +
> +#endif /* !__ASSEMBLY__ */
> +#endif /* _ASMARM_ARCH_GICV3_H_ */
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> new file mode 100644
> index ..e0f303d82508
> --- /dev/null
> +++ b/lib/arm/asm/gic-v3.h
> @@ -0,0 +1,94 @@
> +/*
> + * All GIC* defines are lifted from include/linux/irqchip/arm-gic-v3.h
> + *
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_GIC_V3_H_
> +#define _ASMARM_GIC_V3_H_
> +
> +#ifndef _ASMARM_GIC_H_
> +#error Do not directly include . Include 
> +#endif
> +
> +#define GICD_CTLR_RWP(1U << 31)
> +#define GICD_CTLR_ARE_NS (1U << 4)
> +#define GICD_CTLR_ENABLE_G1A (1U << 1)
> +#define GICD_CTLR_ENABLE_G1  (1U << 0)

+1 to Alex for adding a comment noting the non-secure view here.

> +
> +/* Re-Distributor registers, offsets from RD_base */
> +#define GICR_TYPER   0x0008
> +
> +#define GICR_TYPER_LAST  (1U << 4)
> +
> +/* Re-Distributor registers, offsets from SGI_base */
> +#define GICR_IGROUPR0GICD_IGROUPR
> +#define GICR_ISENABLER0  GICD_ISENABLER
> +#define GICR_IPRIORITYR0 GICD_IPRIORITYR
> +
> +#include 
> +
> +#ifndef __ASSEMBLY__
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct gicv3_data {
> + void *dist_base;
> + void *redist_base[NR_CPUS];
> + unsigned int irq_nr;
> +};
> +extern struct gicv3_data gicv3_data;
> +
> +#define gicv3_dist_base()(gicv3_data.dist_base)
> +#define gicv3_redist_base()  
> (gicv3_data.redist_base[smp_processor_id()])
> +#define gicv3_sgi_base() 
> (gicv3_data.redist_base[smp_processor_id()] + SZ_64K)
> +
> +extern int gicv3_init(void);
> +extern void gicv3_enable_defaults(void);
> +extern void gicv3_set_redist_base(void);
> +
> +static inline void gicv3_do_wait_for_rwp(void *base)
> +{
> + int count = 10; /* 1s */
> +
> + while (readl(base + GICD_CTLR) & GICD_CTLR_RWP) {
> + if (!--count) {
> + printf("GICv3: RWP timeout!\n");
> + abort();
> + }
> + cpu_relax();
> + udelay(10);
> + };
> +}
> +
> +static inline void gicv3_dist_wait_for_rwp(void)
> +{
> + gicv3_do_wait_for_rwp(gicv3_dist_base());
> +}
> +
> +static inline void 

Re: [Qemu-devel] [kvm-unit-tests PATCH v8 3/3] arm: pmu: Add CPI checking

[Wei Huang]

On 11/11/2016 02:08 AM, Andrew Jones wrote:
> On Tue, Nov 08, 2016 at 12:17:15PM -0600, Wei Huang wrote:
>> From: Christopher Covington 
>>
>> Calculate the numbers of cycles per instruction (CPI) implied by ARM
>> PMU cycle counter values. The code includes a strict checking facility
>> intended for the -icount option in TCG mode in the configuration file.
>>
>> Signed-off-by: Christopher Covington 
>> Signed-off-by: Wei Huang 
>> ---
>>  arm/pmu.c | 101 
>> +-
>>  arm/unittests.cfg |  14 
>>  2 files changed, 114 insertions(+), 1 deletion(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index d5e3ac3..09aff89 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -15,6 +15,7 @@
>>  #include "libcflat.h"
>>  
>>  #define PMU_PMCR_E (1 << 0)
>> +#define PMU_PMCR_C (1 << 2)
>>  #define PMU_PMCR_N_SHIFT   11
>>  #define PMU_PMCR_N_MASK0x1f
>>  #define PMU_PMCR_ID_SHIFT  16
>> @@ -75,6 +76,23 @@ static inline void pmccfiltr_write(uint32_t value)
>>  pmselr_write(PMU_CYCLE_IDX);
>>  pmxevtyper_write(value);
>>  }
>> +
>> +/*
>> + * Extra instructions inserted by the compiler would be difficult to 
>> compensate
>> + * for, so hand assemble everything between, and including, the PMCR 
>> accesses
>> + * to start and stop counting.
>> + */
>> +static inline void loop(int i, uint32_t pmcr)
> 
> We should probably pick a more descriptive name for this function, as
> we intend to add many more PMU tests to this file. While at it, let's
> change 'i' to 'n', as it's the number of times to loop.

I will rename it to fixed_num_loop(). When more tests are added to it,
we can standardize the name, e.g. *_test().

> 
>> +{
>> +asm volatile(
>> +"   mcr p15, 0, %[pmcr], c9, c12, 0\n"
>> +"1: subs%[i], %[i], #1\n"
>> +"   bgt 1b\n"
>> +"   mcr p15, 0, %[z], c9, c12, 0\n"
>> +: [i] "+r" (i)
>> +: [pmcr] "r" (pmcr), [z] "r" (0)
>> +: "cc");
>> +}
>>  #elif defined(__aarch64__)
>>  static inline uint32_t pmcr_read(void)
>>  {
>> @@ -106,6 +124,23 @@ static inline void pmccfiltr_write(uint32_t value)
>>  {
>>  asm volatile("msr pmccfiltr_el0, %0" : : "r" (value));
>>  }
>> +
>> +/*
>> + * Extra instructions inserted by the compiler would be difficult to 
>> compensate
>> + * for, so hand assemble everything between, and including, the PMCR 
>> accesses
>> + * to start and stop counting.
>> + */
>> +static inline void loop(int i, uint32_t pmcr)
>> +{
>> +asm volatile(
>> +"   msr pmcr_el0, %[pmcr]\n"
>> +"1: subs%[i], %[i], #1\n"
>> +"   b.gt1b\n"
>> +"   msr pmcr_el0, xzr\n"
>> +: [i] "+r" (i)
>> +: [pmcr] "r" (pmcr)
>> +: "cc");
>> +}
>>  #endif
>>  
>>  /*
>> @@ -156,8 +191,71 @@ static bool check_cycles_increase(void)
>>  return true;
>>  }
>>  
>> -int main(void)
>> +/*
>> + * Execute a known number of guest instructions. Only odd instruction counts
>> + * greater than or equal to 3 are supported by the in-line assembly code. 
>> The
>> + * control register (PMCR_EL0) is initialized with the provided value 
>> (allowing
>> + * for example for the cycle counter or event counters to be reset). At the 
>> end
>> + * of the exact instruction loop, zero is written to PMCR_EL0 to disable
>> + * counting, allowing the cycle counter or event counters to be read at the
>> + * leisure of the calling code.
>> + */
>> +static void measure_instrs(int num, uint32_t pmcr)
>> +{
>> +int i = (num - 1) / 2;
>> +
>> +assert(num >= 3 && ((num - 1) % 2 == 0));
>> +loop(i, pmcr);
>> +}
>> +
>> +/*
>> + * Measure cycle counts for various known instruction counts. Ensure that 
>> the
>> + * cycle counter progresses (similar to check_cycles_increase() but with 
>> more
>> + * instructions and using reset and stop controls). If supplied a positive,
>> + * nonzero CPI parameter, also strictly check that every measurement matches
>> + * it. Strict CPI checking is used to test -icount mode.
>> + */
>> +static bool check_cpi(int cpi)
>> +{
>> +uint32_t pmcr = pmcr_read() | PMU_PMCR_C | PMU_PMCR_E;
>> +
>> +if (cpi > 0)
>> +printf("Checking for CPI=%d.\n", cpi);
>> +printf("instrs : cycles0 cycles1 ...\n");
>> +
>> +for (int i = 3; i < 300; i += 32) {
>> +int avg, sum = 0;
>> +
>> +printf("%d :", i);
>> +for (int j = 0; j < NR_SAMPLES; j++) {
>> +int cycles;
>> +
>> +measure_instrs(i, pmcr);
>> +cycles =pmccntr_read();
> ^ missing space
> 
>> +printf(" %d", cycles);
>> +
>> +if (!cycles || (cpi > 0 && cycles != i * cpi)) {
>> +printf("\n");
>> +return false;
>> +}
>> +
>> + 

Re: [kvm-unit-tests PATCH v5 08/11] libcflat: add IS_ALIGNED() macro, and page sizes

[Andrew Jones]

On Fri, Nov 11, 2016 at 03:02:42PM +, Alex Bennée wrote:
> 
> Andrew Jones  writes:
> 
> > From: Peter Xu 
> >
> > These macros will be useful to do page alignment checks.
> >
> > Reviewed-by: Andre Przywara 
> > Signed-off-by: Peter Xu 
> > [drew: also added SZ_64K]
> > Signed-off-by: Andrew Jones 
> > ---
> >  lib/libcflat.h | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index 82005f5d014f..143fc53061fe 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -33,6 +33,12 @@
> >  #define __ALIGN_MASK(x, mask)  (((x) + (mask)) & ~(mask))
> >  #define __ALIGN(x, a)  __ALIGN_MASK(x, (typeof(x))(a) - 1)
> >  #define ALIGN(x, a)__ALIGN((x), (a))
> > +#define IS_ALIGNED(x, a)   (((x) & ((typeof(x))(a) - 1)) == 0)
> > +
> > +#define SZ_4K  (0x1000)
> > +#define SZ_64K (0x1)
> > +#define SZ_2M  (0x20)
> > +#define SZ_1G  (0x4000)
> 
> We don't seem to use IS_ALIGNED, or in fact anything apart from SZ_64K
> (which is multiplied by 2 anyway) so I'm not sure if this patch is worth
> it for this series.

I cherry-picked this patch (and modified it to add SZ_64K) from a
different series, one currently in-flight from Peter Xu.

> 
> Stylistically I thought (1 << foo) was preferred for setting of
> individual bits? Otherwise I'd be tempted to line the values up with
> zero padding to make it easier to read the bit position.

I agree with the style comments and will do that for v6.

> 
> >
> >  typedef uint8_tu8;
> >  typedef int8_t s8;
> 
>

Thanks,
drew 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v5 09/11] arm/arm64: add initial gicv3 support

[Alex Bennée]

Andrew Jones  writes:

> Signed-off-by: Andrew Jones 
>
> ---
> v5: use modern register names [Andre]
> v4:
>  - only take defines from kernel we need now [Andre]
>  - simplify enable by not caring if we reinit the distributor [drew]
> v2:
>  - configure irqs as NS GRP1
> ---
>  lib/arm/asm/arch_gicv3.h   | 42 +
>  lib/arm/asm/gic-v3.h   | 94 
> ++
>  lib/arm/asm/gic.h  |  6 ++-
>  lib/arm/gic.c  | 65 
>  lib/arm64/asm/arch_gicv3.h | 44 ++
>  lib/arm64/asm/gic-v3.h |  1 +
>  lib/arm64/asm/sysreg.h | 44 ++
>  7 files changed, 294 insertions(+), 2 deletions(-)
>  create mode 100644 lib/arm/asm/arch_gicv3.h
>  create mode 100644 lib/arm/asm/gic-v3.h
>  create mode 100644 lib/arm64/asm/arch_gicv3.h
>  create mode 100644 lib/arm64/asm/gic-v3.h
>  create mode 100644 lib/arm64/asm/sysreg.h
>
> diff --git a/lib/arm/asm/arch_gicv3.h b/lib/arm/asm/arch_gicv3.h
> new file mode 100644
> index ..81a1e5f6c29c
> --- /dev/null
> +++ b/lib/arm/asm/arch_gicv3.h
> @@ -0,0 +1,42 @@
> +/*
> + * All ripped off from arch/arm/include/asm/arch_gicv3.h
> + *
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_ARCH_GICV3_H_
> +#define _ASMARM_ARCH_GICV3_H_
> +
> +#ifndef __ASSEMBLY__
> +#include 
> +#include 
> +#include 
> +
> +#define __stringify xstr
> +
> +#define __ACCESS_CP15(CRn, Op1, CRm, Op2)p15, Op1, %0, CRn, CRm, Op2
> +
> +#define ICC_PMR  __ACCESS_CP15(c4, 0, c6, 0)
> +#define ICC_IGRPEN1  __ACCESS_CP15(c12, 0, c12, 7)
> +
> +static inline void gicv3_write_pmr(u32 val)
> +{
> + asm volatile("mcr " __stringify(ICC_PMR) : : "r" (val));
> +}
> +
> +static inline void gicv3_write_grpen1(u32 val)
> +{
> + asm volatile("mcr " __stringify(ICC_IGRPEN1) : : "r" (val));
> + isb();
> +}
> +
> +static inline u64 gicv3_read_typer(const volatile void __iomem *addr)
> +{
> + u64 val = readl(addr);
> + val |= (u64)readl(addr + 4) << 32;

I'd be tempted to wrap the cast in additional parentheses for clarity:

  val |= ((u64)readl(addr + 4)) << 32;

> + return val;
> +}
> +
> +#endif /* !__ASSEMBLY__ */
> +#endif /* _ASMARM_ARCH_GICV3_H_ */
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> new file mode 100644
> index ..e0f303d82508
> --- /dev/null
> +++ b/lib/arm/asm/gic-v3.h
> @@ -0,0 +1,94 @@
> +/*
> + * All GIC* defines are lifted from include/linux/irqchip/arm-gic-v3.h
> + *
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_GIC_V3_H_
> +#define _ASMARM_GIC_V3_H_
> +
> +#ifndef _ASMARM_GIC_H_
> +#error Do not directly include . Include 
> +#endif
> +
> +#define GICD_CTLR_RWP(1U << 31)
> +#define GICD_CTLR_ARE_NS (1U << 4)
> +#define GICD_CTLR_ENABLE_G1A (1U << 1)
> +#define GICD_CTLR_ENABLE_G1  (1U << 0)

I got confused when looking at the data sheet until I noticed Secure and
Non-secure worlds have subtly different bit positions. It might be worth
making that clear in a comment.

> +
> +/* Re-Distributor registers, offsets from RD_base */
> +#define GICR_TYPER   0x0008
> +
> +#define GICR_TYPER_LAST  (1U << 4)
> +
> +/* Re-Distributor registers, offsets from SGI_base */
> +#define GICR_IGROUPR0GICD_IGROUPR
> +#define GICR_ISENABLER0  GICD_ISENABLER
> +#define GICR_IPRIORITYR0 GICD_IPRIORITYR
> +
> +#include 
> +
> +#ifndef __ASSEMBLY__
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct gicv3_data {
> + void *dist_base;
> + void *redist_base[NR_CPUS];
> + unsigned int irq_nr;
> +};
> +extern struct gicv3_data gicv3_data;
> +
> +#define gicv3_dist_base()(gicv3_data.dist_base)
> +#define gicv3_redist_base()  
> (gicv3_data.redist_base[smp_processor_id()])
> +#define gicv3_sgi_base() 
> (gicv3_data.redist_base[smp_processor_id()] + SZ_64K)
> +
> +extern int gicv3_init(void);
> +extern void gicv3_enable_defaults(void);
> +extern void gicv3_set_redist_base(void);
> +
> +static inline void gicv3_do_wait_for_rwp(void *base)
> +{
> + int count = 10; /* 1s */
> +
> + while (readl(base + GICD_CTLR) & GICD_CTLR_RWP) {
> + if (!--count) {
> + printf("GICv3: RWP timeout!\n");
> + abort();
> + }
> + cpu_relax();
> + udelay(10);
> + };
> +}
> +
> +static inline void gicv3_dist_wait_for_rwp(void)
> +{
> + gicv3_do_wait_for_rwp(gicv3_dist_base());
> +}
> +
> +static inline void 

Re: [kvm-unit-tests PATCH v5 06/11] arm/arm64: add initial gicv2 support

[Andre Przywara]

Hi,

On 10/11/16 17:21, Andrew Jones wrote:
> Add some gicv2 support. This just adds init and enable
> functions, allowing unit tests to start messing with it.
> 
> Signed-off-by: Andrew Jones 
> 
> ---
> v5: share/use only the modern register names [Andre]

Thanks! That looks much better now.

Reviewed-by: Andre Przywara 

> v4:
>  - only take defines from kernel we need now [Andre]
>  - moved defines to asm/gic.h so they'll be shared with v3 [drew]
>  - simplify enable by not caring if we reinit the distributor [drew]
>  - init all GICD_INT_DEF_PRI_X4 registers [Eric]
> ---
>  arm/Makefile.common|  1 +
>  lib/arm/asm/gic-v2.h   | 34 ++
>  lib/arm/asm/gic.h  | 37 
>  lib/arm/gic.c  | 76 
> ++
>  lib/arm64/asm/gic-v2.h |  1 +
>  lib/arm64/asm/gic.h|  1 +
>  6 files changed, 150 insertions(+)
>  create mode 100644 lib/arm/asm/gic-v2.h
>  create mode 100644 lib/arm/asm/gic.h
>  create mode 100644 lib/arm/gic.c
>  create mode 100644 lib/arm64/asm/gic-v2.h
>  create mode 100644 lib/arm64/asm/gic.h
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index ccb554d9251a..41239c37e092 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -42,6 +42,7 @@ cflatobjs += lib/arm/mmu.o
>  cflatobjs += lib/arm/bitops.o
>  cflatobjs += lib/arm/psci.o
>  cflatobjs += lib/arm/smp.o
> +cflatobjs += lib/arm/gic.o
>  
>  libeabi = lib/arm/libeabi.a
>  eabiobjs = lib/arm/eabi_compat.o
> diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
> new file mode 100644
> index ..c2d5fecd4886
> --- /dev/null
> +++ b/lib/arm/asm/gic-v2.h
> @@ -0,0 +1,34 @@
> +/*
> + * All GIC* defines are lifted from include/linux/irqchip/arm-gic.h
> + *
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_GIC_V2_H_
> +#define _ASMARM_GIC_V2_H_
> +
> +#ifndef _ASMARM_GIC_H_
> +#error Do not directly include . Include 
> +#endif
> +
> +#define GICD_ENABLE  0x1
> +#define GICC_ENABLE  0x1
> +
> +#ifndef __ASSEMBLY__
> +
> +struct gicv2_data {
> + void *dist_base;
> + void *cpu_base;
> + unsigned int irq_nr;
> +};
> +extern struct gicv2_data gicv2_data;
> +
> +#define gicv2_dist_base()(gicv2_data.dist_base)
> +#define gicv2_cpu_base() (gicv2_data.cpu_base)
> +
> +extern int gicv2_init(void);
> +extern void gicv2_enable_defaults(void);
> +
> +#endif /* !__ASSEMBLY__ */
> +#endif /* _ASMARM_GIC_V2_H_ */
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> new file mode 100644
> index ..d44e47bcf404
> --- /dev/null
> +++ b/lib/arm/asm/gic.h
> @@ -0,0 +1,37 @@
> +/*
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_GIC_H_
> +#define _ASMARM_GIC_H_
> +
> +#include 
> +
> +#define GICD_CTLR0x
> +#define GICD_TYPER   0x0004
> +#define GICD_ISENABLER   0x0100
> +#define GICD_IPRIORITYR  0x0400
> +
> +#define GICD_TYPER_IRQS(typer)   typer) & 0x1f) + 1) * 32)
> +#define GICD_INT_EN_SET_SGI  0x
> +#define GICD_INT_DEF_PRI_X4  0xa0a0a0a0
> +
> +#define GICC_CTLR0x
> +#define GICC_PMR 0x0004
> +
> +#define GICC_INT_PRI_THRESHOLD   0xf0
> +
> +#ifndef __ASSEMBLY__
> +
> +/*
> + * gic_init will try to find all known gics, and then
> + * initialize the gic data for the one found.
> + * returns
> + *  0   : no gic was found
> + *  > 0 : the gic version of the gic found
> + */
> +extern int gic_init(void);
> +
> +#endif /* !__ASSEMBLY__ */
> +#endif /* _ASMARM_GIC_H_ */
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> new file mode 100644
> index ..d655105e058b
> --- /dev/null
> +++ b/lib/arm/gic.c
> @@ -0,0 +1,76 @@
> +/*
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include 
> +#include 
> +#include 
> +
> +struct gicv2_data gicv2_data;
> +
> +/*
> + * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> + */
> +static bool
> +gic_get_dt_bases(const char *compatible, void **base1, void **base2)
> +{
> + struct dt_pbus_reg reg;
> + struct dt_device gic;
> + struct dt_bus bus;
> + int node, ret;
> +
> + dt_bus_init_defaults();
> + dt_device_init(, , NULL);
> +
> + node = dt_device_find_compatible(, compatible);
> + assert(node >= 0 || node == -FDT_ERR_NOTFOUND);
> +
> + if (node == -FDT_ERR_NOTFOUND)
> + return false;
> +
> + dt_device_bind_node(, node);
> +
> + ret = dt_pbus_translate(, 

Re: [Qemu-devel] [kvm-unit-tests PATCH v5 06/11] arm/arm64: add initial gicv2 support

[Andrew Jones]

On Fri, Nov 11, 2016 at 03:17:25PM +, Andre Przywara wrote:
> > 
> > This doesn't seem to be used and I'm not sure what GICD_TYPER_IRQS it is
> > trying to achieve.
> 
> The idea is to calculate the number of implemented SPIs. But I am not a
> big fan of copying a macro from the emulation code base to the test code.
>

Consider it rewritten then :-) I did review the spec while copying
over stuff from Linux, so Linux has been getting a review at the
same time. I agree that we should be cautious with copy+paste and
we should probably not bring over super complex macros that are hard
to understand, because we want kvm-unit-tests to be easy to read.
Let's not say "no macros from Linux", but rather case-by-case it.
I think this macro is a nice addition to our little API.

Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v5 03/11] arm/arm64: smp: support more than 8 cpus

[Andre Przywara]

Hi,

On 10/11/16 17:21, Andrew Jones wrote:
> By adding support for launching with gicv3 we can break the 8 vcpu
> limit. This patch adds support to smp code and also selects the
> vgic model corresponding to the host. The vgic model may also be
> manually selected by adding e.g. -machine gic-version=3 to
> extra_params.
> 
> Reviewed-by: Alex Bennée 
> Signed-off-by: Andrew Jones 
> 
> ---
> v5: left cpus a u32 for now. Changing to u64 requires a change to
> devicetree. Will do it later. [Andre]

Given that we address this in the future:

Reviewed-by: Andre Przywara 


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests PATCH v5 06/11] arm/arm64: add initial gicv2 support

[Andrew Jones]

On Fri, Nov 11, 2016 at 02:52:40PM +, Alex Bennée wrote:
> 
> Andrew Jones  writes:
> 
> > Add some gicv2 support. This just adds init and enable
> > functions, allowing unit tests to start messing with it.
> >
> > Signed-off-by: Andrew Jones 
> >
> > ---
> > v5: share/use only the modern register names [Andre]
> > v4:
> >  - only take defines from kernel we need now [Andre]
> >  - moved defines to asm/gic.h so they'll be shared with v3 [drew]
> >  - simplify enable by not caring if we reinit the distributor [drew]
> >  - init all GICD_INT_DEF_PRI_X4 registers [Eric]
> > ---
> >  arm/Makefile.common|  1 +
> >  lib/arm/asm/gic-v2.h   | 34 ++
> >  lib/arm/asm/gic.h  | 37 
> >  lib/arm/gic.c  | 76 
> > ++
> >  lib/arm64/asm/gic-v2.h |  1 +
> >  lib/arm64/asm/gic.h|  1 +
> >  6 files changed, 150 insertions(+)
> >  create mode 100644 lib/arm/asm/gic-v2.h
> >  create mode 100644 lib/arm/asm/gic.h
> >  create mode 100644 lib/arm/gic.c
> >  create mode 100644 lib/arm64/asm/gic-v2.h
> >  create mode 100644 lib/arm64/asm/gic.h
> >
> > diff --git a/arm/Makefile.common b/arm/Makefile.common
> > index ccb554d9251a..41239c37e092 100644
> > --- a/arm/Makefile.common
> > +++ b/arm/Makefile.common
> > @@ -42,6 +42,7 @@ cflatobjs += lib/arm/mmu.o
> >  cflatobjs += lib/arm/bitops.o
> >  cflatobjs += lib/arm/psci.o
> >  cflatobjs += lib/arm/smp.o
> > +cflatobjs += lib/arm/gic.o
> >
> >  libeabi = lib/arm/libeabi.a
> >  eabiobjs = lib/arm/eabi_compat.o
> > diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
> > new file mode 100644
> > index ..c2d5fecd4886
> > --- /dev/null
> > +++ b/lib/arm/asm/gic-v2.h
> > @@ -0,0 +1,34 @@
> > +/*
> > + * All GIC* defines are lifted from include/linux/irqchip/arm-gic.h
> > + *
> > + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > + */
> > +#ifndef _ASMARM_GIC_V2_H_
> > +#define _ASMARM_GIC_V2_H_
> > +
> > +#ifndef _ASMARM_GIC_H_
> > +#error Do not directly include . Include 
> > +#endif
> > +
> > +#define GICD_ENABLE0x1
> > +#define GICC_ENABLE0x1
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +struct gicv2_data {
> > +   void *dist_base;
> > +   void *cpu_base;
> > +   unsigned int irq_nr;
> > +};
> > +extern struct gicv2_data gicv2_data;
> > +
> > +#define gicv2_dist_base()  (gicv2_data.dist_base)
> > +#define gicv2_cpu_base()   (gicv2_data.cpu_base)
> > +
> > +extern int gicv2_init(void);
> > +extern void gicv2_enable_defaults(void);
> > +
> > +#endif /* !__ASSEMBLY__ */
> > +#endif /* _ASMARM_GIC_V2_H_ */
> > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> > new file mode 100644
> > index ..d44e47bcf404
> > --- /dev/null
> > +++ b/lib/arm/asm/gic.h
> > @@ -0,0 +1,37 @@
> > +/*
> > + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > + */
> > +#ifndef _ASMARM_GIC_H_
> > +#define _ASMARM_GIC_H_
> > +
> > +#include 
> > +
> > +#define GICD_CTLR  0x
> > +#define GICD_TYPER 0x0004
> > +#define GICD_ISENABLER 0x0100
> > +#define GICD_IPRIORITYR0x0400
> 
> Maybe GICD_ISENABLER_BASE and GICD_IPRIORITYR_BASE as they are the start
> of a series of registers? Also what happened to the formatting?

These names match the kernel's naming (and the ARM spec) The formatting
is fine after applying, diff just bumped them over prepending the '+'

> 
> > +
> > +#define GICD_TYPER_IRQS(typer) typer) & 0x1f) + 1)
> > * 32)
> > +#define GICD_INT_EN_SET_SGI0x
> > +#define GICD_INT_DEF_PRI_X40xa0a0a0a0
> 
> This doesn't seem to be used and I'm not sure what GICD_TYPER_IRQS it is
> trying to achieve.

All used in lib/arm/gic.c. See the spec for how to interpret the
ITLinesNumber field of GICD_TYPER

> 
> A comment above and here to make it clear we are talking about offsets
> in the distributor and cpu register maps would aid confusion.

Better not add them then, as I wouldn't want to *aid* confusion :-)

I can add some comments (I do add a couple for gicv3), but I think
here it's pretty clear by the name prefix (GICD vs. GICC).

> 
> > +
> > +#define GICC_CTLR  0x
> > +#define GICC_PMR   0x0004
> > +
> > +#define GICC_INT_PRI_THRESHOLD 0xf0
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +/*
> > + * gic_init will try to find all known gics, and then
> > + * initialize the gic data for the one found.
> > + * returns
> > + *  0   : no gic was found
> > + *  > 0 : the gic version of the gic found
> > + */
> > +extern int gic_init(void);
> 
> If we are going to make the library API agnostic I 

Re: [Qemu-devel] [kvm-unit-tests PATCH v5 10/11] arm/arm64: gicv3: add an IPI test

[Andre Przywara]

Hi,

On 11/11/16 14:53, Alex Bennée wrote:
> 
> Andrew Jones  writes:
> 
>> On Fri, Nov 11, 2016 at 10:02:59AM +, Alex Bennée wrote:
>>>
>>> Andrew Jones  writes:
>>>
 On Thu, Nov 10, 2016 at 07:53:58PM +, Alex Bennée wrote:
 [...]
>> +struct gic gicv2 = {
>> +.ipi = {
>> +.enable = gicv2_enable_defaults,
>> +.send_self = gicv2_ipi_send_self,
>> +.send_tlist = gicv2_ipi_send_tlist,
>> +.send_broadcast = gicv2_ipi_send_broadcast,
>> +},
>> +.read_iar = gicv2_read_iar,
>> +.irqnr = gicv2_irqnr,
>> +.write_eoi = gicv2_write_eoi,
>> +};
>> +
>> +struct gic gicv3 = {
>> +.ipi = {
>> +.enable = gicv3_enable_defaults,
>> +.send_self = gicv3_ipi_send_self,
>> +.send_tlist = gicv3_ipi_send_tlist,
>> +.send_broadcast = gicv3_ipi_send_broadcast,
>> +},
>> +.read_iar = gicv3_read_iar,
>> +.irqnr = gicv3_irqnr,
>> +.write_eoi = gicv3_write_eoir,
>> +};
>> +
>
> So I was re-basing my kvm-unit-tests against your GIC rework and found
> myself copy and pasting a bunch of this into my tests that fire IRQs.
> That makes me think the abstraction should be in the library code so
> other tests can fiddle with sending IRQs.
>
> What do you think?
>

 I guess you mean moving the above two structs and their corresponding
 functions (all which aren't already common) to lib/arm/ ? Or do you
 just mean the one non-trivial function gicv3_ipi_send_tlist? I think
 agree with gicv3_ipi_send_tlist getting shared, but the others are
 mostly one-liners, so I'm not sure. I guess I'd have to see how you're
 using them first.
>>>
>>> So it looked like there were some functions in the common code for one
>>> GIC which had local test defined functions for the other. They should at
>>> least be consistent.
>>
>> gicv3_read_iar and gicv3_write_eoir being common already is a product of
>> being sysreg wrappers, allowing for both arm32 and arm64 to use functions
>> of the same names, not because I wanted gicv3 to be inconsistent with
>> gicv2 (which uses MMIO and thus doesn't need wrappers)
>>
>>>
>>> For my use case I could do with a common:
>>>
>>>   gic_enable
>>
>> OK, I can extend gic_init() to initialize a 'struct gic_common_ops' that
>> includes an enable -> *_enable_defaults(void), ipi_send(int cpu),
>> read_iar(void), iar_irqnr(u32 iar), and write_eoi(u32 irqstat). And also
>> provide the wrappers gic_enable, gic_ipi_send(cpu), ...
>>
>>>   gic_send_spi(cpu, irq)
>>
>> I'll let you add this one to the new common ops struct :-)
>>
>>>   gic_irq_ack() which returns the iar.
>>
>> This one will be called read_iar.
>>
>> Would that work for you, Alex?
> 
> Sounds good to me :-)
> 
>>
>> Andre,
>>
>> Would this also satisfy your needs for more common code?

TBH I haven't look deeply enough to give an educated answer. I just
wanted to +1 Alex for the general direction. So I guess it's OK. ;-)

I guess we may need some refactoring later anyway, so any missing pieces
can be added/refactored later once we exactly know what we need.

Cheers,
Andre.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v5 06/11] arm/arm64: add initial gicv2 support

[Andre Przywara]

Hi,

On 11/11/16 14:52, Alex Bennée wrote:
> 
> Andrew Jones  writes:
> 
>> Add some gicv2 support. This just adds init and enable
>> functions, allowing unit tests to start messing with it.
>>
>> Signed-off-by: Andrew Jones 
>>
>> ---
>> v5: share/use only the modern register names [Andre]
>> v4:
>>  - only take defines from kernel we need now [Andre]
>>  - moved defines to asm/gic.h so they'll be shared with v3 [drew]
>>  - simplify enable by not caring if we reinit the distributor [drew]
>>  - init all GICD_INT_DEF_PRI_X4 registers [Eric]
>> ---
>>  arm/Makefile.common|  1 +
>>  lib/arm/asm/gic-v2.h   | 34 ++
>>  lib/arm/asm/gic.h  | 37 
>>  lib/arm/gic.c  | 76 
>> ++
>>  lib/arm64/asm/gic-v2.h |  1 +
>>  lib/arm64/asm/gic.h|  1 +
>>  6 files changed, 150 insertions(+)
>>  create mode 100644 lib/arm/asm/gic-v2.h
>>  create mode 100644 lib/arm/asm/gic.h
>>  create mode 100644 lib/arm/gic.c
>>  create mode 100644 lib/arm64/asm/gic-v2.h
>>  create mode 100644 lib/arm64/asm/gic.h
>>
>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>> index ccb554d9251a..41239c37e092 100644
>> --- a/arm/Makefile.common
>> +++ b/arm/Makefile.common
>> @@ -42,6 +42,7 @@ cflatobjs += lib/arm/mmu.o
>>  cflatobjs += lib/arm/bitops.o
>>  cflatobjs += lib/arm/psci.o
>>  cflatobjs += lib/arm/smp.o
>> +cflatobjs += lib/arm/gic.o
>>
>>  libeabi = lib/arm/libeabi.a
>>  eabiobjs = lib/arm/eabi_compat.o
>> diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
>> new file mode 100644
>> index ..c2d5fecd4886
>> --- /dev/null
>> +++ b/lib/arm/asm/gic-v2.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * All GIC* defines are lifted from include/linux/irqchip/arm-gic.h
>> + *
>> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.
>> + */
>> +#ifndef _ASMARM_GIC_V2_H_
>> +#define _ASMARM_GIC_V2_H_
>> +
>> +#ifndef _ASMARM_GIC_H_
>> +#error Do not directly include . Include 
>> +#endif
>> +
>> +#define GICD_ENABLE 0x1
>> +#define GICC_ENABLE 0x1
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +struct gicv2_data {
>> +void *dist_base;
>> +void *cpu_base;
>> +unsigned int irq_nr;
>> +};
>> +extern struct gicv2_data gicv2_data;
>> +
>> +#define gicv2_dist_base()   (gicv2_data.dist_base)
>> +#define gicv2_cpu_base()(gicv2_data.cpu_base)
>> +
>> +extern int gicv2_init(void);
>> +extern void gicv2_enable_defaults(void);
>> +
>> +#endif /* !__ASSEMBLY__ */
>> +#endif /* _ASMARM_GIC_V2_H_ */
>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>> new file mode 100644
>> index ..d44e47bcf404
>> --- /dev/null
>> +++ b/lib/arm/asm/gic.h
>> @@ -0,0 +1,37 @@
>> +/*
>> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.
>> + */
>> +#ifndef _ASMARM_GIC_H_
>> +#define _ASMARM_GIC_H_
>> +
>> +#include 
>> +
>> +#define GICD_CTLR   0x
>> +#define GICD_TYPER  0x0004
>> +#define GICD_ISENABLER  0x0100
>> +#define GICD_IPRIORITYR 0x0400
> 
> Maybe GICD_ISENABLER_BASE and GICD_IPRIORITYR_BASE as they are the start
> of a series of registers?

We should keep the naming consistent to both the spec and the Linux headers.

> Also what happened to the formatting?

Isn't that the usual diff artifact caused by prepending a single
character? Which moves the tab stops, also the mailer adding quotation
characters?

>> +
>> +#define GICD_TYPER_IRQS(typer)  typer) & 0x1f) + 1)
>> * 32)
>> +#define GICD_INT_EN_SET_SGI 0x
>> +#define GICD_INT_DEF_PRI_X4 0xa0a0a0a0
> 
> This doesn't seem to be used and I'm not sure what GICD_TYPER_IRQS it is
> trying to achieve.

The idea is to calculate the number of implemented SPIs. But I am not a
big fan of copying a macro from the emulation code base to the test code.

Cheers,
Andre.

> A comment above and here to make it clear we are talking about offsets
> in the distributor and cpu register maps would aid confusion.
> 
>> +
>> +#define GICC_CTLR   0x
>> +#define GICC_PMR0x0004
>> +
>> +#define GICC_INT_PRI_THRESHOLD  0xf0
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +/*
>> + * gic_init will try to find all known gics, and then
>> + * initialize the gic data for the one found.
>> + * returns
>> + *  0   : no gic was found
>> + *  > 0 : the gic version of the gic found
>> + */
>> +extern int gic_init(void);
> 
> If we are going to make the library API agnostic I guess returning NULL
> or an ops structure would be best here?
> 
>> +
>> +#endif /* !__ASSEMBLY__ */
>> +#endif /* _ASMARM_GIC_H_ */
>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>> new file mode 100644
>> 

Re: [kvm-unit-tests PATCH v5 08/11] libcflat: add IS_ALIGNED() macro, and page sizes

[Alex Bennée]

Andrew Jones  writes:

> From: Peter Xu 
>
> These macros will be useful to do page alignment checks.
>
> Reviewed-by: Andre Przywara 
> Signed-off-by: Peter Xu 
> [drew: also added SZ_64K]
> Signed-off-by: Andrew Jones 
> ---
>  lib/libcflat.h | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 82005f5d014f..143fc53061fe 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -33,6 +33,12 @@
>  #define __ALIGN_MASK(x, mask)(((x) + (mask)) & ~(mask))
>  #define __ALIGN(x, a)__ALIGN_MASK(x, (typeof(x))(a) - 1)
>  #define ALIGN(x, a)  __ALIGN((x), (a))
> +#define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0)
> +
> +#define SZ_4K(0x1000)
> +#define SZ_64K   (0x1)
> +#define SZ_2M(0x20)
> +#define SZ_1G(0x4000)

We don't seem to use IS_ALIGNED, or in fact anything apart from SZ_64K
(which is multiplied by 2 anyway) so I'm not sure if this patch is worth
it for this series.

Stylistically I thought (1 << foo) was preferred for setting of
individual bits? Otherwise I'd be tempted to line the values up with
zero padding to make it easier to read the bit position.

>
>  typedef uint8_t  u8;
>  typedef int8_t   s8;


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v5 06/11] arm/arm64: add initial gicv2 support

[Alex Bennée]

Andrew Jones  writes:

> Add some gicv2 support. This just adds init and enable
> functions, allowing unit tests to start messing with it.
>
> Signed-off-by: Andrew Jones 
>
> ---
> v5: share/use only the modern register names [Andre]
> v4:
>  - only take defines from kernel we need now [Andre]
>  - moved defines to asm/gic.h so they'll be shared with v3 [drew]
>  - simplify enable by not caring if we reinit the distributor [drew]
>  - init all GICD_INT_DEF_PRI_X4 registers [Eric]
> ---
>  arm/Makefile.common|  1 +
>  lib/arm/asm/gic-v2.h   | 34 ++
>  lib/arm/asm/gic.h  | 37 
>  lib/arm/gic.c  | 76 
> ++
>  lib/arm64/asm/gic-v2.h |  1 +
>  lib/arm64/asm/gic.h|  1 +
>  6 files changed, 150 insertions(+)
>  create mode 100644 lib/arm/asm/gic-v2.h
>  create mode 100644 lib/arm/asm/gic.h
>  create mode 100644 lib/arm/gic.c
>  create mode 100644 lib/arm64/asm/gic-v2.h
>  create mode 100644 lib/arm64/asm/gic.h
>
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index ccb554d9251a..41239c37e092 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -42,6 +42,7 @@ cflatobjs += lib/arm/mmu.o
>  cflatobjs += lib/arm/bitops.o
>  cflatobjs += lib/arm/psci.o
>  cflatobjs += lib/arm/smp.o
> +cflatobjs += lib/arm/gic.o
>
>  libeabi = lib/arm/libeabi.a
>  eabiobjs = lib/arm/eabi_compat.o
> diff --git a/lib/arm/asm/gic-v2.h b/lib/arm/asm/gic-v2.h
> new file mode 100644
> index ..c2d5fecd4886
> --- /dev/null
> +++ b/lib/arm/asm/gic-v2.h
> @@ -0,0 +1,34 @@
> +/*
> + * All GIC* defines are lifted from include/linux/irqchip/arm-gic.h
> + *
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_GIC_V2_H_
> +#define _ASMARM_GIC_V2_H_
> +
> +#ifndef _ASMARM_GIC_H_
> +#error Do not directly include . Include 
> +#endif
> +
> +#define GICD_ENABLE  0x1
> +#define GICC_ENABLE  0x1
> +
> +#ifndef __ASSEMBLY__
> +
> +struct gicv2_data {
> + void *dist_base;
> + void *cpu_base;
> + unsigned int irq_nr;
> +};
> +extern struct gicv2_data gicv2_data;
> +
> +#define gicv2_dist_base()(gicv2_data.dist_base)
> +#define gicv2_cpu_base() (gicv2_data.cpu_base)
> +
> +extern int gicv2_init(void);
> +extern void gicv2_enable_defaults(void);
> +
> +#endif /* !__ASSEMBLY__ */
> +#endif /* _ASMARM_GIC_V2_H_ */
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> new file mode 100644
> index ..d44e47bcf404
> --- /dev/null
> +++ b/lib/arm/asm/gic.h
> @@ -0,0 +1,37 @@
> +/*
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_GIC_H_
> +#define _ASMARM_GIC_H_
> +
> +#include 
> +
> +#define GICD_CTLR0x
> +#define GICD_TYPER   0x0004
> +#define GICD_ISENABLER   0x0100
> +#define GICD_IPRIORITYR  0x0400

Maybe GICD_ISENABLER_BASE and GICD_IPRIORITYR_BASE as they are the start
of a series of registers? Also what happened to the formatting?

> +
> +#define GICD_TYPER_IRQS(typer)   typer) & 0x1f) + 1)
> * 32)
> +#define GICD_INT_EN_SET_SGI  0x
> +#define GICD_INT_DEF_PRI_X4  0xa0a0a0a0

This doesn't seem to be used and I'm not sure what GICD_TYPER_IRQS it is
trying to achieve.

A comment above and here to make it clear we are talking about offsets
in the distributor and cpu register maps would aid confusion.

> +
> +#define GICC_CTLR0x
> +#define GICC_PMR 0x0004
> +
> +#define GICC_INT_PRI_THRESHOLD   0xf0
> +
> +#ifndef __ASSEMBLY__
> +
> +/*
> + * gic_init will try to find all known gics, and then
> + * initialize the gic data for the one found.
> + * returns
> + *  0   : no gic was found
> + *  > 0 : the gic version of the gic found
> + */
> +extern int gic_init(void);

If we are going to make the library API agnostic I guess returning NULL
or an ops structure would be best here?

> +
> +#endif /* !__ASSEMBLY__ */
> +#endif /* _ASMARM_GIC_H_ */
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> new file mode 100644
> index ..d655105e058b
> --- /dev/null
> +++ b/lib/arm/gic.c
> @@ -0,0 +1,76 @@
> +/*
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include 
> +#include 
> +#include 
> +
> +struct gicv2_data gicv2_data;
> +
> +/*
> + * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> + */
> +static bool
> +gic_get_dt_bases(const char *compatible, void **base1, void **base2)
> +{
> + struct dt_pbus_reg reg;
> + struct dt_device gic;
> 

Re: [Qemu-devel] [kvm-unit-tests PATCH v5 10/11] arm/arm64: gicv3: add an IPI test

[Alex Bennée]

Andrew Jones  writes:

> On Fri, Nov 11, 2016 at 10:02:59AM +, Alex Bennée wrote:
>>
>> Andrew Jones  writes:
>>
>> > On Thu, Nov 10, 2016 at 07:53:58PM +, Alex Bennée wrote:
>> > [...]
>> >> > +struct gic gicv2 = {
>> >> > +   .ipi = {
>> >> > +   .enable = gicv2_enable_defaults,
>> >> > +   .send_self = gicv2_ipi_send_self,
>> >> > +   .send_tlist = gicv2_ipi_send_tlist,
>> >> > +   .send_broadcast = gicv2_ipi_send_broadcast,
>> >> > +   },
>> >> > +   .read_iar = gicv2_read_iar,
>> >> > +   .irqnr = gicv2_irqnr,
>> >> > +   .write_eoi = gicv2_write_eoi,
>> >> > +};
>> >> > +
>> >> > +struct gic gicv3 = {
>> >> > +   .ipi = {
>> >> > +   .enable = gicv3_enable_defaults,
>> >> > +   .send_self = gicv3_ipi_send_self,
>> >> > +   .send_tlist = gicv3_ipi_send_tlist,
>> >> > +   .send_broadcast = gicv3_ipi_send_broadcast,
>> >> > +   },
>> >> > +   .read_iar = gicv3_read_iar,
>> >> > +   .irqnr = gicv3_irqnr,
>> >> > +   .write_eoi = gicv3_write_eoir,
>> >> > +};
>> >> > +
>> >>
>> >> So I was re-basing my kvm-unit-tests against your GIC rework and found
>> >> myself copy and pasting a bunch of this into my tests that fire IRQs.
>> >> That makes me think the abstraction should be in the library code so
>> >> other tests can fiddle with sending IRQs.
>> >>
>> >> What do you think?
>> >>
>> >
>> > I guess you mean moving the above two structs and their corresponding
>> > functions (all which aren't already common) to lib/arm/ ? Or do you
>> > just mean the one non-trivial function gicv3_ipi_send_tlist? I think
>> > agree with gicv3_ipi_send_tlist getting shared, but the others are
>> > mostly one-liners, so I'm not sure. I guess I'd have to see how you're
>> > using them first.
>>
>> So it looked like there were some functions in the common code for one
>> GIC which had local test defined functions for the other. They should at
>> least be consistent.
>
> gicv3_read_iar and gicv3_write_eoir being common already is a product of
> being sysreg wrappers, allowing for both arm32 and arm64 to use functions
> of the same names, not because I wanted gicv3 to be inconsistent with
> gicv2 (which uses MMIO and thus doesn't need wrappers)
>
>>
>> For my use case I could do with a common:
>>
>>   gic_enable
>
> OK, I can extend gic_init() to initialize a 'struct gic_common_ops' that
> includes an enable -> *_enable_defaults(void), ipi_send(int cpu),
> read_iar(void), iar_irqnr(u32 iar), and write_eoi(u32 irqstat). And also
> provide the wrappers gic_enable, gic_ipi_send(cpu), ...
>
>>   gic_send_spi(cpu, irq)
>
> I'll let you add this one to the new common ops struct :-)
>
>>   gic_irq_ack() which returns the iar.
>
> This one will be called read_iar.
>
> Would that work for you, Alex?

Sounds good to me :-)

>
> Andre,
>
> Would this also satisfy your needs for more common code?
>
> Thanks,
> drew
>
>>
>> See:
>>
>>   
>> https://github.com/stsquad/kvm-unit-tests/blob/mttcg/current-tests-v6/arm/tcg-test.c#L113
>>
>> >
>> > Thanks,
>> > drew
>>
>>
>> --
>> Alex Bennée
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests PATCH v5 10/11] arm/arm64: gicv3: add an IPI test

[Andrew Jones]

On Fri, Nov 11, 2016 at 10:02:59AM +, Alex Bennée wrote:
> 
> Andrew Jones  writes:
> 
> > On Thu, Nov 10, 2016 at 07:53:58PM +, Alex Bennée wrote:
> > [...]
> >> > +struct gic gicv2 = {
> >> > +.ipi = {
> >> > +.enable = gicv2_enable_defaults,
> >> > +.send_self = gicv2_ipi_send_self,
> >> > +.send_tlist = gicv2_ipi_send_tlist,
> >> > +.send_broadcast = gicv2_ipi_send_broadcast,
> >> > +},
> >> > +.read_iar = gicv2_read_iar,
> >> > +.irqnr = gicv2_irqnr,
> >> > +.write_eoi = gicv2_write_eoi,
> >> > +};
> >> > +
> >> > +struct gic gicv3 = {
> >> > +.ipi = {
> >> > +.enable = gicv3_enable_defaults,
> >> > +.send_self = gicv3_ipi_send_self,
> >> > +.send_tlist = gicv3_ipi_send_tlist,
> >> > +.send_broadcast = gicv3_ipi_send_broadcast,
> >> > +},
> >> > +.read_iar = gicv3_read_iar,
> >> > +.irqnr = gicv3_irqnr,
> >> > +.write_eoi = gicv3_write_eoir,
> >> > +};
> >> > +
> >>
> >> So I was re-basing my kvm-unit-tests against your GIC rework and found
> >> myself copy and pasting a bunch of this into my tests that fire IRQs.
> >> That makes me think the abstraction should be in the library code so
> >> other tests can fiddle with sending IRQs.
> >>
> >> What do you think?
> >>
> >
> > I guess you mean moving the above two structs and their corresponding
> > functions (all which aren't already common) to lib/arm/ ? Or do you
> > just mean the one non-trivial function gicv3_ipi_send_tlist? I think
> > agree with gicv3_ipi_send_tlist getting shared, but the others are
> > mostly one-liners, so I'm not sure. I guess I'd have to see how you're
> > using them first.
> 
> So it looked like there were some functions in the common code for one
> GIC which had local test defined functions for the other. They should at
> least be consistent.

gicv3_read_iar and gicv3_write_eoir being common already is a product of
being sysreg wrappers, allowing for both arm32 and arm64 to use functions
of the same names, not because I wanted gicv3 to be inconsistent with
gicv2 (which uses MMIO and thus doesn't need wrappers)

> 
> For my use case I could do with a common:
> 
>   gic_enable

OK, I can extend gic_init() to initialize a 'struct gic_common_ops' that
includes an enable -> *_enable_defaults(void), ipi_send(int cpu),
read_iar(void), iar_irqnr(u32 iar), and write_eoi(u32 irqstat). And also
provide the wrappers gic_enable, gic_ipi_send(cpu), ...

>   gic_send_spi(cpu, irq)

I'll let you add this one to the new common ops struct :-)

>   gic_irq_ack() which returns the iar.

This one will be called read_iar.

Would that work for you, Alex?

Andre,

Would this also satisfy your needs for more common code?

Thanks,
drew

> 
> See:
> 
>   
> https://github.com/stsquad/kvm-unit-tests/blob/mttcg/current-tests-v6/arm/tcg-test.c#L113
> 
> >
> > Thanks,
> > drew
> 
> 
> --
> Alex Bennée
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3 0/2] arm64: Support systems without FP/ASIMD

[Marc Zyngier]

On 08/11/16 13:56, Suzuki K Poulose wrote:
> This series adds supports to the kernel and KVM hyp to handle
> systems without FP/ASIMD properly. At the moment the kernel
> doesn't check if the FP unit is available before accessing
> the registers (e.g during context switch). Also for KVM,
> we trap the FP/ASIMD accesses and handle it by injecting an
> undefined instruction into the VM on systems without FP.
> 
> Tested on a FVP_Base-AEM-v8A model by disabling VFP on at
> least one CPU ( -C clusterX.cpuY.vfp-present=0 ).
> 
> Changes since V2:
>  - Dropped cleanup patch for arm64/crypto/aes-ce-ccm-glue.c
>  - Removed static_key check from cpus_have_cap. All users with
>constant caps should use the new API to make use of static_keys.
>  - Removed a dedicated static_key used in irqchip-gic-v3.c for
>Cavium errata with the new API.
> 
> Applies on v4.9-rc4 + [1] (which is pushed for rc5)
> 
> [1] http://marc.info/?l=linux-arm-kernel=147819889813214=2
> 
> 
> Suzuki K Poulose (2):
>   arm64: Add hypervisor safe helper for checking constant capabilities
>   arm64: Support systems without FP/ASIMD
> 
>  arch/arm64/include/asm/cpucaps.h|  3 ++-
>  arch/arm64/include/asm/cpufeature.h | 24 +---
>  arch/arm64/include/asm/neon.h   |  3 ++-
>  arch/arm64/kernel/cpufeature.c  | 17 -
>  arch/arm64/kernel/fpsimd.c  | 14 ++
>  arch/arm64/kernel/process.c |  2 +-
>  arch/arm64/kvm/handle_exit.c| 11 +++
>  arch/arm64/kvm/hyp/hyp-entry.S  |  9 -
>  arch/arm64/kvm/hyp/switch.c |  5 -
>  drivers/irqchip/irq-gic-v3.c| 13 +
>  10 files changed, 76 insertions(+), 25 deletions(-)
> 

For the series:

Reviewed-by: Marc Zyngier 

How do we plan on merging this? Catalin, are you willing to take it all?

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests PATCH v5 07/11] arm/arm64: gicv2: add an IPI test

[Andrew Jones]

On Fri, Nov 11, 2016 at 11:13:46AM +, Andre Przywara wrote:
> Hi,
> 
> more a comment loosely related to this patch ...
> 
> > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > index 3f6fa45c587e..68bf5cd6008f 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -54,3 +54,10 @@ file = selftest.flat
> >  smp = $MAX_SMP
> >  extra_params = -append 'smp'
> >  groups = selftest
> > +
> > +# Test GIC emulation
> > +[gicv2-ipi]
> > +file = gic.flat
> > +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> 
> So here we always go with the maximum number of VCPUs in the guest.
> However (as you also noted in your cover-letter) running with a
> different number of CPUs might be interesting, for instance with less
> than 8 CPUs on a GICv2 (the ITARGETSR register must be masked) or in
> general with an odd number (both literally and in the broader sense). I
> have a test case with passes with 8 VCPUs but fails with less.
> 
> Is there any good way to run some tests multiple times with different
> numbers of VCPUS?
> Shall we add some "set" functionality to the smp parameter, so that we
> can specify a list of desired test points?
>

We can just add multiple entries, e.g.

[gicv2-ipi]
file = gic.flat
smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
[gicv2-ipi-3]
file = gic.flat
smp = $((($MAX_SMP > 3)?3:$MAX_SMP))

or whatever. But we need to always consider MAX_SMP, since some
machines may less than 8.

Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v5 07/11] arm/arm64: gicv2: add an IPI test

[Andre Przywara]

Hi,

more a comment loosely related to this patch ...

> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 3f6fa45c587e..68bf5cd6008f 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -54,3 +54,10 @@ file = selftest.flat
>  smp = $MAX_SMP
>  extra_params = -append 'smp'
>  groups = selftest
> +
> +# Test GIC emulation
> +[gicv2-ipi]
> +file = gic.flat
> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))

So here we always go with the maximum number of VCPUs in the guest.
However (as you also noted in your cover-letter) running with a
different number of CPUs might be interesting, for instance with less
than 8 CPUs on a GICv2 (the ITARGETSR register must be masked) or in
general with an odd number (both literally and in the broader sense). I
have a test case with passes with 8 VCPUs but fails with less.

Is there any good way to run some tests multiple times with different
numbers of VCPUS?
Shall we add some "set" functionality to the smp parameter, so that we
can specify a list of desired test points?

Cheers,
Andre.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PULL 0/3] KVM/ARM updates for v4.9-rc4

[Paolo Bonzini]

On 04/11/2016 19:36, Marc Zyngier wrote:
>   git://git.kernel.org:/pub/scm/linux/kernel/git/kvmarm/kvmarm.git 
> tags/kvm-arm-for-v4.9-rc4

What is the extra colon after "org"? :)

Pulled now, and I will send it in time for rc5.

Thanks,

Paolo
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests PATCH v5 10/11] arm/arm64: gicv3: add an IPI test

[Alex Bennée]

Andrew Jones  writes:

> On Thu, Nov 10, 2016 at 07:53:58PM +, Alex Bennée wrote:
> [...]
>> > +struct gic gicv2 = {
>> > +  .ipi = {
>> > +  .enable = gicv2_enable_defaults,
>> > +  .send_self = gicv2_ipi_send_self,
>> > +  .send_tlist = gicv2_ipi_send_tlist,
>> > +  .send_broadcast = gicv2_ipi_send_broadcast,
>> > +  },
>> > +  .read_iar = gicv2_read_iar,
>> > +  .irqnr = gicv2_irqnr,
>> > +  .write_eoi = gicv2_write_eoi,
>> > +};
>> > +
>> > +struct gic gicv3 = {
>> > +  .ipi = {
>> > +  .enable = gicv3_enable_defaults,
>> > +  .send_self = gicv3_ipi_send_self,
>> > +  .send_tlist = gicv3_ipi_send_tlist,
>> > +  .send_broadcast = gicv3_ipi_send_broadcast,
>> > +  },
>> > +  .read_iar = gicv3_read_iar,
>> > +  .irqnr = gicv3_irqnr,
>> > +  .write_eoi = gicv3_write_eoir,
>> > +};
>> > +
>>
>> So I was re-basing my kvm-unit-tests against your GIC rework and found
>> myself copy and pasting a bunch of this into my tests that fire IRQs.
>> That makes me think the abstraction should be in the library code so
>> other tests can fiddle with sending IRQs.
>>
>> What do you think?
>>
>
> I guess you mean moving the above two structs and their corresponding
> functions (all which aren't already common) to lib/arm/ ? Or do you
> just mean the one non-trivial function gicv3_ipi_send_tlist? I think
> agree with gicv3_ipi_send_tlist getting shared, but the others are
> mostly one-liners, so I'm not sure. I guess I'd have to see how you're
> using them first.

So it looked like there were some functions in the common code for one
GIC which had local test defined functions for the other. They should at
least be consistent.

For my use case I could do with a common:

  gic_enable
  gic_send_spi(cpu, irq)
  gic_irq_ack() which returns the iar.

See:

  
https://github.com/stsquad/kvm-unit-tests/blob/mttcg/current-tests-v6/arm/tcg-test.c#L113

>
> Thanks,
> drew


--
Alex Bennée
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v5 10/11] arm/arm64: gicv3: add an IPI test

[Andre Przywara]

Hi,

On 10/11/16 19:53, Alex Bennée wrote:


> So I was re-basing my kvm-unit-tests against your GIC rework and found
> myself copy and pasting a bunch of this into my tests that fire IRQs.

So I take it you are working on (or already have) code to test SPIs,
probably via GICD_ISPENDR?
Just asking because I was thinking about going there and thus could save
my time if you are on it already...

> That makes me think the abstraction should be in the library code so
> other tests can fiddle with sending IRQs.

...because I was wondering the same.

Cheers,
Andre.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests PATCH v8 3/3] arm: pmu: Add CPI checking

[Andrew Jones]

On Tue, Nov 08, 2016 at 12:17:15PM -0600, Wei Huang wrote:
> From: Christopher Covington 
> 
> Calculate the numbers of cycles per instruction (CPI) implied by ARM
> PMU cycle counter values. The code includes a strict checking facility
> intended for the -icount option in TCG mode in the configuration file.
> 
> Signed-off-by: Christopher Covington 
> Signed-off-by: Wei Huang 
> ---
>  arm/pmu.c | 101 
> +-
>  arm/unittests.cfg |  14 
>  2 files changed, 114 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index d5e3ac3..09aff89 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -15,6 +15,7 @@
>  #include "libcflat.h"
>  
>  #define PMU_PMCR_E (1 << 0)
> +#define PMU_PMCR_C (1 << 2)
>  #define PMU_PMCR_N_SHIFT   11
>  #define PMU_PMCR_N_MASK0x1f
>  #define PMU_PMCR_ID_SHIFT  16
> @@ -75,6 +76,23 @@ static inline void pmccfiltr_write(uint32_t value)
>   pmselr_write(PMU_CYCLE_IDX);
>   pmxevtyper_write(value);
>  }
> +
> +/*
> + * Extra instructions inserted by the compiler would be difficult to 
> compensate
> + * for, so hand assemble everything between, and including, the PMCR accesses
> + * to start and stop counting.
> + */
> +static inline void loop(int i, uint32_t pmcr)

We should probably pick a more descriptive name for this function, as
we intend to add many more PMU tests to this file. While at it, let's
change 'i' to 'n', as it's the number of times to loop.

> +{
> + asm volatile(
> + "   mcr p15, 0, %[pmcr], c9, c12, 0\n"
> + "1: subs%[i], %[i], #1\n"
> + "   bgt 1b\n"
> + "   mcr p15, 0, %[z], c9, c12, 0\n"
> + : [i] "+r" (i)
> + : [pmcr] "r" (pmcr), [z] "r" (0)
> + : "cc");
> +}
>  #elif defined(__aarch64__)
>  static inline uint32_t pmcr_read(void)
>  {
> @@ -106,6 +124,23 @@ static inline void pmccfiltr_write(uint32_t value)
>  {
>   asm volatile("msr pmccfiltr_el0, %0" : : "r" (value));
>  }
> +
> +/*
> + * Extra instructions inserted by the compiler would be difficult to 
> compensate
> + * for, so hand assemble everything between, and including, the PMCR accesses
> + * to start and stop counting.
> + */
> +static inline void loop(int i, uint32_t pmcr)
> +{
> + asm volatile(
> + "   msr pmcr_el0, %[pmcr]\n"
> + "1: subs%[i], %[i], #1\n"
> + "   b.gt1b\n"
> + "   msr pmcr_el0, xzr\n"
> + : [i] "+r" (i)
> + : [pmcr] "r" (pmcr)
> + : "cc");
> +}
>  #endif
>  
>  /*
> @@ -156,8 +191,71 @@ static bool check_cycles_increase(void)
>   return true;
>  }
>  
> -int main(void)
> +/*
> + * Execute a known number of guest instructions. Only odd instruction counts
> + * greater than or equal to 3 are supported by the in-line assembly code. The
> + * control register (PMCR_EL0) is initialized with the provided value 
> (allowing
> + * for example for the cycle counter or event counters to be reset). At the 
> end
> + * of the exact instruction loop, zero is written to PMCR_EL0 to disable
> + * counting, allowing the cycle counter or event counters to be read at the
> + * leisure of the calling code.
> + */
> +static void measure_instrs(int num, uint32_t pmcr)
> +{
> + int i = (num - 1) / 2;
> +
> + assert(num >= 3 && ((num - 1) % 2 == 0));
> + loop(i, pmcr);
> +}
> +
> +/*
> + * Measure cycle counts for various known instruction counts. Ensure that the
> + * cycle counter progresses (similar to check_cycles_increase() but with more
> + * instructions and using reset and stop controls). If supplied a positive,
> + * nonzero CPI parameter, also strictly check that every measurement matches
> + * it. Strict CPI checking is used to test -icount mode.
> + */
> +static bool check_cpi(int cpi)
> +{
> + uint32_t pmcr = pmcr_read() | PMU_PMCR_C | PMU_PMCR_E;
> + 
> + if (cpi > 0)
> + printf("Checking for CPI=%d.\n", cpi);
> + printf("instrs : cycles0 cycles1 ...\n");
> +
> + for (int i = 3; i < 300; i += 32) {
> + int avg, sum = 0;
> +
> + printf("%d :", i);
> + for (int j = 0; j < NR_SAMPLES; j++) {
> + int cycles;
> +
> + measure_instrs(i, pmcr);
> + cycles =pmccntr_read();
^ missing space

> + printf(" %d", cycles);
> +
> + if (!cycles || (cpi > 0 && cycles != i * cpi)) {
> + printf("\n");
> + return false;
> + }
> +
> + sum += cycles;
> + }
> + avg = sum / NR_SAMPLES;
> + printf(" sum=%d avg=%d avg_ipc=%d avg_cpi=%d\n",
> + sum, avg, i / avg, avg / i);
> + }
> +
> + return true;
> +}
> +
> +int main(int argc, char *argv[])
>  {
> +