public inbox for stable@vger.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Borislav Petkov <bp@alien8.de>
Cc: Pavel Machek <pavel@denx.de>, Sasha Levin <sashal@kernel.org>,
	linux-kernel@vger.kernel.org,  stable@vger.kernel.org,
	Max Grobecker <max@grobecker.info>,
	Ingo Molnar <mingo@kernel.org>,
	 tglx@linutronix.de, mingo@redhat.com,
	dave.hansen@linux.intel.com,  x86@kernel.org,
	thomas.lendacky@amd.com, perry.yuan@amd.com,
	 mario.limonciello@amd.com, riel@surriel.com, mjguzik@gmail.com,
	 darwi@linutronix.de, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH AUTOSEL 5.10 2/6] x86/cpu: Don't clear X86_FEATURE_LAHF_LM flag in init_amd_k8() on AMD when running in a virtual machine
Date: Tue, 22 Apr 2025 10:22:54 -0700	[thread overview]
Message-ID: <aAfQbiqp_yIV3OOC@google.com> (raw)
In-Reply-To: <20250418191224.GFaAKkGBnb01tGUVhW@fat_crate.local>

+Paolo

On Fri, Apr 18, 2025, Borislav Petkov wrote:
> On Fri, Apr 18, 2025 at 11:31:27AM -0700, Sean Christopherson wrote:
> > IMO, this is blatantly a QEMU bug (I verified the behavior when using "kvm64" on AMD).
> > As per QEMU commit d1cd4bf419 ("introduce kvm64 CPU"), the vendor + FMS enumerates
> > an Intel P4:
> > 
> >         .name = "kvm64",
> >         .level = 0xd,
> >         .vendor = CPUID_VENDOR_INTEL,
> >         .family = 15,
> >         .model = 6,
> > 
> > Per x86_cpu_load_model(), QEMU overrides the vendor when using KVM (at a glance,
> > I can't find the code that actually overrides the vendor, gotta love QEMU's object
> > model):
> 
> LOL, I thought I was the only one who thought this is madness. :-P

Yeah, I've got backtraces and I still don't entirely understand who's doing what.

> >     /*
> >      * vendor property is set here but then overloaded with the
> >      * host cpu vendor for KVM and HVF.
> >      */
> >     object_property_set_str(OBJECT(cpu), "vendor", def->vendor, &error_abort);
> > 
> > Overriding the vendor but using Intel's P4 FMS is flat out wrong.  IMO, QEMU
> > should use the same FMS as qemu64 for kvm64 when running on AMD.
> > 
> >         .name = "qemu64",
> >         .level = 0xd,
> >         .vendor = CPUID_VENDOR_AMD,
> >         .family = 15,
> >         .model = 107,
> >         .stepping = 1,
> > 
> > Yeah, scraping FMS information is a bad idea, but what QEMU is doing is arguably
> > far worse.
> 
> Ok, let's fix qemu. I don't have a clue, though, how to go about that so I'd
> rely on your guidance here.

I have no idea how to fix the QEMU code.


Paolo,

The TL;DR of the problem is that QEMU's "kvm64" CPU type sets FMS to Intel P4,
and doesn't swizzle the FMS to something sane when running on AMD.  This results
in QEMU advertising the CPU as an ancient K8, which causes at least one *known*
problem due software making decisions on the funky FMS.

My stance is that QEMU is buggy/flawed and should stuff a FMS that is sane for
the underlying vendor for kvm64.  I'd send an RFC patch, but for the life of me
I can't figure what that would even look like.

> Because I really hate wagging the dog and "fixing" the kernel because something
> else can't be bothered. I didn't object stronger to that fix because it is
> meh, more of those "if I'm a guest" gunk which we sprinkle nowadays and that's
> apparently not that awful-ish...

FWIW, I think splattering X86_FEATURE_HYPERVISOR everywhere is quite awful.  There
are definitely cases where the kernel needs to know if it's running as a guest,
because the behavior of "hardware" fundamentally changes in ways that can't be
enumerated otherwise.  E.g. that things like the HPET are fully emulated and thus
will be prone to significant jitter.

But when it comes to feature enumeration, IMO sprinkling HYPERVISOR everywhere is
unnecessary because it's the hypervisor/VMM's responsibility to present a sane
model.  And I also think it's outright dangerous, because everywhere the kernel
does X for bare metal and Y for guest results in reduced test coverage.

E.g. things like syzkaller and other bots will largely be testing the HYPERVISOR
code, while humans will largely be testing and using the bare metal code.

  reply	other threads:[~2025-04-22 17:22 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-31 14:37 [PATCH AUTOSEL 5.10 1/6] pm: cpupower: bench: Prevent NULL dereference on malloc failure Sasha Levin
2025-03-31 14:37 ` [PATCH AUTOSEL 5.10 2/6] x86/cpu: Don't clear X86_FEATURE_LAHF_LM flag in init_amd_k8() on AMD when running in a virtual machine Sasha Levin
2025-04-18 16:54   ` Pavel Machek
2025-04-18 17:19     ` Sean Christopherson
2025-04-18 17:36       ` Borislav Petkov
2025-04-18 18:31         ` Sean Christopherson
2025-04-18 19:12           ` Borislav Petkov
2025-04-22 17:22             ` Sean Christopherson [this message]
2025-04-22 17:33               ` CONFIG_X86_HYPERVISOR (was: Re: [PATCH AUTOSEL 5.10 2/6] x86/cpu: Don't clear X86_FEATURE_LAHF_LM flag in init_amd_k8() on AMD when running in a virtual machine) Borislav Petkov
2025-04-22 19:48                 ` Sean Christopherson
2025-04-23  7:20                   ` Borislav Petkov
2025-04-23 14:10                     ` Sean Christopherson
2025-04-23 18:43                       ` Borislav Petkov
2025-04-24 19:18                         ` Sean Christopherson
2025-04-24 20:31                           ` Borislav Petkov
2025-04-26  0:08                             ` Sean Christopherson
2025-04-26 11:26                               ` Borislav Petkov
2025-05-06  1:04                                 ` Sean Christopherson
2025-03-31 14:37 ` [PATCH AUTOSEL 5.10 3/6] perf: arm_pmu: Don't disable counter in armpmu_add() Sasha Levin
2025-03-31 14:37 ` [PATCH AUTOSEL 5.10 4/6] arm64: cputype: Add QCOM_CPU_PART_KRYO_3XX_GOLD Sasha Levin
2025-04-18 16:55   ` Pavel Machek
2025-04-18 19:27     ` Doug Anderson
2025-03-31 14:37 ` [PATCH AUTOSEL 5.10 5/6] xen/mcelog: Add __nonstring annotations for unterminated strings Sasha Levin
2025-03-31 14:37 ` [PATCH AUTOSEL 5.10 6/6] x86/mm/ident_map: Fix theoretical virtual address overflow to zero Sasha Levin
2025-04-18 16:52 ` [PATCH AUTOSEL 5.10 1/6] pm: cpupower: bench: Prevent NULL dereference on malloc failure Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aAfQbiqp_yIV3OOC@google.com \
    --to=seanjc@google.com \
    --cc=bp@alien8.de \
    --cc=darwi@linutronix.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mario.limonciello@amd.com \
    --cc=max@grobecker.info \
    --cc=mingo@kernel.org \
    --cc=mingo@redhat.com \
    --cc=mjguzik@gmail.com \
    --cc=pavel@denx.de \
    --cc=pbonzini@redhat.com \
    --cc=perry.yuan@amd.com \
    --cc=riel@surriel.com \
    --cc=sashal@kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=thomas.lendacky@amd.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox