| Message ID | 20230719024558.8539-3-guang.zeng@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LASS KVM virtualization support | expand |
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). > 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. /* unreadable code segment */ if (!(flags & (X86EMUL_F_FETCH | X86EMUL_F_BRANCH)) && (desc.type & 8) && !(desc.type & 2)) goto bad;
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;
On Wed, Aug 16, 2023, Binbin Wu wrote: > > > 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) Just "features", i.e. no parentheses... > 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 future in changelogs is generally discouraged, but having understandable changelogs is more important. > As of this patch, X86EMUL_F_BRANCH and X86EMUL_F_FETCH are identical as > far as > KVM is concernered. > > 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.
On 8/16/2023 10:38 PM, Sean Christopherson wrote: > On Wed, Aug 16, 2023, Binbin Wu wrote: >> >> 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) > Just "features", i.e. no parentheses... > >> that handle differently on them. > ...and tack on ", e.g. LASS and LAM." at the end. OK, but only LASS here, since LAM only applies to addresses for data accesses, i.e, no need to distingush the two flag. > There's zero reason not to more > explicitly call out why the flag is being added. Trying to predict the future in > changelogs is generally discouraged, but having understandable changelogs is more > important. > >> As of this patch, X86EMUL_F_BRANCH and X86EMUL_F_FETCH are identical as >> far as >> KVM is concernered. >> >> 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. Sure, I think the wrapping lines due to additional intendations I added, it should be OK in changelog.
On Thu, Aug 17, 2023, Binbin Wu wrote: > > > On 8/16/2023 10:38 PM, Sean Christopherson wrote: > > On Wed, Aug 16, 2023, Binbin Wu wrote: > > > > > > On 8/16/2023 6:51 AM, Sean Christopherson wrote: > > > > 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) > > Just "features", i.e. no parentheses... > > > > > that handle differently on them. > > ...and tack on ", e.g. LASS and LAM." at the end. > OK, but only LASS here, since LAM only applies to addresses for data > accesses, i.e, no need to distingush the two flag. Oh, right. Thanks!
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)) goto bad; lim = desc_limit_scaled(&desc); if (!(desc.type & 8) && (desc.type & 4)) { @@ -772,7 +773,7 @@ static inline int assign_eip(struct x86_emulate_ctxt *ctxt, ulong dst) if (ctxt->op_bytes != sizeof(unsigned long)) addr.ea = dst & ((1UL << (ctxt->op_bytes << 3)) - 1); rc = __linearize(ctxt, addr, &max_size, 1, ctxt->mode, &linear, - X86EMUL_F_FETCH); + X86EMUL_F_BRANCH); if (rc == X86EMUL_CONTINUE) ctxt->_eip = addr.ea; return rc; diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h index 86bbe997162d..9fc7d34a4ac1 100644 --- a/arch/x86/kvm/kvm_emulate.h +++ b/arch/x86/kvm/kvm_emulate.h @@ -91,6 +91,7 @@ struct x86_instruction_info { /* x86-specific emulation flags */ #define X86EMUL_F_WRITE BIT(0) #define X86EMUL_F_FETCH BIT(1) +#define X86EMUL_F_BRANCH BIT(2) struct x86_emulate_ops { void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);