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.

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).

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

[1]: https://lore.kernel.org/qemu-devel/20240325141422.1380087-1-pbonzini@redhat.com/
[2]: https://lore.kernel.org/qemu-devel/20230908124534.25027-4-twiederh@redhat.com/
[3]: https://lore.kernel.org/qemu-devel/1300401727-5235-3-git-send-email-glommer@redhat.com/
[4]: https://lore.kernel.org/qemu-devel/1348171412-23669-3-git-send-email-Don@CloudSwitch.com/

Thanks and Best Regards,
Zhao

---
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
    (KVM_FEATURE_CLOCKSOURCE2)
  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(-)

-- 
2.34.1

Hi maintainers,

Ping. Do you like this diea?

Thanks,
Zhao

On Fri, Mar 29, 2024 at 06:19:47PM +0800, Zhao Liu wrote:
> Date: Fri, 29 Mar 2024 18:19:47 +0800
> From: Zhao Liu <zhao1.liu@linux.intel.com>
> Subject: [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock feature
>  name
> X-Mailer: git-send-email 2.34.1
> 
> 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.
> 
> 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).
> 
> Also, try to deprecate the old kvmclock in KVM side.
> 
> [1]: https://lore.kernel.org/qemu-devel/20240325141422.1380087-1-pbonzini@redhat.com/
> [2]: https://lore.kernel.org/qemu-devel/20230908124534.25027-4-twiederh@redhat.com/
> [3]: https://lore.kernel.org/qemu-devel/1300401727-5235-3-git-send-email-glommer@redhat.com/
> [4]: https://lore.kernel.org/qemu-devel/1348171412-23669-3-git-send-email-Don@CloudSwitch.com/
> 
> Thanks and Best Regards,
> Zhao
> 
> ---
> 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
>     (KVM_FEATURE_CLOCKSOURCE2)
>   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(-)
> 
> -- 
> 2.34.1
>

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.

> 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.
> 
> [1]: https://lore.kernel.org/qemu-devel/20240325141422.1380087-1-pbonzini@redhat.com/
> [2]: https://lore.kernel.org/qemu-devel/20230908124534.25027-4-twiederh@redhat.com/
> [3]: https://lore.kernel.org/qemu-devel/1300401727-5235-3-git-send-email-glommer@redhat.com/
> [4]: https://lore.kernel.org/qemu-devel/1348171412-23669-3-git-send-email-Don@CloudSwitch.com/
> 
> Thanks and Best Regards,
> Zhao
> 
> ---
> 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
>      (KVM_FEATURE_CLOCKSOURCE2)
>    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(-)
>

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.

Pls see:
https://elixir.bootlin.com/linux/v4.5/source/arch/x86/include/uapi/asm/kvm_para.h#L22

With this in mind, I'm trying to implement a silky smooth transition to
"only kcmclock2 support".

This series is now separating kvmclock and kvmclock2, and then I plan to
go to the KVM side and deprecate kvmclock2, and then finally remove
kvmclock (old) in QEMU. Separating the two features makes the process
clearer.

Continuing to use the same name equally would have silently caused the
CPUID to change on the Guest side, which would have hurt compatibility
as well.

> > 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.

Please refer to my elaboration on v4.5 above, where kvmclock2 is
available, it should be used in priority.

> > Also, try to deprecate the old kvmclock in KVM side.
> > 
> > [1]: https://lore.kernel.org/qemu-devel/20240325141422.1380087-1-pbonzini@redhat.com/
> > [2]: https://lore.kernel.org/qemu-devel/20230908124534.25027-4-twiederh@redhat.com/
> > [3]: https://lore.kernel.org/qemu-devel/1300401727-5235-3-git-send-email-glommer@redhat.com/
> > [4]: https://lore.kernel.org/qemu-devel/1348171412-23669-3-git-send-email-Don@CloudSwitch.com/

Thanks,
Zhao

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?

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.

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.

> Pls see:
> https://elixir.bootlin.com/linux/v4.5/source/arch/x86/include/uapi/asm/kvm_para.h#L22
> 
> With this in mind, I'm trying to implement a silky smooth transition to
> "only kcmclock2 support".
> 
> This series is now separating kvmclock and kvmclock2, and then I plan to
> go to the KVM side and deprecate kvmclock2, and then finally remove
> kvmclock (old) in QEMU. Separating the two features makes the process
> clearer.
> 
> Continuing to use the same name equally would have silently caused the
> CPUID to change on the Guest side, which would have hurt compatibility
> as well.
> 
>>> 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.
> 
> Please refer to my elaboration on v4.5 above, where kvmclock2 is
> available, it should be used in priority.
> 
>>> Also, try to deprecate the old kvmclock in KVM side.
>>>
>>> [1]: https://lore.kernel.org/qemu-devel/20240325141422.1380087-1-pbonzini@redhat.com/
>>> [2]: https://lore.kernel.org/qemu-devel/20230908124534.25027-4-twiederh@redhat.com/
>>> [3]: https://lore.kernel.org/qemu-devel/1300401727-5235-3-git-send-email-glommer@redhat.com/
>>> [4]: https://lore.kernel.org/qemu-devel/1348171412-23669-3-git-send-email-Don@CloudSwitch.com/
> 
> Thanks,
> Zhao
> 
>

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. 

> 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.

Thanks,
Zhao

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
>

On Thu, Apr 25, 2024 at 08:04:46PM +0800, Xiaoyao Li wrote:
> Date: Thu, 25 Apr 2024 20:04:46 +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 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.
>

Okay, only when old kvmclock deprecation occurs can the values (of
renaming) be recognized, then I'll hold the renamed patches for now and
only keep some of the other cleanups.

Thanks for your review,

-Zhao