Re: [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock feature name

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On 3/29/2024 6:19 PM, Zhao Liu wrote:
From: Zhao Liu <zhao1.liu@xxxxxxxxx>

Hi list,

This series is based on Paolo's guest_phys_bits patchset [1].

Currently, the old and new kvmclocks have the same feature name
"kvmclock" in FeatureWordInfo[FEAT_KVM].

When I tried to dig into the history of this unusual naming and fix it,
I realized that Tim was already trying to rename it, so I picked up his
renaming patch [2] (with a new commit message and other minor changes).

13 years age, the same name was introduced in [3], and its main purpose
is to make it easy for users to enable/disable 2 kvmclocks. Then, in
2012, Don tried to rename the new kvmclock, but the follow-up did not
address Igor and Eduardo's comments about compatibility.

Tim [2], not long ago, and I just now, were both puzzled by the naming
one after the other.

The commit message of [3] said the reason clearly:

  When we tweak flags involving this value - specially when we use "-",
  we have to act on both.

So you are trying to change it to "when people want to disable kvmclock, they need to use '-kvmclock,-kvmclock2' instead of '-kvmclock'"

IMHO, I prefer existing code and I don't see much value of differentiating them. If the current code puzzles you, then we can add comment to explain.

So, this series is to push for renaming the new kvmclock feature to
"kvmclock2" and adding compatibility support for older machines (PC 9.0
and older).

Finally, let's put an end to decades of doubt about this name.

Next Step

This series just separates the two kvmclocks from the naming, and in
subsequent patches I plan to stop setting kvmclock(old kcmclock) by
default as long as KVM supports kvmclock2 (new kvmclock).

No. It will break existing guests that use KVM_FEATURE_CLOCKSOURCE.

Also, try to deprecate the old kvmclock in KVM side.


Thanks and Best Regards,

Tim Wiederhake (1):
   target/i386: Fix duplicated kvmclock name in FEAT_KVM

Zhao Liu (6):
   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 new kvmclock
   target/i386/kvm: Add legacy_kvmclock cpu property
   target/i386/kvm: Update comment in kvm_cpu_realizefn()

  hw/i386/kvm/clock.c       |  5 ++--
  hw/i386/pc.c              |  1 +
  target/i386/cpu.c         |  3 +-
  target/i386/cpu.h         | 32 +++++++++++++++++++++
  target/i386/kvm/kvm-cpu.c | 25 ++++++++++++++++-
  target/i386/kvm/kvm.c     | 59 +++++++++++++++++++++++++--------------
  6 files changed, 99 insertions(+), 26 deletions(-)

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux