Re: [PATCH 0/2] Add the missing resetting LRs at boot time for new-vgic

[Shannon Zhao]

On 2016/12/6 19:47, Marc Zyngier wrote:
> On 06/12/16 06:41, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> Commit 50926d8(KVM: arm/arm64: The GIC is dead, long live the GIC)
>> removes the old vgic and commit 9097773(KVM: arm/arm64: vgic-new: 
>> vgic_init: implement kvm_vgic_hyp_init) doesn't reset LRs for new-vgic
>> when probing GIC. These two patches add the missing part.
>>
>> BTW, here is a strange problem on Huawei D03 board when using
>> upstream kernel that android guest with a goldfish_fb will hang with
>> rcu_stall and interrupt timeout for goldfish_fb. We apply these patches
>> but the problem still exists, while if we revert the commit
>> b40c489(arm64: KVM: vgic-v3: Only wipe LRs on vcpu exit) the guest runs
>> well.
>>
>> We add a trace in kvm_vgic_flush_hwstate() to print the value of 
>> compute_ap_list_depth(vcpu) and the value of vgic_lr before calling
>> vgic_flush_lr_state(). The first output shows that the ap_list_depth is zero
>> but the first one in vgic_lr is 10a02001. I don't understand why
>> there is a valued one in vgic_lr since the memory of vgic_lr is zero
>> allocated. I think It should be zero when the vcpu first run and first
>> call kvm_vgic_flush_hwstate().
>>
>> qemu-system-aar-6673  [016]    501.969251: kvm_vgic_flush_hwstate: VCPU: 
>> 0, lits-count: 0, LR: 10a02001, 0, 0, 0
>>
>> I also add a trace at the end of vgic_flush_lr_state() which shows the
>> kvm_vgic_global_state.nr_lr is 4, used_lrs is 0 and all LRs in vgic_lr
>> are zero.
>>
>> qemu-system-aar-6673  [016]    501.969254: vgic_flush_lr_state_nuke: 
>> kvm_vgic_global_state.nr_lr is :4, irq1:0, irq2:0, irq3:0, irq4:0
>>
>> But the trace at the beginning of kvm_vgic_sync_hwstate() shows the
>> first one of vgic_lr is 10a02001.
>>
>> qemu-system-aar-6673  [016]    501.969261: 
>> kvm_vgic_sync_hwstate_vgic_lr: VCPU: 0, used_lrs: 0, LR: 10a02001, 
>> 0, 0, 0
>>
>> The above three trace outputs are printed by the first KVM_ENTRY/EXIT of 
>> VCPU 0.
> 
> Decoding this LR value is interesting:
> 
> 10a02001
> | | | LPI 8193
> | |
> | Priority 0xa0
> |
> Group1
> 
> Someone is injecting an LPI behind your back. If nobody populates this,
> then you may want to investigate what is happening on the host side. Is
> there anyone using this interrupt?
> 

For this guest, I think nobody populates this LR, but on the host, there
is a LPI interrupt 8193. It's a interrupt of eth2

MBIGEN-V2 8193 Edge  eth2-tx0

It's a little confused to me that the LR registers should only be used
for VM, right? Why does the interrupt on host would affect the LRs?

Thanks,
-- 
Shannon

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

Re: [PATCH] arm/arm64: KVM: Check for properly initialized timer on init

[Christoffer Dall]

On Tue, Dec 06, 2016 at 11:25:42AM +, Marc Zyngier wrote:
> On 05/12/16 09:32, Christoffer Dall wrote:
> > When the arch timer code fails to initialize (for example because the
> > memory mapped timer doesn't work, which is currently seen with the AEM
> > model), then KVM just continues happily with a final result that KVM
> > eventually does a NULL pointer dereference of the uninitialized cycle
> > counter.
> > 
> > Check directly for this in the init path and give the user a reasonable
> > error in this case.
> > 
> > Cc: Shih-Wei Li 
> > Signed-off-by: Christoffer Dall 
> > ---
> >  virt/kvm/arm/arch_timer.c | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 27a1f63..5c12f53 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -425,6 +425,11 @@ int kvm_timer_hyp_init(void)
> > info = arch_timer_get_kvm_info();
> > timecounter = >timecounter;
> >  
> > +   if (!timecounter->cc) {
> > +   kvm_err("arch_timer: uninitialized timecounter\n");
> 
> For consistency, I'll change the error message to say "kvm_arch_timer",
> just like the below case.
> 

No objections, only problem is that the patch you queued uses
kcm_arch_timer ;)

> > +   return -ENODEV;
> > +   }
> > +
> > if (info->virtual_irq <= 0) {
> > kvm_err("kvm_arch_timer: invalid virtual timer IRQ: %d\n",
> > info->virtual_irq);
> > 
> 
> Otherwise looks good to me. I'll queue it now.
> 

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

[kvm-unit-tests PATCH v14 4/5] 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 | 66 +++
 1 file changed, 66 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index bf6ac69..d9ff19d 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -18,6 +18,9 @@
 #include "asm/sysreg.h"
 #include "asm/processor.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
@@ -28,16 +31,47 @@
 #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__)
 #define PMCR __ACCESS_CP15(c9, 0, c12, 0)
 #define ID_DFR0  __ACCESS_CP15(c0, 0, c1, 2)
+#define PMSELR   __ACCESS_CP15(c9, 0, c12, 5)
+#define PMXEVTYPER   __ACCESS_CP15(c9, 0, c13, 1)
+#define PMCNTENSET   __ACCESS_CP15(c9, 0, c12, 1)
+#define PMCCNTR32__ACCESS_CP15(c9, 0, c13, 0)
+#define PMCCNTR64__ACCESS_CP15_64(0, c9)
 
 static inline uint32_t get_id_dfr0(void) { return read_sysreg(ID_DFR0); }
 static inline uint32_t get_pmcr(void) { return read_sysreg(PMCR); }
+static inline void set_pmcr(uint32_t v) { write_sysreg(v, PMCR); }
+static inline void set_pmcntenset(uint32_t v) { write_sysreg(v, PMCNTENSET); }
+
+static inline uint64_t get_pmccntr(void)
+{
+   if (pmu_version == 0x3)
+   return read_sysreg(PMCCNTR64);
+   else
+   return read_sysreg(PMCCNTR32);
+}
+
+/* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */
+static inline void set_pmccfiltr(uint32_t value)
+{
+   write_sysreg(PMU_CYCLE_IDX, PMSELR);
+   write_sysreg(value, PMXEVTYPER);
+   isb();
+}
 #elif defined(__aarch64__)
 static inline uint32_t get_id_dfr0(void) { return read_sysreg(id_dfr0_el1); }
 static inline uint32_t get_pmcr(void) { return read_sysreg(pmcr_el0); }
+static inline void set_pmcr(uint32_t v) { write_sysreg(v, pmcr_el0); }
+static inline uint64_t get_pmccntr(void) { return read_sysreg(pmccntr_el0); }
+static inline void set_pmcntenset(uint32_t v) { write_sysreg(v, 
pmcntenset_el0); }
+static inline void set_pmccfiltr(uint32_t v) { write_sysreg(v, pmccfiltr_el0); 
}
 #endif
 
 /*
@@ -63,6 +97,37 @@ 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;
+
+   /* init before event access, this test only cares about cycle count */
+   set_pmcntenset(1 << PMU_CYCLE_IDX);
+   set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
+
+   set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
+
+   for (int i = 0; i < NR_SAMPLES; i++) {
+   uint64_t a, b;
+
+   a = get_pmccntr();
+   b = get_pmccntr();
+
+   if (a >= b) {
+   printf("Read %"PRId64" then %"PRId64".\n", a, b);
+   success = false;
+   break;
+   }
+   }
+
+   set_pmcr(get_pmcr() & ~PMU_PMCR_E);
+
+   return success;
+}
+
 /* Return FALSE if no PMU found, otherwise return TRUE */
 bool pmu_probe(void)
 {
@@ -88,6 +153,7 @@ int main(void)
report_prefix_push("pmu");
 
report("Control register", check_pmcr());
+   report("Monotonically increasing cycle count", check_cycles_increase());
 
return report_summary();
 }
-- 
1.8.3.1

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

[kvm-unit-tests PATCH v14 2/5] arm: Add support for read_sysreg() and write_sysreg()

[Wei Huang]

This patch adds two new macros to support read/write operations of ARMv7
and ARMv8 system registers. As part of the change, xstr() is revised to
support variable arguments. With it, ARMv7 system register can be defined
with __ACCESS_CP15() or __ACCESS_CP15_64() depending if it is 32-bit or
64-bit. get_mpidr() is re-written with new macros.

Suggested-by: Andrew Jones 
Signed-off-by: Wei Huang 
---
 lib/arm/asm/processor.h   |  6 +++---
 lib/arm/asm/sysreg.h  | 19 +++
 lib/arm64/asm/processor.h | 11 ---
 lib/arm64/asm/sysreg.h| 26 ++
 lib/libcflat.h|  4 ++--
 5 files changed, 54 insertions(+), 12 deletions(-)
 create mode 100644 lib/arm64/asm/sysreg.h

diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
index f25e7ee..c831749 100644
--- a/lib/arm/asm/processor.h
+++ b/lib/arm/asm/processor.h
@@ -6,6 +6,7 @@
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
 #include 
+#include 
 
 enum vector {
EXCPTN_RST,
@@ -33,11 +34,10 @@ static inline unsigned long current_cpsr(void)
 
 #define current_mode() (current_cpsr() & MODE_MASK)
 
+#define MPIDR __ACCESS_CP15(c0, 0, c0, 5)
 static inline unsigned int get_mpidr(void)
 {
-   unsigned int mpidr;
-   asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r" (mpidr));
-   return mpidr;
+   return read_sysreg(MPIDR);
 }
 
 /* Only support Aff0 for now, up to 4 cpus */
diff --git a/lib/arm/asm/sysreg.h b/lib/arm/asm/sysreg.h
index 3e1ad3a..02dbe3d 100644
--- a/lib/arm/asm/sysreg.h
+++ b/lib/arm/asm/sysreg.h
@@ -34,4 +34,23 @@
 #define CR_AFE (1 << 29)   /* Access flag enable   */
 #define CR_TE  (1 << 30)   /* Thumb exception enable   */
 
+#ifndef __ASSEMBLY__
+#include 
+
+#define __ACCESS_CP15(CRn, Op1, CRm, Op2)  \
+   "mrc", "mcr", xstr(p15, Op1, %0, CRn, CRm, Op2), u32
+#define __ACCESS_CP15_64(Op1, CRm) \
+   "mrrc", "mcrr", xstr(p15, Op1, %Q0, %R0, CRm), u64
+
+#define __read_sysreg(r, w, c, t) ({   \
+   t __val;\
+   asm volatile(r " " c : "=r" (__val));   \
+   __val;  \
+   })
+#define read_sysreg(...) __read_sysreg(__VA_ARGS__)
+
+#define __write_sysreg(v, r, w, c, t)   asm volatile(w " " c : : "r" ((t)(v)))
+#define write_sysreg(v, ...)__write_sysreg(v, __VA_ARGS__)
+#endif /* !__ASSEMBLY__ */
+
 #endif /* _ASMARM_SYSREG_H_ */
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 84d5c7c..ed59ad2 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -19,6 +19,7 @@
 #ifndef __ASSEMBLY__
 #include 
 #include 
+#include 
 
 enum vector {
EL1T_SYNC,
@@ -66,14 +67,10 @@ static inline unsigned long current_level(void)
return el & 0xc;
 }
 
