Message-ID: <a76a430a-2a9d-4ff6-88c2-bdfafff92ae4@intel.com>
Date: Wed, 24 Apr 2024 14:07:28 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>
CC: "jmattson@...gle.com" <jmattson@...gle.com>, "Gao, Chao"
	<chao.gao@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "vkuznets@...hat.com" <vkuznets@...hat.com>,
	"Annapurve, Vishal" <vannapurve@...gle.com>, "Li, Xiaoyao"
	<xiaoyao.li@...el.com>, "Aktas, Erdem" <erdemaktas@...gle.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "mlevitsk@...hat.com"
	<mlevitsk@...hat.com>, "Yamahata, Isaku" <isaku.yamahata@...el.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, Yuan Yao <yuan.yao@...el.com>
Subject: Re: [PATCH V4 0/4] KVM: x86: Make bus clock frequency for vAPIC timer
 configurable

(+Yao Yuan)

Hi Sean and Rick,

On 4/24/2024 9:38 AM, Edgecombe, Rick P wrote:
> On Wed, 2024-04-24 at 09:13 -0700, Sean Christopherson wrote:
>> On Tue, Apr 16, 2024, Rick P Edgecombe wrote:
>>> On Thu, 2024-03-21 at 09:37 -0700, Reinette Chatre wrote:
>>>>
>>>> Summary
>>>> -------
>>>> Add KVM_CAP_X86_APIC_BUS_FREQUENCY capability to configure the APIC
>>>> bus clock frequency for APIC timer emulation.
>>>> Allow KVM_ENABLE_CAPABILITY(KVM_CAP_X86_APIC_BUS_FREQUENCY) to set the
>>>> frequency in nanoseconds. When using this capability, the user space
>>>> VMM should configure CPUID leaf 0x15 to advertise the frequency.
>>>
>>> Looks good to me and...
>>> Tested-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
>>>
>>> The only thing missing is actually integrating it into TDX qemu patches and
>>> testing the resulting TD. I think we are making a fair assumption that the
>>> problem should be resolved based on the analysis, but we have not actually
>>> tested that part. Is that right?
>>
>> Please tell me that Rick is wrong, and that this actually has been tested with
>> a TDX guest.  I don't care _who_ tested it, or with what VMM it has been
>> tested,
>> but _someone_ needs to verify that this actually fixes the TDX issue.
> 
> It is in the process of getting a TDX test developed (or rather updated).
> Agreed, it requires verification that it fixes the original TDX issue. That is
> why I raised it.
> 
> Reinette was working on this internally and some iterations were happening, but
> we are trying to work on the public list as much as possible per your earlier
> comments. So that is why she posted it.
> 
> There was at least some level of TDX integration in the past. I'm not sure what
> exactly was tested, but we are going to re-verify it with the latest everything.

Apologies for the delay. I am the one needing to do this testing and it took me a while
to ramp up on all the parts (and I am still learning).

I encountered quite the roadblock (for me) along the way that was caused by a lingering
timer (presumably left by TDVF). Thank you so much to Isaku and Yao Yuan for helping me
to root cause this. I believe that this is unique to the kvm-unit-tests that does
not reset the environment like the OS.

A modified x86/apic.c:test_apic_timer_one_shot() was used to test this feature. Below I
provide the diff of essential parts against
https://github.com/intel/kvm-unit-tests-tdx/blob/tdx/x86/apic.c for your reference. With
these modifications it can be confirmed that the test within a TD fails without the work
in this series, and passes with it. This was tested against a host kernel running a
snapshot of the ongoing KVM TDX work and corresponding QEMU changes (including a QEMU
change that enables the new capability introduced in this series).

Below are the core changes made to the existing APIC test. The two major changes are:
(a) stop any lingering timers before the test starts, (b) use CPUID 0x15 in TDX to
accurately determine the TSC and APIC frequencies instead of making 1GHz assumption
and use similar check as the kselftest test introduced in this series (I did have to
increase the amount with which the frequency is allowed to deviate by 1% in my testing).

Please note that there are some more changes needed to run this test in TDX since all
APIC tests are not appropriate for TDX. This snippet was used in my testing and I
will work with kvm-unit-test folks on the next steps to have it integrated.


@@ -477,11 +478,29 @@ static void lvtt_handler(isr_regs_t *regs)
 
 static void test_apic_timer_one_shot(void)
 {
-	uint64_t tsc1, tsc2;
 	static const uint32_t interval = 0x10000;
+	uint64_t measured_apic_freq, tsc2, tsc1;
+	uint32_t tsc_freq = 0, apic_freq = 0;
+	struct cpuid cpuid_tsc = {};
 
 #define APIC_LVT_TIMER_VECTOR    (0xee)
 
+	/*
+	 * CPUID 0x15 is not available in VMX, can use it to obtain
+	 * TSC and APIC frequency for accurate testing
+	 */
+	if (is_tdx_guest()) {
+		cpuid_tsc = raw_cpuid(0x15, 0);
+		tsc_freq = cpuid_tsc.c * cpuid_tsc.b / cpuid_tsc.a;
+		apic_freq = cpuid_tsc.c;
+	}
+	/*
+	   stop already fired local timer
+	   the test case can be negative failure if the timer fired
+	   after installed lvtt_handler but *before*
+	   write to TIMICT again.
+	 */
+	apic_write(APIC_TMICT, 0);
 	handle_irq(APIC_LVT_TIMER_VECTOR, lvtt_handler);
 
 	/* One shot mode */
@@ -503,8 +522,16 @@ static void test_apic_timer_one_shot(void)
 	 * cases, the following should satisfy on all modern
 	 * processors.
 	 */
-	report((lvtt_counter == 1) && (tsc2 - tsc1 >= interval),
-	       "APIC LVT timer one shot");
+	if (is_tdx_guest()) {
+		measured_apic_freq = interval * (tsc_freq / (tsc2 - tsc1));
+		report((lvtt_counter == 1) &&
+		       (measured_apic_freq < apic_freq * 102 / 100) &&
+		       (measured_apic_freq > apic_freq * 98 / 100),
+		       "APIC LVT timer one shot");
+	} else {
+		report((lvtt_counter == 1) && (tsc2 - tsc1 >= interval),
+		"APIC LVT timer one shot");
+	}
 }
 

Reinette

Powered by blists - more mailing lists