[PATCH v5 00/11] i386: miscellaneous cleanup

[PATCH v5 00/11] i386: miscellaneous cleanup

[Zhao Liu]

Hi Paolo and all,

Is it necessary to include the first patch (AVX10 cleanup/fix) in v9.2?

Others are for v10.0.

Compared with v4 [1],
 * patch 1 (AVX10 fix) and patch 9 (RAPL cleanup) are newly added.
 * rebased on commit 9a7b0a8618b1 ("Merge tag 'pull-aspeed-20241104' of
   https://github.com/legoater/qemu into staging").


Background and Introduction
===========================

This series picks cleanup from my previous kvmclock [2] (as other
renaming attempts were temporarily put on hold).

In addition, this series also include the cleanup on a historically
workaround, recent comment of coco interface [3] and error handling
corner cases in kvm_arch_init().

Avoiding the fragmentation of these misc cleanups, I consolidated them
all in one series and was able to tackle them in one go!

[1]: https://lore.kernel.org/qemu-devel/20240716161015.263031-1-zhao1.liu@intel.com/
[2]: https://lore.kernel.org/qemu-devel/20240329101954.3954987-1-zhao1.liu@linux.intel.com/
[3]: https://lore.kernel.org/qemu-devel/2815f0f1-9e20-4985-849c-d74c6cdc94ae@intel.com/

Thanks and Best Regards,
Zhao
---
Zhao Liu (11):
  i386/cpu: Mark avx10_version filtered when prefix is NULL
  target/i386/kvm: Add feature bit definitions for KVM CPUID
  target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and
    MSR_KVM_SYSTEM_TIME definitions
  target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled
  target/i386/kvm: Save/load MSRs of kvmclock2
    (KVM_FEATURE_CLOCKSOURCE2)
  target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo
  target/i386/confidential-guest: Fix comment of
    x86_confidential_guest_kvm_type()
  target/i386/kvm: Clean up return values of MSR filter related
    functions
  target/i386/kvm: Return -1 when kvm_msr_energy_thread_init() fails
  target/i386/kvm: Clean up error handling in kvm_arch_init()
  target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with
    KVM_MSR_FILTER_MAX_RANGES

 hw/i386/kvm/clock.c              |   5 +-
 target/i386/confidential-guest.h |   2 +-
 target/i386/cpu.c                |   6 +-
 target/i386/cpu.h                |  25 ++++
 target/i386/kvm/kvm.c            | 211 +++++++++++++++++--------------
 5 files changed, 145 insertions(+), 104 deletions(-)

-- 
2.34.1

[PATCH v5 01/11 for v9.2?] i386/cpu: Mark avx10_version filtered when prefix is NULL

[Zhao Liu]

In x86_cpu_filter_features(), if host doesn't support AVX10, the
configured avx10_version should be marked as filtered regardless of
whether prefix is NULL or not.

Check prefix before warn_report() instead of checking for
have_filtered_features.

Cc: qemu-stable@nongnu.org
Fixes: commit bccfb846fd52 ("target/i386: add AVX10 feature and AVX10 version property")
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
v5: new commit.
---
 target/i386/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3baa95481fbc..77c1233daa13 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7711,8 +7711,10 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
             env->avx10_version = version;
             have_filtered_features = true;
         }
-    } else if (env->avx10_version && prefix) {
-        warn_report("%s: avx10.%d.", prefix, env->avx10_version);
+    } else if (env->avx10_version) {
+        if (prefix) {
+            warn_report("%s: avx10.%d.", prefix, env->avx10_version);
+        }
         have_filtered_features = true;
     }
 
-- 
2.34.1

[PATCH v5 02/11] target/i386/kvm: Add feature bit definitions for KVM CPUID

[Zhao Liu]

Add feature definitions for KVM_CPUID_FEATURES in CPUID (
CPUID[4000_0001].EAX and CPUID[4000_0001].EDX), to get rid of lots of
offset calculations.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
v3: Resolved a rebasing conflict.
v2: Changed the prefix from CPUID_FEAT_KVM_* to CPUID_KVM_*. (Xiaoyao)
---
 hw/i386/kvm/clock.c   |  5 ++---
 target/i386/cpu.h     | 23 +++++++++++++++++++++++
 target/i386/kvm/kvm.c | 28 ++++++++++++++--------------
 3 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 40aa9a32c32c..ce416c05a3d0 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -27,7 +27,6 @@
 #include "qapi/error.h"
 
 #include <linux/kvm.h>
-#include "standard-headers/asm-x86/kvm_para.h"
 #include "qom/object.h"
 
 #define TYPE_KVM_CLOCK "kvmclock"
@@ -334,8 +333,8 @@ void kvmclock_create(bool create_always)
 
     assert(kvm_enabled());
     if (create_always ||
-        cpu->env.features[FEAT_KVM] & ((1ULL << KVM_FEATURE_CLOCKSOURCE) |
-                                       (1ULL << KVM_FEATURE_CLOCKSOURCE2))) {
+        cpu->env.features[FEAT_KVM] & (CPUID_KVM_CLOCK |
+                                       CPUID_KVM_CLOCK2)) {
         sysbus_create_simple(TYPE_KVM_CLOCK, -1, NULL);
     }
 }
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 59959b8b7a4d..c4ec64e0078f 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -29,6 +29,7 @@
 #include "qapi/qapi-types-common.h"
 #include "qemu/cpu-float.h"
 #include "qemu/timer.h"
+#include "standard-headers/asm-x86/kvm_para.h"
 
 #define XEN_NR_VIRQS 24
 
@@ -1009,6 +1010,28 @@ uint64_t x86_cpu_get_supported_feature_word(X86CPU *cpu, FeatureWord w);
 #define CPUID_8000_0007_EBX_OVERFLOW_RECOV    (1U << 0)
 #define CPUID_8000_0007_EBX_SUCCOR      (1U << 1)
 
+/* (Old) KVM paravirtualized clocksource */
+#define CPUID_KVM_CLOCK            (1U << KVM_FEATURE_CLOCKSOURCE)
+/* (New) KVM specific paravirtualized clocksource */
+#define CPUID_KVM_CLOCK2           (1U << KVM_FEATURE_CLOCKSOURCE2)
+/* KVM asynchronous page fault */
+#define CPUID_KVM_ASYNCPF          (1U << KVM_FEATURE_ASYNC_PF)
+/* KVM stolen (when guest vCPU is not running) time accounting */
+#define CPUID_KVM_STEAL_TIME       (1U << KVM_FEATURE_STEAL_TIME)
+/* KVM paravirtualized end-of-interrupt signaling */
+#define CPUID_KVM_PV_EOI           (1U << KVM_FEATURE_PV_EOI)
+/* KVM paravirtualized spinlocks support */
+#define CPUID_KVM_PV_UNHALT        (1U << KVM_FEATURE_PV_UNHALT)
+/* KVM host-side polling on HLT control from the guest */
+#define CPUID_KVM_POLL_CONTROL     (1U << KVM_FEATURE_POLL_CONTROL)
+/* KVM interrupt based asynchronous page fault*/
+#define CPUID_KVM_ASYNCPF_INT      (1U << KVM_FEATURE_ASYNC_PF_INT)
+/* KVM 'Extended Destination ID' support for external interrupts */
+#define CPUID_KVM_MSI_EXT_DEST_ID  (1U << KVM_FEATURE_MSI_EXT_DEST_ID)
+
+/* Hint to KVM that vCPUs expect never preempted for an unlimited time */
+#define CPUID_KVM_HINTS_REALTIME    (1U << KVM_HINTS_REALTIME)
+
 /* CLZERO instruction */
 #define CPUID_8000_0008_EBX_CLZERO      (1U << 0)
 /* Always save/restore FP error pointers */
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 8e17942c3ba1..4fb822511a12 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -564,13 +564,13 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
          * be enabled without the in-kernel irqchip
          */
         if (!kvm_irqchip_in_kernel()) {
-            ret &= ~(1U << KVM_FEATURE_PV_UNHALT);
+            ret &= ~CPUID_KVM_PV_UNHALT;
         }
         if (kvm_irqchip_is_split()) {
-            ret |= 1U << KVM_FEATURE_MSI_EXT_DEST_ID;
+            ret |= CPUID_KVM_MSI_EXT_DEST_ID;
         }
     } else if (function == KVM_CPUID_FEATURES && reg == R_EDX) {
-        ret |= 1U << KVM_HINTS_REALTIME;
+        ret |= CPUID_KVM_HINTS_REALTIME;
     }
 
     if (current_machine->cgs) {
@@ -3963,20 +3963,20 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
         kvm_msr_entry_add(cpu, MSR_IA32_TSC, env->tsc);
         kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, env->system_time_msr);
         kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
-        if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF_INT)) {
+        if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
             kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, env->async_pf_int_msr);
         }
-        if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF)) {
+        if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF) {
             kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_EN, env->async_pf_en_msr);
         }
-        if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_PV_EOI)) {
+        if (env->features[FEAT_KVM] & CPUID_KVM_PV_EOI) {
             kvm_msr_entry_add(cpu, MSR_KVM_PV_EOI_EN, env->pv_eoi_en_msr);
         }
-        if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_STEAL_TIME)) {
+        if (env->features[FEAT_KVM] & CPUID_KVM_STEAL_TIME) {
             kvm_msr_entry_add(cpu, MSR_KVM_STEAL_TIME, env->steal_time_msr);
         }
 
-        if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_POLL_CONTROL)) {
+        if (env->features[FEAT_KVM] & CPUID_KVM_POLL_CONTROL) {
             kvm_msr_entry_add(cpu, MSR_KVM_POLL_CONTROL, env->poll_control_msr);
         }
 
@@ -4441,19 +4441,19 @@ static int kvm_get_msrs(X86CPU *cpu)
 #endif
     kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, 0);
     kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, 0);
-    if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF_INT)) {
+    if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
         kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, 0);
     }
-    if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_ASYNC_PF)) {
+    if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF) {
         kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_EN, 0);
     }
-    if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_PV_EOI)) {
+    if (env->features[FEAT_KVM] & CPUID_KVM_PV_EOI) {
         kvm_msr_entry_add(cpu, MSR_KVM_PV_EOI_EN, 0);
     }
-    if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_STEAL_TIME)) {
+    if (env->features[FEAT_KVM] & CPUID_KVM_STEAL_TIME) {
         kvm_msr_entry_add(cpu, MSR_KVM_STEAL_TIME, 0);
     }
-    if (env->features[FEAT_KVM] & (1 << KVM_FEATURE_POLL_CONTROL)) {
+    if (env->features[FEAT_KVM] & CPUID_KVM_POLL_CONTROL) {
         kvm_msr_entry_add(cpu, MSR_KVM_POLL_CONTROL, 1);
     }
     if (has_architectural_pmu_version > 0) {
@@ -6180,7 +6180,7 @@ uint64_t kvm_swizzle_msi_ext_dest_id(uint64_t address)
         return address;
     }
     env = &X86_CPU(first_cpu)->env;
-    if (!(env->features[FEAT_KVM] & (1 << KVM_FEATURE_MSI_EXT_DEST_ID))) {
+    if (!(env->features[FEAT_KVM] & CPUID_KVM_MSI_EXT_DEST_ID)) {
         return address;
     }
 
-- 
2.34.1

[PATCH v5 03/11] target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and MSR_KVM_SYSTEM_TIME definitions

[Zhao Liu]

These 2 MSRs have been already defined in kvm_para.h (standard-headers/
asm-x86/kvm_para.h).

Remove QEMU local definitions to avoid duplication.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
 target/i386/kvm/kvm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 4fb822511a12..54520a77d6af 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -95,9 +95,6 @@
 #define KVM_APIC_BUS_CYCLE_NS       1
 #define KVM_APIC_BUS_FREQUENCY      (1000000000ULL / KVM_APIC_BUS_CYCLE_NS)
 
-#define MSR_KVM_WALL_CLOCK  0x11
-#define MSR_KVM_SYSTEM_TIME 0x12
-
 /* A 4096-byte buffer can hold the 8-byte kvm_msrs header, plus
  * 255 kvm_msr_entry structs */
 #define MSR_BUF_SIZE 4096
-- 
2.34.1

[PATCH v5 04/11] target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled

[Zhao Liu]

MSR_KVM_SYSTEM_TIME and MSR_KVM_WALL_CLOCK are attached with the (old)
kvmclock feature (KVM_FEATURE_CLOCKSOURCE).

So, just save/load them only when kvmclock (KVM_FEATURE_CLOCKSOURCE) is
enabled.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
 target/i386/kvm/kvm.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 54520a77d6af..4aba034865bb 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3958,8 +3958,10 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
      */
     if (level >= KVM_PUT_RESET_STATE) {
         kvm_msr_entry_add(cpu, MSR_IA32_TSC, env->tsc);
-        kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, env->system_time_msr);
-        kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
+        if (env->features[FEAT_KVM] & CPUID_KVM_CLOCK) {
+            kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, env->system_time_msr);
+            kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
+        }
         if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
             kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, env->async_pf_int_msr);
         }
@@ -4436,8 +4438,10 @@ static int kvm_get_msrs(X86CPU *cpu)
         }
     }
 #endif
-    kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, 0);
-    kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, 0);
+    if (env->features[FEAT_KVM] & CPUID_KVM_CLOCK) {
+        kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, 0);
+        kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, 0);
+    }
     if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
         kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, 0);
     }
-- 
2.34.1

[PATCH v5 05/11] target/i386/kvm: Save/load MSRs of kvmclock2 (KVM_FEATURE_CLOCKSOURCE2)

[Zhao Liu]

MSR_KVM_SYSTEM_TIME_NEW and MSR_KVM_WALL_CLOCK_NEW are bound to
kvmclock2 (KVM_FEATURE_CLOCKSOURCE2).

Add the save/load support for these 2 MSRs just like kvmclock MSRs.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
 target/i386/cpu.h     |  2 ++
 target/i386/kvm/kvm.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index c4ec64e0078f..79c28a48eb70 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1886,6 +1886,8 @@ typedef struct CPUArchState {
 
     uint64_t system_time_msr;
     uint64_t wall_clock_msr;
+    uint64_t system_time_new_msr;
+    uint64_t wall_clock_new_msr;
     uint64_t steal_time_msr;
     uint64_t async_pf_en_msr;
     uint64_t async_pf_int_msr;
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 4aba034865bb..b175cd4a4bcb 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3962,6 +3962,12 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
             kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, env->system_time_msr);
             kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, env->wall_clock_msr);
         }
+        if (env->features[FEAT_KVM] & CPUID_KVM_CLOCK2) {
+            kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME_NEW,
+                              env->system_time_new_msr);
+            kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK_NEW,
+                              env->wall_clock_new_msr);
+        }
         if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
             kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, env->async_pf_int_msr);
         }
@@ -4442,6 +4448,10 @@ static int kvm_get_msrs(X86CPU *cpu)
         kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME, 0);
         kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK, 0);
     }
+    if (env->features[FEAT_KVM] & CPUID_KVM_CLOCK2) {
+        kvm_msr_entry_add(cpu, MSR_KVM_SYSTEM_TIME_NEW, 0);
+        kvm_msr_entry_add(cpu, MSR_KVM_WALL_CLOCK_NEW, 0);
+    }
     if (env->features[FEAT_KVM] & CPUID_KVM_ASYNCPF_INT) {
         kvm_msr_entry_add(cpu, MSR_KVM_ASYNC_PF_INT, 0);
     }
@@ -4708,6 +4718,12 @@ static int kvm_get_msrs(X86CPU *cpu)
         case MSR_KVM_WALL_CLOCK:
             env->wall_clock_msr = msrs[i].data;
             break;
+        case MSR_KVM_SYSTEM_TIME_NEW:
+            env->system_time_new_msr = msrs[i].data;
+            break;
+        case MSR_KVM_WALL_CLOCK_NEW:
+            env->wall_clock_new_msr = msrs[i].data;
+            break;
         case MSR_MCG_STATUS:
             env->mcg_status = msrs[i].data;
             break;
-- 
2.34.1

[PATCH v5 06/11] target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo

[Zhao Liu]

The KVM_X86_DISABLE_EXITS_HTL typo has been fixed in commit
77d361b13c19 ("linux-headers: Update to kernel mainline commit
b357bf602").

Drop the related workaround.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
 target/i386/kvm/kvm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index b175cd4a4bcb..1fb4bd19fcf7 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3085,10 +3085,7 @@ static int kvm_vm_set_tss_addr(KVMState *s, uint64_t tss_base)
 static int kvm_vm_enable_disable_exits(KVMState *s)
 {
     int disable_exits = kvm_check_extension(s, KVM_CAP_X86_DISABLE_EXITS);
-/* Work around for kernel header with a typo. TODO: fix header and drop. */
-#if defined(KVM_X86_DISABLE_EXITS_HTL) && !defined(KVM_X86_DISABLE_EXITS_HLT)
-#define KVM_X86_DISABLE_EXITS_HLT KVM_X86_DISABLE_EXITS_HTL
-#endif
+
     if (disable_exits) {
         disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT |
                           KVM_X86_DISABLE_EXITS_HLT |
-- 
2.34.1

[PATCH v5 07/11] target/i386/confidential-guest: Fix comment of x86_confidential_guest_kvm_type()

[Zhao Liu]

Update the comment to match the X86ConfidentialGuestClass
implementation.

Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
 target/i386/confidential-guest.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/confidential-guest.h b/target/i386/confidential-guest.h
index 7342d2843aa5..c90a59bac41a 100644
--- a/target/i386/confidential-guest.h
+++ b/target/i386/confidential-guest.h
@@ -46,7 +46,7 @@ struct X86ConfidentialGuestClass {
 /**
  * x86_confidential_guest_kvm_type:
  *
- * Calls #X86ConfidentialGuestClass.unplug callback of @plug_handler.
+ * Calls #X86ConfidentialGuestClass.kvm_type() callback.
  */
 static inline int x86_confidential_guest_kvm_type(X86ConfidentialGuest *cg)
 {
-- 
2.34.1

[PATCH v5 08/11] target/i386/kvm: Clean up return values of MSR filter related functions

[Zhao Liu]

Before commit 0cc42e63bb54 ("kvm/i386: refactor kvm_arch_init and split
it into smaller functions"), error_report() attempts to print the error
code from kvm_filter_msr(). However, printing error code does not work
due to kvm_filter_msr() returns bool instead int.

0cc42e63bb54 fixed the error by removing error code printing, but this
lost useful error messages. Bring it back by making kvm_filter_msr()
return int.

This also makes the function call chain processing clearer, allowing for
better handling of error result propagation from kvm_filter_msr() to
kvm_arch_init(), preparing for the subsequent cleanup work of error
handling in kvm_arch_init().

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
v5: Rebased and cleaned up kvm_vm_enable_energy_msrs() as well.
v4: Returned kvm_vm_ioctl() directly. (Zide)
v3: new commit.
---
 target/i386/kvm/kvm.c | 87 ++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 43 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 1fb4bd19fcf7..9993382fb40e 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -108,8 +108,8 @@ typedef struct {
 } KVMMSRHandlers;
 
 static void kvm_init_msrs(X86CPU *cpu);
-static bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
-                           QEMUWRMSRHandler *wrmsr);
+static int kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
+                          QEMUWRMSRHandler *wrmsr);
 
 const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_INFO(SET_TSS_ADDR),
@@ -3135,17 +3135,21 @@ static int kvm_vm_enable_notify_vmexit(KVMState *s)
 
 static int kvm_vm_enable_userspace_msr(KVMState *s)
 {
-    int ret = kvm_vm_enable_cap(s, KVM_CAP_X86_USER_SPACE_MSR, 0,
-                                KVM_MSR_EXIT_REASON_FILTER);
+    int ret;
+
+    ret = kvm_vm_enable_cap(s, KVM_CAP_X86_USER_SPACE_MSR, 0,
+                            KVM_MSR_EXIT_REASON_FILTER);
     if (ret < 0) {
         error_report("Could not enable user space MSRs: %s",
                      strerror(-ret));
         exit(1);
     }
 
-    if (!kvm_filter_msr(s, MSR_CORE_THREAD_COUNT,
-                        kvm_rdmsr_core_thread_count, NULL)) {
-        error_report("Could not install MSR_CORE_THREAD_COUNT handler!");
+    ret = kvm_filter_msr(s, MSR_CORE_THREAD_COUNT,
+                         kvm_rdmsr_core_thread_count, NULL);
+    if (ret < 0) {
+        error_report("Could not install MSR_CORE_THREAD_COUNT handler: %s",
+                     strerror(-ret));
         exit(1);
     }
 
@@ -3154,36 +3158,37 @@ static int kvm_vm_enable_userspace_msr(KVMState *s)
 
 static void kvm_vm_enable_energy_msrs(KVMState *s)
 {
-    bool r;
+    int ret;
+
     if (s->msr_energy.enable == true) {
-        r = kvm_filter_msr(s, MSR_RAPL_POWER_UNIT,
-                           kvm_rdmsr_rapl_power_unit, NULL);
-        if (!r) {
-            error_report("Could not install MSR_RAPL_POWER_UNIT \
-                                handler");
+        ret = kvm_filter_msr(s, MSR_RAPL_POWER_UNIT,
+                             kvm_rdmsr_rapl_power_unit, NULL);
+        if (ret < 0) {
+            error_report("Could not install MSR_RAPL_POWER_UNIT handler: %s",
+                         strerror(-ret));
             exit(1);
         }
 
-        r = kvm_filter_msr(s, MSR_PKG_POWER_LIMIT,
-                           kvm_rdmsr_pkg_power_limit, NULL);
-        if (!r) {
-            error_report("Could not install MSR_PKG_POWER_LIMIT \
-                                handler");
+        ret = kvm_filter_msr(s, MSR_PKG_POWER_LIMIT,
+                             kvm_rdmsr_pkg_power_limit, NULL);
+        if (ret < 0) {
+            error_report("Could not install MSR_PKG_POWER_LIMIT handler: %s",
+                         strerror(-ret));
             exit(1);
         }
 
-        r = kvm_filter_msr(s, MSR_PKG_POWER_INFO,
-                           kvm_rdmsr_pkg_power_info, NULL);
-        if (!r) {
-            error_report("Could not install MSR_PKG_POWER_INFO \
-                                handler");
+        ret = kvm_filter_msr(s, MSR_PKG_POWER_INFO,
+                             kvm_rdmsr_pkg_power_info, NULL);
+        if (ret < 0) {
+            error_report("Could not install MSR_PKG_POWER_INFO handler: %s",
+                         strerror(-ret));
             exit(1);
         }
-        r = kvm_filter_msr(s, MSR_PKG_ENERGY_STATUS,
-                           kvm_rdmsr_pkg_energy_status, NULL);
-        if (!r) {
-            error_report("Could not install MSR_PKG_ENERGY_STATUS \
-                                handler");
+        ret = kvm_filter_msr(s, MSR_PKG_ENERGY_STATUS,
+                             kvm_rdmsr_pkg_energy_status, NULL);
+        if (ret < 0) {
+            error_report("Could not install MSR_PKG_ENERGY_STATUS handler: %s",
+                         strerror(-ret));
             exit(1);
         }
     }
@@ -5842,13 +5847,13 @@ void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
     }
 }
 
-static bool kvm_install_msr_filters(KVMState *s)
+static int kvm_install_msr_filters(KVMState *s)
 {
     uint64_t zero = 0;
     struct kvm_msr_filter filter = {
         .flags = KVM_MSR_FILTER_DEFAULT_ALLOW,
     };
-    int r, i, j = 0;
+    int i, j = 0;
 
     for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
         KVMMSRHandlers *handler = &msr_handlers[i];
@@ -5872,18 +5877,13 @@ static bool kvm_install_msr_filters(KVMState *s)
         }
     }
 
-    r = kvm_vm_ioctl(s, KVM_X86_SET_MSR_FILTER, &filter);
-    if (r) {
-        return false;
-    }
-
-    return true;
+    return kvm_vm_ioctl(s, KVM_X86_SET_MSR_FILTER, &filter);
 }
 
-static bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
-                    QEMUWRMSRHandler *wrmsr)
+static int kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
+                          QEMUWRMSRHandler *wrmsr)
 {
-    int i;
+    int i, ret;
 
     for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
         if (!msr_handlers[i].msr) {
@@ -5893,16 +5893,17 @@ static bool kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
                 .wrmsr = wrmsr,
             };
 
-            if (!kvm_install_msr_filters(s)) {
+            ret = kvm_install_msr_filters(s);
+            if (ret) {
                 msr_handlers[i] = (KVMMSRHandlers) { };
-                return false;
+                return ret;
             }
 
-            return true;
+            return 0;
         }
     }
 
-    return false;
+    return -EINVAL;
 }
 
 static int kvm_handle_rdmsr(X86CPU *cpu, struct kvm_run *run)
-- 
2.34.1

[PATCH v5 09/11] target/i386/kvm: Return -1 when kvm_msr_energy_thread_init() fails

[Zhao Liu]

It is common practice to return a negative value (like -1) to indicate
an error, and other functions in kvm_arch_init() follow this style.

To avoid confusion (sometimes returned -1 indicates failure, and
sometimes -1, in a same function), return -1 when
kvm_msr_energy_thread_init() fails.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
v5: new commit.
---
 target/i386/kvm/kvm.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 9993382fb40e..69825b53b6da 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2921,7 +2921,6 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     struct KVMMsrEnergy *r = &s->msr_energy;
-    int ret = 0;
 
     /*
      * Sanity check
@@ -2931,13 +2930,11 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
     if (!is_host_cpu_intel()) {
         error_report("The RAPL feature can only be enabled on hosts "
                      "with Intel CPU models");
-        ret = 1;
-        goto out;
+        return -1;
     }
 
     if (!is_rapl_enabled()) {
-        ret = 1;
-        goto out;
+        return -1;
     }
 
     /* Retrieve the virtual topology */
@@ -2959,16 +2956,14 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
     r->host_topo.maxcpus = vmsr_get_maxcpus();
     if (r->host_topo.maxcpus == 0) {
         error_report("host max cpus = 0");
-        ret = 1;
-        goto out;
+        return -1;
     }
 
     /* Max number of packages on the host */
     r->host_topo.maxpkgs = vmsr_get_max_physical_package(r->host_topo.maxcpus);
     if (r->host_topo.maxpkgs == 0) {
         error_report("host max pkgs = 0");
-        ret = 1;
-        goto out;
+        return -1;
     }
 
     /* Allocate memory for each package on the host */
@@ -2980,8 +2975,7 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
     for (int i = 0; i < r->host_topo.maxpkgs; i++) {
         if (r->host_topo.pkg_cpu_count[i] == 0) {
             error_report("cpu per packages = 0 on package_%d", i);
-            ret = 1;
-            goto out;
+            return -1;
         }
     }
 
@@ -2998,8 +2992,7 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
 
     if (s->msr_energy.sioc == NULL) {
         error_report("vmsr socket opening failed");
-        ret = 1;
-        goto out;
+        return -1;
     }
 
     /* Those MSR values should not change */
@@ -3011,15 +3004,13 @@ static int kvm_msr_energy_thread_init(KVMState *s, MachineState *ms)
                                     s->msr_energy.sioc);
     if (r->msr_unit == 0 || r->msr_limit == 0 || r->msr_info == 0) {
         error_report("can't read any virtual msr");
-        ret = 1;
-        goto out;
+        return -1;
     }
 
     qemu_thread_create(&r->msr_thr, "kvm-msr",
                        kvm_msr_energy_thread,
                        s, QEMU_THREAD_JOINABLE);
-out:
-    return ret;
+    return 0;
 }
 
 int kvm_arch_get_default_type(MachineState *ms)
@@ -3327,7 +3318,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
 
         if (s->msr_energy.enable == true) {
             kvm_vm_enable_energy_msrs(s);
-            if (kvm_msr_energy_thread_init(s, ms)) {
+
+            ret = kvm_msr_energy_thread_init(s, ms);
+            if (ret < 0) {
                 error_report("kvm : error RAPL feature requirement not met");
                 exit(1);
             }
-- 
2.34.1

[PATCH v5 10/11] target/i386/kvm: Clean up error handling in kvm_arch_init()

[Zhao Liu]

Currently, there're following incorrect error handling cases in
kvm_arch_init():
* Missed to handle failure of kvm_get_supported_feature_msrs().
* Missed to return when kvm_vm_enable_disable_exits() fails.
* MSR filter related cases called exit() directly instead of returning
  to kvm_init(). (The caller of kvm_arch_init() - kvm_init() - needs to
  know if kvm_arch_init() fails in order to perform cleanup).

Fix the above cases.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
v5: cleaned up kvm_vm_enable_energy_msrs().
v3: new commit.
---
 target/i386/kvm/kvm.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 69825b53b6da..013c0359acbe 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3147,7 +3147,7 @@ static int kvm_vm_enable_userspace_msr(KVMState *s)
     return 0;
 }
 
-static void kvm_vm_enable_energy_msrs(KVMState *s)
+static int kvm_vm_enable_energy_msrs(KVMState *s)
 {
     int ret;
 
@@ -3157,7 +3157,7 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
         if (ret < 0) {
             error_report("Could not install MSR_RAPL_POWER_UNIT handler: %s",
                          strerror(-ret));
-            exit(1);
+            return ret;
         }
 
         ret = kvm_filter_msr(s, MSR_PKG_POWER_LIMIT,
@@ -3165,7 +3165,7 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
         if (ret < 0) {
             error_report("Could not install MSR_PKG_POWER_LIMIT handler: %s",
                          strerror(-ret));
-            exit(1);
+            return ret;
         }
 
         ret = kvm_filter_msr(s, MSR_PKG_POWER_INFO,
@@ -3173,17 +3173,17 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
         if (ret < 0) {
             error_report("Could not install MSR_PKG_POWER_INFO handler: %s",
                          strerror(-ret));
-            exit(1);
+            return ret;
         }
         ret = kvm_filter_msr(s, MSR_PKG_ENERGY_STATUS,
                              kvm_rdmsr_pkg_energy_status, NULL);
         if (ret < 0) {
             error_report("Could not install MSR_PKG_ENERGY_STATUS handler: %s",
                          strerror(-ret));
-            exit(1);
+            return ret;
         }
     }
-    return;
+    return 0;
 }
 
 int kvm_arch_init(MachineState *ms, KVMState *s)
@@ -3250,7 +3250,10 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         return ret;
     }
 
-    kvm_get_supported_feature_msrs(s);
+    ret = kvm_get_supported_feature_msrs(s);
+    if (ret < 0) {
+        return ret;
+    }
 
     uname(&utsname);
     lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0;
@@ -3286,6 +3289,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         if (ret < 0) {
             error_report("kvm: guest stopping CPU not supported: %s",
                          strerror(-ret));
+            return ret;
         }
     }
 
@@ -3317,12 +3321,15 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
 
         if (s->msr_energy.enable == true) {
-            kvm_vm_enable_energy_msrs(s);
+            ret = kvm_vm_enable_energy_msrs(s);
+            if (ret < 0) {
+                return ret;
+            }
 
             ret = kvm_msr_energy_thread_init(s, ms);
             if (ret < 0) {
                 error_report("kvm : error RAPL feature requirement not met");
-                exit(1);
+                return ret;
             }
         }
     }
-- 
2.34.1

[PATCH v5 11/11] target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with KVM_MSR_FILTER_MAX_RANGES

[Zhao Liu]

kvm_install_msr_filters() uses KVM_MSR_FILTER_MAX_RANGES as the bound
when traversing msr_handlers[], while other places still compute the
size by ARRAY_SIZE(msr_handlers).

In fact, msr_handlers[] is an array with the fixed size
KVM_MSR_FILTER_MAX_RANGES, so there is no difference between the two
ways.

For the code consistency and to avoid additional computational overhead,
use KVM_MSR_FILTER_MAX_RANGES instead of ARRAY_SIZE(msr_handlers).

Suggested-by: Zide Chen <zide.chen@intel.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zide Chen <zide.chen@intel.com>
---
v4: new commit.
---
 target/i386/kvm/kvm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 013c0359acbe..501873475255 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -5885,7 +5885,7 @@ static int kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
 {
     int i, ret;
 
-    for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
+    for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
         if (!msr_handlers[i].msr) {
             msr_handlers[i] = (KVMMSRHandlers) {
                 .msr = msr,
@@ -5911,7 +5911,7 @@ static int kvm_handle_rdmsr(X86CPU *cpu, struct kvm_run *run)
     int i;
     bool r;
 
-    for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
+    for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
         KVMMSRHandlers *handler = &msr_handlers[i];
         if (run->msr.index == handler->msr) {
             if (handler->rdmsr) {
@@ -5931,7 +5931,7 @@ static int kvm_handle_wrmsr(X86CPU *cpu, struct kvm_run *run)
     int i;
     bool r;
 
-    for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
+    for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
         KVMMSRHandlers *handler = &msr_handlers[i];
         if (run->msr.index == handler->msr) {
             if (handler->wrmsr) {
-- 
2.34.1

Re: [PATCH v5 01/11 for v9.2?] i386/cpu: Mark avx10_version filtered when prefix is NULL

[Tao Su]

On Wed, Nov 06, 2024 at 11:07:18AM +0800, Zhao Liu wrote:
> In x86_cpu_filter_features(), if host doesn't support AVX10, the
> configured avx10_version should be marked as filtered regardless of
> whether prefix is NULL or not.
> 
> Check prefix before warn_report() instead of checking for
> have_filtered_features.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: commit bccfb846fd52 ("target/i386: add AVX10 feature and AVX10 version property")
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

Reviewed-by: Tao Su <tao1.su@linux.intel.com>

> ---
> v5: new commit.
> ---
>  target/i386/cpu.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 3baa95481fbc..77c1233daa13 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7711,8 +7711,10 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>              env->avx10_version = version;
>              have_filtered_features = true;
>          }
> -    } else if (env->avx10_version && prefix) {
> -        warn_report("%s: avx10.%d.", prefix, env->avx10_version);
> +    } else if (env->avx10_version) {
> +        if (prefix) {
> +            warn_report("%s: avx10.%d.", prefix, env->avx10_version);
> +        }
>          have_filtered_features = true;
>      }
>  
> -- 
> 2.34.1
> 

Re: [PATCH v5 00/11] i386: miscellaneous cleanup

[Zhao Liu]

Hi Paolo,

A kindly ping.

Thanks,
Zhao

On Wed, Nov 06, 2024 at 11:07:17AM +0800, Zhao Liu wrote:
> Date: Wed, 6 Nov 2024 11:07:17 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: [PATCH v5 00/11] i386: miscellaneous cleanup
> X-Mailer: git-send-email 2.34.1
> 
> Hi Paolo and all,
> 
> Is it necessary to include the first patch (AVX10 cleanup/fix) in v9.2?
> 
> Others are for v10.0.
> 
> Compared with v4 [1],
>  * patch 1 (AVX10 fix) and patch 9 (RAPL cleanup) are newly added.
>  * rebased on commit 9a7b0a8618b1 ("Merge tag 'pull-aspeed-20241104' of
>    https://github.com/legoater/qemu into staging").
> 
> 
> Background and Introduction
> ===========================
> 
> This series picks cleanup from my previous kvmclock [2] (as other
> renaming attempts were temporarily put on hold).
> 
> In addition, this series also include the cleanup on a historically
> workaround, recent comment of coco interface [3] and error handling
> corner cases in kvm_arch_init().
> 
> Avoiding the fragmentation of these misc cleanups, I consolidated them
> all in one series and was able to tackle them in one go!
> 
> [1]: https://lore.kernel.org/qemu-devel/20240716161015.263031-1-zhao1.liu@intel.com/
> [2]: https://lore.kernel.org/qemu-devel/20240329101954.3954987-1-zhao1.liu@linux.intel.com/
> [3]: https://lore.kernel.org/qemu-devel/2815f0f1-9e20-4985-849c-d74c6cdc94ae@intel.com/
> 
> Thanks and Best Regards,
> Zhao
> ---
> Zhao Liu (11):
>   i386/cpu: Mark avx10_version filtered when prefix is NULL
>   target/i386/kvm: Add feature bit definitions for KVM CPUID
>   target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and
>     MSR_KVM_SYSTEM_TIME definitions
>   target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled
>   target/i386/kvm: Save/load MSRs of kvmclock2
>     (KVM_FEATURE_CLOCKSOURCE2)
>   target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo
>   target/i386/confidential-guest: Fix comment of
>     x86_confidential_guest_kvm_type()
>   target/i386/kvm: Clean up return values of MSR filter related
>     functions
>   target/i386/kvm: Return -1 when kvm_msr_energy_thread_init() fails
>   target/i386/kvm: Clean up error handling in kvm_arch_init()
>   target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with
>     KVM_MSR_FILTER_MAX_RANGES
> 
>  hw/i386/kvm/clock.c              |   5 +-
>  target/i386/confidential-guest.h |   2 +-
>  target/i386/cpu.c                |   6 +-
>  target/i386/cpu.h                |  25 ++++
>  target/i386/kvm/kvm.c            | 211 +++++++++++++++++--------------
>  5 files changed, 145 insertions(+), 104 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH v5 01/11 for v9.2?] i386/cpu: Mark avx10_version filtered when prefix is NULL

[Michael Tokarev]

06.11.2024 06:07, Zhao Liu wrote:
> In x86_cpu_filter_features(), if host doesn't support AVX10, the
> configured avx10_version should be marked as filtered regardless of
> whether prefix is NULL or not.
> 
> Check prefix before warn_report() instead of checking for
> have_filtered_features.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: commit bccfb846fd52 ("target/i386: add AVX10 feature and AVX10 version property")
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

Hi!

Has this patch been forgotten?  9.2 is out already and I'm collecting fixes for it...

Thanks,

/mjt

> ---
> v5: new commit.
> ---
>   target/i386/cpu.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 3baa95481fbc..77c1233daa13 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7711,8 +7711,10 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>               env->avx10_version = version;
>               have_filtered_features = true;
>           }
> -    } else if (env->avx10_version && prefix) {
> -        warn_report("%s: avx10.%d.", prefix, env->avx10_version);
> +    } else if (env->avx10_version) {
> +        if (prefix) {
> +            warn_report("%s: avx10.%d.", prefix, env->avx10_version);
> +        }
>           have_filtered_features = true;
>       }
>   


-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH v5 04/11] target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled

[Paolo Bonzini]

On 11/6/24 04:07, Zhao Liu wrote:
> MSR_KVM_SYSTEM_TIME and MSR_KVM_WALL_CLOCK are attached with the (old)
> kvmclock feature (KVM_FEATURE_CLOCKSOURCE).
> 
> So, just save/load them only when kvmclock (KVM_FEATURE_CLOCKSOURCE) is
> enabled.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 

The MSRs contains the same values as the "new" pair; QEMU only has to 
save/restore one of them but the code should be active for both feature 
bits and thus use

+        if (env->env.features[FEAT_KVM] & (CPUID_KVM_CLOCK |
+                                           CPUID_KVM_CLOCK2)) {

Paolo

Re: [PATCH v5 05/11] target/i386/kvm: Save/load MSRs of kvmclock2 (KVM_FEATURE_CLOCKSOURCE2)

[Paolo Bonzini]

On 11/6/24 04:07, Zhao Liu wrote:
> MSR_KVM_SYSTEM_TIME_NEW and MSR_KVM_WALL_CLOCK_NEW are bound to
> kvmclock2 (KVM_FEATURE_CLOCKSOURCE2).
> 
> Add the save/load support for these 2 MSRs just like kvmclock MSRs.

As mentioned in the previous patch, this is not necessary.  If it was 
needed, you'd have to also add VMSTATE fields in machine.c

Paolo

Re: [PATCH v5 10/11] target/i386/kvm: Clean up error handling in kvm_arch_init()

[Paolo Bonzini]

On 11/6/24 04:07, Zhao Liu wrote:
> Currently, there're following incorrect error handling cases in
> kvm_arch_init():
> * Missed to handle failure of kvm_get_supported_feature_msrs().
> * Missed to return when kvm_vm_enable_disable_exits() fails.

At least in these two cases I think it was intentional to avoid hard 
failures.  It's probably not a very likely case and I think your patch 
is overall a good idea.

Paolo

> * MSR filter related cases called exit() directly instead of returning
>    to kvm_init(). (The caller of kvm_arch_init() - kvm_init() - needs to
>    know if kvm_arch_init() fails in order to perform cleanup).
> 
> Fix the above cases.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Zide Chen <zide.chen@intel.com>
> ---
> v5: cleaned up kvm_vm_enable_energy_msrs().
> v3: new commit.
> ---
>   target/i386/kvm/kvm.c | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 69825b53b6da..013c0359acbe 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -3147,7 +3147,7 @@ static int kvm_vm_enable_userspace_msr(KVMState *s)
>       return 0;
>   }
>   
> -static void kvm_vm_enable_energy_msrs(KVMState *s)
> +static int kvm_vm_enable_energy_msrs(KVMState *s)
>   {
>       int ret;
>   
> @@ -3157,7 +3157,7 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
>           if (ret < 0) {
>               error_report("Could not install MSR_RAPL_POWER_UNIT handler: %s",
>                            strerror(-ret));
> -            exit(1);
> +            return ret;
>           }
>   
>           ret = kvm_filter_msr(s, MSR_PKG_POWER_LIMIT,
> @@ -3165,7 +3165,7 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
>           if (ret < 0) {
>               error_report("Could not install MSR_PKG_POWER_LIMIT handler: %s",
>                            strerror(-ret));
> -            exit(1);
> +            return ret;
>           }
>   
>           ret = kvm_filter_msr(s, MSR_PKG_POWER_INFO,
> @@ -3173,17 +3173,17 @@ static void kvm_vm_enable_energy_msrs(KVMState *s)
>           if (ret < 0) {
>               error_report("Could not install MSR_PKG_POWER_INFO handler: %s",
>                            strerror(-ret));
> -            exit(1);
> +            return ret;
>           }
>           ret = kvm_filter_msr(s, MSR_PKG_ENERGY_STATUS,
>                                kvm_rdmsr_pkg_energy_status, NULL);
>           if (ret < 0) {
>               error_report("Could not install MSR_PKG_ENERGY_STATUS handler: %s",
>                            strerror(-ret));
> -            exit(1);
> +            return ret;
>           }
>       }
> -    return;
> +    return 0;
>   }
>   
>   int kvm_arch_init(MachineState *ms, KVMState *s)
> @@ -3250,7 +3250,10 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>           return ret;
>       }
>   
> -    kvm_get_supported_feature_msrs(s);
> +    ret = kvm_get_supported_feature_msrs(s);
> +    if (ret < 0) {
> +        return ret;
> +    }
>   
>       uname(&utsname);
>       lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0;
> @@ -3286,6 +3289,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>           if (ret < 0) {
>               error_report("kvm: guest stopping CPU not supported: %s",
>                            strerror(-ret));
> +            return ret;
>           }
>       }
>   
> @@ -3317,12 +3321,15 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>           }
>   
>           if (s->msr_energy.enable == true) {
> -            kvm_vm_enable_energy_msrs(s);
> +            ret = kvm_vm_enable_energy_msrs(s);
> +            if (ret < 0) {
> +                return ret;
> +            }
>   
>               ret = kvm_msr_energy_thread_init(s, ms);
>               if (ret < 0) {
>                   error_report("kvm : error RAPL feature requirement not met");
> -                exit(1);
> +                return ret;
>               }
>           }
>       }

