[PATCH 0/2] KVM: X86: Make bus lock frequency for vapic timer configurable

[PATCH 0/2] KVM: X86: Make bus lock frequency for vapic timer configurable

[isaku.yamahata]

From: Isaku Yamahata <isaku.yamahata@intel.com>

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
frequency.  When using this capability, the user space VMM should configure
CPUID[0x15] to advertise the frequency.

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
causes the vAPIC timer to fire earlier than the guest expects. [1] The KVM
APIC timer emulation uses hrtimer, whose unit is nanosecond.

There are options to reconcile the mismatch.  1) Make apic bus clock frequency
configurable (this patch).  2) TDX KVM code adjusts TMICT value.  This is hacky
and it results in losing MSB bits from 32 bit width to 30 bit width.  3). Make
the guest kernel use tsc deadline timer instead of acpi oneshot/periodic timer.
This is guest kernel choice.  It's out of control of VMM.

[1] https://lore.kernel.org/lkml/20231006011255.4163884-1-vannapurve@google.com/

Isaku Yamahata (2):
  KVM: x86: Make the hardcoded APIC bus frequency vm variable
  KVM: X86: Add a capability to configure bus frequency for APIC timer

 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/hyperv.c           |  2 +-
 arch/x86/kvm/lapic.c            |  6 ++++--
 arch/x86/kvm/lapic.h            |  4 ++--
 arch/x86/kvm/x86.c              | 16 ++++++++++++++++
 include/uapi/linux/kvm.h        |  1 +
 6 files changed, 26 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/2] KVM: x86: Make the hardcoded APIC bus frequency vm variable

[isaku.yamahata]

From: Isaku Yamahata <isaku.yamahata@intel.com>

TDX virtualizes the advertised APIC bus frequency to be 25MHz.  The KVM
hardcodedes it to be 1GHz.  This mismatch causes the vAPIC timer to fire
earlier than the TDX guest expects.  In order to reconcile this mismatch,
make the frequency configurable for the user space VMM.  As the first step,
Replace the constants with the VM value in struct kvm.

Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 2 ++
 arch/x86/kvm/hyperv.c           | 2 +-
 arch/x86/kvm/lapic.c            | 6 ++++--
 arch/x86/kvm/lapic.h            | 4 ++--
 arch/x86/kvm/x86.c              | 2 ++
 5 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d7036982332e..f2b1c6b3fb11 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1334,6 +1334,8 @@ struct kvm_arch {
 
 	u32 default_tsc_khz;
 	bool user_set_tsc;
+	u64 apic_bus_cycle_ns;
+	u64 apic_bus_frequency;
 
 	seqcount_raw_spinlock_t pvclock_sc;
 	bool use_master_clock;
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 238afd7335e4..995ce2c74ce0 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -1687,7 +1687,7 @@ static int kvm_hv_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
 		data = (u64)vcpu->arch.virtual_tsc_khz * 1000;
 		break;
 	case HV_X64_MSR_APIC_FREQUENCY:
-		data = APIC_BUS_FREQUENCY;
+		data = vcpu->kvm->arch.apic_bus_frequency;
 		break;
 	default:
 		kvm_pr_unimpl_rdmsr(vcpu, msr);
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 245b20973cae..73956b0ac1f1 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1542,7 +1542,8 @@ static u32 apic_get_tmcct(struct kvm_lapic *apic)
 		remaining = 0;
 
 	ns = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period);
-	return div64_u64(ns, (APIC_BUS_CYCLE_NS * apic->divide_count));
+	return div64_u64(ns, (apic->vcpu->kvm->arch.apic_bus_cycle_ns *
+			      apic->divide_count));
 }
 
 static void __report_tpr_access(struct kvm_lapic *apic, bool write)
@@ -1960,7 +1961,8 @@ static void start_sw_tscdeadline(struct kvm_lapic *apic)
 
 static inline u64 tmict_to_ns(struct kvm_lapic *apic, u32 tmict)
 {
-	return (u64)tmict * APIC_BUS_CYCLE_NS * (u64)apic->divide_count;
+	return (u64)tmict * apic->vcpu->kvm->arch.apic_bus_cycle_ns *
+		(u64)apic->divide_count;
 }
 
 static void update_target_expiration(struct kvm_lapic *apic, uint32_t old_divisor)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 0a0ea4b5dd8c..3a425ea2a515 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -16,8 +16,8 @@
 #define APIC_DEST_NOSHORT		0x0
 #define APIC_DEST_MASK			0x800
 
-#define APIC_BUS_CYCLE_NS       1
-#define APIC_BUS_FREQUENCY      (1000000000ULL / APIC_BUS_CYCLE_NS)
+#define APIC_BUS_CYCLE_NS_DEFAULT	1
+#define APIC_BUS_FREQUENCY_DEFAULT	(1000000000ULL / APIC_BUS_CYCLE_NS_DEFAULT)
 
 #define APIC_BROADCAST			0xFF
 #define X2APIC_BROADCAST		0xFFFFFFFFul
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2c924075f6f1..a9f4991b3e2e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12466,6 +12466,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);
 
 	kvm->arch.default_tsc_khz = max_tsc_khz ? : tsc_khz;
+	kvm->arch.apic_bus_cycle_ns = APIC_BUS_CYCLE_NS_DEFAULT;
+	kvm->arch.apic_bus_frequency = APIC_BUS_FREQUENCY_DEFAULT;
 	kvm->arch.guest_can_read_msr_platform_info = true;
 	kvm->arch.enable_pmu = enable_pmu;
 
-- 
2.25.1

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

[isaku.yamahata]

From: Isaku Yamahata <isaku.yamahata@intel.com>

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
frequency.  When using this capability, the user space VMM should configure
CPUID[0x15] to advertise the frequency.

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
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@google.com/
Reported-by: Vishal Annapurve <vannapurve@google.com>
Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
---
 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:
 		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;
+
+		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;
+	}
 	default:
 		r = -EINVAL;
 		break;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 211b86de35ac..d74a057df173 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1201,6 +1201,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE 228
 #define KVM_CAP_ARM_SUPPORTED_BLOCK_SIZES 229
 #define KVM_CAP_ARM_SUPPORTED_REG_MASK_RANGES 230
+#define KVM_CAP_X86_BUS_FREQUENCY_CONTROL 231
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.25.1

KVM: X86: Make bus clock frequency for vapic timer (bus lock -> bus clock) (was Re: [PATCH 0/2] KVM: X86: Make bus lock frequency for vapic timer) configurable

[Isaku Yamahata]

I meant bus clock frequency, not bus lock. Sorry for typo.

On Tue, Nov 07, 2023 at 11:22:32AM -0800,
isaku.yamahata@intel.com wrote:

> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> 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
> frequency.  When using this capability, the user space VMM should configure
> CPUID[0x15] to advertise the frequency.
> 
> 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
> causes the vAPIC timer to fire earlier than the guest expects. [1] The KVM
> APIC timer emulation uses hrtimer, whose unit is nanosecond.
> 
> There are options to reconcile the mismatch.  1) Make apic bus clock frequency
> configurable (this patch).  2) TDX KVM code adjusts TMICT value.  This is hacky
> and it results in losing MSB bits from 32 bit width to 30 bit width.  3). Make
> the guest kernel use tsc deadline timer instead of acpi oneshot/periodic timer.
> This is guest kernel choice.  It's out of control of VMM.
> 
> [1] https://lore.kernel.org/lkml/20231006011255.4163884-1-vannapurve@google.com/
> 
> Isaku Yamahata (2):
>   KVM: x86: Make the hardcoded APIC bus frequency vm variable
>   KVM: X86: Add a capability to configure bus frequency for APIC timer
> 
>  arch/x86/include/asm/kvm_host.h |  2 ++
>  arch/x86/kvm/hyperv.c           |  2 +-
>  arch/x86/kvm/lapic.c            |  6 ++++--
>  arch/x86/kvm/lapic.h            |  4 ++--
>  arch/x86/kvm/x86.c              | 16 ++++++++++++++++
>  include/uapi/linux/kvm.h        |  1 +
>  6 files changed, 26 insertions(+), 5 deletions(-)
> 
> -- 
> 2.25.1
> 

-- 
Isaku Yamahata <isaku.yamahata@linux.intel.com>

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

[Jim Mattson]

On Tue, Nov 7, 2023 at 11:24 AM <isaku.yamahata@intel.com> wrote:
>
> From: Isaku Yamahata <isaku.yamahata@intel.com>
>
> 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.

> 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@google.com/
> Reported-by: Vishal Annapurve <vannapurve@google.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  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?

> +       }
>         default:
>                 r = -EINVAL;
>                 break;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 211b86de35ac..d74a057df173 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1201,6 +1201,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE 228
>  #define KVM_CAP_ARM_SUPPORTED_BLOCK_SIZES 229
>  #define KVM_CAP_ARM_SUPPORTED_REG_MASK_RANGES 230
> +#define KVM_CAP_X86_BUS_FREQUENCY_CONTROL 231
>
>  #ifdef KVM_CAP_IRQ_ROUTING
>
> --
> 2.25.1
>
>

