All of lore.kernel.org
 help / color / mirror / Atom feed
From: Zhao Liu <zhao1.liu@linux.intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Eduardo Habkost <eduardo@habkost.net>,
	"Michael S . Tsirkin" <mst@redhat.com>,
	Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	Igor Mammedov <imammedo@redhat.com>,
	Tim Wiederhake <twiederh@redhat.com>
Cc: qemu-devel@nongnu.org, kvm@vger.kernel.org,
	Zhao Liu <zhao1.liu@intel.com>
Subject: [PATCH for-9.1 6/7] target/i386: Fix duplicated kvmclock name in FEAT_KVM
Date: Fri, 29 Mar 2024 18:19:53 +0800	[thread overview]
Message-ID: <20240329101954.3954987-7-zhao1.liu@linux.intel.com> (raw)
In-Reply-To: <20240329101954.3954987-1-zhao1.liu@linux.intel.com>

From: Tim Wiederhake <twiederh@redhat.com>

The commit 642258c6c7 ("kvm: add kvmclock to its second bit") gave the
old and new kvmclocks with the same name "kvmclock", to facilitate user
to set/unset the feature bits for both 2 kvmclock features together.

This could work because:
* QEMU side:
  - x86_cpu_register_bit_prop() supports "the same property name can be
    registered multiple times to make it affect multiple bits in the
    same FeatureWord".
* KVM side:
  - The only difference between 2 version kvmclocks is their MSRs have
    different addresses.
  - When 2 kvmclocks are both enabled, KVM will prioritize the use of
    new kvmclock's MSRs.

However, there're reasons we need give the second kvmclock a new name:
* Based on the KVM mechanism, it doesn't make sense to bind two
  kvmclocks together. Since kvmclock is enabled by default in most cases
  as a KVM PV feature, the benefit of the same naming is reflected in
  the fact that -kvmclock can disable both. But, following the KVM
  interface style (i.e., separating the two kvmclocks) is clearly
  clearer to the user.
* For developers, identical names have been creating confusion along
  with persistent doubts about the naming.
* FeatureWordInfo should define names based on hardware/Host CPUID bit,
  and the name is used to distinguish the bit.
* User actions based on +/- feature names should only work on
  independent feature bits. The common effect of multiple features
  should be controlled by an additional CPU property or additional code
  logic to show the association between different feature bits.
* The old kvmclock will eventually be removed. Different naming can ease
  the burden of future cleanups.

Therefore, rename the new kvmclock feature as "kvmclock2".

Additionally, add "kvmclock2" entry in kvm_default_props[] since the
oldest kernel supported by QEMU (v4.5) has supported the new kvm clock.

Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
Based on Tim's original patch, rewrote the commit message and added the
tiny fix for compatibility.
---
 target/i386/cpu.c         | 2 +-
 target/i386/kvm/kvm-cpu.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1b6caf071a6d..0a1dac60f5de 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -855,7 +855,7 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
     [FEAT_KVM] = {
         .type = CPUID_FEATURE_WORD,
         .feat_names = {
-            "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
+            "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock2",
             "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
             NULL, "kvm-pv-tlb-flush", "kvm-asyncpf-vmexit", "kvm-pv-ipi",
             "kvm-poll-control", "kvm-pv-sched-yield", "kvm-asyncpf-int", "kvm-msi-ext-dest-id",
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index ae3cb27c8aa8..753f90c18bd6 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -77,7 +77,7 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
 
     if (cpu->legacy_kvmclock) {
         /*
-         * The old and new kvmclock are both set by default from the
+         * The kvmclock and kvmclock2 are both set by default from the
          * oldest KVM supported (v4.5, see "OS requirements" section at
          * docs/system/target-i386.rst). So when one of them is missing,
          * it is only possible that the user is actively masking it.
@@ -179,6 +179,7 @@ static void kvm_cpu_xsave_init(void)
  */
 static PropValue kvm_default_props[] = {
     { "kvmclock", "on" },
+    { "kvmclock2", "on" },
     { "kvm-nopiodelay", "on" },
     { "kvm-asyncpf", "on" },
     { "kvm-steal-time", "on" },
-- 
2.34.1


  parent reply	other threads:[~2024-03-29 10:06 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-29 10:19 [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock feature name Zhao Liu
2024-03-29 10:19 ` [PATCH for-9.1 1/7] target/i386/kvm: Add feature bit definitions for KVM CPUID Zhao Liu
2024-04-24 11:25   ` Xiaoyao Li
2024-03-29 10:19 ` [PATCH for-9.1 2/7] target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and MSR_KVM_SYSTEM_TIME definitions Zhao Liu
2024-04-24 15:11   ` Xiaoyao Li
2024-03-29 10:19 ` [PATCH for-9.1 3/7] target/i386/kvm: Only Save/load kvmclock MSRs when kvmclock enabled Zhao Liu
2024-03-29 10:19 ` [PATCH for-9.1 4/7] target/i386/kvm: Save/load MSRs of new kvmclock (KVM_FEATURE_CLOCKSOURCE2) Zhao Liu
2024-03-29 10:19 ` [PATCH for-9.1 5/7] target/i386/kvm: Add legacy_kvmclock cpu property Zhao Liu
2024-03-29 10:19 ` Zhao Liu [this message]
2024-03-29 10:19 ` [PATCH for-9.1 7/7] target/i386/kvm: Update comment in kvm_cpu_realizefn() Zhao Liu
2024-04-24 10:33 ` [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock feature name Zhao Liu
2024-04-24 15:57 ` Xiaoyao Li
2024-04-25  7:17   ` Zhao Liu
2024-04-25  8:40     ` Xiaoyao Li
2024-04-25 10:29       ` Zhao Liu
2024-04-25 12:04         ` Xiaoyao Li
2024-04-25 13:36           ` Zhao Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240329101954.3954987-7-zhao1.liu@linux.intel.com \
    --to=zhao1.liu@linux.intel.com \
    --cc=eduardo@habkost.net \
    --cc=imammedo@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=marcel.apfelbaum@gmail.com \
    --cc=mst@redhat.com \
    --cc=mtosatti@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=twiederh@redhat.com \
    --cc=zhao1.liu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.