[Qemu-devel] [PATCH v5 0/3] add support for VCPU event states

[Qemu-devel] [PATCH v5 0/3] add support for VCPU event states

[Dongjiu Geng]

Support for KVM_GET/SET_VCPU_EVENTS to get/set the SError exception
state, and support the state migration.

change since v4:
1. update the linux header file to 4.18-rc5 

change since v3:
1. Add a new new subsection with a suitable 'ras_needed' function
controlling whether it is present
2. Add a ARM_FEATURE_RAS feature bit for CPUARMState

change since v2:
1. add header definition for arm platform

change since v1:
1. update the code to fix the build errors

The related kernel change has been already applied to kvmarm/next.
(https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/log/?h=next)

Dongjiu Geng (3):
  Update Linux headers to 4.18-rc5
  kvm: sync linux headers
  target: arm: Add support for VCPU event states

 include/standard-headers/linux/virtio_config.h | 16 +++++--
 linux-headers/asm-arm/kvm.h                    | 13 ++++++
 linux-headers/asm-arm64/kvm.h                  | 13 ++++++
 linux-headers/asm-mips/unistd.h                | 18 +++++---
 linux-headers/asm-powerpc/kvm.h                |  1 +
 linux-headers/asm-powerpc/unistd.h             |  1 +
 linux-headers/asm-s390/unistd_32.h             |  2 +
 linux-headers/asm-s390/unistd_64.h             |  2 +
 linux-headers/linux/kvm.h                      |  1 +
 target/arm/cpu.h                               |  6 +++
 target/arm/kvm64.c                             | 59 ++++++++++++++++++++++++++
 target/arm/machine.c                           | 22 ++++++++++
 12 files changed, 144 insertions(+), 10 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 1/3] Update Linux headers to 4.18-rc5

[Dongjiu Geng]

Update our copy of the Linux headers to upstream 4.18-rc5
(kernel commit 9d3cce1e8b8561fed5f38)

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
 include/standard-headers/linux/virtio_config.h | 16 ++++++++++++----
 linux-headers/asm-mips/unistd.h                | 18 ++++++++++++------
 linux-headers/asm-powerpc/kvm.h                |  1 +
 linux-headers/asm-powerpc/unistd.h             |  1 +
 linux-headers/asm-s390/unistd_32.h             |  2 ++
 linux-headers/asm-s390/unistd_64.h             |  2 ++
 6 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
index b777069..0b19436 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -45,11 +45,14 @@
 /* We've given up on this device. */
 #define VIRTIO_CONFIG_S_FAILED		0x80
 
-/* Some virtio feature bits (currently bits 28 through 32) are reserved for the
- * transport being used (eg. virtio_ring), the rest are per-device feature
- * bits. */
+/*
+ * Virtio feature bits VIRTIO_TRANSPORT_F_START through
+ * VIRTIO_TRANSPORT_F_END are reserved for the transport
+ * being used (e.g. virtio_ring, virtio_pci etc.), the
+ * rest are per-device feature bits.
+ */
 #define VIRTIO_TRANSPORT_F_START	28
-#define VIRTIO_TRANSPORT_F_END		34
+#define VIRTIO_TRANSPORT_F_END		38
 
 #ifndef VIRTIO_CONFIG_NO_LEGACY
 /* Do we get callbacks when the ring is completely used, even if we've
@@ -71,4 +74,9 @@
  * this is for compatibility with legacy systems.
  */
 #define VIRTIO_F_IOMMU_PLATFORM		33
+
+/*
+ * Does the device support Single Root I/O Virtualization?
+ */
+#define VIRTIO_F_SR_IOV			37
 #endif /* _LINUX_VIRTIO_CONFIG_H */
diff --git a/linux-headers/asm-mips/unistd.h b/linux-headers/asm-mips/unistd.h
index 9bfef7f..d4a85ef 100644
--- a/linux-headers/asm-mips/unistd.h
+++ b/linux-headers/asm-mips/unistd.h
@@ -388,17 +388,19 @@
 #define __NR_pkey_alloc			(__NR_Linux + 364)
 #define __NR_pkey_free			(__NR_Linux + 365)
 #define __NR_statx			(__NR_Linux + 366)
+#define __NR_rseq			(__NR_Linux + 367)
+#define __NR_io_pgetevents		(__NR_Linux + 368)
 
 
 /*
  * Offset of the last Linux o32 flavoured syscall
  */
-#define __NR_Linux_syscalls		366
+#define __NR_Linux_syscalls		368
 
 #endif /* _MIPS_SIM == _MIPS_SIM_ABI32 */
 
 #define __NR_O32_Linux			4000
-#define __NR_O32_Linux_syscalls		366
+#define __NR_O32_Linux_syscalls		368
 
 #if _MIPS_SIM == _MIPS_SIM_ABI64
 
@@ -733,16 +735,18 @@
 #define __NR_pkey_alloc			(__NR_Linux + 324)
 #define __NR_pkey_free			(__NR_Linux + 325)
 #define __NR_statx			(__NR_Linux + 326)
+#define __NR_rseq			(__NR_Linux + 327)
+#define __NR_io_pgetevents		(__NR_Linux + 328)
 
 /*
  * Offset of the last Linux 64-bit flavoured syscall
  */
-#define __NR_Linux_syscalls		326
+#define __NR_Linux_syscalls		328
 
 #endif /* _MIPS_SIM == _MIPS_SIM_ABI64 */
 
 #define __NR_64_Linux			5000
-#define __NR_64_Linux_syscalls		326
+#define __NR_64_Linux_syscalls		328
 
 #if _MIPS_SIM == _MIPS_SIM_NABI32
 
@@ -1081,15 +1085,17 @@
 #define __NR_pkey_alloc			(__NR_Linux + 328)
 #define __NR_pkey_free			(__NR_Linux + 329)
 #define __NR_statx			(__NR_Linux + 330)
+#define __NR_rseq			(__NR_Linux + 331)
+#define __NR_io_pgetevents		(__NR_Linux + 332)
 
 /*
  * Offset of the last N32 flavoured syscall
  */
-#define __NR_Linux_syscalls		330
+#define __NR_Linux_syscalls		332
 
 #endif /* _MIPS_SIM == _MIPS_SIM_NABI32 */
 
 #define __NR_N32_Linux			6000
-#define __NR_N32_Linux_syscalls		330
+#define __NR_N32_Linux_syscalls		332
 
 #endif /* _ASM_UNISTD_H */
diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
index 833ed9a..1b32b56 100644
--- a/linux-headers/asm-powerpc/kvm.h
+++ b/linux-headers/asm-powerpc/kvm.h
@@ -633,6 +633,7 @@ struct kvm_ppc_cpu_char {
 #define KVM_REG_PPC_PSSCR	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xbd)
 
 #define KVM_REG_PPC_DEC_EXPIRY	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xbe)
+#define KVM_REG_PPC_ONLINE	(KVM_REG_PPC | KVM_REG_SIZE_U32 | 0xbf)
 
 /* Transactional Memory checkpointed state:
  * This is all GPRs, all VSX regs and a subset of SPRs
diff --git a/linux-headers/asm-powerpc/unistd.h b/linux-headers/asm-powerpc/unistd.h
index 3629858..ec3533b 100644
--- a/linux-headers/asm-powerpc/unistd.h
+++ b/linux-headers/asm-powerpc/unistd.h
@@ -399,5 +399,6 @@
 #define __NR_pkey_free		385
 #define __NR_pkey_mprotect	386
 #define __NR_rseq		387
+#define __NR_io_pgetevents	388
 
 #endif /* _ASM_POWERPC_UNISTD_H_ */
diff --git a/linux-headers/asm-s390/unistd_32.h b/linux-headers/asm-s390/unistd_32.h
index d0f97cd..514e302 100644
--- a/linux-headers/asm-s390/unistd_32.h
+++ b/linux-headers/asm-s390/unistd_32.h
@@ -361,5 +361,7 @@
 #define __NR_statx 379
 #define __NR_s390_sthyi 380
 #define __NR_kexec_file_load 381
+#define __NR_io_pgetevents 382
+#define __NR_rseq 383
 
 #endif /* _ASM_S390_UNISTD_32_H */
diff --git a/linux-headers/asm-s390/unistd_64.h b/linux-headers/asm-s390/unistd_64.h
index 23ffb97..d2b73de 100644
--- a/linux-headers/asm-s390/unistd_64.h
+++ b/linux-headers/asm-s390/unistd_64.h
@@ -328,5 +328,7 @@
 #define __NR_statx 379
 #define __NR_s390_sthyi 380
 #define __NR_kexec_file_load 381
+#define __NR_io_pgetevents 382
+#define __NR_rseq 383
 
 #endif /* _ASM_S390_UNISTD_64_H */
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 2/3] kvm: sync linux headers

[Dongjiu Geng]

Import KVM_CAP_ARM_INJECT_SERROR_ESR and kvm_vcpu_events
struct definition.

The related kernel change has been already applied to kvmarm/next.
(https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/log/?h=next)

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
 linux-headers/asm-arm/kvm.h   | 13 +++++++++++++
 linux-headers/asm-arm64/kvm.h | 13 +++++++++++++
 linux-headers/linux/kvm.h     |  1 +
 3 files changed, 27 insertions(+)

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index 72aa226..e1f8b74 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -27,6 +27,7 @@
 #define __KVM_HAVE_GUEST_DEBUG
 #define __KVM_HAVE_IRQ_LINE
 #define __KVM_HAVE_READONLY_MEM
+#define __KVM_HAVE_VCPU_EVENTS
 
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 
@@ -125,6 +126,18 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
+/* for KVM_GET/SET_VCPU_EVENTS */
+struct kvm_vcpu_events {
+	struct {
+		__u8 serror_pending;
+		__u8 serror_has_esr;
+		/* Align it to 8 bytes */
+		__u8 pad[6];
+		__u64 serror_esr;
+	} exception;
+	__u32 reserved[12];
+};
+
 /* If you need to interpret the index values, here is the key: */
 #define KVM_REG_ARM_COPROC_MASK		0x000000000FFF0000
 #define KVM_REG_ARM_COPROC_SHIFT	16
diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index 99cb9ad..e6a98c1 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -39,6 +39,7 @@
 #define __KVM_HAVE_GUEST_DEBUG
 #define __KVM_HAVE_IRQ_LINE
 #define __KVM_HAVE_READONLY_MEM
+#define __KVM_HAVE_VCPU_EVENTS
 
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 
@@ -154,6 +155,18 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
+/* for KVM_GET/SET_VCPU_EVENTS */
+struct kvm_vcpu_events {
+	struct {
+		__u8 serror_pending;
+		__u8 serror_has_esr;
+		/* Align it to 8 bytes */
+		__u8 pad[6];
+		__u64 serror_esr;
+	} exception;
+	__u32 reserved[12];
+};
+
 /* If you need to interpret the index values, here is the key: */
 #define KVM_REG_ARM_COPROC_MASK		0x000000000FFF0000
 #define KVM_REG_ARM_COPROC_SHIFT	16
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 98f389a..446f694 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -949,6 +949,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_GET_MSR_FEATURES 153
 #define KVM_CAP_HYPERV_EVENTFD 154
 #define KVM_CAP_HYPERV_TLBFLUSH 155
+#define KVM_CAP_ARM_INJECT_SERROR_ESR 156
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 3/3] target: arm: Add support for VCPU event states

[Dongjiu Geng]

This patch extends the qemu-kvm state sync logic with support for
KVM_GET/SET_VCPU_EVENTS, giving access to yet missing SError exception.
And also it can support the exception state migration.

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
change since v4:
1. Rebase the code to latest

change since v3:
1. Add a new new subsection with a suitable 'ras_needed' function
   controlling whether it is present
2. Add a ARM_FEATURE_RAS feature bit for CPUARMState

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
 target/arm/cpu.h     |  6 ++++++
 target/arm/kvm64.c   | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 target/arm/machine.c | 22 ++++++++++++++++++++
 3 files changed, 87 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e310ffc..f00f0b6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -645,6 +645,11 @@ typedef struct CPUARMState {
     const struct arm_boot_info *boot_info;
     /* Store GICv3CPUState to access from this struct */
     void *gicv3state;
+    struct {
+        uint32_t pending;
+        uint32_t has_esr;
+        uint64_t esr;
+    } serror;
 } CPUARMState;
 
 /**
@@ -1486,6 +1491,7 @@ enum arm_features {
     ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
     ARM_FEATURE_V8_FCMA, /* has complex number part of v8.3 extensions.  */
     ARM_FEATURE_M_MAIN, /* M profile Main Extension */
+    ARM_FEATURE_RAS_EXT, /* has RAS Extension */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index e0b8246..ebf7a00 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -527,6 +527,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
         unset_feature(&env->features, ARM_FEATURE_PMU);
     }
 
+    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_INJECT_SERROR_ESR)) {
+        set_feature(&env->features, ARM_FEATURE_RAS_EXT);
+    }
+
     /* Do KVM_ARM_VCPU_INIT ioctl */
     ret = kvm_arm_vcpu_init(cs);
     if (ret) {
@@ -600,6 +604,50 @@ int kvm_arm_cpreg_level(uint64_t regidx)
 #define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
                  KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
 
+static int kvm_put_vcpu_events(ARMCPU *cpu)
+{
+    CPUARMState *env = &cpu->env;
+    struct kvm_vcpu_events events = {};
+
+    if (!kvm_has_vcpu_events()) {
+        return 0;
+    }
+
+    memset(&events, 0, sizeof(events));
+    events.exception.serror_pending = env->serror.pending;
+
+    if (arm_feature(env, ARM_FEATURE_RAS_EXT)) {
+        events.exception.serror_has_esr = env->serror.has_esr;
+        events.exception.serror_esr = env->serror.esr;
+    }
+
+    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
+}
+
+static int kvm_get_vcpu_events(ARMCPU *cpu)
+{
+    CPUARMState *env = &cpu->env;
+    struct kvm_vcpu_events events;
+    int ret;
+
+    if (!kvm_has_vcpu_events()) {
+        return 0;
+    }
+
+    memset(&events, 0, sizeof(events));
+    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_VCPU_EVENTS, &events);
+
+    if (ret < 0) {
+        return ret;
+    }
+
+    env->serror.pending = events.exception.serror_pending;
+    env->serror.has_esr = events.exception.serror_has_esr;
+    env->serror.esr = events.exception.serror_esr;
+
+    return 0;
+}
+
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     struct kvm_one_reg reg;
@@ -727,6 +775,12 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return ret;
     }
 
+    ret = kvm_put_vcpu_events(cpu);
+    if (ret) {
+        printf("return error kvm_put_vcpu_events: %d\n", ret);
+        return ret;
+    }
+
     if (!write_list_to_kvmstate(cpu, level)) {
         return EINVAL;
     }
@@ -863,6 +917,11 @@ int kvm_arch_get_registers(CPUState *cs)
     }
     vfp_set_fpcr(env, fpr);
 
+    ret = kvm_get_vcpu_events(cpu);
+    if (ret) {
+        return ret;
+    }
+
     if (!write_kvmstate_to_list(cpu)) {
         return EINVAL;
     }
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 2e28d08..ead8b2a 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -172,6 +172,27 @@ static const VMStateDescription vmstate_sve = {
 };
 #endif /* AARCH64 */
 
+static bool ras_needed(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+    CPUARMState *env = &cpu->env;
+
+    return arm_feature(env, ARM_FEATURE_RAS_EXT);
+}
+
+static const VMStateDescription vmstate_ras = {
+    .name = "cpu/ras",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = ras_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(env.serror.pending, ARMCPU),
+        VMSTATE_UINT32(env.serror.has_esr, ARMCPU),
+        VMSTATE_UINT64(env.serror.esr, ARMCPU),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static bool m_needed(void *opaque)
 {
     ARMCPU *cpu = opaque;
@@ -723,6 +744,7 @@ const VMStateDescription vmstate_arm_cpu = {
 #ifdef TARGET_AARCH64
         &vmstate_sve,
 #endif
+        &vmstate_ras,
         NULL
     }
 };
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v5 3/3] target: arm: Add support for VCPU event states

[Peter Maydell]

On 21 July 2018 at 18:57, Dongjiu Geng <gengdongjiu@huawei.com> wrote:
> This patch extends the qemu-kvm state sync logic with support for
> KVM_GET/SET_VCPU_EVENTS, giving access to yet missing SError exception.
> And also it can support the exception state migration.

> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> ---
> change since v4:
> 1. Rebase the code to latest
>
> change since v3:
> 1. Add a new new subsection with a suitable 'ras_needed' function
>    controlling whether it is present
> 2. Add a ARM_FEATURE_RAS feature bit for CPUARMState
>
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>

Hi; sorry it's taken me so long to get to this patchset.
I think it's basically the right shape, but I have some
changes to suggest below.

