diff mbox series

[V8,1/2] KVM: selftests: Add x86_64 guest udelay() utility

Message ID ad03cb58323158c1ea14f485f834c5dfb7bf9063.1718043121.git.reinette.chatre@intel.com (mailing list archive)
State New, archived
Headers show
Series KVM: x86: Make bus clock frequency for vAPIC timer configurable | expand

Commit Message

Reinette Chatre June 10, 2024, 6:25 p.m. UTC
Create udelay() utility for x86_64 tests to match
udelay() available to ARM and RISC-V tests.

Calibrate guest frequency using the KVM_GET_TSC_KHZ ioctl()
and share it between host and guest with the new
tsc_khz global variable.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
Changes v8:
- Use appropriate signed type to discover TSC freq from KVM.
- Switch type used to store TSC frequency from unsigned long
  to unsigned int to match the type used by the kernel.

Changes v7:
- New patch
---
 .../selftests/kvm/include/x86_64/processor.h      | 15 +++++++++++++++
 .../testing/selftests/kvm/lib/x86_64/processor.c  | 12 ++++++++++++
 2 files changed, 27 insertions(+)

Comments

Sean Christopherson June 11, 2024, 12:21 a.m. UTC | #1
On Mon, Jun 10, 2024, Reinette Chatre wrote:
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 8eb57de0b587..b473f210ba6c 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -23,6 +23,7 @@
>  
>  extern bool host_cpu_is_intel;
>  extern bool host_cpu_is_amd;
> +extern unsigned int tsc_khz;
>  
>  /* Forced emulation prefix, used to invoke the emulator unconditionally. */
>  #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
> @@ -815,6 +816,20 @@ static inline void cpu_relax(void)
>  	asm volatile("rep; nop" ::: "memory");
>  }
>  
> +static inline void udelay(unsigned long usec)

uint64_t instead of unsigned long?  Practically speaking it doesn't change anything,
but I don't see any reason to mix "unsigned long" and "uint64_t", e.g. the max
delay isn't a property of the address space.

> +{
> +	unsigned long cycles = tsc_khz / 1000 * usec;
> +	uint64_t start, now;
> +
> +	start = rdtsc();
> +	for (;;) {
> +		now = rdtsc();
> +		if (now - start >= cycles)
> +			break;
> +		cpu_relax();
> +	}
> +}
> +
>  #define ud2()			\
>  	__asm__ __volatile__(	\
>  		"ud2\n"	\
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index c664e446136b..ff579674032f 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -25,6 +25,7 @@ vm_vaddr_t exception_handlers;
>  bool host_cpu_is_amd;
>  bool host_cpu_is_intel;
>  bool is_forced_emulation_enabled;
> +unsigned int tsc_khz;

Slight preference for uint32_t, mostly because KVM stores its version as a u32.

>  static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent)
>  {
> @@ -616,6 +617,8 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>  
>  void kvm_arch_vm_post_create(struct kvm_vm *vm)
>  {
> +	int r;
> +
>  	vm_create_irqchip(vm);
>  	vm_init_descriptor_tables(vm);
>  
> @@ -628,6 +631,15 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
>  
>  		vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
>  	}
> +
> +	if (kvm_has_cap(KVM_CAP_GET_TSC_KHZ)) {

I think we should make this a TEST_REQUIRE(), or maybe even a TEST_ASSERT().
Support for KVM_GET_TSC_KHZ predates KVM selftests by 7+ years.

> +		r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
> +		if (r < 0)

Heh, the docs are stale.  KVM hasn't returned an error since commit cc578287e322
("KVM: Infrastructure for software and hardware based TSC rate scaling"), which
again predates selftests by many years (6+ in this case).  To make our lives
much simpler, I think we should assert that KVM_GET_TSC_KHZ succeeds, and maybe
throw in a GUEST_ASSERT(thz_khz) in udelay()?

E.g. as is, if KVM_GET_TSC_KHZ is allowed to fail, then we risk having to deal
with weird failures due to udelay() unexpectedly doing nothing.


> +			tsc_khz = 0;
> +		else
> +			tsc_khz = r;
> +		sync_global_to_guest(vm, tsc_khz);
> +	}
>  }
>  
>  void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code)
> -- 
> 2.34.1
>
Reinette Chatre June 11, 2024, 9:33 p.m. UTC | #2
Hi Sean,

Thank you very much for your detailed feedback.

On 6/10/24 5:21 PM, Sean Christopherson wrote:
> On Mon, Jun 10, 2024, Reinette Chatre wrote:
>> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
>> index 8eb57de0b587..b473f210ba6c 100644
>> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
>> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
>> @@ -23,6 +23,7 @@
>>   
>>   extern bool host_cpu_is_intel;
>>   extern bool host_cpu_is_amd;
>> +extern unsigned int tsc_khz;
>>   
>>   /* Forced emulation prefix, used to invoke the emulator unconditionally. */
>>   #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
>> @@ -815,6 +816,20 @@ static inline void cpu_relax(void)
>>   	asm volatile("rep; nop" ::: "memory");
>>   }
>>   
>> +static inline void udelay(unsigned long usec)
> 
> uint64_t instead of unsigned long?  Practically speaking it doesn't change anything,
> but I don't see any reason to mix "unsigned long" and "uint64_t", e.g. the max
> delay isn't a property of the address space.

I assume that you refer to "cycles" below. Will do.

> 
>> +{
>> +	unsigned long cycles = tsc_khz / 1000 * usec;
>> +	uint64_t start, now;
>> +
>> +	start = rdtsc();
>> +	for (;;) {
>> +		now = rdtsc();
>> +		if (now - start >= cycles)
>> +			break;
>> +		cpu_relax();
>> +	}
>> +}
>> +
>>   #define ud2()			\
>>   	__asm__ __volatile__(	\
>>   		"ud2\n"	\
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> index c664e446136b..ff579674032f 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> @@ -25,6 +25,7 @@ vm_vaddr_t exception_handlers;
>>   bool host_cpu_is_amd;
>>   bool host_cpu_is_intel;
>>   bool is_forced_emulation_enabled;
>> +unsigned int tsc_khz;
> 
> Slight preference for uint32_t, mostly because KVM stores its version as a u32.

Changed it to uint32_t.

> 
>>   static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent)
>>   {
>> @@ -616,6 +617,8 @@ void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
>>   
>>   void kvm_arch_vm_post_create(struct kvm_vm *vm)
>>   {
>> +	int r;
>> +
>>   	vm_create_irqchip(vm);
>>   	vm_init_descriptor_tables(vm);
>>   
>> @@ -628,6 +631,15 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
>>   
>>   		vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
>>   	}
>> +
>> +	if (kvm_has_cap(KVM_CAP_GET_TSC_KHZ)) {
> 
> I think we should make this a TEST_REQUIRE(), or maybe even a TEST_ASSERT().
> Support for KVM_GET_TSC_KHZ predates KVM selftests by 7+ years.

Changed it to a TEST_ASSERT() right at the beginning of kvm_arch_vm_post_create().

> 
>> +		r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
>> +		if (r < 0)
> 
> Heh, the docs are stale.  KVM hasn't returned an error since commit cc578287e322
> ("KVM: Infrastructure for software and hardware based TSC rate scaling"), which
> again predates selftests by many years (6+ in this case).  To make our lives
> much simpler, I think we should assert that KVM_GET_TSC_KHZ succeeds, and maybe
> throw in a GUEST_ASSERT(thz_khz) in udelay()?

I added the GUEST_ASSERT() but I find that it comes with a caveat (more below).

I plan an assert as below that would end up testing the same as what a
GUEST_ASSERT(tsc_khz) would accomplish:

	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
	TEST_ASSERT(r > 0, "KVM_GET_TSC_KHZ did not provide a valid TSC freq.");
	tsc_khz = r;


Caveat is: the additional GUEST_ASSERT() requires all tests that use udelay() in
the guest to now subtly be required to implement a ucall (UCALL_ABORT) handler.
I did a crude grep check to see and of the 69 x86_64 tests there are 47 that do
indeed have a UCALL_ABORT handler. If any of the other use udelay() then the
GUEST_ASSERT() will of course still trigger, but will be quite cryptic. For
example, "Unhandled exception '0xe' at guest RIP '0x0'" vs. "tsc_khz".
  
> E.g. as is, if KVM_GET_TSC_KHZ is allowed to fail, then we risk having to deal
> with weird failures due to udelay() unexpectedly doing nothing.
> 
> 
>> +			tsc_khz = 0;
>> +		else
>> +			tsc_khz = r;
>> +		sync_global_to_guest(vm, tsc_khz);
>> +	}
>>   }
>>   
>>   void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code)
>> -- 
>> 2.34.1
>>

Reinette
Sean Christopherson June 11, 2024, 10:03 p.m. UTC | #3
On Tue, Jun 11, 2024, Reinette Chatre wrote:
> > Heh, the docs are stale.  KVM hasn't returned an error since commit cc578287e322
> > ("KVM: Infrastructure for software and hardware based TSC rate scaling"), which
> > again predates selftests by many years (6+ in this case).  To make our lives
> > much simpler, I think we should assert that KVM_GET_TSC_KHZ succeeds, and maybe
> > throw in a GUEST_ASSERT(thz_khz) in udelay()?
> 
> I added the GUEST_ASSERT() but I find that it comes with a caveat (more below).
> 
> I plan an assert as below that would end up testing the same as what a
> GUEST_ASSERT(tsc_khz) would accomplish:
> 
> 	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
> 	TEST_ASSERT(r > 0, "KVM_GET_TSC_KHZ did not provide a valid TSC freq.");
> 	tsc_khz = r;
> 
> 
> Caveat is: the additional GUEST_ASSERT() requires all tests that use udelay() in
> the guest to now subtly be required to implement a ucall (UCALL_ABORT) handler.
> I did a crude grep check to see and of the 69 x86_64 tests there are 47 that do
> indeed have a UCALL_ABORT handler. If any of the other use udelay() then the
> GUEST_ASSERT() will of course still trigger, but will be quite cryptic. For
> example, "Unhandled exception '0xe' at guest RIP '0x0'" vs. "tsc_khz".

Yeah, we really need to add a bit more infrastructure, there is way, way, waaaay
too much boilerplate needed just to run a guest and handle the basic ucalls.
Reporting guest asserts should Just Work for 99.9% of tests.

Anyways, is it any less cryptic if ucall_assert() forces a failure?  I forget if
the problem with an unhandled GUEST_ASSERT() is that the test re-enters the guest,
or if it's something else.

I don't think we need a perfect solution _now_, as tsc_khz really should never
be 0, just something to not make life completely miserable for future developers.

diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
index 42151e571953..1116bce5cdbf 100644
--- a/tools/testing/selftests/kvm/lib/ucall_common.c
+++ b/tools/testing/selftests/kvm/lib/ucall_common.c
@@ -98,6 +98,8 @@ void ucall_assert(uint64_t cmd, const char *exp, const char *file,
 
        ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
 
+       ucall_arch_do_ucall(GUEST_UCALL_FAILED);
+
        ucall_free(uc);
 }
Reinette Chatre June 11, 2024, 11:07 p.m. UTC | #4
Hi Sean,

On 6/11/24 3:03 PM, Sean Christopherson wrote:
> On Tue, Jun 11, 2024, Reinette Chatre wrote:
>>> Heh, the docs are stale.  KVM hasn't returned an error since commit cc578287e322
>>> ("KVM: Infrastructure for software and hardware based TSC rate scaling"), which
>>> again predates selftests by many years (6+ in this case).  To make our lives
>>> much simpler, I think we should assert that KVM_GET_TSC_KHZ succeeds, and maybe
>>> throw in a GUEST_ASSERT(thz_khz) in udelay()?
>>
>> I added the GUEST_ASSERT() but I find that it comes with a caveat (more below).
>>
>> I plan an assert as below that would end up testing the same as what a
>> GUEST_ASSERT(tsc_khz) would accomplish:
>>
>> 	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
>> 	TEST_ASSERT(r > 0, "KVM_GET_TSC_KHZ did not provide a valid TSC freq.");
>> 	tsc_khz = r;
>>
>>
>> Caveat is: the additional GUEST_ASSERT() requires all tests that use udelay() in
>> the guest to now subtly be required to implement a ucall (UCALL_ABORT) handler.
>> I did a crude grep check to see and of the 69 x86_64 tests there are 47 that do
>> indeed have a UCALL_ABORT handler. If any of the other use udelay() then the
>> GUEST_ASSERT() will of course still trigger, but will be quite cryptic. For
>> example, "Unhandled exception '0xe' at guest RIP '0x0'" vs. "tsc_khz".
> 
> Yeah, we really need to add a bit more infrastructure, there is way, way, waaaay
> too much boilerplate needed just to run a guest and handle the basic ucalls.
> Reporting guest asserts should Just Work for 99.9% of tests.
> 
> Anyways, is it any less cryptic if ucall_assert() forces a failure?  I forget if
> the problem with an unhandled GUEST_ASSERT() is that the test re-enters the guest,
> or if it's something else.
> 
> I don't think we need a perfect solution _now_, as tsc_khz really should never
> be 0, just something to not make life completely miserable for future developers.
> 
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index 42151e571953..1116bce5cdbf 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -98,6 +98,8 @@ void ucall_assert(uint64_t cmd, const char *exp, const char *file,
>   
>          ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
>   
> +       ucall_arch_do_ucall(GUEST_UCALL_FAILED);
> +
>          ucall_free(uc);
>   }
> 

Thank you very much.

With your suggestion an example unhandled GUEST_ASSERT() looks as below.
It does not guide on what (beyond vcpu_run()) triggered the assert but it
indeed provides a hint that adding ucall handling may be needed.

[SNIP]
==== Test Assertion Failure ====
   lib/ucall_common.c:154: addr != (void *)GUEST_UCALL_FAILED
   pid=16002 tid=16002 errno=4 - Interrupted system call
      1  0x000000000040da91: get_ucall at ucall_common.c:154
      2  0x0000000000410142: assert_on_unhandled_exception at processor.c:614
      3  0x0000000000406590: _vcpu_run at kvm_util.c:1718
      4   (inlined by) vcpu_run at kvm_util.c:1729
      5  0x00000000004026cf: test_apic_bus_clock at apic_bus_clock_test.c:115
      6   (inlined by) run_apic_bus_clock_test at apic_bus_clock_test.c:164
      7   (inlined by) main at apic_bus_clock_test.c:201
      8  0x00007fb1d8429d8f: ?? ??:0
      9  0x00007fb1d8429e3f: ?? ??:0
     10  0x00000000004027a4: _start at ??:?
   Guest failed to allocate ucall struct
[SNIP]

Is this acceptable? I can add a new preparatory patch with your
suggestion that has as its goal to provide slightly better error message
when there is an unhandled ucall.

Reinette
Sean Christopherson June 12, 2024, 1:15 a.m. UTC | #5
On Tue, Jun 11, 2024, Reinette Chatre wrote:
> > diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> > index 42151e571953..1116bce5cdbf 100644
> > --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> > +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> > @@ -98,6 +98,8 @@ void ucall_assert(uint64_t cmd, const char *exp, const char *file,
> >          ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
> > +       ucall_arch_do_ucall(GUEST_UCALL_FAILED);
> > +
> >          ucall_free(uc);
> >   }
> > 
> 
> Thank you very much.
> 
> With your suggestion an example unhandled GUEST_ASSERT() looks as below.
> It does not guide on what (beyond vcpu_run()) triggered the assert but it
> indeed provides a hint that adding ucall handling may be needed.
> 
> [SNIP]
> ==== Test Assertion Failure ====
>   lib/ucall_common.c:154: addr != (void *)GUEST_UCALL_FAILED
>   pid=16002 tid=16002 errno=4 - Interrupted system call
>      1  0x000000000040da91: get_ucall at ucall_common.c:154
>      2  0x0000000000410142: assert_on_unhandled_exception at processor.c:614
>      3  0x0000000000406590: _vcpu_run at kvm_util.c:1718
>      4   (inlined by) vcpu_run at kvm_util.c:1729
>      5  0x00000000004026cf: test_apic_bus_clock at apic_bus_clock_test.c:115
>      6   (inlined by) run_apic_bus_clock_test at apic_bus_clock_test.c:164
>      7   (inlined by) main at apic_bus_clock_test.c:201
>      8  0x00007fb1d8429d8f: ?? ??:0
>      9  0x00007fb1d8429e3f: ?? ??:0
>     10  0x00000000004027a4: _start at ??:?
>   Guest failed to allocate ucall struct

/facepalm

No, it won't work, e.g. relies on get_ucall() being invoked.  I'm also being
unnecessarily clever, and missing the obvious, simple solution.

The only reason tests manually handle UCALL_ABORT is because back when it was
added, there was no sprintf support in the guest, i.e. the guest could only spit
out raw information, it couldn't format a human-readable error message.  And so
tests manually handled UCALL_ABORT with a custom message.

When we added sprintf support, (almost) all tests moved formatting to the guest
and converged on using REPORT_GUEST_ASSERT(), but we never completed the cleanup
by moving REPORT_GUEST_ASSERT() to common code.

Even more ridiculous is that assert_on_unhandled_exception() is still a thing.
That code exists _literally_ to handle this scenario, where common guest library
code needs to signal a failure.

In short, the right way to resolve this is to have _vcpu_run() (or maybe even
__vcpu_run()) handle UCALL_ABORT.  The the bajillion REPORT_GUEST_ASSERT() calls
can be removed, as can UCALL_UNHANDLED and assert_on_unhandled_exception() since
they can and should use a normal GUEST_ASSERT() now that guest code can provide
the formating, and library code will ensure the assert is reported.

For this series, just ignore the GUEST_ASSERT() wonkiness.  If someone develops
a test that uses udelay(), doesn't handle ucalls, _and_ runs on funky hardware,
then so be it, they can come yell at me :-)

And I'll work on a series to handle UCALL_ABORT in _vcpu_run() (and poke around
a bit more to see if there's other low hanging cleanup fruit).
Reinette Chatre June 12, 2024, 5:49 p.m. UTC | #6
Hi Sean,

On 6/11/24 6:15 PM, Sean Christopherson wrote:
> On Tue, Jun 11, 2024, Reinette Chatre wrote:
>>> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
>>> index 42151e571953..1116bce5cdbf 100644
>>> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
>>> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
>>> @@ -98,6 +98,8 @@ void ucall_assert(uint64_t cmd, const char *exp, const char *file,
>>>           ucall_arch_do_ucall((vm_vaddr_t)uc->hva);
>>> +       ucall_arch_do_ucall(GUEST_UCALL_FAILED);
>>> +
>>>           ucall_free(uc);
>>>    }
>>>
>>
>> Thank you very much.
>>
>> With your suggestion an example unhandled GUEST_ASSERT() looks as below.
>> It does not guide on what (beyond vcpu_run()) triggered the assert but it
>> indeed provides a hint that adding ucall handling may be needed.
>>
>> [SNIP]
>> ==== Test Assertion Failure ====
>>    lib/ucall_common.c:154: addr != (void *)GUEST_UCALL_FAILED
>>    pid=16002 tid=16002 errno=4 - Interrupted system call
>>       1  0x000000000040da91: get_ucall at ucall_common.c:154
>>       2  0x0000000000410142: assert_on_unhandled_exception at processor.c:614
>>       3  0x0000000000406590: _vcpu_run at kvm_util.c:1718
>>       4   (inlined by) vcpu_run at kvm_util.c:1729
>>       5  0x00000000004026cf: test_apic_bus_clock at apic_bus_clock_test.c:115
>>       6   (inlined by) run_apic_bus_clock_test at apic_bus_clock_test.c:164
>>       7   (inlined by) main at apic_bus_clock_test.c:201
>>       8  0x00007fb1d8429d8f: ?? ??:0
>>       9  0x00007fb1d8429e3f: ?? ??:0
>>      10  0x00000000004027a4: _start at ??:?
>>    Guest failed to allocate ucall struct
> 
> /facepalm
> 
> No, it won't work, e.g. relies on get_ucall() being invoked.  I'm also being
> unnecessarily clever, and missing the obvious, simple solution.
> 
> The only reason tests manually handle UCALL_ABORT is because back when it was
> added, there was no sprintf support in the guest, i.e. the guest could only spit
> out raw information, it couldn't format a human-readable error message.  And so
> tests manually handled UCALL_ABORT with a custom message.
> 
> When we added sprintf support, (almost) all tests moved formatting to the guest
> and converged on using REPORT_GUEST_ASSERT(), but we never completed the cleanup
> by moving REPORT_GUEST_ASSERT() to common code.
> 
> Even more ridiculous is that assert_on_unhandled_exception() is still a thing.
> That code exists _literally_ to handle this scenario, where common guest library
> code needs to signal a failure.
> 
> In short, the right way to resolve this is to have _vcpu_run() (or maybe even
> __vcpu_run()) handle UCALL_ABORT.  The the bajillion REPORT_GUEST_ASSERT() calls
> can be removed, as can UCALL_UNHANDLED and assert_on_unhandled_exception() since
> they can and should use a normal GUEST_ASSERT() now that guest code can provide
> the formating, and library code will ensure the assert is reported.
> 
> For this series, just ignore the GUEST_ASSERT() wonkiness.  If someone develops
> a test that uses udelay(), doesn't handle ucalls, _and_ runs on funky hardware,
> then so be it, they can come yell at me :-)
> 
> And I'll work on a series to handle UCALL_ABORT in _vcpu_run() (and poke around
> a bit more to see if there's other low hanging cleanup fruit).

Thank you very much for explaining these details. Next version intends to address
all your feedback and I will send that shortly.

Reinette
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 8eb57de0b587..b473f210ba6c 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -23,6 +23,7 @@ 
 
 extern bool host_cpu_is_intel;
 extern bool host_cpu_is_amd;
+extern unsigned int tsc_khz;
 
 /* Forced emulation prefix, used to invoke the emulator unconditionally. */
 #define KVM_FEP "ud2; .byte 'k', 'v', 'm';"
@@ -815,6 +816,20 @@  static inline void cpu_relax(void)
 	asm volatile("rep; nop" ::: "memory");
 }
 
+static inline void udelay(unsigned long usec)
+{
+	unsigned long cycles = tsc_khz / 1000 * usec;
+	uint64_t start, now;
+
+	start = rdtsc();
+	for (;;) {
+		now = rdtsc();
+		if (now - start >= cycles)
+			break;
+		cpu_relax();
+	}
+}
+
 #define ud2()			\
 	__asm__ __volatile__(	\
 		"ud2\n"	\
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index c664e446136b..ff579674032f 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -25,6 +25,7 @@  vm_vaddr_t exception_handlers;
 bool host_cpu_is_amd;
 bool host_cpu_is_intel;
 bool is_forced_emulation_enabled;
+unsigned int tsc_khz;
 
 static void regs_dump(FILE *stream, struct kvm_regs *regs, uint8_t indent)
 {
@@ -616,6 +617,8 @@  void assert_on_unhandled_exception(struct kvm_vcpu *vcpu)
 
 void kvm_arch_vm_post_create(struct kvm_vm *vm)
 {
+	int r;
+
 	vm_create_irqchip(vm);
 	vm_init_descriptor_tables(vm);
 
@@ -628,6 +631,15 @@  void kvm_arch_vm_post_create(struct kvm_vm *vm)
 
 		vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
 	}
+
+	if (kvm_has_cap(KVM_CAP_GET_TSC_KHZ)) {
+		r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
+		if (r < 0)
+			tsc_khz = 0;
+		else
+			tsc_khz = r;
+		sync_global_to_guest(vm, tsc_khz);
+	}
 }
 
 void vcpu_arch_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code)