Re: [PATCH v5 11/11] target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with KVM_MSR_FILTER_MAX_RANGES

[Paolo Bonzini]

On 11/6/24 04:07, Zhao Liu wrote:
> kvm_install_msr_filters() uses KVM_MSR_FILTER_MAX_RANGES as the bound
> when traversing msr_handlers[], while other places still compute the
> size by ARRAY_SIZE(msr_handlers).
> 
> In fact, msr_handlers[] is an array with the fixed size
> KVM_MSR_FILTER_MAX_RANGES, so there is no difference between the two
> ways.
> 
> For the code consistency and to avoid additional computational overhead,
> use KVM_MSR_FILTER_MAX_RANGES instead of ARRAY_SIZE(msr_handlers).

I agree with the consistency but I'd go the other direction.

Paolo

> Suggested-by: Zide Chen <zide.chen@intel.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Zide Chen <zide.chen@intel.com>
> ---
> v4: new commit.
> ---
>   target/i386/kvm/kvm.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 013c0359acbe..501873475255 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -5885,7 +5885,7 @@ static int kvm_filter_msr(KVMState *s, uint32_t msr, QEMURDMSRHandler *rdmsr,
>   {
>       int i, ret;
>   
> -    for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
> +    for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
>           if (!msr_handlers[i].msr) {
>               msr_handlers[i] = (KVMMSRHandlers) {
>                   .msr = msr,
> @@ -5911,7 +5911,7 @@ static int kvm_handle_rdmsr(X86CPU *cpu, struct kvm_run *run)
>       int i;
>       bool r;
>   
> -    for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
> +    for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
>           KVMMSRHandlers *handler = &msr_handlers[i];
>           if (run->msr.index == handler->msr) {
>               if (handler->rdmsr) {
> @@ -5931,7 +5931,7 @@ static int kvm_handle_wrmsr(X86CPU *cpu, struct kvm_run *run)
>       int i;
>       bool r;
>   
> -    for (i = 0; i < ARRAY_SIZE(msr_handlers); i++) {
> +    for (i = 0; i < KVM_MSR_FILTER_MAX_RANGES; i++) {
>           KVMMSRHandlers *handler = &msr_handlers[i];
>           if (run->msr.index == handler->msr) {
>               if (handler->wrmsr) {

Re: [PATCH v5 00/11] i386: miscellaneous cleanup

[Paolo Bonzini]

On 11/6/24 04:07, Zhao Liu wrote:
> Hi Paolo and all,
> 
> Is it necessary to include the first patch (AVX10 cleanup/fix) in v9.2?
> 
> Others are for v10.0.
> 
> Compared with v4 [1],
>   * patch 1 (AVX10 fix) and patch 9 (RAPL cleanup) are newly added.
>   * rebased on commit 9a7b0a8618b1 ("Merge tag 'pull-aspeed-20241104' of
>     https://github.com/legoater/qemu into staging").

Removed patches 5 and 11, fixed patch 4 to include CPUID_KVM_CLOCK2, and 
queued - thanks!

Paolo

> 
> Background and Introduction
> ===========================
> 
> This series picks cleanup from my previous kvmclock [2] (as other
> renaming attempts were temporarily put on hold).
> 
> In addition, this series also include the cleanup on a historically
> workaround, recent comment of coco interface [3] and error handling
> corner cases in kvm_arch_init().
> 
> Avoiding the fragmentation of these misc cleanups, I consolidated them
> all in one series and was able to tackle them in one go!
> 
> [1]: https://lore.kernel.org/qemu-devel/20240716161015.263031-1-zhao1.liu@intel.com/
> [2]: https://lore.kernel.org/qemu-devel/20240329101954.3954987-1-zhao1.liu@linux.intel.com/
> [3]: https://lore.kernel.org/qemu-devel/2815f0f1-9e20-4985-849c-d74c6cdc94ae@intel.com/
> 
> Thanks and Best Regards,
> Zhao
> ---
> Zhao Liu (11):
>    i386/cpu: Mark avx10_version filtered when prefix is NULL
>    target/i386/kvm: Add feature bit definitions for KVM CPUID
>    target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and
>      MSR_KVM_SYSTEM_TIME definitions
>    target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled
>    target/i386/kvm: Save/load MSRs of kvmclock2
>      (KVM_FEATURE_CLOCKSOURCE2)
>    target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo
>    target/i386/confidential-guest: Fix comment of
>      x86_confidential_guest_kvm_type()
>    target/i386/kvm: Clean up return values of MSR filter related
>      functions
>    target/i386/kvm: Return -1 when kvm_msr_energy_thread_init() fails
>    target/i386/kvm: Clean up error handling in kvm_arch_init()
>    target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with
>      KVM_MSR_FILTER_MAX_RANGES
> 
>   hw/i386/kvm/clock.c              |   5 +-
>   target/i386/confidential-guest.h |   2 +-
>   target/i386/cpu.c                |   6 +-
>   target/i386/cpu.h                |  25 ++++
>   target/i386/kvm/kvm.c            | 211 +++++++++++++++++--------------
>   5 files changed, 145 insertions(+), 104 deletions(-)
> 

Re: [PATCH v5 10/11] target/i386/kvm: Clean up error handling in kvm_arch_init()

[Zhao Liu]

On Tue, Dec 24, 2024 at 04:53:36PM +0100, Paolo Bonzini wrote:
> Date: Tue, 24 Dec 2024 16:53:36 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH v5 10/11] target/i386/kvm: Clean up error handling in
>  kvm_arch_init()
> 
> On 11/6/24 04:07, Zhao Liu wrote:
> > Currently, there're following incorrect error handling cases in
> > kvm_arch_init():
> > * Missed to handle failure of kvm_get_supported_feature_msrs().
> > * Missed to return when kvm_vm_enable_disable_exits() fails.
> 
> At least in these two cases I think it was intentional to avoid hard
> failures.  It's probably not a very likely case and I think your patch is
> overall a good idea.

I have the idea to clean up the abort()/exit() in KVM and instead use
@errp to handle failure cases. However, this would be a big change, so
this patch only makes a small change, as a first step.

Thanks,
Zhao

Re: [PATCH v5 11/11] target/i386/kvm: Replace ARRAY_SIZE(msr_handlers) with KVM_MSR_FILTER_MAX_RANGES

[Zhao Liu]

On Tue, Dec 24, 2024 at 04:54:41PM +0100, Paolo Bonzini wrote:
> Date: Tue, 24 Dec 2024 16:54:41 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH v5 11/11] target/i386/kvm: Replace
>  ARRAY_SIZE(msr_handlers) with KVM_MSR_FILTER_MAX_RANGES
> 
> On 11/6/24 04:07, Zhao Liu wrote:
> > kvm_install_msr_filters() uses KVM_MSR_FILTER_MAX_RANGES as the bound
> > when traversing msr_handlers[], while other places still compute the
> > size by ARRAY_SIZE(msr_handlers).
> > 
> > In fact, msr_handlers[] is an array with the fixed size
> > KVM_MSR_FILTER_MAX_RANGES, so there is no difference between the two
> > ways.
> > 
> > For the code consistency and to avoid additional computational overhead,
> > use KVM_MSR_FILTER_MAX_RANGES instead of ARRAY_SIZE(msr_handlers).
> 
> I agree with the consistency but I'd go the other direction.
>

OK, I'll switch to the other way.

Thanks,
Zhao

Re: [PATCH v5 04/11] target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled

[Zhao Liu]

On Tue, Dec 24, 2024 at 04:31:28PM +0100, Paolo Bonzini wrote:
> Date: Tue, 24 Dec 2024 16:31:28 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH v5 04/11] target/i386/kvm: Only save/load kvmclock MSRs
>  when kvmclock enabled
> 
> On 11/6/24 04:07, Zhao Liu wrote:
> > MSR_KVM_SYSTEM_TIME and MSR_KVM_WALL_CLOCK are attached with the (old)
> > kvmclock feature (KVM_FEATURE_CLOCKSOURCE).
> > 
> > So, just save/load them only when kvmclock (KVM_FEATURE_CLOCKSOURCE) is
> > enabled.
> > 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > 
> 
> The MSRs contains the same values as the "new" pair; QEMU only has to
> save/restore one of them but the code should be active for both feature bits
> and thus use
> 
> +        if (env->env.features[FEAT_KVM] & (CPUID_KVM_CLOCK |
> +                                           CPUID_KVM_CLOCK2)) {
> 

This is the correct way, thanks.

Regards,
Zhao

Re: [PATCH v5 05/11] target/i386/kvm: Save/load MSRs of kvmclock2 (KVM_FEATURE_CLOCKSOURCE2)

[Zhao Liu]

On Tue, Dec 24, 2024 at 04:32:42PM +0100, Paolo Bonzini wrote:
> Date: Tue, 24 Dec 2024 16:32:42 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH v5 05/11] target/i386/kvm: Save/load MSRs of kvmclock2
>  (KVM_FEATURE_CLOCKSOURCE2)
> 
> On 11/6/24 04:07, Zhao Liu wrote:
> > MSR_KVM_SYSTEM_TIME_NEW and MSR_KVM_WALL_CLOCK_NEW are bound to
> > kvmclock2 (KVM_FEATURE_CLOCKSOURCE2).
> > 
> > Add the save/load support for these 2 MSRs just like kvmclock MSRs.
> 
> As mentioned in the previous patch, this is not necessary.  If it was
> needed, you'd have to also add VMSTATE fields in machine.c
> 

I see, thanks!

Regards,
Zhao

Re: [PATCH v5 01/11 for v9.2?] i386/cpu: Mark avx10_version filtered when prefix is NULL

[Michael Tokarev]

On 21.12.2024 00:04, Michael Tokarev wrote:
> 06.11.2024 06:07, Zhao Liu wrote:
>> In x86_cpu_filter_features(), if host doesn't support AVX10, the
>> configured avx10_version should be marked as filtered regardless of
>> whether prefix is NULL or not.
>>
>> Check prefix before warn_report() instead of checking for
>> have_filtered_features.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: commit bccfb846fd52 ("target/i386: add AVX10 feature and AVX10 
>> version property")
>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> 
> Hi!
> 
> Has this patch been forgotten?  9.2 is out already and I'm collecting 
> fixes for it...

Ping #2?  It's a 10.0.1 time already.. :)

Thanks,

/mjt

>> ---
>> v5: new commit.
>> ---
>>   target/i386/cpu.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 3baa95481fbc..77c1233daa13 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -7711,8 +7711,10 @@ static bool x86_cpu_filter_features(X86CPU 
>> *cpu, bool verbose)
>>               env->avx10_version = version;
>>               have_filtered_features = true;
>>           }
>> -    } else if (env->avx10_version && prefix) {
>> -        warn_report("%s: avx10.%d.", prefix, env->avx10_version);
>> +    } else if (env->avx10_version) {
>> +        if (prefix) {
>> +            warn_report("%s: avx10.%d.", prefix, env->avx10_version);
>> +        }
>>           have_filtered_features = true;
>>       }
> 
> 

Re: [PATCH v5 01/11 for v9.2?] i386/cpu: Mark avx10_version filtered when prefix is NULL

[Zhao Liu]

On Mon, May 12, 2025 at 12:35:35PM +0300, Michael Tokarev wrote:
> Date: Mon, 12 May 2025 12:35:35 +0300
> From: Michael Tokarev <mjt@tls.msk.ru>
> Subject: Re: [PATCH v5 01/11 for v9.2?] i386/cpu: Mark avx10_version
>  filtered when prefix is NULL
> 
> On 21.12.2024 00:04, Michael Tokarev wrote:
> > 06.11.2024 06:07, Zhao Liu wrote:
> > > In x86_cpu_filter_features(), if host doesn't support AVX10, the
> > > configured avx10_version should be marked as filtered regardless of
> > > whether prefix is NULL or not.
> > > 
> > > Check prefix before warn_report() instead of checking for
> > > have_filtered_features.
> > > 
> > > Cc: qemu-stable@nongnu.org
> > > Fixes: commit bccfb846fd52 ("target/i386: add AVX10 feature and
> > > AVX10 version property")
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > 
> > Hi!
> > 
> > Has this patch been forgotten?  9.2 is out already and I'm collecting
> > fixes for it...
> 
> Ping #2?  It's a 10.0.1 time already.. :)
>

Hi Michael,

I'm sorry, but now I think it is not necessary, since it doesn't
affect the normal use of avx10. And I don't think anyone is actively
setting the wrong avx10 version in a normal production environment (
even KVM only supports v1).

Hope this helps and saves you the effort!

Thanks,
Zhao