From: Sean Christopherson <seanjc@google.com>
To: Binbin Wu <binbin.wu@linux.intel.com>
Cc: Zeng Guang <guang.zeng@intel.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
Dave Hansen <dave.hansen@linux.intel.com>,
H Peter Anvin <hpa@zytor.com>,
kvm@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/8] KVM: x86: Add X86EMUL_F_INVTLB and pass it in em_invlpg()
Date: Wed, 16 Aug 2023 07:27:44 -0700 [thread overview]
Message-ID: <ZNzc4FMukTamEseJ@google.com> (raw)
In-Reply-To: <a7ecab8d-a77c-77eb-68cb-383de569fe6d@linux.intel.com>
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@google.com
> > > 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.
next prev parent reply other threads:[~2023-08-16 14:28 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-19 2:45 [PATCH v2 0/8] LASS KVM virtualization support Zeng Guang
2023-07-19 2:45 ` [PATCH v2 1/8] KVM: x86: Consolidate flags for __linearize() Zeng Guang
2023-07-19 2:45 ` [PATCH v2 2/8] KVM: x86: Use a new flag for branch instructions Zeng Guang
2023-08-15 22:51 ` Sean Christopherson
2023-08-16 7:34 ` Binbin Wu
2023-08-16 14:38 ` Sean Christopherson
2023-08-17 1:38 ` Binbin Wu
2023-08-17 14:45 ` Sean Christopherson
2023-07-19 2:45 ` [PATCH v2 3/8] KVM: x86: Add an emulation flag for implicit system access Zeng Guang
2023-07-19 2:45 ` [PATCH v2 4/8] KVM: x86: Add X86EMUL_F_INVTLB and pass it in em_invlpg() Zeng Guang
2023-08-15 23:11 ` Sean Christopherson
2023-08-16 7:55 ` Binbin Wu
2023-08-16 14:27 ` Sean Christopherson [this message]
2023-07-19 2:45 ` [PATCH v2 5/8] KVM: emulator: Add emulation of LASS violation checks on linear address Zeng Guang
2023-07-19 2:45 ` [PATCH v2 6/8] KVM: VMX: Implement and apply vmx_is_lass_violation() for LASS protection Zeng Guang
2023-08-07 7:03 ` Binbin Wu
2023-08-15 23:46 ` Sean Christopherson
2023-08-17 16:15 ` Zeng Guang
2023-07-19 2:45 ` [PATCH v2 7/8] KVM: x86: Virtualize CR4.LASS Zeng Guang
2023-07-19 2:45 ` [PATCH v2 8/8] KVM: x86: Advertise LASS CPUID to user space Zeng Guang
-- strict thread matches above, loose matches on Subject: below --
2023-07-18 13:18 [PATCH v2 0/8] LASS KVM virtualization support Zeng Guang
2023-07-18 13:18 ` [PATCH v2 4/8] KVM: x86: Add X86EMUL_F_INVTLB and pass it in em_invlpg() Zeng Guang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZNzc4FMukTamEseJ@google.com \
--to=seanjc@google.com \
--cc=binbin.wu@linux.intel.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=guang.zeng@intel.com \
--cc=hpa@zytor.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox