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;
Message-ID: <e2662efe-9c53-77de-836c-a29076d3ccdc@linux.intel.com>
Date:   Wed, 16 Aug 2023 15:34:15 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH v2 2/8] KVM: x86: Use a new flag for branch instructions
To:     Sean Christopherson <seanjc@google.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
References: <20230719024558.8539-1-guang.zeng@intel.com>
 <20230719024558.8539-3-guang.zeng@intel.com> <ZNwBeN8mGr1sJJ6i@google.com>
From:   Binbin Wu <binbin.wu@linux.intel.com>
In-Reply-To: <ZNwBeN8mGr1sJJ6i@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 8/16/2023 6:51 AM, Sean Christopherson wrote:
> Branch *targets*, not branch instructions.
>
> On Wed, Jul 19, 2023, Zeng Guang wrote:
>> From: Binbin Wu <binbin.wu@linux.intel.com>
>>
>> Use the new flag X86EMUL_F_BRANCH instead of X86EMUL_F_FETCH in
>> assign_eip(), since strictly speaking it is not behavior of instruction
>> 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.
>
>> Another reason is to distinguish instruction fetch and execution of branch
>> instruction for feature(s) that handle differently on them.
> Similar to the shortlog, it's about computing the branch target, not executing a
> branch instruction.  That distinction matters, e.g. a Jcc that is not taken 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 people would
> typically consider a branch instruction, e.g. XBEGIN.
>
>> Branch instruction is not data access instruction, so skip checking against
>> 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 change in
> behavior and (b) that the intent is to effectively split all consumption of
> X86EMUL_F_FETCH into (X86EMUL_F_FETCH | X86EMUL_F_BRANCH).

How about this:

     KVM: x86: Use a new flag for branch targets

     Use the new flag X86EMUL_F_BRANCH instead of X86EMUL_F_FETCH in 
assign_eip()
     to distinguish instruction fetch and branch target computation for 
feature(s)
     that handle differently on them.

     As of this patch, X86EMUL_F_BRANCH and X86EMUL_F_FETCH are 
identical as far as
     KVM is concernered.

     No functional change intended.


>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> Signed-off-by: Zeng Guang <guang.zeng@intel.com>
>> ---
>>   arch/x86/kvm/emulate.c     | 5 +++--
>>   arch/x86/kvm/kvm_emulate.h | 1 +
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 3ddfbc99fa4f..8e706d19ae45 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -721,7 +721,8 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt,
>>   		    (flags & X86EMUL_F_WRITE))
>>   			goto bad;
>>   		/* unreadable code segment */
>> -		if (!(flags & X86EMUL_F_FETCH) && (desc.type & 8) && !(desc.type & 2))
>> +		if (!(flags & (X86EMUL_F_FETCH | X86EMUL_F_BRANCH))
>> +			&& (desc.type & 8) && !(desc.type & 2))
> Put the && on the first line, and align indendation.
I should have been more careful on the alignment & indentation.
Will update it. Thanks.

>
> 		/* unreadable code segment */
> 		if (!(flags & (X86EMUL_F_FETCH | X86EMUL_F_BRANCH)) &&
> 		    (desc.type & 8) && !(desc.type & 2))
> 			goto bad;