I've now got around to testing this patch on Linux/KVM.
The thing I was worried about, my <linux/types.h> wrapper, appears to
work exactly as intended, everything builds nicely on Linux. So that
aspect of the patch is now a matter of taste.
However, I *have* run into a minor snag with this proposed change: the
X86_FEATURE_KVM_PV_SEND_IPI bit isn't actually enabled by default. It
looks like it needs to be specified explicitly as a +kvm-pv-ipi flag
in the -cpu option on the Qemu command line. KVM itself still handles
the IPI hypercall either way, as there's another flag you'd have to
opt into for only handling advertised hypercalls.
I think the cleanest way to fix this is probably to add +kvm-pv-ipi to
the apic-split, x2apic, and xapic test suites in x86/unittests.cfg and
keep the strict feature flag check in the test code. As I understand
it, Qemu will filter the feature bit if the underlying KVM
implementation doesn't support it, so that would appear to give us the
best compatibility, except perhaps for Qemu versions that predate this
flag, which will presumably fail to run the test suites altogether.
Alternatively, we could simply check whether we're running on KVM and
skip the feature bit check entirely - it certainly wouldn't make any
additional assumptions; as of right now, the master branch already
assumes we're running on KVM *and* the KVM implementation supports the
IPI HC. Dropping the feature bit check doesn't make the patch tangibly
smaller though.
I'll wait a couple of days for any other suggestions or objections,
and in the absence of such I'll roll your draft patch, my
modifications, and the x86/unittests.cfg tweaks into a v2 patch and
re-submit. (And I'll tag it with you, Sean, as Co-authored-by:)
Thanks,
Phil
On Thu, 5 Oct 2023 at 22:19, Phil Dennis-Jordan <lists@xxxxxxxxxxxxx> wrote:
>
> On Wed, Sep 27, 2023 at 4:18 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > Gah, sorry. This is why I usually inline patches, I forget to actually att=
> > ach
> > the darn things 50% of the time.
>
> Thanks for that, and apologies for only just getting around to taking
> a closer look. Out of the box, this doesn't build on macOS at least,
> as that's missing <linux/types.h>. I tried going down the rabbit hole
> of pulling that header and its various transitive dependencies in from
> the Linux tree, but ended up in a horrible mess where those headers
> try to define things like bool, which libcflat has already pulled in
> from the standard system headers (on Linux I suspect the libc #include
> guards match up with the stuff in <linux/*>, so there's no issue).
> On macOS, the problem is easy to resolve via a cut-down types.h with a
> minimal set of definitions:
>
> #include <libcflat.h>
> typedef u8 __u8;
> typedef u32 __u32;
> typedef u64 __u64;
> typedef s64 __s64;
>
> …but I assume that breaks things on Linux. I'm thinking something like
> this might work:
>
> #if __LINUX__
> #include_next <linux/types.h>
> #else
> [minimal types.h definitions]
> #endif
>
> But I'm unsure if that's really the direction you'd want to go with
> this? (And I still need to set myself up with a dev environment on a
> physical Linux box that I can test this all on.)
>
> Another option might be a symlinked linux/types.h created by
> ./configure if not running on Linux?
>
>
> On the substance of the patch itself:
>
> > unsigned long a0 = 0xFFFFFFFF, a1 = 0, a2 = 0xFFFFFFFF, a3 = 0x0;
> > - if (!test_device_enabled())
> > + if (!this_cpu_has(X86_FEATURE_KVM_PV_SEND_IPI))
>
> So this check will (erroneously IMO) succeed if we're running on a
> non-KVM hypervisor which happens to expose a flag at bit 11 of ecx on
> CPUID leaf 0x40000001 page 0, right? With this in mind, your earlier
> idea seems better:
>
> if (!is_hypervisor_kvm() ||
> !this_cpu_has(X86_FEATURE_KVM_PV_SEND_IPI)) {
>
> So I've gone ahead and made an attempt at fixing up your draft
> implementation of is_hypervisor_kvm() below.
>
> The partial struct memcmp in get_hypervisor_cpuid_base is a bit icky;
> I'm not sure if that's worth fixing up at the cost of readability.
>
>
> Thoughts?
>
> (I've attached the full set of WIP changes on top of yours as another
> patch. Feel free to squash it all into one if you decide to run with
> it.)
>
> Thanks,
> Phil
>
>
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 7a7048f9..3d3930c8 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -240,6 +240,7 @@ static inline bool is_intel(void)
> #define X86_FEATURE_XSAVE (CPUID(0x1, 0, ECX, 26))
> #define X86_FEATURE_OSXSAVE (CPUID(0x1, 0, ECX, 27))
> #define X86_FEATURE_RDRAND (CPUID(0x1, 0, ECX, 30))
> +#define X86_FEATURE_HYPERVISOR (CPUID(0x1, 0, ECX, 31))
> #define X86_FEATURE_MCE (CPUID(0x1, 0, EDX, 7))
> #define X86_FEATURE_APIC (CPUID(0x1, 0, EDX, 9))
> #define X86_FEATURE_CLFLUSH (CPUID(0x1, 0, EDX, 19))
> @@ -286,7 +287,8 @@ 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))
>
> -#define X86_FEATURE_KVM_PV_SEND_IPI (CPUID(KVM_CPUID_FEATURES, 0,
> EAX, KVM_FEATURE_PV_SEND_IPI))
> +#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)
> {
> @@ -303,6 +305,40 @@ static inline bool this_cpu_has(u64 feature)
> return ((*(tmp + (output_reg % 32))) & (1 << bit));
> }
>
> +static inline u32 get_hypervisor_cpuid_base(const char *sig)
> +{
> + u32 base;
> + struct cpuid signature;
> +
> + if (!this_cpu_has(X86_FEATURE_HYPERVISOR))
> + return 0;
> +
> + for (base = 0x40000000; base < 0x40010000; base += 0x100) {
> + signature = cpuid(base);
> +
> + if (!memcmp(sig, &signature.b, 12))
> + return base;
> + }
> +
> + return 0;
> +}
> +
> +static inline bool is_hypervisor_kvm(void)
> +{
> + u32 base = get_hypervisor_cpuid_base(KVM_SIGNATURE);
> +
> + if (!base)
> + return false;
> +
> + /*
> + * Require that KVM be placed at its default base so that macros can be
> + * used to query individual KVM feature bits.
> + */
> + assert_msg(base == KVM_CPUID_SIGNATURE,
> + "Expect KVM at its default cpuid base (now at: 0x%x)", base);
> + return true;
> +}
> +
> struct far_pointer32 {
> u32 offset;
> u16 selector;