Re: KVM: X86: Make bus clock frequency for vapic timer (bus lock -> bus clock) (was Re: [PATCH 0/2] KVM: X86: Make bus lock frequency for vapic timer) configurable

[Jim Mattson]

On Tue, Nov 7, 2023 at 11:29 AM Isaku Yamahata
<isaku.yamahata@linux.intel.com> wrote:
>
> I meant bus clock frequency, not bus lock. Sorry for typo.
>
> On Tue, Nov 07, 2023 at 11:22:32AM -0800,
> isaku.yamahata@intel.com wrote:
>
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> >
> > 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
> > frequency.  When using this capability, the user space VMM should configure
> > CPUID[0x15] to advertise the frequency.
> >
> > 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
> > causes the vAPIC timer to fire earlier than the guest expects. [1] The KVM
> > APIC timer emulation uses hrtimer, whose unit is nanosecond.
> >
> > There are options to reconcile the mismatch.  1) Make apic bus clock frequency
> > configurable (this patch).  2) TDX KVM code adjusts TMICT value.  This is hacky
> > and it results in losing MSB bits from 32 bit width to 30 bit width.  3). Make
> > the guest kernel use tsc deadline timer instead of acpi oneshot/periodic timer.
> > This is guest kernel choice.  It's out of control of VMM.
> >
> > [1] https://lore.kernel.org/lkml/20231006011255.4163884-1-vannapurve@google.com/
> >
> > Isaku Yamahata (2):
> >   KVM: x86: Make the hardcoded APIC bus frequency vm variable
> >   KVM: X86: Add a capability to configure bus frequency for APIC timer

I think I know the answer, but do you have any tests for this new feature?

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@google.com> wrote:

> On Tue, Nov 7, 2023 at 11:24 AM <isaku.yamahata@intel.com> wrote:
> >
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> >
> > 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@google.com/
> > Reported-by: Vishal Annapurve <vannapurve@google.com>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  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@linux.intel.com>

Re: KVM: X86: Make bus clock frequency for vapic timer (bus lock -> bus clock) (was Re: [PATCH 0/2] KVM: X86: Make bus lock frequency for vapic timer) configurable

[Isaku Yamahata]

On Tue, Nov 07, 2023 at 12:03:35PM -0800,
Jim Mattson <jmattson@google.com> wrote:

> On Tue, Nov 7, 2023 at 11:29 AM Isaku Yamahata
> <isaku.yamahata@linux.intel.com> wrote:
> >
> > I meant bus clock frequency, not bus lock. Sorry for typo.
> >
> > On Tue, Nov 07, 2023 at 11:22:32AM -0800,
> > isaku.yamahata@intel.com wrote:
> >
> > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > >
> > > 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
> > > frequency.  When using this capability, the user space VMM should configure
> > > CPUID[0x15] to advertise the frequency.
> > >
> > > 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
> > > causes the vAPIC timer to fire earlier than the guest expects. [1] The KVM
> > > APIC timer emulation uses hrtimer, whose unit is nanosecond.
> > >
> > > There are options to reconcile the mismatch.  1) Make apic bus clock frequency
> > > configurable (this patch).  2) TDX KVM code adjusts TMICT value.  This is hacky
> > > and it results in losing MSB bits from 32 bit width to 30 bit width.  3). Make
> > > the guest kernel use tsc deadline timer instead of acpi oneshot/periodic timer.
> > > This is guest kernel choice.  It's out of control of VMM.
> > >
> > > [1] https://lore.kernel.org/lkml/20231006011255.4163884-1-vannapurve@google.com/
> > >
> > > Isaku Yamahata (2):
> > >   KVM: x86: Make the hardcoded APIC bus frequency vm variable
> > >   KVM: X86: Add a capability to configure bus frequency for APIC timer
> 
> I think I know the answer, but do you have any tests for this new feature?

If you mean kvm kselftest, no.
I have
- TDX patched qemu
- kvm-unit-tests: test_apic_timer_one_shot() @ kvm-unit-tests/x86/apic.c
  TDX version is found at https://github.com/intel/kvm-unit-tests-tdx
  We're planning to upstream the changes for TDX

How far do we want to go?
- Run kvm-unit-tests with TDX. What I have right now.
- kvm-unit-tests: extend qemu for default VM case and update
  test_apic_timer_one_host()
