Re: [PATCH v2 15/15] KVM: x86: Move the bulk of register specific code from x86.c to regs.c

[Sean Christopherson <seanjc@google.com>]

On Wed, May 20, 2026, Kai Huang wrote:
> On Tue, 2026-05-19 at 08:04 -0700, Sean Christopherson wrote:
> > On Tue, May 19, 2026, Kai Huang wrote:
> Just wondering is it possible we might want to move events handling to some
> other C file since you are cleanup x86.c?  But we can deal with this when it
> happens.

Events are a hard one.  There's a decent amount of code, but not _so_ much that
it's a no-brainer to move them out of x86.c.  And there's no super clear cut
boundary, e.g. events can mean exceptions, INIT+SIPI, IRQs, APIC stuff, etc.,
several of which already have substantial amounts of code outside of x86.c.

> > Hmm, though looking at all of this again, I think we're actually quite close to
> > having somewhat sane rules.  Over the past few years, I've tried multiple times
> > to move what I felt should be KVM-internal structures from asm/kvm_host.h to x86.h,
> > and I've failed miserably every time because inevitably even the most innocuous
> > struct manages to have usage that leads to cyclical header dependencies and/or is
> > used by arch-neutral KVM code.
> 
> The problem is some other kernel code includes <linux/kvm_host.h> (which in turn
> includes <asm/kvm_host.h>) but the KVM internal structures have nothing to do
> with them.
> 
> E.g., some drivers are using <linux/kvm_host.h>:
> 
> #$ grep kvm_host.h drivers/ -Rn
> drivers/vfio/pci/vfio_pci_zdev.c:14:#include <linux/kvm_host.h>
> drivers/vfio/vfio_main.c:20:#include <linux/kvm_host.h>
> drivers/firmware/arm_sdei.c:19:#include <linux/kvm_host.h>
> drivers/hwtracing/coresight/coresight-trbe.c:20:#include <linux/kvm_host.h>
> drivers/hwtracing/coresight/coresight-etm4x-core.c:10:#include
> <linux/kvm_host.h>
> drivers/s390/crypto/vfio_ap_ops.c:17:#include <linux/kvm_host.h>
> drivers/s390/crypto/vfio_ap_private.h:20:#include <linux/kvm_host.h>
> 
> But looking at them, AFAICT what they need is only some structure declarations
> (e.g., 'struct kvm;') for type safety (plus some function declarations), but
> don't actually need to see the actual structure.

Ya.

> For x86, AFAICT there's (only) "arch/x86/events/intel/core.c" actually uses the
> 'struct kvm_pmu', though.

I have a patch to fix that :-)

https://lore.kernel.org/all/20260508231353.406465-7-seanjc@google.com

> I haven't checked other ARCHs whether there's cases actually need to use any
> structure.

PPC, arm64, and IIRC s390 all have assets defined by KVM that are consumed by
the kernel at-large.  E.g. because KVM for arm64 can't be built as a module, the
kernel calls directly into KVM during boot.  IIRC, PPC has similar code.

A few years ago (wow, time flies), I was able to hide KVM internals, using #ifdef
shenanigans to deal with cases where non-KVM really truly needed to get at things
defined in kvm_host.h

https://lore.kernel.org/all/20230916003118.2540661-27-seanjc@google.com

More recently, I tried to standardize KVM arch=>common includes[1], to help pave
the way to splitting up kvm_host.h, but then s390's crazy arm64 support killed
that (at least for now).

[1] https://lore.kernel.org/all/20250611001042.170501-1-seanjc@google.com
[2] https://lore.kernel.org/all/20260428160527.1378085-1-seiden@linux.ibm.com

> > I think it's probably time to admit I've been looking at the asm/kvm_host.h vs.
> > x86.h split all wrong, i.e. finally give up on moving structures out of kvm_host.h,
> > and do the exact opposite: commit to using kvm_host.h to define and declare widely
> > used structures.
> 
> If the structure(s) are only used within arch/x86/kvm/, it doesn't seem right to
> define them in asm/kvm_host.h?

The problem is that anything that feeds into kvm_vcpu_arch needs to be visible
to virt/kvm.  And burying kvm_x86_ops in arch/kvm/x86 would mean one-liners like
kvm_arch_vcpu_blocking() couldn't be inlined.

I've looked at this far too many times :-)

> > Because literally the only reason that x86.h doesn't include mmu.h is that mmu.h
> > references struct kvm_host, which is currently defined in x86.h.  
> > 
> 
> Yes. But I wouldn't worry about this too much since it's a small thing we can
> always find a way to fix.  E.g., we can move kvm_mmu_max_gfn() out of "mmu.h"
> (with a renaming perhaps).

I hacked on moving more stuff out of x86.{c,h} and kvm_host.h.  The diff stats
are quite promising :-)

 arch/x86/include/asm/kvm_host.h           |  444 ++-------------
 arch/x86/kvm/x86.c                        | 3784 +++-----------------------------------------------------------------------------------------------------------------------
 arch/x86/kvm/x86.h                        |  474 ++++++++--------

> > If we "fix"
> > that, then (a) we can make x86.h the "central" include everyone expects it to be,
> > and (b) it can be the start of a cleanup of asm/kvm_host.h and a big step towards
> > defining maintainable "rules" for what goes where.  E.g. there are a pile of
> > functional declarations in asm/kvm_host.h that can live elsewhere; if we trim
> > those down, then the rules become:
> > 
> >   - asm/kvm_host.h holds "common" structure definitions and associated key global
> >     variables, and things that are referenced by arch-neutral KVM.
> 
> It's a bit weird the arch-neutral KVM code needs to reference variables in
> asm/kvm_host.h, and I am afraid the "common" structure definitions will
> effectively be a lot of structures only used by arch/x86/kvm/.  
> 
> Which isn't necessarily a bad thing, from the perspective we might finally clean
> this up by a giant move.
> 
> E.g., <linux/kvm_types.h> is already used by other kernel components where they
> don't need <linux/kvm_host.h>.  Ideally, maybe eventually we can use
> <linux/kvm_types.h> and <asm/kvm_types.h> for things needed by other kernel
> components, or keep <linux/kvm_host.h> and <asm/kvm_host.h> minimal after moving
> majority things to some KVM internal headers.
> 
> E.g., maybe:
> 
>   virt/kvm/include/kvm_host.h
>   arch/x86/kvm/kvm_host.h (can even be merged to x86.h)
> 
> I think the problem is "struct kvm_arch" and "struct kvm_vcpu_arch", that they
> are not a pointer but a fully embedded structure in "struct kvm" and "struct
> kvm_vcpu" respectively.  That caused that you need to keep the actual structure
> definition of "struct kvm_arch" and "kvm_vcpu_arch" in asm/kvm_host.h, which in
> turns makes a lot of structures only used by arch/x86/kvm/ need to stay in
> asm/kvm_host.h.
> 
> I am not sure whether there's a mandatory requirement that "struct kvm_arch" and
> "struct kvm_vcpu_arch" must be fully embedded, and it would be kinda painful to
> covert to a pointer (e.g., there's kvm_x86_ops::vm_size), but perhaps that is
> also an option to consider?

The idea I had in the past, and where I was going with things before s390's love
for arm64 came along, was to add a kvm_arch.h in arch/<arch>/kvm, and have virt/kvm
include _that_ instead of kvm_host.h.  That way we don't need to make any fundamental
changes to structures, but we can still significantly cut down on what's exposed
via kvm_host.h.  At some point I'll try to take another look; it's really the
s390+arm64 combo that's problematic :-/