[v2,1/3] KVM: x86: Make the hardcoded APIC bus frequency vm variable

Commit Message

Isaku Yamahata Nov. 14, 2023, 4:35 a.m. UTC

From: Isaku Yamahata <[e-mail]>

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 <[e-mail]>
---
Changes v2:
- no change
---
 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(-)

Comments

Maxim Levitsky Dec. 13, 2023, 10:39 p.m. UTC | #1

On Mon, 2023-11-13 at 20:35 -0800, [e-mail] wrote:
> From: Isaku Yamahata <[e-mail]>
> 
> TDX virtualizes the advertised APIC bus frequency to be 25MHz. 

Can you explain a bit better why TDX needs this? I am not familiar
with TDX well enough yet to fully understand.

AFAIK, the guest writes the TMICT, that makes the KVM set up a HR timer,
and KVM is free to use any apic frequency to determine the deadline of that timer,
and then once the HR timer fires, KVM injects an interrupt to the guest.

Are some parts of this process overridden by the TDX?

I am sure that there is a good reason to do this, but I would be very happy
to see a detailed explanation in the changelog for future readers who
might know nothing about TDX.


>  The KVM
> hardcodedes it to be 1GHz.  This mismatch causes the vAPIC timer to fire
> earlier than the TDX guest expects.

Here too, what do you mean by "TDX guest expects"? Is the APIC frequency
given to the guest using some TDX specific way like HV_X64_MSR_APIC_FREQUENCY? 

>   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 <[e-mail]>
> ---
> Changes v2:
> - no change
> ---
>  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;
>  

Only one minor nitpick: We might not need 'apic_bus_frequency' and instead have
it calculated from apic_bus_cycle_ns? (to have single source of truth)

Frequency is only used by HV_X64_MSR_APIC_FREQUENCY, and I don't think that HyperV guests read
this MSR often, nor that a division will make a dent in the emulation time of this msr,
even if they do.

But if you prefer, I won't mind either.

Best regards,
	Maxim Levitsky

Sean Christopherson Dec. 13, 2023, 11:10 p.m. UTC | #2

On Thu, Dec 14, 2023, Maxim Levitsky wrote:
> On Mon, 2023-11-13 at 20:35 -0800, [e-mail] wrote:
> > From: Isaku Yamahata <[e-mail]>
> > 
> > TDX virtualizes the advertised APIC bus frequency to be 25MHz. 
> 
> Can you explain a bit better why TDX needs this? I am not familiar
> with TDX well enough yet to fully understand.

TDX (the module/architecture) hardcodes the core crystal frequency to 25Mhz,
whereas KVM hardcodes the APIC bus frequency to 1Ghz.  And TDX (again, the module)
*unconditionally* enumerates CPUID 0x15 to TDX guests, i.e. _tells_ the guest that
the frequency is 25MHz regardless of what the VMM/hypervisor actually emulates.
And so the guest skips calibrating the APIC timer, which results in the guest
scheduling timer interrupts waaaaaaay too frequently, i.e. the guest ends up
gettings interrupts at 40x the rate it wants.

Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
the TDX guest core crystal frequency of 25Mhz.

I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
dying on since the KVM changes are relatively simple.

https://lore.kernel.org/all/[e-mail]

Jim Mattson Dec. 13, 2023, 11:18 p.m. UTC | #3

On Wed, Dec 13, 2023 at 3:10 PM Sean Christopherson <[e-mail]> wrote:
>
> On Thu, Dec 14, 2023, Maxim Levitsky wrote:
> > On Mon, 2023-11-13 at 20:35 -0800, [e-mail] wrote:
> > > From: Isaku Yamahata <[e-mail]>
> > >
> > > TDX virtualizes the advertised APIC bus frequency to be 25MHz.
> >
> > Can you explain a bit better why TDX needs this? I am not familiar
> > with TDX well enough yet to fully understand.
>
> TDX (the module/architecture) hardcodes the core crystal frequency to 25Mhz,
> whereas KVM hardcodes the APIC bus frequency to 1Ghz.  And TDX (again, the module)
> *unconditionally* enumerates CPUID 0x15 to TDX guests, i.e. _tells_ the guest that
> the frequency is 25MHz regardless of what the VMM/hypervisor actually emulates.
> And so the guest skips calibrating the APIC timer, which results in the guest
> scheduling timer interrupts waaaaaaay too frequently, i.e. the guest ends up
> gettings interrupts at 40x the rate it wants.
>
> Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
> for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
> CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
> needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
> the TDX guest core crystal frequency of 25Mhz.

Aside from placating a broken guest infrastructure that ignores a
17-year old contract between KVM and its guests, what are the
advantages to supporting a range of APIC bus frequencies?

> I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
> dying on since the KVM changes are relatively simple.

Not making the KVM changes is even simpler. :)

Maxim Levitsky Dec. 14, 2023, 9:31 a.m. UTC | #4

On Wed, 2023-12-13 at 15:10 -0800, Sean Christopherson wrote:
> On Thu, Dec 14, 2023, Maxim Levitsky wrote:
> > On Mon, 2023-11-13 at 20:35 -0800, [e-mail] wrote:
> > > From: Isaku Yamahata <[e-mail]>
> > > 
> > > TDX virtualizes the advertised APIC bus frequency to be 25MHz. 
> > 
> > Can you explain a bit better why TDX needs this? I am not familiar
> > with TDX well enough yet to fully understand.
> 
> TDX (the module/architecture) hardcodes the core crystal frequency to 25Mhz,
> whereas KVM hardcodes the APIC bus frequency to 1Ghz.  And TDX (again, the module)
> *unconditionally* enumerates CPUID 0x15 to TDX guests, i.e. _tells_ the guest that
> the frequency is 25MHz regardless of what the VMM/hypervisor actually emulates.
> And so the guest skips calibrating the APIC timer, which results in the guest
> scheduling timer interrupts waaaaaaay too frequently, i.e. the guest ends up
> gettings interrupts at 40x the rate it wants.

That is what I wanted to hear without opening the PRM ;) - so there is a CPUID leaf,
but KVM just doesn't advertise it. Now it makes sense.

Please add something like that to the commit message:

"TDX guests have the APIC bus frequency hardcoded to 25 Mhz in the CPUID leaf 0x15.
KVM doesn't expose this leaf, but TDX mandates it to be exposed,
and doesn't allow to override it's value either.

To ensure that the guest doesn't have a conflicting view of the APIC bus frequency, 
allow the userspace to tell KVM to use the same frequency that TDX mandates,
instead of the default 1Ghz"

> 
> Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
> for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
> CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
> needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
> the TDX guest core crystal frequency of 25Mhz.

I assume that TDX doesn't allow to change the CPUID 0x15 leaf.

> 
> I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
> dying on since the KVM changes are relatively simple.
> 
> https://lore.kernel.org/all/[e-mail]
> 

Best regards,
	Maxim Levitsky

Sean Christopherson Dec. 14, 2023, 4:41 p.m. UTC | #5

On Thu, Dec 14, 2023, Maxim Levitsky wrote:
> On Wed, 2023-12-13 at 15:10 -0800, Sean Christopherson wrote:
> > Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
> > for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
> > CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
> > needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
> > the TDX guest core crystal frequency of 25Mhz.
> 
> I assume that TDX doesn't allow to change the CPUID 0x15 leaf.

Correct.  I meant to call that out below, but left my sentence half-finished.  It
was supposed to say:

  I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
  use 1Ghz as the base frequency or to allow configuring the base frequency
  advertised to the guest.

> > I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> > use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
> > dying on since the KVM changes are relatively simple.
> > 
> > https://lore.kernel.org/all/[e-mail]
> > 
> 
> Best regards,
> 	Maxim Levitsky
>

Isaku Yamahata Dec. 19, 2023, 1:40 a.m. UTC | #6

On Thu, Dec 14, 2023 at 08:41:43AM -0800,
Sean Christopherson <[e-mail]> wrote:

> On Thu, Dec 14, 2023, Maxim Levitsky wrote:
> > On Wed, 2023-12-13 at 15:10 -0800, Sean Christopherson wrote:
> > > Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
> > > for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
> > > CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
> > > needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
> > > the TDX guest core crystal frequency of 25Mhz.
> > 
> > I assume that TDX doesn't allow to change the CPUID 0x15 leaf.
> 
> Correct.  I meant to call that out below, but left my sentence half-finished.  It
> was supposed to say:
> 
>   I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
>   use 1Ghz as the base frequency or to allow configuring the base frequency
>   advertised to the guest.
> 
> > > I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> > > use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
> > > dying on since the KVM changes are relatively simple.
> > > 
> > > https://lore.kernel.org/all/[e-mail]
> > > 
> > 
> > Best regards,
> > 	Maxim Levitsky

The followings are the updated version of the commit message.


KVM: x86: Make the hardcoded APIC bus frequency VM variable

The TDX architecture hard-codes the APIC bus frequency to 25MHz in the
CPUID leaf 0x15.  The
TDX mandates it to be exposed and doesn't allow the VMM to override
its value.  The KVM APIC timer emulation hard-codes the frequency to
1GHz.  It doesn't unconditionally enumerate it to the guest unless the
user space VMM sets the CPUID leaf 0x15 by KVM_SET_CPUID.

If the CPUID leaf 0x15 is enumerated, the guest kernel uses it as the
APIC bus frequency.  If not, the guest kernel measures the frequency
based on other known timers like the ACPI timer or the legacy PIT.
The TDX guest kernel gets timer interrupt more times by 1GHz / 25MHz.

To ensure that the guest doesn't have a conflicting view of the APIC
bus frequency, allow the userspace to tell KVM to use the same
frequency that TDX mandates instead of the default 1Ghz.

There are several options to address this.
1. Make the KVM able to configure APIC bus frequency (This patch).
   Pros: It resembles the existing hardware.  The recent Intel CPUs
   adapts 25MHz.
   Cons: Require the VMM to emulate the APIC timer at 25MHz.
2. Make the TDX architecture enumerate CPUID 0x15 to configurable
   frequency or not enumerate it.
   Pros: Any APIC bus frequency is allowed.
   Cons: Deviation from the real hardware.
3. Make the TDX guest kernel use 1GHz when it's running on KVM.
   Cons: The kernel ignores CPUID leaf 0x15.

Jim Mattson Dec. 19, 2023, 3:53 a.m. UTC | #7

On Mon, Dec 18, 2023 at 5:40 PM Isaku Yamahata
<[e-mail]> wrote:
>
> On Thu, Dec 14, 2023 at 08:41:43AM -0800,
> Sean Christopherson <[e-mail]> wrote:
>
> > On Thu, Dec 14, 2023, Maxim Levitsky wrote:
> > > On Wed, 2023-12-13 at 15:10 -0800, Sean Christopherson wrote:
> > > > Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
> > > > for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
> > > > CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
> > > > needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
> > > > the TDX guest core crystal frequency of 25Mhz.
> > >
> > > I assume that TDX doesn't allow to change the CPUID 0x15 leaf.
> >
> > Correct.  I meant to call that out below, but left my sentence half-finished.  It
> > was supposed to say:
> >
> >   I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> >   use 1Ghz as the base frequency or to allow configuring the base frequency
> >   advertised to the guest.
> >
> > > > I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> > > > use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
> > > > dying on since the KVM changes are relatively simple.
> > > >
> > > > https://lore.kernel.org/all/[e-mail]
> > > >
> > >
> > > Best regards,
> > >     Maxim Levitsky
>
> The followings are the updated version of the commit message.
>
>
> KVM: x86: Make the hardcoded APIC bus frequency VM variable
>
> The TDX architecture hard-codes the APIC bus frequency to 25MHz in the
> CPUID leaf 0x15.  The
> TDX mandates it to be exposed and doesn't allow the VMM to override
> its value.  The KVM APIC timer emulation hard-codes the frequency to
> 1GHz.  It doesn't unconditionally enumerate it to the guest unless the
> user space VMM sets the CPUID leaf 0x15 by KVM_SET_CPUID.
>
> If the CPUID leaf 0x15 is enumerated, the guest kernel uses it as the
> APIC bus frequency.  If not, the guest kernel measures the frequency
> based on other known timers like the ACPI timer or the legacy PIT.
> The TDX guest kernel gets timer interrupt more times by 1GHz / 25MHz.
>
> To ensure that the guest doesn't have a conflicting view of the APIC
> bus frequency, allow the userspace to tell KVM to use the same
> frequency that TDX mandates instead of the default 1Ghz.
>
> There are several options to address this.
> 1. Make the KVM able to configure APIC bus frequency (This patch).
>    Pros: It resembles the existing hardware.  The recent Intel CPUs
>    adapts 25MHz.
>    Cons: Require the VMM to emulate the APIC timer at 25MHz.
> 2. Make the TDX architecture enumerate CPUID 0x15 to configurable
>    frequency or not enumerate it.
>    Pros: Any APIC bus frequency is allowed.
>    Cons: Deviation from the real hardware.
> 3. Make the TDX guest kernel use 1GHz when it's running on KVM.
>    Cons: The kernel ignores CPUID leaf 0x15.

4. Change CPUID.15H under TDX to report the crystal clock frequency as 1 GHz.
Pro: This has been the virtual APIC frequency for KVM guests for 13 years.
Pro: This requires changing only one hard-coded constant in TDX.

