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

[Huang, Kai]

On Tue, 2026-05-19 at 18:25 -0700, Sean Christopherson wrote:
> 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.

Yes agreed.

> 
> > > 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@xxxxxxxxxx

Oh great!

> 
> > 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@xxxxxxxxxx

Oh I never thought from this perspective (thanks for the info):

  --
  Hiding KVM details for all architectures will, in the very distant future, 
  allow loading a new (or old) KVM module without needing to rebuild and reboot 
  the entire kernel, or to even allow loading and running multiple versions of 
  KVM simultaneously on a single host.
  --

> 
> 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@xxxxxxxxxx
> [2] https://lore.kernel.org/all/20260428160527.1378085-1-seiden@xxxxxxxxxxxxx

:-)

> 
> > > 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.  
> 

Yeah that's the problem.

> And burying kvm_x86_ops in arch/kvm/x86 would mean one-liners like
> kvm_arch_vcpu_blocking() couldn't be inlined.

Oh right, sad but acceptable tradeoff I guess.

> 
> 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 ++++++++--------
> 

Indeed!

> > > 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.  
> 

Not sure whether there's other code doing so? :-)

> 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.  
> 

Yeah.

I saw below from you in [1]:

  --
  We've explore several alternatives to the #ifdef __KVM__ approach, and
  they all sucked, hard.  What I really wanted (and still want) to do, is to
  bury the bulk of kvm_host.h (and other KVM headers) in virt/kvm, but every
  attempt to do that ended in flames.  Even with the __KVM__ guards in place,
  each architecture's kvm_host.h is too intertwined with the common kvm_host.h,
  and trying to extract small-ish pieces just doesn't work (each patch
  inevitably snowballed into a gigantic beast).

  The other idea we considered (which I thought of, and feel dirty for even
  proposing it internally), is to move all headers under virt/kvm, add
  virt/kvm/include to the global header path, and then have KVM x86 omit
  virt/kvm/include when configured to hide KVM internals.  I hate this idea
  because it sets a bad precedent, and requires a lot of file movement
  without providing any benefit to other architectures.  E.g. I hope that
  guarding KVM internals with #ifdef __KVM__ will allow us to slowly clean
  things up so that some day KVM only exposes a handful of APIs to the rest
  of the kernel (probably a pipe dream).
  --

I haven't looked into details of your #ifdef __KVM__ approach yet, but seems you
don't quite like moving KVM internal staff to virt/kvm/include/ ?

But if we want to hide KVM internal structures, I don't see any other options
except virt/kvm/include/ is the place to go?

Btw, have you considered reverting the inclusion of "strut kvm" and "struct
kvm_arch" (and the vcpu structure), i.e., to make "struct kvm_arch" include
"struct kvm"?  I don't have any clue of whether it is feasible or how much
effort it needs, though -- it's just something came to mind when replying.

[1]: https://lore.kernel.org/all/20230916003118.2540661-1-seanjc@xxxxxxxxxx/

> At some point I'll try to take another look; it's really the
> s390+arm64 combo that's problematic :-/

If you want, I can take a look.  I think I'll have bandwidth in near feature.

Given you have tried multiple times so I am not sure what I can achieve, though.

Anyway, seems "allow loading a new (or old) KVM module without needing to
rebuild and reboot the entire kernel" is a good reason to do this.