> ---
>  target/arm/cpu.h     |  6 ++++++
>  target/arm/kvm64.c   | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/arm/machine.c | 22 ++++++++++++++++++++
>  3 files changed, 87 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e310ffc..f00f0b6 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -645,6 +645,11 @@ typedef struct CPUARMState {
>      const struct arm_boot_info *boot_info;
>      /* Store GICv3CPUState to access from this struct */
>      void *gicv3state;
> +    struct {
> +        uint32_t pending;
> +        uint32_t has_esr;
> +        uint64_t esr;
> +    } serror;

I recommend putting this earlier in the CPUARMState struct.
Specifically, you want to put it somewhere before the
"end_reset_fields" marker. All fields before that are cleared
to zero on CPU reset. If you put the struct after that then
you would need manual code in the cpu reset function to
reset it appropriately.

>  } CPUARMState;
>
>  /**
> @@ -1486,6 +1491,7 @@ enum arm_features {
>      ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
>      ARM_FEATURE_V8_FCMA, /* has complex number part of v8.3 extensions.  */
>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
> +    ARM_FEATURE_RAS_EXT, /* has RAS Extension */
>  };
>
>  static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index e0b8246..ebf7a00 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -527,6 +527,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          unset_feature(&env->features, ARM_FEATURE_PMU);
>      }
>
> +    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_INJECT_SERROR_ESR)) {
> +        set_feature(&env->features, ARM_FEATURE_RAS_EXT);
> +    }

I think this is confusing two things. Whether the host kernel
has support for injecting SError ESRs is not the same as
whether the guest CPU has the RAS extensions. So you don't want
to use an ARM_FEATURE_* bit to track whether we need to migrate
the SError ESR state.

Instead you should just use a bool variable local to this file
to track whether the kernel has state we need to migrate.
Compare the way we use "have_guest_debug" to track whether
KVM_CAP_SET_GUEST_DEBUG is set. You can call this
have_inject_serror_esr.

> +
>      /* Do KVM_ARM_VCPU_INIT ioctl */
>      ret = kvm_arm_vcpu_init(cs);
>      if (ret) {
> @@ -600,6 +604,50 @@ int kvm_arm_cpreg_level(uint64_t regidx)
>  #define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
>                   KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>
> +static int kvm_put_vcpu_events(ARMCPU *cpu)
> +{
> +    CPUARMState *env = &cpu->env;
> +    struct kvm_vcpu_events events = {};
> +
> +    if (!kvm_has_vcpu_events()) {
> +        return 0;
> +    }
> +
> +    memset(&events, 0, sizeof(events));
> +    events.exception.serror_pending = env->serror.pending;
> +
> +    if (arm_feature(env, ARM_FEATURE_RAS_EXT)) {

Here you can check have_inject_serror_esr.

> +        events.exception.serror_has_esr = env->serror.has_esr;
> +        events.exception.serror_esr = env->serror.esr;
> +    }
> +
> +    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
> +}
> +
> +static int kvm_get_vcpu_events(ARMCPU *cpu)
> +{
> +    CPUARMState *env = &cpu->env;
> +    struct kvm_vcpu_events events;
> +    int ret;
> +
> +    if (!kvm_has_vcpu_events()) {
> +        return 0;
> +    }
> +
> +    memset(&events, 0, sizeof(events));
> +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_VCPU_EVENTS, &events);
> +
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    env->serror.pending = events.exception.serror_pending;
> +    env->serror.has_esr = events.exception.serror_has_esr;
> +    env->serror.esr = events.exception.serror_esr;
> +
> +    return 0;
> +}
> +
>  int kvm_arch_put_registers(CPUState *cs, int level)
>  {
>      struct kvm_one_reg reg;
> @@ -727,6 +775,12 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          return ret;
>      }
>
> +    ret = kvm_put_vcpu_events(cpu);
> +    if (ret) {
> +        printf("return error kvm_put_vcpu_events: %d\n", ret);

Stray debug printf() left in?

> +        return ret;
> +    }
> +
>      if (!write_list_to_kvmstate(cpu, level)) {
>          return EINVAL;
>      }
> @@ -863,6 +917,11 @@ int kvm_arch_get_registers(CPUState *cs)
>      }
>      vfp_set_fpcr(env, fpr);
>
> +    ret = kvm_get_vcpu_events(cpu);
> +    if (ret) {
> +        return ret;
> +    }
> +
>      if (!write_kvmstate_to_list(cpu)) {
>          return EINVAL;
>      }
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 2e28d08..ead8b2a 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -172,6 +172,27 @@ static const VMStateDescription vmstate_sve = {
>  };
>  #endif /* AARCH64 */
>
> +static bool ras_needed(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +    CPUARMState *env = &cpu->env;
> +
> +    return arm_feature(env, ARM_FEATURE_RAS_EXT);

I think the best check to use here is
"return env.serror.pending != 0". If no SError is
pending then there's no new state to migrate.

We should also call this VMState something other than
"ras", because it doesn't contain RAS-specific data.
"serror" will do (so "vmstate_serror", "cpu/serror", etc).

> +}
> +
> +static const VMStateDescription vmstate_ras = {
> +    .name = "cpu/ras",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = ras_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(env.serror.pending, ARMCPU),
> +        VMSTATE_UINT32(env.serror.has_esr, ARMCPU),
> +        VMSTATE_UINT64(env.serror.esr, ARMCPU),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static bool m_needed(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
> @@ -723,6 +744,7 @@ const VMStateDescription vmstate_arm_cpu = {
>  #ifdef TARGET_AARCH64
>          &vmstate_sve,
>  #endif
> +        &vmstate_ras,
>          NULL
>      }
>  };

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 3/3] target: arm: Add support for VCPU event states

