Message-ID: <ZdjzIgS6EAeCsUue@google.com>
Date: Fri, 23 Feb 2024 11:33:54 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Isaku Yamahata <isaku.yamahata@...el.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com, 
	Vishal Annapurve <vannapurve@...gle.com>, Jim Mattson <jmattson@...gle.com>, 
	Maxim Levitsky <mlevitsk@...hat.com>, Xiaoyao Li <xiaoyao.li@...el.com>, isaku.yamahata@...il.com
Subject: Re: [PATCH v3 3/4] KVM: X86: Add a capability to configure bus
 frequency for APIC timer

On Tue, Dec 19, 2023, Isaku Yamahata wrote:
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 7025b3751027..cc976df2651e 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -7858,6 +7858,20 @@ This capability is aimed to mitigate the threat that malicious VMs can
>  cause CPU stuck (due to event windows don't open up) and make the CPU
>  unavailable to host or other VMs.
>  
> +7.34 KVM_CAP_X86_BUS_FREQUENCY_CONTROL

BUS_FREQUENCY_CONTROL is simultaneously too long, yet not descriptive enough.
Depending on whether people get hung up on nanoseconds not being a "frequency",
either KVM_CAP_X86_APIC_BUS_FREQUENCY or KVM_CAP_X86_APIC_BUS_CYCLES_NS.

Also, this series needs to be rebased onto kvm-x86/next.

> +:Architectures: x86
> +:Target: VM
> +:Parameters: args[0] is the value of apic bus clock frequency
> +:Returns: 0 on success, -EINVAL if args[0] contains invalid value for the
> +          frequency, or -ENXIO if virtual local APIC isn't enabled by
> +          KVM_CREATE_IRQCHIP, or -EBUSY if any vcpu is created.
> +
> +This capability sets the APIC bus clock frequency (or core crystal clock
> +frequency) for kvm to emulate APIC in the kernel.  The default value is 1000000
> +(1GHz).

If we're going to add a capability, might as well make KVM's default value
discoverable.

This also needs to clarify the units.  Having to count the number of zeros in the
code to figure that out is ridiculous.

And as Xiaoyao, this is NOT the core crystal clock.  Though conversely, this
documentation should make it clear that setting CPUID 0x15 is userspace's problem.
E.g.

7.35 KVM_CAP_X86_APIC_BUS_FREQUENCY
-----------------------------------

:Architectures: x86
:Target: VM
:Parameters: args[0] is the desired APIC bus clock rate, in nanoseconds
:Returns: 0 on success, -EINVAL if args[0] contains invalid value for the
          frequency, or -ENXIO if virtual local APIC isn't enabled by
          KVM_CREATE_IRQCHIP, or -EBUSY if any vcpu is created.

This capability sets VM's APIC bus clock frequency, used by KVM's in-kernel
virtual APIC when emulating APIC timers.  KVM's default value can be retrieved
by KVM_CHECK_EXTENSION.

Note: Userspace is responsible for correctly configuring CPUID 0x15, a.k.a. the
core crystal clock frequency, if a non-zero CPUID 0x15 is exposed to the guest.

>  8. Other capabilities.
>  ======================
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d7d865f7c847..97f81d612366 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4625,6 +4625,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ENABLE_CAP:
>  	case KVM_CAP_VM_DISABLE_NX_HUGE_PAGES:
>  	case KVM_CAP_IRQFD_RESAMPLE:
> +	case KVM_CAP_X86_BUS_FREQUENCY_CONTROL:
>  		r = 1;

And instead of returning '1', return APIC_BUS_CYCLE_NS_DEFAULT (which is amusingly
also '1', but there's no reason to rely on that, it's unnecessarily confusing).

>  		break;
>  	case KVM_CAP_EXIT_HYPERCALL:
> @@ -6616,6 +6617,38 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		}
>  		mutex_unlock(&kvm->lock);
>  		break;
> +	case KVM_CAP_X86_BUS_FREQUENCY_CONTROL: {
> +		u64 bus_frequency = cap->args[0];
> +		u64 bus_cycle_ns;
> +
> +		if (!bus_frequency)
> +			return -EINVAL;
> +		/* CPUID[0x15] only support 32bits.  */

So?  This capability might exist to play nice with TDX forcing CPUID 0x15, but
that doesn't mean the capability is beholden to CPUID 0x15.

> +		if (bus_frequency != (u32)bus_frequency)
> +			return -EINVAL;
> +
> +		/* Cast to avoid 64bit division on 32bit platform. */
> +		bus_cycle_ns = 1000000000UL / (u32)bus_frequency;

Why take the userspace value as a frequency?  That will unnecessarily result in
loss of fidelity if 1000000000UL isn't cleanly disibile by bus_frequency, e.g.
reversing the math in the Hyper-V code will yield the "wrong" frequency.

> +		if (!bus_cycle_ns)

This needs to guard against overflow in tmict_to_ns().  The max divide count is
14, I think?  Whatever this yields:

	apic->divide_count = 0x1 << (tmp2 & 0x7);

So from that, we can derive the max allowed bus_cycle_ns.

> +			return -EINVAL;

Use break, like literally every other case statement.  Burying a return in the
middle of this pile will result in breakage if kvm_vm_ioctl_enable_cap() ever
gains an epilogue.

> +
> +		r = 0;
> +		mutex_lock(&kvm->lock);
> +		/*
> +		 * Don't allow to change the frequency dynamically during vcpu
> +		 * running to avoid potentially bizarre behavior.
> +		 */
> +		if (kvm->created_vcpus)
> +			r = -EBUSY;

EINVAL, not EBUSY, because userspace can't magically uncreate vCPUs.

> +		/* This is for in-kernel vAPIC emulation. */

Meh, just drop the comment.  Same for the one above created_vcpus, it's fairly
self-explantory.

> +		else if (!irqchip_in_kernel(kvm))
> +			r = -ENXIO;

This should go before created_vcpus, e.g. creating a vCPU shouldn't change the
error code.

> +
> +		if (!r)

Make this an else...

> +			kvm->arch.apic_bus_cycle_ns = bus_cycle_ns;

> +		mutex_unlock(&kvm->lock);
> +		return r;

		break;

Something like:

	case KVM_CAP_X86_APIC_BUS_FREQUENCY: {
		u64 bus_cycle_ns = cap->args[0];

		r = -EINVAL;
		if (!bus_frequency || bus_frequency > (whatever cause overflow))
			break;

		r = 0;
		mutex_lock(&kvm->lock);
		if (!irqchip_in_kernel(kvm))
			r = -ENXIO;
		else if (kvm->created_vcpus)
			r = -EINVAL;
		else
			kvm->arch.apic_bus_cycle_ns = bus_cycle_ns;
		mutex_unlock(&kvm->lock);
		break;
	}

Powered by blists - more mailing lists