Re: [kvm-unit-tests PATCH v13 4/4] arm: pmu: Add CPI checking
On 01/12/16 21:12, Wei Huang wrote: Hi Wei, > On 12/01/2016 02:27 PM, Andre Przywara wrote: >> Hi, >> >> On 01/12/16 05:16, Wei Huang wrote: >>> From: Christopher Covington>>> >>> Calculate the numbers of cycles per instruction (CPI) implied by ARM >>> PMU cycle counter values. The code includes a strict checking facility >>> intended for the -icount option in TCG mode in the configuration file. >>> >>> Signed-off-by: Christopher Covington >>> Signed-off-by: Wei Huang >>> Reviewed-by: Andrew Jones >>> --- >>> arm/pmu.c | 123 >>> +- >>> arm/unittests.cfg | 14 +++ >>> 2 files changed, 136 insertions(+), 1 deletion(-) >>> >>> diff --git a/arm/pmu.c b/arm/pmu.c >>> index 3566a27..29d7c2c 100644 >>> --- a/arm/pmu.c >>> +++ b/arm/pmu.c >>> @@ -69,6 +69,27 @@ static inline void set_pmccfiltr(uint32_t value) >>> set_pmxevtyper(value); >>> 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__) >>> DEFINE_GET_SYSREG32(pmcr, el0) >>> DEFINE_SET_SYSREG32(pmcr, el0) >>> @@ -77,6 +98,27 @@ DEFINE_GET_SYSREG64(pmccntr, el0); >>> DEFINE_SET_SYSREG64(pmccntr, el0); >>> DEFINE_SET_SYSREG32(pmcntenset, el0); >>> DEFINE_SET_SYSREG32(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 >>> >>> /* >>> @@ -134,6 +176,79 @@ 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, also strictly check that every measurement >>> matches >>> + * it. Strict CPI checking is used to test -icount mode. >>> + */ >>> +static bool check_cpi(int cpi) >>> +{ >>> + uint32_t pmcr = 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"); >> >> Do we really need this line? >> >> In general I find the output quite confusing, actually distracting from >> the other, actual tests. To make it more readable, I tweaked it a bit to >> look like: > >
Re: [kvm-unit-tests PATCH v13 4/4] arm: pmu: Add CPI checking
On 01/12/16 21:18, Christopher Covington wrote: > On 12/01/2016 03:27 PM, Andre Przywara wrote: Hi, >>> + } >>> + avg = sum / NR_SAMPLES; >>> + printf(" sum=%"PRId64" avg=%"PRId64" avg_ipc=%"PRId64" " >>> + "avg_cpi=%"PRId64"\n", sum, avg, i / avg, avg / i); >> >> printf(" avg=%4"PRId64": %3"PRId64" %s\n", >>sum / NR_SAMPLES, i > avg ? i / avg : avg / i, >>i > avg ? "ipc" : "cpi"); >> >> In general I question the usefulness of the cpi/ipc output, it didn't >> seem meaningful in any way to me, neither in KVM or in TCG. >> See the last line (68: ...) in the example above, we shouldn't use an >> average with that deviation for statistical purposes. >> For KVM I get values ranging from 60 to 4383 cpi, which doesn't convey >> any real information to me, in fact the actual cycles look like constant >> to me, probably due to emulation overhead. >> >> So what are we supposed to learn from those numbers? > > I think they were mostly useful in debugging the checking of TCG's > -icount mode, where the numbers are precise. > > I think seeing variable numbers from TCG when -icount is off illustrates > why -icount is useful. But justifying TCG best practices is a non-goal of > kvm-unit-tests. > > I'd like to think is possible to see anomalies in the KVM info which are > due to bugs, but perhaps that's unrealistic or unlikely. > > Feel free to drop the prints, or only print in -icount mode, or only print > when there's error in -icount mode. No, it's totally fine to keep them, especially since they only appear when one runs a test manually. So thanks for the explanation, those numbers _are_ useful, it was just me being clueless and/or ignorant ;-) Cheers, Andre P.S. It looks like we should have some documentation, explaining these numbers, for instance, and the expected results. Along with explanations what all the other tests do, possibly with pointers where to look for in case of failures. And no, I am not volunteering, at least not for the PMU ... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v13 4/4] arm: pmu: Add CPI checking
On 12/01/2016 03:27 PM, Andre Przywara wrote: > Hi, > > On 01/12/16 05:16, Wei Huang wrote: >> From: Christopher Covington>> >> Calculate the numbers of cycles per instruction (CPI) implied by ARM >> PMU cycle counter values. The code includes a strict checking facility >> intended for the -icount option in TCG mode in the configuration file. >> >> Signed-off-by: Christopher Covington >> Signed-off-by: Wei Huang >> Reviewed-by: Andrew Jones >> --- >> arm/pmu.c | 123 >> +- >> arm/unittests.cfg | 14 +++ >> 2 files changed, 136 insertions(+), 1 deletion(-) >> >> diff --git a/arm/pmu.c b/arm/pmu.c >> index 3566a27..29d7c2c 100644 >> --- a/arm/pmu.c >> +++ b/arm/pmu.c >> @@ -69,6 +69,27 @@ static inline void set_pmccfiltr(uint32_t value) >> set_pmxevtyper(value); >> 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__) >> DEFINE_GET_SYSREG32(pmcr, el0) >> DEFINE_SET_SYSREG32(pmcr, el0) >> @@ -77,6 +98,27 @@ DEFINE_GET_SYSREG64(pmccntr, el0); >> DEFINE_SET_SYSREG64(pmccntr, el0); >> DEFINE_SET_SYSREG32(pmcntenset, el0); >> DEFINE_SET_SYSREG32(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 >> >> /* >> @@ -134,6 +176,79 @@ 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, also strictly check that every measurement matches >> + * it. Strict CPI checking is used to test -icount mode. >> + */ >> +static bool check_cpi(int cpi) >> +{ >> +uint32_t pmcr = 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"); > > Do we really need this line? > > In general I find the output quite confusing, actually distracting from > the other, actual tests. To make it more readable, I tweaked it a bit to > look like: > 4: 9996 173 222 122 118 119 120 212 240 233 avg=1155: 288 cpi > 36: 773 282 291 314 291 335 315 264 162 308 avg= 333: 9 cpi > 68:
Re: [kvm-unit-tests PATCH v13 4/4] arm: pmu: Add CPI checking
On 12/01/2016 02:27 PM, Andre Przywara wrote: > Hi, > > On 01/12/16 05:16, Wei Huang wrote: >> From: Christopher Covington>> >> Calculate the numbers of cycles per instruction (CPI) implied by ARM >> PMU cycle counter values. The code includes a strict checking facility >> intended for the -icount option in TCG mode in the configuration file. >> >> Signed-off-by: Christopher Covington >> Signed-off-by: Wei Huang >> Reviewed-by: Andrew Jones >> --- >> arm/pmu.c | 123 >> +- >> arm/unittests.cfg | 14 +++ >> 2 files changed, 136 insertions(+), 1 deletion(-) >> >> diff --git a/arm/pmu.c b/arm/pmu.c >> index 3566a27..29d7c2c 100644 >> --- a/arm/pmu.c >> +++ b/arm/pmu.c >> @@ -69,6 +69,27 @@ static inline void set_pmccfiltr(uint32_t value) >> set_pmxevtyper(value); >> 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__) >> DEFINE_GET_SYSREG32(pmcr, el0) >> DEFINE_SET_SYSREG32(pmcr, el0) >> @@ -77,6 +98,27 @@ DEFINE_GET_SYSREG64(pmccntr, el0); >> DEFINE_SET_SYSREG64(pmccntr, el0); >> DEFINE_SET_SYSREG32(pmcntenset, el0); >> DEFINE_SET_SYSREG32(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 >> >> /* >> @@ -134,6 +176,79 @@ 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, also strictly check that every measurement matches >> + * it. Strict CPI checking is used to test -icount mode. >> + */ >> +static bool check_cpi(int cpi) >> +{ >> +uint32_t pmcr = 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"); > > Do we really need this line? > > In general I find the output quite confusing, actually distracting from > the other, actual tests. To make it more readable, I tweaked it a bit to > look like: Formatting the output can be useful and it indeed makes the output easier to read. > 4: 9996 173 222 122 118 119 120 212 240 233 avg=1155: 288
Re: [kvm-unit-tests PATCH v13 4/4] arm: pmu: Add CPI checking
Hi, On 01/12/16 05:16, Wei Huang wrote: > From: Christopher Covington> > Calculate the numbers of cycles per instruction (CPI) implied by ARM > PMU cycle counter values. The code includes a strict checking facility > intended for the -icount option in TCG mode in the configuration file. > > Signed-off-by: Christopher Covington > Signed-off-by: Wei Huang > Reviewed-by: Andrew Jones > --- > arm/pmu.c | 123 > +- > arm/unittests.cfg | 14 +++ > 2 files changed, 136 insertions(+), 1 deletion(-) > > diff --git a/arm/pmu.c b/arm/pmu.c > index 3566a27..29d7c2c 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -69,6 +69,27 @@ static inline void set_pmccfiltr(uint32_t value) > set_pmxevtyper(value); > 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__) > DEFINE_GET_SYSREG32(pmcr, el0) > DEFINE_SET_SYSREG32(pmcr, el0) > @@ -77,6 +98,27 @@ DEFINE_GET_SYSREG64(pmccntr, el0); > DEFINE_SET_SYSREG64(pmccntr, el0); > DEFINE_SET_SYSREG32(pmcntenset, el0); > DEFINE_SET_SYSREG32(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 > > /* > @@ -134,6 +176,79 @@ 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, also strictly check that every measurement matches > + * it. Strict CPI checking is used to test -icount mode. > + */ > +static bool check_cpi(int cpi) > +{ > + uint32_t pmcr = 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"); Do we really need this line? In general I find the output quite confusing, actually distracting from the other, actual tests. To make it more readable, I tweaked it a bit to look like: 4: 9996 173 222 122 118 119 120 212 240 233 avg=1155: 288 cpi 36: 773 282 291 314 291 335 315 264 162 308 avg= 333: 9 cpi 68: 229 356 400 339 203 201 335 233 201 372 avg= 286: 4 cpi with some padding hints and limiting the line to at most 80 characters, by: > + > + for (unsigned int
[PATCH v3] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
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 |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; - write_sysreg(val, cnthctl_el2); + /* +* We don't need to do this for VHE since the host kernel runs in EL2 +* with HCR_EL2.TGE ==1, which makes those bits have no impact. +*/ + if (!has_vhe()) { + /* Allow physical
Re: [PATCH v2] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
Hi Marc, On Thu, Dec 1, 2016 at 8:30 AM, Marc Zyngierwrote: > Hi Jintack, > > On 01/12/16 11:38, 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: Skip configuring cnthctl_el2 in world switch path on VHE system. >> This patch is based on linux-next. >> >> --- >> arch/arm/kvm/arm.c | 1 + >> include/kvm/arm_arch_timer.h | 1 + >> virt/kvm/arm/arch_timer.c| 23 ++ >> virt/kvm/arm/hyp/timer-sr.c | 45 >> >> 4 files changed, 58 insertions(+), 12 deletions(-) >> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index 8f92efa..38c0645 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -1286,6 +1286,7 @@ static void teardown_hyp_mode(void) >> >> static int init_vhe_mode(void) >> { >> + on_each_cpu(kvm_timer_init_vhe, NULL, 1); > > I'm pretty sure this is not going to work with CPU hotplug, as you only > perform this once and for all at boot time. > > I think it would be better if you called this init function as part of > cpu_init_hyp_mode(). All right. I'll do that. > >> kvm_info("VHE mode initialized successfully\n"); >> return 0; >> } >> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h >> index dda39d8..5853399 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 *dummy); >> #endif >> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c >> index 17b8fa5..7a0d85d7 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 *dummy) >> +{ >> + /* 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); >> +} > > If you make this called from cpu_init_hyp_mode(), it will have to be > guarded with is_kernel_in_hyp_mode(). > >> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c >> index 798866a..f7fc957 100644 >> --- a/virt/kvm/arm/hyp/timer-sr.c >> +++ b/virt/kvm/arm/hyp/timer-sr.c >> @@ -21,6 +21,18 @@ >> >> #include >> >> +#ifdef CONFIG_ARM64 >> +static inline bool has_vhe(void) >> +{ >> + if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) >> + return true; >> + >> + return false; >> +} >> +#else >> +#define has_vhe()false > > Could this now be moved to asm/virt.h, and maybe replace the current > implementation of is_kernel_in_hyp_mode? The latter may require some > more hacking around, so I'm not sure this is worth it. I can move that to asm/virt.h, but I'll leave is_kernel_in_hyp_mode() as it is because, as you pointed out in another e-mail, is_kernel_in_hyp_mode() can be used before setting up the CPU capabilities (e.g. runs_at_el2()). I'll send out v3 soon. Thanks, Jintack > >> +#endif >> + >> /* vcpu is already in the HYP VA space */ >> void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu) >> { >> @@ -35,10 +47,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 |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; >> - write_sysreg(val,
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 3/4] arm: pmu: Check cycle count increases
On 12/01/2016 03:18 AM, Andrew Jones wrote: > On Wed, Nov 30, 2016 at 11:16:41PM -0600, Wei Huang wrote: >> From: Christopher Covington>> >> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing, >> even for the smallest delta of two subsequent reads. >> >> Signed-off-by: Christopher Covington >> Signed-off-by: Wei Huang >> Reviewed-by: Andrew Jones >> --- >> arm/pmu.c | 94 >> +++ >> 1 file changed, 94 insertions(+) >> >> diff --git a/arm/pmu.c b/arm/pmu.c >> index 1fe2b1a..3566a27 100644 >> --- a/arm/pmu.c >> +++ b/arm/pmu.c >> @@ -16,6 +16,9 @@ >> #include "asm/barrier.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 >> @@ -23,10 +26,57 @@ >> #define PMU_PMCR_IMP_SHIFT 24 >> #define PMU_PMCR_IMP_MASK 0xff >> >> +#define ID_DFR0_PERFMON_SHIFT 24 >> +#define ID_DFR0_PERFMON_MASK 0xf >> + >> +#define PMU_CYCLE_IDX 31 >> + >> +#define NR_SAMPLES 10 >> + >> +static unsigned int pmu_version; >> #if defined(__arm__) >> DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0) >> +DEFINE_SET_SYSREG32(pmcr, 0, c9, c12, 0) >> +DEFINE_GET_SYSREG32(id_dfr0, 0, c0, c1, 2) >> +DEFINE_SET_SYSREG32(pmselr, 0, c9, c12, 5) >> +DEFINE_SET_SYSREG32(pmxevtyper, 0, c9, c13, 1) >> +DEFINE_GET_SYSREG32(pmccntr32, 0, c9, c13, 0) >> +DEFINE_SET_SYSREG32(pmccntr32, 0, c9, c13, 0) >> +DEFINE_GET_SYSREG64(pmccntr64, 0, c9) >> +DEFINE_SET_SYSREG64(pmccntr64, 0, c9) >> +DEFINE_SET_SYSREG32(pmcntenset, 0, c9, c12, 1) > > Seeing how we get lots of redundant looking lines, I think instead > of defining DEFINE_SET/GET_SYSREG32/64, we should instead have > > DEFINE_SYSREG32/64 ... creates both get_ and set_ > DEFINE_SYSREG32/64_RO ... creates just get_ Don't like the naming. I think we can create a new macro, named DEFINE_GET_SET_SYSREG32/64. I know it is boring, but readers should get the idea easily. > >> + >> +static inline uint64_t get_pmccntr(void) >> +{ >> +if (pmu_version == 0x3) >> +return get_pmccntr64(); >> +else >> +return get_pmccntr32(); >> +} >> + >> +static inline void set_pmccntr(uint64_t value) >> +{ >> +if (pmu_version == 0x3) >> +set_pmccntr64(value); >> +else >> +set_pmccntr32(value & 0x); >> +} > > So the two accessors above are exceptional, which is why we don't > use SYSREG for them. These can have uint64_t for there external > interface. We can't require 'unsigned long' or 'unsigned long long' > >> + >> +/* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */ >> +static inline void set_pmccfiltr(uint32_t value) >> +{ >> +set_pmselr(PMU_CYCLE_IDX); >> +set_pmxevtyper(value); >> +isb(); >> +} >> #elif defined(__aarch64__) >> DEFINE_GET_SYSREG32(pmcr, el0) >> +DEFINE_SET_SYSREG32(pmcr, el0) >> +DEFINE_GET_SYSREG32(id_dfr0, el1) >> +DEFINE_GET_SYSREG64(pmccntr, el0); >> +DEFINE_SET_SYSREG64(pmccntr, el0); >> +DEFINE_SET_SYSREG32(pmcntenset, el0); >> +DEFINE_SET_SYSREG32(pmccfiltr, el0); >> #endif >> >> /* >> @@ -52,11 +102,55 @@ static bool check_pmcr(void) >> return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0; >> } >> >> +/* >> + * Ensure that the cycle counter progresses between back-to-back reads. >> + */ >> +static bool check_cycles_increase(void) >> +{ >> +bool success = true; >> + >> +/* 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_pmccntr(0); >> + >> +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; >> +} >> + >> +void pmu_init(void) >> +{ >> +uint32_t dfr0; >> + >> +/* probe pmu version */ >> +dfr0 = get_id_dfr0(); >> +pmu_version = (dfr0 >> ID_DFR0_PERFMON_SHIFT) & ID_DFR0_PERFMON_MASK; >> +report_info("PMU version: %d", pmu_version); >> +} >> + >> int main(void) >> { >> report_prefix_push("pmu"); >> >> +pmu_init(); >> report("Control register", check_pmcr()); >> +report("Monotonically increasing cycle count", check_cycles_increase()); >> >> return report_summary(); >> } >> -- >> 1.8.3.1 >> >> > > drew > ___
Re: [kvm-unit-tests PATCH v13 3/4] arm: pmu: Check cycle count increases
On 12/01/2016 05:27 AM, Andre Przywara wrote: > Hi, > > On 01/12/16 05:16, Wei Huang wrote: >> From: Christopher Covington>> >> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing, >> even for the smallest delta of two subsequent reads. >> >> Signed-off-by: Christopher Covington >> Signed-off-by: Wei Huang >> Reviewed-by: Andrew Jones >> --- >> arm/pmu.c | 94 >> +++ >> 1 file changed, 94 insertions(+) >> >> diff --git a/arm/pmu.c b/arm/pmu.c >> index 1fe2b1a..3566a27 100644 >> --- a/arm/pmu.c >> +++ b/arm/pmu.c >> @@ -16,6 +16,9 @@ >> #include "asm/barrier.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 >> @@ -23,10 +26,57 @@ >> #define PMU_PMCR_IMP_SHIFT 24 >> #define PMU_PMCR_IMP_MASK 0xff >> >> +#define ID_DFR0_PERFMON_SHIFT 24 >> +#define ID_DFR0_PERFMON_MASK 0xf >> + >> +#define PMU_CYCLE_IDX 31 >> + >> +#define NR_SAMPLES 10 >> + >> +static unsigned int pmu_version; >> #if defined(__arm__) >> DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0) >> +DEFINE_SET_SYSREG32(pmcr, 0, c9, c12, 0) >> +DEFINE_GET_SYSREG32(id_dfr0, 0, c0, c1, 2) >> +DEFINE_SET_SYSREG32(pmselr, 0, c9, c12, 5) >> +DEFINE_SET_SYSREG32(pmxevtyper, 0, c9, c13, 1) >> +DEFINE_GET_SYSREG32(pmccntr32, 0, c9, c13, 0) >> +DEFINE_SET_SYSREG32(pmccntr32, 0, c9, c13, 0) >> +DEFINE_GET_SYSREG64(pmccntr64, 0, c9) >> +DEFINE_SET_SYSREG64(pmccntr64, 0, c9) >> +DEFINE_SET_SYSREG32(pmcntenset, 0, c9, c12, 1) >> + >> +static inline uint64_t get_pmccntr(void) >> +{ >> +if (pmu_version == 0x3) >> +return get_pmccntr64(); >> +else >> +return get_pmccntr32(); >> +} >> + >> +static inline void set_pmccntr(uint64_t value) >> +{ >> +if (pmu_version == 0x3) >> +set_pmccntr64(value); >> +else >> +set_pmccntr32(value & 0x); >> +} >> + >> +/* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */ >> +static inline void set_pmccfiltr(uint32_t value) >> +{ >> +set_pmselr(PMU_CYCLE_IDX); >> +set_pmxevtyper(value); >> +isb(); >> +} >> #elif defined(__aarch64__) >> DEFINE_GET_SYSREG32(pmcr, el0) >> +DEFINE_SET_SYSREG32(pmcr, el0) >> +DEFINE_GET_SYSREG32(id_dfr0, el1) >> +DEFINE_GET_SYSREG64(pmccntr, el0); >> +DEFINE_SET_SYSREG64(pmccntr, el0); >> +DEFINE_SET_SYSREG32(pmcntenset, el0); >> +DEFINE_SET_SYSREG32(pmccfiltr, el0); >> #endif >> >> /* >> @@ -52,11 +102,55 @@ static bool check_pmcr(void) >> return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0; >> } >> >> +/* >> + * Ensure that the cycle counter progresses between back-to-back reads. >> + */ >> +static bool check_cycles_increase(void) >> +{ >> +bool success = true; >> + >> +/* 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_pmccntr(0); > > Why do we need this? Shouldn't PMU_PMCR_C below take care of that? PMU_PMCR_C does reset cycle counter, I can remove this one. > >> + >> +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; >> +} >> + >> +void pmu_init(void) > > Mmh, this function doesn't really initialize anything, does it? > Should it be named pmu_available() or pmu_version() or the like? > This function used to contain cycle counter configuration code. It sets up PMCCNFILTR, PMCNTENSET, etc. Since then, the configuration code has been moved to sub-unit tests. We can change its name to something like pmu_probe(). > And should we bail out early here (or rather at the caller) if this > register reports that no PMU is available? For instance by making it > return a boolean? This could do. > >> +{ >> +uint32_t dfr0; >> + >> +/* probe pmu version */ >> +dfr0 = get_id_dfr0(); >> +pmu_version = (dfr0 >> ID_DFR0_PERFMON_SHIFT) & ID_DFR0_PERFMON_MASK; >> +report_info("PMU version: %d", pmu_version); >> +} >> + >> int main(void) >> { >> report_prefix_push("pmu"); >> >> +pmu_init(); >> report("Control register", check_pmcr()); >> +report("Monotonically increasing cycle count", check_cycles_increase()); > > I wonder if we should skip this test if check_pmcr() has returned
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 1/4] arm: Define macros for accessing system registers
On Thu, Dec 01, 2016 at 09:27:59AM -0600, Wei Huang wrote: > > > On 12/01/2016 02:59 AM, Andrew Jones wrote: > > > > Should this be From: Andre? > > > > On Wed, Nov 30, 2016 at 11:16:39PM -0600, Wei Huang wrote: > >> This patch defines four macros to assist creating system register > >> accessors under both ARMv7 and AArch64: > >>* DEFINE_GET_SYSREG32(name, ...) > >>* DEFINE_SET_SYSREG32(name, ...) > >>* DEFINE_GET_SYSREG64(name, ...) > >>* DEFINE_SET_SYSREG64(name, ...) > >> These macros are translated to inline functions with consistent naming, > >> get_##name() and set_##name(), which can be used by C code directly. > >> > >> Signed-off-by: Andre Przywara> >> Signed-off-by: Wei Huang > >> --- > >> lib/arm/asm/processor.h | 37 - > >> lib/arm64/asm/processor.h | 35 --- > >> 2 files changed, 60 insertions(+), 12 deletions(-) > >> > >> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h > >> index f25e7ee..3ca6b42 100644 > >> --- a/lib/arm/asm/processor.h > >> +++ b/lib/arm/asm/processor.h > >> @@ -33,13 +33,40 @@ static inline unsigned long current_cpsr(void) > >> > >> #define current_mode() (current_cpsr() & MODE_MASK) > >> > >> -static inline unsigned int get_mpidr(void) > >> -{ > >> - unsigned int mpidr; > >> - asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r" (mpidr)); > >> - return mpidr; > >> +#define DEFINE_GET_SYSREG32(name, opc1, crn, crm, opc2) > >> \ > >> +static inline uint32_t get_##name(void) > >> \ > >> +{ \ > >> + uint32_t reg; \ > >> + asm volatile("mrc p15, " #opc1 ", %0, " #crn ", " #crm ", " \ > >> + #opc2 : "=r" (reg)); \ > >> + return reg; \ > >> +} > >> + > >> +#define DEFINE_SET_SYSREG32(name, opc1, crn, crm, opc2) > >> \ > >> +static inline void set_##name(uint32_t value) > >> \ > >> +{ \ > >> + asm volatile("mcr p15, " #opc1 ", %0, " #crn ", " #crm ", " \ > >> + #opc2 :: "r" (value)); \ > >^ nit: no space here, checkpatch would complain > > Which checkpatch script you are using? I didn't find one in > kvm-unit-tests. I tried kernel's checkpatch script, but it didn't > complain anything against this patch. I use the kernel's, and it, at least used to, complains about no spaces between the ':' in asms. If it doesn't complain now, then it doesn't matter. Actually, it didn't really matter before :-) Thanks, drew > > >> +} > >> + > > > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 1/4] arm: Define macros for accessing system registers
On 12/01/2016 02:59 AM, Andrew Jones wrote: > > Should this be From: Andre? > > On Wed, Nov 30, 2016 at 11:16:39PM -0600, Wei Huang wrote: >> This patch defines four macros to assist creating system register >> accessors under both ARMv7 and AArch64: >>* DEFINE_GET_SYSREG32(name, ...) >>* DEFINE_SET_SYSREG32(name, ...) >>* DEFINE_GET_SYSREG64(name, ...) >>* DEFINE_SET_SYSREG64(name, ...) >> These macros are translated to inline functions with consistent naming, >> get_##name() and set_##name(), which can be used by C code directly. >> >> Signed-off-by: Andre Przywara>> Signed-off-by: Wei Huang >> --- >> lib/arm/asm/processor.h | 37 - >> lib/arm64/asm/processor.h | 35 --- >> 2 files changed, 60 insertions(+), 12 deletions(-) >> >> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h >> index f25e7ee..3ca6b42 100644 >> --- a/lib/arm/asm/processor.h >> +++ b/lib/arm/asm/processor.h >> @@ -33,13 +33,40 @@ static inline unsigned long current_cpsr(void) >> >> #define current_mode() (current_cpsr() & MODE_MASK) >> >> -static inline unsigned int get_mpidr(void) >> -{ >> -unsigned int mpidr; >> -asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r" (mpidr)); >> -return mpidr; >> +#define DEFINE_GET_SYSREG32(name, opc1, crn, crm, opc2) >> \ >> +static inline uint32_t get_##name(void) >> \ >> +{ \ >> +uint32_t reg; \ >> +asm volatile("mrc p15, " #opc1 ", %0, " #crn ", " #crm ", " \ >> + #opc2 : "=r" (reg)); \ >> +return reg; \ >> +} >> + >> +#define DEFINE_SET_SYSREG32(name, opc1, crn, crm, opc2) >> \ >> +static inline void set_##name(uint32_t value) >> \ >> +{ \ >> +asm volatile("mcr p15, " #opc1 ", %0, " #crn ", " #crm ", " \ >> + #opc2 :: "r" (value)); \ >^ nit: no space here, checkpatch would complain Which checkpatch script you are using? I didn't find one in kvm-unit-tests. I tried kernel's checkpatch script, but it didn't complain anything against this patch. >> +} >> + ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL] KVM/ARM updates for 4.9-rc7
2016-12-01 09:25+, Marc Zyngier: > Paolo, Radim, > > Hopefully, this is the last update for 4.9. This time, a single patch > that prevents bogus acknoledgement of interrupts. > > It'd be great if this could make it into v4.9-final Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 4/4] arm: pmu: Add CPI checking
On Thu, Dec 01, 2016 at 10:19:13AM +, Andre Przywara wrote: > Hi Drew, > > actually unrelated to this actual patch, but since you mentioned it: > > > As we work out how best to handle tcg-only tests in order to get Alex > > Bennee's MTTCG tests merged, we'll probably revisit this file, > > So when I was experimenting with kvmtool, I realized that some tests > would need to be QEMU only. Also I was tempted to try some tests either > on bare metal machines or in a Fast Model. > So I wonder if we should have some constraints or tags on the tests, so > that a certain backend can filter on this and skip if it's not capable? So far we've been using unittests.cfg flags for filtering, but we could also teach configure how to set up the build for only certain tests, i.e. new config options and makefile targets could take use further. The unittests.cfg file could be split into multiple cfg files as well, teaching run_tests.sh how to select the right one. > > Just wanted to mention this so that we can use this refactoring > opportunity to come up with something more generic than just a boolean > TGC vs. KVM. > Maybe we should introduce the notion of a "test backend"? That could be > QEMU/KVM and TCG for now, but later extended to cover kvmtool and > So the $TEST_DIR/run (e.g. arm/run) script is currently qemu focused, and is learning how to deal with both KVM and TCG. We haven't been trying to keep qemu knowledge in that one file, but we could probably do it, i.e. rename it to run-qemu and make sure all common script files are kvm userspace agnostic. I don't think that should be too difficult. We'll likely need more build config options for this though too, as load addresses, etc. may change. > probably other hypervisors like Xen as well. Also doable once we isolate the hypervisor userspace specifics from everything else. Same goes for getting tests to run on bare-metal, launched from the grub prompt or UEFI. But, eventually people will be confused with the project's name *kvm*-unit-tests :-) Thanks, drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
Hi Jintack, On 01/12/16 11:38, 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: Skip configuring cnthctl_el2 in world switch path on VHE system. > This patch is based on linux-next. > > --- > arch/arm/kvm/arm.c | 1 + > include/kvm/arm_arch_timer.h | 1 + > virt/kvm/arm/arch_timer.c| 23 ++ > virt/kvm/arm/hyp/timer-sr.c | 45 > > 4 files changed, 58 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index 8f92efa..38c0645 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -1286,6 +1286,7 @@ static void teardown_hyp_mode(void) > > static int init_vhe_mode(void) > { > + on_each_cpu(kvm_timer_init_vhe, NULL, 1); I'm pretty sure this is not going to work with CPU hotplug, as you only perform this once and for all at boot time. I think it would be better if you called this init function as part of cpu_init_hyp_mode(). > kvm_info("VHE mode initialized successfully\n"); > return 0; > } > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h > index dda39d8..5853399 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 *dummy); > #endif > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index 17b8fa5..7a0d85d7 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 *dummy) > +{ > + /* 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); > +} If you make this called from cpu_init_hyp_mode(), it will have to be guarded with is_kernel_in_hyp_mode(). > diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c > index 798866a..f7fc957 100644 > --- a/virt/kvm/arm/hyp/timer-sr.c > +++ b/virt/kvm/arm/hyp/timer-sr.c > @@ -21,6 +21,18 @@ > > #include > > +#ifdef CONFIG_ARM64 > +static inline bool has_vhe(void) > +{ > + if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) > + return true; > + > + return false; > +} > +#else > +#define has_vhe()false Could this now be moved to asm/virt.h, and maybe replace the current implementation of is_kernel_in_hyp_mode? The latter may require some more hacking around, so I'm not sure this is worth it. > +#endif > + > /* vcpu is already in the HYP VA space */ > void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu) > { > @@ -35,10 +47,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 |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; > - write_sysreg(val, cnthctl_el2); > + /* > + * We don't need to do this for VHE since the host kernel runs in EL2 > + * with HCR_EL2.TGE ==1, which makes those bits have no impact. > + */ > + if (!has_vhe()) { > + /* Allow physical timer/counter access for the host */ > + val = read_sysreg(cnthctl_el2); > + val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; > + write_sysreg(val, cnthctl_el2); > + } > > /* Clear cntvoff for the host */ >
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 1/4] arm: Define macros for accessing system registers
On Thu, Dec 01, 2016 at 11:11:55AM +, Andre Przywara wrote: > Hi, > > On 01/12/16 08:59, Andrew Jones wrote: > > > > Should this be From: Andre? > > No need from my side, this way all the bug reports are send to Wei ;-) > > > On Wed, Nov 30, 2016 at 11:16:39PM -0600, Wei Huang wrote: > >> This patch defines four macros to assist creating system register > >> accessors under both ARMv7 and AArch64: > >>* DEFINE_GET_SYSREG32(name, ...) > >>* DEFINE_SET_SYSREG32(name, ...) > >>* DEFINE_GET_SYSREG64(name, ...) > >>* DEFINE_SET_SYSREG64(name, ...) > >> These macros are translated to inline functions with consistent naming, > >> get_##name() and set_##name(), which can be used by C code directly. > >> > >> Signed-off-by: Andre Przywara> >> Signed-off-by: Wei Huang > >> --- > >> lib/arm/asm/processor.h | 37 - > >> lib/arm64/asm/processor.h | 35 --- > >> 2 files changed, 60 insertions(+), 12 deletions(-) > >> > >> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h > >> index f25e7ee..3ca6b42 100644 > >> --- a/lib/arm/asm/processor.h > >> +++ b/lib/arm/asm/processor.h > >> @@ -33,13 +33,40 @@ static inline unsigned long current_cpsr(void) > >> > >> #define current_mode() (current_cpsr() & MODE_MASK) > >> > >> -static inline unsigned int get_mpidr(void) > >> -{ > >> - unsigned int mpidr; > >> - asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r" (mpidr)); > >> - return mpidr; > >> +#define DEFINE_GET_SYSREG32(name, opc1, crn, crm, opc2) > >> \ > >> +static inline uint32_t get_##name(void) > >> \ > >> +{ \ > >> + uint32_t reg; \ > >> + asm volatile("mrc p15, " #opc1 ", %0, " #crn ", " #crm ", " \ > >> + #opc2 : "=r" (reg)); \ > >> + return reg; \ > >> +} > >> + > >> +#define DEFINE_SET_SYSREG32(name, opc1, crn, crm, opc2) > >> \ > >> +static inline void set_##name(uint32_t value) > >> \ > >> +{ \ > >> + asm volatile("mcr p15, " #opc1 ", %0, " #crn ", " #crm ", " \ > >> + #opc2 :: "r" (value)); \ > >^ nit: no space here, checkpatch would complain > >> +} > >> + > >> +#define DEFINE_GET_SYSREG64(name, opc, crm) > >> \ > >> +static inline uint64_t get_##name(void) > >> \ > >> +{ \ > >> + uint32_t lo, hi;\ > >> + asm volatile("mrrc p15, " #opc ", %0, %1, " #crm\ > >> + : "=r" (lo), "=r" (hi)); \ > >> + return (uint64_t)hi << 32 | lo; \ > >> +} > >> + > >> +#define DEFINE_SET_SYSREG64(name, opc, crm) > >> \ > >> +static inline void set_##name(uint64_t value) > >> \ > >> +{ \ > >> + asm volatile("mcrr p15, " #opc ", %0, %1, " #crm\ > >> + :: "r" (value & 0x), "r" (value >> 32)); \ > >> } > >> > >> +DEFINE_GET_SYSREG32(mpidr, 0, c0, c0, 5) > >> + > >> /* Only support Aff0 for now, up to 4 cpus */ > >> #define mpidr_to_cpu(mpidr) ((int)((mpidr) & 0xff)) > >> > >> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h > >> index 84d5c7c..dfa75eb 100644 > >> --- a/lib/arm64/asm/processor.h > >> +++ b/lib/arm64/asm/processor.h > >> @@ -66,14 +66,35 @@ 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; \ > >> +#define DEFINE_GET_SYSREG32(reg, el) > >> \ > >> +static inline uint32_t get_##reg(void) > >> \ > >> +{ \ > >> + uint32_t reg; \ > >> + asm volatile("mrs %0, " #reg "_" #el : "=r" (reg)); \ > >> + return reg; \ > >> } > >> -DEFINE_GET_SYSREG32(mpidr) > >> + > >> +#define
Re: [PATCH] KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs
On 01/12/16 12:15, Shameerali Kolothum Thodi wrote: >> For these interrupts, which are purely virtual, there is absolutely >> nothing to do at the physical distributor level anyway. Furthermore, >> kvm_notify_acked_irq doesn't know about per-cpu interrupt, which is why >> we cannot notify them. > > Just to confirm, that means for any phys interrupt(PPI/SPI) to be injected > to the Guest, the mapping bit has(HW bit set) to be used. Yup. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 2/4] arm: Add PMU test
On 1 December 2016 at 12:19, Andre Przywarawrote: > Hi, > > On 01/12/16 12:02, Peter Maydell wrote: >> On 1 December 2016 at 11:28, Andre Przywara wrote: >>> I don't think so. At least here as a _variable_ type uint32_t is >>> probably the right one, as the ARMv8 ARM explicitly says that PMCR is a >>> 32-bit register, for both bitnesses. >> >> For 64-bit ARM this is strictly speaking just shorthand for "64-bit >> register with the top 32-bit being RES0". It is in theory possible that >> a future architecture extension might define uses for those RES0 >> bits. > > I trade: "in theory possible that a future architecture extension might" > (that's four speculative terms, right?) against: > > ARMv8 ARM, D7.4.7 PMCR_EL0, Performance Monitors Control Register: > Attributes > PMCR_EL0 is a 32-bit register. As I say, this just means "64 bit with 32 RES0 bits". See DDI0487A.k C5.1.1 "System register width": # In AArch64 state, each encoding in the System instruction space can # provide access to a 64-bit register. An AArch64 System register is # described as either a 32-bit register or a 64-bit register. For # a 32-bit register, the upper bits, bits[63:32], are RES0. (ie the register is 64-bits, it's just "described as" 32-bits for convenience.) thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 2/4] arm: Add PMU test
Hi, On 01/12/16 12:02, Peter Maydell wrote: > On 1 December 2016 at 11:28, Andre Przywarawrote: >> I don't think so. At least here as a _variable_ type uint32_t is >> probably the right one, as the ARMv8 ARM explicitly says that PMCR is a >> 32-bit register, for both bitnesses. > > For 64-bit ARM this is strictly speaking just shorthand for "64-bit > register with the top 32-bit being RES0". It is in theory possible that > a future architecture extension might define uses for those RES0 > bits. I trade: "in theory possible that a future architecture extension might" (that's four speculative terms, right?) against: ARMv8 ARM, D7.4.7 PMCR_EL0, Performance Monitors Control Register: Attributes PMCR_EL0 is a 32-bit register. If this ever gets extended, we would need extra code to deal with the new bits, so we would need to touch the code anyway. And again, it's just a local variable, not an interface. Cheers, Andre. P.S. We really should save this discussion for a Friday afternoon ;-) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
RE: [PATCH] KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs
> -Original Message- > From: Marc Zyngier [mailto:marc.zyng...@arm.com] > Sent: Thursday, December 01, 2016 10:58 AM > To: Shameerali Kolothum Thodi; Paolo Bonzini; Radim Krčmář > Cc: Catalin Marinas; kvmarm@lists.cs.columbia.edu; linux-arm- > ker...@lists.infradead.org; k...@vger.kernel.org > Subject: Re: [PATCH] KVM: arm/arm64: vgic: Don't notify EOI for non- > SPIs > > On 01/12/16 10:28, Shameerali Kolothum Thodi wrote: > > Hi Marc, > > > >> -Original Message- > >> From: kvmarm-boun...@lists.cs.columbia.edu [mailto:kvmarm- > >> boun...@lists.cs.columbia.edu] On Behalf Of Marc Zyngier > >> Sent: Thursday, December 01, 2016 9:26 AM > >> To: Paolo Bonzini; Radim Krčmář > >> Cc: Catalin Marinas; kvmarm@lists.cs.columbia.edu; linux-arm- > >> ker...@lists.infradead.org; k...@vger.kernel.org > >> Subject: [PATCH] KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs > >> > >> When we inject a level triggerered interrupt (and unless it is > backed > >> by the physical distributor - timer style), we request a maintenance > >> interrupt. Part of the processing for that interrupt is to feed to > the > >> rest of KVM (and to the eventfd subsystem) the information that the > >> interrupt has been EOIed. > >> > >> But that notification only makes sense for SPIs, and not PPIs (such > as > >> the PMU interrupt). Skip over the notification if the interrupt is > not > >> an SPI. > > > > Just to clarify my understanding, the maintenance interrupt is > generated > > for cases where there is no mapping of virt to phys interrupts > > (ie, ICH_LR HW bit is not set). And I was under the impression that > > kvm_notify_acked_irq will eventually deactivate the interrupt on > distributor > > for such cases. Its not clear to me how the deactivation is done > > otherwise. > > > > Could you please help me to understand this better. > > kvm_notify_acked_irq() doesn't do *anything* at the distributor level, > ever (it has no idea of anything GIC-specific anyway). It's sole job is > to signal the rest of the stack that an interrupt has been EOIed in the > guest. Thanks Marc. Understood. I got confused by the kvm_set_irq in the kvm_notify_acked_irq path. > For these interrupts, which are purely virtual, there is absolutely > nothing to do at the physical distributor level anyway. Furthermore, > kvm_notify_acked_irq doesn't know about per-cpu interrupt, which is why > we cannot notify them. Just to confirm, that means for any phys interrupt(PPI/SPI) to be injected to the Guest, the mapping bit has(HW bit set) to be used. Thanks, Shameer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 2/4] arm: Add PMU test
On 1 December 2016 at 11:28, Andre Przywarawrote: > I don't think so. At least here as a _variable_ type uint32_t is > probably the right one, as the ARMv8 ARM explicitly says that PMCR is a > 32-bit register, for both bitnesses. For 64-bit ARM this is strictly speaking just shorthand for "64-bit register with the top 32-bit being RES0". It is in theory possible that a future architecture extension might define uses for those RES0 bits. thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v2] KVM: arm/arm64: Access CNTHCTL_EL2 bit fields correctly
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: Skip configuring cnthctl_el2 in world switch path on VHE system. This patch is based on linux-next. --- arch/arm/kvm/arm.c | 1 + include/kvm/arm_arch_timer.h | 1 + virt/kvm/arm/arch_timer.c| 23 ++ virt/kvm/arm/hyp/timer-sr.c | 45 4 files changed, 58 insertions(+), 12 deletions(-) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 8f92efa..38c0645 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -1286,6 +1286,7 @@ static void teardown_hyp_mode(void) static int init_vhe_mode(void) { + on_each_cpu(kvm_timer_init_vhe, NULL, 1); kvm_info("VHE mode initialized successfully\n"); return 0; } diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h index dda39d8..5853399 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 *dummy); #endif diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 17b8fa5..7a0d85d7 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 *dummy) +{ + /* 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..f7fc957 100644 --- a/virt/kvm/arm/hyp/timer-sr.c +++ b/virt/kvm/arm/hyp/timer-sr.c @@ -21,6 +21,18 @@ #include +#ifdef CONFIG_ARM64 +static inline bool has_vhe(void) +{ + if (cpus_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN)) + return true; + + return false; +} +#else +#define has_vhe() false +#endif + /* vcpu is already in the HYP VA space */ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu) { @@ -35,10 +47,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 |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; - write_sysreg(val, cnthctl_el2); + /* +* We don't need to do this for VHE since the host kernel runs in EL2 +* with HCR_EL2.TGE ==1, which makes those bits have no impact. +*/ + if (!has_vhe()) { + /* Allow physical timer/counter access for the host */ + val = read_sysreg(cnthctl_el2); + val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; + write_sysreg(val, cnthctl_el2); + } /* Clear cntvoff for the host */ write_sysreg(0, cntvoff_el2); @@ -50,14 +68,17 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu) struct arch_timer_cpu *timer = >arch.timer_cpu; u64 val; - /* -* Disallow physical timer access for the guest -* Physical counter access is allowed -*/ - val = read_sysreg(cnthctl_el2); - val &= ~CNTHCTL_EL1PCEN; - val |= CNTHCTL_EL1PCTEN; - write_sysreg(val, cnthctl_el2); + /* Those bits are already configured at boot on VHE-system */ + if (!has_vhe()) { + /* +* Disallow physical timer access for the guest +* Physical counter access is allowed +*/ + val =
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 2/4] arm: Add PMU test
Hi, On 01/12/16 09:03, Andrew Jones wrote: > On Wed, Nov 30, 2016 at 11:16:40PM -0600, Wei Huang wrote: >> From: Christopher Covington>> >> Beginning with a simple sanity check of the control register, add >> a unit test for the ARM Performance Monitors Unit (PMU). PMU register >> was read using the newly defined macros. >> >> Signed-off-by: Christopher Covington >> Signed-off-by: Wei Huang >> Reviewed-by: Andrew Jones >> --- >> arm/Makefile.common | 3 ++- >> arm/pmu.c | 62 >> + >> arm/unittests.cfg | 5 + >> 3 files changed, 69 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..1fe2b1a >> --- /dev/null >> +++ b/arm/pmu.c >> @@ -0,0 +1,62 @@ >> +/* >> + * Test the ARM Performance Monitors Unit (PMU). >> + * >> + * Copyright (c) 2015-2016, The Linux Foundation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU Lesser General Public License version 2.1 and >> + * only version 2.1 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, but >> WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public >> License >> + * for more details. >> + */ >> +#include "libcflat.h" >> +#include "asm/barrier.h" >> +#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 >> + >> +#if defined(__arm__) >> +DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0) >> +#elif defined(__aarch64__) >> +DEFINE_GET_SYSREG32(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; > > So based on my comments from the previous patch, pmcr should be > 'unsigned int' I don't think so. At least here as a _variable_ type uint32_t is probably the right one, as the ARMv8 ARM explicitly says that PMCR is a 32-bit register, for both bitnesses. I find it only natural to express it here accordingly. I believe this is a different (though related) discussion from the return type of the accessor functions. Cheers, Andre. >> + >> +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; >> +} >> + >> +int main(void) >> +{ >> +report_prefix_push("pmu"); >> + >> +report("Control register", check_pmcr()); >> + >> +return report_summary(); >> +} >> diff --git a/arm/unittests.cfg b/arm/unittests.cfg >> index 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 >> >> > > drew > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH v13 3/4] arm: pmu: Check cycle count increases
Hi, On 01/12/16 05:16, Wei Huang wrote: > From: Christopher Covington> > Ensure that reads of the PMCCNTR_EL0 are monotonically increasing, > even for the smallest delta of two subsequent reads. > > Signed-off-by: Christopher Covington > Signed-off-by: Wei Huang > Reviewed-by: Andrew Jones > --- > arm/pmu.c | 94 > +++ > 1 file changed, 94 insertions(+) > > diff --git a/arm/pmu.c b/arm/pmu.c > index 1fe2b1a..3566a27 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -16,6 +16,9 @@ > #include "asm/barrier.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 > @@ -23,10 +26,57 @@ > #define PMU_PMCR_IMP_SHIFT 24 > #define PMU_PMCR_IMP_MASK 0xff > > +#define ID_DFR0_PERFMON_SHIFT 24 > +#define ID_DFR0_PERFMON_MASK 0xf > + > +#define PMU_CYCLE_IDX 31 > + > +#define NR_SAMPLES 10 > + > +static unsigned int pmu_version; > #if defined(__arm__) > DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0) > +DEFINE_SET_SYSREG32(pmcr, 0, c9, c12, 0) > +DEFINE_GET_SYSREG32(id_dfr0, 0, c0, c1, 2) > +DEFINE_SET_SYSREG32(pmselr, 0, c9, c12, 5) > +DEFINE_SET_SYSREG32(pmxevtyper, 0, c9, c13, 1) > +DEFINE_GET_SYSREG32(pmccntr32, 0, c9, c13, 0) > +DEFINE_SET_SYSREG32(pmccntr32, 0, c9, c13, 0) > +DEFINE_GET_SYSREG64(pmccntr64, 0, c9) > +DEFINE_SET_SYSREG64(pmccntr64, 0, c9) > +DEFINE_SET_SYSREG32(pmcntenset, 0, c9, c12, 1) > + > +static inline uint64_t get_pmccntr(void) > +{ > + if (pmu_version == 0x3) > + return get_pmccntr64(); > + else > + return get_pmccntr32(); > +} > + > +static inline void set_pmccntr(uint64_t value) > +{ > + if (pmu_version == 0x3) > + set_pmccntr64(value); > + else > + set_pmccntr32(value & 0x); > +} > + > +/* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */ > +static inline void set_pmccfiltr(uint32_t value) > +{ > + set_pmselr(PMU_CYCLE_IDX); > + set_pmxevtyper(value); > + isb(); > +} > #elif defined(__aarch64__) > DEFINE_GET_SYSREG32(pmcr, el0) > +DEFINE_SET_SYSREG32(pmcr, el0) > +DEFINE_GET_SYSREG32(id_dfr0, el1) > +DEFINE_GET_SYSREG64(pmccntr, el0); > +DEFINE_SET_SYSREG64(pmccntr, el0); > +DEFINE_SET_SYSREG32(pmcntenset, el0); > +DEFINE_SET_SYSREG32(pmccfiltr, el0); > #endif > > /* > @@ -52,11 +102,55 @@ static bool check_pmcr(void) > return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0; > } > > +/* > + * Ensure that the cycle counter progresses between back-to-back reads. > + */ > +static bool check_cycles_increase(void) > +{ > + bool success = true; > + > + /* 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_pmccntr(0); Why do we need this? Shouldn't PMU_PMCR_C below take care of that? > + > + 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; > +} > + > +void pmu_init(void) Mmh, this function doesn't really initialize anything, does it? Should it be named pmu_available() or pmu_version() or the like? And should we bail out early here (or rather at the caller) if this register reports that no PMU is available? For instance by making it return a boolean? > +{ > + uint32_t dfr0; > + > + /* probe pmu version */ > + dfr0 = get_id_dfr0(); > + pmu_version = (dfr0 >> ID_DFR0_PERFMON_SHIFT) & ID_DFR0_PERFMON_MASK; > + report_info("PMU version: %d", pmu_version); > +} > + > int main(void) > { > report_prefix_push("pmu"); > > + pmu_init(); > report("Control register", check_pmcr()); > + report("Monotonically increasing cycle count", check_cycles_increase()); I wonder if we should skip this test if check_pmcr() has returned false before? We let it return a boolean, so it seems quite natural to use this information here. This would avoid a lot of false FAILs due to the PMU not being available (because QEMU is too old, for instance). Cheers, Andre. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 1/4] arm: Define macros for accessing system registers
Hi, On 01/12/16 08:59, Andrew Jones wrote: > > Should this be From: Andre? No need from my side, this way all the bug reports are send to Wei ;-) > On Wed, Nov 30, 2016 at 11:16:39PM -0600, Wei Huang wrote: >> This patch defines four macros to assist creating system register >> accessors under both ARMv7 and AArch64: >>* DEFINE_GET_SYSREG32(name, ...) >>* DEFINE_SET_SYSREG32(name, ...) >>* DEFINE_GET_SYSREG64(name, ...) >>* DEFINE_SET_SYSREG64(name, ...) >> These macros are translated to inline functions with consistent naming, >> get_##name() and set_##name(), which can be used by C code directly. >> >> Signed-off-by: Andre Przywara>> Signed-off-by: Wei Huang >> --- >> lib/arm/asm/processor.h | 37 - >> lib/arm64/asm/processor.h | 35 --- >> 2 files changed, 60 insertions(+), 12 deletions(-) >> >> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h >> index f25e7ee..3ca6b42 100644 >> --- a/lib/arm/asm/processor.h >> +++ b/lib/arm/asm/processor.h >> @@ -33,13 +33,40 @@ static inline unsigned long current_cpsr(void) >> >> #define current_mode() (current_cpsr() & MODE_MASK) >> >> -static inline unsigned int get_mpidr(void) >> -{ >> -unsigned int mpidr; >> -asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r" (mpidr)); >> -return mpidr; >> +#define DEFINE_GET_SYSREG32(name, opc1, crn, crm, opc2) >> \ >> +static inline uint32_t get_##name(void) >> \ >> +{ \ >> +uint32_t reg; \ >> +asm volatile("mrc p15, " #opc1 ", %0, " #crn ", " #crm ", " \ >> + #opc2 : "=r" (reg)); \ >> +return reg; \ >> +} >> + >> +#define DEFINE_SET_SYSREG32(name, opc1, crn, crm, opc2) >> \ >> +static inline void set_##name(uint32_t value) >> \ >> +{ \ >> +asm volatile("mcr p15, " #opc1 ", %0, " #crn ", " #crm ", " \ >> + #opc2 :: "r" (value)); \ >^ nit: no space here, checkpatch would complain >> +} >> + >> +#define DEFINE_GET_SYSREG64(name, opc, crm) \ >> +static inline uint64_t get_##name(void) >> \ >> +{ \ >> +uint32_t lo, hi;\ >> +asm volatile("mrrc p15, " #opc ", %0, %1, " #crm\ >> + : "=r" (lo), "=r" (hi)); \ >> +return (uint64_t)hi << 32 | lo; \ >> +} >> + >> +#define DEFINE_SET_SYSREG64(name, opc, crm) \ >> +static inline void set_##name(uint64_t value) >> \ >> +{ \ >> +asm volatile("mcrr p15, " #opc ", %0, %1, " #crm\ >> + :: "r" (value & 0x), "r" (value >> 32)); \ >> } >> >> +DEFINE_GET_SYSREG32(mpidr, 0, c0, c0, 5) >> + >> /* Only support Aff0 for now, up to 4 cpus */ >> #define mpidr_to_cpu(mpidr) ((int)((mpidr) & 0xff)) >> >> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h >> index 84d5c7c..dfa75eb 100644 >> --- a/lib/arm64/asm/processor.h >> +++ b/lib/arm64/asm/processor.h >> @@ -66,14 +66,35 @@ 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; \ >> +#define DEFINE_GET_SYSREG32(reg, el) >> \ >> +static inline uint32_t get_##reg(void) >> \ >> +{ \ >> +uint32_t reg; \ >> +asm volatile("mrs %0, " #reg "_" #el : "=r" (reg)); \ >> +return reg; \ >> } >> -DEFINE_GET_SYSREG32(mpidr) >> + >> +#define DEFINE_SET_SYSREG32(reg, el) >> \ >> +static inline void set_##reg(uint32_t value) >> \ >> +{ \ >> +asm
Re: [PATCH] KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs
On 01/12/16 10:28, Shameerali Kolothum Thodi wrote: > Hi Marc, > >> -Original Message- >> From: kvmarm-boun...@lists.cs.columbia.edu [mailto:kvmarm- >> boun...@lists.cs.columbia.edu] On Behalf Of Marc Zyngier >> Sent: Thursday, December 01, 2016 9:26 AM >> To: Paolo Bonzini; Radim Krčmář >> Cc: Catalin Marinas; kvmarm@lists.cs.columbia.edu; linux-arm- >> ker...@lists.infradead.org; k...@vger.kernel.org >> Subject: [PATCH] KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs >> >> When we inject a level triggerered interrupt (and unless it is backed >> by the physical distributor - timer style), we request a maintenance >> interrupt. Part of the processing for that interrupt is to feed to the >> rest of KVM (and to the eventfd subsystem) the information that the >> interrupt has been EOIed. >> >> But that notification only makes sense for SPIs, and not PPIs (such as >> the PMU interrupt). Skip over the notification if the interrupt is not >> an SPI. > > Just to clarify my understanding, the maintenance interrupt is generated > for cases where there is no mapping of virt to phys interrupts > (ie, ICH_LR HW bit is not set). And I was under the impression that > kvm_notify_acked_irq will eventually deactivate the interrupt on distributor > for such cases. Its not clear to me how the deactivation is done > otherwise. > > Could you please help me to understand this better. kvm_notify_acked_irq() doesn't do *anything* at the distributor level, ever (it has no idea of anything GIC-specific anyway). It's sole job is to signal the rest of the stack that an interrupt has been EOIed in the guest. For these interrupts, which are purely virtual, there is absolutely nothing to do at the physical distributor level anyway. Furthermore, kvm_notify_acked_irq doesn't know about per-cpu interrupt, which is why we cannot notify them. 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] KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs
Hi Marc, > -Original Message- > From: kvmarm-boun...@lists.cs.columbia.edu [mailto:kvmarm- > boun...@lists.cs.columbia.edu] On Behalf Of Marc Zyngier > Sent: Thursday, December 01, 2016 9:26 AM > To: Paolo Bonzini; Radim Krčmář > Cc: Catalin Marinas; kvmarm@lists.cs.columbia.edu; linux-arm- > ker...@lists.infradead.org; k...@vger.kernel.org > Subject: [PATCH] KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs > > When we inject a level triggerered interrupt (and unless it is backed > by the physical distributor - timer style), we request a maintenance > interrupt. Part of the processing for that interrupt is to feed to the > rest of KVM (and to the eventfd subsystem) the information that the > interrupt has been EOIed. > > But that notification only makes sense for SPIs, and not PPIs (such as > the PMU interrupt). Skip over the notification if the interrupt is not > an SPI. Just to clarify my understanding, the maintenance interrupt is generated for cases where there is no mapping of virt to phys interrupts (ie, ICH_LR HW bit is not set). And I was under the impression that kvm_notify_acked_irq will eventually deactivate the interrupt on distributor for such cases. Its not clear to me how the deactivation is done otherwise. Could you please help me to understand this better. Thanks, Shameer > Cc: sta...@vger.kernel.org # 4.7+ > Fixes: 140b086dd197 ("KVM: arm/arm64: vgic-new: Add GICv2 world switch > backend") > Fixes: 59529f69f504 ("KVM: arm/arm64: vgic-new: Add GICv3 world switch > backend") > Reported-by: Catalin Marinas> Tested-by: Catalin Marinas > Acked-by: Christoffer Dall > Signed-off-by: Marc Zyngier > --- > virt/kvm/arm/vgic/vgic-v2.c | 6 -- > virt/kvm/arm/vgic/vgic-v3.c | 6 -- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > index 0a063af..9bab867 100644 > --- a/virt/kvm/arm/vgic/vgic-v2.c > +++ b/virt/kvm/arm/vgic/vgic-v2.c > @@ -50,8 +50,10 @@ void vgic_v2_process_maintenance(struct kvm_vcpu > *vcpu) > > WARN_ON(cpuif->vgic_lr[lr] & GICH_LR_STATE); > > - kvm_notify_acked_irq(vcpu->kvm, 0, > - intid - VGIC_NR_PRIVATE_IRQS); > + /* Only SPIs require notification */ > + if (vgic_valid_spi(vcpu->kvm, intid)) > + kvm_notify_acked_irq(vcpu->kvm, 0, > + intid - > VGIC_NR_PRIVATE_IRQS); > } > } > > diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c > index 9f0dae3..5c9f974 100644 > --- a/virt/kvm/arm/vgic/vgic-v3.c > +++ b/virt/kvm/arm/vgic/vgic-v3.c > @@ -41,8 +41,10 @@ void vgic_v3_process_maintenance(struct kvm_vcpu > *vcpu) > > WARN_ON(cpuif->vgic_lr[lr] & ICH_LR_STATE); > > - kvm_notify_acked_irq(vcpu->kvm, 0, > - intid - VGIC_NR_PRIVATE_IRQS); > + /* Only SPIs require notification */ > + if (vgic_valid_spi(vcpu->kvm, intid)) > + kvm_notify_acked_irq(vcpu->kvm, 0, > + intid - > VGIC_NR_PRIVATE_IRQS); > } > > /* > -- > 2.1.4 > > ___ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 4/4] arm: pmu: Add CPI checking
Hi Drew, actually unrelated to this actual patch, but since you mentioned it: > As we work out how best to handle tcg-only tests in order to get Alex > Bennee's MTTCG tests merged, we'll probably revisit this file, So when I was experimenting with kvmtool, I realized that some tests would need to be QEMU only. Also I was tempted to try some tests either on bare metal machines or in a Fast Model. So I wonder if we should have some constraints or tags on the tests, so that a certain backend can filter on this and skip if it's not capable? Just wanted to mention this so that we can use this refactoring opportunity to come up with something more generic than just a boolean TGC vs. KVM. Maybe we should introduce the notion of a "test backend"? That could be QEMU/KVM and TCG for now, but later extended to cover kvmtool and probably other hypervisors like Xen as well. Cheers, Andre. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 1/4] arm: Define macros for accessing system registers
On Thu, Dec 01, 2016 at 09:59:03AM +0100, Andrew Jones wrote: > > Should this be From: Andre? > > On Wed, Nov 30, 2016 at 11:16:39PM -0600, Wei Huang wrote: > > This patch defines four macros to assist creating system register > > accessors under both ARMv7 and AArch64: > >* DEFINE_GET_SYSREG32(name, ...) > >* DEFINE_SET_SYSREG32(name, ...) > >* DEFINE_GET_SYSREG64(name, ...) > >* DEFINE_SET_SYSREG64(name, ...) > > These macros are translated to inline functions with consistent naming, > > get_##name() and set_##name(), which can be used by C code directly. > > > > Signed-off-by: Andre Przywara> > Signed-off-by: Wei Huang > > --- > > lib/arm/asm/processor.h | 37 - > > lib/arm64/asm/processor.h | 35 --- > > 2 files changed, 60 insertions(+), 12 deletions(-) > > > > diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h > > index f25e7ee..3ca6b42 100644 > > --- a/lib/arm/asm/processor.h > > +++ b/lib/arm/asm/processor.h > > @@ -33,13 +33,40 @@ static inline unsigned long current_cpsr(void) > > > > #define current_mode() (current_cpsr() & MODE_MASK) > > > > -static inline unsigned int get_mpidr(void) > > -{ > > - unsigned int mpidr; > > - asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r" (mpidr)); > > - return mpidr; > > +#define DEFINE_GET_SYSREG32(name, opc1, crn, crm, opc2) > > \ > > +static inline uint32_t get_##name(void) > > \ > > +{ \ > > + uint32_t reg; \ > > + asm volatile("mrc p15, " #opc1 ", %0, " #crn ", " #crm ", " \ > > +#opc2 : "=r" (reg)); \ > > + return reg; \ > > +} > > + > > +#define DEFINE_SET_SYSREG32(name, opc1, crn, crm, opc2) > > \ > > +static inline void set_##name(uint32_t value) > > \ > > +{ \ > > + asm volatile("mcr p15, " #opc1 ", %0, " #crn ", " #crm ", " \ > > +#opc2 :: "r" (value)); \ >^ nit: no space here, checkpatch would complain > > +} > > + > > +#define DEFINE_GET_SYSREG64(name, opc, crm) > > \ > > +static inline uint64_t get_##name(void) > > \ > > +{ \ > > + uint32_t lo, hi;\ > > + asm volatile("mrrc p15, " #opc ", %0, %1, " #crm\ > > +: "=r" (lo), "=r" (hi)); \ > > + return (uint64_t)hi << 32 | lo; \ > > +} > > + > > +#define DEFINE_SET_SYSREG64(name, opc, crm) > > \ > > +static inline void set_##name(uint64_t value) > > \ > > +{ \ > > + asm volatile("mcrr p15, " #opc ", %0, %1, " #crm\ > > +:: "r" (value & 0x), "r" (value >> 32)); \ > > } > > > > +DEFINE_GET_SYSREG32(mpidr, 0, c0, c0, 5) > > + > > /* Only support Aff0 for now, up to 4 cpus */ > > #define mpidr_to_cpu(mpidr) ((int)((mpidr) & 0xff)) > > > > diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h > > index 84d5c7c..dfa75eb 100644 > > --- a/lib/arm64/asm/processor.h > > +++ b/lib/arm64/asm/processor.h > > @@ -66,14 +66,35 @@ 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; \ > > +#define DEFINE_GET_SYSREG32(reg, el) > > \ > > +static inline uint32_t get_##reg(void) > > \ > > +{ \ > > + uint32_t reg; \ > > + asm volatile("mrs %0, " #reg "_" #el : "=r" (reg)); \ > > + return reg; \ > > } > > -DEFINE_GET_SYSREG32(mpidr) > > + > > +#define DEFINE_SET_SYSREG32(reg, el) > > \ > > +static inline void set_##reg(uint32_t value) > > \ > > +{
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 4/4] arm: pmu: Add CPI checking
On Wed, Nov 30, 2016 at 11:16:42PM -0600, Wei Huang wrote: > From: Christopher Covington> > Calculate the numbers of cycles per instruction (CPI) implied by ARM > PMU cycle counter values. The code includes a strict checking facility > intended for the -icount option in TCG mode in the configuration file. > > Signed-off-by: Christopher Covington > Signed-off-by: Wei Huang > Reviewed-by: Andrew Jones > --- > arm/pmu.c | 123 > +- > arm/unittests.cfg | 14 +++ > 2 files changed, 136 insertions(+), 1 deletion(-) > > diff --git a/arm/pmu.c b/arm/pmu.c > index 3566a27..29d7c2c 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -69,6 +69,27 @@ static inline void set_pmccfiltr(uint32_t value) > set_pmxevtyper(value); > 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__) > DEFINE_GET_SYSREG32(pmcr, el0) > DEFINE_SET_SYSREG32(pmcr, el0) > @@ -77,6 +98,27 @@ DEFINE_GET_SYSREG64(pmccntr, el0); > DEFINE_SET_SYSREG64(pmccntr, el0); > DEFINE_SET_SYSREG32(pmcntenset, el0); > DEFINE_SET_SYSREG32(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 > > /* > @@ -134,6 +176,79 @@ 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, also strictly check that every measurement matches > + * it. Strict CPI checking is used to test -icount mode. > + */ > +static bool check_cpi(int cpi) > +{ > + uint32_t pmcr = 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 (unsigned int i = 4; i < 300; i += 32) { > + uint64_t avg, sum = 0; > + > + printf("%d :", i); > + for (int j = 0; j < NR_SAMPLES; j++) { > + uint64_t cycles; > + > + set_pmccntr(0); > + measure_instrs(i, pmcr); > + cycles = get_pmccntr(); > + printf(" %"PRId64"", cycles); > + > + if (!cycles) { > +
[PATCH] KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs
When we inject a level triggerered interrupt (and unless it is backed by the physical distributor - timer style), we request a maintenance interrupt. Part of the processing for that interrupt is to feed to the rest of KVM (and to the eventfd subsystem) the information that the interrupt has been EOIed. But that notification only makes sense for SPIs, and not PPIs (such as the PMU interrupt). Skip over the notification if the interrupt is not an SPI. Cc: sta...@vger.kernel.org # 4.7+ Fixes: 140b086dd197 ("KVM: arm/arm64: vgic-new: Add GICv2 world switch backend") Fixes: 59529f69f504 ("KVM: arm/arm64: vgic-new: Add GICv3 world switch backend") Reported-by: Catalin MarinasTested-by: Catalin Marinas Acked-by: Christoffer Dall Signed-off-by: Marc Zyngier --- virt/kvm/arm/vgic/vgic-v2.c | 6 -- virt/kvm/arm/vgic/vgic-v3.c | 6 -- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c index 0a063af..9bab867 100644 --- a/virt/kvm/arm/vgic/vgic-v2.c +++ b/virt/kvm/arm/vgic/vgic-v2.c @@ -50,8 +50,10 @@ void vgic_v2_process_maintenance(struct kvm_vcpu *vcpu) WARN_ON(cpuif->vgic_lr[lr] & GICH_LR_STATE); - kvm_notify_acked_irq(vcpu->kvm, 0, -intid - VGIC_NR_PRIVATE_IRQS); + /* Only SPIs require notification */ + if (vgic_valid_spi(vcpu->kvm, intid)) + kvm_notify_acked_irq(vcpu->kvm, 0, +intid - VGIC_NR_PRIVATE_IRQS); } } diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c index 9f0dae3..5c9f974 100644 --- a/virt/kvm/arm/vgic/vgic-v3.c +++ b/virt/kvm/arm/vgic/vgic-v3.c @@ -41,8 +41,10 @@ void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu) WARN_ON(cpuif->vgic_lr[lr] & ICH_LR_STATE); - kvm_notify_acked_irq(vcpu->kvm, 0, -intid - VGIC_NR_PRIVATE_IRQS); + /* Only SPIs require notification */ + if (vgic_valid_spi(vcpu->kvm, intid)) + kvm_notify_acked_irq(vcpu->kvm, 0, +intid - VGIC_NR_PRIVATE_IRQS); } /* -- 2.1.4 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PULL] KVM/ARM updates for 4.9-rc7
Paolo, Radim, Hopefully, this is the last update for 4.9. This time, a single patch that prevents bogus acknoledgement of interrupts. It'd be great if this could make it into v4.9-final Thanks, M. The following changes since commit b112c84a6ff035271d41d548c10215f18443d6a6: KVM: arm64: Fix the issues when guest PMCCFILTR is configured (2016-11-18 09:06:58 +) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-for-4.9-rc7 for you to fetch changes up to 8ca18eec2b2276b449c1dc86b98bf083c5fe4e09: KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs (2016-11-24 13:12:07 +) KVM/ARM updates for v4.9-rc7 - Do not call kvm_notify_acked for PPIs Marc Zyngier (1): KVM: arm/arm64: vgic: Don't notify EOI for non-SPIs virt/kvm/arm/vgic/vgic-v2.c | 6 -- virt/kvm/arm/vgic/vgic-v3.c | 6 -- 2 files changed, 8 insertions(+), 4 deletions(-) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 3/4] arm: pmu: Check cycle count increases
On Wed, Nov 30, 2016 at 11:16:41PM -0600, Wei Huang wrote: > From: Christopher Covington> > Ensure that reads of the PMCCNTR_EL0 are monotonically increasing, > even for the smallest delta of two subsequent reads. > > Signed-off-by: Christopher Covington > Signed-off-by: Wei Huang > Reviewed-by: Andrew Jones > --- > arm/pmu.c | 94 > +++ > 1 file changed, 94 insertions(+) > > diff --git a/arm/pmu.c b/arm/pmu.c > index 1fe2b1a..3566a27 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -16,6 +16,9 @@ > #include "asm/barrier.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 > @@ -23,10 +26,57 @@ > #define PMU_PMCR_IMP_SHIFT 24 > #define PMU_PMCR_IMP_MASK 0xff > > +#define ID_DFR0_PERFMON_SHIFT 24 > +#define ID_DFR0_PERFMON_MASK 0xf > + > +#define PMU_CYCLE_IDX 31 > + > +#define NR_SAMPLES 10 > + > +static unsigned int pmu_version; > #if defined(__arm__) > DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0) > +DEFINE_SET_SYSREG32(pmcr, 0, c9, c12, 0) > +DEFINE_GET_SYSREG32(id_dfr0, 0, c0, c1, 2) > +DEFINE_SET_SYSREG32(pmselr, 0, c9, c12, 5) > +DEFINE_SET_SYSREG32(pmxevtyper, 0, c9, c13, 1) > +DEFINE_GET_SYSREG32(pmccntr32, 0, c9, c13, 0) > +DEFINE_SET_SYSREG32(pmccntr32, 0, c9, c13, 0) > +DEFINE_GET_SYSREG64(pmccntr64, 0, c9) > +DEFINE_SET_SYSREG64(pmccntr64, 0, c9) > +DEFINE_SET_SYSREG32(pmcntenset, 0, c9, c12, 1) Seeing how we get lots of redundant looking lines, I think instead of defining DEFINE_SET/GET_SYSREG32/64, we should instead have DEFINE_SYSREG32/64 ... creates both get_ and set_ DEFINE_SYSREG32/64_RO ... creates just get_ > + > +static inline uint64_t get_pmccntr(void) > +{ > + if (pmu_version == 0x3) > + return get_pmccntr64(); > + else > + return get_pmccntr32(); > +} > + > +static inline void set_pmccntr(uint64_t value) > +{ > + if (pmu_version == 0x3) > + set_pmccntr64(value); > + else > + set_pmccntr32(value & 0x); > +} So the two accessors above are exceptional, which is why we don't use SYSREG for them. These can have uint64_t for there external interface. We can't require 'unsigned long' or 'unsigned long long' > + > +/* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */ > +static inline void set_pmccfiltr(uint32_t value) > +{ > + set_pmselr(PMU_CYCLE_IDX); > + set_pmxevtyper(value); > + isb(); > +} > #elif defined(__aarch64__) > DEFINE_GET_SYSREG32(pmcr, el0) > +DEFINE_SET_SYSREG32(pmcr, el0) > +DEFINE_GET_SYSREG32(id_dfr0, el1) > +DEFINE_GET_SYSREG64(pmccntr, el0); > +DEFINE_SET_SYSREG64(pmccntr, el0); > +DEFINE_SET_SYSREG32(pmcntenset, el0); > +DEFINE_SET_SYSREG32(pmccfiltr, el0); > #endif > > /* > @@ -52,11 +102,55 @@ static bool check_pmcr(void) > return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0; > } > > +/* > + * Ensure that the cycle counter progresses between back-to-back reads. > + */ > +static bool check_cycles_increase(void) > +{ > + bool success = true; > + > + /* 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_pmccntr(0); > + > + 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; > +} > + > +void pmu_init(void) > +{ > + uint32_t dfr0; > + > + /* probe pmu version */ > + dfr0 = get_id_dfr0(); > + pmu_version = (dfr0 >> ID_DFR0_PERFMON_SHIFT) & ID_DFR0_PERFMON_MASK; > + report_info("PMU version: %d", pmu_version); > +} > + > int main(void) > { > report_prefix_push("pmu"); > > + pmu_init(); > report("Control register", check_pmcr()); > + report("Monotonically increasing cycle count", check_cycles_increase()); > > return report_summary(); > } > -- > 1.8.3.1 > > drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: Unable to use perf in VM
On 30/11/16 19:17, Wei Huang wrote: > > > On 11/30/2016 07:37 AM, Marc Zyngier wrote: >> On 30/11/16 11:48, Marc Zyngier wrote: >>> + Shannon >>> >>> On 29/11/16 22:04, Itaru Kitayama wrote: Hi, In a VM (virsh controlled, KVM acceleration enabled) on a recent kvmarm kernel host, I find I am unable to use perf to obtain performance statistics for a complex task like kernel build. (I've verified this is seen with a Fedora 25 VM and host combination as well) APM folks CC'ed think this might be caused by a bug in the core PMU framework code, thus I'd like to have experts opinion on this issue. [root@localhost linux]# perf stat -B make CHK include/config/kernel.release [ 119.617684] git[1144]: undefined instruction: pc=fc000808ff30 [ 119.623040] Code: 51000442 92401042 d51b9ca2 d5033fdf (d53b9d40) [ 119.627607] Internal error: undefined instruction: 0 [#1] SMP >>> >>> [...] >>> >>> In a VM running mainline hosted on an AMD Seattle box: >>> >>> Performance counter stats for 'make': >>> >>> 1526089.499304 task-clock:u (msec) #0.932 CPUs utilized >>> >>> 0 context-switches:u#0.000 K/sec >>> >>> 0 cpu-migrations:u #0.000 K/sec >>> >>> 29527793 page-faults:u #0.019 M/sec >>> >>> 2913174122673 cycles:u #1.909 GHz >>> >>> 2365040892322 instructions:u#0.81 insn per cycle >>> >>> branches:u >>> >>>32049215378 branch-misses:u #0.00% of all >>> branches >>> >>> 1637.531444837 seconds time elapsed >>> >>> Running the same host kernel on a Mustang system, the guest explodes >>> in the way you reported. The failing instruction always seems to be >>> an access to pmxevcntr_el0 (I've seen both reads and writes). >>> >>> Funnily enough, it dies if you try any HW event other than cycles >>> ("perf stat -e cycles ls" works, and "perf stat -e instructions ls" >>> explodes). Which would tend to indicate that we're screwing up >>> the counter selection, but I have no proof of that (specially that >>> the Seattle guest is working just as expected). >> >> It turns out that we *don't* inject an undef. It seems to be generated >> locally at EL1. >> >> Still digging. > > Just FYI: I saw it on Mustang before. My initial thought was HW related, > but without proof. I am interested to see your findings... It would have been good to report it earlier. Anyway, I've identified the root issue, which seems to boil down to how you interpret a small corner of the PMU architecture. I've raised it with the architecture team here, and I should have a workaround/fix shortly. Thanks, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 2/4] arm: Add PMU test
On Wed, Nov 30, 2016 at 11:16:40PM -0600, Wei Huang wrote: > From: Christopher Covington> > Beginning with a simple sanity check of the control register, add > a unit test for the ARM Performance Monitors Unit (PMU). PMU register > was read using the newly defined macros. > > Signed-off-by: Christopher Covington > Signed-off-by: Wei Huang > Reviewed-by: Andrew Jones > --- > arm/Makefile.common | 3 ++- > arm/pmu.c | 62 > + > arm/unittests.cfg | 5 + > 3 files changed, 69 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..1fe2b1a > --- /dev/null > +++ b/arm/pmu.c > @@ -0,0 +1,62 @@ > +/* > + * Test the ARM Performance Monitors Unit (PMU). > + * > + * Copyright (c) 2015-2016, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Lesser General Public License version 2.1 and > + * only version 2.1 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but > WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public > License > + * for more details. > + */ > +#include "libcflat.h" > +#include "asm/barrier.h" > +#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 > + > +#if defined(__arm__) > +DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0) > +#elif defined(__aarch64__) > +DEFINE_GET_SYSREG32(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; So based on my comments from the previous patch, pmcr should be 'unsigned int' > + > + 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; > +} > + > +int main(void) > +{ > + report_prefix_push("pmu"); > + > + report("Control register", check_pmcr()); > + > + return report_summary(); > +} > diff --git a/arm/unittests.cfg b/arm/unittests.cfg > index 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 > > drew ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [kvm-unit-tests PATCH v13 1/4] arm: Define macros for accessing system registers
Should this be From: Andre? On Wed, Nov 30, 2016 at 11:16:39PM -0600, Wei Huang wrote: > This patch defines four macros to assist creating system register > accessors under both ARMv7 and AArch64: >* DEFINE_GET_SYSREG32(name, ...) >* DEFINE_SET_SYSREG32(name, ...) >* DEFINE_GET_SYSREG64(name, ...) >* DEFINE_SET_SYSREG64(name, ...) > These macros are translated to inline functions with consistent naming, > get_##name() and set_##name(), which can be used by C code directly. > > Signed-off-by: Andre Przywara> Signed-off-by: Wei Huang > --- > lib/arm/asm/processor.h | 37 - > lib/arm64/asm/processor.h | 35 --- > 2 files changed, 60 insertions(+), 12 deletions(-) > > diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h > index f25e7ee..3ca6b42 100644 > --- a/lib/arm/asm/processor.h > +++ b/lib/arm/asm/processor.h > @@ -33,13 +33,40 @@ static inline unsigned long current_cpsr(void) > > #define current_mode() (current_cpsr() & MODE_MASK) > > -static inline unsigned int get_mpidr(void) > -{ > - unsigned int mpidr; > - asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r" (mpidr)); > - return mpidr; > +#define DEFINE_GET_SYSREG32(name, opc1, crn, crm, opc2) > \ > +static inline uint32_t get_##name(void) > \ > +{\ > + uint32_t reg; \ > + asm volatile("mrc p15, " #opc1 ", %0, " #crn ", " #crm ", " \ > + #opc2 : "=r" (reg)); \ > + return reg; \ > +} > + > +#define DEFINE_SET_SYSREG32(name, opc1, crn, crm, opc2) > \ > +static inline void set_##name(uint32_t value) > \ > +{\ > + asm volatile("mcr p15, " #opc1 ", %0, " #crn ", " #crm ", " \ > + #opc2 :: "r" (value)); \ ^ nit: no space here, checkpatch would complain > +} > + > +#define DEFINE_GET_SYSREG64(name, opc, crm) \ > +static inline uint64_t get_##name(void) > \ > +{\ > + uint32_t lo, hi;\ > + asm volatile("mrrc p15, " #opc ", %0, %1, " #crm\ > + : "=r" (lo), "=r" (hi)); \ > + return (uint64_t)hi << 32 | lo; \ > +} > + > +#define DEFINE_SET_SYSREG64(name, opc, crm) \ > +static inline void set_##name(uint64_t value) > \ > +{\ > + asm volatile("mcrr p15, " #opc ", %0, %1, " #crm\ > + :: "r" (value & 0x), "r" (value >> 32)); \ > } > > +DEFINE_GET_SYSREG32(mpidr, 0, c0, c0, 5) > + > /* Only support Aff0 for now, up to 4 cpus */ > #define mpidr_to_cpu(mpidr) ((int)((mpidr) & 0xff)) > > diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h > index 84d5c7c..dfa75eb 100644 > --- a/lib/arm64/asm/processor.h > +++ b/lib/arm64/asm/processor.h > @@ -66,14 +66,35 @@ 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; \ > +#define DEFINE_GET_SYSREG32(reg, el) \ > +static inline uint32_t get_##reg(void) > \ > +{\ > + uint32_t reg; \ > + asm volatile("mrs %0, " #reg "_" #el : "=r" (reg)); \ > + return reg; \ > } > -DEFINE_GET_SYSREG32(mpidr) > + > +#define DEFINE_SET_SYSREG32(reg, el) \ > +static inline void set_##reg(uint32_t value) \ > +{\ > + asm volatile("msr " #reg "_" #el ", %0" :: "r" (value));\ > +} > + > +#define DEFINE_GET_SYSREG64(reg, el) \ > +static inline uint64_t get_##reg(void)