| Message ID | 1399400175-23754-3-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Paolo Bonzini <pbonzini@redhat.com> writes: > do_insn_fetch_bytes will only be called once in a given insn_fetch and > insn_fetch_arr, because in fact it will only be called at most twice > for any instruction and the first call is explicit in x86_decode_insn. > This observation lets us hoist the call out of the memory copying loop. > It does not buy performance, because most fetches are one byte long > anyway, but it prepares for the next patch. > > The overflow check is tricky, but correct. Because do_insn_fetch_bytes > has already been called once, we know that fc->end is at least 15. So > it is okay to subtract the number of bytes we want to read. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/emulate.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index c7b625bf0b5d..886f9a88010f 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -706,7 +706,7 @@ static int segmented_read_std(struct x86_emulate_ctxt *ctxt, > * Prefetch the remaining bytes of the instruction without crossing page > * boundary if they are not in fetch_cache yet. > */ > -static int do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt) > +static int do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, int op_size) > { > struct fetch_cache *fc = &ctxt->fetch; > int rc; > @@ -718,7 +718,14 @@ static int do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt) > cur_size = fc->end - fc->start; > size = min(15UL - cur_size, > PAGE_SIZE - offset_in_page(fc->end)); > - if (unlikely(size == 0)) > + > + /* > + * One instruction can only straddle two pages, > + * and one has been loaded at the beginning of > + * x86_decode_insn. So, if not enough bytes > + * still, we must have hit the 15-byte boundary. > + */ > + if (unlikely(size < op_size)) > return X86EMUL_UNHANDLEABLE; > rc = __linearize(ctxt, addr, size, false, true, &linear); > if (unlikely(rc != X86EMUL_CONTINUE)) > @@ -739,12 +746,14 @@ static int do_insn_fetch(struct x86_emulate_ctxt *ctxt, > u8 *dest = __dest; > u8 *src = &fc->data[ctxt->_eip - fc->start]; > > + /* We have to be careful about overflow! */ > + if (unlikely(ctxt->_eip > fc->end - size)) { > + rc != do_insn_fetch_bytes(ctxt, size); This is a typo I guess .. > + if (rc != X86EMUL_CONTINNUE) > + goto done; > + } > + > while (size--) { > - if (unlikely(ctxt->_eip == fc->end)) { > - rc = do_insn_fetch_bytes(ctxt); > - if (rc != X86EMUL_CONTINUE) > - return rc; > - } > *dest++ = *src++; > ctxt->_eip++; > continue; > @@ -4273,7 +4282,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len) > if (insn_len > 0) > memcpy(ctxt->fetch.data, insn, insn_len); > else { > - rc = do_insn_fetch_bytes(ctxt); > + rc = do_insn_fetch_bytes(ctxt, 1); Is this saying that if the cache is full, then we fetch one more byte ? > if (rc != X86EMUL_CONTINUE) > return rc; > } -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 07/05/2014 06:21, Bandan Das ha scritto: >> > + if (rc != X86EMUL_CONTINNUE) >> > + goto done; >> > + } >> > + >> > while (size--) { >> > - if (unlikely(ctxt->_eip == fc->end)) { >> > - rc = do_insn_fetch_bytes(ctxt); >> > - if (rc != X86EMUL_CONTINUE) >> > - return rc; >> > - } >> > *dest++ = *src++; >> > ctxt->_eip++; >> > continue; >> > @@ -4273,7 +4282,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len) >> > if (insn_len > 0) >> > memcpy(ctxt->fetch.data, insn, insn_len); >> > else { >> > - rc = do_insn_fetch_bytes(ctxt); >> > + rc = do_insn_fetch_bytes(ctxt, 1); > Is this saying that if the cache is full, then we fetch one more byte ? > No, it is saying that if the instruction is being executed for the first time (we can execute it multiple times if we reenter a repeated instruction after a userspace exit) we try to get at least one byte from RIP. Most of the time, do_insn_fetch_bytes will fetch 15 bytes which are the maximum length of an instruction. Passing op_size == 1 matches this change in do_insn_fetch_bytes: - if (unlikely(size == 0)) + if (unlikely(size < op_size)) Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index c7b625bf0b5d..886f9a88010f 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -706,7 +706,7 @@ static int segmented_read_std(struct x86_emulate_ctxt *ctxt, * Prefetch the remaining bytes of the instruction without crossing page * boundary if they are not in fetch_cache yet. */ -static int do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt) +static int do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt, int op_size) { struct fetch_cache *fc = &ctxt->fetch; int rc; @@ -718,7 +718,14 @@ static int do_insn_fetch_bytes(struct x86_emulate_ctxt *ctxt) cur_size = fc->end - fc->start; size = min(15UL - cur_size, PAGE_SIZE - offset_in_page(fc->end)); - if (unlikely(size == 0)) + + /* + * One instruction can only straddle two pages, + * and one has been loaded at the beginning of + * x86_decode_insn. So, if not enough bytes + * still, we must have hit the 15-byte boundary. + */ + if (unlikely(size < op_size)) return X86EMUL_UNHANDLEABLE; rc = __linearize(ctxt, addr, size, false, true, &linear); if (unlikely(rc != X86EMUL_CONTINUE)) @@ -739,12 +746,14 @@ static int do_insn_fetch(struct x86_emulate_ctxt *ctxt, u8 *dest = __dest; u8 *src = &fc->data[ctxt->_eip - fc->start]; + /* We have to be careful about overflow! */ + if (unlikely(ctxt->_eip > fc->end - size)) { + rc != do_insn_fetch_bytes(ctxt, size); + if (rc != X86EMUL_CONTINNUE) + goto done; + } + while (size--) { - if (unlikely(ctxt->_eip == fc->end)) { - rc = do_insn_fetch_bytes(ctxt); - if (rc != X86EMUL_CONTINUE) - return rc; - } *dest++ = *src++; ctxt->_eip++; continue; @@ -4273,7 +4282,7 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len) if (insn_len > 0) memcpy(ctxt->fetch.data, insn, insn_len); else { - rc = do_insn_fetch_bytes(ctxt); + rc = do_insn_fetch_bytes(ctxt, 1); if (rc != X86EMUL_CONTINUE) return rc; }
do_insn_fetch_bytes will only be called once in a given insn_fetch and insn_fetch_arr, because in fact it will only be called at most twice for any instruction and the first call is explicit in x86_decode_insn. This observation lets us hoist the call out of the memory copying loop. It does not buy performance, because most fetches are one byte long anyway, but it prepares for the next patch. The overflow check is tricky, but correct. Because do_insn_fetch_bytes has already been called once, we know that fc->end is at least 15. So it is okay to subtract the number of bytes we want to read. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/emulate.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)