- kselftest
  Right now kvm kselftest doesn't have test cases even for in-kernel IRQCHIP
  creation.
  - Add test cases to create/destory in-kernel IRQCHIP
  - Add KVM_ENABLE_CAPABILITY(KVM_CAP_X86_BUS_FREQUENCY_CONTROL) test cases
  - Test if apic timer frequency is as spcified.

-- 
Isaku Yamahata <isaku.yamahata@linux.intel.com>

Re: KVM: X86: Make bus clock frequency for vapic timer (bus lock -> bus clock) (was Re: [PATCH 0/2] KVM: X86: Make bus lock frequency for vapic timer) configurable

[Sean Christopherson]

On Wed, Nov 08, 2023, Isaku Yamahata wrote:
> On Tue, Nov 07, 2023 at 12:03:35PM -0800, Jim Mattson <jmattson@google.com> wrote:
> > I think I know the answer, but do you have any tests for this new feature?
> 
> If you mean kvm kselftest, no.
> I have
> - TDX patched qemu
> - kvm-unit-tests: test_apic_timer_one_shot() @ kvm-unit-tests/x86/apic.c
>   TDX version is found at https://github.com/intel/kvm-unit-tests-tdx
>   We're planning to upstream the changes for TDX
> 
> How far do we want to go?
> - Run kvm-unit-tests with TDX. What I have right now.
> - kvm-unit-tests: extend qemu for default VM case and update
>   test_apic_timer_one_host()

Hrm, I'm not sure that we can do a whole lot for test_apic_timer_one_shot().  Or
rather, I'm not sure it's worth the effort to try and add coverage beyond what's
already there.

As for TDX, *if* we extend KUT, please don't make it depend on TDX.  Very few people
have access to TDX platforms and anything CoCo is pretty much guaranteed to be harder
to debug.

> - kselftest
>   Right now kvm kselftest doesn't have test cases even for in-kernel IRQCHIP
>   creation.

Selftests always create an in-kernel APIC.  And I think selftests are perfectly
suited to complement the coverage provided by KUT.  Specifically, the failure
scenario for this is that KVM emulates at 1Ghz whereas TDX advertises 25Mhz, i.e.
the test case we want is to verify that the APIC timer doesn't expire early.

There's no need for any APIC infrastructure, e.g. a selftest doesn't even need to
handle an interrupt.  Get the TSC frequency from KVM, program up an arbitrary APIC
bus clock frequency, set TMICT such that it expires waaaay in the future, and then
verify that the APIC timer counts reasonably close to the programmed frequency.
E.g. if the test sets the bus clock to 25Mhz, the "drift" due to KVM counting at
1Ghz should be super obvious.

LOL, side topic, KUT has this:

	/*
	 * For LVT Timer clock, SDM vol 3 10.5.4 says it should be
	 * derived from processor's bus clock (IIUC which is the same  <======
	 * as TSC), however QEMU seems to be using nanosecond. In all
	 * cases, the following should satisfy on all modern
	 * processors.
	 */
	report((lvtt_counter == 1) && (tsc2 - tsc1 >= interval),
	       "APIC LVT timer one shot");

Re: KVM: X86: Make bus clock frequency for vapic timer (bus lock -> bus clock) (was Re: [PATCH 0/2] KVM: X86: Make bus lock frequency for vapic timer) configurable

[Isaku Yamahata]

On Thu, Nov 09, 2023 at 07:55:45AM -0800,
Sean Christopherson <seanjc@google.com> wrote:

> On Wed, Nov 08, 2023, Isaku Yamahata wrote:
> > On Tue, Nov 07, 2023 at 12:03:35PM -0800, Jim Mattson <jmattson@google.com> wrote:
> > > I think I know the answer, but do you have any tests for this new feature?
> > 
> > If you mean kvm kselftest, no.
> > I have
> > - TDX patched qemu
> > - kvm-unit-tests: test_apic_timer_one_shot() @ kvm-unit-tests/x86/apic.c
> >   TDX version is found at https://github.com/intel/kvm-unit-tests-tdx
> >   We're planning to upstream the changes for TDX
> > 
> > How far do we want to go?
> > - Run kvm-unit-tests with TDX. What I have right now.
> > - kvm-unit-tests: extend qemu for default VM case and update
> >   test_apic_timer_one_host()
> 
> Hrm, I'm not sure that we can do a whole lot for test_apic_timer_one_shot().  Or
> rather, I'm not sure it's worth the effort to try and add coverage beyond what's
> already there.
> 
> As for TDX, *if* we extend KUT, please don't make it depend on TDX.  Very few people
> have access to TDX platforms and anything CoCo is pretty much guaranteed to be harder
> to debug.

