Re: [PATCH] KVM: arm: fix missing free_percpu_irq in kvm_timer_hyp_init()
Marc Zyngier wrote: >On 2019-11-23 02:30, linmiaohe wrote: >> From: Miaohe Lin >> >> When host_ptimer_irq request irq resource failed, we forget to release >> the host_vtimer_irq resource already requested. >> Fix this missing irq release and other similar scenario. > >That's really not a big deal, as nothing but KVM can use the timers anyway, >but I guess it doesn't hurt to be correct. I think It's a good practice to release the never used resources though it may be harmless. >> >> -out_free_irq: >> + >> +out_free_ptimer_irq: >> +free_percpu_irq(host_ptimer_irq, kvm_get_running_vcpus()); >> +out_disable_gic_state: >> +if (has_gic) >> +static_branch_disable(&has_gic_active_state); > >Given that we're failing the init of KVM, this is totally superfluous. Also, >this state is still valid, no matter what happens (the GIC is not going away >from under our feet). > Would you like a v2 patch without out_disable_gic_state cleanup ? If so, I would send a new one. But if you think this patch isn't worth to pick up, I would drop it. Many thanks for your review. >> +out_free_vtimer_irq: >> free_percpu_irq(host_vtimer_irq, kvm_get_running_vcpus()); >> + ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/2] kvm/arm64: don't log IMP DEF sysreg traps
On Thu, 05 Dec 2019 18:06:52 +, Mark Rutland wrote: > > We don't intend to support IMPLEMENATION DEFINED system registers, but > have to trap them (and emulate them as UNDEFINED). These traps aren't > interesting to the system administrator or to the KVM developers, so > let's not bother logging when we do so. > > Signed-off-by: Mark Rutland > Cc: Alexandru Elisei > Cc: James Morse > Cc: Julien Thierry > Cc: Marc Zyngier > Cc: Suzuki K Poulose > Cc: kvmarm@lists.cs.columbia.edu > --- > arch/arm64/kvm/sys_regs.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index d128abd38656..61f019104841 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -2233,6 +2233,12 @@ int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct > kvm_run *run) > NULL, 0); > } > > +static bool is_imp_def_sys_reg(struct sys_reg_params *params) > +{ > + // See ARM DDI 0487E.a, section D12.3.2 > + return params->Op0 == 3 && (params->CRn & 0b1011) == 0b1011; > +} > + > static int emulate_sys_reg(struct kvm_vcpu *vcpu, > struct sys_reg_params *params) > { > @@ -2248,6 +2254,8 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu, > > if (likely(r)) { > perform_access(vcpu, params, r); > + } else if (is_imp_def_sysreg(params)) { Meh. Doesn't compile... :-( Fixing it locally. 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 v2 3/5] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions
On 12/6/19 6:08 AM, Peter Maydell wrote: >> DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64) >> DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64) >> DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64) >> + >> +DEF_HELPER_3(check_hcr_el2_trap, void, env, i32, i32) > > This has to be in helper.h, not helper-a64.h, otherwise > the arm-softmmu target won't build. helper-a64.h is for > helper functions which only exist in the aarch64 binary. Oh, while we're at it, DEF_HELPER_FLAGS_3(..., TCG_CALL_NO_WG, ...) The helper does not modify tcg globals (on successful return). It does read globals (via the exception path), and of course it has side effects (the exception). r~ ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests RFC 01/10] arm64: Provide read/write_sysreg_s
Hi, On 12/6/19 5:27 PM, Eric Auger wrote: > From: Andrew Jones > > Sometimes we need to test access to system registers which are > missing assembler mnemonics. > > Signed-off-by: Andrew Jones > --- > lib/arm64/asm/sysreg.h | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h > index a03830b..a45eebd 100644 > --- a/lib/arm64/asm/sysreg.h > +++ b/lib/arm64/asm/sysreg.h > @@ -38,6 +38,17 @@ > asm volatile("msr " xstr(r) ", %x0" : : "rZ" (__val)); \ > } while (0) > > +#define read_sysreg_s(r) ({ \ > + u64 __val; \ > + asm volatile("mrs_s %0, " xstr(r) : "=r" (__val)); \ > + __val; \ > +}) > + > +#define write_sysreg_s(v, r) do {\ > + u64 __val = (u64)v; \ > + asm volatile("msr_s " xstr(r) ", %x0" : : "rZ" (__val));\ > +} while (0) > + > asm( > ".irp > num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n" > ".equ.L__reg_num_x\\num, \\num\n" That's exactly the code that I wrote for my EL2 series :) Reviewed-by: Alexandru Elisei Thanks, Alex ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests RFC 10/10] pmu: Test overflow interrupts
Test overflows for MEM_ACESS and SW_INCR events. Also tests overflows with 64-bit events. Signed-off-by: Eric Auger --- arm/pmu.c | 133 +- arm/unittests.cfg | 6 +++ 2 files changed, 138 insertions(+), 1 deletion(-) diff --git a/arm/pmu.c b/arm/pmu.c index 47d46a2..a63b93e 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -45,8 +45,12 @@ struct pmu { uint32_t pmcr_ro; }; -static struct pmu pmu; +struct pmu_stats { + unsigned long bitmap; + uint32_t interrupts[32]; +}; +static struct pmu pmu; #if defined(__arm__) #define ID_DFR0_PERFMON_SHIFT 24 @@ -117,6 +121,7 @@ static void test_mem_access(void) {} static void test_chained_counters(void) {} static void test_chained_sw_incr(void) {} static void test_chain_promotion(void) {} +static void test_overflow_interrupt(void) {} #elif defined(__aarch64__) #define ID_AA64DFR0_PERFMON_SHIFT 8 @@ -263,6 +268,43 @@ asm volatile( : ); } +static struct pmu_stats pmu_stats; + +static void irq_handler(struct pt_regs *regs) +{ +uint32_t irqstat, irqnr; + +irqstat = gic_read_iar(); +irqnr = gic_iar_irqnr(irqstat); +gic_write_eoir(irqstat); + +if (irqnr == 23) { +unsigned long overflows = read_sysreg(pmovsclr_el0); + int i; + +report_info("--> PMU overflow interrupt %d (counter bitmask 0x%lx)", irqnr, overflows); + for (i = 0; i < 32; i++) { + if (test_and_clear_bit(i, &overflows)) { + pmu_stats.interrupts[i]++; + pmu_stats.bitmap |= 1 << i; + } + } +write_sysreg(0x, pmovsclr_el0); +} else { +report_info("Unexpected interrupt: %d\n", irqnr); +} +} + +static void pmu_reset_stats(void) +{ + int i; + + for (i = 0; i < 32; i++) { + pmu_stats.interrupts[i] = 0; + } + pmu_stats.bitmap = 0; +} + static void pmu_reset(void) { /* reset all counters, counting disabled at PMCR level*/ @@ -273,6 +315,7 @@ static void pmu_reset(void) write_sysreg(0x, pmovsclr_el0); /* disable overflow interrupts on all counters */ write_sysreg(0x, pmintenclr_el1); + pmu_reset_stats(); isb(); } @@ -691,8 +734,93 @@ static void test_chain_promotion(void) read_sysreg(pmovsclr_el0)); } +static bool expect_interrupts(uint32_t bitmap) +{ + int i; + + if (pmu_stats.bitmap ^ bitmap) + return false; + + for (i = 0; i < 32; i++) { + if (test_and_clear_bit(i, &pmu_stats.bitmap)) + if (pmu_stats.interrupts[i] != 1) + return false; + } + return true; +} + +static void test_overflow_interrupt(void) +{ + uint32_t events[] = { 0x13 /* MEM_ACCESS */, 0x00 /* SW_INCR */}; + void *addr = malloc(PAGE_SIZE); + int i; + + if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) + return; + + setup_irq(irq_handler); + gic_enable_irq(23); + + pmu_reset(); + +write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0); +write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0); + write_sysreg_s(0x3, PMCNTENSET_EL0); + write_regn(pmevcntr, 0, 0xFFF0); + write_regn(pmevcntr, 1, 0xFFF0); + isb(); + + /* interrupts are disabled */ + + mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E); + report("no overflow interrupt received", expect_interrupts(0)); + + set_pmcr(pmu.pmcr_ro | PMU_PMCR_E); + for (i = 0; i < 100; i++) { + write_sysreg(0x2, pmswinc_el0); + } + set_pmcr(pmu.pmcr_ro); + report("no overflow interrupt received", expect_interrupts(0)); + + /* enable interrupts */ + + pmu_reset_stats(); + + write_regn(pmevcntr, 0, 0xFFF0); + write_regn(pmevcntr, 1, 0xFFF0); + write_sysreg(0x, pmintenset_el1); + isb(); + + mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E); + for (i = 0; i < 100; i++) { + write_sysreg(0x3, pmswinc_el0); + } + mem_access_loop(addr, 200, pmu.pmcr_ro); + report_info("overflow=0x%lx", read_sysreg(pmovsclr_el0)); + report("overflow interrupts expected on #0 and #1", expect_interrupts(0x3)); + + /* promote to 64-b */ + + pmu_reset_stats(); + + events[1] = 0x1E /* CHAIN */; +write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0); + write_regn(pmevcntr, 0, 0xFFF0); + isb(); + mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E); + report("no overflow interrupt expected on 32b boundary", expect_interrupts(0)); + + /* overflow on odd counter */ + pmu_reset_stats(); + wri
[kvm-unit-tests RFC 08/10] arm: gic: Provide per-IRQ helper functions
From: Andre Przywara A common theme when accessing per-IRQ parameters in the GIC distributor is to set fields of a certain bit width in a range of MMIO registers. Examples are the enabled status (one bit per IRQ), the level/edge configuration (2 bits per IRQ) or the priority (8 bits per IRQ). Add a generic helper function which is able to mask and set the respective number of bits, given the IRQ number and the MMIO offset. Provide wrappers using this function to easily allow configuring an IRQ. For now assume that private IRQ numbers always refer to the current CPU. In a GICv2 accessing the "other" private IRQs is not easily doable (the registers are banked per CPU on the same MMIO address), so we impose the same limitation on GICv3, even though those registers are not banked there anymore. Signed-off-by: Andre Przywara --- initialize reg --- lib/arm/asm/gic-v3.h | 2 + lib/arm/asm/gic.h| 9 + lib/arm/gic.c| 90 3 files changed, 101 insertions(+) diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h index 347be2f..4a445a5 100644 --- a/lib/arm/asm/gic-v3.h +++ b/lib/arm/asm/gic-v3.h @@ -23,6 +23,8 @@ #define GICD_CTLR_ENABLE_G1A (1U << 1) #define GICD_CTLR_ENABLE_G1(1U << 0) +#define GICD_IROUTER 0x6000 + /* Re-Distributor registers, offsets from RD_base */ #define GICR_TYPER 0x0008 diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h index 1fc10a0..21cdb58 100644 --- a/lib/arm/asm/gic.h +++ b/lib/arm/asm/gic.h @@ -15,6 +15,7 @@ #define GICD_IIDR 0x0008 #define GICD_IGROUPR 0x0080 #define GICD_ISENABLER 0x0100 +#define GICD_ICENABLER 0x0180 #define GICD_ISPENDR 0x0200 #define GICD_ICPENDR 0x0280 #define GICD_ISACTIVER 0x0300 @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat); extern void gic_ipi_send_single(int irq, int cpu); extern void gic_ipi_send_mask(int irq, const cpumask_t *dest); +void gic_set_irq_bit(int irq, int offset); +void gic_enable_irq(int irq); +void gic_disable_irq(int irq); +void gic_set_irq_priority(int irq, u8 prio); +void gic_set_irq_target(int irq, int cpu); +void gic_set_irq_group(int irq, int group); +int gic_get_irq_group(int irq); + #endif /* !__ASSEMBLY__ */ #endif /* _ASMARM_GIC_H_ */ diff --git a/lib/arm/gic.c b/lib/arm/gic.c index 9430116..aa9cb86 100644 --- a/lib/arm/gic.c +++ b/lib/arm/gic.c @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest) assert(gic_common_ops && gic_common_ops->ipi_send_mask); gic_common_ops->ipi_send_mask(irq, dest); } + +enum gic_bit_access { + ACCESS_READ, + ACCESS_SET, + ACCESS_RMW +}; + +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value, + enum gic_bit_access access) +{ + void *base; + int split = 32 / bits; + int shift = (irq % split) * bits; + u32 reg = 0, mask = ((1U << bits) - 1) << shift; + + switch (gic_version()) { + case 2: + base = gicv2_dist_base(); + break; + case 3: + if (irq < 32) + base = gicv3_sgi_base(); + else + base = gicv3_dist_base(); + break; + default: + return 0; + } + base += offset + (irq / split) * 4; + + switch (access) { + case ACCESS_READ: + return (readl(base) & mask) >> shift; + case ACCESS_SET: + reg = 0; + break; + case ACCESS_RMW: + reg = readl(base) & ~mask; + break; + } + + writel(reg | ((u32)value << shift), base); + + return 0; +} + +void gic_set_irq_bit(int irq, int offset) +{ + gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET); +} + +void gic_enable_irq(int irq) +{ + gic_set_irq_bit(irq, GICD_ISENABLER); +} + +void gic_disable_irq(int irq) +{ + gic_set_irq_bit(irq, GICD_ICENABLER); +} + +void gic_set_irq_priority(int irq, u8 prio) +{ + gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW); +} + +void gic_set_irq_target(int irq, int cpu) +{ + if (irq < 32) + return; + + if (gic_version() == 2) { + gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu, + ACCESS_RMW); + + return; + } + + writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8); +} + +void gic_set_irq_group(int irq, int group) +{ + gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW); +} + +int gic_get_irq_group(int irq) +{ + return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ); +} -- 2.20.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lis
[kvm-unit-tests RFC 06/10] pmu: Test chained counter
Add 2 tests exercising chained counters. The first one uses CPU_CYCLES and the second one uses SW_INCR. Signed-off-by: Eric Auger --- arm/pmu.c | 125 ++ arm/unittests.cfg | 12 + 2 files changed, 137 insertions(+) diff --git a/arm/pmu.c b/arm/pmu.c index 8ffeb93..e185809 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -114,6 +114,8 @@ static void test_event_introspection(void) {} static void test_event_counter_config(void) {} static void test_basic_event_count(void) {} static void test_mem_access(void) {} +static void test_chained_counters(void) {} +static void test_chained_sw_incr(void) {} #elif defined(__aarch64__) #define ID_AA64DFR0_PERFMON_SHIFT 8 @@ -452,6 +454,123 @@ static void test_mem_access(void) read_sysreg(pmovsclr_el0)); } +static void test_chained_counters(void) +{ + uint32_t events[] = { 0x11 /* CPU_CYCLES */, 0x1E /* CHAIN */}; + + if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) + return; + + pmu_reset(); + +write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0); +write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0); + /* enable counters #0 and #1 */ + write_sysreg_s(0x3, PMCNTENSET_EL0); + /* preset counter #0 at 0xFFF0 */ + write_regn(pmevcntr, 0, 0xFFF0); + + precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E); + + report("CHAIN counter #1 incremented", read_regn(pmevcntr, 1) == 1); + report("check no overflow is recorded", !read_sysreg(pmovsclr_el0)); + + /* test 64b overflow */ + + pmu_reset(); + write_sysreg_s(0x3, PMCNTENSET_EL0); + + write_regn(pmevcntr, 0, 0xFFF0); + write_regn(pmevcntr, 1, 0x1); + precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E); + report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0)); + report("CHAIN counter #1 incremented", read_regn(pmevcntr, 1) == 2); + report("check no overflow is recorded", !read_sysreg(pmovsclr_el0)); + + write_regn(pmevcntr, 0, 0xFFF0); + write_regn(pmevcntr, 1, 0x); + + precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E); + report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0)); + report("CHAIN counter #1 wrapped", !read_regn(pmevcntr, 1)); + report("check no overflow is recorded", read_sysreg(pmovsclr_el0) == 0x2); +} + +static void test_chained_sw_incr(void) +{ + uint32_t events[] = { 0x0 /* SW_INCR */, 0x0 /* SW_INCR */}; + int i; + + if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) + return; + + pmu_reset(); + +write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0); +write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0); + /* enable counters #0 and #1 */ + write_sysreg_s(0x3, PMCNTENSET_EL0); + + /* preset counter #0 at 0xFFF0 */ + write_regn(pmevcntr, 0, 0xFFF0); + + for (i = 0; i < 100; i++) { + write_sysreg(0x1, pmswinc_el0); + } + report_info("SW_INCR counter #0 has value %ld", read_regn(pmevcntr, 0)); + report("PWSYNC does not increment if PMCR.E is unset", + read_regn(pmevcntr, 0) == 0xFFF0); + + pmu_reset(); + + write_regn(pmevcntr, 0, 0xFFF0); + write_sysreg_s(0x3, PMCNTENSET_EL0); + set_pmcr(pmu.pmcr_ro | PMU_PMCR_E); + + for (i = 0; i < 100; i++) { + write_sysreg(0x3, pmswinc_el0); + } + report("counter #1 after + 100 SW_INCR", read_regn(pmevcntr, 0) == 84); + report("counter #0 after + 100 SW_INCR", read_regn(pmevcntr, 1) == 100); + report_info(" counter values after 100 SW_INCR #0=%ld #1=%ld", + read_regn(pmevcntr, 0), read_regn(pmevcntr, 1)); + report("overflow reg after 100 SW_INCR", read_sysreg(pmovsclr_el0) == 0x1); + + /* 64b SW_INCR */ + pmu_reset(); + + events[1] = 0x1E /* CHAIN */; +write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0); + write_regn(pmevcntr, 0, 0xFFF0); + write_sysreg_s(0x3, PMCNTENSET_EL0); + set_pmcr(pmu.pmcr_ro | PMU_PMCR_E); + for (i = 0; i < 100; i++) { + write_sysreg(0x3, pmswinc_el0); + } + report("overflow reg after 100 SW_INCR/CHAIN", + !read_sysreg(pmovsclr_el0) && (read_regn(pmevcntr, 1) == 1)); + report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0), + read_regn(pmevcntr, 0), read_regn(pmevcntr, 1)); + + /* 64b SW_INCR and overflow on CHAIN counter*/ + pmu_reset(); + +write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0); + write_regn(pmevcntr, 0, 0xFFF0); + write_regn(pmevcntr, 1, 0x); + write_sysreg_s(0x3, PMCNTENSET_EL0); + set_pmcr(pmu.pmcr_ro | PMU_PMCR_E); + for (i = 0; i < 100; i++) { +
[kvm-unit-tests RFC 09/10] arm/arm64: gic: Introduce setup_irq() helper
ipi_enable() code would be reusable for other interrupts than IPI. Let's rename it setup_irq() and pass an interrupt handler pointer. We also export it to use it in other tests such as the PMU's one. Signed-off-by: Eric Auger --- arm/gic.c | 24 +++- lib/arm/asm/gic.h | 3 +++ lib/arm/gic.c | 11 +++ 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/arm/gic.c b/arm/gic.c index adb6aa4..04919ae 100644 --- a/arm/gic.c +++ b/arm/gic.c @@ -215,20 +215,9 @@ static void ipi_test_smp(void) report_prefix_pop(); } -static void ipi_enable(void) -{ - gic_enable_defaults(); -#ifdef __arm__ - install_exception_handler(EXCPTN_IRQ, ipi_handler); -#else - install_irq_handler(EL1H_IRQ, ipi_handler); -#endif - local_irq_enable(); -} - static void ipi_send(void) { - ipi_enable(); + setup_irq(ipi_handler); wait_on_ready(); ipi_test_self(); ipi_test_smp(); @@ -238,7 +227,7 @@ static void ipi_send(void) static void ipi_recv(void) { - ipi_enable(); + setup_irq(ipi_handler); cpumask_set_cpu(smp_processor_id(), &ready); while (1) wfi(); @@ -295,14 +284,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused) static void run_active_clear_test(void) { report_prefix_push("active"); - gic_enable_defaults(); -#ifdef __arm__ - install_exception_handler(EXCPTN_IRQ, ipi_clear_active_handler); -#else - install_irq_handler(EL1H_IRQ, ipi_clear_active_handler); -#endif - local_irq_enable(); - + setup_irq(ipi_clear_active_handler); ipi_test_self(); report_prefix_pop(); } diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h index 21cdb58..55dd84b 100644 --- a/lib/arm/asm/gic.h +++ b/lib/arm/asm/gic.h @@ -82,5 +82,8 @@ void gic_set_irq_target(int irq, int cpu); void gic_set_irq_group(int irq, int group); int gic_get_irq_group(int irq); +typedef void (*handler_t)(struct pt_regs *regs __unused); +extern void setup_irq(handler_t handler); + #endif /* !__ASSEMBLY__ */ #endif /* _ASMARM_GIC_H_ */ diff --git a/lib/arm/gic.c b/lib/arm/gic.c index aa9cb86..0c5511f 100644 --- a/lib/arm/gic.c +++ b/lib/arm/gic.c @@ -236,3 +236,14 @@ int gic_get_irq_group(int irq) { return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ); } + +void setup_irq(handler_t handler) +{ +gic_enable_defaults(); +#ifdef __arm__ +install_exception_handler(EXCPTN_IRQ, handler); +#else +install_irq_handler(EL1H_IRQ, handler); +#endif +local_irq_enable(); +} -- 2.20.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests RFC 02/10] pmu: Let pmu tests take a sub-test parameter
As we intend to introduce more PMU tests, let's add a sub-test parameter that will allow to categorize them. Existing tests are in the cycle-counter category. Signed-off-by: Eric Auger --- arm/pmu.c | 22 ++ arm/unittests.cfg | 7 --- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/arm/pmu.c b/arm/pmu.c index 1de7d77..2ad6469 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -287,21 +287,27 @@ int main(int argc, char *argv[]) { int cpi = 0; - if (argc > 1) - cpi = atol(argv[1]); - if (!pmu_probe()) { printf("No PMU found, test skipped...\n"); return report_summary(); } + if (argc < 2) + report_abort("no test specified"); + report_prefix_push("pmu"); - report("Control register", check_pmcr()); - report("Monotonically increasing cycle count", check_cycles_increase()); - report("Cycle/instruction ratio", check_cpi(cpi)); - - pmccntr64_test(); + if (strcmp(argv[1], "cycle-counter") == 0) { + report_prefix_push(argv[1]); + if (argc > 2) + cpi = atol(argv[2]); + report("Control register", check_pmcr()); + report("Monotonically increasing cycle count", check_cycles_increase()); + report("Cycle/instruction ratio", check_cpi(cpi)); + pmccntr64_test(); + } else { + report_abort("Unknown subtest '%s'", argv[1]); + } return report_summary(); } diff --git a/arm/unittests.cfg b/arm/unittests.cfg index daeb5a0..79f0d7a 100644 --- a/arm/unittests.cfg +++ b/arm/unittests.cfg @@ -61,21 +61,22 @@ file = pci-test.flat groups = pci # Test PMU support -[pmu] +[pmu-cycle-counter] file = pmu.flat groups = pmu +extra_params = -append 'cycle-counter 0' # Test PMU support (TCG) with -icount IPC=1 #[pmu-tcg-icount-1] #file = pmu.flat -#extra_params = -icount 0 -append '1' +#extra_params = -icount 0 -append 'cycle-counter 1' #groups = pmu #accel = tcg # Test PMU support (TCG) with -icount IPC=256 #[pmu-tcg-icount-256] #file = pmu.flat -#extra_params = -icount 8 -append '256' +#extra_params = -icount 8 -append 'cycle-counter 256' #groups = pmu #accel = tcg -- 2.20.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests RFC 07/10] arm: pmu: test 32-bit <-> 64-bit transitions
--- arm/pmu.c | 125 +- arm/unittests.cfg | 6 +++ 2 files changed, 130 insertions(+), 1 deletion(-) diff --git a/arm/pmu.c b/arm/pmu.c index e185809..47d46a2 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -116,6 +116,7 @@ static void test_basic_event_count(void) {} static void test_mem_access(void) {} static void test_chained_counters(void) {} static void test_chained_sw_incr(void) {} +static void test_chain_promotion(void) {} #elif defined(__aarch64__) #define ID_AA64DFR0_PERFMON_SHIFT 8 @@ -262,7 +263,6 @@ asm volatile( : ); } - static void pmu_reset(void) { /* reset all counters, counting disabled at PMCR level*/ @@ -571,6 +571,126 @@ static void test_chained_sw_incr(void) read_regn(pmevcntr, 0), read_regn(pmevcntr, 1)); } +static void test_chain_promotion(void) +{ + uint32_t events[] = { 0x13 /* MEM_ACCESS */, 0x1E /* CHAIN */}; + void *addr = malloc(PAGE_SIZE); + + if (!satisfy_prerequisites(events, ARRAY_SIZE(events))) + return; + + /* Only enable CHAIN counter */ + pmu_reset(); +write_regn(pmevtyper, 0, events[0] | PMEVTYPER_EXCLUDE_EL0); +write_regn(pmevtyper, 1, events[1] | PMEVTYPER_EXCLUDE_EL0); + write_sysreg_s(0x2, PMCNTENSET_EL0); + isb(); + + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E); + report("chain counter not counting if even counter is disabled", + !read_regn(pmevcntr, 0)); + + /* Only enable even counter */ + pmu_reset(); + write_regn(pmevcntr, 0, 0xFFF0); + write_sysreg_s(0x1, PMCNTENSET_EL0); + isb(); + + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E); + report("odd counter did not increment on overflow if disabled", + !read_regn(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1)); + report_info("MEM_ACCESS counter #0 has value %ld", read_regn(pmevcntr, 0)); + report_info("CHAIN counter #1 has value %ld", read_regn(pmevcntr, 1)); + report_info("overflow counter %ld", read_sysreg(pmovsclr_el0)); + + /* start at 0xFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */ + pmu_reset(); + write_sysreg_s(0x3, PMCNTENSET_EL0); + write_regn(pmevcntr, 0, 0xFFDC); + isb(); + + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E); + report_info("MEM_ACCESS counter #0 has value 0x%lx", read_regn(pmevcntr, 0)); + + /* disable the CHAIN event */ + write_sysreg_s(0x2, PMCNTENCLR_EL0); + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E); + report_info("MEM_ACCESS counter #0 has value 0x%lx", read_regn(pmevcntr, 0)); + report("should have triggered an overflow on #0", read_sysreg(pmovsclr_el0) == 0x1); + report("CHAIN counter #1 shouldn't have incremented", !read_regn(pmevcntr, 1)); + + /* start at 0xFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */ + + pmu_reset(); + write_sysreg_s(0x1, PMCNTENSET_EL0); + write_regn(pmevcntr, 0, 0xFFDC); + isb(); + report_info("counter #0 = 0x%lx, counter #1 = 0x%lx overflow=0x%lx", + read_regn(pmevcntr, 0), read_regn(pmevcntr, 1), + read_sysreg(pmovsclr_el0)); + + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E); + report_info("MEM_ACCESS counter #0 has value 0x%lx", read_regn(pmevcntr, 0)); + + /* enable the CHAIN event */ + write_sysreg_s(0x3, PMCNTENSET_EL0); + isb(); + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E); + report_info("MEM_ACCESS counter #0 has value 0x%lx", read_regn(pmevcntr, 0)); + + report("CHAIN counter #1 should have incremented and no overflow expected", + (read_regn(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0)); + + report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx", + read_regn(pmevcntr, 1), read_sysreg(pmovsclr_el0)); + + /* start as MEM_ACCESS/CPU_CYCLES and move to CHAIN/MEM_ACCESS */ + pmu_reset(); +write_regn(pmevtyper, 0, 0x13 /* MEM_ACCESS */ | PMEVTYPER_EXCLUDE_EL0); +write_regn(pmevtyper, 1, 0x11 /* CPU_CYCLES */ | PMEVTYPER_EXCLUDE_EL0); + write_sysreg_s(0x3, PMCNTENSET_EL0); + write_regn(pmevcntr, 0, 0xFFDC); + isb(); + + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E); + report_info("MEM_ACCESS counter #0 has value 0x%lx", read_regn(pmevcntr, 0)); + + /* 0 becomes CHAINED */ + write_sysreg_s(0x0, PMCNTENSET_EL0); +write_regn(pmevtyper, 1, 0x1E /* CHAIN */ | PMEVTYPER_EXCLUDE_EL0); + write_sysreg_s(0x3, PMCNTENSET_EL0); + + mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E); + report_info("MEM_ACCESS counter #0 has value 0x%lx", read_regn(pmevcntr, 0)); + + report("CHAIN counter #1 should have incremented and no overflow expected", +
[kvm-unit-tests RFC 04/10] pmu: Check Required Event Support
If event counters are implemented check the common events required by the PMUv3 are implemented. Some are unconditionally required (SW_INCR, CPU_CYCLES, either INST_RETIRED or INST_SPEC). Some others only are required if the implementation implements some other features. Check those wich are unconditionally required. This test currently fails on TCG as neither INST_RETIRED or INST_SPEC are supported. Signed-off-by: Eric Auger --- arm/pmu.c | 70 +++ arm/unittests.cfg | 6 2 files changed, 76 insertions(+) diff --git a/arm/pmu.c b/arm/pmu.c index 8e95251..f78c43f 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -102,6 +102,10 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr) : [pmcr] "r" (pmcr), [z] "r" (0) : "cc"); } + +/* event counter tests only implemented for aarch64 */ +static void test_event_introspection(void) {} + #elif defined(__aarch64__) #define ID_AA64DFR0_PERFMON_SHIFT 8 #define ID_AA64DFR0_PERFMON_MASK 0xf @@ -140,6 +144,69 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr) : [pmcr] "r" (pmcr) : "cc"); } + +#define PMCEID1_EL0 sys_reg(11, 3, 9, 12, 7) + +static bool is_event_supported(uint32_t n, bool warn) +{ + uint64_t pmceid0 = read_sysreg(pmceid0_el0); + uint64_t pmceid1 = read_sysreg_s(PMCEID1_EL0); + bool supported; + uint32_t reg; + + if (n >= 0x0 && n <= 0x1F) { + reg = pmceid0 & 0x; + } else if (n >= 0x4000 && n <= 0x401F) { + reg = pmceid0 >> 32; + } else if (n >= 0x20 && n <= 0x3F) { + reg = pmceid1 & 0x; + } else if (n >= 0x4020 && n <= 0x403F) { + reg = pmceid1 >> 32; + } else { + abort(); + } + supported = reg & (1 << n); + if (!supported && warn) + report_info("event %d is not supported", n); + return supported; +} + +static void test_event_introspection(void) +{ + bool required_events; + + if (!pmu.nb_implemented_counters) { + report_skip("No event counter, skip ..."); + return; + } + if (pmu.nb_implemented_counters < 2) + report_info("%d event counters are implemented. " +"ARM recommends to implement at least 2", +pmu.nb_implemented_counters); + + /* PMUv3 requires an implementation includes some common events */ + required_events = is_event_supported(0x0, true) /* SW_INCR */ && + is_event_supported(0x11, true) /* CPU_CYCLES */ && + (is_event_supported(0x8, true) /* INST_RETIRED */ || + is_event_supported(0x1B, true) /* INST_PREC */); + if (!is_event_supported(0x8, false)) + report_info("ARM strongly recomments INST_RETIRED (0x8) event " + "to be implemented"); + + if (pmu.version == 0x4) { + /* ARMv8.1 PMU: STALL_FRONTEND and STALL_BACKEND are required */ + required_events = required_events || + is_event_supported(0x23, true) || + is_event_supported(0x24, true); + } + + /* L1D_CACHE_REFILL(0x3) and L1D_CACHE(0x4) are only required if + L1 data / unified cache. BR_MIS_PRED(0x10), BR_PRED(0x12) are only + required if program-flow prediction is implemented. */ + + report("Check required events are implemented", required_events); +} + #endif /* @@ -324,6 +391,9 @@ int main(int argc, char *argv[]) report("Monotonically increasing cycle count", check_cycles_increase()); report("Cycle/instruction ratio", check_cpi(cpi)); pmccntr64_test(); + } else if (strcmp(argv[1], "event-introspection") == 0) { + report_prefix_push(argv[1]); + test_event_introspection(); } else { report_abort("Unknown subtest '%s'", argv[1]); } diff --git a/arm/unittests.cfg b/arm/unittests.cfg index 79f0d7a..4433ef3 100644 --- a/arm/unittests.cfg +++ b/arm/unittests.cfg @@ -66,6 +66,12 @@ file = pmu.flat groups = pmu extra_params = -append 'cycle-counter 0' +[pmu-event-introspection] +file = pmu.flat +groups = pmu +arch = arm64 +extra_params = -append 'event-introspection' + # Test PMU support (TCG) with -icount IPC=1 #[pmu-tcg-icount-1] #file = pmu.flat -- 2.20.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests RFC 05/10] pmu: Basic event counter Tests
Adds the following tests: - event-counter-config: test event counter configuration - basic-event-count: - programs counters #0 and #1 to count 2 required events (resp. CPU_CYCLES and INST_RETIRED). Counter #0 is preset to a value close enough to the 32b overflow limit so that we check the overflow bit is set after the execution of the asm loop. - mem-access: counts MEM_ACCESS event on counters #0 and #1 with and without 32-bit overflow. Signed-off-by: Eric Auger --- arm/pmu.c | 254 ++ arm/unittests.cfg | 18 2 files changed, 272 insertions(+) diff --git a/arm/pmu.c b/arm/pmu.c index f78c43f..8ffeb93 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -18,9 +18,15 @@ #include "asm/barrier.h" #include "asm/sysreg.h" #include "asm/processor.h" +#include +#include #define PMU_PMCR_E (1 << 0) +#define PMU_PMCR_P (1 << 1) #define PMU_PMCR_C (1 << 2) +#define PMU_PMCR_D (1 << 3) +#define PMU_PMCR_X (1 << 4) +#define PMU_PMCR_DP(1 << 5) #define PMU_PMCR_LC(1 << 6) #define PMU_PMCR_N_SHIFT 11 #define PMU_PMCR_N_MASK0x1f @@ -105,6 +111,9 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr) /* event counter tests only implemented for aarch64 */ static void test_event_introspection(void) {} +static void test_event_counter_config(void) {} +static void test_basic_event_count(void) {} +static void test_mem_access(void) {} #elif defined(__aarch64__) #define ID_AA64DFR0_PERFMON_SHIFT 8 @@ -146,6 +155,32 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr) } #define PMCEID1_EL0 sys_reg(11, 3, 9, 12, 7) +#define PMCNTENSET_EL0 sys_reg(11, 3, 9, 12, 1) +#define PMCNTENCLR_EL0 sys_reg(11, 3, 9, 12, 2) + +#define PMEVTYPER_EXCLUDE_EL1 (1 << 31) +#define PMEVTYPER_EXCLUDE_EL0 (1 << 30) + +#define regn_el0(__reg, __n) __reg ## __n ## _el0 +#define write_regn(__reg, __n, __val) \ + write_sysreg((__val), __reg ## __n ## _el0) + +#define read_regn(__reg, __n) \ + read_sysreg(__reg ## __n ## _el0) + +#define print_pmevtyper(__s , __n) do { \ + uint32_t val; \ + val = read_regn(pmevtyper, __n);\ + report_info("%s pmevtyper%d=0x%x, eventcount=0x%x (p=%ld, u=%ld nsk=%ld, nsu=%ld, nsh=%ld m=%ld, mt=%ld)", \ + (__s), (__n), val, val & 0x, \ + (BIT_MASK(31) & val) >> 31, \ + (BIT_MASK(30) & val) >> 30, \ + (BIT_MASK(29) & val) >> 29, \ + (BIT_MASK(28) & val) >> 28, \ + (BIT_MASK(27) & val) >> 27, \ + (BIT_MASK(26) & val) >> 26, \ + (BIT_MASK(25) & val) >> 25); \ + } while (0) static bool is_event_supported(uint32_t n, bool warn) { @@ -207,6 +242,216 @@ static void test_event_introspection(void) report("Check required events are implemented", required_events); } +static inline void mem_access_loop(void *addr, int loop, uint32_t pmcr) +{ +asm volatile( + " msr pmcr_el0, %[pmcr]\n" + " isb\n" + " mov x10, %[loop]\n" + "1: sub x10, x10, #1\n" + " mov x8, %[addr]\n" + " ldr x9, [x8]\n" + " cmp x10, #0x0\n" + " b.gt1b\n" + " msr pmcr_el0, xzr\n" + " isb\n" + : + : [addr] "r" (addr), [pmcr] "r" (pmcr), [loop] "r" (loop) + : ); +} + + +static void pmu_reset(void) +{ + /* reset all counters, counting disabled at PMCR level*/ + set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P); + /* Disable all counters */ + write_sysreg_s(0x, PMCNTENCLR_EL0); + /* clear overflow reg */ + write_sysreg(0x, pmovsclr_el0); + /* disable overflow interrupts on all counters */ + write_sysreg(0x, pmintenclr_el1); + isb(); +} + +static void test_event_counter_config(void) { + int i; + + if (!pmu.nb_implemented_counters) { + report_skip("No event counter, skip ..."); + return; + } + + pmu_reset(); + + /* Test setting through PMESELR/PMXEVTYPER and PMEVTYPERn read */ +/* select counter 0 */ +write_sysreg(1, PMSELR_EL0); +/* program this counter to count unsupported event */ +write_sysreg(0xEA, PMXEVTYPER_EL0); +write_sysreg(0xdeadbeef, PMXEVCNTR_EL0); + report("PMESELR/PMXEVTYPER/PMEVTYPERn", + (read_regn(pmevtyper, 1) & 0xFFF) == 0xEA); + report("PMESELR/PMXEVCNTR/PMEVCNTRn", + (read_regn(pmevcntr, 1) == 0xdeadbeef)); + + /* try configure an unsupported event within the range [0x0, 0x3F] */ + for (i = 0; i <= 0x3F; i++) { + if (!is_event_supported(i, false)) + goto test_unsupported; + } + report_skip("pmevtyper: all events withi
[kvm-unit-tests RFC 01/10] arm64: Provide read/write_sysreg_s
From: Andrew Jones Sometimes we need to test access to system registers which are missing assembler mnemonics. Signed-off-by: Andrew Jones --- lib/arm64/asm/sysreg.h | 11 +++ 1 file changed, 11 insertions(+) diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h index a03830b..a45eebd 100644 --- a/lib/arm64/asm/sysreg.h +++ b/lib/arm64/asm/sysreg.h @@ -38,6 +38,17 @@ asm volatile("msr " xstr(r) ", %x0" : : "rZ" (__val)); \ } while (0) +#define read_sysreg_s(r) ({\ + u64 __val; \ + asm volatile("mrs_s %0, " xstr(r) : "=r" (__val)); \ + __val; \ +}) + +#define write_sysreg_s(v, r) do { \ + u64 __val = (u64)v; \ + asm volatile("msr_s " xstr(r) ", %x0" : : "rZ" (__val));\ +} while (0) + asm( " .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n" " .equ.L__reg_num_x\\num, \\num\n" -- 2.20.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests RFC 00/10] KVM: arm64: PMUv3 Event Counter Tests
This series implements tests exercising the PMUv3 event counters. It tests both the 32-bit and 64-bit versions. Overflow interrupts also are checked. Those tests only are written for arm64. It allowed to reveal some issues related to SW_INCR implementation (esp. related to 64-bit implementation), some problems related to 32-bit <-> 64-bit transitions and consistency of enabled states of odd and event counters. Overflow interrupt testing relies of one patch from Andre ("arm: gic: Provide per-IRQ helper functions") to enable the PPI 23, coming from "arm: gic: Test SPIs and interrupt groups" (https://patchwork.kernel.org/cover/11234975/). Drew kindly provided "arm64: Provide read/write_sysreg_s". All PMU tests can be launched with: ./run_tests.sh -g pmu Tests also can be launched individually. For example: ./arm-run arm/pmu.flat -append 'chained-sw-incr' With KVM: - chain-promotion and chained-sw-incr are known to be failing. - Problems were reported upstream. With TCG: - pmu-event-introspection is failing due to missing required events (we may remove this from TCG actually) - chained-sw-incr also fails. I haven't investigated yet. Andre Przywara (1): arm: gic: Provide per-IRQ helper functions Andrew Jones (1): arm64: Provide read/write_sysreg_s Eric Auger (8): pmu: Let pmu tests take a sub-test parameter pmu: Add a pmu struct pmu: Check Required Event Support pmu: Basic event counter Tests pmu: Test chained counter arm: pmu: test 32-bit <-> 64-bit transitions arm/arm64: gic: Introduce setup_irq() helper pmu: Test overflow interrupts arm/gic.c | 24 +- arm/pmu.c | 754 - arm/unittests.cfg | 55 ++- lib/arm/asm/gic-v3.h | 2 + lib/arm/asm/gic.h | 12 + lib/arm/gic.c | 101 ++ lib/arm64/asm/sysreg.h | 11 + 7 files changed, 922 insertions(+), 37 deletions(-) -- 2.20.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests RFC 03/10] pmu: Add a pmu struct
This struct aims at storing information potentially used by all tests such as the pmu version, the read-only part of the PMCR, the number of implemented event counters, ... Signed-off-by: Eric Auger --- arm/pmu.c | 29 - 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/arm/pmu.c b/arm/pmu.c index 2ad6469..8e95251 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -33,7 +33,15 @@ #define NR_SAMPLES 10 -static unsigned int pmu_version; +struct pmu { + unsigned int version; + unsigned int nb_implemented_counters; + uint32_t pmcr_ro; +}; + +static struct pmu pmu; + + #if defined(__arm__) #define ID_DFR0_PERFMON_SHIFT 24 #define ID_DFR0_PERFMON_MASK 0xf @@ -265,7 +273,7 @@ static bool check_cpi(int cpi) static void pmccntr64_test(void) { #ifdef __arm__ - if (pmu_version == 0x3) { + if (pmu.version == 0x3) { if (ERRATA(9e3f7a296940)) { write_sysreg(0xdead, PMCCNTR64); report("pmccntr64", read_sysreg(PMCCNTR64) == 0xdead); @@ -278,9 +286,20 @@ static void pmccntr64_test(void) /* Return FALSE if no PMU found, otherwise return TRUE */ static bool pmu_probe(void) { - pmu_version = get_pmu_version(); - report_info("PMU version: %d", pmu_version); - return pmu_version != 0 && pmu_version != 0xf; + uint32_t pmcr; + + pmu.version = get_pmu_version(); + report_info("PMU version: %d", pmu.version); + + if (pmu.version == 0 || pmu.version == 0xF) + return false; + + pmcr = get_pmcr(); + pmu.pmcr_ro = pmcr & 0xFF80; + pmu.nb_implemented_counters = (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK; + report_info("Implements %d event counters", pmu.nb_implemented_counters); + + return true; } int main(int argc, char *argv[]) -- 2.20.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC 2/3] KVM: arm64: pmu: Fix chained SW_INCR counters
On Fri, Dec 06, 2019 at 03:35:03PM +, Marc Zyngier wrote: > On 2019-12-06 15:21, Andrew Murray wrote: > > On Thu, Dec 05, 2019 at 02:52:26PM +, Marc Zyngier wrote: > > > On 2019-12-05 14:06, Auger Eric wrote: > > > > Hi Marc, > > > > > > > > > > I think the whole function is a bit of a mess, and could be > > > better > > > > > structured to treat 64bit counters as a first class citizen. > > > > > > > > > > I'm suggesting something along those lines, which tries to > > > > > streamline things a bit and keep the flow uniform between the > > > > > two word sizes. IMHO, it helps reasonning about it and gives > > > > > scope to the ARMv8.5 full 64bit counters... It is of course > > > > > completely untested. > > > > > > > > Looks OK to me as well. One remark though, don't we need to test > > > if the > > > > n+1th reg is enabled before incrementing it? > > > > Indeed - we don't want to indicate an overflow on a disabled counter. > > > > > > > > > > Hmmm. I'm not sure. I think we should make sure that we don't flag > > > a counter as being chained if the odd counter is disabled, rather > > > than checking it here. As long as the odd counter is not chained > > > *and* enabled, we shouldn't touch it. > > > > Does this mean that we don't care if the low counter is enabled or not > > when deciding if the pair is chained? > > > > I would find the code easier to follow if we had an explicit 'is the > > high counter enabled here' check (at the point of deciding where to > > put the overflow). > > Sure. But the point is that we're spreading that kind of checks all over > the map, and that we don't have a way to even reason about the state of > a 64bit counter. Doesn't it strike you as being mildly broken? > Yup! To the point where I can't trust the function names and have to look at what the code does... > > > @@ -645,7 +647,8 @@ static void kvm_pmu_update_pmc_chained(struct > > > kvm_vcpu > > > *vcpu, u64 select_idx) > > > struct kvm_pmu *pmu = &vcpu->arch.pmu; > > > struct kvm_pmc *pmc = &pmu->pmc[select_idx]; > > > > > > - if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx)) { > > > + if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx) && > > > + kvm_pmu_counter_is_enabled(vcpu, pmc->idx)) { > > > > I.e. here we don't care what the state of enablement is for the low > > counter. > > > > Also at present, this may break the following use-case > > > > - User creates and uses a pair of chained counters > > - User disables odd/high counter > > - User reads values of both counters > > - User rewrites CHAIN event to odd/high counter OR user re-enables > > just the even/low counter > > - User reads value of both counters <- this may now different to the > > last read > > Hey, I didn't say it was perfect ;-). But for sure we can't let the > PMU bitrot more than it already has, and I'm not sure this is heading > the right way. I think we're aligned here. To me this code is becoming very fragile, difficult for me to make sense of and is stretching the abstractions we've made. This is why it is difficult to enhance it without breaking something. It's why I felt it was safer to add 'an extra check' in the SWINCR than to risk touching something that I didn't have the confidence I could be sure was correct. > > I'm certainly going to push back on new PMU features until we can properly > reason about 64bit counters as a top-level entity (as opposed to a bunch > of discrete counters). Thanks, Andrew Murray > > 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: [RFC 2/3] KVM: arm64: pmu: Fix chained SW_INCR counters
On Thu, Dec 05, 2019 at 08:01:42PM +0100, Auger Eric wrote: > Hi Marc, > > On 12/5/19 3:52 PM, Marc Zyngier wrote: > > On 2019-12-05 14:06, Auger Eric wrote: > >> Hi Marc, > >> > >> On 12/5/19 10:43 AM, Marc Zyngier wrote: > >>> Hi Eric, > >>> > >>> On 2019-12-04 20:44, Eric Auger wrote: > At the moment a SW_INCR counter always overflows on 32-bit > boundary, independently on whether the n+1th counter is > programmed as CHAIN. > > Check whether the SW_INCR counter is a 64b counter and if so, > implement the 64b logic. > > Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU counters") > Signed-off-by: Eric Auger > --- > virt/kvm/arm/pmu.c | 16 +++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > index c3f8b059881e..7ab477db2f75 100644 > --- a/virt/kvm/arm/pmu.c > +++ b/virt/kvm/arm/pmu.c > @@ -491,6 +491,8 @@ void kvm_pmu_software_increment(struct kvm_vcpu > *vcpu, u64 val) > > enable = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0); > for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) { > + bool chained = test_bit(i >> 1, vcpu->arch.pmu.chained); > + > >>> > >>> I'd rather you use kvm_pmu_pmc_is_chained() rather than open-coding > >>> this. But see below: > >>> > if (!(val & BIT(i))) > continue; > type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i) > @@ -500,8 +502,20 @@ void kvm_pmu_software_increment(struct kvm_vcpu > *vcpu, u64 val) > reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1; > reg = lower_32_bits(reg); > __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg; > - if (!reg) > + if (reg) /* no overflow */ > + continue; > + if (chained) { > + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) + 1; > + reg = lower_32_bits(reg); > + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) = reg; > + if (reg) > + continue; > + /* mark an overflow on high counter */ > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i + 1); > + } else { > + /* mark an overflow */ > __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i); > + } > } > } > } > >>> > >>> I think the whole function is a bit of a mess, and could be better > >>> structured to treat 64bit counters as a first class citizen. > >>> > >>> I'm suggesting something along those lines, which tries to > >>> streamline things a bit and keep the flow uniform between the > >>> two word sizes. IMHO, it helps reasonning about it and gives > >>> scope to the ARMv8.5 full 64bit counters... It is of course > >>> completely untested. > >> > >> Looks OK to me as well. One remark though, don't we need to test if the > >> n+1th reg is enabled before incrementing it? > > > > Hmmm. I'm not sure. I think we should make sure that we don't flag > > a counter as being chained if the odd counter is disabled, rather > > than checking it here. As long as the odd counter is not chained > > *and* enabled, we shouldn't touch it.> > > Again, untested: > > > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > > index cf371f643ade..47366817cd2a 100644 > > --- a/virt/kvm/arm/pmu.c > > +++ b/virt/kvm/arm/pmu.c > > @@ -15,6 +15,7 @@ > > #include > > > > static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 > > select_idx); > > +static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 > > select_idx); > > > > #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1 > > > > @@ -298,6 +299,7 @@ void kvm_pmu_enable_counter_mask(struct kvm_vcpu > > *vcpu, u64 val) > > * For high counters of chained events we must recreate the > > * perf event with the long (64bit) attribute set. > > */ > > + kvm_pmu_update_pmc_chained(vcpu, i); > > if (kvm_pmu_pmc_is_chained(pmc) && > > kvm_pmu_idx_is_high_counter(i)) { > > kvm_pmu_create_perf_event(vcpu, i); > > @@ -645,7 +647,8 @@ static void kvm_pmu_update_pmc_chained(struct > > kvm_vcpu *vcpu, u64 select_idx) > > struct kvm_pmu *pmu = &vcpu->arch.pmu; > > struct kvm_pmc *pmc = &pmu->pmc[select_idx]; > > > > - if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx)) { > > + if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx) && > > + kvm_pmu_counter_is_enabled(vcpu, pmc->idx)) { > > In create_perf_event(), has_chain_evtype() is used and a 64b sample > period would be chosen even if the counters are disjoined (since the odd > is disabled). We would need to use pmc_is_chained() instead. So in this use-case, someone has configured a pair of chained counters but only enable
Re: [RFC 2/3] KVM: arm64: pmu: Fix chained SW_INCR counters
On 2019-12-06 15:21, Andrew Murray wrote: On Thu, Dec 05, 2019 at 02:52:26PM +, Marc Zyngier wrote: On 2019-12-05 14:06, Auger Eric wrote: > Hi Marc, > > On 12/5/19 10:43 AM, Marc Zyngier wrote: > > Hi Eric, > > > > On 2019-12-04 20:44, Eric Auger wrote: > > > At the moment a SW_INCR counter always overflows on 32-bit > > > boundary, independently on whether the n+1th counter is > > > programmed as CHAIN. > > > > > > Check whether the SW_INCR counter is a 64b counter and if so, > > > implement the 64b logic. > > > > > > Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU > > > counters") > > > Signed-off-by: Eric Auger > > > --- > > > virt/kvm/arm/pmu.c | 16 +++- > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > > > index c3f8b059881e..7ab477db2f75 100644 > > > --- a/virt/kvm/arm/pmu.c > > > +++ b/virt/kvm/arm/pmu.c > > > @@ -491,6 +491,8 @@ void kvm_pmu_software_increment(struct kvm_vcpu > > > *vcpu, u64 val) > > > > > > enable = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0); > > > for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) { > > > + bool chained = test_bit(i >> 1, vcpu->arch.pmu.chained); > > > + > > > > I'd rather you use kvm_pmu_pmc_is_chained() rather than open-coding > > this. But see below: > > > > > if (!(val & BIT(i))) > > > continue; > > > type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i) > > > @@ -500,8 +502,20 @@ void kvm_pmu_software_increment(struct > > > kvm_vcpu > > > *vcpu, u64 val) > > > reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1; > > > reg = lower_32_bits(reg); > > > __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg; > > > - if (!reg) > > > + if (reg) /* no overflow */ > > > + continue; > > > + if (chained) { > > > + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + > > > 1) + 1; > > > + reg = lower_32_bits(reg); > > > + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) = reg; > > > + if (reg) > > > + continue; > > > + /* mark an overflow on high counter */ > > > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i + 1); > > > + } else { > > > + /* mark an overflow */ > > > __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i); > > > + } > > > } > > > } > > > } > > > > I think the whole function is a bit of a mess, and could be better > > structured to treat 64bit counters as a first class citizen. > > > > I'm suggesting something along those lines, which tries to > > streamline things a bit and keep the flow uniform between the > > two word sizes. IMHO, it helps reasonning about it and gives > > scope to the ARMv8.5 full 64bit counters... It is of course > > completely untested. > > Looks OK to me as well. One remark though, don't we need to test if the > n+1th reg is enabled before incrementing it? Indeed - we don't want to indicate an overflow on a disabled counter. Hmmm. I'm not sure. I think we should make sure that we don't flag a counter as being chained if the odd counter is disabled, rather than checking it here. As long as the odd counter is not chained *and* enabled, we shouldn't touch it. Does this mean that we don't care if the low counter is enabled or not when deciding if the pair is chained? I would find the code easier to follow if we had an explicit 'is the high counter enabled here' check (at the point of deciding where to put the overflow). Sure. But the point is that we're spreading that kind of checks all over the map, and that we don't have a way to even reason about the state of a 64bit counter. Doesn't it strike you as being mildly broken? Again, untested: diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c index cf371f643ade..47366817cd2a 100644 --- a/virt/kvm/arm/pmu.c +++ b/virt/kvm/arm/pmu.c @@ -15,6 +15,7 @@ #include static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx); +static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx); #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1 @@ -298,6 +299,7 @@ void kvm_pmu_enable_counter_mask(struct kvm_vcpu *vcpu, u64 val) * For high counters of chained events we must recreate the * perf event with the long (64bit) attribute set. */ + kvm_pmu_update_pmc_chained(vcpu, i); if (kvm_pmu_pmc_is_chained(pmc) && kvm_pmu_idx_is_high_counter(i)) { kvm_pmu_create_perf_event(vcpu, i); @@ -645,7 +647,8 @@ static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx) struct kvm_pmu *pmu = &vcpu->arch.pmu; struct kvm_pmc *pmc = &pmu->pmc[select_idx]; - if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx)) { + if
Re: [RFC 2/3] KVM: arm64: pmu: Fix chained SW_INCR counters
On Thu, Dec 05, 2019 at 02:52:26PM +, Marc Zyngier wrote: > On 2019-12-05 14:06, Auger Eric wrote: > > Hi Marc, > > > > On 12/5/19 10:43 AM, Marc Zyngier wrote: > > > Hi Eric, > > > > > > On 2019-12-04 20:44, Eric Auger wrote: > > > > At the moment a SW_INCR counter always overflows on 32-bit > > > > boundary, independently on whether the n+1th counter is > > > > programmed as CHAIN. > > > > > > > > Check whether the SW_INCR counter is a 64b counter and if so, > > > > implement the 64b logic. > > > > > > > > Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU > > > > counters") > > > > Signed-off-by: Eric Auger > > > > --- > > > > virt/kvm/arm/pmu.c | 16 +++- > > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > > > > index c3f8b059881e..7ab477db2f75 100644 > > > > --- a/virt/kvm/arm/pmu.c > > > > +++ b/virt/kvm/arm/pmu.c > > > > @@ -491,6 +491,8 @@ void kvm_pmu_software_increment(struct kvm_vcpu > > > > *vcpu, u64 val) > > > > > > > > enable = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0); > > > > for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) { > > > > + bool chained = test_bit(i >> 1, vcpu->arch.pmu.chained); > > > > + > > > > > > I'd rather you use kvm_pmu_pmc_is_chained() rather than open-coding > > > this. But see below: > > > > > > > if (!(val & BIT(i))) > > > > continue; > > > > type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i) > > > > @@ -500,8 +502,20 @@ void kvm_pmu_software_increment(struct > > > > kvm_vcpu > > > > *vcpu, u64 val) > > > > reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1; > > > > reg = lower_32_bits(reg); > > > > __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg; > > > > - if (!reg) > > > > + if (reg) /* no overflow */ > > > > + continue; > > > > + if (chained) { > > > > + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + > > > > 1) + 1; > > > > + reg = lower_32_bits(reg); > > > > + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) = reg; > > > > + if (reg) > > > > + continue; > > > > + /* mark an overflow on high counter */ > > > > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i + 1); > > > > + } else { > > > > + /* mark an overflow */ > > > > __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i); > > > > + } > > > > } > > > > } > > > > } > > > > > > I think the whole function is a bit of a mess, and could be better > > > structured to treat 64bit counters as a first class citizen. > > > > > > I'm suggesting something along those lines, which tries to > > > streamline things a bit and keep the flow uniform between the > > > two word sizes. IMHO, it helps reasonning about it and gives > > > scope to the ARMv8.5 full 64bit counters... It is of course > > > completely untested. > > > > Looks OK to me as well. One remark though, don't we need to test if the > > n+1th reg is enabled before incrementing it? Indeed - we don't want to indicate an overflow on a disabled counter. > > Hmmm. I'm not sure. I think we should make sure that we don't flag > a counter as being chained if the odd counter is disabled, rather > than checking it here. As long as the odd counter is not chained > *and* enabled, we shouldn't touch it. Does this mean that we don't care if the low counter is enabled or not when deciding if the pair is chained? I would find the code easier to follow if we had an explicit 'is the high counter enabled here' check (at the point of deciding where to put the overflow). > > Again, untested: > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > index cf371f643ade..47366817cd2a 100644 > --- a/virt/kvm/arm/pmu.c > +++ b/virt/kvm/arm/pmu.c > @@ -15,6 +15,7 @@ > #include > > static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 > select_idx); > +static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 > select_idx); > > #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1 > > @@ -298,6 +299,7 @@ void kvm_pmu_enable_counter_mask(struct kvm_vcpu *vcpu, > u64 val) >* For high counters of chained events we must recreate the >* perf event with the long (64bit) attribute set. >*/ > + kvm_pmu_update_pmc_chained(vcpu, i); > if (kvm_pmu_pmc_is_chained(pmc) && > kvm_pmu_idx_is_high_counter(i)) { > kvm_pmu_create_perf_event(vcpu, i); > @@ -645,7 +647,8 @@ static void kvm_pmu_update_pmc_chained(struct kvm_vcpu > *vcpu, u64 select_idx) > struct kvm_pmu *pmu = &vcpu->arch.pmu; > struct kvm_pmc *pmc = &pmu->pmc[select_idx]; > > - if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx)) { > + if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx) && > + kvm_pmu_c
Re: [PATCH v2 0/5] target/arm: More EL2 trapping fixes
On 2019-12-06 14:13, Peter Maydell wrote: On Sun, 1 Dec 2019 at 12:20, Marc Zyngier wrote: Hi all, This series is a follow-up on [1], which tried to address the remaining missing HCR_EL2.TIDx traps. I've hopefully now adressed the comments that Peter and Edgar raised. I've also tried to tackle missing traps generated by HSTR_EL2, which got completely ignored so far. Note that this results in the use of a new TB bit, which I understand is a rare resource. I'd welcome comments on how to handle it differently if at all possible. Finally, and as a bonus non-feature, I've added support for the missing Jazelle registers, giving me the opportunity to allow trapping of JIDR to EL2 using HCR_EL2.TID0. Yay, Christmas! ;-) I'm now going back to kernel stuff. I swear! To save you from having to roll a v3, I've fixed up the handful of nits Richard and I found as I applied this series to target-arm.next. Ah, brilliant. Thanks a lot Peter. 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 v2 0/5] target/arm: More EL2 trapping fixes
On Sun, 1 Dec 2019 at 12:20, Marc Zyngier wrote: > > Hi all, > > This series is a follow-up on [1], which tried to address the > remaining missing HCR_EL2.TIDx traps. I've hopefully now adressed the > comments that Peter and Edgar raised. > > I've also tried to tackle missing traps generated by HSTR_EL2, which > got completely ignored so far. Note that this results in the use of a > new TB bit, which I understand is a rare resource. I'd welcome > comments on how to handle it differently if at all possible. > > Finally, and as a bonus non-feature, I've added support for the > missing Jazelle registers, giving me the opportunity to allow trapping > of JIDR to EL2 using HCR_EL2.TID0. Yay, Christmas! ;-) > > I'm now going back to kernel stuff. I swear! To save you from having to roll a v3, I've fixed up the handful of nits Richard and I found as I applied this series to target-arm.next. thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 3/5] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions
On 2019-12-06 14:08, Peter Maydell wrote: On Sun, 1 Dec 2019 at 12:20, Marc Zyngier wrote: HCR_EL2.TID3 requires that AArch32 reads of MVFR[012] are trapped to EL2, and HCR_EL2.TID0 does the same for reads of FPSID. In order to handle this, introduce a new TCG helper function that checks for these control bits before executing the VMRC instruction. Tested with a hacked-up version of KVM/arm64 that sets the control bits for 32bit guests. Reviewed-by: Edgar E. Iglesias Signed-off-by: Marc Zyngier --- target/arm/helper-a64.h| 2 ++ target/arm/translate-vfp.inc.c | 18 +++--- target/arm/vfp_helper.c| 29 + 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h index a915c1247f..0af44dc814 100644 --- a/target/arm/helper-a64.h +++ b/target/arm/helper-a64.h @@ -102,3 +102,5 @@ DEF_HELPER_FLAGS_3(autda, TCG_CALL_NO_WG, i64, env, i64, i64) DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64) DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64) DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64) + +DEF_HELPER_3(check_hcr_el2_trap, void, env, i32, i32) This has to be in helper.h, not helper-a64.h, otherwise the arm-softmmu target won't build. helper-a64.h is for helper functions which only exist in the aarch64 binary. Ah, fair enough. I guess I should build all targets rather than limit myself to aarch64... I'll fix that and repost the series, having hopefully addressed Richard's comments. 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 v2 3/5] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions
On Sun, 1 Dec 2019 at 12:20, Marc Zyngier wrote: > > HCR_EL2.TID3 requires that AArch32 reads of MVFR[012] are trapped to > EL2, and HCR_EL2.TID0 does the same for reads of FPSID. > In order to handle this, introduce a new TCG helper function that > checks for these control bits before executing the VMRC instruction. > > Tested with a hacked-up version of KVM/arm64 that sets the control > bits for 32bit guests. > > Reviewed-by: Edgar E. Iglesias > Signed-off-by: Marc Zyngier > --- > target/arm/helper-a64.h| 2 ++ > target/arm/translate-vfp.inc.c | 18 +++--- > target/arm/vfp_helper.c| 29 + > 3 files changed, 46 insertions(+), 3 deletions(-) > > diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h > index a915c1247f..0af44dc814 100644 > --- a/target/arm/helper-a64.h > +++ b/target/arm/helper-a64.h > @@ -102,3 +102,5 @@ DEF_HELPER_FLAGS_3(autda, TCG_CALL_NO_WG, i64, env, i64, > i64) > DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64) > DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64) > DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64) > + > +DEF_HELPER_3(check_hcr_el2_trap, void, env, i32, i32) This has to be in helper.h, not helper-a64.h, otherwise the arm-softmmu target won't build. helper-a64.h is for helper functions which only exist in the aarch64 binary. thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 5/5] target/arm: Add support for missing Jazelle system registers
On Mon, 2 Dec 2019 at 15:58, Richard Henderson wrote: > > On 12/1/19 12:20 PM, Marc Zyngier wrote: > > +if (cpu_isar_feature(jazelle, cpu)) { > > +ARMCPRegInfo jazelle_regs[] = { > > static const. If this can be static const we should just declare it at file scope. The only arrays we put inline in this function are the ones which need some non-const fields. thanks -- PMM ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] arm64: KVM: Invoke compute_layout() before alternatives are applied
On Fri, Dec 06, 2019 at 12:12:10PM +, Marc Zyngier wrote: > On 2019-12-06 11:57, Catalin Marinas wrote: > > On Thu, Nov 28, 2019 at 08:58:05PM +0100, Sebastian Andrzej Siewior > > wrote: > > > @@ -408,6 +410,8 @@ static void __init hyp_mode_check(void) > > > "CPU: CPUs started in inconsistent modes"); > > > else > > > pr_info("CPU: All CPU(s) started at EL1\n"); > > > + if (IS_ENABLED(CONFIG_KVM_ARM_HOST)) > > > + kvm_compute_layout(); > > > } > > > > It looks like we call this unconditionally here even if the kernel was > > booted at EL1. > > It has always been the case. My motivation was to be able to debug > this in a guest, because doing this on the host is... painful! ;-) > > Feel free to condition it on !EL1 though. I'll leave the patch as is. Thanks for confirming. -- Catalin ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] arm64: KVM: Invoke compute_layout() before alternatives are applied
On 2019-12-06 11:57, Catalin Marinas wrote: On Thu, Nov 28, 2019 at 08:58:05PM +0100, Sebastian Andrzej Siewior wrote: @@ -408,6 +410,8 @@ static void __init hyp_mode_check(void) "CPU: CPUs started in inconsistent modes"); else pr_info("CPU: All CPU(s) started at EL1\n"); + if (IS_ENABLED(CONFIG_KVM_ARM_HOST)) + kvm_compute_layout(); } It looks like we call this unconditionally here even if the kernel was booted at EL1. It has always been the case. My motivation was to be able to debug this in a guest, because doing this on the host is... painful! ;-) Feel free to condition it on !EL1 though. void __init smp_cpus_done(unsigned int max_cpus) diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c index 2cf7d4b606c38..dab1fea4752aa 100644 --- a/arch/arm64/kvm/va_layout.c +++ b/arch/arm64/kvm/va_layout.c @@ -22,7 +22,7 @@ static u8 tag_lsb; static u64 tag_val; static u64 va_mask; -static void compute_layout(void) +__init void kvm_compute_layout(void) { phys_addr_t idmap_addr = __pa_symbol(__hyp_idmap_text_start); u64 hyp_va_msb; @@ -110,9 +110,6 @@ void __init kvm_update_va_mask(struct alt_instr *alt, BUG_ON(nr_inst != 5); - if (!has_vhe() && !va_mask) - compute_layout(); - for (i = 0; i < nr_inst; i++) { u32 rd, rn, insn, oinsn; @@ -156,9 +153,6 @@ void kvm_patch_vector_branch(struct alt_instr *alt, return; } - if (!va_mask) - compute_layout(); And here we had a few more checks. Maybe it's still correct but asking anyway. It should be correct. These checks were there to ensure that we only computed the layout once, and this now happens by construction (calling compute_layout from a single location instead of doing it from the patch callbacks). 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 v2] arm64: KVM: Invoke compute_layout() before alternatives are applied
On Thu, Nov 28, 2019 at 08:58:05PM +0100, Sebastian Andrzej Siewior wrote: > @@ -408,6 +410,8 @@ static void __init hyp_mode_check(void) > "CPU: CPUs started in inconsistent modes"); > else > pr_info("CPU: All CPU(s) started at EL1\n"); > + if (IS_ENABLED(CONFIG_KVM_ARM_HOST)) > + kvm_compute_layout(); > } It looks like we call this unconditionally here even if the kernel was booted at EL1. > void __init smp_cpus_done(unsigned int max_cpus) > diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c > index 2cf7d4b606c38..dab1fea4752aa 100644 > --- a/arch/arm64/kvm/va_layout.c > +++ b/arch/arm64/kvm/va_layout.c > @@ -22,7 +22,7 @@ static u8 tag_lsb; > static u64 tag_val; > static u64 va_mask; > > -static void compute_layout(void) > +__init void kvm_compute_layout(void) > { > phys_addr_t idmap_addr = __pa_symbol(__hyp_idmap_text_start); > u64 hyp_va_msb; > @@ -110,9 +110,6 @@ void __init kvm_update_va_mask(struct alt_instr *alt, > > BUG_ON(nr_inst != 5); > > - if (!has_vhe() && !va_mask) > - compute_layout(); > - > for (i = 0; i < nr_inst; i++) { > u32 rd, rn, insn, oinsn; > > @@ -156,9 +153,6 @@ void kvm_patch_vector_branch(struct alt_instr *alt, > return; > } > > - if (!va_mask) > - compute_layout(); And here we had a few more checks. Maybe it's still correct but asking anyway. -- Catalin ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: vgic: Fix potential double free dist->spis in __kvm_vgic_destroy()
On 2019-11-28 06:38, linmiaohe wrote: From: Miaohe Lin In kvm_vgic_dist_init() called from kvm_vgic_map_resources(), if dist->vgic_model is invalid, dist->spis will be freed without set dist->spis = NULL. And in vgicv2 resources clean up path, __kvm_vgic_destroy() will be called to free allocated resources. And dist->spis will be freed again in clean up chain because we forget to set dist->spis = NULL in kvm_vgic_dist_init() failed path. So double free would happen. Signed-off-by: Miaohe Lin --- virt/kvm/arm/vgic/vgic-init.c | 1 + 1 file changed, 1 insertion(+) diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c index 53e3969dfb52..c17c29beeb72 100644 --- a/virt/kvm/arm/vgic/vgic-init.c +++ b/virt/kvm/arm/vgic/vgic-init.c @@ -171,6 +171,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis) break; default: kfree(dist->spis); + dist->spis = NULL; return -EINVAL; } } Applied, thanks. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3] KVM: vgic: Use wrapper function to lock/unlock all vcpus in kvm_vgic_create()
On 2019-11-30 02:45, linmiaohe wrote: From: Miaohe Lin Use wrapper function lock_all_vcpus()/unlock_all_vcpus() in kvm_vgic_create() to remove duplicated code dealing with locking and unlocking all vcpus in a vm. Reviewed-by: Eric Auger Reviewed-by: Steven Price Signed-off-by: Miaohe Lin --- -v2: Fix some spelling mistake in patch title and commit log. -v3: Remove the comment that no longer makes sense. --- virt/kvm/arm/vgic/vgic-init.c | 19 --- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c index b3c5de48064c..22ff73ecac80 100644 --- a/virt/kvm/arm/vgic/vgic-init.c +++ b/virt/kvm/arm/vgic/vgic-init.c @@ -70,7 +70,7 @@ void kvm_vgic_early_init(struct kvm *kvm) */ int kvm_vgic_create(struct kvm *kvm, u32 type) { - int i, vcpu_lock_idx = -1, ret; + int i, ret; struct kvm_vcpu *vcpu; if (irqchip_in_kernel(kvm)) @@ -86,17 +86,9 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) !kvm_vgic_global_state.can_emulate_gicv2) return -ENODEV; - /* - * Any time a vcpu is run, vcpu_load is called which tries to grab the -* vcpu->mutex. By grabbing the vcpu->mutex of all VCPUs we ensure -* that no other VCPUs are run while we create the vgic. -*/ ret = -EBUSY; - kvm_for_each_vcpu(i, vcpu, kvm) { - if (!mutex_trylock(&vcpu->mutex)) - goto out_unlock; - vcpu_lock_idx = i; - } + if (!lock_all_vcpus(kvm)) + return ret; kvm_for_each_vcpu(i, vcpu, kvm) { if (vcpu->arch.has_run_once) @@ -125,10 +117,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) INIT_LIST_HEAD(&kvm->arch.vgic.rd_regions); out_unlock: - for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) { - vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx); - mutex_unlock(&vcpu->mutex); - } + unlock_all_vcpus(kvm); return ret; } Applied, thanks. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] arm64: KVM: Invoke compute_layout() before alternatives are applied
On Fri, Dec 06, 2019 at 11:22:02AM +, Marc Zyngier wrote: > On 2019-11-28 19:58, Sebastian Andrzej Siewior wrote: > > compute_layout() is invoked as part of an alternative fixup under > > stop_machine(). This function invokes get_random_long() which acquires a > > sleeping lock on -RT which can not be acquired in this context. > > > > Rename compute_layout() to kvm_compute_layout() and invoke it before > > stop_machine() applies the alternatives. Add a __init prefix to > > kvm_compute_layout() because the caller has it, too (and so the code can > > be > > discarded after boot). > > > > Signed-off-by: Sebastian Andrzej Siewior > > Acked-by: Marc Zyngier > > I think this should go via the arm64 tree, so I'll let Catalin > and Will pick this up (unless they think otherwise). I can pick this up. Thanks. -- Catalin ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 0/2] kvm/arm64: unimplemented sysreg improvements
On 2019-12-05 18:06, Mark Rutland wrote: While testing some other patches, I realised that KVM's logging of trapped sysreg accesses can log inconsistent information, and this is arguably unnecessary for IMPLEMENTATION DEFINED system registers. This patches fix that up, ensureing that logged information is consistent, and avoiding logging for IMPLEMENTATION DEFINED registers. Mark. Mark Rutland (2): kvm/arm64: sanely ratelimit sysreg messages kvm/arm64: don't log IMP DEF sysreg traps arch/arm64/kvm/sys_regs.c | 20 ++-- arch/arm64/kvm/sys_regs.h | 17 +++-- 2 files changed, 29 insertions(+), 8 deletions(-) Applied, thanks. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm: remove excessive permission check in kvm_arch_prepare_memory_region
On 2019-12-06 02:08, Jia He wrote: In kvm_arch_prepare_memory_region, arm kvm regards the memory region as writable if the flag has no KVM_MEM_READONLY, and the vm is readonly if !VM_WRITE. But there is common usage for setting kvm memory region as follows: e.g. qemu side (see the PROT_NONE flag) 1. mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); memory_region_init_ram_ptr() 2. re mmap the above area with read/write authority. Such example is used in virtio-fs qemu codes which hasn't been upstreamed [1]. But seems we can't forbid this example. Without this patch, it will cause an EPERM during kvm_set_memory_region() and cause qemu boot crash. As told by Ard, "the underlying assumption is incorrect, i.e., that the value of vm_flags at this point in time defines how the VMA is used during its lifetime. There may be other cases where a VMA is created with VM_READ vm_flags that are changed to VM_READ|VM_WRITE later, and we are currently rejecting this use case as well." [1] https://gitlab.com/virtio-fs/qemu/blob/5a356e/hw/virtio/vhost-user-fs.c#L488 Cc: Ard Biesheuvel Suggested-by: Ard Biesheuvel Signed-off-by: Jia He --- virt/kvm/arm/mmu.c | 9 - 1 file changed, 9 deletions(-) diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c index 38b4c910b6c3..a48994af70b8 100644 --- a/virt/kvm/arm/mmu.c +++ b/virt/kvm/arm/mmu.c @@ -2301,15 +2301,6 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, if (!vma || vma->vm_start >= reg_end) break; - /* -* Mapping a read-only VMA is only allowed if the -* memory region is configured as read-only. -*/ - if (writable && !(vma->vm_flags & VM_WRITE)) { - ret = -EPERM; - break; - } - /* * Take the intersection of this VMA with the memory region */ Applied, thanks. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm: get rid of unused arg in cpu_init_hyp_mode()
On 2019-11-21 07:15, linmiaohe wrote: From: Miaohe Lin As arg dummy is not really needed, there's no need to pass NULL when calling cpu_init_hyp_mode(). So clean it up. Signed-off-by: Miaohe Lin --- virt/kvm/arm/arm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index 86c6aa1cb58e..a5470f1b1a19 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -1315,7 +1315,7 @@ long kvm_arch_vm_ioctl(struct file *filp, } } -static void cpu_init_hyp_mode(void *dummy) +static void cpu_init_hyp_mode(void) { phys_addr_t pgd_ptr; unsigned long hyp_stack_ptr; @@ -1349,7 +1349,7 @@ static void cpu_hyp_reinit(void) if (is_kernel_in_hyp_mode()) kvm_timer_init_vhe(); else - cpu_init_hyp_mode(NULL); + cpu_init_hyp_mode(); kvm_arm_init_debug(); Applied, thanks. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] arm64: KVM: Invoke compute_layout() before alternatives are applied
On 2019-11-28 19:58, Sebastian Andrzej Siewior wrote: compute_layout() is invoked as part of an alternative fixup under stop_machine(). This function invokes get_random_long() which acquires a sleeping lock on -RT which can not be acquired in this context. Rename compute_layout() to kvm_compute_layout() and invoke it before stop_machine() applies the alternatives. Add a __init prefix to kvm_compute_layout() because the caller has it, too (and so the code can be discarded after boot). Signed-off-by: Sebastian Andrzej Siewior Acked-by: Marc Zyngier I think this should go via the arm64 tree, so I'll let Catalin and Will pick this up (unless they think otherwise). 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: fix missing free_percpu_irq in kvm_timer_hyp_init()
On 2019-11-23 02:30, linmiaohe wrote: From: Miaohe Lin When host_ptimer_irq request irq resource failed, we forget to release the host_vtimer_irq resource already requested. Fix this missing irq release and other similar scenario. That's really not a big deal, as nothing but KVM can use the timers anyway, but I guess it doesn't hurt to be correct. Fixes: 9e01dc76be6a ("KVM: arm/arm64: arch_timer: Assign the phys timer on VHE systems") Signed-off-by: Miaohe Lin --- virt/kvm/arm/arch_timer.c | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index f182b2380345..73867f97040c 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -935,7 +935,7 @@ int kvm_timer_hyp_init(bool has_gic) kvm_get_running_vcpus()); if (err) { kvm_err("kvm_arch_timer: error setting vcpu affinity\n"); - goto out_free_irq; + goto out_free_vtimer_irq; } static_branch_enable(&has_gic_active_state); @@ -960,7 +960,7 @@ int kvm_timer_hyp_init(bool has_gic) if (err) { kvm_err("kvm_arch_timer: can't request ptimer interrupt %d (%d)\n", host_ptimer_irq, err); - return err; + goto out_disable_gic_state; } if (has_gic) { @@ -968,7 +968,7 @@ int kvm_timer_hyp_init(bool has_gic) kvm_get_running_vcpus()); if (err) { kvm_err("kvm_arch_timer: error setting vcpu affinity\n"); - goto out_free_irq; + goto out_free_ptimer_irq; } } @@ -977,15 +977,22 @@ int kvm_timer_hyp_init(bool has_gic) kvm_err("kvm_arch_timer: invalid physical timer IRQ: %d\n", info->physical_irq); err = -ENODEV; - goto out_free_irq; + goto out_disable_gic_state; } cpuhp_setup_state(CPUHP_AP_KVM_ARM_TIMER_STARTING, "kvm/arm/timer:starting", kvm_timer_starting_cpu, kvm_timer_dying_cpu); return 0; -out_free_irq: + +out_free_ptimer_irq: + free_percpu_irq(host_ptimer_irq, kvm_get_running_vcpus()); +out_disable_gic_state: + if (has_gic) + static_branch_disable(&has_gic_active_state); Given that we're failing the init of KVM, this is totally superfluous. Also, this state is still valid, no matter what happens (the GIC is not going away from under our feet). +out_free_vtimer_irq: free_percpu_irq(host_vtimer_irq, kvm_get_running_vcpus()); + return err; } 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: [RFC 1/3] KVM: arm64: pmu: Don't increment SW_INCR if PMCR.E is unset
On Wed, Dec 04, 2019 at 09:44:24PM +0100, Eric Auger wrote: > The specification says PMSWINC increments PMEVCNTR_EL1 by 1 > if PMEVCNTR_EL0 is enabled and configured to count SW_INCR. > > For PMEVCNTR_EL0 to be enabled, we need both PMCNTENSET to > be set for the corresponding event counter but we also need > the PMCR.E bit to be set. > > Fixes: 7a0adc7064b8 ("arm64: KVM: Add access handler for PMSWINC register") > Signed-off-by: Eric Auger > --- > virt/kvm/arm/pmu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > index 8731dfeced8b..c3f8b059881e 100644 > --- a/virt/kvm/arm/pmu.c > +++ b/virt/kvm/arm/pmu.c > @@ -486,6 +486,9 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, > u64 val) > if (val == 0) > return; > > + if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E)) > + return; > + Reviewed-by: Andrew Murray > enable = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0); > for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) { > if (!(val & BIT(i))) > -- > 2.20.1 > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: arm: get rid of unused arg in cpu_init_hyp_mode()
On 30/11/2019 07:20, linmiaohe wrote: >> From: Miaohe Lin >> >> As arg dummy is not really needed, there's no need to pass NULL when calling >> cpu_init_hyp_mode(). So clean it up. It looks like the requirement for this dummy arg was removed in 67f691976662 ("arm64: kvm: allows kvm cpu hotplug"). FWIW: Reviewed-by: Steven Price >> Signed-off-by: Miaohe Lin >> --- >> virt/kvm/arm/arm.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index >> 86c6aa1cb58e..a5470f1b1a19 100644 >> --- a/virt/kvm/arm/arm.c >> +++ b/virt/kvm/arm/arm.c >> @@ -1315,7 +1315,7 @@ long kvm_arch_vm_ioctl(struct file *filp, >> } >> } >> >> -static void cpu_init_hyp_mode(void *dummy) >> +static void cpu_init_hyp_mode(void) >> { >> phys_addr_t pgd_ptr; >> unsigned long hyp_stack_ptr; >> @@ -1349,7 +1349,7 @@ static void cpu_hyp_reinit(void) >> if (is_kernel_in_hyp_mode()) >> kvm_timer_init_vhe(); >> else >> -cpu_init_hyp_mode(NULL); >> +cpu_init_hyp_mode(); >> >> kvm_arm_init_debug(); >> > friendly ping ... > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC 2/3] KVM: arm64: pmu: Fix chained SW_INCR counters
Hi, On 12/5/19 8:01 PM, Auger Eric wrote: > Hi Marc, > > On 12/5/19 3:52 PM, Marc Zyngier wrote: >> On 2019-12-05 14:06, Auger Eric wrote: >>> Hi Marc, >>> >>> On 12/5/19 10:43 AM, Marc Zyngier wrote: Hi Eric, On 2019-12-04 20:44, Eric Auger wrote: > At the moment a SW_INCR counter always overflows on 32-bit > boundary, independently on whether the n+1th counter is > programmed as CHAIN. > > Check whether the SW_INCR counter is a 64b counter and if so, > implement the 64b logic. > > Fixes: 80f393a23be6 ("KVM: arm/arm64: Support chained PMU counters") > Signed-off-by: Eric Auger > --- > virt/kvm/arm/pmu.c | 16 +++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c > index c3f8b059881e..7ab477db2f75 100644 > --- a/virt/kvm/arm/pmu.c > +++ b/virt/kvm/arm/pmu.c > @@ -491,6 +491,8 @@ void kvm_pmu_software_increment(struct kvm_vcpu > *vcpu, u64 val) > > enable = __vcpu_sys_reg(vcpu, PMCNTENSET_EL0); > for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) { > + bool chained = test_bit(i >> 1, vcpu->arch.pmu.chained); > + I'd rather you use kvm_pmu_pmc_is_chained() rather than open-coding this. But see below: > if (!(val & BIT(i))) > continue; > type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i) > @@ -500,8 +502,20 @@ void kvm_pmu_software_increment(struct kvm_vcpu > *vcpu, u64 val) > reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1; > reg = lower_32_bits(reg); > __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg; > - if (!reg) > + if (reg) /* no overflow */ > + continue; > + if (chained) { > + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) + 1; > + reg = lower_32_bits(reg); > + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) = reg; > + if (reg) > + continue; > + /* mark an overflow on high counter */ > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i + 1); > + } else { > + /* mark an overflow */ > __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i); > + } > } > } > } I think the whole function is a bit of a mess, and could be better structured to treat 64bit counters as a first class citizen. I'm suggesting something along those lines, which tries to streamline things a bit and keep the flow uniform between the two word sizes. IMHO, it helps reasonning about it and gives scope to the ARMv8.5 full 64bit counters... It is of course completely untested. >>> >>> Looks OK to me as well. One remark though, don't we need to test if the >>> n+1th reg is enabled before incrementing it? >> >> Hmmm. I'm not sure. I think we should make sure that we don't flag >> a counter as being chained if the odd counter is disabled, rather >> than checking it here. As long as the odd counter is not chained >> *and* enabled, we shouldn't touch it.> >> Again, untested: >> >> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c >> index cf371f643ade..47366817cd2a 100644 >> --- a/virt/kvm/arm/pmu.c >> +++ b/virt/kvm/arm/pmu.c >> @@ -15,6 +15,7 @@ >> #include >> >> static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 >> select_idx); >> +static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 >> select_idx); >> >> #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1 >> >> @@ -298,6 +299,7 @@ void kvm_pmu_enable_counter_mask(struct kvm_vcpu >> *vcpu, u64 val) >> * For high counters of chained events we must recreate the >> * perf event with the long (64bit) attribute set. >> */ >> + kvm_pmu_update_pmc_chained(vcpu, i); >> if (kvm_pmu_pmc_is_chained(pmc) && >> kvm_pmu_idx_is_high_counter(i)) { >> kvm_pmu_create_perf_event(vcpu, i); >> @@ -645,7 +647,8 @@ static void kvm_pmu_update_pmc_chained(struct >> kvm_vcpu *vcpu, u64 select_idx) >> struct kvm_pmu *pmu = &vcpu->arch.pmu; >> struct kvm_pmc *pmc = &pmu->pmc[select_idx]; >> >> - if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx)) { >> + if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx) && >> + kvm_pmu_counter_is_enabled(vcpu, pmc->idx)) { > > In create_perf_event(), has_chain_evtype() is used and a 64b sample > period would be chosen even if the counters are disjoined (since the odd > is disabled). We would need to use pmc_is_chained() instead. > > With perf_events, the check of whether the odd register is enabled is > properly done (create_perf_event). Hum that's not fully true. If we do not enable the CHAIN odd one but only the event one, the correct 32b per
Re: [PATCH] KVM: arm: remove excessive permission check in kvm_arch_prepare_memory_region
On Fri, Dec 06, 2019 at 10:08:02AM +0800, Jia He wrote: > In kvm_arch_prepare_memory_region, arm kvm regards the memory region as > writable if the flag has no KVM_MEM_READONLY, and the vm is readonly if > !VM_WRITE. > > But there is common usage for setting kvm memory region as follows: > e.g. qemu side (see the PROT_NONE flag) > 1. mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); >memory_region_init_ram_ptr() > 2. re mmap the above area with read/write authority. > > Such example is used in virtio-fs qemu codes which hasn't been upstreamed > [1]. But seems we can't forbid this example. > > Without this patch, it will cause an EPERM during kvm_set_memory_region() > and cause qemu boot crash. > > As told by Ard, "the underlying assumption is incorrect, i.e., that the > value of vm_flags at this point in time defines how the VMA is used > during its lifetime. There may be other cases where a VMA is created > with VM_READ vm_flags that are changed to VM_READ|VM_WRITE later, and > we are currently rejecting this use case as well." > > [1] > https://gitlab.com/virtio-fs/qemu/blob/5a356e/hw/virtio/vhost-user-fs.c#L488 Reviewed-by: Christoffer Dall > > Cc: Ard Biesheuvel > Suggested-by: Ard Biesheuvel > Signed-off-by: Jia He > --- > virt/kvm/arm/mmu.c | 9 - > 1 file changed, 9 deletions(-) > > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c > index 38b4c910b6c3..a48994af70b8 100644 > --- a/virt/kvm/arm/mmu.c > +++ b/virt/kvm/arm/mmu.c > @@ -2301,15 +2301,6 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > if (!vma || vma->vm_start >= reg_end) > break; > > - /* > - * Mapping a read-only VMA is only allowed if the > - * memory region is configured as read-only. > - */ > - if (writable && !(vma->vm_flags & VM_WRITE)) { > - ret = -EPERM; > - break; > - } > - > /* >* Take the intersection of this VMA with the memory region >*/ > -- > 2.17.1 > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] KVM: arm: remove excessive permission check in kvm_arch_prepare_memory_region
In kvm_arch_prepare_memory_region, arm kvm regards the memory region as writable if the flag has no KVM_MEM_READONLY, and the vm is readonly if !VM_WRITE. But there is common usage for setting kvm memory region as follows: e.g. qemu side (see the PROT_NONE flag) 1. mmap(NULL, size, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); memory_region_init_ram_ptr() 2. re mmap the above area with read/write authority. Such example is used in virtio-fs qemu codes which hasn't been upstreamed [1]. But seems we can't forbid this example. Without this patch, it will cause an EPERM during kvm_set_memory_region() and cause qemu boot crash. As told by Ard, "the underlying assumption is incorrect, i.e., that the value of vm_flags at this point in time defines how the VMA is used during its lifetime. There may be other cases where a VMA is created with VM_READ vm_flags that are changed to VM_READ|VM_WRITE later, and we are currently rejecting this use case as well." [1] https://gitlab.com/virtio-fs/qemu/blob/5a356e/hw/virtio/vhost-user-fs.c#L488 Cc: Ard Biesheuvel Suggested-by: Ard Biesheuvel Signed-off-by: Jia He --- virt/kvm/arm/mmu.c | 9 - 1 file changed, 9 deletions(-) diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c index 38b4c910b6c3..a48994af70b8 100644 --- a/virt/kvm/arm/mmu.c +++ b/virt/kvm/arm/mmu.c @@ -2301,15 +2301,6 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, if (!vma || vma->vm_start >= reg_end) break; - /* -* Mapping a read-only VMA is only allowed if the -* memory region is configured as read-only. -*/ - if (writable && !(vma->vm_flags & VM_WRITE)) { - ret = -EPERM; - break; - } - /* * Take the intersection of this VMA with the memory region */ -- 2.17.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm