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

[Huang, Kai]

On Wed, 2026-05-20 at 11:11 -0700, Sean Christopherson wrote:
> On Wed, May 20, 2026, Kai Huang wrote:
> > On Tue, 2026-05-19 at 18:25 -0700, Sean Christopherson wrote:
> > > On Wed, May 20, 2026, Kai Huang wrote:
> > > > 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/ ?
> 
> Not for arch code, which is the trickiest bit to handle.
> 
> > 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?
> 
> arch/$(ARCH)/kvm/kvm_arch.h is the obvious approach.  Code in virt/kvm can reach
> arch/$(ARCH)/kvm, we just need to add it to the include path.  That's why I was
> working on unifying the include definitions.

Yeah, for asm/kvm_host.h.

But if I am still following you we still need a place for linux/kvm_host.h, for
which I thought virt/kvm/include/ would be the place at first glance.

> 
> > 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.
> 
> Consolidating includes and creating arch/$(ARCH)/kvm/kvm_arch.h should be more
> doable, 
> 

Right.

> what has failed spectacularly over and over is effectively trying to hide
> most of asm/kvm_host.h, which sounds *really* stupid when I phrase it that way :-)

I see :-)

> 
> > 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.
> 
> Note, we've scrapped upstreaming multi-KVM, and I don't see it ever getting accepted
> upstream, as live update is far superior, e.g. isn't limited to just KVM, doesn't
> have the same orchestration or testing problems, etc.
> 
> And FWIW, today, you _can_ reload a new (or old) KVM module without rebuilding
> the kernel or rebooting the host, it's just that you can't modify certain structures.

Right.