Re: [PATCH 2/2] KVM: X86: Add a capability to configure bus frequency for APIC timer

[Isaku Yamahata]

On Tue, Nov 07, 2023 at 11:59:35AM -0800,
Jim Mattson <jmattson@xxxxxxxxxx> wrote:

> On Tue, Nov 7, 2023 at 11:24 AM <isaku.yamahata@xxxxxxxxx> wrote:
> >
> > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> >
> > Add KVM_CAP_X86_BUS_FREQUENCY_CONTROL capability to configure the core
> > crystal clock (or processor's bus clock) for APIC timer emulation.  Allow
> > KVM_ENABLE_CAPABILITY(KVM_CAP_X86_BUS_FREUQNCY_CONTROL) to set the
> Nit: FREQUENCY
> > frequency.  When using this capability, the user space VMM should configure
> > CPUID[0x15] to advertise the frequency.
> 
> Is it necessary to advertise the frequency in CPUID.15H? No hardware
> that I know of has a 1 GHz crystal clock, but the current
> implementation works fine without CPUID.15H.

It's not necessary. When the kernel can't determine the frequency based on
cpuid (or cpu model), it determines the frequency based on other known
frequency. e.g. TSC, cmos. I'll drop the sentence.


> > TDX virtualizes CPUID[0x15] for the core crystal clock to be 25MHz.  The
> > x86 KVM hardcodes its freuqncy for APIC timer to be 1GHz.  This mismatch
> Nit: frequency
> > causes the vAPIC timer to fire earlier than the guest expects. [1] The KVM
> > APIC timer emulation uses hrtimer, whose unit is nanosecond.  Make the
> > parameter configurable for conversion from the TMICT value to nanosecond.
> >
> > This patch doesn't affect the TSC deadline timer emulation.  The TSC
> > deadline emulation path records its expiring TSC value and calculates the
> > expiring time in nanoseconds.  The APIC timer emulation path calculates the
> > TSC value from the TMICT register value and uses the TSC deadline timer
> > path.  This patch touches the APIC timer-specific code but doesn't touch
> > common logic.
> >
> > [1] https://lore.kernel.org/lkml/20231006011255.4163884-1-vannapurve@xxxxxxxxxx/
> > Reported-by: Vishal Annapurve <vannapurve@xxxxxxxxxx>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > ---
> >  arch/x86/kvm/x86.c       | 14 ++++++++++++++
> >  include/uapi/linux/kvm.h |  1 +
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index a9f4991b3e2e..20849d2cd0e8 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:
> 
> This capability should be documented in Documentation/virtual/kvm/api.txt.
> 
> >                 r = 1;
> >                 break;
> >         case KVM_CAP_EXIT_HYPERCALL:
> > @@ -6616,6 +6617,19 @@ 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;
> > +
> 
> To avoid potentially bizarre behavior, perhaps we should disallow
> changing the APIC bus frequency once a vCPU has been created?
> 
> > +               if (!bus_frequency)
> > +                       return -EINVAL;
> > +               bus_cycle_ns = 1000000000UL / bus_frequency;
> > +               if (!bus_cycle_ns)
> > +                       return -EINVAL;
> > +               kvm->arch.apic_bus_cycle_ns = bus_cycle_ns;
> > +               kvm->arch.apic_bus_frequency = bus_frequency;
> > +               return 0;
> 
> Should this be disallowed if !lapic_in_kernel?

That makes sense. How about this?
It's difficult to check if vcpu has been created because vcpu may be destroyed.
Check if the vm has vcpus now instead.


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
+--------------------------------------
+
+: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).
+
 8. Other capabilities.
 ======================
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 20849d2cd0e8..388a9989ef7c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6626,9 +6626,25 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 		bus_cycle_ns = 1000000000UL / bus_frequency;
 		if (!bus_cycle_ns)
 			return -EINVAL;
-		kvm->arch.apic_bus_cycle_ns = bus_cycle_ns;
-		kvm->arch.apic_bus_frequency = bus_frequency;
-		return 0;
+
+		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;
+		/* This is for in-kernel vAPIC emulation. */
+		else if (!irqchip_in_kernel(kvm))
+			r = ENXIO;
+
+		if (!r) {
+			kvm->arch.apic_bus_cycle_ns = bus_cycle_ns;
+			kvm->arch.apic_bus_frequency = bus_frequency;
+		}
+		mutex_unlock(&kvm->lock);
+		return r;
 	}
 	default:
 		r = -EINVAL;



-- 
Isaku Yamahata <isaku.yamahata@xxxxxxxxxxxxxxx>