[PATCH 0/7] target/arm: Add vSPE support to KVM guest

[PATCH 0/7] target/arm: Add vSPE support to KVM guest

[Haibo Xu]

This series add support for SPE(Statistical Profiling Extension)[1]
in KVM guest. It's based on Andrew Murray's kernel KVM patches V2[2],
and has been tested to ensure that guest can use SPE with valid data.
E.g.

In host:
$ ./qemu-system-aarch64 \
        -cpu host -M virt,accel=kvm,gic-version=3 -nographic -m 2048M \
        -kernel ./Image-new \
        -initrd /boot/initrd.img-5.6.0-rc2+ \
        -append "root=/dev/vda rw console=ttyAMA0" -nodefaults -serial stdio\
        -drive if=none,file=./xenial.rootfs.ext4,id=hd0,format=raw \
        -device virtio-blk-device,drive=hd0  \

In guest:
$ perf record -e arm_spe/ts_enable=1,pa_enable=1,pct_enable=1/ \
        dd if=/dev/zero of=/dev/null count=1000
$ perf report --dump-raw-trace > spe_buf.txt

The spe_buf.txt should contain similar data as below:

. ... ARM SPE data: size 135944 bytes
.  00000000:  b0 f4 d3 29 10 00 80 ff a0                      PC 0xff80001029d3f4 el1 ns=1
.  00000009:  99 0b 00                                        LAT 11 ISSUE
.  0000000c:  98 0d 00                                        LAT 13 TOT
.  0000000f:  52 16 00                                        EV RETIRED L1D-ACCESS TLB-ACCESS
.  00000012:  49 00                                           LD
.  00000014:  b2 d0 40 d8 70 00 00 ff 00                      VA 0xff000070d840d0
.  0000001d:  9a 01 00                                        LAT 1 XLAT
.  00000020:  00 00 00                                        PAD
.  00000023:  71 a5 1f b3 20 14 00 00 00                      TS 86447955877
.  0000002c:  b0 7c f9 29 10 00 80 ff a0                      PC 0xff80001029f97c el1 ns=1
.  00000035:  99 02 00                                        LAT 2 ISSUE
.  00000038:  98 03 00                                        LAT 3 TOT
.  0000003b:  52 02 00                                        EV RETIRED
.  0000003e:  48 00                                           INSN-OTHER
.  00000040:  00 00 00                                        PAD
.  00000043:  71 ef 1f b3 20 14 00 00 00                      TS 86447955951
.  0000004c:  b0 f0 e9 29 10 00 80 ff a0                      PC 0xff80001029e9f0 el1 ns=1
.  00000055:  99 02 00                                        LAT 2 ISSUE
.  00000058:  98 03 00                                        LAT 3 TOT
.  0000005b:  52 02 00                                        EV RETIRED

If you want to disable the vSPE support, you can use the 'spe=off' cpu
property:

./qemu-system-aarch64 \
        -cpu host,spe=off -M virt,accel=kvm,gic-version=3 -nographic -m 2048M \
        -kernel ./Image-new \
        -initrd /boot/initrd.img-5.6.0-rc2+ \
        -append "root=/dev/vda rw console=ttyAMA0" -nodefaults -serial stdio\
        -drive if=none,file=./xenial.rootfs.ext4,id=hd0,format=raw \
        -device virtio-blk-device,drive=hd0  \

Note:
(1) Since the kernel patches are still under review, some of the macros
    in the header files may be changed after merging. We may need to
    update them accordingly.
(2) These patches only add vSPE support in KVM mode, for TCG mode, I'm
    not sure whether we need to support it.
(3) Just followed the 'pmu' property, we only allow this feature to be
    removed from CPUs which enable it by default. But since the SPE is
    an optional feature extension for Armv8.2, I think a better way may
    be to disable it by default, and only enable it when the host cpu
    do have the feature.

[1]https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/
   posts/statistical-profiling-extension-for-armv8-a
[2]https://www.spinics.net/lists/arm-kernel/msg776228.html

Haibo Xu (7):
  update Linux headers with new vSPE macros
  target/arm/kvm: spe: Add helper to detect SPE when using KVM
  target/arm/cpu: spe: Add an option to turn on/off vSPE support
  target/arm/kvm: spe: Unify device attr operatioin helper
  target/arm/kvm: spe: Add device init and set_irq operations
  hw/arm/virt: spe: Add SPE fdt binding for virt machine
  target/arm/cpu: spe: Enable spe to work with host cpu

 hw/arm/virt-acpi-build.c      |  3 +++
 hw/arm/virt.c                 | 42 ++++++++++++++++++++++++++++++
 include/hw/acpi/acpi-defs.h   |  1 +
 include/hw/arm/virt.h         |  1 +
 linux-headers/asm-arm64/kvm.h |  4 +++
 linux-headers/linux/kvm.h     |  2 ++
 target/arm/cpu.c              | 34 +++++++++++++++++++++++++
 target/arm/cpu.h              |  5 ++++
 target/arm/kvm.c              | 11 ++++++++
 target/arm/kvm64.c            | 48 ++++++++++++++++++++++++++++++++---
 target/arm/kvm_arm.h          | 18 +++++++++++++
 11 files changed, 166 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/7] update Linux headers with new vSPE macros

[Haibo Xu]

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 linux-headers/asm-arm64/kvm.h | 4 ++++
 linux-headers/linux/kvm.h     | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index 9e34f0f875..802319ee02 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -106,6 +106,7 @@ struct kvm_regs {
 #define KVM_ARM_VCPU_SVE               4 /* enable SVE for this CPU */
 #define KVM_ARM_VCPU_PTRAUTH_ADDRESS   5 /* VCPU uses address authentication */
 #define KVM_ARM_VCPU_PTRAUTH_GENERIC   6 /* VCPU uses generic authentication */
+#define KVM_ARM_VCPU_SPE_V1            7 /* Support guest SPEv1 */
 
 struct kvm_vcpu_init {
        __u32 target;
@@ -334,6 +335,9 @@ struct kvm_vcpu_events {
 #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER                1   
 #define KVM_ARM_VCPU_PVTIME_CTRL       2   
 #define   KVM_ARM_VCPU_PVTIME_IPA      0
+#define KVM_ARM_VCPU_SPE_V1_CTRL       3
+#define   KVM_ARM_VCPU_SPE_V1_IRQ      0
+#define   KVM_ARM_VCPU_SPE_V1_INIT     1

 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_VCPU2_SHIFT                28
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index a28c366737..35ef0ae842 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_ARM_SPE_V1 184

 #ifdef KVM_CAP_IRQ_ROUTING

@@ -1671,6 +1672,7 @@ struct kvm_assigned_msix_entry {
 #define KVM_ARM_DEV_EL1_VTIMER         (1 << 0)
 #define KVM_ARM_DEV_EL1_PTIMER         (1 << 1)
 #define KVM_ARM_DEV_PMU                        (1 << 2)
+#define KVM_ARM_DEV_SPE                        (1 << 3)

 struct kvm_hyperv_eventfd {
        __u32 conn_id;
-- 
2.17.1

[PATCH 2/7] target/arm/kvm: spe: Add helper to detect SPE when using KVM

[Haibo Xu]

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/kvm.c     |  5 +++++
 target/arm/kvm_arm.h | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 8bb7318378..58f991e890 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -214,6 +214,11 @@ bool kvm_arm_pmu_supported(void)
     return kvm_check_extension(kvm_state, KVM_CAP_ARM_PMU_V3);
 }
 
+bool kvm_arm_spe_supported(void)
+{
+    return kvm_check_extension(kvm_state, KVM_CAP_ARM_SPE_V1);
+}
+
 int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
 {
     KVMState *s = KVM_STATE(ms->accelerator);
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index adb38514bf..f79655674e 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -283,6 +283,14 @@ bool kvm_arm_aarch32_supported(void);
  */
 bool kvm_arm_pmu_supported(void);

+/**
+ * kvm_arm_spe_supported:
+ *
+ * Returns: true if the KVM VCPU can enable its SPE
+ * and false otherwise.
+ */
+bool kvm_arm_spe_supported(void);
+
 /**
  * kvm_arm_sve_supported:
  *
@@ -366,6 +374,11 @@ static inline bool kvm_arm_pmu_supported(void)
     return false;
 }

+static inline bool kvm_arm_spe_supported(void)
+{
+    return false;
+}
+
 static inline bool kvm_arm_sve_supported(void)
 {
     return false;
-- 
2.17.1

[PATCH 3/7] target/arm/cpu: spe: Add an option to turn on/off vSPE support

[Haibo Xu]

Adds a spe=[on/off] option to enable/disable vSPE support in
guest vCPU. Note this option is only available for "-cpu host"
with KVM mode, and default value is on. 

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/cpu.c | 28 ++++++++++++++++++++++++++++
 target/arm/cpu.h |  3 +++ 
 2 files changed, 31 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 111579554f..40768b4d19 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
     cpu->has_pmu = value;
 }
 
+static bool arm_get_spe(Object *obj, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    return cpu->has_spe;
+}
+
+static void arm_set_spe(Object *obj, bool value, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    if (value) {
+        if (kvm_enabled() && !kvm_arm_spe_supported()) {
+            error_setg(errp, "'spe' feature not supported by KVM on this host");
+            return;
+        }
+        set_feature(&cpu->env, ARM_FEATURE_SPE);
+    } else {
+        unset_feature(&cpu->env, ARM_FEATURE_SPE);
+    }
+    cpu->has_spe = value;
+}
+
 unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
 {
     /*
@@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj)
         object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
     }

+    if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) {
+        cpu->has_spe = true;
+        object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe);
+    }
+
     /*
      * Allow user to turn off VFP and Neon support, but only for TCG --
      * KVM does not currently allow us to lie to the guest about its
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e8ed423ea..fe0ac14386 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -822,6 +822,8 @@ struct ARMCPU {
     bool has_el3;
     /* CPU has PMU (Performance Monitor Unit) */
     bool has_pmu;
+    /* CPU has SPE (Statistical Profiling Extension) */
+    bool has_spe;
     /* CPU has VFP */
     bool has_vfp;
     /* CPU has Neon */
@@ -1959,6 +1961,7 @@ enum arm_features {
     ARM_FEATURE_VBAR, /* has cp15 VBAR */
     ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
     ARM_FEATURE_M_MAIN, /* M profile Main Extension */
+    ARM_FEATURE_SPE, /* has SPE support */
 };

 static inline int arm_feature(CPUARMState *env, int feature)
-- 
2.17.1

[PATCH 4/7] target/arm/kvm: spe: Unify device attr operatioin helper

[Haibo Xu]

Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(),
So both the vPMU and vSPE device can share the same API.

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/kvm64.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 1169237905..75a417d65c 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
     return NULL;
 }
 
-static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
+static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr)
 {
     int err;
 
@@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs)
     if (!ARM_CPU(cs)->has_pmu) {
         return;
     }
-    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
+    if (!kvm_arm_dev_set_attr(cs, &attr)) {
         error_report("failed to init PMU");
         abort();
     }
@@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
     if (!ARM_CPU(cs)->has_pmu) {
         return;
     }
-    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
+    if (!kvm_arm_dev_set_attr(cs, &attr)) {
         error_report("failed to set irq for PMU");
         abort();
     }
-- 
2.17.1

[PATCH 5/7] target/arm/kvm: spe: Add device init and set_irq operations

[Haibo Xu]

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/kvm64.c   | 33 +++++++++++++++++++++++++++++++++
 target/arm/kvm_arm.h |  5 +++++
 2 files changed, 38 insertions(+)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 75a417d65c..be045ccc5f 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -450,6 +450,39 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
     }   
 }
 
+void kvm_arm_spe_init(CPUState *cs)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_ARM_VCPU_SPE_V1_CTRL,
+        .attr = KVM_ARM_VCPU_SPE_V1_INIT,
+    };
+
+    if (!ARM_CPU(cs)->has_spe) {
+        return;
+    }
+    if (!kvm_arm_dev_set_attr(cs, &attr)) {
+        error_report("failed to init SPE");
+        abort();
+    }
+}
+
+void kvm_arm_spe_set_irq(CPUState *cs, int irq)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_ARM_VCPU_SPE_V1_CTRL,
+        .addr = (intptr_t)&irq,
+        .attr = KVM_ARM_VCPU_SPE_V1_IRQ,
+    };
+
+    if (!ARM_CPU(cs)->has_spe) {
+        return;
+    }
+    if (!kvm_arm_dev_set_attr(cs, &attr)) {
+        error_report("failed to set irq for SPE");
+        abort();
+    }
+}
+
 static int read_sys_reg32(int fd, uint32_t *pret, uint64_t id)
 {
     uint64_t ret;
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index f79655674e..bb155322eb 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -348,6 +348,8 @@ int kvm_arm_vgic_probe(void);

 void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
 void kvm_arm_pmu_init(CPUState *cs);
+void kvm_arm_spe_set_irq(CPUState *cs, int irq);
+void kvm_arm_spe_init(CPUState *cs);
 int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level);

 #else
