[PATCH 0/2] Improve KVM_SET_TSC_KHZ handling for CoCo VMs

[PATCH 0/2] Improve KVM_SET_TSC_KHZ handling for CoCo VMs

[Kai Huang]

This series follows Sean's suggestions [1][2] to:

 - Reject vCPU scope KVM_SET_TSC_KHZ ioctl for TSC protected vCPU
 - Reject VM scope KVM_SET_TSC_KHZ ioctl when vCPUs have been created

.. in the discussion of SEV-SNP Secure TSC support series.

This series has been sanity tested with TDX guests using today's Qemu:

 - With this series Qemu can still run TDX guests successfully.
 - With some hack to the Qemu, both VM and vCPU scope KVM_SET_TSC_KHZ
   ioctls failed as expected.

Currently only TDX guests are TSC protected.  The SEV-SNP Secure TSC
support will enable protected TSC too and can also benefit from this
series.

[1]: https://lore.kernel.org/kvm/aG0uUdY6QPnit6my@google.com/
[2]: https://lore.kernel.org/kvm/aG2k2BFBJHL-szZc@google.com/

Kai Huang (2):
  KVM: x86: Reject KVM_SET_TSC_KHZ vCPU ioctl for TSC protected guest
  KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

 arch/x86/kvm/x86.c | 8 ++++++++
 1 file changed, 8 insertions(+)


base-commit: 6c7ecd725e503bf2ca69ff52c6cc48bb650b1f11
-- 
2.50.0

[PATCH 1/2] KVM: x86: Reject KVM_SET_TSC_KHZ vCPU ioctl for TSC protected guest

[Kai Huang]

Reject KVM_SET_TSC_KHZ vCPU ioctl if guest's TSC is protected and not
changeable by KVM.

For such TSC protected guests, e.g. TDX guests, typically the TSC is
configured once at VM level before any vCPU are created and remains
unchanged during VM's lifetime.  KVM provides the KVM_SET_TSC_KHZ VM
scope ioctl to allow the userspace VMM to configure the TSC of such VM.
After that the userspace VMM is not supposed to call the KVM_SET_TSC_KHZ
vCPU scope ioctl anymore when creating the vCPU.

The de facto userspace VMM Qemu does this for TDX guests.  The upcoming
SEV-SNP guests with Secure TSC should follow.

Note this could be a break of ABI.  But for now only TDX guests are TSC
protected and only Qemu supports TDX, thus in practice this should not
break any existing userspace.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kvm/x86.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2806f7104295..699ca5e74bba 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6186,6 +6186,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		u32 user_tsc_khz;
 
 		r = -EINVAL;
+
+		if (vcpu->arch.guest_tsc_protected)
+			goto out;
+
 		user_tsc_khz = (u32)arg;
 
 		if (kvm_caps.has_tsc_control &&
-- 
2.50.0

Re: [PATCH 1/2] KVM: x86: Reject KVM_SET_TSC_KHZ vCPU ioctl for TSC protected guest

[Xiaoyao Li]

On 7/9/2025 1:37 PM, Kai Huang wrote:
> Reject KVM_SET_TSC_KHZ vCPU ioctl if guest's TSC is protected and not
> changeable by KVM.
> 
> For such TSC protected guests, e.g. TDX guests, typically the TSC is
> configured once at VM level before any vCPU are created and remains
> unchanged during VM's lifetime.  KVM provides the KVM_SET_TSC_KHZ VM
> scope ioctl to allow the userspace VMM to configure the TSC of such VM.
> After that the userspace VMM is not supposed to call the KVM_SET_TSC_KHZ
> vCPU scope ioctl anymore when creating the vCPU.
> 
> The de facto userspace VMM Qemu does this for TDX guests.  The upcoming
> SEV-SNP guests with Secure TSC should follow.
> 
> Note this could be a break of ABI.  But for now only TDX guests are TSC
> protected and only Qemu supports TDX, thus in practice this should not
> break any existing userspace.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   arch/x86/kvm/x86.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2806f7104295..699ca5e74bba 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6186,6 +6186,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>   		u32 user_tsc_khz;
>   
>   		r = -EINVAL;

I wondered if -EPERM is more appropriate. But I search the KVM files and 
find the KVM convention is to return -EINVAL.

> +		if (vcpu->arch.guest_tsc_protected)
> +			goto out;
> +
>   		user_tsc_khz = (u32)arg;
>   
>   		if (kvm_caps.has_tsc_control &&

Re: [PATCH 1/2] KVM: x86: Reject KVM_SET_TSC_KHZ vCPU ioctl for TSC protected guest

[Nikunj A. Dadhania]

On 7/9/2025 11:07 AM, Kai Huang wrote:
> Reject KVM_SET_TSC_KHZ vCPU ioctl if guest's TSC is protected and not
> changeable by KVM.
> 
> For such TSC protected guests, e.g. TDX guests, typically the TSC is
> configured once at VM level before any vCPU are created and remains
> unchanged during VM's lifetime.  KVM provides the KVM_SET_TSC_KHZ VM
> scope ioctl to allow the userspace VMM to configure the TSC of such VM.
> After that the userspace VMM is not supposed to call the KVM_SET_TSC_KHZ
> vCPU scope ioctl anymore when creating the vCPU.
> 
> The de facto userspace VMM Qemu does this for TDX guests.  The upcoming
> SEV-SNP guests with Secure TSC should follow.
> 
> Note this could be a break of ABI.  But for now only TDX guests are TSC
> protected and only Qemu supports TDX, thus in practice this should not
> break any existing userspace.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Need to add this in Documentation/virt/kvm/api.rst as well, saying that
for TDX and SecureTSC enabled SNP guests, KVM_SET_TSC_KHZ vCPU ioctl is
not valid.

> ---
>  arch/x86/kvm/x86.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2806f7104295..699ca5e74bba 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6186,6 +6186,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		u32 user_tsc_khz;
>  
>  		r = -EINVAL;
> +
> +		if (vcpu->arch.guest_tsc_protected)
> +			goto out;
> +
>  		user_tsc_khz = (u32)arg;
>  
>  		if (kvm_caps.has_tsc_control &&

Regards
Nikunj

Re: [PATCH 1/2] KVM: x86: Reject KVM_SET_TSC_KHZ vCPU ioctl for TSC protected guest

[Huang, Kai]

On Wed, 2025-07-09 at 14:09 +0530, Nikunj A. Dadhania wrote:
> 
> On 7/9/2025 11:07 AM, Kai Huang wrote:
> > Reject KVM_SET_TSC_KHZ vCPU ioctl if guest's TSC is protected and not
> > changeable by KVM.
> > 
> > For such TSC protected guests, e.g. TDX guests, typically the TSC is
> > configured once at VM level before any vCPU are created and remains
> > unchanged during VM's lifetime.  KVM provides the KVM_SET_TSC_KHZ VM
> > scope ioctl to allow the userspace VMM to configure the TSC of such VM.
> > After that the userspace VMM is not supposed to call the KVM_SET_TSC_KHZ
> > vCPU scope ioctl anymore when creating the vCPU.
> > 
> > The de facto userspace VMM Qemu does this for TDX guests.  The upcoming
> > SEV-SNP guests with Secure TSC should follow.
> > 
> > Note this could be a break of ABI.  But for now only TDX guests are TSC
> > protected and only Qemu supports TDX, thus in practice this should not
> > break any existing userspace.
> > 
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> 
> Need to add this in Documentation/virt/kvm/api.rst as well, saying that
> for TDX and SecureTSC enabled SNP guests, KVM_SET_TSC_KHZ vCPU ioctl is
> not valid.
> 
> 

Good point.  Thanks for bringing it up.

I will add below to the doc unless someone has comments?

I'll probably split the doc diff into two parts and merge each to the
respective code change patch, since the change to the doc contains change
to both vm ioctl and vcpu ioctl.

Btw, I think I'll not mention Secure TSC enabled SEV-SNP guests for now
because it is not in upstream yet.  But I tried to make the text in a way
that could be easily extended to cover Secure TSC guests.

diff --git a/Documentation/virt/kvm/api.rst
b/Documentation/virt/kvm/api.rst
index 43ed57e048a8..ad61bcba3791 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -2006,7 +2006,13 @@ frequency is KHz.
 
 If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
 be used as a vm ioctl to set the initial tsc frequency of subsequently
-created vCPUs.
+created vCPUs.  It must be called before any vCPU is created.
+
+For TSC protected CoCo VMs where TSC is configured once at VM scope and
+remains unchanged during VM's lifetime, the VM ioctl should be used to
+configure the TSC and the vCPU ioctl fails.
+
+Example of such CoCo VMs: TDX guests.

Re: [PATCH 1/2] KVM: x86: Reject KVM_SET_TSC_KHZ vCPU ioctl for TSC protected guest

[Nikunj A Dadhania]

"Huang, Kai" <kai.huang@intel.com> writes:

> On Wed, 2025-07-09 at 14:09 +0530, Nikunj A. Dadhania wrote:
>> 
>> On 7/9/2025 11:07 AM, Kai Huang wrote:
>> > Reject KVM_SET_TSC_KHZ vCPU ioctl if guest's TSC is protected and not
>> > changeable by KVM.
>> > 
>> > For such TSC protected guests, e.g. TDX guests, typically the TSC is
>> > configured once at VM level before any vCPU are created and remains
>> > unchanged during VM's lifetime.  KVM provides the KVM_SET_TSC_KHZ VM
>> > scope ioctl to allow the userspace VMM to configure the TSC of such VM.
>> > After that the userspace VMM is not supposed to call the KVM_SET_TSC_KHZ
>> > vCPU scope ioctl anymore when creating the vCPU.
>> > 
>> > The de facto userspace VMM Qemu does this for TDX guests.  The upcoming
>> > SEV-SNP guests with Secure TSC should follow.
>> > 
>> > Note this could be a break of ABI.  But for now only TDX guests are TSC
>> > protected and only Qemu supports TDX, thus in practice this should not
>> > break any existing userspace.
>> > 
>> > Suggested-by: Sean Christopherson <seanjc@google.com>
>> > Signed-off-by: Kai Huang <kai.huang@intel.com>
>> 
>> Need to add this in Documentation/virt/kvm/api.rst as well, saying that
>> for TDX and SecureTSC enabled SNP guests, KVM_SET_TSC_KHZ vCPU ioctl is
>> not valid.
>> 
>> 
>
> Good point.  Thanks for bringing it up.
>
> I will add below to the doc unless someone has comments?
>
> I'll probably split the doc diff into two parts and merge each to the
> respective code change patch, since the change to the doc contains change
> to both vm ioctl and vcpu ioctl.
>
> Btw, I think I'll not mention Secure TSC enabled SEV-SNP guests for now
> because it is not in upstream yet.  But I tried to make the text in a way
> that could be easily extended to cover Secure TSC guests.

Sure, I can add that later.

>
> diff --git a/Documentation/virt/kvm/api.rst
> b/Documentation/virt/kvm/api.rst
> index 43ed57e048a8..ad61bcba3791 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -2006,7 +2006,13 @@ frequency is KHz.
>  
>  If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
>  be used as a vm ioctl to set the initial tsc frequency of subsequently
> -created vCPUs.
> +created vCPUs.  It must be called before any vCPU is created.

s/It/The VM Scope ioctl/

> +
> +For TSC protected CoCo VMs where TSC is configured once at VM scope
> and

s/CoCo/Confidential Computing (CoCo)/
s/TSC is/TSC frequency is/

> +remains unchanged during VM's lifetime, the VM ioctl should be used to
> +configure the TSC and the vCPU ioctl fails.

s/TSC/TSC frequency/

s/vcpu ioctl fails/vcpu ioctl is not supported/

> +
> +
> +Example of such CoCo VMs: TDX guests.

Regards
Nikunj

Re: [PATCH 1/2] KVM: x86: Reject KVM_SET_TSC_KHZ vCPU ioctl for TSC protected guest

[Huang, Kai]

On Fri, 2025-07-11 at 06:22 +0000, Nikunj A Dadhania wrote:
> "Huang, Kai" <kai.huang@intel.com> writes:
> 
> > On Wed, 2025-07-09 at 14:09 +0530, Nikunj A. Dadhania wrote:
> > > 
> > > On 7/9/2025 11:07 AM, Kai Huang wrote:
> > > > Reject KVM_SET_TSC_KHZ vCPU ioctl if guest's TSC is protected and not
> > > > changeable by KVM.
> > > > 
> > > > For such TSC protected guests, e.g. TDX guests, typically the TSC is
> > > > configured once at VM level before any vCPU are created and remains
> > > > unchanged during VM's lifetime.  KVM provides the KVM_SET_TSC_KHZ VM
> > > > scope ioctl to allow the userspace VMM to configure the TSC of such VM.
> > > > After that the userspace VMM is not supposed to call the KVM_SET_TSC_KHZ
> > > > vCPU scope ioctl anymore when creating the vCPU.
> > > > 
> > > > The de facto userspace VMM Qemu does this for TDX guests.  The upcoming
> > > > SEV-SNP guests with Secure TSC should follow.
> > > > 
> > > > Note this could be a break of ABI.  But for now only TDX guests are TSC
> > > > protected and only Qemu supports TDX, thus in practice this should not
> > > > break any existing userspace.
> > > > 
> > > > Suggested-by: Sean Christopherson <seanjc@google.com>
> > > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > > 
> > > Need to add this in Documentation/virt/kvm/api.rst as well, saying that
> > > for TDX and SecureTSC enabled SNP guests, KVM_SET_TSC_KHZ vCPU ioctl is
> > > not valid.
> > > 
> > > 
> > 
> > Good point.  Thanks for bringing it up.
> > 
> > I will add below to the doc unless someone has comments?
> > 
> > I'll probably split the doc diff into two parts and merge each to the
> > respective code change patch, since the change to the doc contains change
> > to both vm ioctl and vcpu ioctl.
> > 
> > Btw, I think I'll not mention Secure TSC enabled SEV-SNP guests for now
> > because it is not in upstream yet.  But I tried to make the text in a way
> > that could be easily extended to cover Secure TSC guests.
> 
> Sure, I can add that later.
> 
> > 
> > diff --git a/Documentation/virt/kvm/api.rst
> > b/Documentation/virt/kvm/api.rst
> > index 43ed57e048a8..ad61bcba3791 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -2006,7 +2006,13 @@ frequency is KHz.
> >  
> >  If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
> >  be used as a vm ioctl to set the initial tsc frequency of subsequently
> > -created vCPUs.
> > +created vCPUs.  It must be called before any vCPU is created.
> 
> s/It/The VM Scope ioctl/

OK.  I'll use "The vm ioctl", though, to make it consistent with the
previous sentence.

> 
> > +
> > +For TSC protected CoCo VMs where TSC is configured once at VM scope
> > and
> 
> s/CoCo/Confidential Computing (CoCo)/
> s/TSC is/TSC frequency is/

OK.

> 
> > +remains unchanged during VM's lifetime, the VM ioctl should be used to
> > +configure the TSC and the vCPU ioctl fails.
> 
> s/TSC/TSC frequency/
> 
> s/vcpu ioctl fails/vcpu ioctl is not supported/

OK.

> 
> > +
> > +
> > +Example of such CoCo VMs: TDX guests.
> 
> Regards
> Nikunj

[PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Kai Huang]

Reject the KVM_SET_TSC_KHZ VM ioctl when there's vCPU has already been
created.

The VM scope KVM_SET_TSC_KHZ ioctl is used to set up the default TSC
frequency that all subsequent created vCPUs use.  It is only intended to
be called before any vCPU is created.  Allowing it to be called after
that only results in confusion but nothing good.

Note this is an ABI change.  But currently in Qemu (the de facto
userspace VMM) only TDX uses this VM ioctl, and it is only called once
before creating any vCPU, therefore the risk of breaking userspace is
pretty low.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kvm/x86.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 699ca5e74bba..e5e55d549468 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7194,6 +7194,10 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		u32 user_tsc_khz;
 
 		r = -EINVAL;
+
+		if (kvm->created_vcpus)
+			goto out;
+
 		user_tsc_khz = (u32)arg;
 
 		if (kvm_caps.has_tsc_control &&
-- 
2.50.0

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Xiaoyao Li]

On 7/9/2025 1:38 PM, Kai Huang wrote:
> Reject the KVM_SET_TSC_KHZ VM ioctl when there's vCPU has already been
> created.
> 
> The VM scope KVM_SET_TSC_KHZ ioctl is used to set up the default TSC
> frequency that all subsequent created vCPUs use.  It is only intended to
> be called before any vCPU is created.  Allowing it to be called after
> that only results in confusion but nothing good.
> 
> Note this is an ABI change.  But currently in Qemu (the de facto
> userspace VMM) only TDX uses this VM ioctl, and it is only called once
> before creating any vCPU, therefore the risk of breaking userspace is
> pretty low.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
>   arch/x86/kvm/x86.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 699ca5e74bba..e5e55d549468 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7194,6 +7194,10 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>   		u32 user_tsc_khz;
>   
>   		r = -EINVAL;
> +
> +		if (kvm->created_vcpus)
> +			goto out;
> +
>   		user_tsc_khz = (u32)arg;
>   
>   		if (kvm_caps.has_tsc_control &&

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Chao Gao]

On Wed, Jul 09, 2025 at 05:38:00PM +1200, Kai Huang wrote:
>Reject the KVM_SET_TSC_KHZ VM ioctl when there's vCPU has already been
>created.
>
>The VM scope KVM_SET_TSC_KHZ ioctl is used to set up the default TSC
>frequency that all subsequent created vCPUs use.  It is only intended to
>be called before any vCPU is created.  Allowing it to be called after
>that only results in confusion but nothing good.
>
>Note this is an ABI change.  But currently in Qemu (the de facto
>userspace VMM) only TDX uses this VM ioctl, and it is only called once
>before creating any vCPU, therefore the risk of breaking userspace is
>pretty low.
>
>Suggested-by: Sean Christopherson <seanjc@google.com>
>Signed-off-by: Kai Huang <kai.huang@intel.com>
>---
> arch/x86/kvm/x86.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 699ca5e74bba..e5e55d549468 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -7194,6 +7194,10 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> 		u32 user_tsc_khz;
> 
> 		r = -EINVAL;
>+
>+		if (kvm->created_vcpus)
>+			goto out;
>+

shouldn't kvm->lock be held?

> 		user_tsc_khz = (u32)arg;
> 
> 		if (kvm_caps.has_tsc_control &&
>-- 
>2.50.0
>
>

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Sean Christopherson]

On Wed, Jul 09, 2025, Chao Gao wrote:
> On Wed, Jul 09, 2025 at 05:38:00PM +1200, Kai Huang wrote:
> >Reject the KVM_SET_TSC_KHZ VM ioctl when there's vCPU has already been
> >created.
> >
> >The VM scope KVM_SET_TSC_KHZ ioctl is used to set up the default TSC
> >frequency that all subsequent created vCPUs use.  It is only intended to
> >be called before any vCPU is created.  Allowing it to be called after
> >that only results in confusion but nothing good.
> >
> >Note this is an ABI change.  But currently in Qemu (the de facto
> >userspace VMM) only TDX uses this VM ioctl, and it is only called once
> >before creating any vCPU, therefore the risk of breaking userspace is
> >pretty low.
> >
> >Suggested-by: Sean Christopherson <seanjc@google.com>
> >Signed-off-by: Kai Huang <kai.huang@intel.com>
> >---
> > arch/x86/kvm/x86.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 699ca5e74bba..e5e55d549468 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -7194,6 +7194,10 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> > 		u32 user_tsc_khz;
> > 
> > 		r = -EINVAL;
> >+
> >+		if (kvm->created_vcpus)
> >+			goto out;
> >+
> 
> shouldn't kvm->lock be held?

Yep.

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Huang, Kai]

On Wed, 2025-07-09 at 06:55 -0700, Sean Christopherson wrote:
> On Wed, Jul 09, 2025, Chao Gao wrote:
> > On Wed, Jul 09, 2025 at 05:38:00PM +1200, Kai Huang wrote:
> > > Reject the KVM_SET_TSC_KHZ VM ioctl when there's vCPU has already been
> > > created.
> > > 
> > > The VM scope KVM_SET_TSC_KHZ ioctl is used to set up the default TSC
> > > frequency that all subsequent created vCPUs use.  It is only intended to
> > > be called before any vCPU is created.  Allowing it to be called after
> > > that only results in confusion but nothing good.
> > > 
> > > Note this is an ABI change.  But currently in Qemu (the de facto
> > > userspace VMM) only TDX uses this VM ioctl, and it is only called once
> > > before creating any vCPU, therefore the risk of breaking userspace is
> > > pretty low.
> > > 
> > > Suggested-by: Sean Christopherson <seanjc@google.com>
> > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > > ---
> > > arch/x86/kvm/x86.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 699ca5e74bba..e5e55d549468 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -7194,6 +7194,10 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> > > 		u32 user_tsc_khz;
> > > 
> > > 		r = -EINVAL;
> > > +
> > > +		if (kvm->created_vcpus)
> > > +			goto out;
> > > +
> > 
> > shouldn't kvm->lock be held?
> 
> Yep.

My bad.  I'll fixup and send out v2 soon,  together with the doc update.

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Huang, Kai]

On Thu, 2025-07-10 at 22:53 +0000, Huang, Kai wrote:
> On Wed, 2025-07-09 at 06:55 -0700, Sean Christopherson wrote:
> > On Wed, Jul 09, 2025, Chao Gao wrote:
> > > On Wed, Jul 09, 2025 at 05:38:00PM +1200, Kai Huang wrote:
> > > > Reject the KVM_SET_TSC_KHZ VM ioctl when there's vCPU has already been
> > > > created.
> > > > 
> > > > The VM scope KVM_SET_TSC_KHZ ioctl is used to set up the default TSC
> > > > frequency that all subsequent created vCPUs use.  It is only intended to
> > > > be called before any vCPU is created.  Allowing it to be called after
> > > > that only results in confusion but nothing good.
> > > > 
> > > > Note this is an ABI change.  But currently in Qemu (the de facto
> > > > userspace VMM) only TDX uses this VM ioctl, and it is only called once
> > > > before creating any vCPU, therefore the risk of breaking userspace is
> > > > pretty low.
> > > > 
> > > > Suggested-by: Sean Christopherson <seanjc@google.com>
> > > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > > > ---
> > > > arch/x86/kvm/x86.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index 699ca5e74bba..e5e55d549468 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -7194,6 +7194,10 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> > > > 		u32 user_tsc_khz;
> > > > 
> > > > 		r = -EINVAL;
> > > > +
> > > > +		if (kvm->created_vcpus)
> > > > +			goto out;
> > > > +
> > > 
> > > shouldn't kvm->lock be held?
> > 
> > Yep.
> 
> My bad.  I'll fixup and send out v2 soon,  together with the doc update.

Sorry for multiple emails, but I ended up with below.

AFAICT the actual updating of kvm->arch.default_tsc_khz needs to be in the
kvm->lock mutex too.  Please let me know if you found any issue?

diff --git a/Documentation/virt/kvm/api.rst
b/Documentation/virt/kvm/api.rst
index 43ed57e048a8..86ea1e2b2737 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -2006,7 +2006,7 @@ frequency is KHz.

 If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
 be used as a vm ioctl to set the initial tsc frequency of subsequently
-created vCPUs.
+created vCPUs.  It must be called before any vCPU is created.

 4.56 KVM_GET_TSC_KHZ
 --------------------
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2806f7104295..4051c0cacb92 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7199,9 +7199,12 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned
int ioctl, unsigned long arg)
                if (user_tsc_khz == 0)
                        user_tsc_khz = tsc_khz;

-               WRITE_ONCE(kvm->arch.default_tsc_khz, user_tsc_khz);
-               r = 0;
-
+               mutex_lock(&kvm->lock);
+               if (!kvm->created_vcpus) {
+                       WRITE_ONCE(kvm->arch.default_tsc_khz,
user_tsc_khz);
+                       r = 0;
+               }
+               mutex_unlock(&kvm->lock);
                goto out;
        }
        case KVM_GET_TSC_KHZ: {

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Chao Gao]

>AFAICT the actual updating of kvm->arch.default_tsc_khz needs to be in the
>kvm->lock mutex too.

Yep.

>Please let me know if you found any issue?
>
>diff --git a/Documentation/virt/kvm/api.rst
>b/Documentation/virt/kvm/api.rst
>index 43ed57e048a8..86ea1e2b2737 100644
>--- a/Documentation/virt/kvm/api.rst
>+++ b/Documentation/virt/kvm/api.rst
>@@ -2006,7 +2006,7 @@ frequency is KHz.
>
> If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
> be used as a vm ioctl to set the initial tsc frequency of subsequently
>-created vCPUs.
>+created vCPUs.  It must be called before any vCPU is created.

		^^ remove one space here.

"must be" sounds like a mandatory action, but IIUC the vm ioctl is optional for
non-CC VMs. I'm not sure if this is just a problem of my interpretation.

To make the API documentation super clear, how about:

If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
be used as a vm ioctl to set the initial tsc frequency of vCPUs before
any vCPU is created. Attempting to call this vm ioctl after vCPU creation
will return an EINVAL error.

>
> 4.56 KVM_GET_TSC_KHZ
> --------------------
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 2806f7104295..4051c0cacb92 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -7199,9 +7199,12 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned
>int ioctl, unsigned long arg)
>                if (user_tsc_khz == 0)
>                        user_tsc_khz = tsc_khz;
>
>-               WRITE_ONCE(kvm->arch.default_tsc_khz, user_tsc_khz);
>-               r = 0;
>-
>+               mutex_lock(&kvm->lock);
>+               if (!kvm->created_vcpus) {
>+                       WRITE_ONCE(kvm->arch.default_tsc_khz,
>user_tsc_khz);
>+                       r = 0;
>+               }
>+               mutex_unlock(&kvm->lock);

LGTM.

>                goto out;
>        }
>        case KVM_GET_TSC_KHZ: {

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Huang, Kai]

On Fri, 2025-07-11 at 10:17 +0800, Chao Gao wrote:
> > AFAICT the actual updating of kvm->arch.default_tsc_khz needs to be in the
> > kvm->lock mutex too.
> 
> Yep.
> 
> > Please let me know if you found any issue?
> > 
> > diff --git a/Documentation/virt/kvm/api.rst
> > b/Documentation/virt/kvm/api.rst
> > index 43ed57e048a8..86ea1e2b2737 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -2006,7 +2006,7 @@ frequency is KHz.
> > 
> > If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
> > be used as a vm ioctl to set the initial tsc frequency of subsequently
> > -created vCPUs.
> > +created vCPUs.  It must be called before any vCPU is created.
> 
> 		^^ remove one space here.

OK.

> 
> "must be" sounds like a mandatory action, but IIUC the vm ioctl is optional for
> non-CC VMs. I'm not sure if this is just a problem of my interpretation.

The context of that paragraph has "can also be used ...", so to me it's
implied that "if it is called", i.e., it's implied that it is optional for
non-CC VMs.

> 
> To make the API documentation super clear, how about:
> 
> If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
> be used as a vm ioctl to set the initial tsc frequency of vCPUs before
> any vCPU is created. Attempting to call this vm ioctl after vCPU creation
> will return an EINVAL error.

I am not sure we need to mention -EINVAL.  This IOCTL is already returning
-EINVAL for other errors (when invalid user_tsc_khz is supplied).

IMHO: Userspace should just care about whether success or not.  Being
explicit is good, but sometimes it's better to have some room here.

But I'll let Sean/Paolo to decide.

> 
> > 
> > 4.56 KVM_GET_TSC_KHZ
> > --------------------
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2806f7104295..4051c0cacb92 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -7199,9 +7199,12 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned
> > int ioctl, unsigned long arg)
> >                if (user_tsc_khz == 0)
> >                        user_tsc_khz = tsc_khz;
> > 
> > -               WRITE_ONCE(kvm->arch.default_tsc_khz, user_tsc_khz);
> > -               r = 0;
> > -
> > +               mutex_lock(&kvm->lock);
> > +               if (!kvm->created_vcpus) {
> > +                       WRITE_ONCE(kvm->arch.default_tsc_khz,
> > user_tsc_khz);
> > +                       r = 0;
> > +               }
> > +               mutex_unlock(&kvm->lock);
> 
> LGTM.
> 
> >                goto out;
> >        }
> >        case KVM_GET_TSC_KHZ: {

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Nikunj A. Dadhania]

