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 08:04 -0700, Sean Christopherson wrote:
> On Tue, May 19, 2026, Kai Huang wrote:
> > > @@ -12712,19 +11913,8 @@ static void store_regs(struct kvm_vcpu *vcpu)
> > >  
> > >  static int sync_regs(struct kvm_vcpu *vcpu)
> > >  {
> > > -	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_REGS) {
> > > -		__set_regs(vcpu, &vcpu->run->s.regs.regs);
> > > -		vcpu->run->kvm_dirty_regs &= ~KVM_SYNC_X86_REGS;
> > > -	}
> > > -
> > > -	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_SREGS) {
> > > -		struct kvm_sregs sregs = vcpu->run->s.regs.sregs;
> > > -
> > > -		if (__set_sregs(vcpu, &sregs))
> > > -			return -EINVAL;
> > > -
> > > -		vcpu->run->kvm_dirty_regs &= ~KVM_SYNC_X86_SREGS;
> > > -	}
> > > +	if (kvm_run_set_regs(vcpu))
> > > +		return -EINVAL;
> > 
> > Nit:
> > 
> > Do you think 'kvm_run_sync_regs()' is better than 'kvm_run_set_regs()'?
> > 
> > Because I think "sync" reflects better that vcpu->run->kvm_dirty_regs is cleared
> > after the "set" operation.
> 
> The problem I have with "sync" is that it doesn't communicate the direction of
> the sync.  What about kvm_run_sync_regs_{to,from}_user()?

Yeah that's better to me too.

> 
> > >  
> > >  	if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_EVENTS) {
> > >  		struct kvm_vcpu_events events = vcpu->run->s.regs.events;
> > 
> > Also, I wonder whether it's better to add a helper for events so sync_regs() and
> > store_regs() can be simplified to:
> > 
> > static int sync_regs(struct kvm_vcpu *vcpu)
> > {
> > 	if (kvm_run_sync_regs(vcpu))
> > 		return -EINVAL;
> > 	return kvm_run_sync_events(vcpu);
> > }
> > 
> > static void store_regs(struct kvm_vcpu *vcpu)
> > {
> > 	kvm_run_get_regs(vcpu);
> > 	kvm_run_get_events(vcpu);
> > }
> > 
> > And maybe 'kvm_run_get_regs()' could be 'kvm_run_store_regs()' too , so that the
> > store_regs() could be:
> > 
> > static void store_regs(struct kvm_vcpu *vcpu)
> > {
> > 	kvm_run_store_regs(vcpu);
> > 	kvm_run_store_events(vcpu);
> > }
> 
> {store,sync}_regs() look pretty, but IMO the overall code is uglier because we
> end up with super small helpers that have one caller, e.g.
> 
>   static void kvm_run_sync_events_to_user(struct kvm_vcpu *vcpu)
>   {
> 	if (vcpu->run->kvm_valid_regs & KVM_SYNC_X86_EVENTS)
> 		kvm_vcpu_ioctl_x86_get_vcpu_events(vcpu, &vcpu->run->s.regs.events);
>   }
> 
>   static void store_regs(struct kvm_vcpu *vcpu)
>   {
> 	BUILD_BUG_ON(sizeof(struct kvm_sync_regs) > SYNC_REGS_SIZE_BYTES);
> 
> 	kvm_run_sync_regs_to_user(vcpu);
> 	kvm_run_sync_events_to_user(vcpu);
>   }
> 
> For me, the extra "jump" is undesirable, but it allows burying __{g,s}et_{s,}regs()
> in regs.c, and so is a net positive for registers.  But for events, it's pure
> overhead.

Sure.

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.

> 
> > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > > index 185062a26924..fd55cd031b1c 100644
> > > --- a/arch/x86/kvm/x86.h
> > > +++ b/arch/x86/kvm/x86.h
> > > @@ -414,6 +414,7 @@ int handle_ud(struct kvm_vcpu *vcpu);
> > >  
> > >  void kvm_deliver_exception_payload(struct kvm_vcpu *vcpu,
> > >  				   struct kvm_queued_exception *ex);
> > > +void kvm_handle_exception_payload_quirk(struct kvm_vcpu *vcpu);
> > >  
> > >  int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data);
> > >  int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata);
> > > @@ -604,6 +605,7 @@ static inline void kvm_machine_check(void)
> > >  int kvm_spec_ctrl_test_value(u64 value);
> > >  int kvm_handle_memory_failure(struct kvm_vcpu *vcpu, int r,
> > >  			      struct x86_exception *e);
> > > +void kvm_invalidate_pcid(struct kvm_vcpu *vcpu, unsigned long pcid);
> > > 
> > 
> > If I read correct, this is because "regs.c" calls kvm_invalidate_pcid() but you
> > want to keep it in x86.c.  But it seems the "x86.h" isn't included by "regs.c"
> > directly but via other headers ("mmu.h" does include "x86.h").
> > 
> > Should the "regs.c" include "x86.h" directly?
> 
> Oh, yeah, I just goofed that.
> 
> > Btw, I am a bit confused the relationship between "x86.h" and other headers like
> > "mmu.h" and the new "regs.h".  That is, headers like "mmu.h" include "x86.h",
> > but headers like "regs.h" do not (instead, "x86.h" includes them).
> 
> Heh, don't look for a theme/plan, because there isn't one.  Over the years, x86.h
> and x86.c became dumping grounds for everything that didn't have an obvious home,
> and so there aren't real "rules".

My guess too.

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

For x86, AFAICT there's (only) "arch/x86/events/intel/core.c" actually uses the
'struct kvm_pmu', though.  I haven't checked other ARCHs whether there's cases
actually need to use any structure.

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

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

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

>   - <area>.{c,h} holds relevant declarations and definitions.
>   - x86.{c,h} is the kitchen sink for everything else.

Yeah the two are reasonable to me.