[PATCH 0/2] accel/kvm: Sanitize KVM_HAVE_MCE_INJECTION definition

[PATCH 0/2] accel/kvm: Sanitize KVM_HAVE_MCE_INJECTION definition

[Philippe Mathieu-Daudé]

Trivial replacement of KVM_HAVE_MCE_INJECTION by
KVM_ARCH_HAVE_MCE_INJECTION (not the "ARCH_" difference).

Philippe Mathieu-Daudé (2):
  accel/kvm: Define KVM_ARCH_HAVE_MCE_INJECTION in each target
  accel/kvm: Directly check KVM_ARCH_HAVE_MCE_INJECTION value in place

 include/sysemu/kvm.h         |  7 ++++++-
 target/arm/cpu-param.h       |  5 +++++
 target/arm/cpu.h             |  4 ----
 target/i386/cpu-param.h      |  2 ++
 target/i386/cpu.h            |  2 --
 target/loongarch/cpu-param.h |  2 ++
 target/mips/cpu-param.h      |  2 ++
 target/ppc/cpu-param.h       |  2 ++
 target/riscv/cpu-param.h     |  2 ++
 target/s390x/cpu-param.h     |  2 ++
 accel/kvm/kvm-all.c          | 10 +++++-----
 11 files changed, 28 insertions(+), 12 deletions(-)

-- 
2.41.0

[PATCH 1/2] accel/kvm: Define KVM_ARCH_HAVE_MCE_INJECTION in each target

[Philippe Mathieu-Daudé]

Instead of having KVM_HAVE_MCE_INJECTION optionally defined,
always define KVM_ARCH_HAVE_MCE_INJECTION for each target,
and set KVM_HAVE_MCE_INJECTION if it is not zero.

It is now clearer for KVM targets to detect if they lack the
MCE injection implementation. Also, moving headers around
is no more painful, because if a target neglects to define
KVM_ARCH_HAVE_MCE_INJECTION, the build will fail.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
I'd rather keep "cpu-param.h" simple/short.

Is a per-target "kvm-param.h" any better?
Or per-target "accel-param.h"? For example TCG_GUEST_DEFAULT_MO
is TCG specific and could also go there. Otherwise it will go
in "cpu-param.h".
---
 include/sysemu/kvm.h         | 5 +++++
 target/arm/cpu-param.h       | 5 +++++
 target/arm/cpu.h             | 4 ----
 target/i386/cpu-param.h      | 2 ++
 target/i386/cpu.h            | 2 --
 target/loongarch/cpu-param.h | 2 ++
 target/mips/cpu-param.h      | 2 ++
 target/ppc/cpu-param.h       | 2 ++
 target/riscv/cpu-param.h     | 2 ++
 target/s390x/cpu-param.h     | 2 ++
 10 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index d614878164..2e9aa2fc53 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -212,6 +212,7 @@ int kvm_on_sigbus(int code, void *addr);
 
 #ifdef NEED_CPU_H
 #include "cpu.h"
+#include "cpu-param.h"
 
 void kvm_flush_coalesced_mmio_buffer(void);
 
@@ -349,6 +350,10 @@ bool kvm_vcpu_id_is_valid(int vcpu_id);
 /* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
 unsigned long kvm_arch_vcpu_id(CPUState *cpu);
 
+#if KVM_ARCH_HAVE_MCE_INJECTION
+#define KVM_HAVE_MCE_INJECTION
+#endif
+
 #ifdef KVM_HAVE_MCE_INJECTION
 void kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
 #endif
diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
index f9b462a98f..d71cc29864 100644
--- a/target/arm/cpu-param.h
+++ b/target/arm/cpu-param.h
@@ -30,7 +30,12 @@
  */
 # define TARGET_PAGE_BITS_VARY
 # define TARGET_PAGE_BITS_MIN  10
+#endif
 
+#ifdef TARGET_AARCH64
+#define KVM_ARCH_HAVE_MCE_INJECTION 1
+#else
+#define KVM_ARCH_HAVE_MCE_INJECTION 0
 #endif
 
 #endif
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ec276fcd57..f92c8d3b88 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -30,10 +30,6 @@
 /* ARM processors have a weak memory model */
 #define TCG_GUEST_DEFAULT_MO      (0)
 
-#ifdef TARGET_AARCH64
-#define KVM_HAVE_MCE_INJECTION 1
-#endif
-
 #define EXCP_UDEF            1   /* undefined instruction */
 #define EXCP_SWI             2   /* software interrupt */
 #define EXCP_PREFETCH_ABORT  3
diff --git a/target/i386/cpu-param.h b/target/i386/cpu-param.h
index 911b4cd51b..5933b0b756 100644
--- a/target/i386/cpu-param.h
+++ b/target/i386/cpu-param.h
@@ -24,4 +24,6 @@
 #endif
 #define TARGET_PAGE_BITS 12
 
+#define KVM_ARCH_HAVE_MCE_INJECTION 1
+
 #endif
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 7f0786e8b9..230ab1cded 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -33,8 +33,6 @@
 /* The x86 has a strong memory model with some store-after-load re-ordering */
 #define TCG_GUEST_DEFAULT_MO      (TCG_MO_ALL & ~TCG_MO_ST_LD)
 
-#define KVM_HAVE_MCE_INJECTION 1
-
 /* support for self modifying code even if the modified instruction is
    close to the modifying instruction */
 #define TARGET_HAS_PRECISE_SMC
diff --git a/target/loongarch/cpu-param.h b/target/loongarch/cpu-param.h
index cfe195db4e..f69a94e6b5 100644
--- a/target/loongarch/cpu-param.h
+++ b/target/loongarch/cpu-param.h
@@ -14,4 +14,6 @@
 
 #define TARGET_PAGE_BITS 12
 
+#define KVM_ARCH_HAVE_MCE_INJECTION 0
+
 #endif
diff --git a/target/mips/cpu-param.h b/target/mips/cpu-param.h
index 594c91a156..45c885e00e 100644
--- a/target/mips/cpu-param.h
+++ b/target/mips/cpu-param.h
@@ -30,4 +30,6 @@
 #define TARGET_PAGE_BITS_MIN 12
 #endif
 
+#define KVM_ARCH_HAVE_MCE_INJECTION 0
+
 #endif
diff --git a/target/ppc/cpu-param.h b/target/ppc/cpu-param.h
index 0a0416e0a8..9975ae73ab 100644
--- a/target/ppc/cpu-param.h
+++ b/target/ppc/cpu-param.h
@@ -33,4 +33,6 @@
 #endif
 #define TARGET_PAGE_BITS 12
 
+#define KVM_ARCH_HAVE_MCE_INJECTION 0
+
 #endif
diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
index b2a9396dec..e6199f4f6d 100644
--- a/target/riscv/cpu-param.h
+++ b/target/riscv/cpu-param.h
@@ -28,4 +28,6 @@
  *  - M mode HLV/HLVX/HSV 0b111
  */
 
+#define KVM_ARCH_HAVE_MCE_INJECTION 0
+
 #endif
diff --git a/target/s390x/cpu-param.h b/target/s390x/cpu-param.h
index 84ca08626b..4728b3957e 100644
--- a/target/s390x/cpu-param.h
+++ b/target/s390x/cpu-param.h
@@ -13,4 +13,6 @@
 #define TARGET_PHYS_ADDR_SPACE_BITS 64
 #define TARGET_VIRT_ADDR_SPACE_BITS 64
 
+#define KVM_ARCH_HAVE_MCE_INJECTION 0
+
 #endif
-- 
2.41.0

[PATCH 2/2] accel/kvm: Directly check KVM_ARCH_HAVE_MCE_INJECTION value in place

[Philippe Mathieu-Daudé]

Directly use KVM_ARCH_HAVE_MCE_INJECTION instead of
KVM_HAVE_MCE_INJECTION.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/sysemu/kvm.h |  6 +++---
 accel/kvm/kvm-all.c  | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 2e9aa2fc53..4107678233 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -350,11 +350,11 @@ bool kvm_vcpu_id_is_valid(int vcpu_id);
 /* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
 unsigned long kvm_arch_vcpu_id(CPUState *cpu);
 
-#if KVM_ARCH_HAVE_MCE_INJECTION
-#define KVM_HAVE_MCE_INJECTION
+#ifndef KVM_ARCH_HAVE_MCE_INJECTION
+#error Missing KVM_ARCH_HAVE_MCE_INJECTION definition in "cpu.h"
 #endif
 
-#ifdef KVM_HAVE_MCE_INJECTION
+#if KVM_ARCH_HAVE_MCE_INJECTION
 void kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
 #endif
 
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 49e755ec4a..b98c0843b1 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2750,7 +2750,7 @@ void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu)
     run_on_cpu(cpu, do_kvm_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
 }
 
-#ifdef KVM_HAVE_MCE_INJECTION
+#if KVM_ARCH_HAVE_MCE_INJECTION
 static __thread void *pending_sigbus_addr;
 static __thread int pending_sigbus_code;
 static __thread bool have_sigbus_pending;
@@ -2855,7 +2855,7 @@ int kvm_cpu_exec(CPUState *cpu)
 
         attrs = kvm_arch_post_run(cpu, run);
 
-#ifdef KVM_HAVE_MCE_INJECTION
+#if KVM_ARCH_HAVE_MCE_INJECTION
         if (unlikely(have_sigbus_pending)) {
             bql_lock();
             kvm_arch_on_sigbus_vcpu(cpu, pending_sigbus_code,
@@ -3339,7 +3339,7 @@ void kvm_init_cpu_signals(CPUState *cpu)
     sigaction(SIG_IPI, &sigact, NULL);
 
     pthread_sigmask(SIG_BLOCK, NULL, &set);
-#if defined KVM_HAVE_MCE_INJECTION
+#if KVM_ARCH_HAVE_MCE_INJECTION
     sigdelset(&set, SIGBUS);
     pthread_sigmask(SIG_SETMASK, &set, NULL);
 #endif
@@ -3358,7 +3358,7 @@ void kvm_init_cpu_signals(CPUState *cpu)
 /* Called asynchronously in VCPU thread.  */
 int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
 {
-#ifdef KVM_HAVE_MCE_INJECTION
+#if KVM_ARCH_HAVE_MCE_INJECTION
     if (have_sigbus_pending) {
         return 1;
     }
@@ -3375,7 +3375,7 @@ int kvm_on_sigbus_vcpu(CPUState *cpu, int code, void *addr)
 /* Called synchronously (via signalfd) in main thread.  */
 int kvm_on_sigbus(int code, void *addr)
 {
-#ifdef KVM_HAVE_MCE_INJECTION
+#if KVM_ARCH_HAVE_MCE_INJECTION
     /* Action required MCE kills the process if SIGBUS is blocked.  Because
      * that's what happens in the I/O thread, where we handle MCE via signalfd,
      * we can only get action optional here.
-- 
2.41.0

Re: [PATCH 1/2] accel/kvm: Define KVM_ARCH_HAVE_MCE_INJECTION in each target

[john strange]

It might be good to replace "MCE" in kvm-all.c with a generic 
abstraction for error injection to avoid carrying the MCE constructs 
into non-x86 sigbus handlers.

thanks,
-john

On 1/24/24 07:54, Philippe Mathieu-Daudé wrote:
> Instead of having KVM_HAVE_MCE_INJECTION optionally defined,
> always define KVM_ARCH_HAVE_MCE_INJECTION for each target,
> and set KVM_HAVE_MCE_INJECTION if it is not zero.
>
> It is now clearer for KVM targets to detect if they lack the
> MCE injection implementation. Also, moving headers around
> is no more painful, because if a target neglects to define
> KVM_ARCH_HAVE_MCE_INJECTION, the build will fail.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> I'd rather keep "cpu-param.h" simple/short.
>
> Is a per-target "kvm-param.h" any better?
> Or per-target "accel-param.h"? For example TCG_GUEST_DEFAULT_MO
> is TCG specific and could also go there. Otherwise it will go
> in "cpu-param.h".
> ---
>   include/sysemu/kvm.h         | 5 +++++
>   target/arm/cpu-param.h       | 5 +++++
>   target/arm/cpu.h             | 4 ----
>   target/i386/cpu-param.h      | 2 ++
>   target/i386/cpu.h            | 2 --
>   target/loongarch/cpu-param.h | 2 ++
>   target/mips/cpu-param.h      | 2 ++
>   target/ppc/cpu-param.h       | 2 ++
>   target/riscv/cpu-param.h     | 2 ++
>   target/s390x/cpu-param.h     | 2 ++
>   10 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index d614878164..2e9aa2fc53 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -212,6 +212,7 @@ int kvm_on_sigbus(int code, void *addr);
>   
>   #ifdef NEED_CPU_H
>   #include "cpu.h"
> +#include "cpu-param.h"
>   
>   void kvm_flush_coalesced_mmio_buffer(void);
>   
> @@ -349,6 +350,10 @@ bool kvm_vcpu_id_is_valid(int vcpu_id);
>   /* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
>   unsigned long kvm_arch_vcpu_id(CPUState *cpu);
>   
> +#if KVM_ARCH_HAVE_MCE_INJECTION
> +#define KVM_HAVE_MCE_INJECTION
> +#endif
> +
>   #ifdef KVM_HAVE_MCE_INJECTION
>   void kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
>   #endif
> diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
> index f9b462a98f..d71cc29864 100644
> --- a/target/arm/cpu-param.h
> +++ b/target/arm/cpu-param.h
> @@ -30,7 +30,12 @@
>    */
>   # define TARGET_PAGE_BITS_VARY
>   # define TARGET_PAGE_BITS_MIN  10
> +#endif
>   
> +#ifdef TARGET_AARCH64
> +#define KVM_ARCH_HAVE_MCE_INJECTION 1
> +#else
> +#define KVM_ARCH_HAVE_MCE_INJECTION 0
>   #endif
>   
>   #endif
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index ec276fcd57..f92c8d3b88 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -30,10 +30,6 @@
>   /* ARM processors have a weak memory model */
>   #define TCG_GUEST_DEFAULT_MO      (0)
>   
> -#ifdef TARGET_AARCH64
> -#define KVM_HAVE_MCE_INJECTION 1
> -#endif
> -
>   #define EXCP_UDEF            1   /* undefined instruction */
>   #define EXCP_SWI             2   /* software interrupt */
>   #define EXCP_PREFETCH_ABORT  3
> diff --git a/target/i386/cpu-param.h b/target/i386/cpu-param.h
> index 911b4cd51b..5933b0b756 100644
> --- a/target/i386/cpu-param.h
> +++ b/target/i386/cpu-param.h
> @@ -24,4 +24,6 @@
>   #endif
>   #define TARGET_PAGE_BITS 12
>   
> +#define KVM_ARCH_HAVE_MCE_INJECTION 1
> +
>   #endif
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 7f0786e8b9..230ab1cded 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -33,8 +33,6 @@
>   /* The x86 has a strong memory model with some store-after-load re-ordering */
>   #define TCG_GUEST_DEFAULT_MO      (TCG_MO_ALL & ~TCG_MO_ST_LD)
>   
> -#define KVM_HAVE_MCE_INJECTION 1
> -
>   /* support for self modifying code even if the modified instruction is
>      close to the modifying instruction */
>   #define TARGET_HAS_PRECISE_SMC
> diff --git a/target/loongarch/cpu-param.h b/target/loongarch/cpu-param.h
> index cfe195db4e..f69a94e6b5 100644
> --- a/target/loongarch/cpu-param.h
> +++ b/target/loongarch/cpu-param.h
> @@ -14,4 +14,6 @@
>   
>   #define TARGET_PAGE_BITS 12
>   
> +#define KVM_ARCH_HAVE_MCE_INJECTION 0
> +
>   #endif
> diff --git a/target/mips/cpu-param.h b/target/mips/cpu-param.h
> index 594c91a156..45c885e00e 100644
> --- a/target/mips/cpu-param.h
> +++ b/target/mips/cpu-param.h
> @@ -30,4 +30,6 @@
>   #define TARGET_PAGE_BITS_MIN 12
>   #endif
>   
> +#define KVM_ARCH_HAVE_MCE_INJECTION 0
> +
>   #endif
> diff --git a/target/ppc/cpu-param.h b/target/ppc/cpu-param.h
> index 0a0416e0a8..9975ae73ab 100644
> --- a/target/ppc/cpu-param.h
> +++ b/target/ppc/cpu-param.h
> @@ -33,4 +33,6 @@
>   #endif
>   #define TARGET_PAGE_BITS 12
>   
> +#define KVM_ARCH_HAVE_MCE_INJECTION 0
> +
>   #endif
> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
> index b2a9396dec..e6199f4f6d 100644
> --- a/target/riscv/cpu-param.h
> +++ b/target/riscv/cpu-param.h
> @@ -28,4 +28,6 @@
>    *  - M mode HLV/HLVX/HSV 0b111
>    */
>   
> +#define KVM_ARCH_HAVE_MCE_INJECTION 0
> +
>   #endif
> diff --git a/target/s390x/cpu-param.h b/target/s390x/cpu-param.h
> index 84ca08626b..4728b3957e 100644
> --- a/target/s390x/cpu-param.h
> +++ b/target/s390x/cpu-param.h
> @@ -13,4 +13,6 @@
>   #define TARGET_PHYS_ADDR_SPACE_BITS 64
>   #define TARGET_VIRT_ADDR_SPACE_BITS 64
>   
> +#define KVM_ARCH_HAVE_MCE_INJECTION 0
> +
>   #endif

Re: [PATCH 0/2] accel/kvm: Sanitize KVM_HAVE_MCE_INJECTION definition

[Paolo Bonzini]

On Wed, Jan 24, 2024 at 4:54 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Trivial replacement of KVM_HAVE_MCE_INJECTION by
> KVM_ARCH_HAVE_MCE_INJECTION (not the "ARCH_" difference).

I am confused, why can't you just rename the symbol and instead you go
through this change?

Paolo

> Philippe Mathieu-Daudé (2):
>   accel/kvm: Define KVM_ARCH_HAVE_MCE_INJECTION in each target
>   accel/kvm: Directly check KVM_ARCH_HAVE_MCE_INJECTION value in place
>
>  include/sysemu/kvm.h         |  7 ++++++-
>  target/arm/cpu-param.h       |  5 +++++
>  target/arm/cpu.h             |  4 ----
>  target/i386/cpu-param.h      |  2 ++
>  target/i386/cpu.h            |  2 --
>  target/loongarch/cpu-param.h |  2 ++
>  target/mips/cpu-param.h      |  2 ++
>  target/ppc/cpu-param.h       |  2 ++
>  target/riscv/cpu-param.h     |  2 ++
>  target/s390x/cpu-param.h     |  2 ++
>  accel/kvm/kvm-all.c          | 10 +++++-----
>  11 files changed, 28 insertions(+), 12 deletions(-)
>
> --
> 2.41.0
>