Re: [PATCH v2 4/8] KVM: x86: Add X86EMUL_F_INVTLB and pass it in em_invlpg()

[Sean Christopherson]

On Wed, Aug 16, 2023, Binbin Wu wrote:
> 
> 
> On 8/16/2023 7:11 AM, Sean Christopherson wrote:
> > On Wed, Jul 19, 2023, Zeng Guang wrote:
> > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > > index 8e706d19ae45..9b4b3ce6d52a 100644
> > > --- a/arch/x86/kvm/emulate.c
> > > +++ b/arch/x86/kvm/emulate.c
> > > @@ -3443,8 +3443,10 @@ static int em_invlpg(struct x86_emulate_ctxt *ctxt)
> > >   {
> > >   	int rc;
> > >   	ulong linear;
> > > +	unsigned max_size;
> > 	unsigned int
> Let me think why I use 'unsigned'...
> It's because the exist code uses 'unsigned'.
> I suppose it is considered bad practice?

Yeah, use "unsigned int" when writing new code.

> I will cleanup the exist code as well. Is it OK to cleanup it
> opportunistically inside this patch?

No, don't bother cleaning up existing usage.  If a patch touches the "bad" code,
then by all means do an opportunistic cleanup.  But we have too much "legacy" code
in KVM for a wholesale cleanup of bare unsigned usage to be worth the churn and
git blame pollution.  See also:

https://lore.kernel.org/all/ZNvIRS%2FYExLtGO2B@xxxxxxxxxx

> > > diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> > > index c0e48f4fa7c4..c944055091e1 100644
> > > --- a/arch/x86/kvm/kvm_emulate.h
> > > +++ b/arch/x86/kvm/kvm_emulate.h
> > > @@ -93,6 +93,7 @@ struct x86_instruction_info {
> > >   #define X86EMUL_F_FETCH			BIT(1)
> > >   #define X86EMUL_F_BRANCH		BIT(2)
> > >   #define X86EMUL_F_IMPLICIT		BIT(3)
> > > +#define X86EMUL_F_INVTLB		BIT(4)
> > Why F_INVTLB instead of X86EMUL_F_INVLPG?  Ah, because LAM is ignored for the
> > linear address in the INVPCID and INVVPID descriptors.  Hrm.
> > 
> > I think my vote is to call this X86EMUL_F_INVLPG even though *in theory* it's not
> > strictly limited to INVLPG.  Odds are good KVM's emulator will never support
> > INVPCID or INVVPID,
> One case is kvm_handle_invpcid() is in the common kvm x86 code.
> LAM doesn't apply to the address in descriptor of invpcid though, but I am
> not sure if there will be the need for SVM in the future.

Right, but the emulator itself doesn't handle INVPCID or INVVPID, so there's no
direct "conflict" at this time.

> But for now, F_INVLPG is OK if you think F_INVTLB brings confusion.

Yeah, please use F_INVLPG unless someone has a strong objection.