Re: [kvm-unit-tests PATCH] x86/apic: Gates test_pv_ipi on KVM cpuid, not test device

[Sean Christopherson <seanjc@google.com>]

On Sat, Sep 23, 2023, Phil Dennis-Jordan wrote:
> This changes the test for the KVM IPI hypercall API to be skipped if the
> relevant cpuid feature bit is not set or if the KVM cpuid leaf is
> missing, rather than the presence of the test device. The latter is an
> unreliable inference on non-KVM platforms.
> 
> It also adds a skip report when these tests are skipped.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>  lib/x86/processor.h | 19 +++++++++++++++++++
>  x86/apic.c          |  9 ++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 44f4fd1e..9a4c0d26 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -284,6 +284,13 @@ static inline bool is_intel(void)
>  #define X86_FEATURE_VNMI		(CPUID(0x8000000A, 0, EDX, 25))
>  #define	X86_FEATURE_AMD_PMU_V2		(CPUID(0x80000022, 0, EAX, 0))
>  
> +/*
> + * Hypervisor specific leaves (KVM, ...)
> + * See:
> + * https://kernel.org/doc/html/latest/virt/kvm/x86/cpuid.html
> + */
> +#define	X86_KVM_FEATURE_PV_SEND_IPI  (CPUID(0x40000001, 0, EAX, 11))

We could actually define this using the uapi headers, then there's no need to
reference the kernel docs, e.g.

#define		X86_FEATURE_KVM_PV_SEND_IPI (CPUID(KVM_CPUID_FEATURES, 0, EAX, KVM_FEATURE_PV_SEND_IPI)

> +
>  static inline bool this_cpu_has(u64 feature)
>  {
>  	u32 input_eax = feature >> 32;
> @@ -299,6 +306,18 @@ static inline bool this_cpu_has(u64 feature)
>  	return ((*(tmp + (output_reg % 32))) & (1 << bit));
>  }
>  
> +static inline bool kvm_feature_flags_supported(void)
> +{
> +	struct cpuid c;
> +
> +	c = cpuid_indexed(0x40000000, 0);
> +	return
> +		c.b == 0x4b4d564b
> +		&& c.c == 0x564b4d56
> +		&& c.d == 0x4d

I would much prefer to provide something similar to the kernel's hypervisor_cpuid_base(),
and then use KVM_SIGNATURE to match the signature.  And assert that KVM is placed
at its default base for tests that require KVM paravirt features, i.e. disallow
relocating KVM to e.g. 0x40000100 to make room for Hyper-V.

Something like this (completely untested)

static inline u32 get_hypervisor_cpuid_base(const char *sig)
{
	u32 base, signature[3];

	if (!this_cpu_has(X86_FEATURE_HYPERVISOR))
		return 0;

	for (base = 0x40000000; base < 0x40010000; base += 0x100) {
		cpuid(base, &eax, &signature[0], &signature[1], &signature[2]);

		if (!memcmp(sig, signature, 12))
			return base;
	}

	return 0;
}

static inline bool is_hypervisor_kvm(void)
{
	u32 base = get_hypervisor_cpuid_base(KVM_SIGNATURE);

	if (!base)
		return 0;

	/*
	 * Require that KVM be placed at its default base so that macros can be
	 * used to query individual KVM feature bits.
	 */
	TEST_ASSERT(base == KVM_CPUID_SIGNATURE);
	return true;
}

> +		&& (c.a >= 0x40000001 || c.a == 0);

Why allow 0?  Though I think we probably forego this check entirely.

> +}
> +
>  struct far_pointer32 {
>  	u32 offset;
>  	u16 selector;
> diff --git a/x86/apic.c b/x86/apic.c
> index dd7e7834..525e08fd 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -30,6 +30,11 @@ static bool is_xapic_enabled(void)
>  	return (rdmsr(MSR_IA32_APICBASE) & (APIC_EN | APIC_EXTD)) == APIC_EN;
>  }
>  
> +static bool is_kvm_ipi_hypercall_supported(void)
> +{
> +	return kvm_feature_flags_supported() && this_cpu_has(X86_KVM_FEATURE_PV_SEND_IPI);
> +}
> +
>  static void test_lapic_existence(void)
>  {
>  	u8 version;
> @@ -658,8 +663,10 @@ static void test_pv_ipi(void)
>  	int ret;
>  	unsigned long a0 = 0xFFFFFFFF, a1 = 0, a2 = 0xFFFFFFFF, a3 = 0x0;
>  
> -	if (!test_device_enabled())
> +	if (!is_kvm_ipi_hypercall_supported()) {

I would rather open code the two independent checks, e.g.

	if (!is_hypervisor_kvm() || !this_cpu_has(X86_FEATURE_KVM_PV_SEND_IPI))

Or alternatively, provide a generic helper in processor.h to handle the hypervisor
check, e.g.

  static inline this_cpu_has_kvm_feature(...)

Though if we go that route it probably makes sense to play nice with relocating
the base since it would be quite easy to do so.

> +		report_skip("PV IPIs testing (No KVM IPI hypercall flag in cpuid)");
>  		return;
> +	}
>  
>  	asm volatile("vmcall" : "=a"(ret) :"a"(KVM_HC_SEND_IPI), "b"(a0), "c"(a1), "d"(a2), "S"(a3));
>  	report(!ret, "PV IPIs testing");
> -- 
> 2.36.1
>