Re: [PATCH v2 1/5] KVM: x86: Centralize KVM's VMware code

[Sean Christopherson <seanjc@google.com>]

+lists

Please keep all replies on-list, no matter how trivial the question/comment.
Pretty much the only time it's ok to take something off-list is if the conversation
is something that can't/shouldn't be had in public.

On Wed, Jul 23, 2025, Zack Rusin wrote:
> On Wed, Jul 23, 2025 at 1:48 PM Sean Christopherson <seanjc@google.com> wrote:
> > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > > index 60986f67c35a..b42988ce8043 100644
> > > --- a/arch/x86/kvm/emulate.c
> > > +++ b/arch/x86/kvm/emulate.c
> > > @@ -26,6 +26,7 @@
> > >  #include <asm/debugreg.h>
> > >  #include <asm/nospec-branch.h>
> > >  #include <asm/ibt.h>
> > > +#include "kvm_vmware.h"
> >
> > Please sort includes as best as possible.  KVM's loose rule is to organize by
> > linux => asm => local, and sort alphabetically within each section, e.g.
> >
> > #include <linux/aaaa.h>
> > #include <linux/blah.h>
> >
> > #include <asm/aaaa.h>
> > #include <asm/blah.h>
> >
> > #include "aaaa.h"
> > #include "blah.h"
> 
> Yea, that's what I do in my code but in this case I had no idea where
> to put it because none of the sections in that file are sorted, where
> would you like the include among:
> ```
> #include <linux/kvm_host.h>
> #include "kvm_cache_regs.h"
> #include "kvm_emulate.h"
> #include <linux/stringify.h>
> #include <asm/debugreg.h>
> #include <asm/nospec-branch.h>
> #include <asm/ibt.h>
> 
> #include "x86.h"
> #include "tss.h"
> #include "mmu.h"
> #include "pmu.h"
> ```
> below kvm_emulate or would you like me to resort all the includes?

Nah, don't bother sorting them all (though that might happen anyways[*]).  Just
do your best to not make things worse.  Luckily, 'v' is quite near the end, so I
think the least-awful option will be fairly obvious in all/most cases, e.g.

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 78e0064dd73e..9b7e71f4e26f 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -26,12 +26,12 @@
 #include <asm/debugreg.h>
 #include <asm/nospec-branch.h>
 #include <asm/ibt.h>
-#include "kvm_vmware.h"
 
 #include "x86.h"
 #include "tss.h"
 #include "mmu.h"
 #include "pmu.h"
+#include "vmware.h"
 
 /*
  * Operand types
---

[*] https://lore.kernel.org/lkml/aH-dqcMWj3cFDos2@google.com

> > > @@ -2565,8 +2563,8 @@ static bool emulator_io_port_access_allowed(struct x86_emulate_ctxt *ctxt,
> > >        * VMware allows access to these ports even if denied
> > >        * by TSS I/O permission bitmap. Mimic behavior.
> > >        */
> > > -     if (enable_vmware_backdoor &&
> > > -         ((port == VMWARE_PORT_VMPORT) || (port == VMWARE_PORT_VMRPC)))
> > > +     if (kvm_vmware_backdoor_enabled(ctxt->vcpu) &&
> >
> > Maybe kvm_is_vmware_backdoor_enabled()?  To make it super clear it's a predicate.
> >
> > Regarding namespacing, I think for the "is" predicates, the code reads better if
> > it's kvm_is_vmware_xxx versus kvm_vware_is_xxx.  E.g. is the VMware backdoor
> > enabled vs. VMware is the backdoor enabled.  Either way is fine for me if someone
> > has a strong preference though.
> >
> > > +         kvm_vmware_io_port_allowed(port))
> >
> > Please separate the addition of helpers from the code movement.  That way the
> > code movement patch can be acked/reviewed super easily, and then we can focus on
> > the helpers (and it also makes it much easier to review the helpers changes).
> 
> Sorry, I'm confused about this one. I find it a lot easier to review
> helpers if I know what code they're supposed to replace and that's
> harder to do if a change is just adding some code without any
> indication of where it's coming from but I'm happy to adjust this in
> whatever way is easiest for you.

All I'm saying is do the initial bulk, pure code movement in a single patch,
with no other changes whatsoever.  And then add and rename helpers in a separate
patch(es).  The add/renames can go before/after the code movement, what I care
most about is isolating the pure code movement.

I'm guessing the cleanest approach would be to do pure code movement, then do
renames, and finally add helpers.  That'll require an ugly double move of the
VMWARE_PORT_VMPORT and VMWARE_PORT_VMRPC #defines to vmware.h, and then again to
vmware.c.  But while annoying, that makes the individual patches super easy to
review and apply.

Holler if it's still unclear, I can put together a quick comple of example patches.