It made the test cases work with TDX + UEFI bios by adjusting command line to
invoke qemu.  And skip unsuitable tests.
Maybe we can generalize the way to twist qemu command line.


> > - kselftest
> >   Right now kvm kselftest doesn't have test cases even for in-kernel IRQCHIP
> >   creation.
> 
> Selftests always create an in-kernel APIC.  And I think selftests are perfectly
> suited to complement the coverage provided by KUT.  Specifically, the failure
> scenario for this is that KVM emulates at 1Ghz whereas TDX advertises 25Mhz, i.e.
> the test case we want is to verify that the APIC timer doesn't expire early.
> 
> There's no need for any APIC infrastructure, e.g. a selftest doesn't even need to
> handle an interrupt.  Get the TSC frequency from KVM, program up an arbitrary APIC
> bus clock frequency, set TMICT such that it expires waaaay in the future, and then
> verify that the APIC timer counts reasonably close to the programmed frequency.
> E.g. if the test sets the bus clock to 25Mhz, the "drift" due to KVM counting at
> 1Ghz should be super obvious.

Oh, only check the register value without interrupt. Good idea.  Let me give it
a try.
-- 
Isaku Yamahata <isaku.yamahata@linux.intel.com>

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

[kernel test robot]

Hi,

kernel test robot noticed the following build errors:

[auto build test ERROR on kvm/queue]
[also build test ERROR on linus/master next-20231109]
[cannot apply to mst-vhost/linux-next kvm/linux-next v6.6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/isaku-yamahata-intel-com/KVM-x86-Make-the-hardcoded-APIC-bus-frequency-vm-variable/20231108-032736
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link:    https://lore.kernel.org/r/70c2a2277f57b804c715c5b4b4aa0b3561ed6a4f.1699383993.git.isaku.yamahata%40intel.com
patch subject: [PATCH 2/2] KVM: X86: Add a capability to configure bus frequency for APIC timer
config: i386-buildonly-randconfig-002-20231109 (https://download.01.org/0day-ci/archive/20231110/202311100209.zIaZqZhg-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231110/202311100209.zIaZqZhg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <yujie.liu@intel.com>
| Closes: https://lore.kernel.org/r/202311100209.zIaZqZhg-lkp@intel.com/

All errors (this is a 32-bit build, new ones prefixed by >>):

   ld: arch/x86/kvm/x86.o: in function `kvm_vm_ioctl_enable_cap':
>> x86.c:(.text+0x1265b): undefined reference to `__udivdi3'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Sean Christopherson]

On Fri, Nov 10, 2023, kernel test robot wrote:
> Hi,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on kvm/queue]
> [also build test ERROR on linus/master next-20231109]
> [cannot apply to mst-vhost/linux-next kvm/linux-next v6.6]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/isaku-yamahata-intel-com/KVM-x86-Make-the-hardcoded-APIC-bus-frequency-vm-variable/20231108-032736
> base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
> patch link:    https://lore.kernel.org/r/70c2a2277f57b804c715c5b4b4aa0b3561ed6a4f.1699383993.git.isaku.yamahata%40intel.com
> patch subject: [PATCH 2/2] KVM: X86: Add a capability to configure bus frequency for APIC timer
> config: i386-buildonly-randconfig-002-20231109 (https://download.01.org/0day-ci/archive/20231110/202311100209.zIaZqZhg-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231110/202311100209.zIaZqZhg-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <yujie.liu@intel.com>
> | Closes: https://lore.kernel.org/r/202311100209.zIaZqZhg-lkp@intel.com/
> 
> All errors (this is a 32-bit build, new ones prefixed by >>):
> 
>    ld: arch/x86/kvm/x86.o: in function `kvm_vm_ioctl_enable_cap':
> >> x86.c:(.text+0x1265b): undefined reference to `__udivdi3'

Heh, this inscrutable error is due to 64-bit division on 32-bit kernels.

	u64 bus_frequency = cap->args[0];
	u64 bus_cycle_ns;

	if (!bus_frequency)
		return -EINVAL;

	bus_cycle_ns = 1000000000UL / bus_frequency;  <========

I don't see any reason to allow 64-bit values, e.g. Intel's CPUID 0x15 only
supports a 32-bit frequency in Hz.  I.e. just truncate it to a u32.