@@ -397,6 +399,9 @@ static inline int kvm_arm_vgic_probe(void)
 static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {}
 static inline void kvm_arm_pmu_init(CPUState *cs) {}

+static inline void kvm_arm_spe_set_irq(CPUState *cs, int irq) {}
+static inline void kvm_arm_spe_init(CPUState *cs) {}
+
 static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) {}

 static inline void kvm_arm_get_virtual_time(CPUState *cs) {}
-- 
2.17.1

[PATCH 6/7] hw/arm/virt: spe: Add SPE fdt binding for virt machine

[Haibo Xu]

Add a virtual SPE device for virt machine while using PPI 
5 for SPE overflow interrupt number.

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 hw/arm/virt-acpi-build.c    |  3 +++ 
 hw/arm/virt.c               | 42 +++++++++++++++++++++++++++++++++++++
 include/hw/acpi/acpi-defs.h |  1 + 
 include/hw/arm/virt.h       |  1 + 
 target/arm/cpu.c            |  2 ++
 target/arm/cpu.h            |  2 ++
 target/arm/kvm.c            |  6 ++++++
 7 files changed, 57 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 91f0df7b13..5073ba22a5 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
             gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
         }
+        if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
+            gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ));
+        }
         if (vms->virt) {
             gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ));
         }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ecfee362a1..c40819705d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
     }
 }

+static void fdt_add_spe_nodes(const VirtMachineState *vms)
+{
+    CPUState *cpu;
+    ARMCPU *armcpu;
+    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
+
+    CPU_FOREACH(cpu) {
+        armcpu = ARM_CPU(cpu);
+        if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
+            return;
+        }
+        if (kvm_enabled()) {
+            if (kvm_irqchip_in_kernel()) {
+                kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ));
+            }
+            kvm_arm_spe_init(cpu);
+        }
+    }
+
+    if (vms->gic_version == VIRT_GIC_VERSION_2) {
+        irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
+                             GIC_FDT_IRQ_PPI_CPU_WIDTH,
+                             (1 << vms->smp_cpus) - 1);
+    }
+
+    armcpu = ARM_CPU(qemu_get_cpu(0));
+    qemu_fdt_add_subnode(vms->fdt, "/spe");
+    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
+        const char compat[] = "arm,statistical-profiling-extension-v1";
+        qemu_fdt_setprop(vms->fdt, "/spe", "compatible",
+                         compat, sizeof(compat));
+        qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts",
+                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags);
+    }
+}
+
 static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
 {
     DeviceState *dev;
@@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms)
                                     qdev_get_gpio_in(vms->gic, ppibase
                                                      + VIRTUAL_PMU_IRQ));

+        qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0,
+                                    qdev_get_gpio_in(vms->gic, ppibase
+                                                     + VIRTUAL_SPE_IRQ));
+
         sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
         sysbus_connect_irq(gicbusdev, i + smp_cpus,
                            qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
@@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine)

     fdt_add_pmu_nodes(vms);

+    fdt_add_spe_nodes(vms);
+
     create_uart(vms, VIRT_UART, sysmem, serial_hd(0));

     if (vms->secure) {
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 38a42f409a..56a7f38ae4 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface {
     uint32_t vgic_interrupt;
     uint64_t gicr_base_address;
     uint64_t arm_mpidr;
+    uint16_t spe_interrupt; /* ACPI 6.3 */
 } QEMU_PACKED;

 typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface;
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index dff67e1bef..56c83224d2 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -49,6 +49,7 @@
 #define ARCH_TIMER_NS_EL1_IRQ 14
 #define ARCH_TIMER_NS_EL2_IRQ 10

+#define VIRTUAL_SPE_IRQ 5
 #define VIRTUAL_PMU_IRQ 7

 #define PPI(irq) ((irq) + 16)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 40768b4d19..67ab0089fd 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj)
                              "gicv3-maintenance-interrupt", 1);
     qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt,
                              "pmu-interrupt", 1);
+    qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt,
+                             "spe-interrupt", 1);
 #endif

     /* DTB consumers generally don't in fact care what the 'compatible'
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index fe0ac14386..4bf8591df8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -790,6 +790,8 @@ struct ARMCPU {
     qemu_irq gicv3_maintenance_interrupt;
     /* GPIO output for the PMU interrupt */
     qemu_irq pmu_interrupt;
+    /* GPIO output for the SPE interrupt */
+    qemu_irq spe_interrupt;

     /* MemoryRegion to use for secure physical accesses */
     MemoryRegion *secure_memory;
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 58f991e890..ecafdda364 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
             switched_level &= ~KVM_ARM_DEV_PMU;
         }

+        if (switched_level & KVM_ARM_DEV_SPE) {
+            qemu_set_irq(cpu->spe_interrupt,
+                         !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE));
+            switched_level &= ~KVM_ARM_DEV_SPE;
+        }
+
         if (switched_level) {
             qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n",
                           __func__, switched_level);
-- 
2.17.1

[PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Haibo Xu]

Turn on the spe cpu property by default when working with host
cpu type in KVM mode, i.e. we can now do '-cpu host' to add the 
vSPE, and '-cpu host,spe=off' to remove it. 

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/cpu.c   | 4 ++++
 target/arm/kvm64.c | 9 +++++++++
 2 files changed, 13 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 67ab0089fd..42fa99953c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1719,6 +1719,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         cpu->pmceid1 = 0;
     }   
 
+    if (!cpu->has_spe || !kvm_enabled()) {
+        unset_feature(env, ARM_FEATURE_SPE);
+    }
+
     if (!arm_feature(env, ARM_FEATURE_EL2)) {
         /* Disable the hypervisor feature bits in the processor feature
          * registers if we don't have EL2. These are id_pfr1[15:12] and
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index be045ccc5f..4ea58afc1d 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -679,6 +679,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     features |= 1ULL << ARM_FEATURE_AARCH64;
     features |= 1ULL << ARM_FEATURE_PMU;
     features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
+    features |= 1ULL << ARM_FEATURE_SPE;

     ahcf->features = features;

@@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
     } else {
         env->features &= ~(1ULL << ARM_FEATURE_PMU);
     }
+    if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) {
+        cpu->has_spe = false;
+    }
+    if (cpu->has_spe) {
+        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1;
+    } else {
+        env->features &= ~(1ULL << ARM_FEATURE_SPE);
+    }
     if (cpu_isar_feature(aa64_sve, cpu)) {
         assert(kvm_arm_sve_supported());
         cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
-- 
2.17.1

Re: [PATCH 4/7] target/arm/kvm: spe: Unify device attr operatioin helper

[Philippe Mathieu-Daudé]

On 8/7/20 10:10 AM, Haibo Xu wrote:
> Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(),

Maybe rename kvm_arm_device_set_attr() to match the structure
name?

> So both the vPMU and vSPE device can share the same API.
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>

Regardless, with the typo "operation" in patch subject fixed:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/kvm64.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 1169237905..75a417d65c 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
>      return NULL;
>  }
>  
> -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
> +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr)
>  {
>      int err;
>  
> @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs)
>      if (!ARM_CPU(cs)->has_pmu) {
>          return;
>      }
> -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> +    if (!kvm_arm_dev_set_attr(cs, &attr)) {
>          error_report("failed to init PMU");
>          abort();
>      }
> @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
>      if (!ARM_CPU(cs)->has_pmu) {
>          return;
>      }
> -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> +    if (!kvm_arm_dev_set_attr(cs, &attr)) {
>          error_report("failed to set irq for PMU");
>          abort();
>      }
> 

Re: [PATCH 3/7] target/arm/cpu: spe: Add an option to turn on/off vSPE support

[Philippe Mathieu-Daudé]

Hi Haibo,

On 8/7/20 10:10 AM, Haibo Xu wrote:
> Adds a spe=[on/off] option to enable/disable vSPE support in
> guest vCPU. Note this option is only available for "-cpu host"
> with KVM mode, and default value is on. 
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/cpu.c | 28 ++++++++++++++++++++++++++++
>  target/arm/cpu.h |  3 +++ 
>  2 files changed, 31 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 111579554f..40768b4d19 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>      cpu->has_pmu = value;
>  }
>  
> +static bool arm_get_spe(Object *obj, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    return cpu->has_spe;
> +}
> +
> +static void arm_set_spe(Object *obj, bool value, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    if (value) {
> +        if (kvm_enabled() && !kvm_arm_spe_supported()) {
> +            error_setg(errp, "'spe' feature not supported by KVM on this host");
> +            return;
> +        }
> +        set_feature(&cpu->env, ARM_FEATURE_SPE);
> +    } else {
> +        unset_feature(&cpu->env, ARM_FEATURE_SPE);
> +    }
> +    cpu->has_spe = value;
> +}
> +
>  unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
>  {
>      /*
> @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj)
>          object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
>      }
> 
> +    if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) {
> +        cpu->has_spe = true;

Being a profiling feature, are you sure you want it to be ON by default?

I'd expect the opposite, either being turned on via command line at
startup or by a management layer at runtime, when someone is ready
to record the perf events and analyze them.

> +        object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe);
> +    }
> +
>      /*
>       * Allow user to turn off VFP and Neon support, but only for TCG --
>       * KVM does not currently allow us to lie to the guest about its
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9e8ed423ea..fe0ac14386 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -822,6 +822,8 @@ struct ARMCPU {
>      bool has_el3;
>      /* CPU has PMU (Performance Monitor Unit) */
>      bool has_pmu;
> +    /* CPU has SPE (Statistical Profiling Extension) */
> +    bool has_spe;
>      /* CPU has VFP */
>      bool has_vfp;
>      /* CPU has Neon */
> @@ -1959,6 +1961,7 @@ enum arm_features {
>      ARM_FEATURE_VBAR, /* has cp15 VBAR */
>      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
> +    ARM_FEATURE_SPE, /* has SPE support */
>  };
> 
>  static inline int arm_feature(CPUARMState *env, int feature)
> 

Re: [PATCH 4/7] target/arm/kvm: spe: Unify device attr operatioin helper

[Haibo Xu]

On Fri, 7 Aug 2020 at 16:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 8/7/20 10:10 AM, Haibo Xu wrote:
> > Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(),
>
> Maybe rename kvm_arm_device_set_attr() to match the structure
> name?
>

Thanks for the review! I will update it in the next version.

> > So both the vPMU and vSPE device can share the same API.
> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
>
> Regardless, with the typo "operation" in patch subject fixed:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> > ---
> >  target/arm/kvm64.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index 1169237905..75a417d65c 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
> >      return NULL;
> >  }
> >
> > -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
> > +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr)
> >  {
> >      int err;
> >
> > @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs)
> >      if (!ARM_CPU(cs)->has_pmu) {
> >          return;
> >      }
> > -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> > +    if (!kvm_arm_dev_set_attr(cs, &attr)) {
> >          error_report("failed to init PMU");
> >          abort();
> >      }
> > @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> >      if (!ARM_CPU(cs)->has_pmu) {
> >          return;
> >      }
> > -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> > +    if (!kvm_arm_dev_set_attr(cs, &attr)) {
> >          error_report("failed to set irq for PMU");
> >          abort();
> >      }
> >
>

Re: [PATCH 3/7] target/arm/cpu: spe: Add an option to turn on/off vSPE support

[Haibo Xu]

On Fri, 7 Aug 2020 at 16:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Haibo,
>
> On 8/7/20 10:10 AM, Haibo Xu wrote:
> > Adds a spe=[on/off] option to enable/disable vSPE support in
> > guest vCPU. Note this option is only available for "-cpu host"
> > with KVM mode, and default value is on.
> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> > ---
> >  target/arm/cpu.c | 28 ++++++++++++++++++++++++++++
> >  target/arm/cpu.h |  3 +++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 111579554f..40768b4d19 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
> >      cpu->has_pmu = value;
> >  }
> >
> > +static bool arm_get_spe(Object *obj, Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +    return cpu->has_spe;
> > +}
> > +
> > +static void arm_set_spe(Object *obj, bool value, Error **errp)
> > +{
> > +    ARMCPU *cpu = ARM_CPU(obj);
> > +
> > +    if (value) {
> > +        if (kvm_enabled() && !kvm_arm_spe_supported()) {
> > +            error_setg(errp, "'spe' feature not supported by KVM on this host");
> > +            return;
> > +        }
> > +        set_feature(&cpu->env, ARM_FEATURE_SPE);
> > +    } else {
> > +        unset_feature(&cpu->env, ARM_FEATURE_SPE);
> > +    }
> > +    cpu->has_spe = value;
> > +}
> > +
> >  unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
> >  {
> >      /*
> > @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj)
> >          object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
> >      }
> >
> > +    if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) {
> > +        cpu->has_spe = true;
>
> Being a profiling feature, are you sure you want it to be ON by default?
>
> I'd expect the opposite, either being turned on via command line at
> startup or by a management layer at runtime, when someone is ready
> to record the perf events and analyze them.
>

I'm not sure whether it's proper to follow the 'pmu' setting here
which has it on  by default.
To be honest, I also prefer to turn it off by default(Please refer to
the comments in the cover letter).

> > +        object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe);
> > +    }
> > +
> >      /*
> >       * Allow user to turn off VFP and Neon support, but only for TCG --
> >       * KVM does not currently allow us to lie to the guest about its
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 9e8ed423ea..fe0ac14386 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -822,6 +822,8 @@ struct ARMCPU {
> >      bool has_el3;
> >      /* CPU has PMU (Performance Monitor Unit) */
> >      bool has_pmu;
> > +    /* CPU has SPE (Statistical Profiling Extension) */
> > +    bool has_spe;
> >      /* CPU has VFP */
> >      bool has_vfp;
> >      /* CPU has Neon */
> > @@ -1959,6 +1961,7 @@ enum arm_features {
> >      ARM_FEATURE_VBAR, /* has cp15 VBAR */
> >      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
> >      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
> > +    ARM_FEATURE_SPE, /* has SPE support */
> >  };
> >
> >  static inline int arm_feature(CPUARMState *env, int feature)
> >
>

Re: [PATCH 3/7] target/arm/cpu: spe: Add an option to turn on/off vSPE support

[Philippe Mathieu-Daudé]

On 8/10/20 5:03 AM, Haibo Xu wrote:
> On Fri, 7 Aug 2020 at 16:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Hi Haibo,
>>
>> On 8/7/20 10:10 AM, Haibo Xu wrote:
>>> Adds a spe=[on/off] option to enable/disable vSPE support in
>>> guest vCPU. Note this option is only available for "-cpu host"
>>> with KVM mode, and default value is on.

You are right, we don't know whether if the feature is announced
to the guest, the guest will use the SPE registers, so similarly
to PMU have it default ON if available.

>>>
>>> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
>>> ---
>>>  target/arm/cpu.c | 28 ++++++++++++++++++++++++++++
>>>  target/arm/cpu.h |  3 +++
>>>  2 files changed, 31 insertions(+)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 111579554f..40768b4d19 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>>>      cpu->has_pmu = value;
>>>  }
>>>
>>> +static bool arm_get_spe(Object *obj, Error **errp)
>>> +{
>>> +    ARMCPU *cpu = ARM_CPU(obj);
>>> +
>>> +    return cpu->has_spe;
>>> +}
>>> +
>>> +static void arm_set_spe(Object *obj, bool value, Error **errp)
>>> +{
>>> +    ARMCPU *cpu = ARM_CPU(obj);
>>> +
>>> +    if (value) {
>>> +        if (kvm_enabled() && !kvm_arm_spe_supported()) {
>>> +            error_setg(errp, "'spe' feature not supported by KVM on this host");
>>> +            return;
>>> +        }
>>> +        set_feature(&cpu->env, ARM_FEATURE_SPE);
>>> +    } else {
>>> +        unset_feature(&cpu->env, ARM_FEATURE_SPE);
>>> +    }
>>> +    cpu->has_spe = value;
>>> +}
>>> +
>>>  unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
>>>  {
>>>      /*
>>> @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj)
>>>          object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
>>>      }
>>>
>>> +    if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) {
>>> +        cpu->has_spe = true;
>>
>> Being a profiling feature, are you sure you want it to be ON by default?
>>
>> I'd expect the opposite, either being turned on via command line at
>> startup or by a management layer at runtime, when someone is ready
>> to record the perf events and analyze them.
>>
> 
> I'm not sure whether it's proper to follow the 'pmu' setting here
> which has it on  by default.
> To be honest, I also prefer to turn it off by default(Please refer to
> the comments in the cover letter).
> 
>>> +        object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe);
>>> +    }
>>> +
>>>      /*
>>>       * Allow user to turn off VFP and Neon support, but only for TCG --
>>>       * KVM does not currently allow us to lie to the guest about its
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 9e8ed423ea..fe0ac14386 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -822,6 +822,8 @@ struct ARMCPU {
>>>      bool has_el3;
>>>      /* CPU has PMU (Performance Monitor Unit) */
>>>      bool has_pmu;
>>> +    /* CPU has SPE (Statistical Profiling Extension) */
>>> +    bool has_spe;
>>>      /* CPU has VFP */
>>>      bool has_vfp;
>>>      /* CPU has Neon */
>>> @@ -1959,6 +1961,7 @@ enum arm_features {
>>>      ARM_FEATURE_VBAR, /* has cp15 VBAR */
>>>      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>>>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>>> +    ARM_FEATURE_SPE, /* has SPE support */
>>>  };
>>>
>>>  static inline int arm_feature(CPUARMState *env, int feature)
>>>
>>
> 

Re: [PATCH 4/7] target/arm/kvm: spe: Unify device attr operatioin helper

[Andrew Jones]

On Mon, Aug 10, 2020 at 10:48:41AM +0800, Haibo Xu wrote:
> On Fri, 7 Aug 2020 at 16:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 8/7/20 10:10 AM, Haibo Xu wrote:
> > > Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(),
> >
> > Maybe rename kvm_arm_device_set_attr() to match the structure
> > name?
> >
> 
> Thanks for the review! I will update it in the next version.

I've already renamed it to kvm_arm_set_device_attr() in [1]. Also, it's
not enough to just rename the function. The error messages the function
may generate have "PMU" embedded in them.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg727590.html

Thanks,
drew

> 
> > > So both the vPMU and vSPE device can share the same API.
> > >
> > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> >
> > Regardless, with the typo "operation" in patch subject fixed:
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >
> > > ---
> > >  target/arm/kvm64.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > > index 1169237905..75a417d65c 100644
> > > --- a/target/arm/kvm64.c
> > > +++ b/target/arm/kvm64.c
> > > @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
> > >      return NULL;
> > >  }
> > >
> > > -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
> > > +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr)
> > >  {
> > >      int err;
> > >
> > > @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs)
> > >      if (!ARM_CPU(cs)->has_pmu) {
> > >          return;
> > >      }
> > > -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> > > +    if (!kvm_arm_dev_set_attr(cs, &attr)) {
> > >          error_report("failed to init PMU");
> > >          abort();
> > >      }
> > > @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> > >      if (!ARM_CPU(cs)->has_pmu) {
> > >          return;
> > >      }
> > > -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> > > +    if (!kvm_arm_dev_set_attr(cs, &attr)) {
> > >          error_report("failed to set irq for PMU");
> > >          abort();
> > >      }
> > >
> >
> 

Re: [PATCH 0/7] target/arm: Add vSPE support to KVM guest

[Andrew Jones]

On Fri, Aug 07, 2020 at 08:10:30AM +0000, Haibo Xu wrote:
> This series add support for SPE(Statistical Profiling Extension)[1]
> in KVM guest. It's based on Andrew Murray's kernel KVM patches V2[2],
> and has been tested to ensure that guest can use SPE with valid data.
> E.g.
> 
> In host:
> $ ./qemu-system-aarch64 \
>         -cpu host -M virt,accel=kvm,gic-version=3 -nographic -m 2048M \
>         -kernel ./Image-new \
>         -initrd /boot/initrd.img-5.6.0-rc2+ \
>         -append "root=/dev/vda rw console=ttyAMA0" -nodefaults -serial stdio\
>         -drive if=none,file=./xenial.rootfs.ext4,id=hd0,format=raw \
>         -device virtio-blk-device,drive=hd0  \
> 
> In guest:
> $ perf record -e arm_spe/ts_enable=1,pa_enable=1,pct_enable=1/ \
>         dd if=/dev/zero of=/dev/null count=1000
> $ perf report --dump-raw-trace > spe_buf.txt
> 
> The spe_buf.txt should contain similar data as below:
> 
> . ... ARM SPE data: size 135944 bytes
> .  00000000:  b0 f4 d3 29 10 00 80 ff a0                      PC 0xff80001029d3f4 el1 ns=1
> .  00000009:  99 0b 00                                        LAT 11 ISSUE
> .  0000000c:  98 0d 00                                        LAT 13 TOT
> .  0000000f:  52 16 00                                        EV RETIRED L1D-ACCESS TLB-ACCESS
> .  00000012:  49 00                                           LD
> .  00000014:  b2 d0 40 d8 70 00 00 ff 00                      VA 0xff000070d840d0
> .  0000001d:  9a 01 00                                        LAT 1 XLAT
> .  00000020:  00 00 00                                        PAD
> .  00000023:  71 a5 1f b3 20 14 00 00 00                      TS 86447955877
> .  0000002c:  b0 7c f9 29 10 00 80 ff a0                      PC 0xff80001029f97c el1 ns=1
> .  00000035:  99 02 00                                        LAT 2 ISSUE
> .  00000038:  98 03 00                                        LAT 3 TOT
> .  0000003b:  52 02 00                                        EV RETIRED
> .  0000003e:  48 00                                           INSN-OTHER
> .  00000040:  00 00 00                                        PAD
> .  00000043:  71 ef 1f b3 20 14 00 00 00                      TS 86447955951
> .  0000004c:  b0 f0 e9 29 10 00 80 ff a0                      PC 0xff80001029e9f0 el1 ns=1
> .  00000055:  99 02 00                                        LAT 2 ISSUE
> .  00000058:  98 03 00                                        LAT 3 TOT
> .  0000005b:  52 02 00                                        EV RETIRED
> 
> If you want to disable the vSPE support, you can use the 'spe=off' cpu
> property:
> 
> ./qemu-system-aarch64 \
>         -cpu host,spe=off -M virt,accel=kvm,gic-version=3 -nographic -m 2048M \
>         -kernel ./Image-new \
>         -initrd /boot/initrd.img-5.6.0-rc2+ \
>         -append "root=/dev/vda rw console=ttyAMA0" -nodefaults -serial stdio\
>         -drive if=none,file=./xenial.rootfs.ext4,id=hd0,format=raw \
>         -device virtio-blk-device,drive=hd0  \
> 
> Note:
> (1) Since the kernel patches are still under review, some of the macros
>     in the header files may be changed after merging. We may need to
>     update them accordingly.
> (2) These patches only add vSPE support in KVM mode, for TCG mode, I'm
>     not sure whether we need to support it.
> (3) Just followed the 'pmu' property, we only allow this feature to be
>     removed from CPUs which enable it by default. But since the SPE is
>     an optional feature extension for Armv8.2, I think a better way may
>     be to disable it by default, and only enable it when the host cpu
>     do have the feature.

* When not using KVM, if TCG supports SPE emulation, then it should be
  enabled by default with '-cpu max'.

* When using KVM, if KVM supports the feature, then it should be enabled
  by default with '-cpu max' or '-cpu host' (which are currently the only
  ways to use KVM on AArch64)

* When using KVM, if KVM does not support the feature (either the CPU
  doesn't support it, or the version of KVM in use doesn't know what to
  do with it), then the feature should obviously be disabled for the VM
  by default, since there's no way to enable it.

> 
> [1]https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/
>    posts/statistical-profiling-extension-for-armv8-a
> [2]https://www.spinics.net/lists/arm-kernel/msg776228.html
> 
> Haibo Xu (7):
>   update Linux headers with new vSPE macros
>   target/arm/kvm: spe: Add helper to detect SPE when using KVM
>   target/arm/cpu: spe: Add an option to turn on/off vSPE support
>   target/arm/kvm: spe: Unify device attr operatioin helper
>   target/arm/kvm: spe: Add device init and set_irq operations
>   hw/arm/virt: spe: Add SPE fdt binding for virt machine
>   target/arm/cpu: spe: Enable spe to work with host cpu
> 
>  hw/arm/virt-acpi-build.c      |  3 +++
>  hw/arm/virt.c                 | 42 ++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-defs.h   |  1 +
>  include/hw/arm/virt.h         |  1 +
>  linux-headers/asm-arm64/kvm.h |  4 +++
>  linux-headers/linux/kvm.h     |  2 ++
>  target/arm/cpu.c              | 34 +++++++++++++++++++++++++
>  target/arm/cpu.h              |  5 ++++
>  target/arm/kvm.c              | 11 ++++++++
>  target/arm/kvm64.c            | 48 ++++++++++++++++++++++++++++++++---
>  target/arm/kvm_arm.h          | 18 +++++++++++++

This series should also be updating these files:

 docs/system/arm/cpu-features.rst
 target/arm/monitor.c
 tests/qtest/arm-cpu-features.c

Thanks,
drew

>  11 files changed, 166 insertions(+), 3 deletions(-)
> 
> -- 
> 2.17.1
> 
> 

Re: [PATCH 3/7] target/arm/cpu: spe: Add an option to turn on/off vSPE support

[Andrew Jones]

On Fri, Aug 07, 2020 at 08:10:33AM +0000, Haibo Xu wrote:
> Adds a spe=[on/off] option to enable/disable vSPE support in
> guest vCPU. Note this option is only available for "-cpu host"
> with KVM mode, and default value is on. 
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/cpu.c | 28 ++++++++++++++++++++++++++++
>  target/arm/cpu.h |  3 +++ 
>  2 files changed, 31 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 111579554f..40768b4d19 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1122,6 +1122,29 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>      cpu->has_pmu = value;
>  }
>  
> +static bool arm_get_spe(Object *obj, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    return cpu->has_spe;
> +}
> +
> +static void arm_set_spe(Object *obj, bool value, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    if (value) {
> +        if (kvm_enabled() && !kvm_arm_spe_supported()) {
> +            error_setg(errp, "'spe' feature not supported by KVM on this host");
> +            return;
> +        }
> +        set_feature(&cpu->env, ARM_FEATURE_SPE);
> +    } else {
> +        unset_feature(&cpu->env, ARM_FEATURE_SPE);

See below, we should be using ID register bits instead.

> +    }
> +    cpu->has_spe = value;
> +}
> +
>  unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
>  {
>      /*
> @@ -1195,6 +1218,11 @@ void arm_cpu_post_init(Object *obj)
>          object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
>      }
> 
> +    if (arm_feature(&cpu->env, ARM_FEATURE_SPE)) {
> +        cpu->has_spe = true;
> +        object_property_add_bool(obj, "spe", arm_get_spe, arm_set_spe);

The property should be available even when the host doesn't support the
feature. How else can one migrate from a host where the feature is
available, but disabled, to a host that doesn't support the feature?

> +    }
> +
>      /*
>       * Allow user to turn off VFP and Neon support, but only for TCG --
>       * KVM does not currently allow us to lie to the guest about its
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9e8ed423ea..fe0ac14386 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -822,6 +822,8 @@ struct ARMCPU {
>      bool has_el3;
>      /* CPU has PMU (Performance Monitor Unit) */
>      bool has_pmu;
> +    /* CPU has SPE (Statistical Profiling Extension) */
> +    bool has_spe;
>      /* CPU has VFP */
>      bool has_vfp;
>      /* CPU has Neon */
> @@ -1959,6 +1961,7 @@ enum arm_features {
>      ARM_FEATURE_VBAR, /* has cp15 VBAR */
>      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
> +    ARM_FEATURE_SPE, /* has SPE support */

We shouldn't need to add this feature bit. SPE should have an ID register
bit to use instead.

>  };
> 
>  static inline int arm_feature(CPUARMState *env, int feature)
> -- 
> 2.17.1
> 
>

Thanks,
drew 

Re: [PATCH 6/7] hw/arm/virt: spe: Add SPE fdt binding for virt machine

[Andrew Jones]

On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote:
> Add a virtual SPE device for virt machine while using PPI 
> 5 for SPE overflow interrupt number.

Any reason PPI 5 was selected?

> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  hw/arm/virt-acpi-build.c    |  3 +++ 
>  hw/arm/virt.c               | 42 +++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-defs.h |  1 + 
>  include/hw/arm/virt.h       |  1 + 
>  target/arm/cpu.c            |  2 ++
>  target/arm/cpu.h            |  2 ++
>  target/arm/kvm.c            |  6 ++++++
>  7 files changed, 57 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 91f0df7b13..5073ba22a5 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>          }
> +        if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
> +            gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ));
> +        }
>          if (vms->virt) {
>              gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ));
>          }
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ecfee362a1..c40819705d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
>      }
>  }
> 
> +static void fdt_add_spe_nodes(const VirtMachineState *vms)
> +{
> +    CPUState *cpu;
> +    ARMCPU *armcpu;
> +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> +
> +    CPU_FOREACH(cpu) {
> +        armcpu = ARM_CPU(cpu);
> +        if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
> +            return;
> +        }
> +        if (kvm_enabled()) {
> +            if (kvm_irqchip_in_kernel()) {
> +                kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ));
> +            }
> +            kvm_arm_spe_init(cpu);
> +        }
> +    }
> +
> +    if (vms->gic_version == VIRT_GIC_VERSION_2) {
> +        irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> +                             GIC_FDT_IRQ_PPI_CPU_WIDTH,
> +                             (1 << vms->smp_cpus) - 1);
> +    }
> +
> +    armcpu = ARM_CPU(qemu_get_cpu(0));
> +    qemu_fdt_add_subnode(vms->fdt, "/spe");
> +    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> +        const char compat[] = "arm,statistical-profiling-extension-v1";
> +        qemu_fdt_setprop(vms->fdt, "/spe", "compatible",
> +                         compat, sizeof(compat));
> +        qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts",
> +                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags);
> +    }
> +}

Please look at [1]. I've refactored fdt_add_pmu_nodes(), so it no longer
makes a good pattern for this function.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg727588.html


> +
>  static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
>  {
>      DeviceState *dev;
> @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms)
>                                      qdev_get_gpio_in(vms->gic, ppibase
>                                                       + VIRTUAL_PMU_IRQ));
> 
> +        qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0,
> +                                    qdev_get_gpio_in(vms->gic, ppibase
> +                                                     + VIRTUAL_SPE_IRQ));
> +
>          sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
>          sysbus_connect_irq(gicbusdev, i + smp_cpus,
>                             qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
> @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine)
> 
>      fdt_add_pmu_nodes(vms);
> 
> +    fdt_add_spe_nodes(vms);
> +

You didn't add any compat code, which means all virt machine types are now
getting an SPE FDT node, ACPI table change, and, most importantly, PPI 5
has gone from unallocated to allocated. We definitely need compat code.

>      create_uart(vms, VIRT_UART, sysmem, serial_hd(0));
> 
>      if (vms->secure) {
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 38a42f409a..56a7f38ae4 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface {
>      uint32_t vgic_interrupt;
>      uint64_t gicr_base_address;
>      uint64_t arm_mpidr;
> +    uint16_t spe_interrupt; /* ACPI 6.3 */
>  } QEMU_PACKED;
> 
>  typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface;
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index dff67e1bef..56c83224d2 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -49,6 +49,7 @@
>  #define ARCH_TIMER_NS_EL1_IRQ 14
>  #define ARCH_TIMER_NS_EL2_IRQ 10
> 
> +#define VIRTUAL_SPE_IRQ 5
>  #define VIRTUAL_PMU_IRQ 7
> 
>  #define PPI(irq) ((irq) + 16)
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 40768b4d19..67ab0089fd 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj)
>                               "gicv3-maintenance-interrupt", 1);
>      qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt,
>                               "pmu-interrupt", 1);
> +    qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt,
> +                             "spe-interrupt", 1);
>  #endif
> 
>      /* DTB consumers generally don't in fact care what the 'compatible'
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index fe0ac14386..4bf8591df8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -790,6 +790,8 @@ struct ARMCPU {
>      qemu_irq gicv3_maintenance_interrupt;
>      /* GPIO output for the PMU interrupt */
>      qemu_irq pmu_interrupt;
> +    /* GPIO output for the SPE interrupt */
> +    qemu_irq spe_interrupt;
> 
>      /* MemoryRegion to use for secure physical accesses */
>      MemoryRegion *secure_memory;
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 58f991e890..ecafdda364 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>              switched_level &= ~KVM_ARM_DEV_PMU;
>          }
> 
> +        if (switched_level & KVM_ARM_DEV_SPE) {
> +            qemu_set_irq(cpu->spe_interrupt,
> +                         !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE));
> +            switched_level &= ~KVM_ARM_DEV_SPE;
> +        }
> +

Did you test with a userspace irqchip?

>          if (switched_level) {
>              qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n",
>                            __func__, switched_level);
> -- 
> 2.17.1
> 
>

Thanks,
drew

Re: [PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Andrew Jones]

On Fri, Aug 07, 2020 at 08:10:37AM +0000, Haibo Xu wrote:
> Turn on the spe cpu property by default when working with host
> cpu type in KVM mode, i.e. we can now do '-cpu host' to add the 
> vSPE, and '-cpu host,spe=off' to remove it. 

-cpu max with KVM should also enable it by default

> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/cpu.c   | 4 ++++
>  target/arm/kvm64.c | 9 +++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 67ab0089fd..42fa99953c 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1719,6 +1719,10 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          cpu->pmceid1 = 0;
>      }   
>  
> +    if (!cpu->has_spe || !kvm_enabled()) {
> +        unset_feature(env, ARM_FEATURE_SPE);
> +    }

I don't think this should be necessary.

> +
>      if (!arm_feature(env, ARM_FEATURE_EL2)) {
>          /* Disable the hypervisor feature bits in the processor feature
>           * registers if we don't have EL2. These are id_pfr1[15:12] and
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index be045ccc5f..4ea58afc1d 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -679,6 +679,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>      features |= 1ULL << ARM_FEATURE_AARCH64;
>      features |= 1ULL << ARM_FEATURE_PMU;
>      features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
> +    features |= 1ULL << ARM_FEATURE_SPE;

No, SPE is not a feature we assume is present in v8.0 CPUs.

> 
>      ahcf->features = features;
> 
> @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      } else {
>          env->features &= ~(1ULL << ARM_FEATURE_PMU);
>      }
> +    if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) {
> +        cpu->has_spe = false;
> +    }
> +    if (cpu->has_spe) {
> +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1;
> +    } else {
> +        env->features &= ~(1ULL << ARM_FEATURE_SPE);
> +    }

The PMU code above this isn't a good pattern to copy. The SVE code below
is better. SVE uses an ID bit and doesn't do the redundant KVM cap check.
It'd be nice to cleanup the PMU code (with a separate patch) and then add
SPE in a better way.

>      if (cpu_isar_feature(aa64_sve, cpu)) {
>          assert(kvm_arm_sve_supported());
>          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> -- 
> 2.17.1
> 

Thanks,
drew

Re: [PATCH 4/7] target/arm/kvm: spe: Unify device attr operatioin helper

[Haibo Xu]

On Mon, 10 Aug 2020 at 18:29, Andrew Jones <drjones@redhat.com> wrote:
>
> On Mon, Aug 10, 2020 at 10:48:41AM +0800, Haibo Xu wrote:
> > On Fri, 7 Aug 2020 at 16:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > >
> > > On 8/7/20 10:10 AM, Haibo Xu wrote:
> > > > Rename kvm_arm_pmu_set_attr() to kvm_arm_dev_set_attr(),
> > >
> > > Maybe rename kvm_arm_device_set_attr() to match the structure
> > > name?
> > >
> >
> > Thanks for the review! I will update it in the next version.
>
> I've already renamed it to kvm_arm_set_device_attr() in [1]. Also, it's
> not enough to just rename the function. The error messages the function
> may generate have "PMU" embedded in them.
>
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg727590.html
>
> Thanks,
> drew
>

Thanks for your review, Andrew!
Will rebase on your patches in the next version.

> >
> > > > So both the vPMU and vSPE device can share the same API.
> > > >
> > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> > >
> > > Regardless, with the typo "operation" in patch subject fixed:
> > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > >
> > > > ---
> > > >  target/arm/kvm64.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > > > index 1169237905..75a417d65c 100644
> > > > --- a/target/arm/kvm64.c
> > > > +++ b/target/arm/kvm64.c
> > > > @@ -398,7 +398,7 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
> > > >      return NULL;
> > > >  }
> > > >
> > > > -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
> > > > +static bool kvm_arm_dev_set_attr(CPUState *cs, struct kvm_device_attr *attr)
> > > >  {
> > > >      int err;
> > > >
> > > > @@ -427,7 +427,7 @@ void kvm_arm_pmu_init(CPUState *cs)
> > > >      if (!ARM_CPU(cs)->has_pmu) {
> > > >          return;
> > > >      }
> > > > -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> > > > +    if (!kvm_arm_dev_set_attr(cs, &attr)) {
> > > >          error_report("failed to init PMU");
> > > >          abort();
> > > >      }
> > > > @@ -444,7 +444,7 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> > > >      if (!ARM_CPU(cs)->has_pmu) {
> > > >          return;
> > > >      }
> > > > -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> > > > +    if (!kvm_arm_dev_set_attr(cs, &attr)) {
> > > >          error_report("failed to set irq for PMU");
> > > >          abort();
> > > >      }
> > > >
> > >
> >
>

Re: [PATCH 6/7] hw/arm/virt: spe: Add SPE fdt binding for virt machine

[Haibo Xu]

On Mon, 10 Aug 2020 at 19:05, Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote:
> > Add a virtual SPE device for virt machine while using PPI
> > 5 for SPE overflow interrupt number.
>
> Any reason PPI 5 was selected?
>

No special reason to choose PPI 5. Just re-use the setting in kvmtool.

> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> > ---
> >  hw/arm/virt-acpi-build.c    |  3 +++
> >  hw/arm/virt.c               | 42 +++++++++++++++++++++++++++++++++++++
> >  include/hw/acpi/acpi-defs.h |  1 +
> >  include/hw/arm/virt.h       |  1 +
> >  target/arm/cpu.c            |  2 ++
> >  target/arm/cpu.h            |  2 ++
> >  target/arm/kvm.c            |  6 ++++++
> >  7 files changed, 57 insertions(+)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 91f0df7b13..5073ba22a5 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >          if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> >              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> >          }
> > +        if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
> > +            gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ));
> > +        }
> >          if (vms->virt) {
> >              gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ));
> >          }
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index ecfee362a1..c40819705d 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> >      }
> >  }
> >
> > +static void fdt_add_spe_nodes(const VirtMachineState *vms)
> > +{
> > +    CPUState *cpu;
> > +    ARMCPU *armcpu;
> > +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> > +
> > +    CPU_FOREACH(cpu) {
> > +        armcpu = ARM_CPU(cpu);
> > +        if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
> > +            return;
> > +        }
> > +        if (kvm_enabled()) {
> > +            if (kvm_irqchip_in_kernel()) {
> > +                kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ));
> > +            }
> > +            kvm_arm_spe_init(cpu);
> > +        }
> > +    }
> > +
> > +    if (vms->gic_version == VIRT_GIC_VERSION_2) {
> > +        irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> > +                             GIC_FDT_IRQ_PPI_CPU_WIDTH,
> > +                             (1 << vms->smp_cpus) - 1);
> > +    }
> > +
> > +    armcpu = ARM_CPU(qemu_get_cpu(0));
> > +    qemu_fdt_add_subnode(vms->fdt, "/spe");
> > +    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> > +        const char compat[] = "arm,statistical-profiling-extension-v1";
> > +        qemu_fdt_setprop(vms->fdt, "/spe", "compatible",
> > +                         compat, sizeof(compat));
> > +        qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts",
> > +                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags);
> > +    }
> > +}
>
> Please look at [1]. I've refactored fdt_add_pmu_nodes(), so it no longer
> makes a good pattern for this function.
>
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg727588.html
>
>

Ok, I will spend some time studying your patches, and rework the related patches
in the next version.

> > +
> >  static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> >  {
> >      DeviceState *dev;
> > @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms)
> >                                      qdev_get_gpio_in(vms->gic, ppibase
> >                                                       + VIRTUAL_PMU_IRQ));
> >
> > +        qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0,
> > +                                    qdev_get_gpio_in(vms->gic, ppibase
> > +                                                     + VIRTUAL_SPE_IRQ));
> > +
> >          sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
> >          sysbus_connect_irq(gicbusdev, i + smp_cpus,
> >                             qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
> > @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine)
> >
> >      fdt_add_pmu_nodes(vms);
> >
> > +    fdt_add_spe_nodes(vms);
> > +
>
> You didn't add any compat code, which means all virt machine types are now
> getting an SPE FDT node, ACPI table change, and, most importantly, PPI 5
> has gone from unallocated to allocated. We definitely need compat code.
>

So the 'compat code' here means to only add the SPE node in KVM mode?

> >      create_uart(vms, VIRT_UART, sysmem, serial_hd(0));
> >
> >      if (vms->secure) {
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 38a42f409a..56a7f38ae4 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface {
> >      uint32_t vgic_interrupt;
> >      uint64_t gicr_base_address;
> >      uint64_t arm_mpidr;
> > +    uint16_t spe_interrupt; /* ACPI 6.3 */
> >  } QEMU_PACKED;
> >
> >  typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface;
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index dff67e1bef..56c83224d2 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -49,6 +49,7 @@
> >  #define ARCH_TIMER_NS_EL1_IRQ 14
> >  #define ARCH_TIMER_NS_EL2_IRQ 10
> >
> > +#define VIRTUAL_SPE_IRQ 5
> >  #define VIRTUAL_PMU_IRQ 7
> >
> >  #define PPI(irq) ((irq) + 16)
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 40768b4d19..67ab0089fd 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj)
> >                               "gicv3-maintenance-interrupt", 1);
> >      qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt,
> >                               "pmu-interrupt", 1);
> > +    qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt,
> > +                             "spe-interrupt", 1);
> >  #endif
> >
> >      /* DTB consumers generally don't in fact care what the 'compatible'
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index fe0ac14386..4bf8591df8 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -790,6 +790,8 @@ struct ARMCPU {
> >      qemu_irq gicv3_maintenance_interrupt;
> >      /* GPIO output for the PMU interrupt */
> >      qemu_irq pmu_interrupt;
> > +    /* GPIO output for the SPE interrupt */
> > +    qemu_irq spe_interrupt;
> >
> >      /* MemoryRegion to use for secure physical accesses */
> >      MemoryRegion *secure_memory;
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 58f991e890..ecafdda364 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
> >              switched_level &= ~KVM_ARM_DEV_PMU;
> >          }
> >
> > +        if (switched_level & KVM_ARM_DEV_SPE) {
> > +            qemu_set_irq(cpu->spe_interrupt,
> > +                         !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE));
> > +            switched_level &= ~KVM_ARM_DEV_SPE;
> > +        }
> > +
>
> Did you test with a userspace irqchip?

No, I just tested with an in-kernel irqchip.
Actually, the current kernel vSPE patch doesn't support a userspace irqchip.
AFAIK, the userspace irqchip support should be ready in the next
kernel patch series
which will be sent out for review in the middle of September.

>
> >          if (switched_level) {
> >              qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n",
> >                            __func__, switched_level);
> > --
> > 2.17.1
> >
> >
>
> Thanks,
> drew
>

Re: [PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Haibo Xu]

[-- Attachment #1: Type: text/plain, Size: 3345 bytes --]

On Mon, 10 Aug 2020 at 19:16, Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Aug 07, 2020 at 08:10:37AM +0000, Haibo Xu wrote:
> > Turn on the spe cpu property by default when working with host
> > cpu type in KVM mode, i.e. we can now do '-cpu host' to add the
> > vSPE, and '-cpu host,spe=off' to remove it.
>
> -cpu max with KVM should also enable it by default
>

Ok, will fix it!

> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> > ---
> >  target/arm/cpu.c   | 4 ++++
> >  target/arm/kvm64.c | 9 +++++++++
> >  2 files changed, 13 insertions(+)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 67ab0089fd..42fa99953c 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1719,6 +1719,10 @@ static void arm_cpu_realizefn(DeviceState *dev,
Error **errp)
> >          cpu->pmceid1 = 0;
> >      }
> >
> > +    if (!cpu->has_spe || !kvm_enabled()) {
> > +        unset_feature(env, ARM_FEATURE_SPE);
> > +    }
>
> I don't think this should be necessary.
>

Yes, I have tried to remove this check, and the vSPE can still work
correctly.
But I don't know whether there are some corner cases that trigger an error.
The similar logic is added in commit 929e754d5a to enable vPMU support.

