Re: [PATCH 10/29] KVM: share statistics for same vCPU id on different planes

[Sean Christopherson <seanjc@google.com>]

On Tue, Apr 01, 2025, Paolo Bonzini wrote:
> Statistics are protected by vcpu->mutex; because KVM_RUN takes the
> plane-0 vCPU mutex, there is no race on applying statistics for all
> planes to the plane-0 kvm_vcpu struct.
> 
> This saves the burden on the kernel of implementing the binary stats
> interface for vCPU plane file descriptors, and on userspace of gathering
> info from multiple planes.  The disadvantage is a slight loss of
> information, and an extra pointer dereference when updating stats.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/arm64/kvm/arm.c                 |  2 +-
>  arch/arm64/kvm/handle_exit.c         |  6 +--
>  arch/arm64/kvm/hyp/nvhe/gen-hyprel.c |  4 +-
>  arch/arm64/kvm/mmio.c                |  4 +-
>  arch/loongarch/kvm/exit.c            |  8 ++--
>  arch/loongarch/kvm/vcpu.c            |  2 +-
>  arch/mips/kvm/emulate.c              |  2 +-
>  arch/mips/kvm/mips.c                 | 30 +++++++-------
>  arch/mips/kvm/vz.c                   | 18 ++++-----
>  arch/powerpc/kvm/book3s.c            |  2 +-
>  arch/powerpc/kvm/book3s_hv.c         | 46 ++++++++++-----------
>  arch/powerpc/kvm/book3s_hv_rm_xics.c |  8 ++--
>  arch/powerpc/kvm/book3s_pr.c         | 22 +++++-----
>  arch/powerpc/kvm/book3s_pr_papr.c    |  2 +-
>  arch/powerpc/kvm/powerpc.c           |  4 +-
>  arch/powerpc/kvm/timing.h            | 28 ++++++-------
>  arch/riscv/kvm/vcpu.c                |  2 +-
>  arch/riscv/kvm/vcpu_exit.c           | 10 ++---
>  arch/riscv/kvm/vcpu_insn.c           | 16 ++++----
>  arch/riscv/kvm/vcpu_sbi.c            |  2 +-
>  arch/riscv/kvm/vcpu_sbi_hsm.c        |  2 +-
>  arch/s390/kvm/diag.c                 | 18 ++++-----
>  arch/s390/kvm/intercept.c            | 20 +++++-----
>  arch/s390/kvm/interrupt.c            | 48 +++++++++++-----------
>  arch/s390/kvm/kvm-s390.c             |  8 ++--
>  arch/s390/kvm/priv.c                 | 60 ++++++++++++++--------------
>  arch/s390/kvm/sigp.c                 | 50 +++++++++++------------
>  arch/s390/kvm/vsie.c                 |  2 +-
>  arch/x86/kvm/debugfs.c               |  2 +-
>  arch/x86/kvm/hyperv.c                |  4 +-
>  arch/x86/kvm/kvm_cache_regs.h        |  4 +-
>  arch/x86/kvm/mmu/mmu.c               | 18 ++++-----
>  arch/x86/kvm/mmu/tdp_mmu.c           |  2 +-
>  arch/x86/kvm/svm/sev.c               |  2 +-
>  arch/x86/kvm/svm/svm.c               | 18 ++++-----
>  arch/x86/kvm/vmx/tdx.c               |  8 ++--
>  arch/x86/kvm/vmx/vmx.c               | 20 +++++-----
>  arch/x86/kvm/x86.c                   | 40 +++++++++----------
>  include/linux/kvm_host.h             |  5 ++-
>  virt/kvm/kvm_main.c                  | 19 ++++-----
>  40 files changed, 285 insertions(+), 283 deletions(-)

...

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index dbca418d64f5..d2e0c0e8ff17 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -393,7 +393,8 @@ struct kvm_vcpu {
>  	bool ready;
>  	bool scheduled_out;
>  	struct kvm_vcpu_arch arch;
> -	struct kvm_vcpu_stat stat;
> +	struct kvm_vcpu_stat *stat;
> +	struct kvm_vcpu_stat __stat;

Rather than special case invidiual fields, I think we should give kvm_vcpu the
same treatment as "struct kvm", and have kvm_vcpu represent the overall vCPU,
with an array of planes to hold the sub-vCPUs.

Having "kvm_vcpu" represent a plane, while "kvm" represents the overall VM, is
conceptually messy.  And more importantly, I think the approach taken here will
be nigh impossible to maintain, and will have quite a bit of baggage.  E.g. planes1+
will be filled with dead memory, and we also risk goofs where KVM could access
__stat in a plane1+ vCPU.

Documenting which fields are plane0-only, i.e. per-vCPU, via comments isn't
sustainable, whereas a hard split via structures will naturally what fields are
scope to the overall vCPU, versus what is per-plane, and will force us to more
explicitly audit the code.  E.g. ____srcu_idx (and thus srcu_depth) is something
that I think should be shared by all planes.  Ditto for preempt_notifier, vcpu_id,
vcpu_idx, pid, etc.

Aha!  And to prove my point, this series breaks legacy signal handling, because
sigset_active and sigset are accessed using the plane1+ vCPU in kvm_vcpu_ioctl_run_plane(),
but KVM_SET_SIGNAL_MASK is only allowed to operated on plane0.  And I definitely
don't think the answer is to let KVM_SET_SIGNAL_MASK operate on plane1+, because
forcing userspace to duplicate the sigmal masks to all planes is pointless.

Yeeeaaap.  pid and pid_lock are also broken.  As is vmx_hwapic_isr_update()
and kvm_sched_out()'s usage of wants_to_run.  And guest_debug.

Long term, I just don't see this approach as being maintainable.  We're pretty
much guaranteed to end up with bugs where KVM operates on the wrong kvm_vcpu
structure due to lack of explicit isolation in code.  And those bugs are going
to absolutely brutal to debug (or even notice).  E.g. failure to set "preempted"
on planes 1+ will mostly manifest as subtle performance issues.

Oof.  And that would force us to document that duplicating cpuid and cpu_caps to
planes1+ is actually necessary, due to dynamic CPUID features (ugh).  Though FWIW,
we could dodge that by special casing dynamic features, which isn't a bad idea
irrespective of planes.

Somewhat of a side topic: unless we need/want to explicitly support concurrent
GET/SET on planes of a vCPU, I think we should make vcpu->mutex per-vCPU, not
per-plane, so that there's zero chance of having bugs due to thinking that holding
vcpu->mutex provides protection against a race.

Extracing fields to a separate kvm_vcpu_plane will obviously require a *lot* more
churn, but I think in the long run it will be less work in total, because we won't
spend as much time chasing down bugs.

Very little per-plane state is in "struct kvm_vcpu", so I think we can do the big
conversion on a per-arch basis via a small amount of #ifdefs, i.e. not be force to
immediatedly convert all architectures to a kvm_vcpu vs. kvm_vcpu_plane world.