[PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Gao Shiyuan]

From: Shiyuan Gao <gaoshiyuan@baidu.com>

When live-migrate VM on icelake microarchitecture, if the source
host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
number of vPMU fixed counters to 3") and the dest host kernel after this
commit, the migration will fail.

The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
This inconsistency leads to migration failure.

The QEMU limits the maximum number of vPMU fixed counters to 3, so ignore
the check of IA32_PERF_GLOBAL_CTRL bit35.

Fixes: 2e8cd7a3b828 ("kvm: x86: limit the maximum number of vPMU fixed counters to 3")

Signed-off-by: Shiyuan Gao <gaoshiyuan@baidu.com>
---
 arch/x86/kvm/pmu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 5c7bbf03b599..9895311d334e 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -91,7 +91,7 @@ static inline bool pmc_is_fixed(struct kvm_pmc *pmc)
 static inline bool kvm_valid_perf_global_ctrl(struct kvm_pmu *pmu,
 						 u64 data)
 {
-	return !(pmu->global_ctrl_mask & data);
+	return !(pmu->global_ctrl_mask & (data & ~(1ULL << 35)));
 }
 
 /* returns general purpose PMC with the specified MSR. Note that it can be
-- 
2.36.1

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Jim Mattson]

On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com> wrote:
>
> From: Shiyuan Gao <gaoshiyuan@baidu.com>
>
> When live-migrate VM on icelake microarchitecture, if the source
> host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> number of vPMU fixed counters to 3") and the dest host kernel after this
> commit, the migration will fail.
>
> The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> This inconsistency leads to migration failure.
>
> The QEMU limits the maximum number of vPMU fixed counters to 3, so ignore
> the check of IA32_PERF_GLOBAL_CTRL bit35.

Today, the fixed counters are limited to 3, but I hope we get support
for top down slots soon.

Perhaps this inconsistency is best addressed with a quirk?

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Sean Christopherson]

On Fri, Jun 02, 2023, Jim Mattson wrote:
> On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com> wrote:
> >
> > From: Shiyuan Gao <gaoshiyuan@baidu.com>
> >
> > When live-migrate VM on icelake microarchitecture, if the source
> > host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> > number of vPMU fixed counters to 3") and the dest host kernel after this
> > commit, the migration will fail.
> >
> > The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> > the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> > CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> > This inconsistency leads to migration failure.

IMO, this is a userspace bug.  KVM provided userspace all the information it needed
to know that the target is incompatible (3 counters instead of 4), it's userspace's
fault for not sanity checking that the target is compatible.

I agree that KVM isn't blame free, but hacking KVM to cover up userspace mistakes
everytime a feature appears or disappears across kernel versions or configs isn't
maintainable.

> > The QEMU limits the maximum number of vPMU fixed counters to 3, so ignore
> > the check of IA32_PERF_GLOBAL_CTRL bit35.

Unconditionally allowing the bit is unsafe, e.g. will cause KVM to miss consistency
checks on PERF_GLOBAL_CTRL when emulating nested VM-Enter.  They should eventually
be caught by hardware, but it's still undesirable.

> Today, the fixed counters are limited to 3, but I hope we get support
> for top down slots soon.

Is there any reason KVM can't simply add support for the fourth fixed counter?
Perf is already aware of the topdown slots event, so isn't this "just" a matter
of adding the appropriate entries?

> Perhaps this inconsistency is best addressed with a quirk?

*If* we ended up adding a hack to KVM, I'd prefer not to add a quirk, this shouldn't
be something KVM needs to carry long term.

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Jim Mattson]

On Fri, Jun 2, 2023 at 12:16 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Jun 02, 2023, Jim Mattson wrote:
> > On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com> wrote:
> > >
> > > From: Shiyuan Gao <gaoshiyuan@baidu.com>
> > >
> > > When live-migrate VM on icelake microarchitecture, if the source
> > > host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> > > number of vPMU fixed counters to 3") and the dest host kernel after this
> > > commit, the migration will fail.
> > >
> > > The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> > > the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> > > CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> > > This inconsistency leads to migration failure.
>
> IMO, this is a userspace bug.  KVM provided userspace all the information it needed
> to know that the target is incompatible (3 counters instead of 4), it's userspace's
> fault for not sanity checking that the target is compatible.
>
> I agree that KVM isn't blame free, but hacking KVM to cover up userspace mistakes
> everytime a feature appears or disappears across kernel versions or configs isn't
> maintainable.

Um...

"You may never migrate this VM to a newer kernel. Sucks to be you."

That's not very user-friendly.

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Sean Christopherson]

On Fri, Jun 02, 2023, Jim Mattson wrote:
> On Fri, Jun 2, 2023 at 12:16 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com> wrote:
> > > >
> > > > From: Shiyuan Gao <gaoshiyuan@baidu.com>
> > > >
> > > > When live-migrate VM on icelake microarchitecture, if the source
> > > > host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> > > > number of vPMU fixed counters to 3") and the dest host kernel after this
> > > > commit, the migration will fail.
> > > >
> > > > The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> > > > the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> > > > CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> > > > This inconsistency leads to migration failure.
> >
> > IMO, this is a userspace bug.  KVM provided userspace all the information it needed
> > to know that the target is incompatible (3 counters instead of 4), it's userspace's
> > fault for not sanity checking that the target is compatible.
> >
> > I agree that KVM isn't blame free, but hacking KVM to cover up userspace mistakes
> > everytime a feature appears or disappears across kernel versions or configs isn't
> > maintainable.
> 
> Um...
> 
> "You may never migrate this VM to a newer kernel. Sucks to be you."

Userspace can fudge/fixup state to migrate the VM.

> That's not very user-friendly.

Heh, I never claimed it was.  

I don't think KVM should treat this any differently than if userspace didn't strip
a new feature when regurgitating KVM_GET_SUPPORTED_CPUID, and ended up with VMs
that couldn't migrate to *older* kernels.

The only way this is KVM's responsibility is if KVM's ABI is defined such that
KVM_GET_SUPPORTED_CPUID is strictly "increasing" across kernel versions (on the
same hardware).  I reall don't want to go down that route, as that would complicate
fixing KVM bugs, and would pull in things beyond KVM's control.  E.g. PCID support
is about to disappear on hardware affected by the recent INVLPG erratum (commit
ce0b15d11ad8 "x86/mm: Avoid incomplete Global INVLPG flushes").

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Jim Mattson]

On Fri, Jun 2, 2023 at 2:48 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Jun 02, 2023, Jim Mattson wrote:
> > On Fri, Jun 2, 2023 at 12:16 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com> wrote:
> > > > >
> > > > > From: Shiyuan Gao <gaoshiyuan@baidu.com>
> > > > >
> > > > > When live-migrate VM on icelake microarchitecture, if the source
> > > > > host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> > > > > number of vPMU fixed counters to 3") and the dest host kernel after this
> > > > > commit, the migration will fail.
> > > > >
> > > > > The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> > > > > the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> > > > > CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> > > > > This inconsistency leads to migration failure.
> > >
> > > IMO, this is a userspace bug.  KVM provided userspace all the information it needed
> > > to know that the target is incompatible (3 counters instead of 4), it's userspace's
> > > fault for not sanity checking that the target is compatible.
> > >
> > > I agree that KVM isn't blame free, but hacking KVM to cover up userspace mistakes
> > > everytime a feature appears or disappears across kernel versions or configs isn't
> > > maintainable.
> >
> > Um...
> >
> > "You may never migrate this VM to a newer kernel. Sucks to be you."
>
> Userspace can fudge/fixup state to migrate the VM.

Um, yeah. Userspace can clear bit 35 from the saved
IA32_PERF_GLOBAL_CTRL MSR so that the migration will complete. But
what happens the next time the guest tries to set bit 35 in
IA32_PERF_GLOBAL_CTRL, which it will probably do, since it cached
CPUID.0AH at boot?

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Sean Christopherson]

On Fri, Jun 02, 2023, Jim Mattson wrote:
> On Fri, Jun 2, 2023 at 2:48 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > On Fri, Jun 2, 2023 at 12:16 PM Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > > On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com> wrote:
> > > > > >
> > > > > > From: Shiyuan Gao <gaoshiyuan@baidu.com>
> > > > > >
> > > > > > When live-migrate VM on icelake microarchitecture, if the source
> > > > > > host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> > > > > > number of vPMU fixed counters to 3") and the dest host kernel after this
> > > > > > commit, the migration will fail.
> > > > > >
> > > > > > The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> > > > > > the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> > > > > > CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> > > > > > This inconsistency leads to migration failure.
> > > >
> > > > IMO, this is a userspace bug.  KVM provided userspace all the information it needed
> > > > to know that the target is incompatible (3 counters instead of 4), it's userspace's
> > > > fault for not sanity checking that the target is compatible.
> > > >
> > > > I agree that KVM isn't blame free, but hacking KVM to cover up userspace mistakes
> > > > everytime a feature appears or disappears across kernel versions or configs isn't
> > > > maintainable.
> > >
> > > Um...
> > >
> > > "You may never migrate this VM to a newer kernel. Sucks to be you."
> >
> > Userspace can fudge/fixup state to migrate the VM.
> 
> Um, yeah. Userspace can clear bit 35 from the saved
> IA32_PERF_GLOBAL_CTRL MSR so that the migration will complete. But
> what happens the next time the guest tries to set bit 35 in
> IA32_PERF_GLOBAL_CTRL, which it will probably do, since it cached
> CPUID.0AH at boot?

Ah, right.  Yeah, guest is hosed.

I'm still not convinced this is KVM's problem to fix.

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Jim Mattson]

On Fri, Jun 2, 2023 at 3:52 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Jun 02, 2023, Jim Mattson wrote:
> > On Fri, Jun 2, 2023 at 2:48 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > On Fri, Jun 2, 2023 at 12:16 PM Sean Christopherson <seanjc@google.com> wrote:
> > > > >
> > > > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > > > On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com> wrote:
> > > > > > >
> > > > > > > From: Shiyuan Gao <gaoshiyuan@baidu.com>
> > > > > > >
> > > > > > > When live-migrate VM on icelake microarchitecture, if the source
> > > > > > > host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> > > > > > > number of vPMU fixed counters to 3") and the dest host kernel after this
> > > > > > > commit, the migration will fail.
> > > > > > >
> > > > > > > The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> > > > > > > the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> > > > > > > CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> > > > > > > This inconsistency leads to migration failure.
> > > > >
> > > > > IMO, this is a userspace bug.  KVM provided userspace all the information it needed
> > > > > to know that the target is incompatible (3 counters instead of 4), it's userspace's
> > > > > fault for not sanity checking that the target is compatible.
> > > > >
> > > > > I agree that KVM isn't blame free, but hacking KVM to cover up userspace mistakes
> > > > > everytime a feature appears or disappears across kernel versions or configs isn't
> > > > > maintainable.
> > > >
> > > > Um...
> > > >
> > > > "You may never migrate this VM to a newer kernel. Sucks to be you."
> > >
> > > Userspace can fudge/fixup state to migrate the VM.
> >
> > Um, yeah. Userspace can clear bit 35 from the saved
> > IA32_PERF_GLOBAL_CTRL MSR so that the migration will complete. But
> > what happens the next time the guest tries to set bit 35 in
> > IA32_PERF_GLOBAL_CTRL, which it will probably do, since it cached
> > CPUID.0AH at boot?
>
> Ah, right.  Yeah, guest is hosed.
>
> I'm still not convinced this is KVM's problem to fix.

One could argue that userspace should have known better than to
believe KVM_GET_SUPPORTED_CPUID in the first place. Or that it should
have known better than to blindly pass that through to KVM_SET_CPUID2.
I mean, *obviously* KVM didn't really support TOPDOWN.SLOTS. Right?

But if userspace can't trust KVM_GET_SUPPORTED_CPUID to tell it about
which fixed counters are supported, how is it supposed to find out?

Another way of solving this, which should make everyone happy, is to
add KVM support for TOPDOWN.SLOTS.

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Gao,Shiyuan]

On Fri, Jun 3, 2023 at 6:52 AM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com>> wrote:

> On Fri, Jun 02, 2023, Jim Mattson wrote:
> > On Fri, Jun 2, 2023 at 2:48 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com>> wrote:
> > >
> > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > On Fri, Jun 2, 2023 at 12:16 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com>> wrote:
> > > > >
> > > > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > > > On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com <mailto:gaoshiyuan@baidu.com>> wrote:
> > > > > > >
> > > > > > > From: Shiyuan Gao <gaoshiyuan@baidu.com <mailto:gaoshiyuan@baidu.com>>
> > > > > > >
> > > > > > > When live-migrate VM on icelake microarchitecture, if the source
> > > > > > > host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> > > > > > > number of vPMU fixed counters to 3") and the dest host kernel after this
> > > > > > > commit, the migration will fail.
> > > > > > >
> > > > > > > The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> > > > > > > the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> > > > > > > CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> > > > > > > This inconsistency leads to migration failure.
> > > > >
> > > > > IMO, this is a userspace bug. KVM provided userspace all the information it needed
> > > > > to know that the target is incompatible (3 counters instead of 4), it's userspace's
> > > > > fault for not sanity checking that the target is compatible.
> > > > >
> > > > > I agree that KVM isn't blame free, but hacking KVM to cover up userspace mistakes
> > > > > everytime a feature appears or disappears across kernel versions or configs isn't
> > > > > maintainable.
> > > >
yeah, this is userspace's fault, I also submmit a patch to QEMU:
https://lore.kernel.org/kvm/20230602073857.96790-1-gaoshiyuan@baidu.com/T/#u

> > > >
> > > > "You may never migrate this VM to a newer kernel. Sucks to be you."
> > >
> > > Userspace can fudge/fixup state to migrate the VM.
> >
> > Um, yeah. Userspace can clear bit 35 from the saved
> > IA32_PERF_GLOBAL_CTRL MSR so that the migration will complete. But
> > what happens the next time the guest tries to set bit 35 in
> > IA32_PERF_GLOBAL_CTRL, which it will probably do, since it cached
> > CPUID.0AH at boot?
>
>
> Ah, right. Yeah, guest is hosed.
>
>
> I'm still not convinced this is KVM's problem to fix.
>
I tried to clear bit 35 in userspace during the migration process, the migration is successful.
However, the perf cann't be used in the dest VM.

I admit that fixed this problem in KVM is unsuitable. I can't find any better way.

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Gao,Shiyuan]

On Fri, Jun 3, 2023, Jim Mattson wrote:

> On Fri, Jun 2, 2023 at 3:52 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com>> wrote:
> >
> > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > On Fri, Jun 2, 2023 at 2:48 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com>> wrote:
> > > >
> > > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > > On Fri, Jun 2, 2023 at 12:16 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com>> wrote:
> > > > > >
> > > > > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > > > > On Fri, Jun 2, 2023 at 12:18 AM Gao Shiyuan <gaoshiyuan@baidu.com <mailto:gaoshiyuan@baidu.com>> wrote:
> > > > > > > >
> > > > > > > > From: Shiyuan Gao <gaoshiyuan@baidu.com <mailto:gaoshiyuan@baidu.com>>
> > > > > > > >
> > > > > > > > When live-migrate VM on icelake microarchitecture, if the source
> > > > > > > > host kernel before commit 2e8cd7a3b828 ("kvm: x86: limit the maximum
> > > > > > > > number of vPMU fixed counters to 3") and the dest host kernel after this
> > > > > > > > commit, the migration will fail.
> > > > > > > >
> > > > > > > > The source VM's CPUID.0xA.edx[0..4]=4 that is reported by KVM and
> > > > > > > > the IA32_PERF_GLOBAL_CTRL MSR is 0xf000000ff. However the dest VM's
> > > > > > > > CPUID.0xA.edx[0..4]=3 and the IA32_PERF_GLOBAL_CTRL MSR is 0x7000000ff.
> > > > > > > > This inconsistency leads to migration failure.
> > > > > >
> > > > > > IMO, this is a userspace bug. KVM provided userspace all the information it needed
> > > > > > to know that the target is incompatible (3 counters instead of 4), it's userspace's
> > > > > > fault for not sanity checking that the target is compatible.
> > > > > >
> > > > > > I agree that KVM isn't blame free, but hacking KVM to cover up userspace mistakes
> > > > > > everytime a feature appears or disappears across kernel versions or configs isn't
> > > > > > maintainable.
> > > > >
> > > > > Um...
> > > > >
> > > > > "You may never migrate this VM to a newer kernel. Sucks to be you."
> > > >
> > > > Userspace can fudge/fixup state to migrate the VM.
> > >
> > > Um, yeah. Userspace can clear bit 35 from the saved
> > > IA32_PERF_GLOBAL_CTRL MSR so that the migration will complete. But
> > > what happens the next time the guest tries to set bit 35 in
> > > IA32_PERF_GLOBAL_CTRL, which it will probably do, since it cached
> > > CPUID.0AH at boot?
> >
> > Ah, right. Yeah, guest is hosed.
> >
> > I'm still not convinced this is KVM's problem to fix.
>
>
> One could argue that userspace should have known better than to
> believe KVM_GET_SUPPORTED_CPUID in the first place. Or that it should
> have known better than to blindly pass that through to KVM_SET_CPUID2.
> I mean, *obviously* KVM didn't really support TOPDOWN.SLOTS. Right?
>
>
> But if userspace can't trust KVM_GET_SUPPORTED_CPUID to tell it about
> which fixed counters are supported, how is it supposed to find out?
>
>
> Another way of solving this, which should make everyone happy, is to
> add KVM support for TOPDOWN.SLOTS.
>
Yeah, this way may make everyone happly, but we need guarantee the VM that
not support TOPDOWN.SLOTS migrate success. I think this also need be addressed
with a quirk like this submmit.

I can't find an elegant solution...

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Sean Christopherson]

On Mon, Jun 05, 2023, Gao,Shiyuan wrote:
> On Fri, Jun 3, 2023, Jim Mattson wrote:
> 
> > On Fri, Jun 2, 2023 at 3:52 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com>> wrote:
> > >
> > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > On Fri, Jun 2, 2023 at 2:48 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com>> wrote:
> > > > >
> > > > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > Um, yeah. Userspace can clear bit 35 from the saved
> > > > IA32_PERF_GLOBAL_CTRL MSR so that the migration will complete. But
> > > > what happens the next time the guest tries to set bit 35 in
> > > > IA32_PERF_GLOBAL_CTRL, which it will probably do, since it cached
> > > > CPUID.0AH at boot?
> > >
> > > Ah, right. Yeah, guest is hosed.
> > >
> > > I'm still not convinced this is KVM's problem to fix.
> >
> > One could argue that userspace should have known better than to
> > believe KVM_GET_SUPPORTED_CPUID in the first place. Or that it should
> > have known better than to blindly pass that through to KVM_SET_CPUID2.
> > I mean, *obviously* KVM didn't really support TOPDOWN.SLOTS. Right?
> >
> >
> > But if userspace can't trust KVM_GET_SUPPORTED_CPUID to tell it about
> > which fixed counters are supported, how is it supposed to find out?
> >
> >
> > Another way of solving this, which should make everyone happy, is to
> > add KVM support for TOPDOWN.SLOTS.
> >
> Yeah, this way may make everyone happly, but we need guarantee the VM that
> not support TOPDOWN.SLOTS migrate success. I think this also need be addressed
> with a quirk like this submmit.
> 
> I can't find an elegant solution...

I can't think of an elegant solution either.  That said, I still don't think we
should add a quirk to upstream KVM.  This is not a longstanding KVM goof that
userspace has come to rely on, it's a combination of bugs in KVM, QEMU, and the
deployment (for presumably not validating before pushing to production).  And the
issue affects a only relatively new CPUs.  Silently suppressing a known bad config
also makes me uncomfortable, even though it's unlikely that any deplyoment would
rather terminate VMs than run with a messed up vPMU.

I'm not dead set against a quirk, but unless the issue affects a broad set of
users, I would prefer to not carry anything in upstream, and instead have (the
(hopefully small set of) users carry an out-of-tree hack-a-fix until all their
affected VMs are rebooted on a fixed KVM and/or QEMU.

Re: [PATCH] KVM: x86/vPMU: ignore the check of IA32_PERF_GLOBAL_CTRL bit35

[Gao,Shiyuan]

On 2023/6/14 at 5:00 AM，Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com> wrote:

> On Mon, Jun 05, 2023, Gao,Shiyuan wrote:
>
> > On Fri, Jun 3, 2023, Jim Mattson wrote:
> >
> > > On Fri, Jun 2, 2023 at 3:52 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com> <mailto:seanjc@google.com <mailto:seanjc@google.com>>> wrote:
> > > >
> > > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > > On Fri, Jun 2, 2023 at 2:48 PM Sean Christopherson <seanjc@google.com <mailto:seanjc@google.com> <mailto:seanjc@google.com <mailto:seanjc@google.com>>> wrote:
> > > > > >
> > > > > > On Fri, Jun 02, 2023, Jim Mattson wrote:
> > > > > Um, yeah. Userspace can clear bit 35 from the saved
> > > > > IA32_PERF_GLOBAL_CTRL MSR so that the migration will complete. But
> > > > > what happens the next time the guest tries to set bit 35 in
> > > > > IA32_PERF_GLOBAL_CTRL, which it will probably do, since it cached
> > > > > CPUID.0AH at boot?
> > > >
> > > > Ah, right. Yeah, guest is hosed.
> > > >
> > > > I'm still not convinced this is KVM's problem to fix.
> > >
> > > One could argue that userspace should have known better than to
> > > believe KVM_GET_SUPPORTED_CPUID in the first place. Or that it should
> > > have known better than to blindly pass that through to KVM_SET_CPUID2.
> > > I mean, *obviously* KVM didn't really support TOPDOWN.SLOTS. Right?
> > >
> > >
> > > But if userspace can't trust KVM_GET_SUPPORTED_CPUID to tell it about
> > > which fixed counters are supported, how is it supposed to find out?
> > >
> > >
> > > Another way of solving this, which should make everyone happy, is to
> > > add KVM support for TOPDOWN.SLOTS.
> > >
> > Yeah, this way may make everyone happly, but we need guarantee the VM that
> > not support TOPDOWN.SLOTS migrate success. I think this also need be addressed
> > with a quirk like this submmit.
> >
> > I can't find an elegant solution...
>
>
> I can't think of an elegant solution either. That said, I still don't think we
> should add a quirk to upstream KVM. This is not a longstanding KVM goof that
> userspace has come to rely on, it's a combination of bugs in KVM, QEMU, and the
> deployment (for presumably not validating before pushing to production). And the
> issue affects a only relatively new CPUs. Silently suppressing a known bad config
> also makes me uncomfortable, even though it's unlikely that any deplyoment would
> rather terminate VMs than run with a messed up vPMU.
>
>
> I'm not dead set against a quirk, but unless the issue affects a broad set of
> users, I would prefer to not carry anything in upstream, and instead have (the
> (hopefully small set of) users carry an out-of-tree hack-a-fix until all their
> affected VMs are rebooted on a fixed KVM and/or QEMU.
>

As long as limit the maximum number of vPMU fixed counters to 3 in kvm, I think the
check of IA32_PERF_GLOBAL_CTRL bit35-63 is unnecessary.

Maybe define a macro such as IA32_PERF_GLOBAL_CTRL_RESERVED under MAX_FIXED_COUNTERS,
and ignore the check from IA32_PERF_GLOBAL_CTRL_RESERVED to bit63.

 #define MAX_FIXED_COUNTERS     3
+#define IA32_PERF_GLOBAL_CTRL_RESERVED 35

 static inline bool kvm_valid_perf_global_ctrl(struct kvm_pmu *pmu,
                                                 u64 data)
 {
-       return !(pmu->global_ctrl_mask & data);
+       return !(pmu->global_ctrl_mask & (data & (1ULL << IA32_PERF_GLOBAL_CTRL_RESERVED) - 1));
 }