> > +
> >      if (!arm_feature(env, ARM_FEATURE_EL2)) {
> >          /* Disable the hypervisor feature bits in the processor feature
> >           * registers if we don't have EL2. These are id_pfr1[15:12] and
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index be045ccc5f..4ea58afc1d 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -679,6 +679,7 @@ bool
kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> >      features |= 1ULL << ARM_FEATURE_AARCH64;
> >      features |= 1ULL << ARM_FEATURE_PMU;
> >      features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
> > +    features |= 1ULL << ARM_FEATURE_SPE;
>
> No, SPE is not a feature we assume is present in v8.0 CPUs.
>

Yes, SPE is an optional feature for v8.2. How about changing to the
following logic:

spe_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SPE_V1)
> 0;
if (spe_supported) {
    features |= 1ULL << ARM_FEATURE_SPE;
}

> >
> >      ahcf->features = features;
> >
> > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >      } else {
> >          env->features &= ~(1ULL << ARM_FEATURE_PMU);
> >      }
> > +    if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) {
> > +        cpu->has_spe = false;
> > +    }
> > +    if (cpu->has_spe) {
> > +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1;
> > +    } else {
> > +        env->features &= ~(1ULL << ARM_FEATURE_SPE);
> > +    }
>
> The PMU code above this isn't a good pattern to copy. The SVE code below
> is better. SVE uses an ID bit and doesn't do the redundant KVM cap check.
> It'd be nice to cleanup the PMU code (with a separate patch) and then add
> SPE in a better way.
>

I noticed that Peter had sent out a mail
<https://www.mail-archive.com/qemu-devel@nongnu.org/msg727640.html> to talk
about the feature-identification strategy.
So shall we adapt it to the vPMU and vSPE feature?

> >      if (cpu_isar_feature(aa64_sve, cpu)) {
> >          assert(kvm_arm_sve_supported());
> >          cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SVE;
> > --
> > 2.17.1
> >
>
> Thanks,
> drew
>

[-- Attachment #2: Type: text/html, Size: 4393 bytes --]

Re: [PATCH 6/7] hw/arm/virt: spe: Add SPE fdt binding for virt machine

[Andrew Jones]

On Tue, Aug 11, 2020 at 10:38:02AM +0800, Haibo Xu wrote:
> On Mon, 10 Aug 2020 at 19:05, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote:
> > > Add a virtual SPE device for virt machine while using PPI
> > > 5 for SPE overflow interrupt number.
> >
> > Any reason PPI 5 was selected?
> >
> 
> No special reason to choose PPI 5. Just re-use the setting in kvmtool.

Please write in the commit message that kvmtool has already selected PPI 5
for this purpose.

> > > +    fdt_add_spe_nodes(vms);
> > > +
> >
> > You didn't add any compat code, which means all virt machine types are now
> > getting an SPE FDT node, ACPI table change, and, most importantly, PPI 5
> > has gone from unallocated to allocated. We definitely need compat code.
> >
> 
> So the 'compat code' here means to only add the SPE node in KVM mode?

No, it means only add it for the 5.2 and later machine types. You'll see
what I mean when you study the patchset I pointed out, which is also only
for 5.2 and later machine types.

> > > +        if (switched_level & KVM_ARM_DEV_SPE) {
> > > +            qemu_set_irq(cpu->spe_interrupt,
> > > +                         !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE));
> > > +            switched_level &= ~KVM_ARM_DEV_SPE;
> > > +        }
> > > +
> >
> > Did you test with a userspace irqchip?
> 
> No, I just tested with an in-kernel irqchip.
> Actually, the current kernel vSPE patch doesn't support a userspace irqchip.
> AFAIK, the userspace irqchip support should be ready in the next
> kernel patch series
> which will be sent out for review in the middle of September.

It probably doesn't hurt to do the above hunk already, hoping it will just
work when it's possible to test, but I generally prefer only adding tested
code. Maybe this hunk should be a separate patch with a commit message
explaining that it's untested?

Thanks,
drew

Re: [PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Andrew Jones]

On Tue, Aug 11, 2020 at 11:15:42AM +0800, Haibo Xu wrote:
> > > +    if (!cpu->has_spe || !kvm_enabled()) {
> > > +        unset_feature(env, ARM_FEATURE_SPE);
> > > +    }
> >
> > I don't think this should be necessary.
> >
> 
> Yes, I have tried to remove this check, and the vSPE can still work
> correctly.
> But I don't know whether there are some corner cases that trigger an error.
> The similar logic is added in commit 929e754d5a to enable vPMU support.

I think the PMU logic needs a cleanup, rather than to be imitated.

> 
> > > +
> > >      if (!arm_feature(env, ARM_FEATURE_EL2)) {
> > >          /* Disable the hypervisor feature bits in the processor feature
> > >           * registers if we don't have EL2. These are id_pfr1[15:12] and
> > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > > index be045ccc5f..4ea58afc1d 100644
> > > --- a/target/arm/kvm64.c
> > > +++ b/target/arm/kvm64.c
> > > @@ -679,6 +679,7 @@ bool
> kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> > >      features |= 1ULL << ARM_FEATURE_AARCH64;
> > >      features |= 1ULL << ARM_FEATURE_PMU;
> > >      features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
> > > +    features |= 1ULL << ARM_FEATURE_SPE;
> >
> > No, SPE is not a feature we assume is present in v8.0 CPUs.
> >
> 
> Yes, SPE is an optional feature for v8.2. How about changing to the
> following logic:
> 
> spe_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SPE_V1)
> > 0;
> if (spe_supported) {
>     features |= 1ULL << ARM_FEATURE_SPE;
> }

Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID
register bit instead like "sve_supported" does.

> 
> > >
> > >      ahcf->features = features;
> > >
> > > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > >      } else {
> > >          env->features &= ~(1ULL << ARM_FEATURE_PMU);
> > >      }
> > > +    if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) {
> > > +        cpu->has_spe = false;
> > > +    }
> > > +    if (cpu->has_spe) {
> > > +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1;
> > > +    } else {
> > > +        env->features &= ~(1ULL << ARM_FEATURE_SPE);
> > > +    }
> >
> > The PMU code above this isn't a good pattern to copy. The SVE code below
> > is better. SVE uses an ID bit and doesn't do the redundant KVM cap check.
> > It'd be nice to cleanup the PMU code (with a separate patch) and then add
> > SPE in a better way.
> >
> 
> I noticed that Peter had sent out a mail
> <https://www.mail-archive.com/qemu-devel@nongnu.org/msg727640.html> to talk
> about the feature-identification strategy.
> So shall we adapt it to the vPMU and vSPE feature?

At least SPE. You'll have to double check that it makes sense to do for
PMU. But, if so, then it should be done with a separate series.

Thanks,
drew

Re: [PATCH 6/7] hw/arm/virt: spe: Add SPE fdt binding for virt machine

[Haibo Xu]

[-- Attachment #1: Type: text/plain, Size: 2288 bytes --]

On Wed, 12 Aug 2020 at 00:40, Andrew Jones <drjones@redhat.com> wrote:

> On Tue, Aug 11, 2020 at 10:38:02AM +0800, Haibo Xu wrote:
> > On Mon, 10 Aug 2020 at 19:05, Andrew Jones <drjones@redhat.com> wrote:
> > >
> > > On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote:
> > > > Add a virtual SPE device for virt machine while using PPI
> > > > 5 for SPE overflow interrupt number.
> > >
> > > Any reason PPI 5 was selected?
> > >
> >
> > No special reason to choose PPI 5. Just re-use the setting in kvmtool.
>
> Please write in the commit message that kvmtool has already selected PPI 5
> for this purpose.
>

Ok, will fix it.


> > > > +    fdt_add_spe_nodes(vms);
> > > > +
> > >
> > > You didn't add any compat code, which means all virt machine types are
> now
> > > getting an SPE FDT node, ACPI table change, and, most importantly, PPI
> 5
> > > has gone from unallocated to allocated. We definitely need compat code.
> > >
> >
> > So the 'compat code' here means to only add the SPE node in KVM mode?
>
> No, it means only add it for the 5.2 and later machine types. You'll see
> what I mean when you study the patchset I pointed out, which is also only
> for 5.2 and later machine types.
>

Ok, thanks for the clarification!


> > > > +        if (switched_level & KVM_ARM_DEV_SPE) {
> > > > +            qemu_set_irq(cpu->spe_interrupt,
> > > > +                         !!(run->s.regs.device_irq_level &
> KVM_ARM_DEV_SPE));
> > > > +            switched_level &= ~KVM_ARM_DEV_SPE;
> > > > +        }
> > > > +
> > >
> > > Did you test with a userspace irqchip?
> >
> > No, I just tested with an in-kernel irqchip.
> > Actually, the current kernel vSPE patch doesn't support a userspace
> irqchip.
> > AFAIK, the userspace irqchip support should be ready in the next
> > kernel patch series
> > which will be sent out for review in the middle of September.
>
> It probably doesn't hurt to do the above hunk already, hoping it will just
> work when it's possible to test, but I generally prefer only adding tested
> code. Maybe this hunk should be a separate patch with a commit message
> explaining that it's untested?
>

Good idea! I will drop the hunk in this series, and send out a separate
patch to enable it
once the kernel support is ready!


> Thanks,
> drew
>
>

[-- Attachment #2: Type: text/html, Size: 3542 bytes --]

Re: [PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Haibo Xu]

On Wed, 12 Aug 2020 at 00:50, Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Aug 11, 2020 at 11:15:42AM +0800, Haibo Xu wrote:
> > > > +    if (!cpu->has_spe || !kvm_enabled()) {
> > > > +        unset_feature(env, ARM_FEATURE_SPE);
> > > > +    }
> > >
> > > I don't think this should be necessary.
> > >
> >
> > Yes, I have tried to remove this check, and the vSPE can still work
> > correctly.
> > But I don't know whether there are some corner cases that trigger an error.
> > The similar logic is added in commit 929e754d5a to enable vPMU support.
>
> I think the PMU logic needs a cleanup, rather than to be imitated.
>
> >
> > > > +
> > > >      if (!arm_feature(env, ARM_FEATURE_EL2)) {
> > > >          /* Disable the hypervisor feature bits in the processor feature
> > > >           * registers if we don't have EL2. These are id_pfr1[15:12] and
> > > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > > > index be045ccc5f..4ea58afc1d 100644
> > > > --- a/target/arm/kvm64.c
> > > > +++ b/target/arm/kvm64.c
> > > > @@ -679,6 +679,7 @@ bool
> > kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
> > > >      features |= 1ULL << ARM_FEATURE_AARCH64;
> > > >      features |= 1ULL << ARM_FEATURE_PMU;
> > > >      features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
> > > > +    features |= 1ULL << ARM_FEATURE_SPE;
> > >
> > > No, SPE is not a feature we assume is present in v8.0 CPUs.
> > >
> >
> > Yes, SPE is an optional feature for v8.2. How about changing to the
> > following logic:
> >
> > spe_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SPE_V1)
> > > 0;
> > if (spe_supported) {
> >     features |= 1ULL << ARM_FEATURE_SPE;
> > }
>
> Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID
> register bit instead like "sve_supported" does.
>
> >
> > > >
> > > >      ahcf->features = features;
> > > >
> > > > @@ -826,6 +827,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > > >      } else {
> > > >          env->features &= ~(1ULL << ARM_FEATURE_PMU);
> > > >      }
> > > > +    if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_SPE_V1)) {
> > > > +        cpu->has_spe = false;
> > > > +    }
> > > > +    if (cpu->has_spe) {
> > > > +        cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_SPE_V1;
> > > > +    } else {
> > > > +        env->features &= ~(1ULL << ARM_FEATURE_SPE);
> > > > +    }
> > >
> > > The PMU code above this isn't a good pattern to copy. The SVE code below
> > > is better. SVE uses an ID bit and doesn't do the redundant KVM cap check.
> > > It'd be nice to cleanup the PMU code (with a separate patch) and then add
> > > SPE in a better way.
> > >
> >
> > I noticed that Peter had sent out a mail
> > <https://www.mail-archive.com/qemu-devel@nongnu.org/msg727640.html> to talk
> > about the feature-identification strategy.
> > So shall we adapt it to the vPMU and vSPE feature?
>
> At least SPE. You'll have to double check that it makes sense to do for
> PMU. But, if so, then it should be done with a separate series.
>

Ok, will adapt the SPE support to this new feature-identification
strategy first, and
investigate whether it makes sense to do so for PMU later.

Thank you very much for helping review the patch series!

> Thanks,
> drew
>

Re: [PATCH 3/7] target/arm/cpu: spe: Add an option to turn on/off vSPE support

[Richard Henderson]

On 8/10/20 3:50 AM, Andrew Jones wrote:
>> @@ -1959,6 +1961,7 @@ enum arm_features {
>>      ARM_FEATURE_VBAR, /* has cp15 VBAR */
>>      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>> +    ARM_FEATURE_SPE, /* has SPE support */
> 
> We shouldn't need to add this feature bit. SPE should have an ID register
> bit to use instead.

Yes indeed: ID_AA64DFR0_EL1.PMSVer.


r~

Re: [PATCH 3/7] target/arm/cpu: spe: Add an option to turn on/off vSPE support

[Richard Henderson]

On 8/7/20 1:10 AM, Haibo Xu wrote:
> +static void arm_set_spe(Object *obj, bool value, Error **errp)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    if (value) {
> +        if (kvm_enabled() && !kvm_arm_spe_supported()) {
> +            error_setg(errp, "'spe' feature not supported by KVM on this host");
> +            return;
> +        }
> +        set_feature(&cpu->env, ARM_FEATURE_SPE);
> +    } else {
> +        unset_feature(&cpu->env, ARM_FEATURE_SPE);
> +    }
> +    cpu->has_spe = value;
> +}

I think you want to simply set cpu->has_spe here, and leave the adjustment of
ID_AA64DFR0 to a finalize routine.  Because there are multiple values that
PMSVer can take.

Once the get/set routines are only setting a flag on ARMCPU, you can use a
simpler property interface:

static Property arm_cpu_spe_property =
    DEFINE_PROP_BOOL("spe", ARMCPU, has_spe, true);

qdev_property_add_static(DEVICE(obj), &arm_cpu_spe_property);

The finalize routine would be called from arm_cpu_finalize_features(), much
like the existing arm_cpu_sve_finalize().

Since you're only registering the spe property when the selected cpu supports
spe, the finalize routine only needs to set PMSVer to 0 to turn it off,
preserving the initial enabled value of 1 or 2.


r~

Re: [PATCH 2/7] target/arm/kvm: spe: Add helper to detect SPE when using KVM

[Richard Henderson]

On 8/7/20 1:10 AM, Haibo Xu wrote:
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/kvm.c     |  5 +++++
>  target/arm/kvm_arm.h | 13 +++++++++++++
>  2 files changed, 18 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Richard Henderson]

On 8/11/20 9:49 AM, Andrew Jones wrote:
> Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID
> register bit instead like "sve_supported" does.

On a related note, I think we have a latent bug, or at least a mis-feature:

    sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0;
...
    /* Add feature bits that can't appear until after VCPU init. */
    if (sve_supported) {
        t = ahcf->isar.id_aa64pfr0;
        t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
        ahcf->isar.id_aa64pfr0 = t;
    }


Should it in fact be

    if (!sve_supported) {
        t = ahcf->isar.id_aa64pfr0;
        t = FIELD_DP64(t, ID_AA64PFR0, SVE, 0);
        ahcf->isar.id_aa64pfr0 = t;
    }

?

Forcing the value to 1 here is going to be wrong the moment we have an SVE2
enabled cpu.

Similarly, SPE has more than one "enabled" value for PMSVer.


r~

Re: [PATCH 7/7] target/arm/cpu: spe: Enable spe to work with host cpu

[Andrew Jones]

