date:20191206

Re: [PATCH] KVM: arm: fix missing free_percpu_irq in kvm_timer_hyp_init()

2019-12-06 Thread linmiaohe

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(_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

2019-12-06 Thread Marc Zyngier

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

2019-12-06 Thread Richard Henderson

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

2019-12-06 Thread Alexandru Elisei

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

2019-12-06 Thread Eric Auger

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, )) {
+   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, _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();
+

[kvm-unit-tests RFC 08/10] arm: gic: Provide per-IRQ helper functions

2019-12-06 Thread Eric Auger

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

[kvm-unit-tests RFC 06/10] pmu: Test chained counter

2019-12-06 Thread Eric Auger

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

2019-12-06 Thread Eric Auger

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(), );
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

2019-12-06 Thread Eric Auger

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

2019-12-06 Thread Eric Auger

---
 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 05/10] pmu: Basic event counter Tests

2019-12-06 Thread Eric Auger

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

[kvm-unit-tests RFC 00/10] KVM: arm64: PMUv3 Event Counter Tests

2019-12-06 Thread Eric Auger

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

2019-12-06 Thread Eric Auger

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

2019-12-06 Thread Andrew Murray

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 = >arch.pmu;
> > >   struct kvm_pmc *pmc = >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

2019-12-06 Thread Andrew Murray

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 = >arch.pmu;
> >  struct kvm_pmc *pmc = >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 enabled the

Re: [RFC 2/3] KVM: arm64: pmu: Fix chained SW_INCR counters

2019-12-06 Thread Marc Zyngier


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 = >arch.pmu;
struct kvm_pmc *pmc = >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

2019-12-06 Thread Andrew Murray

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 = >arch.pmu;
>   struct kvm_pmc *pmc = >pmc[select_idx];
> 
> - if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx)) {
> + if (kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx) &&
> +

Re: [PATCH v2 0/5] target/arm: More EL2 trapping fixes

2019-12-06 Thread Marc Zyngier


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

2019-12-06 Thread Peter Maydell

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

2019-12-06 Thread Marc Zyngier


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

2019-12-06 Thread Peter Maydell

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

2019-12-06 Thread Peter Maydell

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

2019-12-06 Thread Catalin Marinas

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

2019-12-06 Thread Marc Zyngier


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

2019-12-06 Thread Catalin Marinas

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()

2019-12-06 Thread Marc Zyngier


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()

2019-12-06 Thread Marc Zyngier


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(>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(>arch.vgic.rd_regions);

 out_unlock:
-   for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
-   vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx);
-   mutex_unlock(>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

2019-12-06 Thread Catalin Marinas

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

2019-12-06 Thread Marc Zyngier


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

2019-12-06 Thread Marc Zyngier


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()

2019-12-06 Thread Marc Zyngier


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

2019-12-06 Thread Marc Zyngier


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()

2019-12-06 Thread Marc Zyngier


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(_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(_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

2019-12-06 Thread Andrew Murray

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()

2019-12-06 Thread Steven Price

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

2019-12-06 Thread Auger Eric

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 = >arch.pmu;
>>  struct kvm_pmc *pmc = >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 perf counter

Re: [PATCH] KVM: arm: remove excessive permission check in kvm_arch_prepare_memory_region

2019-12-06 Thread Christoffer Dall

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

2019-12-06 Thread Jia He

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

38 matches

Mail list logo