-#define DEFINE_GET_SYSREG32(reg)   \
-static inline unsigned int get_##reg(void) \
-{  \
-   unsigned int reg;   \
-   asm volatile("mrs %0, " #reg "_el1" : "=r" (reg));  \
-   return reg; \
+static inline unsigned int get_mpidr(void)
+{
+   return read_sysreg(mpidr_el1);
 }
-DEFINE_GET_SYSREG32(mpidr)
 
 /* Only support Aff0 for now, gicv2 only */
 #define mpidr_to_cpu(mpidr) ((int)((mpidr) & 0xff))
diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h
new file mode 100644
index 000..05b9fcb
--- /dev/null
+++ b/lib/arm64/asm/sysreg.h
@@ -0,0 +1,26 @@
+/*
+ * Ripped off from arch/arm64/include/asm/sysreg.h
+ *
+ * Copyright (C) 2016, Red Hat Inc, Andrew Jones 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#ifndef _ASMARM64_SYSREG_H_
+#define _ASMARM64_SYSREG_H_
+
+#ifndef __ASSEMBLY__
+#include 
+
+#define read_sysreg(r) ({  \
+   u64 __val;  \
+   asm volatile("mrs %0, " xstr(r) : "=r" (__val));\
+   __val;  \
+})
+
+#define write_sysreg(v, r) do {\
+   u64 __val = (u64)v; \
+   asm volatile("msr " xstr(r) ", %x0" : : "rZ" (__val));  \
+} while (0)
+
+#endif /* !__ASSEMBLY__ */
+#endif /* _ASMARM64_SYSREG_H_ */
diff --git a/lib/libcflat.h b/lib/libcflat.h
index c622198..c3fa4f2 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -27,8 +27,8 @@
 
 #define __unused __attribute__((__unused__))
 
-#define xstr(s) xxstr(s)
-#define xxstr(s) #s
+#define xstr(s...) xxstr(s)
+#define xxstr(s...) #s
 
 #define __ALIGN_MASK(x, mask)  (((x) + 

[kvm-unit-tests PATCH v14 1/5] arm: rename cp15.h to sysreg.h

[Wei Huang]

To prepare for future support of ARMv8 system register, rename cp15.h file
to sysreg.h, with _ASMARM_CP15_H_ renamed to _ASMARM_SYSREG_H_ in header
file.

Signed-off-by: Wei Huang 
---
 arm/cstart.S | 2 +-
 lib/arm/asm/{cp15.h => sysreg.h} | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename lib/arm/asm/{cp15.h => sysreg.h} (94%)

diff --git a/arm/cstart.S b/arm/cstart.S
index 3943867..9822fb7 100644
--- a/arm/cstart.S
+++ b/arm/cstart.S
@@ -9,7 +9,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 #define THREAD_START_SP ((THREAD_SIZE - S_FRAME_SIZE * 8) & ~7)
 
diff --git a/lib/arm/asm/cp15.h b/lib/arm/asm/sysreg.h
similarity index 94%
rename from lib/arm/asm/cp15.h
rename to lib/arm/asm/sysreg.h
index 7690a48..3e1ad3a 100644
--- a/lib/arm/asm/cp15.h
+++ b/lib/arm/asm/sysreg.h
@@ -1,5 +1,5 @@
-#ifndef _ASMARM_CP15_H_
-#define _ASMARM_CP15_H_
+#ifndef _ASMARM_SYSREG_H_
+#define _ASMARM_SYSREG_H_
 /*
  * From the Linux kernel arch/arm/include/asm/cp15.h
  *
@@ -34,4 +34,4 @@
 #define CR_AFE (1 << 29)   /* Access flag enable   */
 #define CR_TE  (1 << 30)   /* Thumb exception enable   */
 
-#endif /* _ASMARM_CP15_H_ */
+#endif /* _ASMARM_SYSREG_H_ */
-- 
1.8.3.1

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

[kvm-unit-tests PATCH v14 5/5] 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 | 133 +-
 arm/unittests.cfg |  14 ++
 2 files changed, 146 insertions(+), 1 deletion(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index d9ff19d..a39dae4 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -58,6 +58,14 @@ static inline uint64_t get_pmccntr(void)
return read_sysreg(PMCCNTR32);
 }
 
+static inline void set_pmccntr(uint64_t value)
+{
+   if (pmu_version == 0x3)
+   write_sysreg(value, PMCCNTR64);
+   else
+   write_sysreg(value & 0x, PMCCNTR32);
+}
+
 /* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */
 static inline void set_pmccfiltr(uint32_t value)
 {
@@ -65,13 +73,56 @@ static inline void set_pmccfiltr(uint32_t value)
write_sysreg(value, PMXEVTYPER);
isb();
 }
+
+/*
+ * 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. isb instructions were inserted to make sure
+ * pmccntr read after this function returns the exact instructions executed in
+ * the controlled block. Total instrs = isb + mcr + 2*loop = 2 + 2*loop.
+ */
+static inline void precise_instrs_loop(int loop, uint32_t pmcr)
+{
+   asm volatile(
+   "   mcr p15, 0, %[pmcr], c9, c12, 0\n"
+   "   isb\n"
+   "1: subs%[loop], %[loop], #1\n"
+   "   bgt 1b\n"
+   "   mcr p15, 0, %[z], c9, c12, 0\n"
+   "   isb\n"
+   : [loop] "+r" (loop)
+   : [pmcr] "r" (pmcr), [z] "r" (0)
+   : "cc");
+}
 #elif defined(__aarch64__)
 static inline uint32_t get_id_dfr0(void) { return read_sysreg(id_dfr0_el1); }
 static inline uint32_t get_pmcr(void) { return read_sysreg(pmcr_el0); }
 static inline void set_pmcr(uint32_t v) { write_sysreg(v, pmcr_el0); }
 static inline uint64_t get_pmccntr(void) { return read_sysreg(pmccntr_el0); }
+static inline void set_pmccntr(uint64_t v) { write_sysreg(v, pmccntr_el0); }
 static inline void set_pmcntenset(uint32_t v) { write_sysreg(v, 
pmcntenset_el0); }
 static inline void set_pmccfiltr(uint32_t v) { write_sysreg(v, pmccfiltr_el0); 
}
+
+/*
+ * 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. isb instructions are inserted to make sure
+ * pmccntr read after this function returns the exact instructions executed
+ * in the controlled block. Total instrs = isb + msr + 2*loop = 2 + 2*loop.
+ */
+static inline void precise_instrs_loop(int loop, uint32_t pmcr)
+{
+   asm volatile(
+   "   msr pmcr_el0, %[pmcr]\n"
+   "   isb\n"
+   "1: subs%[loop], %[loop], #1\n"
+   "   b.gt1b\n"
+   "   msr pmcr_el0, xzr\n"
+   "   isb\n"
+   : [loop] "+r" (loop)
+   : [pmcr] "r" (pmcr)
+   : "cc");
+}
 #endif
 
 /*
@@ -128,6 +179,80 @@ static bool check_cycles_increase(void)
return success;
 }
 
+/*
+ * Execute a known number of guest instructions. Only even instruction counts
+ * greater than or equal to 4 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 loop = (num - 2) / 2;
+
+   assert(num >= 4 && ((num - 2) % 2 == 0));
+   precise_instrs_loop(loop, 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, it also strictly checks that every measurement 
matches
+ * it. Strict CPI checking is used to test -icount mode.
+ */
+static bool check_cpi(int cpi)
+{
+   uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
+
+   /* init before event access, this test only cares about cycle count */
+   set_pmcntenset(1 << PMU_CYCLE_IDX);
+   set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
+
+   if (cpi > 0)
+   printf("Checking for CPI=%d.\n", cpi);
+   printf("instrs : cycles0 cycles1 ...\n");
+
+   for 

[kvm-unit-tests PATCH v14 3/5] 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 
---
 arm/Makefile.common |  3 +-
 arm/pmu.c   | 93 +
 arm/unittests.cfg   |  5 +++
 3 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 arm/pmu.c

diff --git a/arm/Makefile.common b/arm/Makefile.common
index f37b5c2..5da2fdd 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -12,7 +12,8 @@ endif
 tests-common = \
$(TEST_DIR)/selftest.flat \
$(TEST_DIR)/spinlock-test.flat \
-   $(TEST_DIR)/pci-test.flat
+   $(TEST_DIR)/pci-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..bf6ac69
--- /dev/null
+++ b/arm/pmu.c
@@ -0,0 +1,93 @@
+/*
+ * Test the ARM Performance Monitors Unit (PMU).
+ *
+ * Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2016, Red Hat Inc, Wei Huang 
+ *
+ * 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"
+#include "asm/sysreg.h"
+#include "asm/processor.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
+
+#define ID_DFR0_PERFMON_SHIFT 24
+#define ID_DFR0_PERFMON_MASK  0xf
+
+static unsigned int pmu_version;
+#if defined(__arm__)
+#define PMCR __ACCESS_CP15(c9, 0, c12, 0)
+#define ID_DFR0  __ACCESS_CP15(c0, 0, c1, 2)
+
+static inline uint32_t get_id_dfr0(void) { return read_sysreg(ID_DFR0); }
+static inline uint32_t get_pmcr(void) { return read_sysreg(PMCR); }
+#elif defined(__aarch64__)
+static inline uint32_t get_id_dfr0(void) { return read_sysreg(id_dfr0_el1); }
+static inline uint32_t get_pmcr(void) { return read_sysreg(pmcr_el0); }
+#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 = get_pmcr();
+
+   report_info("PMU implementer/ID code/counters: 0x%x(\"%c\")/0x%x/%d",
+   (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK,
+   ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) ? : ' ',
+   (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK,
+   (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK);
+
+   return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0;
+}
+
+/* Return FALSE if no PMU found, otherwise return TRUE */
+bool pmu_probe(void)
+{
+   uint32_t dfr0;
+
+   /* probe pmu version */
+   dfr0 = get_id_dfr0();
+   pmu_version = (dfr0 >> ID_DFR0_PERFMON_SHIFT) & ID_DFR0_PERFMON_MASK;
+
+   if (pmu_version)
+   report_info("PMU version: %d", pmu_version);
+
+   return pmu_version;
+}
+
+int main(void)
+{
+   if (!pmu_probe()) {
+   printf("No PMU found, test skipped...\n");
+   return report_summary();
+   }
+
+   report_prefix_push("pmu");
+
+   report("Control register", check_pmcr());
+
+   return report_summary();
+}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index ae32a42..816f494 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -58,3 +58,8 @@ groups = selftest
 [pci-test]
 file = pci-test.flat
 groups = pci
+
+# 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 v14 0/5] ARM PMU tests

[Wei Huang]

Changes from v13:
* Rename cp15.h to sysreg.h for ARMv7 and add a new file sysreg.h for ARMv8
* Add macros for read_sysreg() and write_sysreg(). CP15 registers can be
  defined with __ACCESS_CP15() or __ACCESS_CP15_64(). sysreg.h (ARMv8) was
  from Drew's GIC testing code, which can be leveraged when his GIC testing
  code is imported.
* Rewrite PMU testing code based on new macros. All get_xxx() and set_xxx()
  functions are defined in pmu.c based on read_sysreg() and write_sysreg().
  So the code parsing tool, like cscope, can parse them easily.
* Minor fixes inside pmu.c, printf formatting, pmu_probe() func, based on
  Andre's comments.

Note:
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.
Link: https://lists.cs.columbia.edu/pipermail/kvmarm/2016-November/022134.html.

Thanks,
-Wei

Christopher Covington (3):
  arm: Add PMU test
  arm: pmu: Check cycle count increases
  arm: pmu: Add CPI checking

Wei Huang (2):
  arm: rename cp15.h to sysreg.h
  arm: Add support for read_sysreg() and write_sysreg()

 arm/Makefile.common  |   3 +-
 arm/cstart.S |   2 +-
 arm/pmu.c| 290 +++
 arm/unittests.cfg|  19 +++
 lib/arm/asm/processor.h  |   6 +-
 lib/arm/asm/{cp15.h => sysreg.h} |  25 +++-
 lib/arm64/asm/processor.h|  11 +-
 lib/arm64/asm/sysreg.h   |  26 
 lib/libcflat.h   |   4 +-
 9 files changed, 369 insertions(+), 17 deletions(-)
 create mode 100644 arm/pmu.c
 rename lib/arm/asm/{cp15.h => sysreg.h} (68%)
 create mode 100644 lib/arm64/asm/sysreg.h

-- 
1.8.3.1

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

Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest

[Marc Zyngier]

On 06/12/16 16:29, Christoffer Dall wrote:
> On Tue, Dec 06, 2016 at 02:56:50PM +, Marc Zyngier wrote:
>> The ARMv8 architecture allows the cycle counter to be configured
>> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0,
>> hence accessing PMCCFILTR_EL0. But it disallows the use of
>> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through
>> PMXEVCNTR_EL0.
>>
>> Linux itself doesn't violate this rule, but we may end up with
>> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that
>> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1,
>> despite the guest not having done anything wrong.
>>
>> In order to avoid this unfortunate course of events (haha!), let's
> 
> I actually did find this funny.
> 
>> sanitize PMSELR_EL0 on guest entry. This ensures that the guest
>> won't explode unexpectedly.
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>> This is another approach to fix this issue, this time nuking PMSELR_EL0
>> on guest entry instead of relying on perf not to clobber the register.
>>
>> Tested on v4.9-rc8 with a Rev A3 X-Gene.
>>
>>  arch/arm64/kvm/hyp/switch.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 83037cd..3b7cfbd 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu 
>> *vcpu)
>>  write_sysreg(val, hcr_el2);
>>  /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
>>  write_sysreg(1 << 15, hstr_el2);
>> -/* Make sure we trap PMU access from EL0 to EL2 */
>> +/*
>> + * Make sure we trap PMU access from EL0 to EL2. Also sanitize
>> + * PMSELR_EL0 to make sure it never contains the cycle
>> + * counter, which could make a PMXEVCNTR_EL0 access UNDEF.
>> + */
>> +if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK)
>> +write_sysreg(0, pmselr_el0);
> 
> I'm a bit confused about how we use the HPMN field.  This value is
> always set to the full number of counters available on the system and
> never modified by the guest, right?  So this is in essence a check that
> says 'do you have any performance counters, then make sure accesses
> don't undef to el1 instead of trapping to el2', but then my question is,
> why not just set pmselr_el0 to zero unconditionally, because in the case
> where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have
> no counters, which we'll have exposed to the guest anyhow, and we should
> undef at el1 in the guest, or did I get this completely wrong (like
> everything else today)?

Yeah, that's probably the best course of action. If the guest does
something silly, tough. I'll drop the test and repost the thing.

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: [PATCH 1/1] arm64: Correcting format specifier for printing 64 bit addresses

[Robin Murphy]

On 05/12/16 08:09, Maninder Singh wrote:
> This patch corrects format specifier for printing 64 bit addresses.
> 
> Signed-off-by: Maninder Singh 
> Signed-off-by: Vaneet Narang 
> ---
>  arch/arm64/kernel/signal.c |  2 +-
>  arch/arm64/kvm/sys_regs.c  |  8 ++--
>  arch/arm64/mm/fault.c  | 15 ++-
>  arch/arm64/mm/mmu.c|  4 ++--
>  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index c7b6de6..c89d5fd 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -155,7 +155,7 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
>  
>  badframe:
>   if (show_unhandled_signals)
> - pr_info_ratelimited("%s[%d]: bad frame in %s: pc=%08llx 
> sp=%08llx\n",
> + pr_info_ratelimited("%s[%d]: bad frame in %s: pc=%016llx 
> sp=%016llx\n",
>   current->comm, task_pid_nr(current), 
> __func__,
>   regs->pc, regs->sp);
>   force_sig(SIGSEGV, current);
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 87e7e66..89bf5c1 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1554,8 +1554,12 @@ static void unhandled_cp_access(struct kvm_vcpu *vcpu,
>   WARN_ON(1);
>   }
>  
> - kvm_err("Unsupported guest CP%d access at: %08lx\n",
> - cp, *vcpu_pc(vcpu));
> + if (params->is_32bit)
> + kvm_err("Unsupported guest CP%d access at: %08lx\n",
> + cp, *vcpu_pc(vcpu));
> + else
> + kvm_err("Unsupported guest CP%d access at: %016lx\n",
> + cp, *vcpu_pc(vcpu));

As with the other patch - use '%0*lx' in these cases rather than
pointlessly duplicating everything.

Robin.

>   print_sys_reg_instr(params);
>   kvm_inject_undefined(vcpu);
>  }
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index a78a5c4..d96a42a 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -77,7 +77,7 @@ void show_pte(struct mm_struct *mm, unsigned long addr)
>  
>   pr_alert("pgd = %p\n", mm->pgd);
>   pgd = pgd_offset(mm, addr);
> - pr_alert("[%08lx] *pgd=%016llx", addr, pgd_val(*pgd));
> + pr_alert("[%016lx] *pgd=%016llx", addr, pgd_val(*pgd));
>  
>   do {
>   pud_t *pud;
> @@ -177,7 +177,7 @@ static void __do_kernel_fault(struct mm_struct *mm, 
> unsigned long addr,
>* No handler, we'll have to terminate things with extreme prejudice.
>*/
>   bust_spinlocks(1);
> - pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
> + pr_alert("Unable to handle kernel %s at virtual address %016lx\n",
>(addr < PAGE_SIZE) ? "NULL pointer dereference" :
>"paging request", addr);
>  
> @@ -198,9 +198,14 @@ static void __do_user_fault(struct task_struct *tsk, 
> unsigned long addr,
>   struct siginfo si;
>  
>   if (unhandled_signal(tsk, sig) && show_unhandled_signals_ratelimited()) 
> {
> - pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
> - tsk->comm, task_pid_nr(tsk), fault_name(esr), sig,
> - addr, esr);
> + if (compat_user_mode(regs))
> + pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 
> 0x%03x\n",
> + tsk->comm, task_pid_nr(tsk), fault_name(esr), 
> sig,
> + addr, esr);
> + else
> + pr_info("%s[%d]: unhandled %s (%d) at 0x%016lx, esr 
> 0x%03x\n",
> + tsk->comm, task_pid_nr(tsk), fault_name(esr), 
> sig,
> + addr, esr);
>   show_pte(tsk->mm, addr);
>   show_regs(regs);
>   }
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 17243e4..cbf444c 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -683,9 +683,9 @@ void __init early_fixmap_init(void)
>   pr_warn("pmd %p != %p, %p\n",
>   pmd, fixmap_pmd(fix_to_virt(FIX_BTMAP_BEGIN)),
>   fixmap_pmd(fix_to_virt(FIX_BTMAP_END)));
> - pr_warn("fix_to_virt(FIX_BTMAP_BEGIN): %08lx\n",
> + pr_warn("fix_to_virt(FIX_BTMAP_BEGIN): %016lx\n",
>   fix_to_virt(FIX_BTMAP_BEGIN));
> - pr_warn("fix_to_virt(FIX_BTMAP_END):   %08lx\n",
> + pr_warn("fix_to_virt(FIX_BTMAP_END):   %016lx\n",
>   fix_to_virt(FIX_BTMAP_END));
>  
>   pr_warn("FIX_BTMAP_END:   %d\n", FIX_BTMAP_END);
> 

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

Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest

[Will Deacon]

On Tue, Dec 06, 2016 at 05:29:18PM +0100, Christoffer Dall wrote:
> On Tue, Dec 06, 2016 at 02:56:50PM +, Marc Zyngier wrote:
> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index 83037cd..3b7cfbd 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu 
> > *vcpu)
> > write_sysreg(val, hcr_el2);
> > /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
> > write_sysreg(1 << 15, hstr_el2);
> > -   /* Make sure we trap PMU access from EL0 to EL2 */
> > +   /*
> > +* Make sure we trap PMU access from EL0 to EL2. Also sanitize
> > +* PMSELR_EL0 to make sure it never contains the cycle
> > +* counter, which could make a PMXEVCNTR_EL0 access UNDEF.
> > +*/
> > +   if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK)
> > +   write_sysreg(0, pmselr_el0);
> 
> I'm a bit confused about how we use the HPMN field.  This value is
> always set to the full number of counters available on the system and
> never modified by the guest, right?  So this is in essence a check that
> says 'do you have any performance counters, then make sure accesses
> don't undef to el1 instead of trapping to el2', but then my question is,
> why not just set pmselr_el0 to zero unconditionally, because in the case
> where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have
> no counters, which we'll have exposed to the guest anyhow, and we should
> undef at el1 in the guest, or did I get this completely wrong (like
> everything else today)?

I think Marc and I came to the same conclusion a few minutes ago. The check
you might want is "Have I instantiated a virtual PMU for this device?",
but that's probably a micro-optimisation.

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

Re: [PATCH 1/1] arm64: Correcting format specifier for printing 64 bit addresses

[Robin Murphy]

On 06/12/16 16:11, Christoffer Dall wrote:
> On Mon, Dec 05, 2016 at 01:39:53PM +0530, Maninder Singh wrote:
>> This patch corrects format specifier for printing 64 bit addresses.
>>
>> Signed-off-by: Maninder Singh 
>> Signed-off-by: Vaneet Narang 
>> ---
>>  arch/arm64/kernel/signal.c |  2 +-
>>  arch/arm64/kvm/sys_regs.c  |  8 ++--
>>  arch/arm64/mm/fault.c  | 15 ++-
>>  arch/arm64/mm/mmu.c|  4 ++--
>>  4 files changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
>> index c7b6de6..c89d5fd 100644
>> --- a/arch/arm64/kernel/signal.c
>> +++ b/arch/arm64/kernel/signal.c
>> @@ -155,7 +155,7 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
>>  
>>  badframe:
>>  if (show_unhandled_signals)
>> -pr_info_ratelimited("%s[%d]: bad frame in %s: pc=%08llx 
>> sp=%08llx\n",
>> +pr_info_ratelimited("%s[%d]: bad frame in %s: pc=%016llx 
>> sp=%016llx\n",
>>  current->comm, task_pid_nr(current), 
>> __func__,
>>  regs->pc, regs->sp);
>>  force_sig(SIGSEGV, current);
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 87e7e66..89bf5c1 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1554,8 +1554,12 @@ static void unhandled_cp_access(struct kvm_vcpu *vcpu,
>>  WARN_ON(1);
>>  }
>>  
>> -kvm_err("Unsupported guest CP%d access at: %08lx\n",
>> -cp, *vcpu_pc(vcpu));
>> +if (params->is_32bit)
>> +kvm_err("Unsupported guest CP%d access at: %08lx\n",
>> +cp, *vcpu_pc(vcpu));
>> +else
>> +kvm_err("Unsupported guest CP%d access at: %016lx\n",
>> +cp, *vcpu_pc(vcpu));
> 
> It feels a bit much to me to have an if-statement to differentiate the
> number of leading zeros, so if it's important to always have fixed
> widths then I would just use %016lx in both cases.

Actually, it looks like vsnprintf does support the '*' field width
specifier, so even if the format _is_ critical there's still no reason
to have such duplicated code.

Robin.

>>  print_sys_reg_instr(params);
>>  kvm_inject_undefined(vcpu);
>>  }
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index a78a5c4..d96a42a 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -77,7 +77,7 @@ void show_pte(struct mm_struct *mm, unsigned long addr)
>>  
>>  pr_alert("pgd = %p\n", mm->pgd);
>>  pgd = pgd_offset(mm, addr);
>> -pr_alert("[%08lx] *pgd=%016llx", addr, pgd_val(*pgd));
>> +pr_alert("[%016lx] *pgd=%016llx", addr, pgd_val(*pgd));
>>  
>>  do {
>>  pud_t *pud;
>> @@ -177,7 +177,7 @@ static void __do_kernel_fault(struct mm_struct *mm, 
>> unsigned long addr,
>>   * No handler, we'll have to terminate things with extreme prejudice.
>>   */
>>  bust_spinlocks(1);
>> -pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
>> +pr_alert("Unable to handle kernel %s at virtual address %016lx\n",
>>   (addr < PAGE_SIZE) ? "NULL pointer dereference" :
>>   "paging request", addr);
>>  
>> @@ -198,9 +198,14 @@ static void __do_user_fault(struct task_struct *tsk, 
>> unsigned long addr,
>>  struct siginfo si;
>>  
>>  if (unhandled_signal(tsk, sig) && show_unhandled_signals_ratelimited()) 
>> {
>> -pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
>> -tsk->comm, task_pid_nr(tsk), fault_name(esr), sig,
>> -addr, esr);
>> +if (compat_user_mode(regs))
>> +pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 
>> 0x%03x\n",
>> +tsk->comm, task_pid_nr(tsk), fault_name(esr), 
>> sig,
>> +addr, esr);
>> +else
>> +pr_info("%s[%d]: unhandled %s (%d) at 0x%016lx, esr 
>> 0x%03x\n",
>> +tsk->comm, task_pid_nr(tsk), fault_name(esr), 
>> sig,
>> +addr, esr);
> 
> same here.
> 
> Thanks,
> -Christoffer
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest

[Christoffer Dall]

On Tue, Dec 06, 2016 at 02:56:50PM +, Marc Zyngier wrote:
> The ARMv8 architecture allows the cycle counter to be configured
> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0,
> hence accessing PMCCFILTR_EL0. But it disallows the use of
> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through
> PMXEVCNTR_EL0.
> 
> Linux itself doesn't violate this rule, but we may end up with
> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that
> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1,
> despite the guest not having done anything wrong.
> 
> In order to avoid this unfortunate course of events (haha!), let's

I actually did find this funny.

> sanitize PMSELR_EL0 on guest entry. This ensures that the guest
> won't explode unexpectedly.
> 
> Signed-off-by: Marc Zyngier 
> ---
> This is another approach to fix this issue, this time nuking PMSELR_EL0
> on guest entry instead of relying on perf not to clobber the register.
> 
> Tested on v4.9-rc8 with a Rev A3 X-Gene.
> 
>  arch/arm64/kvm/hyp/switch.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 83037cd..3b7cfbd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu 
> *vcpu)
>   write_sysreg(val, hcr_el2);
>   /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
>   write_sysreg(1 << 15, hstr_el2);
> - /* Make sure we trap PMU access from EL0 to EL2 */
> + /*
> +  * Make sure we trap PMU access from EL0 to EL2. Also sanitize
> +  * PMSELR_EL0 to make sure it never contains the cycle
> +  * counter, which could make a PMXEVCNTR_EL0 access UNDEF.
> +  */
> + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK)
> + write_sysreg(0, pmselr_el0);

I'm a bit confused about how we use the HPMN field.  This value is
always set to the full number of counters available on the system and
never modified by the guest, right?  So this is in essence a check that
says 'do you have any performance counters, then make sure accesses
don't undef to el1 instead of trapping to el2', but then my question is,
why not just set pmselr_el0 to zero unconditionally, because in the case
where (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK) == 0, it means we have
no counters, which we'll have exposed to the guest anyhow, and we should
undef at el1 in the guest, or did I get this completely wrong (like
everything else today)?

>   write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0);
>   write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
>   __activate_traps_arch()();
> -- 
> 2.1.4
> 

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

Re: [PATCH 1/1] arm64: Correcting format specifier for printing 64 bit addresses

[Christoffer Dall]

On Mon, Dec 05, 2016 at 01:39:53PM +0530, Maninder Singh wrote:
> This patch corrects format specifier for printing 64 bit addresses.
> 
> Signed-off-by: Maninder Singh 
> Signed-off-by: Vaneet Narang 
> ---
>  arch/arm64/kernel/signal.c |  2 +-
>  arch/arm64/kvm/sys_regs.c  |  8 ++--
>  arch/arm64/mm/fault.c  | 15 ++-
>  arch/arm64/mm/mmu.c|  4 ++--
>  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index c7b6de6..c89d5fd 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -155,7 +155,7 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
>  
>  badframe:
>   if (show_unhandled_signals)
> - pr_info_ratelimited("%s[%d]: bad frame in %s: pc=%08llx 
> sp=%08llx\n",
> + pr_info_ratelimited("%s[%d]: bad frame in %s: pc=%016llx 
> sp=%016llx\n",
>   current->comm, task_pid_nr(current), 
> __func__,
>   regs->pc, regs->sp);
>   force_sig(SIGSEGV, current);
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 87e7e66..89bf5c1 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1554,8 +1554,12 @@ static void unhandled_cp_access(struct kvm_vcpu *vcpu,
>   WARN_ON(1);
>   }
>  
> - kvm_err("Unsupported guest CP%d access at: %08lx\n",
> - cp, *vcpu_pc(vcpu));
> + if (params->is_32bit)
> + kvm_err("Unsupported guest CP%d access at: %08lx\n",
> + cp, *vcpu_pc(vcpu));
> + else
> + kvm_err("Unsupported guest CP%d access at: %016lx\n",
> + cp, *vcpu_pc(vcpu));

It feels a bit much to me to have an if-statement to differentiate the
number of leading zeros, so if it's important to always have fixed
widths then I would just use %016lx in both cases.

>   print_sys_reg_instr(params);
>   kvm_inject_undefined(vcpu);
>  }
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index a78a5c4..d96a42a 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -77,7 +77,7 @@ void show_pte(struct mm_struct *mm, unsigned long addr)
>  
>   pr_alert("pgd = %p\n", mm->pgd);
>   pgd = pgd_offset(mm, addr);
> - pr_alert("[%08lx] *pgd=%016llx", addr, pgd_val(*pgd));
> + pr_alert("[%016lx] *pgd=%016llx", addr, pgd_val(*pgd));
>  
>   do {
>   pud_t *pud;
> @@ -177,7 +177,7 @@ static void __do_kernel_fault(struct mm_struct *mm, 
> unsigned long addr,
>* No handler, we'll have to terminate things with extreme prejudice.
>*/
>   bust_spinlocks(1);
> - pr_alert("Unable to handle kernel %s at virtual address %08lx\n",
> + pr_alert("Unable to handle kernel %s at virtual address %016lx\n",
>(addr < PAGE_SIZE) ? "NULL pointer dereference" :
>"paging request", addr);
>  
> @@ -198,9 +198,14 @@ static void __do_user_fault(struct task_struct *tsk, 
> unsigned long addr,
>   struct siginfo si;
>  
>   if (unhandled_signal(tsk, sig) && show_unhandled_signals_ratelimited()) 
> {
> - pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n",
> - tsk->comm, task_pid_nr(tsk), fault_name(esr), sig,
> - addr, esr);
> + if (compat_user_mode(regs))
> + pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 
> 0x%03x\n",
> + tsk->comm, task_pid_nr(tsk), fault_name(esr), 
> sig,
> + addr, esr);
> + else
> + pr_info("%s[%d]: unhandled %s (%d) at 0x%016lx, esr 
> 0x%03x\n",
> + tsk->comm, task_pid_nr(tsk), fault_name(esr), 
> sig,
> + addr, esr);

same here.

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

Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest

[Marc Zyngier]

On 06/12/16 15:27, Will Deacon wrote:
> On Tue, Dec 06, 2016 at 02:56:50PM +, Marc Zyngier wrote:
>> The ARMv8 architecture allows the cycle counter to be configured
>> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0,
>> hence accessing PMCCFILTR_EL0. But it disallows the use of
>> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through
>> PMXEVCNTR_EL0.
>>
>> Linux itself doesn't violate this rule, but we may end up with
>> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that
>> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1,
>> despite the guest not having done anything wrong.
>>
>> In order to avoid this unfortunate course of events (haha!), let's
>> sanitize PMSELR_EL0 on guest entry. This ensures that the guest
>> won't explode unexpectedly.
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>> This is another approach to fix this issue, this time nuking PMSELR_EL0
>> on guest entry instead of relying on perf not to clobber the register.
>>
>> Tested on v4.9-rc8 with a Rev A3 X-Gene.
>>
>>  arch/arm64/kvm/hyp/switch.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
>> index 83037cd..3b7cfbd 100644
>> --- a/arch/arm64/kvm/hyp/switch.c
>> +++ b/arch/arm64/kvm/hyp/switch.c
>> @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu 
>> *vcpu)
>>  write_sysreg(val, hcr_el2);
>>  /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
>>  write_sysreg(1 << 15, hstr_el2);
>> -/* Make sure we trap PMU access from EL0 to EL2 */
>> +/*
>> + * Make sure we trap PMU access from EL0 to EL2. Also sanitize
>> + * PMSELR_EL0 to make sure it never contains the cycle
>> + * counter, which could make a PMXEVCNTR_EL0 access UNDEF.
> 
> "UNDEF at EL1, as opposed to trapping to EL2" might be clearer, but up to
> you.

Sure, that's a useful clarification.

>> + */
>> +if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK)
>> +write_sysreg(0, pmselr_el0);
>>  write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0);
> 
> Curious, but why do you check MDCR.HPMN for PMSELR_EL0, but not for
> PMUSERENR_EL0?

Why would PMUSERENR_EL0 be constrained by the number of counters
available to the guest?

> 
> Anyway:
> 
> Acked-by: Will Deacon 

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: [PATCH 1/1] arm64: Correcting format specifier for printing 64 bit addresses

[Will Deacon]

On Tue, Dec 06, 2016 at 03:26:37PM +, Catalin Marinas wrote:
> On Mon, Dec 05, 2016 at 11:24:21AM +, Will Deacon wrote:
> > On Mon, Dec 05, 2016 at 01:39:53PM +0530, Maninder Singh wrote:
> > > This patch corrects format specifier for printing 64 bit addresses.
> > > 
> > > Signed-off-by: Maninder Singh 
> > > Signed-off-by: Vaneet Narang 
> > > ---
> > >  arch/arm64/kernel/signal.c |  2 +-
> > >  arch/arm64/kvm/sys_regs.c  |  8 ++--
> > >  arch/arm64/mm/fault.c  | 15 ++-
> > >  arch/arm64/mm/mmu.c|  4 ++--
> > >  4 files changed, 19 insertions(+), 10 deletions(-)
> > 
> > Any reason not to fix kvm/trace.h too?
> 
> If the KVM guys are ok, I can fold the hunk below into this patch:
> 
> diff --git a/arch/arm64/kvm/trace.h b/arch/arm64/kvm/trace.h
> index 7fb0008c4fa3..e117123d414b 100644
> --- a/arch/arm64/kvm/trace.h
> +++ b/arch/arm64/kvm/trace.h
> @@ -20,7 +20,7 @@ TRACE_EVENT(kvm_wfx_arm64,
>   __entry->is_wfe  = is_wfe;
>   ),
>  
> - TP_printk("guest executed wf%c at: 0x%08lx",
> + TP_printk("guest executed wf%c at: 0x%016lx",
> __entry->is_wfe ? 'e' : 'i', __entry->vcpu_pc)
>  );
>  
> @@ -40,7 +40,7 @@ TRACE_EVENT(kvm_hvc_arm64,
>   __entry->imm = imm;
>   ),
>  
> - TP_printk("HVC at 0x%08lx (r0: 0x%08lx, imm: 0x%lx)",
> + TP_printk("HVC at 0x%016lx (r0: 0x%016lx, imm: 0x%lx)",

Not sure we need the 016 prefix for r0.

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

Re: [PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest

[Will Deacon]

On Tue, Dec 06, 2016 at 02:56:50PM +, Marc Zyngier wrote:
> The ARMv8 architecture allows the cycle counter to be configured
> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0,
> hence accessing PMCCFILTR_EL0. But it disallows the use of
> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through
> PMXEVCNTR_EL0.
> 
> Linux itself doesn't violate this rule, but we may end up with
> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that
> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1,
> despite the guest not having done anything wrong.
> 
> In order to avoid this unfortunate course of events (haha!), let's
> sanitize PMSELR_EL0 on guest entry. This ensures that the guest
> won't explode unexpectedly.
> 
> Signed-off-by: Marc Zyngier 
> ---
> This is another approach to fix this issue, this time nuking PMSELR_EL0
> on guest entry instead of relying on perf not to clobber the register.
> 
> Tested on v4.9-rc8 with a Rev A3 X-Gene.
> 
>  arch/arm64/kvm/hyp/switch.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 83037cd..3b7cfbd 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu 
> *vcpu)
>   write_sysreg(val, hcr_el2);
>   /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
>   write_sysreg(1 << 15, hstr_el2);
> - /* Make sure we trap PMU access from EL0 to EL2 */
> + /*
> +  * Make sure we trap PMU access from EL0 to EL2. Also sanitize
> +  * PMSELR_EL0 to make sure it never contains the cycle
> +  * counter, which could make a PMXEVCNTR_EL0 access UNDEF.

"UNDEF at EL1, as opposed to trapping to EL2" might be clearer, but up to
you.

> +  */
> + if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK)
> + write_sysreg(0, pmselr_el0);
>   write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0);

Curious, but why do you check MDCR.HPMN for PMSELR_EL0, but not for
PMUSERENR_EL0?

Anyway:

Acked-by: Will Deacon 

Thanks,

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

Re: [PATCH 1/1] arm64: Correcting format specifier for printing 64 bit addresses

[Catalin Marinas]

On Mon, Dec 05, 2016 at 11:24:21AM +, Will Deacon wrote:
> On Mon, Dec 05, 2016 at 01:39:53PM +0530, Maninder Singh wrote:
> > This patch corrects format specifier for printing 64 bit addresses.
> > 
> > Signed-off-by: Maninder Singh 
> > Signed-off-by: Vaneet Narang 
> > ---
> >  arch/arm64/kernel/signal.c |  2 +-
> >  arch/arm64/kvm/sys_regs.c  |  8 ++--
> >  arch/arm64/mm/fault.c  | 15 ++-
> >  arch/arm64/mm/mmu.c|  4 ++--
> >  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> Any reason not to fix kvm/trace.h too?

If the KVM guys are ok, I can fold the hunk below into this patch:

diff --git a/arch/arm64/kvm/trace.h b/arch/arm64/kvm/trace.h
index 7fb0008c4fa3..e117123d414b 100644
--- a/arch/arm64/kvm/trace.h
+++ b/arch/arm64/kvm/trace.h
@@ -20,7 +20,7 @@ TRACE_EVENT(kvm_wfx_arm64,
__entry->is_wfe  = is_wfe;
),
 
-   TP_printk("guest executed wf%c at: 0x%08lx",
+   TP_printk("guest executed wf%c at: 0x%016lx",
  __entry->is_wfe ? 'e' : 'i', __entry->vcpu_pc)
 );
 
@@ -40,7 +40,7 @@ TRACE_EVENT(kvm_hvc_arm64,
__entry->imm = imm;
),
 
-   TP_printk("HVC at 0x%08lx (r0: 0x%08lx, imm: 0x%lx)",
+   TP_printk("HVC at 0x%016lx (r0: 0x%016lx, imm: 0x%lx)",
  __entry->vcpu_pc, __entry->r0, __entry->imm)
 );
 
@@ -131,7 +131,7 @@ TRACE_EVENT(trap_reg,
__entry->write_value = write_value;
),
 
-   TP_printk("%s %s reg %d (0x%08llx)", __entry->fn,  
__entry->is_write?"write to":"read from", __entry->reg, __entry->write_value)
+   TP_printk("%s %s reg %d (0x%016llx)", __entry->fn,  
__entry->is_write?"write to":"read from", __entry->reg, __entry->write_value)
 );
 
 TRACE_EVENT(kvm_handle_sys_reg,

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

[PATCH v2] arm64: KVM: pmu: Reset PMSELR_EL0.SEL to a sane value before entering the guest

[Marc Zyngier]

The ARMv8 architecture allows the cycle counter to be configured
by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0,
hence accessing PMCCFILTR_EL0. But it disallows the use of
PMSELR_EL0.SEL==0x1f to access the cycle counter itself through
PMXEVCNTR_EL0.

Linux itself doesn't violate this rule, but we may end up with
PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that
guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1,
despite the guest not having done anything wrong.

In order to avoid this unfortunate course of events (haha!), let's
sanitize PMSELR_EL0 on guest entry. This ensures that the guest
won't explode unexpectedly.

Signed-off-by: Marc Zyngier 
---
This is another approach to fix this issue, this time nuking PMSELR_EL0
on guest entry instead of relying on perf not to clobber the register.

Tested on v4.9-rc8 with a Rev A3 X-Gene.

 arch/arm64/kvm/hyp/switch.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 83037cd..3b7cfbd 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -85,7 +85,13 @@ static void __hyp_text __activate_traps(struct kvm_vcpu 
*vcpu)
write_sysreg(val, hcr_el2);
/* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */
write_sysreg(1 << 15, hstr_el2);
-   /* Make sure we trap PMU access from EL0 to EL2 */
+   /*
+* Make sure we trap PMU access from EL0 to EL2. Also sanitize
+* PMSELR_EL0 to make sure it never contains the cycle
+* counter, which could make a PMXEVCNTR_EL0 access UNDEF.
+*/
+   if (vcpu->arch.mdcr_el2 & MDCR_EL2_HPMN_MASK)
+   write_sysreg(0, pmselr_el0);
write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0);
write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
__activate_traps_arch()();
-- 
2.1.4

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

Re: [PATCH v9 01/11] arm/arm64: vgic: Implement support for userspace access

[Christoffer Dall]

On Tue, Dec 06, 2016 at 12:42:09PM +0100, Auger Eric wrote:
> Hi,
> 
> On 28/11/2016 14:05, Christoffer Dall wrote:
> > On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kil...@gmail.com wrote:
> >> From: Vijaya Kumar K 
> >>
> >> Read and write of some registers like ISPENDR and ICPENDR
> >> from userspace requires special handling when compared to
> >> guest access for these registers.
> >>
> >> Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> >> for handling of ISPENDR, ICPENDR registers handling.
> >>
> >> Add infrastructure to support guest and userspace read
> >> and write for the required registers
> >> Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
> >>
> >> Signed-off-by: Vijaya Kumar K 
> >> ---
> >>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  25 --
> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 
> >> ---
> >>  virt/kvm/arm/vgic/vgic-mmio.c|  78 +++---
> >>  virt/kvm/arm/vgic/vgic-mmio.h|  19 
> >>  4 files changed, 175 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
> >> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >> index b44b359..0b32f40 100644
> >> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> >> @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, 
> >> struct kvm_device_attr *attr)
> >>return -ENXIO;
> >>  }
> >>  
> >> -/*
> >> - * When userland tries to access the VGIC register handlers, we need to
> >> - * create a usable struct vgic_io_device to be passed to the handlers and 
> >> we
> >> - * have to set up a buffer similar to what would have happened if a guest 
> >> MMIO
> >> - * access occurred, including doing endian conversions on BE systems.
> >> - */
> >> -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> >> -  bool is_write, int offset, u32 *val)
> >> -{
> >> -  unsigned int len = 4;
> >> -  u8 buf[4];
> >> -  int ret;
> >> -
> >> -  if (is_write) {
> >> -  vgic_data_host_to_mmio_bus(buf, len, *val);
> >> -  ret = kvm_io_gic_ops.write(vcpu, >dev, offset, len, buf);
> >> -  } else {
> >> -  ret = kvm_io_gic_ops.read(vcpu, >dev, offset, len, buf);
> >> -  if (!ret)
> >> -  *val = vgic_data_mmio_bus_to_host(buf, len);
> >> -  }
> >> -
> >> -  return ret;
> >> -}
> >> -
> >>  int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> >>  int offset, u32 *val)
> >>  {
> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
> >> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> index 50f42f0..8e76d04 100644
> >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> @@ -207,6 +207,66 @@ static unsigned long vgic_mmio_read_v3_idregs(struct 
> >> kvm_vcpu *vcpu,
> >>return 0;
> >>  }
> >>  
> >> +static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> >> +gpa_t addr, unsigned int len)
> >> +{
> >> +  u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> +  u32 value = 0;
> >> +  int i;
> >> +
> >> +  /*
> >> +   * A level triggerred interrupt pending state is latched in both
> >> +   * "soft_pending" and "line_level" variables. Userspace will save
> >> +   * and restore soft_pending and line_level separately.
> >> +   * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> >> +   * handling of ISPENDR and ICPENDR.
> >> +   */
> >> +  for (i = 0; i < len * 8; i++) {
> >> +  struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >> +
> >> +  if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
> >> +  value |= (1U << i);
> >> +  if (irq->config == VGIC_CONFIG_EDGE && irq->pending)
> >> +  value |= (1U << i);
> >> +
> >> +  vgic_put_irq(vcpu->kvm, irq);
> >> +  }
> >> +
> >> +  return value;
> >> +}
> >> +
> >> +static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
> >> +gpa_t addr, unsigned int len,
> >> +unsigned long val)
> >> +{
> >> +  u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> +  int i;
> >> +
> >> +  for (i = 0; i < len * 8; i++) {
> >> +  struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >> +
> >> +  spin_lock(>irq_lock);
> >> +  if (test_bit(i, )) {
> >> +  /* soft_pending is set irrespective of irq type
> >> +   * (level or edge) to avoid dependency that VM should
> >> +   * restore irq config before pending info.
> >> +   */
> > 
> > nit: kernel commenting style
> > 
> >> +  irq->pending = true;
> >> +  irq->soft_pending = true;
> >> +  vgic_queue_irq_unlock(vcpu->kvm, irq);
> >> +  } else {
> >> +  

Re: [PATCH 1/2] arm64: PMU: Do not use PMSELR_EL0 to access PMCCFILTR_EL0

[Marc Zyngier]

On 06/12/16 13:50, Will Deacon wrote:
> On Fri, Dec 02, 2016 at 03:50:58PM +, Marc Zyngier wrote:
>> The ARMv8 architecture allows the cycle counter to be configured
>> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0,
>> hence accessing PMCCFILTR_EL0. But it disallows the use of
>> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through
>> PMXEVCNTR_EL0.
>>
>> Linux itself doesn't violate this rule, but we may end up with
>> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that
>> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1,
>> despite the guest not having done anything wrong.
>>
>> In order to avoid this unfortunate course of events (haha!), let's
>> apply the same method armv8pmu_write_counter and co are using,
>> explicitely checking for the cycle counter and writing to
>> PMCCFILTR_EL0 directly. This prevents writing 0x1f to PMSELR_EL0,
>> and saves a Linux guest an extra trap.
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>>  arch/arm64/kernel/perf_event.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index 57ae9d9..a65b757 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -632,7 +632,10 @@ static inline void armv8pmu_write_counter(struct 
>> perf_event *event, u32 value)
>>  
>>  static inline void armv8pmu_write_evtype(int idx, u32 val)
>>  {
>> -if (armv8pmu_select_counter(idx) == idx) {
>> +if (idx == ARMV8_IDX_CYCLE_COUNTER) {
>> +val &= ARMV8_PMU_EVTYPE_MASK & ~ARMV8_PMU_EVTYPE_EVENT;
>> +write_sysreg(val, pmccfiltr_el0);
>> +} else if (armv8pmu_select_counter(idx) == idx) {
> 
> If we go down this route, then we also have to "fix" the 32-bit code,
> which uses PMSELR in a similar way. However, neither of the perf drivers
> are actually doing anything wrong here -- the problem comes about because
> the architecture doesn't guarantee that PMU accesses trap to EL2 unless
> both MDCR.TPM=1 *and* PMSELR_EL0 is valid. So I think that this should
> be handled together, in the KVM code that enables PMU traps.
> 
> Given that the perf callbacks tend to run with preemption disabled, I
> think you should be fine nuking PMSELR_EL0 to zero (i.e. no need to
> save/restore).

Fair enough. I'll respin another patch in a bit.

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: [PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Christoffer Dall]

On Tue, Dec 06, 2016 at 01:09:26PM +, Marc Zyngier wrote:
> On 06/12/16 12:16, Christoffer Dall wrote:
> > On Tue, Dec 06, 2016 at 01:12:21PM +0100, Christoffer Dall wrote:
> >> On Tue, Dec 06, 2016 at 11:17:40AM +, Marc Zyngier wrote:
> >>> On 01/12/16 19:32, Jintack Lim wrote:
>  Current KVM world switch code is unintentionally setting wrong bits to
>  CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
>  timer.  Bit positions of CNTHCTL_EL2 are changing depending on
>  HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
>  not set, but they are 11th and 10th bits respectively when E2H is set.
> 
>  In fact, on VHE we only need to set those bits once, not for every world
>  switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
>  1, which makes those bits have no effect for the host kernel execution.
>  So we just set those bits once for guests, and that's it.
> 
>  Signed-off-by: Jintack Lim 
>  ---
>  v2->v3: 
>  - Perform the initialization including CPU hotplug case.
>  - Move has_vhe() to asm/virt.h
> 
>  v1->v2: 
>  - Skip configuring cnthctl_el2 in world switch path on VHE system.
>  - Write patch based on linux-next.
>  ---
>   arch/arm/include/asm/virt.h   |  5 +
>   arch/arm/kvm/arm.c|  3 +++
>   arch/arm64/include/asm/virt.h | 10 ++
>   include/kvm/arm_arch_timer.h  |  1 +
>   virt/kvm/arm/arch_timer.c | 23 +++
>   virt/kvm/arm/hyp/timer-sr.c   | 33 +
>   6 files changed, 63 insertions(+), 12 deletions(-)
> 
>  diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
>  index a2e75b8..6dae195 100644
>  --- a/arch/arm/include/asm/virt.h
>  +++ b/arch/arm/include/asm/virt.h
>  @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return false;
>   }
>   
>  +static inline bool has_vhe(void)
>  +{
>  +return false;
>  +}
>  +
>   /* The section containing the hypervisor idmap text */
>   extern char __hyp_idmap_text_start[];
>   extern char __hyp_idmap_text_end[];
>  diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>  index 8f92efa..13e54e8 100644
>  --- a/arch/arm/kvm/arm.c
>  +++ b/arch/arm/kvm/arm.c
>  @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
>   __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
>   __cpu_init_stage2();
>   
>  +if (is_kernel_in_hyp_mode())
>  +kvm_timer_init_vhe();
>  +
>   kvm_arm_init_debug();
>   }
>   
>  diff --git a/arch/arm64/include/asm/virt.h 
>  b/arch/arm64/include/asm/virt.h
>  index fea1073..b043cfd 100644
>  --- a/arch/arm64/include/asm/virt.h
>  +++ b/arch/arm64/include/asm/virt.h
>  @@ -47,6 +47,7 @@
>   #include 
>   #include 
>   #include 
>  +#include 
>   
>   /*
>    * __boot_cpu_mode records what mode CPUs were booted in.
>  @@ -80,6 +81,15 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return read_sysreg(CurrentEL) == CurrentEL_EL2;
>   }
>   
>  +static inline bool has_vhe(void)
>  +{
>  +#ifdef CONFIG_ARM64_VHE
> >>>
> >>> Is there a particular reason why this should be guarded by a #ifdef? All
> >>> the symbols should always be available, and since this is a static key,
> >>> the overhead should be really insignificant (provided that you use a
> >>> non-prehistoric compiler...).
> >>>
> >>
> >> Isn't this code called from a file shared between 32-bit arm and arm64?
> >> Does the cpus_have_const_cap work on ARM64?
> > 
> > Duh, I meant on 32-bit arm of course.
> 
> Well, this is a pure 64bit file - 32bit has the canonical implementation
> that always returns false. So I can't really see how this can ever break
> 32bit. Unless my lack of sleep is really showing, and I'm missing
> something terribly obvious?
> 
No, I'm being an idiot, too many things at once and a lack of coffee.

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

Re: [PATCH v9 04/11] irqchip/gic-v3: Add missing system register definitions

[Auger Eric]

Hi,
On 23/11/2016 14:01, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K 
> 
> Define register definitions for ICH_VMCR_EL2, ICC_CTLR_EL1 and
> ICH_VTR_EL2, ICC_BPR0_EL1, ICC_BPR1_EL1 registers.
> 
> Signed-off-by: Vijaya Kumar K 

> ---
>  include/linux/irqchip/arm-gic-v3.h | 43 
> --
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/irqchip/arm-gic-v3.h 
> b/include/linux/irqchip/arm-gic-v3.h
> index 0deea34..b4f8287 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -352,8 +352,30 @@
>  /*
>   * CPU interface registers
>   */
> -#define ICC_CTLR_EL1_EOImode_drop_dir(0U << 1)
> -#define ICC_CTLR_EL1_EOImode_drop(1U << 1)
> +#define ICC_CTLR_EL1_EOImode_SHIFT   (1)
> +#define ICC_CTLR_EL1_EOImode_drop_dir(0U << 
> ICC_CTLR_EL1_EOImode_SHIFT)
> +#define ICC_CTLR_EL1_EOImode_drop(1U << ICC_CTLR_EL1_EOImode_SHIFT)
> +#define ICC_CTLR_EL1_EOImode_MASK(1 << ICC_CTLR_EL1_EOImode_SHIFT)
> +#define ICC_CTLR_EL1_CBPR_SHIFT  0
> +#define ICC_CTLR_EL1_CBPR_MASK   (1 << ICC_CTLR_EL1_CBPR_SHIFT)
> +#define ICC_CTLR_EL1_PRI_BITS_SHIFT  8
> +#define ICC_CTLR_EL1_PRI_BITS_MASK   (0x7 << ICC_CTLR_EL1_PRI_BITS_SHIFT)
> +#define ICC_CTLR_EL1_ID_BITS_SHIFT   11
> +#define ICC_CTLR_EL1_ID_BITS_MASK(0x7 << ICC_CTLR_EL1_ID_BITS_SHIFT)
> +#define ICC_CTLR_EL1_SEIS_SHIFT  14
> +#define ICC_CTLR_EL1_SEIS_MASK   (0x1 << ICC_CTLR_EL1_SEIS_SHIFT)
> +#define ICC_CTLR_EL1_A3V_SHIFT   15
> +#define ICC_CTLR_EL1_A3V_MASK(0x1 << ICC_CTLR_EL1_A3V_SHIFT)
> +#define ICC_PMR_EL1_SHIFT0
> +#define ICC_PMR_EL1_MASK (0xff << ICC_PMR_EL1_SHIFT)
> +#define ICC_BPR0_EL1_SHIFT   0
> +#define ICC_BPR0_EL1_MASK(0x7 << ICC_BPR0_EL1_SHIFT)
> +#define ICC_BPR1_EL1_SHIFT   0
> +#define ICC_BPR1_EL1_MASK(0x7 << ICC_BPR1_EL1_SHIFT)
> +#define ICC_IGRPEN0_EL1_SHIFT0
> +#define ICC_IGRPEN0_EL1_MASK (1 << ICC_IGRPEN0_EL1_SHIFT)
> +#define ICC_IGRPEN1_EL1_SHIFT0
> +#define ICC_IGRPEN1_EL1_MASK (1 << ICC_IGRPEN1_EL1_SHIFT)
>  #define ICC_SRE_EL1_SRE  (1U << 0)
>  
>  /*
> @@ -384,12 +406,29 @@
>  
>  #define ICH_VMCR_CTLR_SHIFT  0
>  #define ICH_VMCR_CTLR_MASK   (0x21f << ICH_VMCR_CTLR_SHIFT)
> +#define ICH_VMCR_CBPR_SHIFT  4
> +#define ICH_VMCR_CBPR_MASK   (1 << ICH_VMCR_CBPR_SHIFT)
> +#define ICH_VMCR_EOIM_SHIFT  9
> +#define ICH_VMCR_EOIM_MASK   (1 << ICH_VMCR_EOIM_SHIFT)
>  #define ICH_VMCR_BPR1_SHIFT  18
>  #define ICH_VMCR_BPR1_MASK   (7 << ICH_VMCR_BPR1_SHIFT)
>  #define ICH_VMCR_BPR0_SHIFT  21
>  #define ICH_VMCR_BPR0_MASK   (7 << ICH_VMCR_BPR0_SHIFT)
>  #define ICH_VMCR_PMR_SHIFT   24
>  #define ICH_VMCR_PMR_MASK(0xffUL << ICH_VMCR_PMR_SHIFT)
> +#define ICH_VMCR_ENG0_SHIFT  0
> +#define ICH_VMCR_ENG0_MASK   (1 << ICH_VMCR_ENG0_SHIFT)
> +#define ICH_VMCR_ENG1_SHIFT  1
> +#define ICH_VMCR_ENG1_MASK   (1 << ICH_VMCR_ENG1_SHIFT)
Besides the fact the V* was omitted (for instance VENG0) this looks good
to me.
> +
> +#define ICH_VTR_PRI_BITS_SHIFT   29
> +#define ICH_VTR_PRI_BITS_MASK(7 << ICH_VTR_PRI_BITS_SHIFT)
> +#define ICH_VTR_ID_BITS_SHIFT23
> +#define ICH_VTR_ID_BITS_MASK (7 << ICH_VTR_ID_BITS_SHIFT)
> +#define ICH_VTR_SEIS_SHIFT   22
> +#define ICH_VTR_SEIS_MASK(1 << ICH_VTR_SEIS_SHIFT)
> +#define ICH_VTR_A3V_SHIFT21
> +#define ICH_VTR_A3V_MASK (1 << ICH_VTR_A3V_SHIFT)
Some fields are omitted but I guess they are not used.

Reviewed-by: Eric Auger 

Eric

>  
>  #define ICC_IAR1_EL1_SPURIOUS0x3ff
>  
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/2] arm64: PMU: Do not use PMSELR_EL0 to access PMCCFILTR_EL0

[Will Deacon]

On Fri, Dec 02, 2016 at 03:50:58PM +, Marc Zyngier wrote:
> The ARMv8 architecture allows the cycle counter to be configured
> by setting PMSELR_EL0.SEL==0x1f and then accessing PMXEVTYPER_EL0,
> hence accessing PMCCFILTR_EL0. But it disallows the use of
> PMSELR_EL0.SEL==0x1f to access the cycle counter itself through
> PMXEVCNTR_EL0.
> 
> Linux itself doesn't violate this rule, but we may end up with
> PMSELR_EL0.SEL being set to 0x1f when we enter a guest. If that
> guest accesses PMXEVCNTR_EL0, the access may UNDEF at EL1,
> despite the guest not having done anything wrong.
> 
> In order to avoid this unfortunate course of events (haha!), let's
> apply the same method armv8pmu_write_counter and co are using,
> explicitely checking for the cycle counter and writing to
> PMCCFILTR_EL0 directly. This prevents writing 0x1f to PMSELR_EL0,
> and saves a Linux guest an extra trap.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  arch/arm64/kernel/perf_event.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
> index 57ae9d9..a65b757 100644
> --- a/arch/arm64/kernel/perf_event.c
> +++ b/arch/arm64/kernel/perf_event.c
> @@ -632,7 +632,10 @@ static inline void armv8pmu_write_counter(struct 
> perf_event *event, u32 value)
>  
>  static inline void armv8pmu_write_evtype(int idx, u32 val)
>  {
> - if (armv8pmu_select_counter(idx) == idx) {
> + if (idx == ARMV8_IDX_CYCLE_COUNTER) {
> + val &= ARMV8_PMU_EVTYPE_MASK & ~ARMV8_PMU_EVTYPE_EVENT;
> + write_sysreg(val, pmccfiltr_el0);
> + } else if (armv8pmu_select_counter(idx) == idx) {

If we go down this route, then we also have to "fix" the 32-bit code,
which uses PMSELR in a similar way. However, neither of the perf drivers
are actually doing anything wrong here -- the problem comes about because
the architecture doesn't guarantee that PMU accesses trap to EL2 unless
both MDCR.TPM=1 *and* PMSELR_EL0 is valid. So I think that this should
be handled together, in the KVM code that enables PMU traps.

Given that the perf callbacks tend to run with preemption disabled, I
think you should be fine nuking PMSELR_EL0 to zero (i.e. no need to
save/restore).

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

Re: [PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

Hi Christoffer,
Thanks for the review.

On Tue, Dec 6, 2016 at 7:12 AM, Christoffer Dall
 wrote:
> On Tue, Dec 06, 2016 at 11:17:40AM +, Marc Zyngier wrote:
>> On 01/12/16 19:32, Jintack Lim wrote:
>> > Current KVM world switch code is unintentionally setting wrong bits to
>> > CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
>> > timer.  Bit positions of CNTHCTL_EL2 are changing depending on
>> > HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
>> > not set, but they are 11th and 10th bits respectively when E2H is set.
>> >
>> > In fact, on VHE we only need to set those bits once, not for every world
>> > switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
>> > 1, which makes those bits have no effect for the host kernel execution.
>> > So we just set those bits once for guests, and that's it.
>> >
>> > Signed-off-by: Jintack Lim 
>> > ---
>> > v2->v3:
>> > - Perform the initialization including CPU hotplug case.
>> > - Move has_vhe() to asm/virt.h
>> >
>> > v1->v2:
>> > - Skip configuring cnthctl_el2 in world switch path on VHE system.
>> > - Write patch based on linux-next.
>> > ---
>> >  arch/arm/include/asm/virt.h   |  5 +
>> >  arch/arm/kvm/arm.c|  3 +++
>> >  arch/arm64/include/asm/virt.h | 10 ++
>> >  include/kvm/arm_arch_timer.h  |  1 +
>> >  virt/kvm/arm/arch_timer.c | 23 +++
>> >  virt/kvm/arm/hyp/timer-sr.c   | 33 +
>> >  6 files changed, 63 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
>> > index a2e75b8..6dae195 100644
>> > --- a/arch/arm/include/asm/virt.h
>> > +++ b/arch/arm/include/asm/virt.h
>> > @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
>> > return false;
>> >  }
>> >
>> > +static inline bool has_vhe(void)
>> > +{
>> > +   return false;
>> > +}

This is the one called on 32-bit arm.

>> > +
>> >  /* The section containing the hypervisor idmap text */
>> >  extern char __hyp_idmap_text_start[];
>> >  extern char __hyp_idmap_text_end[];
>> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> > index 8f92efa..13e54e8 100644
>> > --- a/arch/arm/kvm/arm.c
>> > +++ b/arch/arm/kvm/arm.c
>> > @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
>> > __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
>> > __cpu_init_stage2();
>> >
>> > +   if (is_kernel_in_hyp_mode())
>> > +   kvm_timer_init_vhe();
>> > +
>> > kvm_arm_init_debug();
>> >  }
>> >
>> > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
>> > index fea1073..b043cfd 100644
>> > --- a/arch/arm64/include/asm/virt.h
>> > +++ b/arch/arm64/include/asm/virt.h
>> > @@ -47,6 +47,7 @@
>> >  #include 
>> >  #include 
>> >  #include 
>> > +#include 
>> >
>> >  /*
>> >   * __boot_cpu_mode records what mode CPUs were booted in.
>> > @@ -80,6 +81,15 @@ static inline bool is_kernel_in_hyp_mode(void)
>> > return read_sysreg(CurrentEL) == CurrentEL_EL2;
>> >  }
>> >
>> > +static inline bool has_vhe(void)
>> > +{
>> > +#ifdef CONFIG_ARM64_VHE
>>
>> Is there a particular reason why this should be guarded by a #ifdef? All
>> the symbols should always be available, and since this is a static key,
>> the overhead should be really insignificant (provided that you use a
>> non-prehistoric compiler...).
>>
>
> Isn't this code called from a file shared between 32-bit arm and arm64?

This code is only for ARM64. See above for 32-bit arm.

> Does the cpus_have_const_cap work on ARM64?
>
> -Christoffer
>

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

Re: [PATCH v9 03/11] arm/arm64: vgic: Introduce find_reg_by_id()

[Auger Eric]

Hi Vijay,
On 23/11/2016 14:01, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K 
> 
> In order to implement vGICv3 CPU interface access, we will need to perform
> table lookup of system registers. We would need both index_to_params() and
> find_reg() exported for that purpose, but instead we export a single
> function which combines them both.
> 
> Signed-off-by: Pavel Fedin 
> Signed-off-by: Vijaya Kumar K 
> Reviewed-by: Andre Przywara 
> Acked-by: Christoffer Dall 
> ---
>  arch/arm64/kvm/sys_regs.c | 22 +++---
>  arch/arm64/kvm/sys_regs.h |  4 
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index f302fdb..1330d4c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1789,6 +1789,17 @@ static bool index_to_params(u64 id, struct 
> sys_reg_params *params)
>   }
>  }
>  
> +const struct sys_reg_desc *find_reg_by_id(u64 id,
> +   struct sys_reg_params *params,
> +   const struct sys_reg_desc table[],
> +   unsigned int num)
> +{
> + if (!index_to_params(id, params))
> + return NULL;
> +
> + return find_reg(params, table, num);
> +}
> +
Can't you use find_reg_by_id in index_to_sys_reg_desc too?

Besides Reviewed-by: Eric Auger 

Thanks

Eric
>  /* Decode an index value, and find the sys_reg_desc entry. */
>  static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu 
> *vcpu,
>   u64 id)
> @@ -1912,10 +1923,8 @@ static int get_invariant_sys_reg(u64 id, void __user 
> *uaddr)
>   struct sys_reg_params params;
>   const struct sys_reg_desc *r;
>  
> - if (!index_to_params(id, ))
> - return -ENOENT;
> -
> - r = find_reg(, invariant_sys_regs, 
> ARRAY_SIZE(invariant_sys_regs));
> + r = find_reg_by_id(id, , invariant_sys_regs,
> +ARRAY_SIZE(invariant_sys_regs));
>   if (!r)
>   return -ENOENT;
>  
> @@ -1929,9 +1938,8 @@ static int set_invariant_sys_reg(u64 id, void __user 
> *uaddr)
>   int err;
>   u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
>  
> - if (!index_to_params(id, ))
> - return -ENOENT;
> - r = find_reg(, invariant_sys_regs, 
> ARRAY_SIZE(invariant_sys_regs));
> + r = find_reg_by_id(id, , invariant_sys_regs,
> +ARRAY_SIZE(invariant_sys_regs));
>   if (!r)
>   return -ENOENT;
>  
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index dbbb01c..9c6ffd0 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -136,6 +136,10 @@ static inline int cmp_sys_reg(const struct sys_reg_desc 
> *i1,
>   return i1->Op2 - i2->Op2;
>  }
>  
> +const struct sys_reg_desc *find_reg_by_id(u64 id,
> +   struct sys_reg_params *params,
> +   const struct sys_reg_desc table[],
> +   unsigned int num);
>  
>  #define Op0(_x)  .Op0 = _x
>  #define Op1(_x)  .Op1 = _x
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Jintack Lim]

On Tue, Dec 6, 2016 at 6:17 AM, Marc Zyngier  wrote:
> On 01/12/16 19:32, Jintack Lim wrote:
>> Current KVM world switch code is unintentionally setting wrong bits to
>> CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
>> timer.  Bit positions of CNTHCTL_EL2 are changing depending on
>> HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
>> not set, but they are 11th and 10th bits respectively when E2H is set.
>>
>> In fact, on VHE we only need to set those bits once, not for every world
>> switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
>> 1, which makes those bits have no effect for the host kernel execution.
>> So we just set those bits once for guests, and that's it.
>>
>> Signed-off-by: Jintack Lim 
>> ---
>> v2->v3:
>> - Perform the initialization including CPU hotplug case.
>> - Move has_vhe() to asm/virt.h
>>
>> v1->v2:
>> - Skip configuring cnthctl_el2 in world switch path on VHE system.
>> - Write patch based on linux-next.
>> ---
>>  arch/arm/include/asm/virt.h   |  5 +
>>  arch/arm/kvm/arm.c|  3 +++
>>  arch/arm64/include/asm/virt.h | 10 ++
>>  include/kvm/arm_arch_timer.h  |  1 +
>>  virt/kvm/arm/arch_timer.c | 23 +++
>>  virt/kvm/arm/hyp/timer-sr.c   | 33 +
>>  6 files changed, 63 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
>> index a2e75b8..6dae195 100644
>> --- a/arch/arm/include/asm/virt.h
>> +++ b/arch/arm/include/asm/virt.h
>> @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
>>   return false;
>>  }
>>
>> +static inline bool has_vhe(void)
>> +{
>> + return false;
>> +}
>> +
>>  /* The section containing the hypervisor idmap text */
>>  extern char __hyp_idmap_text_start[];
>>  extern char __hyp_idmap_text_end[];
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 8f92efa..13e54e8 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
>>   __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
>>   __cpu_init_stage2();
>>
>> + if (is_kernel_in_hyp_mode())
>> + kvm_timer_init_vhe();
>> +
>>   kvm_arm_init_debug();
>>  }
>>
>> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
>> index fea1073..b043cfd 100644
>> --- a/arch/arm64/include/asm/virt.h
>> +++ b/arch/arm64/include/asm/virt.h
>> @@ -47,6 +47,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  /*
>>   * __boot_cpu_mode records what mode CPUs were booted in.
>> @@ -80,6 +81,15 @@ static inline bool is_kernel_in_hyp_mode(void)
>>   return read_sysreg(CurrentEL) == CurrentEL_EL2;
>>  }
>>
>> +static inline bool has_vhe(void)
>> +{
>> +#ifdef CONFIG_ARM64_VHE
>
> Is there a particular reason why this should be guarded by a #ifdef? All
> the symbols should always be available, and since this is a static key,
> the overhead should be really insignificant (provided that you use a
> non-prehistoric compiler...).

It was only for reducing overhead on non-VHE system.

>
>> + if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
>> + return true;
>> +#endif
>> + return false;
>> +}
>> +
>>  #ifdef CONFIG_ARM64_VHE
>>  extern void verify_cpu_run_el(void);
>>  #else
>> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
>> index dda39d8..2d54903 100644
>> --- a/include/kvm/arm_arch_timer.h
>> +++ b/include/kvm/arm_arch_timer.h
>> @@ -76,4 +76,5 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>
>>  void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
>>
>> +void kvm_timer_init_vhe(void);
>>  #endif
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 17b8fa5..c7c3bfd 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -24,6 +24,7 @@
>>
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -507,3 +508,25 @@ void kvm_timer_init(struct kvm *kvm)
>>  {
>>   kvm->arch.timer.cntvoff = kvm_phys_timer_read();
>>  }
>> +
>> +/*
>> + * On VHE system, we only need to configure trap on physical timer and 
>> counter
>> + * accesses in EL0 and EL1 once, not for every world switch.
>> + * The host kernel runs at EL2 with HCR_EL2.TGE == 1,
>> + * and this makes those bits have no effect for the host kernel execution.
>> + */
>> +void kvm_timer_init_vhe(void)
>> +{
>> + /* When HCR_EL2.E2H ==1, EL1PCEN and EL1PCTEN are shifted by 10 */
>> + u32 cnthctl_shift = 10;
>> + u64 val;
>> +
>> + /*
>> +  * Disallow physical timer access for the guest.
>> +  * Physical counter access is allowed.
>> +  */
>> + val = read_sysreg(cnthctl_el2);
>> + val &= ~(CNTHCTL_EL1PCEN << cnthctl_shift);
>> + val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
>> + write_sysreg(val, 

Re: [PATCH v9 02/11] arm/arm64: vgic: Add distributor and redistributor access

[Auger Eric]

Hi,

On 28/11/2016 14:08, Christoffer Dall wrote:
> On Wed, Nov 23, 2016 at 06:31:49PM +0530, vijay.kil...@gmail.com wrote:
>> From: Vijaya Kumar K 
>>
>> VGICv3 Distributor and Redistributor registers are accessed using
>> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_REDIST_REGS
>> with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
>> These registers are accessed as 32-bit and cpu mpidr
>> value passed along with register offset is used to identify the
>> cpu for redistributor registers access.
>>
>> The version of VGIC v3 specification is define here
s/define/defined
>> Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>>
>> Also update arch/arm/include/uapi/asm/kvm.h to compile for
>> AArch32 mode.
>>
>> Signed-off-by: Vijaya Kumar K 
>> ---
>>  arch/arm/include/uapi/asm/kvm.h |   4 +
>>  arch/arm64/include/uapi/asm/kvm.h   |   4 +
>>  virt/kvm/arm/vgic/vgic-kvm-device.c | 144 
>> ++--
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c|  16 +---
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  72 ++
>>  virt/kvm/arm/vgic/vgic-mmio.c   |  22 ++
>>  virt/kvm/arm/vgic/vgic-mmio.h   |   4 +
>>  virt/kvm/arm/vgic/vgic.h|  49 +++-
>>  8 files changed, 292 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/arm/include/uapi/asm/kvm.h 
>> b/arch/arm/include/uapi/asm/kvm.h
>> index af05f8e..0ae6035 100644
>> --- a/arch/arm/include/uapi/asm/kvm.h
>> +++ b/arch/arm/include/uapi/asm/kvm.h
>> @@ -181,10 +181,14 @@ struct kvm_arch_memory_slot {
>>  #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS   2
>>  #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT  32
>>  #define   KVM_DEV_ARM_VGIC_CPUID_MASK   (0xffULL << 
>> KVM_DEV_ARM_VGIC_CPUID_SHIFT)
>> +#define   KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT 32
>> +#define   KVM_DEV_ARM_VGIC_V3_MPIDR_MASK \
>> +(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_GRP_NR_IRQS3
>>  #define KVM_DEV_ARM_VGIC_GRP_CTRL   4
>> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT0
>>  
>>  /* KVM_IRQ_LINE irq field index values */
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> b/arch/arm64/include/uapi/asm/kvm.h
>> index 3051f86..56dc08d 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -201,10 +201,14 @@ struct kvm_arch_memory_slot {
>>  #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS   2
>>  #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT  32
>>  #define   KVM_DEV_ARM_VGIC_CPUID_MASK   (0xffULL << 
>> KVM_DEV_ARM_VGIC_CPUID_SHIFT)
>> +#define   KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT 32
>> +#define   KVM_DEV_ARM_VGIC_V3_MPIDR_MASK \
>> +(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_GRP_NR_IRQS3
>>  #define KVM_DEV_ARM_VGIC_GRP_CTRL   4
>> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT0
>>  
>>  /* Device Control API on vcpu fd */
>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c 
>> b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> index fbe87a6..bc7de95 100644
>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> @@ -235,7 +235,7 @@ struct vgic_reg_attr {
>>  gpa_t addr;
>>  };
>>  
>> -static int parse_vgic_v2_attr(struct kvm_device *dev,
>> +static int vgic_v2_parse_attr(struct kvm_device *dev,
>>struct kvm_device_attr *attr,
>>struct vgic_reg_attr *reg_attr)
>>  {
>> @@ -292,14 +292,14 @@ static bool lock_all_vcpus(struct kvm *kvm)
>>  }
>>  
>>  /**
>> - * vgic_attr_regs_access_v2 - allows user space to access VGIC v2 state
>> + * vgic_v2_attr_regs_access - allows user space to access VGIC v2 state
>>   *
>>   * @dev:  kvm device handle
>>   * @attr: kvm device attribute
>>   * @reg:  address the value is read or written
>>   * @is_write: true if userspace is writing a register
>>   */
>> -static int vgic_attr_regs_access_v2(struct kvm_device *dev,
>> +static int vgic_v2_attr_regs_access(struct kvm_device *dev,
>>  struct kvm_device_attr *attr,
>>  u32 *reg, bool is_write)
>>  {
>> @@ -308,7 +308,7 @@ static int vgic_attr_regs_access_v2(struct kvm_device 
>> *dev,
>>  struct kvm_vcpu *vcpu;
>>  int ret;
>>  
>> -ret = parse_vgic_v2_attr(dev, attr, _attr);
>> +ret = vgic_v2_parse_attr(dev, attr, _attr);
>>  if (ret)
>>  return ret;
>>  
>> @@ -362,7 +362,7 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
>>  if 

Re: [PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 06/12/16 12:16, Christoffer Dall wrote:
> On Tue, Dec 06, 2016 at 01:12:21PM +0100, Christoffer Dall wrote:
>> On Tue, Dec 06, 2016 at 11:17:40AM +, Marc Zyngier wrote:
>>> On 01/12/16 19:32, Jintack Lim wrote:
 Current KVM world switch code is unintentionally setting wrong bits to
 CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
 timer.  Bit positions of CNTHCTL_EL2 are changing depending on
 HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
 not set, but they are 11th and 10th bits respectively when E2H is set.

 In fact, on VHE we only need to set those bits once, not for every world
 switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
 1, which makes those bits have no effect for the host kernel execution.
 So we just set those bits once for guests, and that's it.

 Signed-off-by: Jintack Lim 
 ---
 v2->v3: 
 - Perform the initialization including CPU hotplug case.
 - Move has_vhe() to asm/virt.h

 v1->v2: 
 - Skip configuring cnthctl_el2 in world switch path on VHE system.
 - Write patch based on linux-next.
 ---
  arch/arm/include/asm/virt.h   |  5 +
  arch/arm/kvm/arm.c|  3 +++
  arch/arm64/include/asm/virt.h | 10 ++
  include/kvm/arm_arch_timer.h  |  1 +
  virt/kvm/arm/arch_timer.c | 23 +++
  virt/kvm/arm/hyp/timer-sr.c   | 33 +
  6 files changed, 63 insertions(+), 12 deletions(-)

 diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
 index a2e75b8..6dae195 100644
 --- a/arch/arm/include/asm/virt.h
 +++ b/arch/arm/include/asm/virt.h
 @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
return false;
  }
  
 +static inline bool has_vhe(void)
 +{
 +  return false;
 +}
 +
  /* The section containing the hypervisor idmap text */
  extern char __hyp_idmap_text_start[];
  extern char __hyp_idmap_text_end[];
 diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
 index 8f92efa..13e54e8 100644
 --- a/arch/arm/kvm/arm.c
 +++ b/arch/arm/kvm/arm.c
 @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
__cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
__cpu_init_stage2();
  
 +  if (is_kernel_in_hyp_mode())
 +  kvm_timer_init_vhe();
 +
kvm_arm_init_debug();
  }
  
 diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
 index fea1073..b043cfd 100644
 --- a/arch/arm64/include/asm/virt.h
 +++ b/arch/arm64/include/asm/virt.h
 @@ -47,6 +47,7 @@
  #include 
  #include 
  #include 
 +#include 
  
  /*
   * __boot_cpu_mode records what mode CPUs were booted in.
 @@ -80,6 +81,15 @@ static inline bool is_kernel_in_hyp_mode(void)
return read_sysreg(CurrentEL) == CurrentEL_EL2;
  }
  
 +static inline bool has_vhe(void)
 +{
 +#ifdef CONFIG_ARM64_VHE
>>>
>>> Is there a particular reason why this should be guarded by a #ifdef? All
>>> the symbols should always be available, and since this is a static key,
>>> the overhead should be really insignificant (provided that you use a
>>> non-prehistoric compiler...).
>>>
>>
>> Isn't this code called from a file shared between 32-bit arm and arm64?
>> Does the cpus_have_const_cap work on ARM64?
> 
> Duh, I meant on 32-bit arm of course.

Well, this is a pure 64bit file - 32bit has the canonical implementation
that always returns false. So I can't really see how this can ever break
32bit. Unless my lack of sleep is really showing, and I'm missing
something terribly obvious?

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: [PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Christoffer Dall]

On Thu, Dec 01, 2016 at 02:32:05PM -0500, Jintack Lim wrote:
> Current KVM world switch code is unintentionally setting wrong bits to
> CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
> timer.  Bit positions of CNTHCTL_EL2 are changing depending on
> HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
> not set, but they are 11th and 10th bits respectively when E2H is set.
> 
> In fact, on VHE we only need to set those bits once, not for every world
> switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
> 1, which makes those bits have no effect for the host kernel execution.
> So we just set those bits once for guests, and that's it.
> 
> Signed-off-by: Jintack Lim 
> ---
> v2->v3: 
> - Perform the initialization including CPU hotplug case.
> - Move has_vhe() to asm/virt.h
> 
> v1->v2: 
> - Skip configuring cnthctl_el2 in world switch path on VHE system.
> - Write patch based on linux-next.
> ---
>  arch/arm/include/asm/virt.h   |  5 +
>  arch/arm/kvm/arm.c|  3 +++
>  arch/arm64/include/asm/virt.h | 10 ++
>  include/kvm/arm_arch_timer.h  |  1 +
>  virt/kvm/arm/arch_timer.c | 23 +++
>  virt/kvm/arm/hyp/timer-sr.c   | 33 +
>  6 files changed, 63 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
> index a2e75b8..6dae195 100644
> --- a/arch/arm/include/asm/virt.h
> +++ b/arch/arm/include/asm/virt.h
> @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return false;
>  }
>  
> +static inline bool has_vhe(void)
> +{
> + return false;
> +}
> +
>  /* The section containing the hypervisor idmap text */
>  extern char __hyp_idmap_text_start[];
>  extern char __hyp_idmap_text_end[];
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 8f92efa..13e54e8 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
>   __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
>   __cpu_init_stage2();
>  
> + if (is_kernel_in_hyp_mode())
> + kvm_timer_init_vhe();
> +
>   kvm_arm_init_debug();
>  }
>  
> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> index fea1073..b043cfd 100644
> --- a/arch/arm64/include/asm/virt.h
> +++ b/arch/arm64/include/asm/virt.h
> @@ -47,6 +47,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * __boot_cpu_mode records what mode CPUs were booted in.
> @@ -80,6 +81,15 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return read_sysreg(CurrentEL) == CurrentEL_EL2;
>  }
>  
> +static inline bool has_vhe(void)
> +{
> +#ifdef CONFIG_ARM64_VHE
> + if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
> + return true;
> +#endif
> + return false;
> +}
> +
>  #ifdef CONFIG_ARM64_VHE
>  extern void verify_cpu_run_el(void);
>  #else
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index dda39d8..2d54903 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -76,4 +76,5 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  
>  void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
>  
> +void kvm_timer_init_vhe(void);
>  #endif
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 17b8fa5..c7c3bfd 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -24,6 +24,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -507,3 +508,25 @@ void kvm_timer_init(struct kvm *kvm)
>  {
>   kvm->arch.timer.cntvoff = kvm_phys_timer_read();
>  }
> +
> +/*
> + * On VHE system, we only need to configure trap on physical timer and 
> counter
> + * accesses in EL0 and EL1 once, not for every world switch.
> + * The host kernel runs at EL2 with HCR_EL2.TGE == 1,
> + * and this makes those bits have no effect for the host kernel execution.
> + */
> +void kvm_timer_init_vhe(void)
> +{
> + /* When HCR_EL2.E2H ==1, EL1PCEN and EL1PCTEN are shifted by 10 */
> + u32 cnthctl_shift = 10;
> + u64 val;
> +
> + /*
> +  * Disallow physical timer access for the guest.
> +  * Physical counter access is allowed.
> +  */
> + val = read_sysreg(cnthctl_el2);
> + val &= ~(CNTHCTL_EL1PCEN << cnthctl_shift);
> + val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
> + write_sysreg(val, cnthctl_el2);
> +}
> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
> index 798866a..63e28dd 100644
> --- a/virt/kvm/arm/hyp/timer-sr.c
> +++ b/virt/kvm/arm/hyp/timer-sr.c
> @@ -35,10 +35,16 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>   /* Disable the virtual timer */
>   write_sysreg_el0(0, cntv_ctl);
>  
> - /* Allow physical timer/counter access for the host */
> - val = read_sysreg(cnthctl_el2);
> - val |= 

Re: [PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Christoffer Dall]

On Tue, Dec 06, 2016 at 01:12:21PM +0100, Christoffer Dall wrote:
> On Tue, Dec 06, 2016 at 11:17:40AM +, Marc Zyngier wrote:
> > On 01/12/16 19:32, Jintack Lim wrote:
> > > Current KVM world switch code is unintentionally setting wrong bits to
> > > CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
> > > timer.  Bit positions of CNTHCTL_EL2 are changing depending on
> > > HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
> > > not set, but they are 11th and 10th bits respectively when E2H is set.
> > > 
> > > In fact, on VHE we only need to set those bits once, not for every world
> > > switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
> > > 1, which makes those bits have no effect for the host kernel execution.
> > > So we just set those bits once for guests, and that's it.
> > > 
> > > Signed-off-by: Jintack Lim 
> > > ---
> > > v2->v3: 
> > > - Perform the initialization including CPU hotplug case.
> > > - Move has_vhe() to asm/virt.h
> > > 
> > > v1->v2: 
> > > - Skip configuring cnthctl_el2 in world switch path on VHE system.
> > > - Write patch based on linux-next.
> > > ---
> > >  arch/arm/include/asm/virt.h   |  5 +
> > >  arch/arm/kvm/arm.c|  3 +++
> > >  arch/arm64/include/asm/virt.h | 10 ++
> > >  include/kvm/arm_arch_timer.h  |  1 +
> > >  virt/kvm/arm/arch_timer.c | 23 +++
> > >  virt/kvm/arm/hyp/timer-sr.c   | 33 +
> > >  6 files changed, 63 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
> > > index a2e75b8..6dae195 100644
> > > --- a/arch/arm/include/asm/virt.h
> > > +++ b/arch/arm/include/asm/virt.h
> > > @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
> > >   return false;
> > >  }
> > >  
> > > +static inline bool has_vhe(void)
> > > +{
> > > + return false;
> > > +}
> > > +
> > >  /* The section containing the hypervisor idmap text */
> > >  extern char __hyp_idmap_text_start[];
> > >  extern char __hyp_idmap_text_end[];
> > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> > > index 8f92efa..13e54e8 100644
> > > --- a/arch/arm/kvm/arm.c
> > > +++ b/arch/arm/kvm/arm.c
> > > @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
> > >   __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
> > >   __cpu_init_stage2();
> > >  
> > > + if (is_kernel_in_hyp_mode())
> > > + kvm_timer_init_vhe();
> > > +
> > >   kvm_arm_init_debug();
> > >  }
> > >  
> > > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> > > index fea1073..b043cfd 100644
> > > --- a/arch/arm64/include/asm/virt.h
> > > +++ b/arch/arm64/include/asm/virt.h
> > > @@ -47,6 +47,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  /*
> > >   * __boot_cpu_mode records what mode CPUs were booted in.
> > > @@ -80,6 +81,15 @@ static inline bool is_kernel_in_hyp_mode(void)
> > >   return read_sysreg(CurrentEL) == CurrentEL_EL2;
> > >  }
> > >  
> > > +static inline bool has_vhe(void)
> > > +{
> > > +#ifdef CONFIG_ARM64_VHE
> > 
> > Is there a particular reason why this should be guarded by a #ifdef? All
> > the symbols should always be available, and since this is a static key,
> > the overhead should be really insignificant (provided that you use a
> > non-prehistoric compiler...).
> > 
> 
> Isn't this code called from a file shared between 32-bit arm and arm64?
> Does the cpus_have_const_cap work on ARM64?

Duh, I meant on 32-bit arm of course.

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

Re: [PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Christoffer Dall]

On Tue, Dec 06, 2016 at 11:17:40AM +, Marc Zyngier wrote:
> On 01/12/16 19:32, Jintack Lim wrote:
> > Current KVM world switch code is unintentionally setting wrong bits to
> > CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
> > timer.  Bit positions of CNTHCTL_EL2 are changing depending on
> > HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
> > not set, but they are 11th and 10th bits respectively when E2H is set.
> > 
> > In fact, on VHE we only need to set those bits once, not for every world
> > switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
> > 1, which makes those bits have no effect for the host kernel execution.
> > So we just set those bits once for guests, and that's it.
> > 
> > Signed-off-by: Jintack Lim 
> > ---
> > v2->v3: 
> > - Perform the initialization including CPU hotplug case.
> > - Move has_vhe() to asm/virt.h
> > 
> > v1->v2: 
> > - Skip configuring cnthctl_el2 in world switch path on VHE system.
> > - Write patch based on linux-next.
> > ---
> >  arch/arm/include/asm/virt.h   |  5 +
> >  arch/arm/kvm/arm.c|  3 +++
> >  arch/arm64/include/asm/virt.h | 10 ++
> >  include/kvm/arm_arch_timer.h  |  1 +
> >  virt/kvm/arm/arch_timer.c | 23 +++
> >  virt/kvm/arm/hyp/timer-sr.c   | 33 +
> >  6 files changed, 63 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
> > index a2e75b8..6dae195 100644
> > --- a/arch/arm/include/asm/virt.h
> > +++ b/arch/arm/include/asm/virt.h
> > @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
> > return false;
> >  }
> >  
> > +static inline bool has_vhe(void)
> > +{
> > +   return false;
> > +}
> > +
> >  /* The section containing the hypervisor idmap text */
> >  extern char __hyp_idmap_text_start[];
> >  extern char __hyp_idmap_text_end[];
> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> > index 8f92efa..13e54e8 100644
> > --- a/arch/arm/kvm/arm.c
> > +++ b/arch/arm/kvm/arm.c
> > @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
> > __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
> > __cpu_init_stage2();
> >  
> > +   if (is_kernel_in_hyp_mode())
> > +   kvm_timer_init_vhe();
> > +
> > kvm_arm_init_debug();
> >  }
> >  
> > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> > index fea1073..b043cfd 100644
> > --- a/arch/arm64/include/asm/virt.h
> > +++ b/arch/arm64/include/asm/virt.h
> > @@ -47,6 +47,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  /*
> >   * __boot_cpu_mode records what mode CPUs were booted in.
> > @@ -80,6 +81,15 @@ static inline bool is_kernel_in_hyp_mode(void)
> > return read_sysreg(CurrentEL) == CurrentEL_EL2;
> >  }
> >  
> > +static inline bool has_vhe(void)
> > +{
> > +#ifdef CONFIG_ARM64_VHE
> 
> Is there a particular reason why this should be guarded by a #ifdef? All
> the symbols should always be available, and since this is a static key,
> the overhead should be really insignificant (provided that you use a
> non-prehistoric compiler...).
> 

Isn't this code called from a file shared between 32-bit arm and arm64?
Does the cpus_have_const_cap work on ARM64?

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

Re: [PATCH v9 01/11] arm/arm64: vgic: Implement support for userspace access

[Auger Eric]

Hi,

On 28/11/2016 14:05, Christoffer Dall wrote:
> On Wed, Nov 23, 2016 at 06:31:48PM +0530, vijay.kil...@gmail.com wrote:
>> From: Vijaya Kumar K 
>>
>> Read and write of some registers like ISPENDR and ICPENDR
>> from userspace requires special handling when compared to
>> guest access for these registers.
>>
>> Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> for handling of ISPENDR, ICPENDR registers handling.
>>
>> Add infrastructure to support guest and userspace read
>> and write for the required registers
>> Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
>>
>> Signed-off-by: Vijaya Kumar K 
>> ---
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c |  25 --
>>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 102 
>> ---
>>  virt/kvm/arm/vgic/vgic-mmio.c|  78 +++---
>>  virt/kvm/arm/vgic/vgic-mmio.h|  19 
>>  4 files changed, 175 insertions(+), 49 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
>> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index b44b359..0b32f40 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, 
>> struct kvm_device_attr *attr)
>>  return -ENXIO;
>>  }
>>  
>> -/*
>> - * When userland tries to access the VGIC register handlers, we need to
>> - * create a usable struct vgic_io_device to be passed to the handlers and we
>> - * have to set up a buffer similar to what would have happened if a guest 
>> MMIO
>> - * access occurred, including doing endian conversions on BE systems.
>> - */
>> -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
>> -bool is_write, int offset, u32 *val)
>> -{
>> -unsigned int len = 4;
>> -u8 buf[4];
>> -int ret;
>> -
>> -if (is_write) {
>> -vgic_data_host_to_mmio_bus(buf, len, *val);
>> -ret = kvm_io_gic_ops.write(vcpu, >dev, offset, len, buf);
>> -} else {
>> -ret = kvm_io_gic_ops.read(vcpu, >dev, offset, len, buf);
>> -if (!ret)
>> -*val = vgic_data_mmio_bus_to_host(buf, len);
>> -}
>> -
>> -return ret;
>> -}
>> -
>>  int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>>int offset, u32 *val)
>>  {
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
>> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> index 50f42f0..8e76d04 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
>> @@ -207,6 +207,66 @@ static unsigned long vgic_mmio_read_v3_idregs(struct 
>> kvm_vcpu *vcpu,
>>  return 0;
>>  }
>>  
>> +static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
>> +  gpa_t addr, unsigned int len)
>> +{
>> +u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>> +u32 value = 0;
>> +int i;
>> +
>> +/*
>> + * A level triggerred interrupt pending state is latched in both
>> + * "soft_pending" and "line_level" variables. Userspace will save
>> + * and restore soft_pending and line_level separately.
>> + * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
>> + * handling of ISPENDR and ICPENDR.
>> + */
>> +for (i = 0; i < len * 8; i++) {
>> +struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>> +
>> +if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
>> +value |= (1U << i);
>> +if (irq->config == VGIC_CONFIG_EDGE && irq->pending)
>> +value |= (1U << i);
>> +
>> +vgic_put_irq(vcpu->kvm, irq);
>> +}
>> +
>> +return value;
>> +}
>> +
>> +static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
>> +  gpa_t addr, unsigned int len,
>> +  unsigned long val)
>> +{
>> +u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>> +int i;
>> +
>> +for (i = 0; i < len * 8; i++) {
>> +struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>> +
>> +spin_lock(>irq_lock);
>> +if (test_bit(i, )) {
>> +/* soft_pending is set irrespective of irq type
>> + * (level or edge) to avoid dependency that VM should
>> + * restore irq config before pending info.
>> + */
> 
> nit: kernel commenting style
> 
>> +irq->pending = true;
>> +irq->soft_pending = true;
>> +vgic_queue_irq_unlock(vcpu->kvm, irq);
>> +} else {
>> +irq->soft_pending = false;
>> +if (irq->config == VGIC_CONFIG_EDGE ||
>> +(irq->config == VGIC_CONFIG_LEVEL &&
>> +!irq->line_level))
>> + 

Re: [PATCH 2/2] KVM: arm/arm64: vgic-v2: Add the missing resetting LRs at boot time

[Marc Zyngier]

On 06/12/16 06:41, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> This is the corresponding part of commit d6400d7(KVM: arm/arm64:
> vgic-v2: Reset LRs at boot time) which is missed for new-vgic.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  virt/kvm/arm/vgic/vgic-v2.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index 9bab867..c636a19 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -300,6 +300,15 @@ int vgic_v2_map_resources(struct kvm *kvm)
>  
>  DEFINE_STATIC_KEY_FALSE(vgic_v2_cpuif_trap);
>  
> +static void vgic_cpu_init_lrs(void *params)
> +{
> + int i;
> +
> + for (i = 0; i < kvm_vgic_global_state.nr_lr; i++)
> + writel_relaxed(0, kvm_vgic_global_state.vctrl_base +
> +   GICH_LR0 + (i * 4));
> +}
> +
>  /**
>   * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
>   * @node:pointer to the DT node
> @@ -368,6 +377,8 @@ int vgic_v2_probe(const struct gic_kvm_info *info)
>   kvm_vgic_global_state.type = VGIC_V2;
>   kvm_vgic_global_state.max_gic_vcpus = VGIC_V2_MAX_CPUS;
>  
> + on_each_cpu(vgic_cpu_init_lrs, NULL, 1);
> +
>   kvm_info("vgic-v2@%llx\n", info->vctrl.start);
>  
>   return 0;
> 

Same remark as the GICv3 version.

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: [PATCH 1/2] arm64: KVM: vgic-v3: Add the missing resetting LRs at boot time

[Marc Zyngier]

Hi Shannon,

On 06/12/16 06:41, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> This is the corresponding part of commit 0d98d00(arm64: KVM:
> vgic-v3: Reset LRs at boot time) which is missed for new-vgic.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 5c9f974..7262f3b 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -307,6 +307,11 @@ int vgic_v3_map_resources(struct kvm *kvm)
>   return ret;
>  }
>  
> +static void vgic_cpu_init_lrs(void *params)
> +{
> + kvm_call_hyp(__vgic_v3_init_lrs);
> +}
> +
>  /**
>   * vgic_v3_probe - probe for a GICv3 compatible interrupt controller in DT
>   * @node:pointer to the DT node
> @@ -361,5 +366,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
>   kvm_vgic_global_state.type = VGIC_V3;
>   kvm_vgic_global_state.max_gic_vcpus = VGIC_V3_MAX_CPUS;
>  
> + on_each_cpu(vgic_cpu_init_lrs, NULL, 1);

I don't think that's enough. If you have CPU hotplug, your CPU will come
up with reset values long after boot. I think you need something that is
triggered from cpu_init_hyp_mode/cpu_hyp_reinit.

> +
>   return 0;
>  }
> 

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 RFC 00/15] arm/arm64: add ITS framework

[Andrew Jones]

On Tue, Dec 06, 2016 at 11:14:41AM +, Andre Przywara wrote:
> Hi,
> 
> On 06/12/16 09:48, Andrew Jones wrote:
> > On Mon, Dec 05, 2016 at 10:46:31PM +0100, Eric Auger wrote:
> >> This series proposes a framework to test the virtual ITS.
> >> This is based on Drew's v7 series [1]. The last patch tests
> >> several ITS commands (collection/device mapping, interrupt
> >> translation service entry creation and LPI trigger through INT
> >> command). At this point we don't use any external PCIe device
> >> to write into the GITS_TRANSLATER register.
> >>
> >> The bulk of the code derives from the ITS driver code so all
> >> the credit is due to Marc.
> >>
> >> Many other ITS commands could be tested. Also existing MMIO
> >> accesses could be enhanced into standalone tests. Current focus
> >> was to make it functional.
> >>
> >> The code deserves more cleanup with respect to cacheability
> >> attributes in general.
> >>
> >> Tested on Cavium ThunderX [2].
> >>
> >> Best Regards
> >>
> >> Eric
> >>
> >> [1] [kvm-unit-tests PATCH v7 00/11] arm/arm64: add gic framework
> >>
> >> [2] sample command line:
> >>
> >> $QEMU -machine virt,accel=kvm -cpu host \
> >>  -device virtio-serial-device \
> >>  -device virtconsole,chardev=ctd -chardev testdev,id=ctd \
> >>  -display none -serial stdio \
> >>  -kernel arm/gic.flat \
> >>  -smp 8 -machine gic-version=3 -append 'its'
> >>
> >> Eric Auger (15):
> >>   libcflat: Add other size defines
> >>   arm/arm64: gicv3: Add some re-distributor defines
> >>   arm/arm64: ITS skeleton
> >>   arm/arm64: ITS: BASER parsing and setup
> >>   arm/arm64: GICv3: add cpu count
> >>   arm/arm64: ITS: Set the LPI config and pending tables
> >>   arm/arm64: ITS: Init the command queue
> >>   arm/arm64: ITS: enable LPIs at re-distributor level
> >>   arm/arm64: ITS: Parse the typer register
> >>   arm/arm64: ITS: its_enable_defaults
> >>   arm/arm64: ITS: create device
> >>   arm/arm64: ITS: create collection
> >>   arm/arm64: ITS: commands
> >>   arm/arm64: gic: Generalize ipi_enable()
> >>   arm/arm64: ITS test
> >>
> >>  arm/Makefile.common|   1 +
> >>  arm/gic.c  | 101 +++-
> >>  lib/arm/asm/gic-v3-its.h   | 238 +++
> >>  lib/arm/asm/gic-v3.h   |  84 ++
> >>  lib/arm/asm/gic.h  |   1 +
> >>  lib/arm/gic-v3-its-cmd.c   | 399 
> >> +
> >>  lib/arm/gic-v3-its.c   | 305 ++
> >>  lib/arm/gic-v3.c   |   2 +
> >>  lib/arm/gic.c  |  30 +++-
> >>  lib/arm64/asm/gic-v3-its.h |   1 +
> >>  lib/libcflat.h |   3 +
> >>  11 files changed, 1154 insertions(+), 11 deletions(-)
> >>  create mode 100644 lib/arm/asm/gic-v3-its.h
> >>  create mode 100644 lib/arm/gic-v3-its-cmd.c
> >>  create mode 100644 lib/arm/gic-v3-its.c
> >>  create mode 100644 lib/arm64/asm/gic-v3-its.h
> >>
> >> -- 
> >> 2.5.5
> >>
> >>
> > 
> > Thanks for this Eric! I'm glad to see we're getting more GIC test
> > coverage written, even before v8 of the gic series is posted :-)
> > v8 will be rebased on some sysreg stuff Wei is doing for the PMU
> > series,
> 
> Are you planning on a v8 post any time soon?

I think Wei is going to post PMU today. Hopefully everyone will be happy
with the sysreg bits. If so, then I'll post v8 no later than tomorrow
sometime.

> 
> > that's why it's held up. I'll need to set plenty of time
> > aside to learn enough in order to review all the 'ITS:' patches
> > in this series.
> 
> Are you sure you want to really taint yourself with this stuff? You
> wouldn't be the first who risks his mental health by understanding the
> ITS ;-)
> 
> That being said, I will take a look, I am in ITS land anyway for Xen ...

Excellent! Thanks!

drew

> 
> Cheers,
> Andre.
> 
> 
> > Apologies if I can't get to it right away.
> > 
> > Thanks again,
> > drew
> > 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2] arm/arm64: KVM: VGIC: limit ITARGETSR bits to number of VCPUs

[Marc Zyngier]

On 16/11/16 17:57, Andre Przywara wrote:
> The GICv2 spec says in section 4.3.12 that a "CPU targets field bit that
> corresponds to an unimplemented CPU interface is RAZ/WI."
> Currently we allow the guest to write any value in there and it can
> read that back.
> Mask the written value with the proper CPU mask to be spec compliant.
> 
> Signed-off-by: Andre Przywara 
> ---
> Changes v1 .. v2:
> - use GENMASK() instead of open-coding mask
> - drop explicit 0xff masking, since cpu_mask is stronger anyway
> 
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b44b359..78e34bc 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -129,6 +129,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>  unsigned long val)
>  {
>   u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
> + u8 cpu_mask = GENMASK(atomic_read(>kvm->online_vcpus) - 1, 0);
>   int i;
>  
>   /* GICD_ITARGETSR[0-7] are read-only */
> @@ -141,7 +142,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
>  
>   spin_lock(>irq_lock);
>  
> - irq->targets = (val >> (i * 8)) & 0xff;
> + irq->targets = (val >> (i * 8)) & cpu_mask;
>   target = irq->targets ? __ffs(irq->targets) : 0;
>   irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
>  
> 

Applied, 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: [PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly

[Marc Zyngier]

On 01/12/16 19:32, Jintack Lim wrote:
> Current KVM world switch code is unintentionally setting wrong bits to
> CNTHCTL_EL2 when E2H == 1, which may allow guest OS to access physical
> timer.  Bit positions of CNTHCTL_EL2 are changing depending on
> HCR_EL2.E2H bit.  EL1PCEN and EL1PCTEN are 1st and 0th bits when E2H is
> not set, but they are 11th and 10th bits respectively when E2H is set.
> 
> In fact, on VHE we only need to set those bits once, not for every world
> switch. This is because the host kernel runs in EL2 with HCR_EL2.TGE ==
> 1, which makes those bits have no effect for the host kernel execution.
> So we just set those bits once for guests, and that's it.
> 
> Signed-off-by: Jintack Lim 
> ---
> v2->v3: 
> - Perform the initialization including CPU hotplug case.
> - Move has_vhe() to asm/virt.h
> 
> v1->v2: 
> - Skip configuring cnthctl_el2 in world switch path on VHE system.
> - Write patch based on linux-next.
> ---
>  arch/arm/include/asm/virt.h   |  5 +
>  arch/arm/kvm/arm.c|  3 +++
>  arch/arm64/include/asm/virt.h | 10 ++
>  include/kvm/arm_arch_timer.h  |  1 +
>  virt/kvm/arm/arch_timer.c | 23 +++
>  virt/kvm/arm/hyp/timer-sr.c   | 33 +
>  6 files changed, 63 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/include/asm/virt.h b/arch/arm/include/asm/virt.h
> index a2e75b8..6dae195 100644
> --- a/arch/arm/include/asm/virt.h
> +++ b/arch/arm/include/asm/virt.h
> @@ -80,6 +80,11 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return false;
>  }
>  
> +static inline bool has_vhe(void)
> +{
> + return false;
> +}
> +
>  /* The section containing the hypervisor idmap text */
>  extern char __hyp_idmap_text_start[];
>  extern char __hyp_idmap_text_end[];
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 8f92efa..13e54e8 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -1099,6 +1099,9 @@ static void cpu_init_hyp_mode(void *dummy)
>   __cpu_init_hyp_mode(pgd_ptr, hyp_stack_ptr, vector_ptr);
>   __cpu_init_stage2();
>  
> + if (is_kernel_in_hyp_mode())
> + kvm_timer_init_vhe();
> +
>   kvm_arm_init_debug();
>  }
>  
> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h
> index fea1073..b043cfd 100644
> --- a/arch/arm64/include/asm/virt.h
> +++ b/arch/arm64/include/asm/virt.h
> @@ -47,6 +47,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * __boot_cpu_mode records what mode CPUs were booted in.
> @@ -80,6 +81,15 @@ static inline bool is_kernel_in_hyp_mode(void)
>   return read_sysreg(CurrentEL) == CurrentEL_EL2;
>  }
>  
> +static inline bool has_vhe(void)
> +{
> +#ifdef CONFIG_ARM64_VHE

Is there a particular reason why this should be guarded by a #ifdef? All
the symbols should always be available, and since this is a static key,
the overhead should be really insignificant (provided that you use a
non-prehistoric compiler...).

> + if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
> + return true;
> +#endif
> + return false;
> +}
> +
>  #ifdef CONFIG_ARM64_VHE
>  extern void verify_cpu_run_el(void);
>  #else
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index dda39d8..2d54903 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -76,4 +76,5 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  
>  void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
>  
> +void kvm_timer_init_vhe(void);
>  #endif
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 17b8fa5..c7c3bfd 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -24,6 +24,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -507,3 +508,25 @@ void kvm_timer_init(struct kvm *kvm)
>  {
>   kvm->arch.timer.cntvoff = kvm_phys_timer_read();
>  }
> +
> +/*
> + * On VHE system, we only need to configure trap on physical timer and 
> counter
> + * accesses in EL0 and EL1 once, not for every world switch.
> + * The host kernel runs at EL2 with HCR_EL2.TGE == 1,
> + * and this makes those bits have no effect for the host kernel execution.
> + */
> +void kvm_timer_init_vhe(void)
> +{
> + /* When HCR_EL2.E2H ==1, EL1PCEN and EL1PCTEN are shifted by 10 */
> + u32 cnthctl_shift = 10;
> + u64 val;
> +
> + /*
> +  * Disallow physical timer access for the guest.
> +  * Physical counter access is allowed.
> +  */
> + val = read_sysreg(cnthctl_el2);
> + val &= ~(CNTHCTL_EL1PCEN << cnthctl_shift);
> + val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
> + write_sysreg(val, cnthctl_el2);
> +}
> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
> index 798866a..63e28dd 100644
> --- a/virt/kvm/arm/hyp/timer-sr.c
> +++ b/virt/kvm/arm/hyp/timer-sr.c
> @@ -35,10 +35,16 @@ void __hyp_text __timer_save_state(struct 

Re: [Qemu-devel] [kvm-unit-tests RFC 00/15] arm/arm64: add ITS framework

[Andre Przywara]

Hi,

On 06/12/16 09:48, Andrew Jones wrote:
> On Mon, Dec 05, 2016 at 10:46:31PM +0100, Eric Auger wrote:
>> This series proposes a framework to test the virtual ITS.
>> This is based on Drew's v7 series [1]. The last patch tests
>> several ITS commands (collection/device mapping, interrupt
>> translation service entry creation and LPI trigger through INT
>> command). At this point we don't use any external PCIe device
>> to write into the GITS_TRANSLATER register.
>>
>> The bulk of the code derives from the ITS driver code so all
>> the credit is due to Marc.
>>
>> Many other ITS commands could be tested. Also existing MMIO
>> accesses could be enhanced into standalone tests. Current focus
>> was to make it functional.
>>
>> The code deserves more cleanup with respect to cacheability
>> attributes in general.
>>
>> Tested on Cavium ThunderX [2].
>>
>> Best Regards
>>
>> Eric
>>
>> [1] [kvm-unit-tests PATCH v7 00/11] arm/arm64: add gic framework
>>
>> [2] sample command line:
>>
>> $QEMU -machine virt,accel=kvm -cpu host \
>>  -device virtio-serial-device \
>>  -device virtconsole,chardev=ctd -chardev testdev,id=ctd \
>>  -display none -serial stdio \
>>  -kernel arm/gic.flat \
>>  -smp 8 -machine gic-version=3 -append 'its'
>>
>> Eric Auger (15):
>>   libcflat: Add other size defines
>>   arm/arm64: gicv3: Add some re-distributor defines
>>   arm/arm64: ITS skeleton
>>   arm/arm64: ITS: BASER parsing and setup
>>   arm/arm64: GICv3: add cpu count
>>   arm/arm64: ITS: Set the LPI config and pending tables
>>   arm/arm64: ITS: Init the command queue
>>   arm/arm64: ITS: enable LPIs at re-distributor level
>>   arm/arm64: ITS: Parse the typer register
>>   arm/arm64: ITS: its_enable_defaults
>>   arm/arm64: ITS: create device
>>   arm/arm64: ITS: create collection
>>   arm/arm64: ITS: commands
>>   arm/arm64: gic: Generalize ipi_enable()
>>   arm/arm64: ITS test
>>
>>  arm/Makefile.common|   1 +
>>  arm/gic.c  | 101 +++-
>>  lib/arm/asm/gic-v3-its.h   | 238 +++
>>  lib/arm/asm/gic-v3.h   |  84 ++
>>  lib/arm/asm/gic.h  |   1 +
>>  lib/arm/gic-v3-its-cmd.c   | 399 
>> +
>>  lib/arm/gic-v3-its.c   | 305 ++
>>  lib/arm/gic-v3.c   |   2 +
>>  lib/arm/gic.c  |  30 +++-
>>  lib/arm64/asm/gic-v3-its.h |   1 +
>>  lib/libcflat.h |   3 +
>>  11 files changed, 1154 insertions(+), 11 deletions(-)
>>  create mode 100644 lib/arm/asm/gic-v3-its.h
>>  create mode 100644 lib/arm/gic-v3-its-cmd.c
>>  create mode 100644 lib/arm/gic-v3-its.c
>>  create mode 100644 lib/arm64/asm/gic-v3-its.h
>>
>> -- 
>> 2.5.5
>>
>>
> 
> Thanks for this Eric! I'm glad to see we're getting more GIC test
> coverage written, even before v8 of the gic series is posted :-)
> v8 will be rebased on some sysreg stuff Wei is doing for the PMU
> series,

Are you planning on a v8 post any time soon?

> that's why it's held up. I'll need to set plenty of time
> aside to learn enough in order to review all the 'ITS:' patches
> in this series.

Are you sure you want to really taint yourself with this stuff? You
wouldn't be the first who risks his mental health by understanding the
ITS ;-)

That being said, I will take a look, I am in ITS land anyway for Xen ...

Cheers,
Andre.


> Apologies if I can't get to it right away.
> 
> Thanks again,
> drew
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests RFC 05/15] arm/arm64: GICv3: add cpu count

[Auger Eric]

Hi Andre, Drew,

On 06/12/2016 10:32, Andre Przywara wrote:
> Hi,
> 
> On 06/12/16 09:29, Andrew Jones wrote:
>> On Mon, Dec 05, 2016 at 10:46:36PM +0100, Eric Auger wrote:
>>> Add a new cpu_count field in gicv3_data indicating the
>>> number of redistributors. This will be useful for enumeration
>>> of their resources such as LPI pending tables.
>>
>> I'm fine with the additional state, but just curious, will it
>> ever be possible for gicv3.cpu_count != nr_cpus?
> 
> If not you are in trouble, so that should in fact be one test.
> 
> Which brings me to my comment ...
> 
>>>
>>> Signed-off-by: Eric Auger 
>>> ---
>>>  lib/arm/asm/gic-v3.h | 1 +
>>>  lib/arm/gic-v3.c | 2 ++
>>>  2 files changed, 3 insertions(+)
>>>
>>> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
>>> index ed330af..039b7c2 100644
>>> --- a/lib/arm/asm/gic-v3.h
>>> +++ b/lib/arm/asm/gic-v3.h
>>> @@ -58,6 +58,7 @@ struct gicv3_data {
>>> void *dist_base;
>>> void *redist_base[NR_CPUS];
>>> unsigned int irq_nr;
>>> +   unsigned int cpu_count;
> 
> Should that be called "nr_redists" or the like?
> Since this is what it counts in the code below.
> Later we can then compare this with nr_cpus to check for a match.

I fully agree with you suggestion.

Thanks

Eric
> 
> Cheers,
> Andre.
> 
>>>  };
>>>  extern struct gicv3_data gicv3_data;
>>>  
>>> diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
>>> index 6246221..9921f4d 100644
>>> --- a/lib/arm/gic-v3.c
>>> +++ b/lib/arm/gic-v3.c
>>> @@ -12,12 +12,14 @@ void gicv3_set_redist_base(size_t stride)
>>> void *ptr = gicv3_data.redist_base[0];
>>> u64 typer;
>>>  
>>> +   gicv3_data.cpu_count = 0;
>>> do {
>>> typer = gicv3_read_typer(ptr + GICR_TYPER);
>>> if ((typer >> 32) == aff) {
>>> gicv3_redist_base() = ptr;
>>> return;
>>> }
>>> +   gicv3_data.cpu_count++;
>>> ptr += stride; /* skip RD_base, SGI_base, etc. */
>>> } while (!(typer & GICR_TYPER_LAST));
>>>  
>>> -- 
>>> 2.5.5
>>>
>>>
> --
> 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: [Qemu-devel] [kvm-unit-tests RFC 00/15] arm/arm64: add ITS framework

[Andrew Jones]

On Mon, Dec 05, 2016 at 10:46:31PM +0100, Eric Auger wrote:
> This series proposes a framework to test the virtual ITS.
> This is based on Drew's v7 series [1]. The last patch tests
> several ITS commands (collection/device mapping, interrupt
> translation service entry creation and LPI trigger through INT
> command). At this point we don't use any external PCIe device
> to write into the GITS_TRANSLATER register.
> 
> The bulk of the code derives from the ITS driver code so all
> the credit is due to Marc.
> 
> Many other ITS commands could be tested. Also existing MMIO
> accesses could be enhanced into standalone tests. Current focus
> was to make it functional.
> 
> The code deserves more cleanup with respect to cacheability
> attributes in general.
> 
> Tested on Cavium ThunderX [2].
> 
> Best Regards
> 
> Eric
> 
> [1] [kvm-unit-tests PATCH v7 00/11] arm/arm64: add gic framework
> 
> [2] sample command line:
> 
> $QEMU -machine virt,accel=kvm -cpu host \
>  -device virtio-serial-device \
>  -device virtconsole,chardev=ctd -chardev testdev,id=ctd \
>  -display none -serial stdio \
>  -kernel arm/gic.flat \
>  -smp 8 -machine gic-version=3 -append 'its'
> 
> Eric Auger (15):
>   libcflat: Add other size defines
>   arm/arm64: gicv3: Add some re-distributor defines
>   arm/arm64: ITS skeleton
>   arm/arm64: ITS: BASER parsing and setup
>   arm/arm64: GICv3: add cpu count
>   arm/arm64: ITS: Set the LPI config and pending tables
>   arm/arm64: ITS: Init the command queue
>   arm/arm64: ITS: enable LPIs at re-distributor level
>   arm/arm64: ITS: Parse the typer register
>   arm/arm64: ITS: its_enable_defaults
>   arm/arm64: ITS: create device
>   arm/arm64: ITS: create collection
>   arm/arm64: ITS: commands
>   arm/arm64: gic: Generalize ipi_enable()
>   arm/arm64: ITS test
> 
>  arm/Makefile.common|   1 +
>  arm/gic.c  | 101 +++-
>  lib/arm/asm/gic-v3-its.h   | 238 +++
>  lib/arm/asm/gic-v3.h   |  84 ++
>  lib/arm/asm/gic.h  |   1 +
>  lib/arm/gic-v3-its-cmd.c   | 399 
> +
>  lib/arm/gic-v3-its.c   | 305 ++
>  lib/arm/gic-v3.c   |   2 +
>  lib/arm/gic.c  |  30 +++-
>  lib/arm64/asm/gic-v3-its.h |   1 +
>  lib/libcflat.h |   3 +
>  11 files changed, 1154 insertions(+), 11 deletions(-)
>  create mode 100644 lib/arm/asm/gic-v3-its.h
>  create mode 100644 lib/arm/gic-v3-its-cmd.c
>  create mode 100644 lib/arm/gic-v3-its.c
>  create mode 100644 lib/arm64/asm/gic-v3-its.h
> 
> -- 
> 2.5.5
> 
>

Thanks for this Eric! I'm glad to see we're getting more GIC test
coverage written, even before v8 of the gic series is posted :-)
v8 will be rebased on some sysreg stuff Wei is doing for the PMU
series, that's why it's held up. I'll need to set plenty of time
aside to learn enough in order to review all the 'ITS:' patches
in this series. Apologies if I can't get to it right away.

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

Re: [Qemu-devel] [kvm-unit-tests RFC 05/15] arm/arm64: GICv3: add cpu count

[Andre Przywara]

Hi,

On 06/12/16 09:29, Andrew Jones wrote:
> On Mon, Dec 05, 2016 at 10:46:36PM +0100, Eric Auger wrote:
>> Add a new cpu_count field in gicv3_data indicating the
>> number of redistributors. This will be useful for enumeration
>> of their resources such as LPI pending tables.
> 
> I'm fine with the additional state, but just curious, will it
> ever be possible for gicv3.cpu_count != nr_cpus?

If not you are in trouble, so that should in fact be one test.

Which brings me to my comment ...

>>
>> Signed-off-by: Eric Auger 
>> ---
>>  lib/arm/asm/gic-v3.h | 1 +
>>  lib/arm/gic-v3.c | 2 ++
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
>> index ed330af..039b7c2 100644
>> --- a/lib/arm/asm/gic-v3.h
>> +++ b/lib/arm/asm/gic-v3.h
>> @@ -58,6 +58,7 @@ struct gicv3_data {
>>  void *dist_base;
>>  void *redist_base[NR_CPUS];
>>  unsigned int irq_nr;
>> +unsigned int cpu_count;

Should that be called "nr_redists" or the like?
Since this is what it counts in the code below.
Later we can then compare this with nr_cpus to check for a match.

Cheers,
Andre.

>>  };
>>  extern struct gicv3_data gicv3_data;
>>  
>> diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
>> index 6246221..9921f4d 100644
>> --- a/lib/arm/gic-v3.c
>> +++ b/lib/arm/gic-v3.c
>> @@ -12,12 +12,14 @@ void gicv3_set_redist_base(size_t stride)
>>  void *ptr = gicv3_data.redist_base[0];
>>  u64 typer;
>>  
>> +gicv3_data.cpu_count = 0;
>>  do {
>>  typer = gicv3_read_typer(ptr + GICR_TYPER);
>>  if ((typer >> 32) == aff) {
>>  gicv3_redist_base() = ptr;
>>  return;
>>  }
>> +gicv3_data.cpu_count++;
>>  ptr += stride; /* skip RD_base, SGI_base, etc. */
>>  } while (!(typer & GICR_TYPER_LAST));
>>  
>> -- 
>> 2.5.5
>>
>>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests RFC 05/15] arm/arm64: GICv3: add cpu count

[Andrew Jones]

On Mon, Dec 05, 2016 at 10:46:36PM +0100, Eric Auger wrote:
> Add a new cpu_count field in gicv3_data indicating the
> number of redistributors. This will be useful for enumeration
> of their resources such as LPI pending tables.

I'm fine with the additional state, but just curious, will it
ever be possible for gicv3.cpu_count != nr_cpus?

> 
> Signed-off-by: Eric Auger 
> ---
>  lib/arm/asm/gic-v3.h | 1 +
>  lib/arm/gic-v3.c | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index ed330af..039b7c2 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -58,6 +58,7 @@ struct gicv3_data {
>   void *dist_base;
>   void *redist_base[NR_CPUS];
>   unsigned int irq_nr;
> + unsigned int cpu_count;
>  };
>  extern struct gicv3_data gicv3_data;
>  
> diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> index 6246221..9921f4d 100644
> --- a/lib/arm/gic-v3.c
> +++ b/lib/arm/gic-v3.c
> @@ -12,12 +12,14 @@ void gicv3_set_redist_base(size_t stride)
>   void *ptr = gicv3_data.redist_base[0];
>   u64 typer;
>  
> + gicv3_data.cpu_count = 0;
>   do {
>   typer = gicv3_read_typer(ptr + GICR_TYPER);
>   if ((typer >> 32) == aff) {
>   gicv3_redist_base() = ptr;
>   return;
>   }
> + gicv3_data.cpu_count++;
>   ptr += stride; /* skip RD_base, SGI_base, etc. */
>   } while (!(typer & GICR_TYPER_LAST));
>  
> -- 
> 2.5.5
> 
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [Qemu-devel] [kvm-unit-tests RFC 03/15] arm/arm64: ITS skeleton

[Andrew Jones]

On Mon, Dec 05, 2016 at 10:46:34PM +0100, Eric Auger wrote:
> At the moment we just detect the presence of ITS as part of the
> GICv3 init routine and initialize its base address.
> 
> Signed-off-by: Eric Auger 
> ---
>  arm/Makefile.common|  1 +
>  lib/arm/asm/gic-v3-its.h   | 22 ++
>  lib/arm/asm/gic.h  |  1 +
>  lib/arm/gic-v3-its.c   |  9 +
>  lib/arm/gic.c  | 30 +-
>  lib/arm64/asm/gic-v3-its.h |  1 +
>  6 files changed, 59 insertions(+), 5 deletions(-)
>  create mode 100644 lib/arm/asm/gic-v3-its.h
>  create mode 100644 lib/arm/gic-v3-its.c
>  create mode 100644 lib/arm64/asm/gic-v3-its.h
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index 6c0898f..070f349 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -47,6 +47,7 @@ cflatobjs += lib/arm/bitops.o
>  cflatobjs += lib/arm/psci.o
>  cflatobjs += lib/arm/smp.o
>  cflatobjs += lib/arm/gic.o lib/arm/gic-v2.o lib/arm/gic-v3.o
> +cflatobjs += lib/arm/gic-v3-its.o
>  
>  libeabi = lib/arm/libeabi.a
>  eabiobjs = lib/arm/eabi_compat.o
> diff --git a/lib/arm/asm/gic-v3-its.h b/lib/arm/asm/gic-v3-its.h
> new file mode 100644
> index 000..2044565
> --- /dev/null
> +++ b/lib/arm/asm/gic-v3-its.h
> @@ -0,0 +1,22 @@
> +/*
> + * All ITS* defines are lifted from include/linux/irqchip/arm-gic-v3.h
> + *
> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones 

s/Andrew/Eric/

> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#ifndef _ASMARM_GIC_V3_ITS_H_
> +#define _ASMARM_GIC_V3_ITS_H_
> +
> +#ifndef __ASSEMBLY__
> +
> +struct its_data {
> + void *base;
> +};
> +
> +extern struct its_data its_data;
> +
> +#define gicv3_its_base() (its_data.base)

Can't we just add the ITS base address to the current gicv3_data struct?

> +
> +#endif /* !__ASSEMBLY__ */
> +#endif /* _ASMARM_GIC_V3_ITS_H_ */
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index ea5fde9..73d4502 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -30,6 +30,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #ifndef __ASSEMBLY__
>  #include 
> diff --git a/lib/arm/gic-v3-its.c b/lib/arm/gic-v3-its.c
> new file mode 100644
> index 000..e382b80
> --- /dev/null
> +++ b/lib/arm/gic-v3-its.c
> @@ -0,0 +1,9 @@
> +/*
> + * Copyright (C) 2016, Red Hat Inc, Eric Auger 
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include 
> +
> +struct its_data its_data;
> +
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 957a146..e551abd 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -6,6 +6,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct gic_common_ops *gic_common_ops;
>  
> @@ -17,12 +18,14 @@ struct gicv3_data gicv3_data;
>   * Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>   */
>  static bool
> -gic_get_dt_bases(const char *compatible, void **base1, void **base2)
> +gic_get_dt_bases(const char *compatible, void **base1, void **base2,
> +  void **base3)
>  {
>   struct dt_pbus_reg reg;
> - struct dt_device gic;
> + struct dt_device gic, its;
>   struct dt_bus bus;
> - int node, ret;
> + int node, subnode, ret, len;
> + const void *fdt = dt_fdt();
>  
>   dt_bus_init_defaults();
>   dt_device_init(, , NULL);
> @@ -43,19 +46,36 @@ gic_get_dt_bases(const char *compatible, void **base1, 
> void **base2)
>   assert(ret == 0);
>   *base2 = ioremap(reg.addr, reg.size);
>  
> + if (base3 && !strcmp(compatible, "arm,gic-v3")) {
> + dt_for_each_subnode(node, subnode) {
> + const struct fdt_property *prop;
> +
> + prop = fdt_get_property(fdt, subnode,
> + "compatible", );
> + if (!strcmp((char *)prop->data, "arm,gic-v3-its")) {
> + dt_device_bind_node(, subnode);
> + ret = dt_pbus_translate(, 0, );
> + assert(ret == 0);
> + *base3 = ioremap(reg.addr, reg.size);
> + break;
> + }
> + }
> +
> + }
> +
>   return true;
>  }
>  
>  int gicv2_init(void)
>  {
>   return gic_get_dt_bases("arm,cortex-a15-gic",
> - _data.dist_base, _data.cpu_base);
> + _data.dist_base, _data.cpu_base, NULL);
>  }
>  
>  int gicv3_init(void)
>  {
>   return gic_get_dt_bases("arm,gic-v3", _data.dist_base,
> - _data.redist_base[0]);
> + _data.redist_base[0], _data.base);
>  }
>  
>  int gic_init(void)
> diff --git a/lib/arm64/asm/gic-v3-its.h b/lib/arm64/asm/gic-v3-its.h
> new file mode 100644
> index 000..083cba4
> --- /dev/null
> +++