[kvm-unit-tests PATCH v9 3/3] arm: pmu: Add CPI checking

[Wei Huang]

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 | 111 +-
 arm/unittests.cfg |  14 +++
 2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index fa87de4..b36c4fb 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -104,6 +104,25 @@ static inline uint32_t id_dfr0_read(void)
asm volatile("mrc p15, 0, %0, c0, c1, 2" : "=r" (val));
return val;
 }
+
+/*
+ * 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(
+   "   mcr p15, 0, %[pmcr], c9, c12, 0\n"
+   "   isb\n"
+   "1: subs%[i], %[i], #1\n"
+   "   bgt 1b\n"
+   "   mcr p15, 0, %[z], c9, c12, 0\n"
+   "   isb\n"
+   : [i] "+r" (i)
+   : [pmcr] "r" (pmcr), [z] "r" (0)
+   : "cc");
+}
 #elif defined(__aarch64__)
 static inline uint32_t pmcr_read(void)
 {
@@ -150,6 +169,25 @@ static inline uint32_t id_dfr0_read(void)
asm volatile("mrs %0, id_dfr0_el1" : "=r" (id));
return id;
 }
+
+/*
+ * 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"
+   "   isb\n"
+   "1: subs%[i], %[i], #1\n"
+   "   b.gt1b\n"
+   "   msr pmcr_el0, xzr\n"
+   "   isb\n"
+   : [i] "+r" (i)
+   : [pmcr] "r" (pmcr)
+   : "cc");
+}
 #endif
 
 /*
@@ -204,6 +242,71 @@ static bool check_cycles_increase(void)
return success;
 }
 
+/*
+ * 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_LC | PMU_PMCR_C | PMU_PMCR_E;
+   
+   if (cpi > 0)
+   printf("Checking for CPI=%d.\n", cpi);
+   printf("instrs : cycles0 cycles1 ...\n");
+
+   for (unsigned int i = 3; i < 300; i += 32) {
+   uint64_t avg, sum = 0;
+
+   printf("%d :", i);
+   for (int j = 0; j < NR_SAMPLES; j++) {
+   uint64_t cycles;
+
+   pmccntr_write(0);
+   measure_instrs(i, pmcr);
+   cycles = pmccntr_read();
+   printf(" %"PRId64"", cycles);
+
+   /*
+* The cycles taken by the loop above should fit in
+* 32 bits easily. We check the upper 32 bits of the
+* cycle counter to make sure there is no supprise.
+*/
+   if (!cycles || (cpi > 0 && cycles != i * cpi) ||
+   (cycles & 0x)) {
+   printf("\n");
+   return false;
+   }
+
+   sum += cycles;
+   }
+   avg = sum / NR_SAMPLES;
+   printf(" sum=%"PRId64" avg=%"PRId64" avg_ipc=%"PRId64" "
+  "avg_cpi=%"PRId64"\n", sum, avg, i / avg, avg / i);
+   }
+
+   return true;
+}
+
 void pmu_init(void)
 {
uint32_t dfr0;
@@ -218,13 +321,19 @@ void pmu_init(void)
pmccfiltr_write(0); /* count cycles in EL0, EL1, but not EL2 */
 }
 
-int main(void)
+int main(int argc, char *argv[])

[kvm-unit-tests PATCH v9 2/3] arm: pmu: Check cycle count increases

[Wei Huang]

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 | 156 ++
 1 file changed, 156 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 9d9c53b..fa87de4 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -15,6 +15,9 @@
 #include "libcflat.h"
 #include "asm/barrier.h"
 
+#define PMU_PMCR_E (1 << 0)
+#define PMU_PMCR_C (1 << 2)
+#define PMU_PMCR_LC(1 << 6)
 #define PMU_PMCR_N_SHIFT   11
 #define PMU_PMCR_N_MASK0x1f
 #define PMU_PMCR_ID_SHIFT  16
@@ -22,6 +25,14 @@
 #define PMU_PMCR_IMP_SHIFT 24
 #define PMU_PMCR_IMP_MASK  0xff
 
+#define ID_DFR0_PERFMON_SHIFT 24
+#define ID_DFR0_PERFMON_MASK  0xf
+
+#define PMU_CYCLE_IDX 31
+
+#define NR_SAMPLES 10
+
+static unsigned int pmu_version;
 #if defined(__arm__)
 static inline uint32_t pmcr_read(void)
 {
@@ -30,6 +41,69 @@ 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));
+   isb();
+}
+
+static inline void pmselr_write(uint32_t value)
+{
+   asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));
+   isb();
+}
+
+static inline void pmxevtyper_write(uint32_t value)
+{
+   asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
+}
+
+static inline uint64_t pmccntr_read(void)
+{
+   uint32_t lo, hi = 0;
+
+   if (pmu_version == 0x3)
+   asm volatile("mrrc p15, 0, %0, %1, c9" : "=r" (lo), "=r" (hi));
+   else
+   asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (lo));
+
+   return ((uint64_t)hi << 32) | lo;
+}
+
+static inline void pmccntr_write(uint64_t value)
+{
+   uint32_t lo, hi;
+
+   lo = value & 0x;
+   hi = (value >> 32) & 0x;
+
+   if (pmu_version == 0x3)
+   asm volatile("mcrr p15, 0, %0, %1, c9" : : "r" (lo), "r" (hi));
+   else
+   asm volatile("mcr p15, 0, %0, c9, c13, 0" : : "r" (lo));
+}
+
+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);
+   isb();
+}
+
+static inline uint32_t id_dfr0_read(void)
+{
+   uint32_t val;
+
+   asm volatile("mrc p15, 0, %0, c0, c1, 2" : "=r" (val));
+   return val;
+}
 #elif defined(__aarch64__)
 static inline uint32_t pmcr_read(void)
 {
@@ -38,6 +112,44 @@ 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));
+   isb();
+}
+
+static inline uint64_t pmccntr_read(void)
+{
+   uint64_t cycles;
+
+   asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
+   return cycles;
+}
+
+static inline void pmccntr_write(uint64_t value)
+{
+   asm volatile("msr pmccntr_el0, %0" : : "r" (value));
+}
+
+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));
+   isb();
+}
+
+static inline uint32_t id_dfr0_read(void)
+{
+   uint32_t id;
+
+   asm volatile("mrs %0, id_dfr0_el1" : "=r" (id));
+   return id;
+}
 #endif
 
 /*
@@ -64,11 +176,55 @@ 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)
+{
+   bool success = true;
+
+   pmccntr_write(0);
+   pmcr_write(pmcr_read() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
+
+   for (int i = 0; i < NR_SAMPLES; i++) {
+   uint64_t a, b;
+
+   a = pmccntr_read();
+   b = pmccntr_read();
+
+   if (a >= b) {
+   printf("Read %"PRId64" then %"PRId64".\n", a, b);
+   success = false;
+   break;
+   }
+   }
+
+   pmcr_write(pmcr_read() & ~PMU_PMCR_E);
+
+   return success;
+}
+
+void pmu_init(void)
+{
+   uint32_t dfr0;
+
+   /* probe pmu version */
+   dfr0 = id_dfr0_read();
+   pmu_version = (dfr0 >> ID_DFR0_PERFMON_SHIFT) & ID_DFR0_PERFMON_MASK;
+   printf("PMU version: %d\n", pmu_version);
+   
+   /* init for PMU event access, right now only care about cycle 

[kvm-unit-tests PATCH v9 1/3] arm: Add PMU test

[Wei Huang]

From: Christopher Covington 

Beginning with a simple sanity check of the control register, add
a unit test for the ARM Performance Monitors Unit (PMU).

Signed-off-by: Christopher Covington 
Signed-off-by: Wei Huang 
Reviewed-by: Andrew Jones 
---
 arm/Makefile.common |  3 ++-
 arm/pmu.c   | 74 +
 arm/unittests.cfg   |  5 
 3 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 arm/pmu.c

diff --git a/arm/Makefile.common b/arm/Makefile.common
index ccb554d..f98f422 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -11,7 +11,8 @@ endif
 
 tests-common = \
$(TEST_DIR)/selftest.flat \
-   $(TEST_DIR)/spinlock-test.flat
+   $(TEST_DIR)/spinlock-test.flat \
+   $(TEST_DIR)/pmu.flat
 
 all: test_cases
 
diff --git a/arm/pmu.c b/arm/pmu.c
new file mode 100644
index 000..9d9c53b
--- /dev/null
+++ b/arm/pmu.c
@@ -0,0 +1,74 @@
+/*
+ * Test the ARM Performance Monitors Unit (PMU).
+ *
+ * Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 2.1 and
+ * only version 2.1 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
+ * for more details.
+ */
+#include "libcflat.h"
+#include "asm/barrier.h"
+
+#define PMU_PMCR_N_SHIFT   11
+#define PMU_PMCR_N_MASK0x1f
+#define PMU_PMCR_ID_SHIFT  16
+#define PMU_PMCR_ID_MASK   0xff
+#define PMU_PMCR_IMP_SHIFT 24
+#define PMU_PMCR_IMP_MASK  0xff
+
+#if defined(__arm__)
+static inline uint32_t pmcr_read(void)
+{
+   uint32_t ret;
+
+   asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
+   return ret;
+}
+#elif defined(__aarch64__)
+static inline uint32_t pmcr_read(void)
+{
+   uint32_t ret;
+
+   asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
+   return ret;
+}
+#endif
+
+/*
+ * As a simple sanity check on the PMCR_EL0, ensure the implementer field isn't
+ * null. Also print out a couple other interesting fields for diagnostic
+ * purposes. For example, as of fall 2016, QEMU TCG mode doesn't implement
+ * event counters and therefore reports zero event counters, but hopefully
+ * support for at least the instructions event will be added in the future and
+ * the reported number of event counters will become nonzero.
+ */
+static bool check_pmcr(void)
+{
+   uint32_t pmcr;
+
+   pmcr = pmcr_read();
+
+   printf("PMU implementer: %c\n",
+  (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK);
+   printf("Identification code: 0x%x\n",
+  (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK);
+   printf("Event counters:  %d\n",
+  (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK);
+
+   return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0;
+}
+
+int main(void)
+{
+   report_prefix_push("pmu");
+
+   report("Control register", check_pmcr());
+
+   return report_summary();
+}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 3f6fa45..7645180 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -54,3 +54,8 @@ file = selftest.flat
 smp = $MAX_SMP
 extra_params = -append 'smp'
 groups = selftest
+
+# Test PMU support
+[pmu]
+file = pmu.flat
+groups = pmu
-- 
1.8.3.1

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

[kvm-unit-tests PATCH v9 0/3] ARM PMU tests

[Wei Huang]

Changes from v8:
* Probe PMU version based on ID_DFR0
* pmccntr_read() now returns 64bit and can handle both 32bit and 64bit
  PMCCNTR based on PMU version.
* Add pmccntr_write() support
* Use a common printf format PRId64 to support 64bit variable smoothly in
  test functions
* Add barriers to several PMU write functions
* Verfied on different execution modes

Note:
1) Current KVM code has bugs in handling PMCCFILTR write. A fix (see
below) is required for this unit testing code to work correctly under
KVM mode.
https://lists.cs.columbia.edu/pipermail/kvmarm/2016-November/022134.html.

Thanks,
-Wei

Wei Huang (3):
  arm: Add PMU test
  arm: pmu: Check cycle count increases
  arm: pmu: Add CPI checking

 arm/Makefile.common |   3 +-
 arm/pmu.c   | 339 
 arm/unittests.cfg   |  19 +++
 3 files changed, 360 insertions(+), 1 deletion(-)
 create mode 100644 arm/pmu.c

-- 
1.8.3.1

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

Re: [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access

[Vijay Kilari]

On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
 wrote:
> On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
>> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
>>  wrote:
>> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kil...@gmail.com wrote:
>> >> From: Vijaya Kumar K 
>> >>
>> >> VGICv3 CPU interface registers are accessed using
>> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>> >> as 64-bit. The cpu MPIDR value is passed along with register id.
>> >> is used to identify the cpu for registers access.
>> >>
>> >> The version of VGIC v3 specification is define here
>> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>> >>
>> >> Signed-off-by: Pavel Fedin 
>> >> Signed-off-by: Vijaya Kumar K 
>> >> ---
>> >>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
>> >>  arch/arm64/kvm/Makefile |   1 +
>> >>  include/kvm/arm_vgic.h  |   9 +
>> >>  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 +++
>> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  19 +++
>> >>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 
>> >> 
>> >>  virt/kvm/arm/vgic/vgic-v3.c |   8 +
>> >>  virt/kvm/arm/vgic/vgic.h|   4 +
>> >>  8 files changed, 395 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> >> b/arch/arm64/include/uapi/asm/kvm.h
>> >> index 56dc08d..91c7137 100644
>> >> --- a/arch/arm64/include/uapi/asm/kvm.h
>> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
>> >>   (0xULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
>> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
>> >> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>> >> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0x)
>> >>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
>> >>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
>> >>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS6
>> >> +
>> >>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
>> >>
>> >>  /* Device Control API on vcpu fd */
>> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> >> index d50a82a..1a14e29 100644
>> >> --- a/arch/arm64/kvm/Makefile
>> >> +++ b/arch/arm64/kvm/Makefile
>> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += 
>> >> $(KVM)/arm/vgic/vgic-mmio-v3.o
>> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
>> >
>> > Thi is making me wonder:  Are we properly handling GICv3 save/restore
>> > for AArch32 now that we have GICv3 support for AArch32?  By properly I
>> > mean that either it is clearly only supported on AArch64 systems or it's
>> > supported on both AArch64 and AArch32, but it shouldn't break randomly
>> > on AArch32.
>>
>> It supports both AArch64 and AArch64 in handling of system registers
>> save/restore.
>> All system registers that we save/restore are 32-bit for both aarch64
>> and aarch32.
>> Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
>> are same. However the codes sent by qemu is matched and register
>> are handled properly irrespective of AArch32 or AArch64.
>>
>> I don't have platform which support AArch32 guests to verify.
>
> Actually this is not about the guest, it's about an ARMv8 AArch32 host
> that has a GICv3.
>
> I just tried to do a v7 compile with your patches, and it results in an
> epic failure, so there's something for you to look at.
>

Could you please share you config file?. I tried with multi_v7 defconfig with
CONFIG KVM and CONFIG_KVM_ARM_HOST enabled. it compiled for me.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access

[Vijay Kilari]

On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
 wrote:
> On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
>> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
>>  wrote:
>> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kil...@gmail.com wrote:
>> >> From: Vijaya Kumar K 
>> >>
>> >> VGICv3 CPU interface registers are accessed using
>> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>> >> as 64-bit. The cpu MPIDR value is passed along with register id.
>> >> is used to identify the cpu for registers access.
>> >>
>> >> The version of VGIC v3 specification is define here
>> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>> >>
>> >> Signed-off-by: Pavel Fedin 
>> >> Signed-off-by: Vijaya Kumar K 
>> >> ---
>> >>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
>> >>  arch/arm64/kvm/Makefile |   1 +
>> >>  include/kvm/arm_vgic.h  |   9 +
>> >>  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 +++
>> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  19 +++
>> >>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 
>> >> 
>> >>  virt/kvm/arm/vgic/vgic-v3.c |   8 +
>> >>  virt/kvm/arm/vgic/vgic.h|   4 +
>> >>  8 files changed, 395 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> >> b/arch/arm64/include/uapi/asm/kvm.h
>> >> index 56dc08d..91c7137 100644
>> >> --- a/arch/arm64/include/uapi/asm/kvm.h
>> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
>> >>   (0xULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
>> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
>> >> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>> >> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0x)
>> >>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
>> >>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
>> >>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS6
>> >> +
>> >>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
>> >>
>> >>  /* Device Control API on vcpu fd */
>> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> >> index d50a82a..1a14e29 100644
>> >> --- a/arch/arm64/kvm/Makefile
>> >> +++ b/arch/arm64/kvm/Makefile
>> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += 
>> >> $(KVM)/arm/vgic/vgic-mmio-v3.o
>> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
>> >
>> > Thi is making me wonder:  Are we properly handling GICv3 save/restore
>> > for AArch32 now that we have GICv3 support for AArch32?  By properly I
>> > mean that either it is clearly only supported on AArch64 systems or it's
>> > supported on both AArch64 and AArch32, but it shouldn't break randomly
>> > on AArch32.
>>
>> It supports both AArch64 and AArch64 in handling of system registers
>> save/restore.
>> All system registers that we save/restore are 32-bit for both aarch64
>> and aarch32.
>> Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
>> are same. However the codes sent by qemu is matched and register
>> are handled properly irrespective of AArch32 or AArch64.
>>
>> I don't have platform which support AArch32 guests to verify.
>
> Actually this is not about the guest, it's about an ARMv8 AArch32 host
> that has a GICv3.
>
> I just tried to do a v7 compile with your patches, and it results in an
> epic failure, so there's something for you to look at.
>
>> >
>> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>> >>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> >> index 002f092..730a18a 100644
>> >> --- a/include/kvm/arm_vgic.h
>> >> +++ b/include/kvm/arm_vgic.h
>> >> @@ -71,6 +71,9 @@ struct vgic_global {
>> >>
>> >>   /* GIC system register CPU interface */
>> >>   struct static_key_false gicv3_cpuif;
>> >> +
>> >> + /* Cache ICH_VTR_EL2 reg value */
>> >> + u32 ich_vtr_el2;
>> >>  };
>> >>
>> >>  extern struct vgic_global kvm_vgic_global_state;
>> >> @@ -269,6 +272,12 @@ struct vgic_cpu {
>> >>   u64 pendbaser;
>> >>
>> >>   bool lpis_enabled;
>> >> +
>> >> + /* Cache guest priority bits */
>> >> + u32 num_pri_bits;
>> >> +
>> >> + /* Cache guest interrupt ID bits */
>> >> + u32 num_id_bits;
>> >>  };
>> >>
>> >>  extern struct static_key_false vgic_v2_cpuif_trap;
>> >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c 
>> >> b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> >> index 6c7d30c..da532d1 100644
>> >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
>> >> +++ 

Re: [kvm-unit-tests PATCH] arm/arm64: introduce is_aarch32

[Christopher Covington]

On 11/17/2016 11:59 AM, Andrew Jones wrote:
> On Wed, Nov 16, 2016 at 05:02:59PM -0500, Christopher Covington wrote:
>> On 11/16/2016 12:46 PM, Marc Zyngier wrote:
>>> On 16/11/16 14:38, Andrew Jones wrote:
 ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
 function allows unit tests to make the distinction.
>>>
>>> Hi Drew,
>>>
>>> Overall, having to find out about the architecture is a bad idea most of
>>> the time. We have feature registers for most things, and it definitely
>>> makes more sense to check for those than trying to cast a wider net.
>>>
 Signed-off-by: Andrew Jones 

 ---
 I'm actually unsure if there's a feature bit or not that I could
 probe instead. It'd be nice if somebody can confirm. Thanks, drew
>>
>> I'd be happy to settle with the hard-coded CPU list.
>>
>> But if you're curious about alternatives, I've taken a look through some
>> documentation. ID_ISAR0.coproc describes whether mrrc is available but
>> I think it is generally available on v7 and above. I think ID_ISAR5 will
>> be zero on v7 and nonzero on v8-A32. But PMCR.LC seems like the best bit
>> to check.
>>
 diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
 index 84d5c7ce752b..b602e1fbbc2d 100644
 --- a/lib/arm64/asm/processor.h
 +++ b/lib/arm64/asm/processor.h
 @@ -81,5 +81,10 @@ DEFINE_GET_SYSREG32(mpidr)
  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
 sp_usr);
  extern bool is_user(void);
  
 +static inline bool is_aarch32(void)
 +{
 +  return false;
 +}
 +
  #endif /* !__ASSEMBLY__ */
  #endif /* _ASMARM64_PROCESSOR_H_ */

>>>
>>> So the real question is: what are you trying to check for?
>>
>> The question is "how many bits wide is pmccntr?" I think we
>> can test whether writing PMCR.LC = 1 sticks. Based on the
>> documentation, it seems to me like it wouldn't for v7 and
>> would for v8-A32.
>>
>> uint8_t size_pmccntr(void) {
>>   uint32_t pmcr = get_pmcr();
>>   if (pmcr & PMU_PMCR_LC_MASK)
>> return 64;
>>   set_pmcr(pmcr | (1 << PMU_PMCR_LC_SHIFT));
>>   if (get_pmcr() & PMU_PMCR_LC_MASK)
>> return 64;
>>   return 32;
>> }
> 
> Thanks for this Cov. I mostly agree with the proposal, except for two
> reasons; the spec says the bit is UNK/SBZP for v7, which doesn't give
> me 100% confidence that we can rely on it behaving a certain way. And,

The v7 definition of UNK/SBZP (page Glossary-2736 of ARM DDI 0406C.c):

Hardware must ... Read-As-Zero, and must ignore writes

> I'd rather not rely on the emulation we're testing to work well enough
> that we can probe it.

I agree that it'd really be best to test that QEMU is behaving correctly
rather than assume it does. Real production hardware has passed the ARM
Architecture Validation Suite, but QEMU hasn't.

> For our PMU testing purposes there is a somewhat ugly, but foolproof
> way to manage it; we could just add a command line input that says
> we're aarch32 (-cpu host,aarch64=off -append aarch32)

I would group ARMv7 versus ARMv8-A32 as an "implementation defined"
choice. There are hundreds of these in the architecture, and many of
them require tests to behave differently. For example AES support:

if (DEVICE_SUPPORTS(AES)) {
  assert(ID_ISAR5.AES == 0001 || ID_ISAR5.AES == 0010);
  /* use some AES instructions and check the results */
} else {
  assrt(ID_ISAR5.AES == 0);
  /* maybe check that AES instructions throw undefined
 instruction exceptions */
}

I'm starting to most prefer the original suggestion, which I'll
reframe as built-in tables of implementation defined options, using
the MIDR (and auxiliary ID registers if necessary) to select which
table to use. That way the tests only have to rely on MIDR being
properly emulated, and can check all the other behavior step by step.

Cov

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests PATCH 4/4] arm/arm64: GICv3: add TYPER test

[Andrew Jones]

On Thu, Nov 17, 2016 at 05:57:52PM +, Andre Przywara wrote:
> Add a simple test for the GICv3 TYPER test, which does only one basic
> check to ensure we have actually enough interrupt IDs if we support
> LPIs.
> Allow a GICv3 guest to do the common MMIO checks as well, where the
> register semantics are shared with a GICv2.
> 
> Signed-off-by: Andre Przywara 
> ---
>  arm/gic.c | 34 +++---
>  arm/unittests.cfg |  6 ++
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 02b1be1..7de0e47 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -327,6 +327,30 @@ static bool test_typer_v2(uint32_t reg)
>   return true;
>  }
>  
> +static bool test_typer_v3(uint32_t reg)
> +{
> + int nr_intids;
> +
> + report("GIC emulation %ssupport%s MBIs", 1,
> +reg & BIT(16) ? "" : "does not ",
> +reg & BIT(16) ? "s" : "");

Could just do the test once

 ("...%s...",  reg & BIT(16) ? "supports" : "does not support"

> + report("GIC emulation %ssupport%s LPIs", 1,
> +reg & BIT(17) ? "" : "does not ",
> +reg & BIT(17) ? "s" : "");
> + report("GIC emulation %ssupport%s Aff3", 1,
> +reg & BIT(24) ? "" : "does not ",
> +reg & BIT(24) ? "s" : "");
> +
> + nr_intids = BIT(((reg >> 19) & 0x1f) + 1);
> + report("%d interrupt IDs implemented", 1, nr_intids);
> +
> + if (reg & BIT(17))
> + report("%d LPIs supported", nr_intids > 8192,
> +nr_intids > 8192 ? nr_intids - 8192 : 0);

I'm wondering if we should try to keep the number of report lines
the same host to host. So anywhere we can't do a PASS/FAIL test we
should do a SKIP. Doing that will allow us to cleanly diff test
results between hosts. (I'm not sure I've been doing a good job of
that with the existing tests though...)

> +
> + return true;

No need to return a value.

> +}
> +
>  #define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
>  #define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) 
> |\
>   ((new) << ((byte) * 8)))
> @@ -460,8 +484,9 @@ static int gic_test_mmio(int gic_version)
>   idreg = gic_dist_base + 0xfe8;
>   break;
>   case 0x3:
> - report_abort("GICv3 MMIO tests NYI");
> - return -1;
> + gic_dist_base = gicv3_dist_base();
> + idreg = gic_dist_base + 0xffe8;

No define for this ID reg?

> + break;
>   default:
>   report_abort("GIC version %d not supported", gic_version);
>   return 0;
> @@ -471,7 +496,10 @@ static int gic_test_mmio(int gic_version)
>   nr_irqs = 32 * ((reg & 0x1f) + 1);
>   report("number of implemented SPIs: %d", 1, nr_irqs - 32);
>  
> - test_typer_v2(reg);
> + if (gic_version == 2)
> + test_typer_v2(reg);
> + else
> + test_typer_v3(reg);

Maybe we should use a switch here too, preparing for v4

>  
>   report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR));
>  
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 0162e5a..b432346 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -78,3 +78,9 @@ file = gic.flat
>  smp = $MAX_SMP
>  extra_params = -machine gic-version=3 -append 'ipi'
>  groups = gic
> +
> +[gicv3-mmio]
> +file = gic.flat
> +smp = $MAX_SMP
> +extra_params = -machine gic-version=3 -append 'mmio'
> +groups = gic
> -- 
> 2.9.0
> 
>

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

Re: [Qemu-devel] [kvm-unit-tests PATCH 1/4] arm/arm64: GIC: basic GICv2 MMIO tests

[Andre Przywara]

Hi Drew,

On 18/11/16 13:06, Andrew Jones wrote:
> Hi Andre,
> 
> I'm so pleased to see this series. Thank you!
> 
> On Thu, Nov 17, 2016 at 05:57:49PM +, Andre Przywara wrote:
>> This adds an MMIO subtest to the GIC test.
>> It accesses some generic GICv2 registers and does some sanity tests,
>> like checking for some of them being read-only.
>>
>> Signed-off-by: Andre Przywara 
>> ---
>>  arm/gic.c | 99 
>> +++
>>  arm/unittests.cfg |  6 
>>  lib/arm/asm/gic.h |  2 ++
>>  3 files changed, 107 insertions(+)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index 638b8b1..ba2585b 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -3,6 +3,7 @@
>>   *
>>   * GICv2
>>   *   + test sending/receiving IPIs
>> + *   + MMIO access tests
>>   * GICv3
>>   *   + test sending/receiving IPIs
>>   *
>> @@ -274,6 +275,98 @@ static struct gic gicv3 = {
>>  },
>>  };
>>  
>> +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig)
>> +{
>> +u32 reg;
>> +
>> +writel(pattern, address);
>> +reg = readl(address);
>> +
>> +if (reg != orig)
>> +writel(orig, address);
>> +
>> +return reg == orig;
>> +}
>> +
>> +static bool test_readonly_32(void *address, bool razwi)
>> +{
>> +u32 orig, pattern;
>> +
>> +orig = readl(address);
>> +if (razwi && orig)
>> +return false;
>> +
>> +pattern = 0x;
>> +if (orig != pattern) {
>> +if (!test_ro_pattern_32(address, pattern, orig))
>> +return false;
>> +}
>> +
>> +pattern = 0xa5a55a5a;
>> +if (orig != pattern) {
>> +if (!test_ro_pattern_32(address, pattern, orig))
>> +return false;
>> +}
>> +
>> +pattern = 0;
>> +if (orig != pattern) {
>> +if (!test_ro_pattern_32(address, pattern, orig))
>> +return false;
>> +}
>> +
>> +return true;
>> +}
>> +
>> +static bool test_typer_v2(uint32_t reg)
>> +{
>> +int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
>> +
>> +report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
>> +   nr_gic_cpus);
>> +
>> +return true;
> 
> This test function can be a void.
> 
>> +}
>> +
>> +static int gic_test_mmio(int gic_version)
>> +{
>> +u32 reg;
>> +int nr_irqs;
>> +void *gic_dist_base, *idreg;
>> +
>> +switch(gic_version) {
>> +case 0x2:
>> +gic_dist_base = gicv2_dist_base();
>> +idreg = gic_dist_base + 0xfe8;
> 
> I see below you introduce GICD_ICPIDR2, so I guess you can use it here.
> 
>> +break;
>> +case 0x3:
>> +report_abort("GICv3 MMIO tests NYI");
>> +return -1;
> 
> can't reach this return

But we need to tell GCC about this, because otherwise we get all kind of
warnings (including bogus "maybe unused" warnings).
__attribute__ ((noreturn)) seems the way to go here, but this is
currently giving me a hard time ...

>> +default:
>> +report_abort("GIC version %d not supported", gic_version);
>> +return 0;
> 
> can't reach this return
> 
>> +}
>> +
>> +reg = readl(gic_dist_base + GICD_TYPER);
>> +nr_irqs = 32 * ((reg & 0x1f) + 1);
> 
> Any reason to avoid using GICD_TYPER_IRQS() here?

On the first write I wasn't aware of it, on a second thought then I
wanted to avoid using the macro copied from Linux.

But you are right, I will use it here.

> 
>> +report("number of implemented SPIs: %d", 1, nr_irqs - 32);
> 
> We usually just use printf for informational output (but we should
> probably add a 'report_info' in order to keep the prefixes. I can
> do that now.) Anyway, please s/1/true

I saw your patch, will use that.

>> +
>> +test_typer_v2(reg);
>> +
>> +report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR));
>> +
>> +report("GICD_TYPER is read-only",
>> +   test_readonly_32(gic_dist_base + GICD_TYPER, false));
>> +report("GICD_IIDR is read-only",
>> +   test_readonly_32(gic_dist_base + GICD_IIDR, false));
>> +
>> +reg = readl(idreg);
>> +report("ICPIDR2 is read-only (0x%x)",
>> +   test_readonly_32(idreg, false),
>> +   reg);
>> +
>> +return 0;
> 
> You may want %08x for all your register printing.
> 
> Since you either abort or always return success, then this function can be
> a void.
> 
>> +}
>> +
>>  int main(int argc, char **argv)
>>  {
>>  char pfx[8];
>> @@ -332,6 +425,12 @@ int main(int argc, char **argv)
>>  }
>>  ipi_test();
>>  
>> +} else if (!strcmp(argv[1], "mmio")) {
>> +report_prefix_push(argv[1]);
>> +
>> +gic_test_mmio(gic_version());
> 
> Any reason to pass gic_version() here instead of just using it
> in gic_test_mmio?

Not really, I originally wanted to pass this variable on in a clean
fashion to allow sharing tests.
But using the function shouldn't make any difference anymore, so I can

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

[Andrew Jones]

On Thu, Nov 17, 2016 at 05:57:50PM +, Andre Przywara wrote:
> Some tests for the IPRIORITY registers. The significant number of bits
> is IMPLEMENTATION DEFINED, but should be the same for every IRQ.
> Also these registers must be byte-accessible.
> Check that accesses beyond the implemented IRQ limit are actually
> read-as-zero/write-ignore.
> 
> Signed-off-by: Andre Przywara 
> ---
>  arm/gic.c | 72 
> +++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index ba2585b..a27da2c 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -327,6 +327,76 @@ static bool test_typer_v2(uint32_t reg)
>   return true;
>  }
>  
> +#define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
> +#define REPLACE_BYTE(reg32, byte, new) (((reg32) & ~(0xff << ((byte) * 8))) 
> |\
> + ((new) << ((byte) * 8)))
> +
> +static bool test_priorities(int nr_irqs, void *priptr)
> +{
> + u32 orig_prio, reg, pri_bits;
> + u32 pri_mask, pattern;
> +
> + orig_prio = readl(priptr + 32);
> + report_prefix_push("IPRIORITYR");
> +
> + /*
> +  * Determine implemented number of priority bits by writing all 1's
> +  * and checking the number of cleared bits in the value read back.
> +  */
> + writel(0x, priptr + 32);
> + pri_mask = readl(priptr + 32);
> +
> + reg = ~pri_mask;
> + report("consistent priority masking (0x%08x)",
> +(((reg >> 16) == (reg & 0x)) &&
> + ((reg & 0xff) == ((reg >> 8) & 0xff))), pri_mask);
> +
> + reg = reg & 0xff;
> + for (pri_bits = 8; reg & 1; reg >>= 1, pri_bits--)
> + ;
> + report("implements at least 4 priority bits (%d)",
> +pri_bits >= 4, pri_bits);
> +
> + pattern = 0;
> + writel(pattern, priptr + 32);
> + report("clearing priorities", readl(priptr + 32) == pattern);
> +
> + pattern = 0x;
> + writel(pattern, priptr + 32);
> + report("filling priorities",
> +readl(priptr + 32) == (pattern & pri_mask));
> +
> + report("accesses beyond limit RAZ/WI",
> +test_readonly_32(priptr + nr_irqs, true));
> +
> + writel(pattern, priptr + nr_irqs - 4);
> + report("accessing last SPIs",
> +readl(priptr + nr_irqs - 4) == (pattern & pri_mask));
> +
> + pattern = 0xff7fbf3f;
> + writel(pattern, priptr + 32);
> + report("priorities are preserved",
> +readl(priptr + 32) == (pattern & pri_mask));
> +
> + /*
> +  * The PRIORITY registers are byte accessible, do a byte-wide
> +  * read and write of known content to check for this.
> +  */
> + reg = readb(priptr + 33);
> + report("byte reads successful (0x%08x => 0x%02x)",
> +reg == (BYTE(pattern, 1) & pri_mask), pattern & pri_mask, reg);
> +
> + pattern = REPLACE_BYTE(pattern, 2, 0x1f);
> + writeb(BYTE(pattern, 2), priptr + 34);
> + reg = readl(priptr + 32);
> + report("byte writes successful (0x%02x => 0x%08x)",
> +reg == (pattern & pri_mask), BYTE(pattern, 2) & pri_mask, reg);
> +
> + report_prefix_pop();
> + writel(orig_prio, priptr + 32);
> + return true;

Might be nice to have FIRST_SPI and maybe LAST_SPI macros to avoid all
the +32's

This function always returns true, so it can be a void.

> +}
> +
>  static int gic_test_mmio(int gic_version)
>  {
>   u32 reg;
> @@ -364,6 +434,8 @@ static int gic_test_mmio(int gic_version)
>  test_readonly_32(idreg, false),
>  reg);
>  
> + test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);

Feel free to add state like nr_irqs and dist_base to the gic struct
defined in arm/gic.c. That struct should provide the abstraction
needed to handle both gicv2 and gicv3 and contain anything that the
test functions need to refer to frequently. Using it should help
reduce the amount of parameters passed around.

> +
>   return 0;
>  }
>  
> -- 
> 2.9.0

Otherwise looks good to me

Reviewed-by: Andrew Jones 
> 
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests PATCH 1/4] arm/arm64: GIC: basic GICv2 MMIO tests

[Andrew Jones]

Hi Andre,

I'm so pleased to see this series. Thank you!

On Thu, Nov 17, 2016 at 05:57:49PM +, Andre Przywara wrote:
> This adds an MMIO subtest to the GIC test.
> It accesses some generic GICv2 registers and does some sanity tests,
> like checking for some of them being read-only.
> 
> Signed-off-by: Andre Przywara 
> ---
>  arm/gic.c | 99 
> +++
>  arm/unittests.cfg |  6 
>  lib/arm/asm/gic.h |  2 ++
>  3 files changed, 107 insertions(+)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 638b8b1..ba2585b 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -3,6 +3,7 @@
>   *
>   * GICv2
>   *   + test sending/receiving IPIs
> + *   + MMIO access tests
>   * GICv3
>   *   + test sending/receiving IPIs
>   *
> @@ -274,6 +275,98 @@ static struct gic gicv3 = {
>   },
>  };
>  
> +static bool test_ro_pattern_32(void *address, u32 pattern, u32 orig)
> +{
> + u32 reg;
> +
> + writel(pattern, address);
> + reg = readl(address);
> +
> + if (reg != orig)
> + writel(orig, address);
> +
> + return reg == orig;
> +}
> +
> +static bool test_readonly_32(void *address, bool razwi)
> +{
> + u32 orig, pattern;
> +
> + orig = readl(address);
> + if (razwi && orig)
> + return false;
> +
> + pattern = 0x;
> + if (orig != pattern) {
> + if (!test_ro_pattern_32(address, pattern, orig))
> + return false;
> + }
> +
> + pattern = 0xa5a55a5a;
> + if (orig != pattern) {
> + if (!test_ro_pattern_32(address, pattern, orig))
> + return false;
> + }
> +
> + pattern = 0;
> + if (orig != pattern) {
> + if (!test_ro_pattern_32(address, pattern, orig))
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static bool test_typer_v2(uint32_t reg)
> +{
> + int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
> +
> + report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
> +nr_gic_cpus);
> +
> + return true;

This test function can be a void.

> +}
> +
> +static int gic_test_mmio(int gic_version)
> +{
> + u32 reg;
> + int nr_irqs;
> + void *gic_dist_base, *idreg;
> +
> + switch(gic_version) {
> + case 0x2:
> + gic_dist_base = gicv2_dist_base();
> + idreg = gic_dist_base + 0xfe8;

I see below you introduce GICD_ICPIDR2, so I guess you can use it here.

> + break;
> + case 0x3:
> + report_abort("GICv3 MMIO tests NYI");
> + return -1;

can't reach this return

> + default:
> + report_abort("GIC version %d not supported", gic_version);
> + return 0;

can't reach this return

> + }
> +
> + reg = readl(gic_dist_base + GICD_TYPER);
> + nr_irqs = 32 * ((reg & 0x1f) + 1);

Any reason to avoid using GICD_TYPER_IRQS() here?

> + report("number of implemented SPIs: %d", 1, nr_irqs - 32);

We usually just use printf for informational output (but we should
probably add a 'report_info' in order to keep the prefixes. I can
do that now.) Anyway, please s/1/true

> +
> + test_typer_v2(reg);
> +
> + report("IIDR: 0x%x", 1, readl(gic_dist_base + GICD_IIDR));
> +
> + report("GICD_TYPER is read-only",
> +test_readonly_32(gic_dist_base + GICD_TYPER, false));
> + report("GICD_IIDR is read-only",
> +test_readonly_32(gic_dist_base + GICD_IIDR, false));
> +
> + reg = readl(idreg);
> + report("ICPIDR2 is read-only (0x%x)",
> +test_readonly_32(idreg, false),
> +reg);
> +
> + return 0;

You may want %08x for all your register printing.

Since you either abort or always return success, then this function can be
a void.

> +}
> +
>  int main(int argc, char **argv)
>  {
>   char pfx[8];
> @@ -332,6 +425,12 @@ int main(int argc, char **argv)
>   }
>   ipi_test();
>  
> + } else if (!strcmp(argv[1], "mmio")) {
> + report_prefix_push(argv[1]);
> +
> + gic_test_mmio(gic_version());

Any reason to pass gic_version() here instead of just using it
in gic_test_mmio?

> +
> + report_prefix_pop();
>   } else {
>   report_abort("Unknown subtest '%s'", argv[1]);
>   }
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index c7392c7..0162e5a 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -67,6 +67,12 @@ smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
>  extra_params = -machine gic-version=2 -append 'ipi'
>  groups = gic
>  
> +[gicv2-mmio]
> +file = gic.flat
> +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> +extra_params = -machine gic-version=2 -append 'mmio'
> +groups = gic
> +
>  [gicv3-ipi]
>  file = gic.flat
>  smp = $MAX_SMP
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index c2267b6..cef748d 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -10,10 +10,12 @@
>  /* 

RE: [kvm-unit-tests PATCH 0/4] kvm-unit-tests: add first GIC MMIO tests

[Itaru Kitayama]

Hi Andre,

I've verified tests you proposed to the lists finish without an
issue with the kvm-arm-for-4.9-rc6 kernel.

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

Re: [kvm-unit-tests PATCH] arm/arm64: introduce is_aarch32

[Andrew Jones]

On Thu, Nov 17, 2016 at 05:47:51PM +, Mark Rutland wrote:
> On Thu, Nov 17, 2016 at 05:45:41PM +0100, Andrew Jones wrote:
> > Hi Mark,
> > 
> > On Wed, Nov 16, 2016 at 05:45:44PM +, Mark Rutland wrote:
> > > So I don't think we should try to distinguish ARMv8-A AArch32 from
> > > ARMv7-A. We should test individual features, or if that's not possible,
> > > group them in the same bucket.
> > 
> > Perhaps I was too quick to look for a general way to approach the first
> > place I saw the need, which is
> > 
> >  check_cntr(read_pmccntr());
> >  if (is_aarch64())
> >  check_cntr(read_pmccntr64());
> > 
> > Is PMCCNTR the only "weird" difference?
> 
> Define "weird". ;)
> 
> I believe the extension of PMCCNTR to 64 bits is part of PMUv3. So you
> should be able to check ID_DFR0.PerfMon == 0b0011. For overflow you'll
> also need to configure PMCR.LC.

Thanks Mark,

checking for ID_DFR0.PerfMon == 0b0011 for this case does indeed look
like the right thing to do.

Wei,

when you respin the PMU test can you add the additional 64-bit read,
when a PMUv3 is present?

Thanks,
drew


> 
> Thanks,
> Mark.
> --
> 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

[PATCH 2/2] KVM: arm64: Fix the issues when guest PMCCFILTR is configured

[Marc Zyngier]

From: Wei Huang 

KVM calls kvm_pmu_set_counter_event_type() when PMCCFILTR is configured.
But this function can't deals with PMCCFILTR correctly because the evtCount
bits of PMCCFILTR, which is reserved 0, conflits with the SW_INCR event
type of other PMXEVTYPER registers. To fix it, when eventsel == 0, this
function shouldn't return immediately; instead it needs to check further
if select_idx is ARMV8_PMU_CYCLE_IDX.

Another issue is that KVM shouldn't copy the eventsel bits of PMCCFILTER
blindly to attr.config. Instead it ought to convert the request to the
"cpu cycle" event type (i.e. 0x11).

To support this patch and to prevent duplicated definitions, a limited
set of ARMv8 perf event types were relocated from perf_event.c to
asm/perf_event.h.

Cc: sta...@vger.kernel.org # 4.6+
Acked-by: Will Deacon 
Signed-off-by: Wei Huang 
Signed-off-by: Marc Zyngier 
---
 arch/arm64/include/asm/perf_event.h | 10 +-
 arch/arm64/kernel/perf_event.c  | 10 +-
 virt/kvm/arm/pmu.c  |  8 +---
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/perf_event.h 
b/arch/arm64/include/asm/perf_event.h
index 2065f46..38b6a2b 100644
--- a/arch/arm64/include/asm/perf_event.h
+++ b/arch/arm64/include/asm/perf_event.h
@@ -46,7 +46,15 @@
 #defineARMV8_PMU_EVTYPE_MASK   0xc800  /* Mask for writable 
bits */
 #defineARMV8_PMU_EVTYPE_EVENT  0x  /* Mask for EVENT bits 
*/
 
-#define ARMV8_PMU_EVTYPE_EVENT_SW_INCR 0   /* Software increment event */
+/*
+ * PMUv3 event types: required events
+ */
+#define ARMV8_PMUV3_PERFCTR_SW_INCR0x00
+#define ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL   0x03
+#define ARMV8_PMUV3_PERFCTR_L1D_CACHE  0x04
+#define ARMV8_PMUV3_PERFCTR_BR_MIS_PRED0x10
+#define ARMV8_PMUV3_PERFCTR_CPU_CYCLES 0x11
+#define ARMV8_PMUV3_PERFCTR_BR_PRED0x12
 
 /*
  * Event filters for PMUv3
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index a9310a6..57ae9d9 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -31,17 +31,9 @@
 
 /*
  * ARMv8 PMUv3 Performance Events handling code.
- * Common event types.
+ * Common event types (some are defined in asm/perf_event.h).
  */
 
-/* Required events. */
-#define ARMV8_PMUV3_PERFCTR_SW_INCR0x00
-#define ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL   0x03
-#define ARMV8_PMUV3_PERFCTR_L1D_CACHE  0x04
-#define ARMV8_PMUV3_PERFCTR_BR_MIS_PRED0x10
-#define ARMV8_PMUV3_PERFCTR_CPU_CYCLES 0x11
-#define ARMV8_PMUV3_PERFCTR_BR_PRED0x12
-
 /* At least one of the following is required. */
 #define ARMV8_PMUV3_PERFCTR_INST_RETIRED   0x08
 #define ARMV8_PMUV3_PERFCTR_INST_SPEC  0x1B
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 6e9c40e..69ccce3 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -305,7 +305,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 
val)
continue;
type = vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i)
   & ARMV8_PMU_EVTYPE_EVENT;
-   if ((type == ARMV8_PMU_EVTYPE_EVENT_SW_INCR)
+   if ((type == ARMV8_PMUV3_PERFCTR_SW_INCR)
&& (enable & BIT(i))) {
reg = vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
reg = lower_32_bits(reg);
@@ -379,7 +379,8 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, 
u64 data,
eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
 
/* Software increment event does't need to be backed by a perf event */
-   if (eventsel == ARMV8_PMU_EVTYPE_EVENT_SW_INCR)
+   if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
+   select_idx != ARMV8_PMU_CYCLE_IDX)
return;
 
memset(, 0, sizeof(struct perf_event_attr));
@@ -391,7 +392,8 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, 
u64 data,
attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
attr.exclude_hv = 1; /* Don't count EL2 events */
attr.exclude_host = 1; /* Don't count host events */
-   attr.config = eventsel;
+   attr.config = (select_idx == ARMV8_PMU_CYCLE_IDX) ?
+   ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
 
counter = kvm_pmu_get_counter_value(vcpu, select_idx);
/* The initial sample period (overflow count) of an event. */
-- 
2.1.4

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

[PULL] KVM/ARM updates for 4.9-rc6

[Marc Zyngier]

Paolo, Radim,

Please find below the pull request for a couple of fixes for the
PMU emulation, courtesy of Wei. Both patches are candidates for stable.

Thanks,

M.

The following changes since commit d42c79701a3ee5c38fbbc82f98a140420bd40134:

  KVM: arm/arm64: vgic: Kick VCPUs when queueing already pending IRQs 
(2016-11-04 17:56:56 +)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git 
tags/kvm-arm-for-4.9-rc6

for you to fetch changes up to b112c84a6ff035271d41d548c10215f18443d6a6:

  KVM: arm64: Fix the issues when guest PMCCFILTR is configured (2016-11-18 
09:06:58 +)


KVM/ARM updates for v4.9-rc6

- Fix handling of the 32bit cycle counter
- Fix cycle counter filtering


Wei Huang (2):
  arm64: KVM: pmu: Fix AArch32 cycle counter access
  KVM: arm64: Fix the issues when guest PMCCFILTR is configured

 arch/arm64/include/asm/perf_event.h | 10 +-
 arch/arm64/kernel/perf_event.c  | 10 +-
 arch/arm64/kvm/sys_regs.c   | 10 --
 virt/kvm/arm/pmu.c  |  8 +---
 4 files changed, 23 insertions(+), 15 deletions(-)
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/2] arm64: KVM: pmu: Fix AArch32 cycle counter access

[Marc Zyngier]

From: Wei Huang 

We're missing the handling code for the cycle counter accessed
from a 32bit guest, leading to unexpected results.

Cc: sta...@vger.kernel.org # 4.6+
Signed-off-by: Wei Huang 
Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/sys_regs.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f302fdb..87e7e66 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -597,8 +597,14 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 
idx = ARMV8_PMU_CYCLE_IDX;
} else {
-   BUG();
+   return false;
}
+   } else if (r->CRn == 0 && r->CRm == 9) {
+   /* PMCCNTR */
+   if (pmu_access_event_counter_el0_disabled(vcpu))
+   return false;
+
+   idx = ARMV8_PMU_CYCLE_IDX;
} else if (r->CRn == 14 && (r->CRm & 12) == 8) {
/* PMEVCNTRn_EL0 */
if (pmu_access_event_counter_el0_disabled(vcpu))
@@ -606,7 +612,7 @@ static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
 
idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
} else {
-   BUG();
+   return false;
}
 
if (!pmu_counter_idx_valid(vcpu, idx))
-- 
2.1.4

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