On Fri, Aug 14, 2020 at 12:28:25PM -0700, Richard Henderson wrote:
> On 8/11/20 9:49 AM, Andrew Jones wrote:
> > Yes, except you need to drop the ARM_FEATURE_SPE define and use the ID
> > register bit instead like "sve_supported" does.
> 
> On a related note, I think we have a latent bug, or at least a mis-feature:
> 
>     sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0;
> ...
>     /* Add feature bits that can't appear until after VCPU init. */
>     if (sve_supported) {
>         t = ahcf->isar.id_aa64pfr0;
>         t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
>         ahcf->isar.id_aa64pfr0 = t;
>     }
> 
> 
> Should it in fact be
> 
>     if (!sve_supported) {
>         t = ahcf->isar.id_aa64pfr0;
>         t = FIELD_DP64(t, ID_AA64PFR0, SVE, 0);
>         ahcf->isar.id_aa64pfr0 = t;
>     }
> 
> ?
> 
> Forcing the value to 1 here is going to be wrong the moment we have an SVE2
> enabled cpu.

Indeed. I think KVM should add a KVM_CAP_ARM_SVE2 cap. Then, we keep the
code similar to the way it is now, but set ID_AA64PFR0 appropriately
based on which CAPs are present. I think we probably need that CAP anyway
in order to ensure a guest that is using SVE2 cannot be migrated to a host
that only has SVE.

> 
> Similarly, SPE has more than one "enabled" value for PMSVer.
>

I'll take a look at the KVM series for SPE next week to see if
the UAPI will allow us to determine what values are possible.

Thanks for the heads up about this.

drew

Re: [PATCH 6/7] hw/arm/virt: spe: Add SPE fdt binding for virt machine

[Auger Eric]

Hi Haibo,

On 8/7/20 10:10 AM, Haibo Xu wrote:
> Add a virtual SPE device for virt machine while using PPI 
> 5 for SPE overflow interrupt number.
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  hw/arm/virt-acpi-build.c    |  3 +++ 
>  hw/arm/virt.c               | 42 +++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-defs.h |  1 + 
>  include/hw/arm/virt.h       |  1 + 
>  target/arm/cpu.c            |  2 ++
>  target/arm/cpu.h            |  2 ++
>  target/arm/kvm.c            |  6 ++++++
>  7 files changed, 57 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 91f0df7b13..5073ba22a5 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>          if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>          }
> +        if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
> +            gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ));
> +        }
>          if (vms->virt) {
>              gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ));
>          }
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ecfee362a1..c40819705d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
>      }
>  }
> 
> +static void fdt_add_spe_nodes(const VirtMachineState *vms)
> +{
> +    CPUState *cpu;
> +    ARMCPU *armcpu;
> +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> +
> +    CPU_FOREACH(cpu) {
> +        armcpu = ARM_CPU(cpu);
> +        if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
> +            return;
> +        }
> +        if (kvm_enabled()) {
> +            if (kvm_irqchip_in_kernel()) {
> +                kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ));
> +            }
> +            kvm_arm_spe_init(cpu);
> +        }
> +    }
> +
> +    if (vms->gic_version == VIRT_GIC_VERSION_2) {
> +        irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> +                             GIC_FDT_IRQ_PPI_CPU_WIDTH,
> +                             (1 << vms->smp_cpus) - 1);
> +    }
> +
> +    armcpu = ARM_CPU(qemu_get_cpu(0));
> +    qemu_fdt_add_subnode(vms->fdt, "/spe");
> +    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> +        const char compat[] = "arm,statistical-profiling-extension-v1";
> +        qemu_fdt_setprop(vms->fdt, "/spe", "compatible",
> +                         compat, sizeof(compat));
> +        qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts",
> +                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags);
> +    }
> +}
> +
>  static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
>  {
>      DeviceState *dev;
> @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms)
>                                      qdev_get_gpio_in(vms->gic, ppibase
>                                                       + VIRTUAL_PMU_IRQ));
> 
> +        qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0,
> +                                    qdev_get_gpio_in(vms->gic, ppibase
> +                                                     + VIRTUAL_SPE_IRQ));
> +
>          sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
>          sysbus_connect_irq(gicbusdev, i + smp_cpus,
>                             qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
> @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine)
> 
>      fdt_add_pmu_nodes(vms);
> 
> +    fdt_add_spe_nodes(vms);
> +
>      create_uart(vms, VIRT_UART, sysmem, serial_hd(0));
> 
>      if (vms->secure) {
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 38a42f409a..56a7f38ae4 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface {
>      uint32_t vgic_interrupt;
>      uint64_t gicr_base_address;
>      uint64_t arm_mpidr;
> +    uint16_t spe_interrupt; /* ACPI 6.3 */
This does not work for me.
You miss 2 uint8_t fields inbetween arm_mpdir and spe_interrupt:
Processor Power Efficiency Class and Reserved.

At the moment arm_spe_acpi_register_device() silently fails on guest
side since
gicc->header.length < ACPI_MADT_GICC_SPE

Thanks

Eric

>  } QEMU_PACKED;
> 
>  typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface;
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index dff67e1bef..56c83224d2 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -49,6 +49,7 @@
>  #define ARCH_TIMER_NS_EL1_IRQ 14
>  #define ARCH_TIMER_NS_EL2_IRQ 10
> 
> +#define VIRTUAL_SPE_IRQ 5
>  #define VIRTUAL_PMU_IRQ 7
> 
>  #define PPI(irq) ((irq) + 16)
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 40768b4d19..67ab0089fd 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj)
>                               "gicv3-maintenance-interrupt", 1);
>      qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt,
>                               "pmu-interrupt", 1);
> +    qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt,
> +                             "spe-interrupt", 1);
>  #endif
> 
>      /* DTB consumers generally don't in fact care what the 'compatible'
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index fe0ac14386..4bf8591df8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -790,6 +790,8 @@ struct ARMCPU {
>      qemu_irq gicv3_maintenance_interrupt;
>      /* GPIO output for the PMU interrupt */
>      qemu_irq pmu_interrupt;
> +    /* GPIO output for the SPE interrupt */
> +    qemu_irq spe_interrupt;
> 
>      /* MemoryRegion to use for secure physical accesses */
>      MemoryRegion *secure_memory;
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 58f991e890..ecafdda364 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
>              switched_level &= ~KVM_ARM_DEV_PMU;
>          }
> 
> +        if (switched_level & KVM_ARM_DEV_SPE) {
> +            qemu_set_irq(cpu->spe_interrupt,
> +                         !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE));
> +            switched_level &= ~KVM_ARM_DEV_SPE;
> +        }
> +
>          if (switched_level) {
>              qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n",
>                            __func__, switched_level);
> 

Re: [PATCH 6/7] hw/arm/virt: spe: Add SPE fdt binding for virt machine

[Haibo Xu]

On Sat, 29 Aug 2020 at 23:22, Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Haibo,
>
> On 8/7/20 10:10 AM, Haibo Xu wrote:
> > Add a virtual SPE device for virt machine while using PPI
> > 5 for SPE overflow interrupt number.
> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> > ---
> >  hw/arm/virt-acpi-build.c    |  3 +++
> >  hw/arm/virt.c               | 42 +++++++++++++++++++++++++++++++++++++
> >  include/hw/acpi/acpi-defs.h |  1 +
> >  include/hw/arm/virt.h       |  1 +
> >  target/arm/cpu.c            |  2 ++
> >  target/arm/cpu.h            |  2 ++
> >  target/arm/kvm.c            |  6 ++++++
> >  7 files changed, 57 insertions(+)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 91f0df7b13..5073ba22a5 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >          if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> >              gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
> >          }
> > +        if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
> > +            gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ));
> > +        }
> >          if (vms->virt) {
> >              gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ));
> >          }
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index ecfee362a1..c40819705d 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> >      }
> >  }
> >
> > +static void fdt_add_spe_nodes(const VirtMachineState *vms)
> > +{
> > +    CPUState *cpu;
> > +    ARMCPU *armcpu;
> > +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> > +
> > +    CPU_FOREACH(cpu) {
> > +        armcpu = ARM_CPU(cpu);
> > +        if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) {
> > +            return;
> > +        }
> > +        if (kvm_enabled()) {
> > +            if (kvm_irqchip_in_kernel()) {
> > +                kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ));
> > +            }
> > +            kvm_arm_spe_init(cpu);
> > +        }
> > +    }
> > +
> > +    if (vms->gic_version == VIRT_GIC_VERSION_2) {
> > +        irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> > +                             GIC_FDT_IRQ_PPI_CPU_WIDTH,
> > +                             (1 << vms->smp_cpus) - 1);
> > +    }
> > +
> > +    armcpu = ARM_CPU(qemu_get_cpu(0));
> > +    qemu_fdt_add_subnode(vms->fdt, "/spe");
> > +    if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> > +        const char compat[] = "arm,statistical-profiling-extension-v1";
> > +        qemu_fdt_setprop(vms->fdt, "/spe", "compatible",
> > +                         compat, sizeof(compat));
> > +        qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts",
> > +                               GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags);
> > +    }
> > +}
> > +
> >  static inline DeviceState *create_acpi_ged(VirtMachineState *vms)
> >  {
> >      DeviceState *dev;
> > @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms)
> >                                      qdev_get_gpio_in(vms->gic, ppibase
> >                                                       + VIRTUAL_PMU_IRQ));
> >
> > +        qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0,
> > +                                    qdev_get_gpio_in(vms->gic, ppibase
> > +                                                     + VIRTUAL_SPE_IRQ));
> > +
> >          sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
> >          sysbus_connect_irq(gicbusdev, i + smp_cpus,
> >                             qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
> > @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine)
> >
> >      fdt_add_pmu_nodes(vms);
> >
> > +    fdt_add_spe_nodes(vms);
> > +
> >      create_uart(vms, VIRT_UART, sysmem, serial_hd(0));
> >
> >      if (vms->secure) {
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 38a42f409a..56a7f38ae4 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface {
> >      uint32_t vgic_interrupt;
> >      uint64_t gicr_base_address;
> >      uint64_t arm_mpidr;
> > +    uint16_t spe_interrupt; /* ACPI 6.3 */
> This does not work for me.
> You miss 2 uint8_t fields inbetween arm_mpdir and spe_interrupt:
> Processor Power Efficiency Class and Reserved.
>
> At the moment arm_spe_acpi_register_device() silently fails on guest
> side since
> gicc->header.length < ACPI_MADT_GICC_SPE
>
> Thanks
>
> Eric
>

Thanks for pointing out this. I will fix it in the next version.

> >  } QEMU_PACKED;
> >
> >  typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface;
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index dff67e1bef..56c83224d2 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -49,6 +49,7 @@
> >  #define ARCH_TIMER_NS_EL1_IRQ 14
> >  #define ARCH_TIMER_NS_EL2_IRQ 10
> >
> > +#define VIRTUAL_SPE_IRQ 5
> >  #define VIRTUAL_PMU_IRQ 7
> >
> >  #define PPI(irq) ((irq) + 16)
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 40768b4d19..67ab0089fd 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj)
> >                               "gicv3-maintenance-interrupt", 1);
> >      qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt,
> >                               "pmu-interrupt", 1);
> > +    qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt,
> > +                             "spe-interrupt", 1);
> >  #endif
> >
> >      /* DTB consumers generally don't in fact care what the 'compatible'
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index fe0ac14386..4bf8591df8 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -790,6 +790,8 @@ struct ARMCPU {
> >      qemu_irq gicv3_maintenance_interrupt;
> >      /* GPIO output for the PMU interrupt */
> >      qemu_irq pmu_interrupt;
> > +    /* GPIO output for the SPE interrupt */
> > +    qemu_irq spe_interrupt;
> >
> >      /* MemoryRegion to use for secure physical accesses */
> >      MemoryRegion *secure_memory;
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 58f991e890..ecafdda364 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
> >              switched_level &= ~KVM_ARM_DEV_PMU;
> >          }
> >
> > +        if (switched_level & KVM_ARM_DEV_SPE) {
> > +            qemu_set_irq(cpu->spe_interrupt,
> > +                         !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE));
> > +            switched_level &= ~KVM_ARM_DEV_SPE;
> > +        }
> > +
> >          if (switched_level) {
> >              qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n",
> >                            __func__, switched_level);
> >
>

Re: [PATCH 0/7] target/arm: Add vSPE support to KVM guest

[Auger Eric]

Hi Haibo,

On 8/7/20 10:10 AM, Haibo Xu wrote:
> This series add support for SPE(Statistical Profiling Extension)[1]
> in KVM guest. It's based on Andrew Murray's kernel KVM patches V2[2],
> and has been tested to ensure that guest can use SPE with valid data.
> E.g.
> 
> In host:
> $ ./qemu-system-aarch64 \
>         -cpu host -M virt,accel=kvm,gic-version=3 -nographic -m 2048M \
>         -kernel ./Image-new \
>         -initrd /boot/initrd.img-5.6.0-rc2+ \
>         -append "root=/dev/vda rw console=ttyAMA0" -nodefaults -serial stdio\
>         -drive if=none,file=./xenial.rootfs.ext4,id=hd0,format=raw \
>         -device virtio-blk-device,drive=hd0  \
> 
> In guest:
> $ perf record -e arm_spe/ts_enable=1,pa_enable=1,pct_enable=1/ \
>         dd if=/dev/zero of=/dev/null count=1000
> $ perf report --dump-raw-trace > spe_buf.txt
> 
> The spe_buf.txt should contain similar data as below:
> 
> . ... ARM SPE data: size 135944 bytes
> .  00000000:  b0 f4 d3 29 10 00 80 ff a0                      PC 0xff80001029d3f4 el1 ns=1
> .  00000009:  99 0b 00                                        LAT 11 ISSUE
> .  0000000c:  98 0d 00                                        LAT 13 TOT
> .  0000000f:  52 16 00                                        EV RETIRED L1D-ACCESS TLB-ACCESS
> .  00000012:  49 00                                           LD
> .  00000014:  b2 d0 40 d8 70 00 00 ff 00                      VA 0xff000070d840d0
> .  0000001d:  9a 01 00                                        LAT 1 XLAT
> .  00000020:  00 00 00                                        PAD
> .  00000023:  71 a5 1f b3 20 14 00 00 00                      TS 86447955877
> .  0000002c:  b0 7c f9 29 10 00 80 ff a0                      PC 0xff80001029f97c el1 ns=1
> .  00000035:  99 02 00                                        LAT 2 ISSUE
> .  00000038:  98 03 00                                        LAT 3 TOT
> .  0000003b:  52 02 00                                        EV RETIRED
> .  0000003e:  48 00                                           INSN-OTHER
> .  00000040:  00 00 00                                        PAD
> .  00000043:  71 ef 1f b3 20 14 00 00 00                      TS 86447955951
> .  0000004c:  b0 f0 e9 29 10 00 80 ff a0                      PC 0xff80001029e9f0 el1 ns=1
> .  00000055:  99 02 00                                        LAT 2 ISSUE
> .  00000058:  98 03 00                                        LAT 3 TOT
> .  0000005b:  52 02 00                                        EV RETIRED
> 
> If you want to disable the vSPE support, you can use the 'spe=off' cpu
> property:
> 
> ./qemu-system-aarch64 \
>         -cpu host,spe=off -M virt,accel=kvm,gic-version=3 -nographic -m 2048M \
>         -kernel ./Image-new \
>         -initrd /boot/initrd.img-5.6.0-rc2+ \
>         -append "root=/dev/vda rw console=ttyAMA0" -nodefaults -serial stdio\
>         -drive if=none,file=./xenial.rootfs.ext4,id=hd0,format=raw \
>         -device virtio-blk-device,drive=hd0  \
> 
> Note:
> (1) Since the kernel patches are still under review, some of the macros
>     in the header files may be changed after merging. We may need to
>     update them accordingly.
to be more explicit one needs to replace on the kernel 5.5-rc2 based series

-#define KVM_CAP_ARM_SPE_V1 179
+#define KVM_CAP_ARM_SPE_V1 184

I got misleaded ;-)

+ Andrew in CC as he contributed the kernel part.

For information, I have been working on a kvm unit test series for
testing SPE. I will send an RFC, most probably this week. At the moment
I still face some weirdness such as some unexpected Service state in the
syndrome register. Anyway I will share the existing code so that we can
discuss the issues.

Are there any plans to respin the kernel series

Thanks

Eric

> (2) These patches only add vSPE support in KVM mode, for TCG mode, I'm
>     not sure whether we need to support it.
> (3) Just followed the 'pmu' property, we only allow this feature to be
>     removed from CPUs which enable it by default. But since the SPE is
>     an optional feature extension for Armv8.2, I think a better way may
>     be to disable it by default, and only enable it when the host cpu
>     do have the feature.
> 
> [1]https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/
>    posts/statistical-profiling-extension-for-armv8-a
> [2]https://www.spinics.net/lists/arm-kernel/msg776228.html
> 
> Haibo Xu (7):
>   update Linux headers with new vSPE macros
>   target/arm/kvm: spe: Add helper to detect SPE when using KVM
>   target/arm/cpu: spe: Add an option to turn on/off vSPE support
>   target/arm/kvm: spe: Unify device attr operatioin helper
>   target/arm/kvm: spe: Add device init and set_irq operations
>   hw/arm/virt: spe: Add SPE fdt binding for virt machine
>   target/arm/cpu: spe: Enable spe to work with host cpu
> 
>  hw/arm/virt-acpi-build.c      |  3 +++
>  hw/arm/virt.c                 | 42 ++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-defs.h   |  1 +
>  include/hw/arm/virt.h         |  1 +
>  linux-headers/asm-arm64/kvm.h |  4 +++
>  linux-headers/linux/kvm.h     |  2 ++
>  target/arm/cpu.c              | 34 +++++++++++++++++++++++++
>  target/arm/cpu.h              |  5 ++++
>  target/arm/kvm.c              | 11 ++++++++
>  target/arm/kvm64.c            | 48 ++++++++++++++++++++++++++++++++---
>  target/arm/kvm_arm.h          | 18 +++++++++++++
>  11 files changed, 166 insertions(+), 3 deletions(-)
> 

Re: [PATCH 0/7] target/arm: Add vSPE support to KVM guest

[Haibo Xu]

On Mon, 31 Aug 2020 at 15:56, Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Haibo,
>
> On 8/7/20 10:10 AM, Haibo Xu wrote:
> > This series add support for SPE(Statistical Profiling Extension)[1]
> > in KVM guest. It's based on Andrew Murray's kernel KVM patches V2[2],
> > and has been tested to ensure that guest can use SPE with valid data.
> > E.g.
> >
> > In host:
> > $ ./qemu-system-aarch64 \
> >         -cpu host -M virt,accel=kvm,gic-version=3 -nographic -m 2048M \
> >         -kernel ./Image-new \
> >         -initrd /boot/initrd.img-5.6.0-rc2+ \
> >         -append "root=/dev/vda rw console=ttyAMA0" -nodefaults -serial stdio\
> >         -drive if=none,file=./xenial.rootfs.ext4,id=hd0,format=raw \
> >         -device virtio-blk-device,drive=hd0  \
> >
> > In guest:
> > $ perf record -e arm_spe/ts_enable=1,pa_enable=1,pct_enable=1/ \
> >         dd if=/dev/zero of=/dev/null count=1000
> > $ perf report --dump-raw-trace > spe_buf.txt
> >
> > The spe_buf.txt should contain similar data as below:
> >
> > . ... ARM SPE data: size 135944 bytes
> > .  00000000:  b0 f4 d3 29 10 00 80 ff a0                      PC 0xff80001029d3f4 el1 ns=1
> > .  00000009:  99 0b 00                                        LAT 11 ISSUE
> > .  0000000c:  98 0d 00                                        LAT 13 TOT
> > .  0000000f:  52 16 00                                        EV RETIRED L1D-ACCESS TLB-ACCESS
> > .  00000012:  49 00                                           LD
> > .  00000014:  b2 d0 40 d8 70 00 00 ff 00                      VA 0xff000070d840d0
> > .  0000001d:  9a 01 00                                        LAT 1 XLAT
> > .  00000020:  00 00 00                                        PAD
> > .  00000023:  71 a5 1f b3 20 14 00 00 00                      TS 86447955877
> > .  0000002c:  b0 7c f9 29 10 00 80 ff a0                      PC 0xff80001029f97c el1 ns=1
> > .  00000035:  99 02 00                                        LAT 2 ISSUE
> > .  00000038:  98 03 00                                        LAT 3 TOT
> > .  0000003b:  52 02 00                                        EV RETIRED
> > .  0000003e:  48 00                                           INSN-OTHER
> > .  00000040:  00 00 00                                        PAD
> > .  00000043:  71 ef 1f b3 20 14 00 00 00                      TS 86447955951
> > .  0000004c:  b0 f0 e9 29 10 00 80 ff a0                      PC 0xff80001029e9f0 el1 ns=1
> > .  00000055:  99 02 00                                        LAT 2 ISSUE
> > .  00000058:  98 03 00                                        LAT 3 TOT
> > .  0000005b:  52 02 00                                        EV RETIRED
> >
> > If you want to disable the vSPE support, you can use the 'spe=off' cpu
> > property:
> >
> > ./qemu-system-aarch64 \
> >         -cpu host,spe=off -M virt,accel=kvm,gic-version=3 -nographic -m 2048M \
> >         -kernel ./Image-new \
> >         -initrd /boot/initrd.img-5.6.0-rc2+ \
> >         -append "root=/dev/vda rw console=ttyAMA0" -nodefaults -serial stdio\
> >         -drive if=none,file=./xenial.rootfs.ext4,id=hd0,format=raw \
> >         -device virtio-blk-device,drive=hd0  \
> >
> > Note:
> > (1) Since the kernel patches are still under review, some of the macros
> >     in the header files may be changed after merging. We may need to
> >     update them accordingly.
> to be more explicit one needs to replace on the kernel 5.5-rc2 based series
>
> -#define KVM_CAP_ARM_SPE_V1 179
> +#define KVM_CAP_ARM_SPE_V1 184
>
> I got misleaded ;-)
>
> + Andrew in CC as he contributed the kernel part.
>
> For information, I have been working on a kvm unit test series for
> testing SPE. I will send an RFC, most probably this week. At the moment
> I still face some weirdness such as some unexpected Service state in the
> syndrome register. Anyway I will share the existing code so that we can
> discuss the issues.
>
> Are there any plans to respin the kernel series
>
> Thanks
>
> Eric
>

Hi Eric,

Thanks for elaborating on the above macro definition!
The next version of the kernel series are supposed to be sent out in
mid-September,
and it should not change so much except for some macro definitions.

Regards,

Haibo

> > (2) These patches only add vSPE support in KVM mode, for TCG mode, I'm
> >     not sure whether we need to support it.
> > (3) Just followed the 'pmu' property, we only allow this feature to be
> >     removed from CPUs which enable it by default. But since the SPE is
> >     an optional feature extension for Armv8.2, I think a better way may
> >     be to disable it by default, and only enable it when the host cpu
> >     do have the feature.
> >
> > [1]https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/
> >    posts/statistical-profiling-extension-for-armv8-a
> > [2]https://www.spinics.net/lists/arm-kernel/msg776228.html
> >
> > Haibo Xu (7):
> >   update Linux headers with new vSPE macros
> >   target/arm/kvm: spe: Add helper to detect SPE when using KVM
> >   target/arm/cpu: spe: Add an option to turn on/off vSPE support
> >   target/arm/kvm: spe: Unify device attr operatioin helper
> >   target/arm/kvm: spe: Add device init and set_irq operations
> >   hw/arm/virt: spe: Add SPE fdt binding for virt machine
> >   target/arm/cpu: spe: Enable spe to work with host cpu
> >
> >  hw/arm/virt-acpi-build.c      |  3 +++
> >  hw/arm/virt.c                 | 42 ++++++++++++++++++++++++++++++
> >  include/hw/acpi/acpi-defs.h   |  1 +
> >  include/hw/arm/virt.h         |  1 +
> >  linux-headers/asm-arm64/kvm.h |  4 +++
> >  linux-headers/linux/kvm.h     |  2 ++
> >  target/arm/cpu.c              | 34 +++++++++++++++++++++++++
> >  target/arm/cpu.h              |  5 ++++
> >  target/arm/kvm.c              | 11 ++++++++
> >  target/arm/kvm64.c            | 48 ++++++++++++++++++++++++++++++++---
> >  target/arm/kvm_arm.h          | 18 +++++++++++++
> >  11 files changed, 166 insertions(+), 3 deletions(-)
> >
>