[gengdongjiu]

On 2018/8/17 22:50, Peter Maydell wrote:
> On 21 July 2018 at 18:57, Dongjiu Geng <gengdongjiu@huawei.com> wrote:
>> This patch extends the qemu-kvm state sync logic with support for
>> KVM_GET/SET_VCPU_EVENTS, giving access to yet missing SError exception.
>> And also it can support the exception state migration.
> 
>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>> ---
>> change since v4:
>> 1. Rebase the code to latest
>>
>> change since v3:
>> 1. Add a new new subsection with a suitable 'ras_needed' function
>>    controlling whether it is present
>> 2. Add a ARM_FEATURE_RAS feature bit for CPUARMState
>>
>> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
> 
> Hi; sorry it's taken me so long to get to this patchset.
> I think it's basically the right shape, but I have some
> changes to suggest below.

You are welcome, thanks for your time that give below  precious suggestion and review comments.

> 
>> ---
>>  target/arm/cpu.h     |  6 ++++++
>>  target/arm/kvm64.c   | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  target/arm/machine.c | 22 ++++++++++++++++++++
>>  3 files changed, 87 insertions(+)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index e310ffc..f00f0b6 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -645,6 +645,11 @@ typedef struct CPUARMState {
>>      const struct arm_boot_info *boot_info;
>>      /* Store GICv3CPUState to access from this struct */
>>      void *gicv3state;
>> +    struct {
>> +        uint32_t pending;
>> +        uint32_t has_esr;
>> +        uint64_t esr;
>> +    } serror;
> 
> I recommend putting this earlier in the CPUARMState struct.
> Specifically, you want to put it somewhere before the
> "end_reset_fields" marker. All fields before that are cleared
> to zero on CPU reset. If you put the struct after that then
> you would need manual code in the cpu reset function to
> reset it appropriately.
Ok, I will move it to the right place. thanks for the pointing out.

> 
>>  } CPUARMState;
>>
>>  /**
>> @@ -1486,6 +1491,7 @@ enum arm_features {
>>      ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
>>      ARM_FEATURE_V8_FCMA, /* has complex number part of v8.3 extensions.  */
>>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>> +    ARM_FEATURE_RAS_EXT, /* has RAS Extension */
>>  };
>>
>>  static inline int arm_feature(CPUARMState *env, int feature)
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index e0b8246..ebf7a00 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -527,6 +527,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>          unset_feature(&env->features, ARM_FEATURE_PMU);
>>      }
>>
>> +    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_INJECT_SERROR_ESR)) {
>> +        set_feature(&env->features, ARM_FEATURE_RAS_EXT);
>> +    }
> 
> I think this is confusing two things. Whether the host kernel
> has support for injecting SError ESRs is not the same as
> whether the guest CPU has the RAS extensions. So you don't want
> to use an ARM_FEATURE_* bit to track whether we need to migrate
> the SError ESR state.
> 
> Instead you should just use a bool variable local to this file
> to track whether the kernel has state we need to migrate.
> Compare the way we use "have_guest_debug" to track whether
> KVM_CAP_SET_GUEST_DEBUG is set. You can call this
> have_inject_serror_esr.

Ok, I will use your suggested method to do it in this file.

> 
>> +
>>      /* Do KVM_ARM_VCPU_INIT ioctl */
>>      ret = kvm_arm_vcpu_init(cs);
>>      if (ret) {
>> @@ -600,6 +604,50 @@ int kvm_arm_cpreg_level(uint64_t regidx)
>>  #define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
>>                   KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>>
>> +static int kvm_put_vcpu_events(ARMCPU *cpu)
>> +{
>> +    CPUARMState *env = &cpu->env;
>> +    struct kvm_vcpu_events events = {};
>> +
>> +    if (!kvm_has_vcpu_events()) {
>> +        return 0;
>> +    }
>> +
>> +    memset(&events, 0, sizeof(events));
>> +    events.exception.serror_pending = env->serror.pending;
>> +
>> +    if (arm_feature(env, ARM_FEATURE_RAS_EXT)) {
> 
> Here you can check have_inject_serror_esr.

Got it, will change it.

> 
>> +        events.exception.serror_has_esr = env->serror.has_esr;
>> +        events.exception.serror_esr = env->serror.esr;
>> +    }
>> +
>> +    return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
>> +}
>> +
>> +static int kvm_get_vcpu_events(ARMCPU *cpu)
>> +{
>> +    CPUARMState *env = &cpu->env;
>> +    struct kvm_vcpu_events events;
>> +    int ret;
>> +
>> +    if (!kvm_has_vcpu_events()) {
>> +        return 0;
>> +    }
>> +
>> +    memset(&events, 0, sizeof(events));
>> +    ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_VCPU_EVENTS, &events);
>> +
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    env->serror.pending = events.exception.serror_pending;
>> +    env->serror.has_esr = events.exception.serror_has_esr;
>> +    env->serror.esr = events.exception.serror_esr;
>> +
>> +    return 0;
>> +}
>> +
>>  int kvm_arch_put_registers(CPUState *cs, int level)
>>  {
>>      struct kvm_one_reg reg;
>> @@ -727,6 +775,12 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>          return ret;
>>      }
>>
>> +    ret = kvm_put_vcpu_events(cpu);
>> +    if (ret) {
>> +        printf("return error kvm_put_vcpu_events: %d\n", ret);
> 
> Stray debug printf() left in?
I will remove this printf().

> 
>> +        return ret;
>> +    }
>> +
>>      if (!write_list_to_kvmstate(cpu, level)) {
>>          return EINVAL;
>>      }
>> @@ -863,6 +917,11 @@ int kvm_arch_get_registers(CPUState *cs)
>>      }
>>      vfp_set_fpcr(env, fpr);
>>
>> +    ret = kvm_get_vcpu_events(cpu);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +
>>      if (!write_kvmstate_to_list(cpu)) {
>>          return EINVAL;
>>      }
>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>> index 2e28d08..ead8b2a 100644
>> --- a/target/arm/machine.c
>> +++ b/target/arm/machine.c
>> @@ -172,6 +172,27 @@ static const VMStateDescription vmstate_sve = {
>>  };
>>  #endif /* AARCH64 */
>>
>> +static bool ras_needed(void *opaque)
>> +{
>> +    ARMCPU *cpu = opaque;
>> +    CPUARMState *env = &cpu->env;
>> +
>> +    return arm_feature(env, ARM_FEATURE_RAS_EXT);
> 
> I think the best check to use here is
> "return env.serror.pending != 0". If no SError is
> pending then there's no new state to migrate.

Ok, it looks reasonable to check "return env.serror.pending != 0",
I will change it.

> 
> We should also call this VMState something other than
> "ras", because it doesn't contain RAS-specific data.
> "serror" will do (so "vmstate_serror", "cpu/serror", etc).

OK, I will rename it to "vmstate_serror" from vmstate_ras.

> 
>> +}
>> +
>> +static const VMStateDescription vmstate_ras = {
>> +    .name = "cpu/ras",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = ras_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(env.serror.pending, ARMCPU),
>> +        VMSTATE_UINT32(env.serror.has_esr, ARMCPU),
>> +        VMSTATE_UINT64(env.serror.esr, ARMCPU),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>  static bool m_needed(void *opaque)
>>  {
>>      ARMCPU *cpu = opaque;
>> @@ -723,6 +744,7 @@ const VMStateDescription vmstate_arm_cpu = {
>>  #ifdef TARGET_AARCH64
>>          &vmstate_sve,
>>  #endif
>> +        &vmstate_ras,
>>          NULL
>>      }
>>  };
> 
> thanks
> -- PMM
> 
> .
>