On 7/9/2025 11:08 AM, Kai Huang wrote:
> Reject the KVM_SET_TSC_KHZ VM ioctl when there's vCPU has already been
> created.

Probably the below is clear:

Reject KVM_SET_TSC_KHZ VM ioctl when vCPUs have been created

> 
> The VM scope KVM_SET_TSC_KHZ ioctl is used to set up the default TSC
> frequency that all subsequent created vCPUs use.  It is only intended to
> be called before any vCPU is created.  Allowing it to be called after
> that only results in confusion but nothing good.
> 
> Note this is an ABI change.  But currently in Qemu (the de facto
> userspace VMM) only TDX uses this VM ioctl, and it is only called once
> before creating any vCPU, therefore the risk of breaking userspace is
> pretty low.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>

> ---
>  arch/x86/kvm/x86.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 699ca5e74bba..e5e55d549468 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7194,6 +7194,10 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  		u32 user_tsc_khz;
>  
>  		r = -EINVAL;
> +
> +		if (kvm->created_vcpus)
> +			goto out;
> +
>  		user_tsc_khz = (u32)arg;
>  
>  		if (kvm_caps.has_tsc_control &&

Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

[Huang, Kai]

On Wed, 2025-07-09 at 14:21 +0530, Nikunj A. Dadhania wrote:
> 
> On 7/9/2025 11:08 AM, Kai Huang wrote:
> > Reject the KVM_SET_TSC_KHZ VM ioctl when there's vCPU has already been
> > created.
> 
> Probably the below is clear:
> 
> Reject KVM_SET_TSC_KHZ VM ioctl when vCPUs have been created

Will do.  Thanks.

> 
> > 
> > The VM scope KVM_SET_TSC_KHZ ioctl is used to set up the default TSC
> > frequency that all subsequent created vCPUs use.  It is only intended to
> > be called before any vCPU is created.  Allowing it to be called after
> > that only results in confusion but nothing good.
> > 
> > Note this is an ABI change.  But currently in Qemu (the de facto
> > userspace VMM) only TDX uses this VM ioctl, and it is only called once
> > before creating any vCPU, therefore the risk of breaking userspace is
> > pretty low.
> > 
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> 
> Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>

Thanks.  I'll keep you guys RB anyway after adding the mutex.  Let me know
if you are not OK.