Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 16 Aug 2023 07:38:59 -0700
In-Reply-To: <e2662efe-9c53-77de-836c-a29076d3ccdc@linux.intel.com>
Mime-Version: 1.0
References: <20230719024558.8539-1-guang.zeng@intel.com> <20230719024558.8539-3-guang.zeng@intel.com>
 <ZNwBeN8mGr1sJJ6i@google.com> <e2662efe-9c53-77de-836c-a29076d3ccdc@linux.intel.com>
Message-ID: <ZNzfgxTnB6KYWENg@google.com>
Subject: Re: [PATCH v2 2/8] KVM: x86: Use a new flag for branch instructions
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
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Aug 16, 2023, Binbin Wu wrote:
>=20
>=20
> On 8/16/2023 6:51 AM, Sean Christopherson wrote:
> > Branch *targets*, not branch instructions.
> >=20
> > On Wed, Jul 19, 2023, Zeng Guang wrote:
> > > From: Binbin Wu <binbin.wu@linux.intel.com>
> > >=20
> > > Use the new flag X86EMUL_F_BRANCH instead of X86EMUL_F_FETCH in
> > > assign_eip(), since strictly speaking it is not behavior of instructi=
on
> > > fetch.
> > Eh, I'd just drop this paragraph, as evidenced by this code existing as=
-is for
> > years, we wouldn't introduce X86EMUL_F_BRANCH just because resolving a =
branch
> > target isn't strictly an instruction fetch.
> >=20
> > > Another reason is to distinguish instruction fetch and execution of b=
ranch
> > > instruction for feature(s) that handle differently on them.
> > Similar to the shortlog, it's about computing the branch target, not ex=
ecuting a
> > branch instruction.  That distinction matters, e.g. a Jcc that is not t=
aken will
> > *not* follow the branch target, but the instruction is still *executed*=
.  And there
> > exist instructions that compute branch targets, but aren't what most pe=
ople would
> > typically consider a branch instruction, e.g. XBEGIN.
> >=20
> > > Branch instruction is not data access instruction, so skip checking a=
gainst
> > > execute-only code segment as instruction fetch.
> > Rather than call out individual use case, I would simply state that as =
of this
> > patch, X86EMUL_F_BRANCH and X86EMUL_F_FETCH are identical as far as KVM=
 is
> > concernered.  That let's the reader know that (a) there's no intended c=
hange in
> > behavior and (b) that the intent is to effectively split all consumptio=
n of
> > X86EMUL_F_FETCH into (X86EMUL_F_FETCH | X86EMUL_F_BRANCH).
>=20
> How about this:
>=20
> =C2=A0=C2=A0=C2=A0 KVM: x86: Use a new flag for branch targets
>=20
> =C2=A0=C2=A0=C2=A0 Use the new flag X86EMUL_F_BRANCH instead of X86EMUL_F=
_FETCH in
> assign_eip()
> =C2=A0=C2=A0=C2=A0 to distinguish instruction fetch and branch target com=
putation for
> feature(s)

Just "features", i.e. no parentheses...

> =C2=A0=C2=A0=C2=A0 that handle differently on them.

...and tack on ", e.g. LASS and LAM." at the end.  There's zero reason not =
to more
explicitly call out why the flag is being added.  Trying to predict the fut=
ure in
changelogs is generally discouraged, but having understandable changelogs i=
s more
important.

> =C2=A0=C2=A0=C2=A0 As of this patch, X86EMUL_F_BRANCH and X86EMUL_F_FETCH=
 are identical as
> far as
> =C2=A0=C2=A0=C2=A0 KVM is concernered.
>=20
> =C2=A0=C2=A0=C2=A0 No functional change intended.

Heh, you need to fix whatever is forcefully wrapping lines, but other than =
the
nit above, the content itself is good.