From: Xiaoyao Li <xiaoyao.li@intel.com>
To: Zhao Liu <zhao1.liu@linux.intel.com>
Cc: 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>,
qemu-devel@nongnu.org, kvm@vger.kernel.org,
Zhao Liu <zhao1.liu@intel.com>
Subject: Re: [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock feature name
Date: Thu, 25 Apr 2024 20:04:46 +0800 [thread overview]
Message-ID: <6ee5cb90-d299-4977-8a2f-529f78dc3913@intel.com> (raw)
In-Reply-To: <ZiowbFCZ75LOgBMC@intel.com>
On 4/25/2024 6:29 PM, Zhao Liu wrote:
> On Thu, Apr 25, 2024 at 04:40:10PM +0800, Xiaoyao Li wrote:
>> Date: Thu, 25 Apr 2024 16:40:10 +0800
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>> Subject: Re: [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock
>> feature name
>>
>> On 4/25/2024 3:17 PM, Zhao Liu wrote:
>>> Hi Xiaoyao,
>>>
>>> On Wed, Apr 24, 2024 at 11:57:11PM +0800, Xiaoyao Li wrote:
>>>> Date: Wed, 24 Apr 2024 23:57:11 +0800
>>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>>> Subject: Re: [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock
>>>> feature name
>>>>
>>>> On 3/29/2024 6:19 PM, Zhao Liu wrote:
>>>>> From: Zhao Liu <zhao1.liu@intel.com>
>>>>>
>>>>> 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.
>>>
>>> It's enough to just enable kvmclock2 for Guest; kvmclock (old) is
>>> redundant in the presence of kvmclock2.
>>>
>>> So operating both feature bits at the same time is not a reasonable
>>> choice, we should only keep kvmclock2 for Guest. It's possible because
>>> the oldest linux (v4.5) which QEMU i386 supports has kvmclock2.
>>
>> who said the oldest Linux QEMU supports is from 4.5? what about 2.x kernel?
>
> For Host (docs/system/target-i386.rst).
>
>> Besides, not only the Linux guest, whatever guest OS is, it will be broken
>> if the guest is using kvmclock and QEMU starts to drop support of kvmclock.
>
> I'm not aware of any minimum version requirements for Guest supported
> by KVM, but there are no commitment.
the common commitment is at least keeping backwards compatibility.
>> So, again, hard NAK to drop the support of kvmclock. It breaks existing
>> guests that use kvmclock. You cannot force people to upgrade their existing
>> VMs to use kvmclock2 instead of kvmclock.
>
> I agree, legacy kvmclock can be left out, if the old kernel does not
> support kvmclock2 and strongly requires kvmclock, it can be enabled
> using 9.1 and earlier machines or legacy-kvmclock, as long as Host still
> supports it.
>
> What's the gap in handling it this way? especially considering that
> kvmclock2 was introduced in v2.6.35, and earlier kernels are no longer
> maintained. The availability of the PV feature requires compatibility
> for both Host and Guest.
>
> Anyway, the above discussion here is about future plans, and this series
> does not prevent any Guest from ignoring kvmclock2 in favor of kvmclock.
>
> What this series is doing, i.e. separating the current two kvmclock and
> ensuring CPUID compatibility via legacy-kvmclock, could balance the
> compatibility requirements of the ancient (unmaintained kernel) with
> the need for future feature changes.
You introduce a user-visible change that people need to use
"-kvmclock,-kvmclock2" to totally disable kvmclock.
The only difference between kvmclock and kvmclock2 is the MSR index. And
from users' perspective, they don't care this difference. The existing
usage is simple. When I want kvmclock, use "+kvmclock". When I don't
want it, use "-kvmclock".
You are complicating things and I don't see a strong reason that we have
to do it.
> Thanks,
> Zhao
>
next prev parent reply other threads:[~2024-04-25 12:04 UTC|newest]
Thread overview: 18+ 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 ` [PATCH for-9.1 6/7] target/i386: Fix duplicated kvmclock name in FEAT_KVM Zhao Liu
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 [this message]
2024-04-25 13:36 ` Zhao Liu
2024-05-29 13:58 ` 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=6ee5cb90-d299-4977-8a2f-529f78dc3913@intel.com \
--to=xiaoyao.li@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 \
--cc=zhao1.liu@linux.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).