I see no compelling reason to complicate KVM with support for
configurable APIC frequencies, and I see no advantages to doing so.

Xiaoyao Li Dec. 19, 2023, 7:56 a.m. UTC | #8

On 12/19/2023 11:53 AM, Jim Mattson wrote:
> On Mon, Dec 18, 2023 at 5:40 PM Isaku Yamahata
> <[e-mail]> wrote:
>>
>> On Thu, Dec 14, 2023 at 08:41:43AM -0800,
>> Sean Christopherson <[e-mail]> wrote:
>>
>>> On Thu, Dec 14, 2023, Maxim Levitsky wrote:
>>>> On Wed, 2023-12-13 at 15:10 -0800, Sean Christopherson wrote:
>>>>> Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
>>>>> for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
>>>>> CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
>>>>> needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
>>>>> the TDX guest core crystal frequency of 25Mhz.
>>>>
>>>> I assume that TDX doesn't allow to change the CPUID 0x15 leaf.
>>>
>>> Correct.  I meant to call that out below, but left my sentence half-finished.  It
>>> was supposed to say:
>>>
>>>    I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
>>>    use 1Ghz as the base frequency or to allow configuring the base frequency
>>>    advertised to the guest.
>>>
>>>>> I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
>>>>> use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
>>>>> dying on since the KVM changes are relatively simple.
>>>>>
>>>>> https://lore.kernel.org/all/[e-mail]
>>>>>
>>>>
>>>> Best regards,
>>>>      Maxim Levitsky
>>
>> The followings are the updated version of the commit message.
>>
>>
>> KVM: x86: Make the hardcoded APIC bus frequency VM variable
>>
>> The TDX architecture hard-codes the APIC bus frequency to 25MHz in the
>> CPUID leaf 0x15.  The
>> TDX mandates it to be exposed and doesn't allow the VMM to override
>> its value.  The KVM APIC timer emulation hard-codes the frequency to
>> 1GHz.  It doesn't unconditionally enumerate it to the guest unless the
>> user space VMM sets the CPUID leaf 0x15 by KVM_SET_CPUID.
>>
>> If the CPUID leaf 0x15 is enumerated, the guest kernel uses it as the
>> APIC bus frequency.  If not, the guest kernel measures the frequency
>> based on other known timers like the ACPI timer or the legacy PIT.
>> The TDX guest kernel gets timer interrupt more times by 1GHz / 25MHz.
>>
>> To ensure that the guest doesn't have a conflicting view of the APIC
>> bus frequency, allow the userspace to tell KVM to use the same
>> frequency that TDX mandates instead of the default 1Ghz.
>>
>> There are several options to address this.
>> 1. Make the KVM able to configure APIC bus frequency (This patch).
>>     Pros: It resembles the existing hardware.  The recent Intel CPUs
>>     adapts 25MHz.
>>     Cons: Require the VMM to emulate the APIC timer at 25MHz.
>> 2. Make the TDX architecture enumerate CPUID 0x15 to configurable
>>     frequency or not enumerate it.
>>     Pros: Any APIC bus frequency is allowed.
>>     Cons: Deviation from the real hardware.
>> 3. Make the TDX guest kernel use 1GHz when it's running on KVM.
>>     Cons: The kernel ignores CPUID leaf 0x15.
> 
> 4. Change CPUID.15H under TDX to report the crystal clock frequency as 1 GHz.

This will have an impact on TSC frequency. Core crystal clock frequency 
is also used to calculate TSC frequency.

> Pro: This has been the virtual APIC frequency for KVM guests for 13 years.
> Pro: This requires changing only one hard-coded constant in TDX.
> 
> I see no compelling reason to complicate KVM with support for
> configurable APIC frequencies, and I see no advantages to doing so.

I'm wondering what's the attitude of KVM community to provide support 
CPUID leaf 0x15? Even KVM decides to never advertise CPUID 0x15 in 
GET_SUPPORTED_CPUID, hard-coded APIC frequency puts additional 
limitation when userspace want to emualte CPUID 0x15

Isaku Yamahata Dec. 19, 2023, 8:11 a.m. UTC | #9

On Mon, Dec 18, 2023 at 07:53:45PM -0800,
Jim Mattson <[e-mail]> wrote:

> On Mon, Dec 18, 2023 at 5:40 PM Isaku Yamahata
> <[e-mail]> wrote:
> >
> > On Thu, Dec 14, 2023 at 08:41:43AM -0800,
> > Sean Christopherson <[e-mail]> wrote:
> >
> > > On Thu, Dec 14, 2023, Maxim Levitsky wrote:
> > > > On Wed, 2023-12-13 at 15:10 -0800, Sean Christopherson wrote:
> > > > > Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
> > > > > for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
> > > > > CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
> > > > > needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
> > > > > the TDX guest core crystal frequency of 25Mhz.
> > > >
> > > > I assume that TDX doesn't allow to change the CPUID 0x15 leaf.
> > >
> > > Correct.  I meant to call that out below, but left my sentence half-finished.  It
> > > was supposed to say:
> > >
> > >   I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> > >   use 1Ghz as the base frequency or to allow configuring the base frequency
> > >   advertised to the guest.
> > >
> > > > > I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> > > > > use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
> > > > > dying on since the KVM changes are relatively simple.
> > > > >
> > > > > https://lore.kernel.org/all/[e-mail]
> > > > >
> > > >
> > > > Best regards,
> > > >     Maxim Levitsky
> >
> > The followings are the updated version of the commit message.
> >
> >
> > KVM: x86: Make the hardcoded APIC bus frequency VM variable
> >
> > The TDX architecture hard-codes the APIC bus frequency to 25MHz in the
> > CPUID leaf 0x15.  The
> > TDX mandates it to be exposed and doesn't allow the VMM to override
> > its value.  The KVM APIC timer emulation hard-codes the frequency to
> > 1GHz.  It doesn't unconditionally enumerate it to the guest unless the
> > user space VMM sets the CPUID leaf 0x15 by KVM_SET_CPUID.
> >
> > If the CPUID leaf 0x15 is enumerated, the guest kernel uses it as the
> > APIC bus frequency.  If not, the guest kernel measures the frequency
> > based on other known timers like the ACPI timer or the legacy PIT.
> > The TDX guest kernel gets timer interrupt more times by 1GHz / 25MHz.
> >
> > To ensure that the guest doesn't have a conflicting view of the APIC
> > bus frequency, allow the userspace to tell KVM to use the same
> > frequency that TDX mandates instead of the default 1Ghz.
> >
> > There are several options to address this.
> > 1. Make the KVM able to configure APIC bus frequency (This patch).
> >    Pros: It resembles the existing hardware.  The recent Intel CPUs
> >    adapts 25MHz.
> >    Cons: Require the VMM to emulate the APIC timer at 25MHz.
> > 2. Make the TDX architecture enumerate CPUID 0x15 to configurable
> >    frequency or not enumerate it.
> >    Pros: Any APIC bus frequency is allowed.
> >    Cons: Deviation from the real hardware.
> > 3. Make the TDX guest kernel use 1GHz when it's running on KVM.
> >    Cons: The kernel ignores CPUID leaf 0x15.
> 
> 4. Change CPUID.15H under TDX to report the crystal clock frequency as 1 GHz.
> Pro: This has been the virtual APIC frequency for KVM guests for 13 years.
> Pro: This requires changing only one hard-coded constant in TDX.
> 
> I see no compelling reason to complicate KVM with support for
> configurable APIC frequencies, and I see no advantages to doing so.

Because TDX isn't specific to KVM, it should work with other VMM technologies.
If we'd like to go for this route, the frequency would be configurable.  What
frequency should be acceptable securely is obscure.  25MHz has long history with
the real hardware.

Sean Christopherson Dec. 20, 2023, 10:07 p.m. UTC | #10

On Tue, Dec 19, 2023, Isaku Yamahata wrote:
> On Mon, Dec 18, 2023 at 07:53:45PM -0800, Jim Mattson <[e-mail]> wrote:
> > > There are several options to address this.
> > > 1. Make the KVM able to configure APIC bus frequency (This patch).
> > >    Pros: It resembles the existing hardware.  The recent Intel CPUs
> > >    adapts 25MHz.
> > >    Cons: Require the VMM to emulate the APIC timer at 25MHz.
> > > 2. Make the TDX architecture enumerate CPUID 0x15 to configurable
> > >    frequency or not enumerate it.
> > >    Pros: Any APIC bus frequency is allowed.
> > >    Cons: Deviation from the real hardware.

I don't buy this as a valid Con.  TDX is one gigantic deviation from real hardware,
and since TDX obviously can't guarantee the APIC timer is emulated at the correct
frequency, there can't possibly be any security benefits.  If this were truly a
Con that anyone cared about, we would have gotten patches to "fix" KVM a long time
ago.

If the TDX module wasn't effectively hardware-defined software, i.e. was actually
able to adapt at the speed of software, then fixing this in TDX would be a complete
no-brainer.

The KVM uAPI required to play nice is relatively minor, so I'm not totally opposed
to adding it.  But I totally agree with Jim that forcing KVM to change 13+ years
of behavior just because someone at Intel decided that 25MHz was a good number is
ridiculous.

> > > 3. Make the TDX guest kernel use 1GHz when it's running on KVM.
> > >    Cons: The kernel ignores CPUID leaf 0x15.
> > 
> > 4. Change CPUID.15H under TDX to report the crystal clock frequency as 1 GHz.
> > Pro: This has been the virtual APIC frequency for KVM guests for 13 years.
> > Pro: This requires changing only one hard-coded constant in TDX.
> > 
> > I see no compelling reason to complicate KVM with support for
> > configurable APIC frequencies, and I see no advantages to doing so.
> 
> Because TDX isn't specific to KVM, it should work with other VMM technologies.
> If we'd like to go for this route, the frequency would be configurable.  What
> frequency should be acceptable securely is obscure.  25MHz has long history with
> the real hardware.

Jim Mattson Dec. 20, 2023, 10:22 p.m. UTC | #11

On Wed, Dec 20, 2023 at 2:07 PM Sean Christopherson <[e-mail]> wrote:
>
> On Tue, Dec 19, 2023, Isaku Yamahata wrote:
> > On Mon, Dec 18, 2023 at 07:53:45PM -0800, Jim Mattson <[e-mail]> wrote:
> > > > There are several options to address this.
> > > > 1. Make the KVM able to configure APIC bus frequency (This patch).
> > > >    Pros: It resembles the existing hardware.  The recent Intel CPUs
> > > >    adapts 25MHz.
> > > >    Cons: Require the VMM to emulate the APIC timer at 25MHz.
> > > > 2. Make the TDX architecture enumerate CPUID 0x15 to configurable
> > > >    frequency or not enumerate it.
> > > >    Pros: Any APIC bus frequency is allowed.
> > > >    Cons: Deviation from the real hardware.
>
> I don't buy this as a valid Con.  TDX is one gigantic deviation from real hardware,
> and since TDX obviously can't guarantee the APIC timer is emulated at the correct
> frequency, there can't possibly be any security benefits.  If this were truly a
> Con that anyone cared about, we would have gotten patches to "fix" KVM a long time
> ago.
>
> If the TDX module wasn't effectively hardware-defined software, i.e. was actually
> able to adapt at the speed of software, then fixing this in TDX would be a complete
> no-brainer.
>
> The KVM uAPI required to play nice is relatively minor, so I'm not totally opposed
> to adding it.  But I totally agree with Jim that forcing KVM to change 13+ years
> of behavior just because someone at Intel decided that 25MHz was a good number is
> ridiculous.
>
> > > > 3. Make the TDX guest kernel use 1GHz when it's running on KVM.
> > > >    Cons: The kernel ignores CPUID leaf 0x15.
> > >
> > > 4. Change CPUID.15H under TDX to report the crystal clock frequency as 1 GHz.
> > > Pro: This has been the virtual APIC frequency for KVM guests for 13 years.
> > > Pro: This requires changing only one hard-coded constant in TDX.
> > >
> > > I see no compelling reason to complicate KVM with support for
> > > configurable APIC frequencies, and I see no advantages to doing so.
> >
> > Because TDX isn't specific to KVM, it should work with other VMM technologies.
> > If we'd like to go for this route, the frequency would be configurable.  What
> > frequency should be acceptable securely is obscure.  25MHz has long history with
> > the real hardware.

I am curious how many other hypervisors either offer a configurable
APIC frequency or happened to also land on 25 MHz.

Xiaoyao Li Dec. 21, 2023, 5:44 a.m. UTC | #12

On 12/21/2023 6:07 AM, Sean Christopherson wrote:
> On Tue, Dec 19, 2023, Isaku Yamahata wrote:
>> On Mon, Dec 18, 2023 at 07:53:45PM -0800, Jim Mattson <[e-mail]> wrote:
>>>> There are several options to address this.
>>>> 1. Make the KVM able to configure APIC bus frequency (This patch).
>>>>     Pros: It resembles the existing hardware.  The recent Intel CPUs
>>>>     adapts 25MHz.
>>>>     Cons: Require the VMM to emulate the APIC timer at 25MHz.
>>>> 2. Make the TDX architecture enumerate CPUID 0x15 to configurable
>>>>     frequency or not enumerate it.
>>>>     Pros: Any APIC bus frequency is allowed.
>>>>     Cons: Deviation from the real hardware.
> 
> I don't buy this as a valid Con.  TDX is one gigantic deviation from real hardware,
> and since TDX obviously can't guarantee the APIC timer is emulated at the correct
> frequency, there can't possibly be any security benefits.  If this were truly a
> Con that anyone cared about, we would have gotten patches to "fix" KVM a long time
> ago.
> 
> If the TDX module wasn't effectively hardware-defined software, i.e. was actually
> able to adapt at the speed of software, then fixing this in TDX would be a complete
> no-brainer.
> 
> The KVM uAPI required to play nice is relatively minor, so I'm not totally opposed
> to adding it.  But I totally agree with Jim that forcing KVM to change 13+ years
> of behavior just because someone at Intel decided that 25MHz was a good number is
> ridiculous.

I believe 25MHz was chosen because it's the value from hardware that 
supports TDX and it is not going to change for the following known 
generations that support TDX.

It's mainly the core crystal frequency. Yes, it also represents the APIC 
frequency when it's enumerated in CPUID 0x15. However, it also relates 
other things, like intel-pt MTC Freq. If it is configured to other value 
different from hardware, I think it will break the correctness of 
INTEL-PT MTC packets in TDs.

>>>> 3. Make the TDX guest kernel use 1GHz when it's running on KVM.
>>>>     Cons: The kernel ignores CPUID leaf 0x15.
>>>
>>> 4. Change CPUID.15H under TDX to report the crystal clock frequency as 1 GHz.
>>> Pro: This has been the virtual APIC frequency for KVM guests for 13 years.
>>> Pro: This requires changing only one hard-coded constant in TDX.
>>>
>>> I see no compelling reason to complicate KVM with support for
>>> configurable APIC frequencies, and I see no advantages to doing so.
>>
>> Because TDX isn't specific to KVM, it should work with other VMM technologies.
>> If we'd like to go for this route, the frequency would be configurable.  What
>> frequency should be acceptable securely is obscure.  25MHz has long history with
>> the real hardware.
>

Jim Mattson Dec. 21, 2023, 2:39 p.m. UTC | #13

On Wed, Dec 20, 2023 at 9:44 PM Xiaoyao Li <[e-mail]> wrote:
>
> On 12/21/2023 6:07 AM, Sean Christopherson wrote:
> > On Tue, Dec 19, 2023, Isaku Yamahata wrote:
> >> On Mon, Dec 18, 2023 at 07:53:45PM -0800, Jim Mattson <[e-mail]> wrote:
> >>>> There are several options to address this.
> >>>> 1. Make the KVM able to configure APIC bus frequency (This patch).
> >>>>     Pros: It resembles the existing hardware.  The recent Intel CPUs
> >>>>     adapts 25MHz.
> >>>>     Cons: Require the VMM to emulate the APIC timer at 25MHz.
> >>>> 2. Make the TDX architecture enumerate CPUID 0x15 to configurable
> >>>>     frequency or not enumerate it.
> >>>>     Pros: Any APIC bus frequency is allowed.
> >>>>     Cons: Deviation from the real hardware.
> >
> > I don't buy this as a valid Con.  TDX is one gigantic deviation from real hardware,
> > and since TDX obviously can't guarantee the APIC timer is emulated at the correct
> > frequency, there can't possibly be any security benefits.  If this were truly a
> > Con that anyone cared about, we would have gotten patches to "fix" KVM a long time
> > ago.
> >
> > If the TDX module wasn't effectively hardware-defined software, i.e. was actually
> > able to adapt at the speed of software, then fixing this in TDX would be a complete
> > no-brainer.
> >
> > The KVM uAPI required to play nice is relatively minor, so I'm not totally opposed
> > to adding it.  But I totally agree with Jim that forcing KVM to change 13+ years
> > of behavior just because someone at Intel decided that 25MHz was a good number is
> > ridiculous.
>
> I believe 25MHz was chosen because it's the value from hardware that
> supports TDX and it is not going to change for the following known
> generations that support TDX.
>
> It's mainly the core crystal frequency. Yes, it also represents the APIC
> frequency when it's enumerated in CPUID 0x15. However, it also relates
> other things, like intel-pt MTC Freq. If it is configured to other value
> different from hardware, I think it will break the correctness of
> INTEL-PT MTC packets in TDs.

LOL! That suggests that no one is really using KVM's Intel PT virtualization.

This is certainly a compelling reason for having a variable frequency
virtual APIC. Thank you!

> >>>> 3. Make the TDX guest kernel use 1GHz when it's running on KVM.
> >>>>     Cons: The kernel ignores CPUID leaf 0x15.
> >>>
> >>> 4. Change CPUID.15H under TDX to report the crystal clock frequency as 1 GHz.
> >>> Pro: This has been the virtual APIC frequency for KVM guests for 13 years.
> >>> Pro: This requires changing only one hard-coded constant in TDX.
> >>>
> >>> I see no compelling reason to complicate KVM with support for
> >>> configurable APIC frequencies, and I see no advantages to doing so.
> >>
> >> Because TDX isn't specific to KVM, it should work with other VMM technologies.
> >> If we'd like to go for this route, the frequency would be configurable.  What
> >> frequency should be acceptable securely is obscure.  25MHz has long history with
> >> the real hardware.
> >
>

Maxim Levitsky Dec. 21, 2023, 5:01 p.m. UTC | #14

On Mon, 2023-12-18 at 17:40 -0800, Isaku Yamahata wrote:
> On Thu, Dec 14, 2023 at 08:41:43AM -0800,
> Sean Christopherson <[e-mail]> wrote:
> 
> > On Thu, Dec 14, 2023, Maxim Levitsky wrote:
> > > On Wed, 2023-12-13 at 15:10 -0800, Sean Christopherson wrote:
> > > > Upstream KVM's non-TDX behavior is fine, because KVM doesn't advertise support
> > > > for CPUID 0x15, i.e. doesn't announce to host userspace that it's safe to expose
> > > > CPUID 0x15 to the guest.  Because TDX makes exposing CPUID 0x15 mandatory, KVM
> > > > needs to be taught to correctly emulate the guest's APIC bus frequency, a.k.a.
> > > > the TDX guest core crystal frequency of 25Mhz.
> > > 
> > > I assume that TDX doesn't allow to change the CPUID 0x15 leaf.
> > 
> > Correct.  I meant to call that out below, but left my sentence half-finished.  It
> > was supposed to say:
> > 
> >   I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> >   use 1Ghz as the base frequency or to allow configuring the base frequency
> >   advertised to the guest.
> > 
> > > > I halfheartedly floated the idea of "fixing" the TDX module/architecture to either
> > > > use 1Ghz as the base frequency (off list), but it definitely isn't a hill worth
> > > > dying on since the KVM changes are relatively simple.
> > > > 
> > > > https://lore.kernel.org/all/[e-mail]
> > > > 
> > > 
> > > Best regards,
> > > 	Maxim Levitsky
> 
> The followings are the updated version of the commit message.
> 
> 
> KVM: x86: Make the hardcoded APIC bus frequency VM variable
> 
> The TDX architecture hard-codes the APIC bus frequency to 25MHz in the
> CPUID leaf 0x15.  The
> TDX mandates it to be exposed and doesn't allow the VMM to override
> its value.  The KVM APIC timer emulation hard-codes the frequency to
> 1GHz.  It doesn't unconditionally enumerate it to the guest unless the
> user space VMM sets the CPUID leaf 0x15 by KVM_SET_CPUID.
> 
> If the CPUID leaf 0x15 is enumerated, the guest kernel uses it as the
> APIC bus frequency.  If not, the guest kernel measures the frequency
> based on other known timers like the ACPI timer or the legacy PIT.
> The TDX guest kernel gets timer interrupt more times by 1GHz / 25MHz.
> 
> To ensure that the guest doesn't have a conflicting view of the APIC
> bus frequency, allow the userspace to tell KVM to use the same
> frequency that TDX mandates instead of the default 1Ghz.

Looks great!

In theory this gives me an idea that KVM could parse the guest CPUID leaf
0x15 and deduce the frequency from it automatically instead of a new capability,
but I understand that this is (also in theory) not backward compatible assuming
that some hypervisors already expose this leaf for some reason,
thus a new capability will be needed anyway.

Thus I have no more complaints, and thanks for addressing my feedback!

Best regards,
	Maxim Levitsky

> 
> There are several options to address this.
> 1. Make the KVM able to configure APIC bus frequency (This patch).
>    Pros: It resembles the existing hardware.  The recent Intel CPUs
>    adapts 25MHz.
>    Cons: Require the VMM to emulate the APIC timer at 25MHz.
> 2. Make the TDX architecture enumerate CPUID 0x15 to configurable
>    frequency or not enumerate it.
>    Pros: Any APIC bus frequency is allowed.
>    Cons: Deviation from the real hardware.
> 3. Make the TDX guest kernel use 1GHz when it's running on KVM.
>    Cons: The kernel ignores CPUID leaf 0x15.
> 
>

Patch

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;