Paolo Bonzini <pbonzini@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
