+KVM and Paolo Please keep the mailing list Cc'd so that other can participate in the conversation, and so that the mails are archived. Adding Paolo, who I believe is still not subscribed to kvm@ :-) On Tue, Sep 26, 2023, Phil Dennis-Jordan wrote: > Hi Sean, > > Thanks for taking a look at this. > > On Mon, Sep 25, 2023 at 5:18 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Sat, Sep 23, 2023, Phil Dennis-Jordan wrote: > > > +#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) > > That sounds sensible, but those symbols are #defined in headers that > are currently only routinely available on Linux systems. I know this > is the *KVM* unit test suite, but it's definitely also very useful > elsewhere. > (My specific motivation for the patch is that on macOS/HVF, Qemu's > software APIC implementation is used. The current implementation for > that turns out to be a massive perf bottleneck and very much > not-spec-compliant, so I've been improving it. This test suite has > been very useful for testing/verifying my work, but this IPI test had > to be disabled.) Ah, right. > A few options I can think of: > > 1. I notice x86/hyperv.h has a bunch of similar #defines. We could > collect only the KVM-related x86 constants and helper functions we > actually need in a corresponding x86/kvm.h. I see there's a kvmclock.h > for the KVM clock already - that could either be renamed and expanded > in scope, or left separate. kvmclock.h is essentially #2, it's a copy of arch/x86/include/asm/pvclock-abi.h from the kernel. > 2. Bring a copy of the necessary KVM uapi header file(s) into the > repo, slightly hacked up to cut down on transitive dependencies. It > looks like lib/linux/*.h might already be similar instances for other > Linux bits. Qemu also does this. This has my vote, though I'd strongly prefer not to strip out anything unless it's absolutely necessary to get KUT to compile. Grabbing entire files should make it easier to maintain the copy+pasted code as future updates to re-sync will hopefully add just the new features. The attached half-baked patch adds everything except the base "is this KVM?" check and has only been compile tested on x86, feel free to use it as a starting point (I wanted to get the basic gist compiling to make sure I wasn't leading you completely astray) > > > + && (c.a >= 0x40000001 || c.a == 0); > > > > Why allow 0? Though I think we probably forego this check entirely. > > "Note also that old hosts set eax value to 0x0. This should be > interpreted as if the value was 0x40000001. " according to > https://www.kernel.org/doc/html/v5.7/virt/kvm/cpuid.html > Though I suppose "old hosts" are probably VERY old and probably don't > expose the IPI hypercall… Ha, so your the one that reads the documentation ;-) > > > @@ -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)) > > Makes sense, especially so we don't have to keep rechecking whether > we're on KVM in case we end up testing for more KVM feature flags at > some point. > > > 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. > > Probably overkill as long as we only have this one instance? Yeah, probably.
