Re: [PATCH 2/9] KVM: x86: simplify read_emulated
On 07/21/2012 03:52 AM, Marcelo Tosatti wrote:
> On Fri, Jul 20, 2012 at 09:15:44PM +0800, Xiao Guangrong wrote:
>> On 07/20/2012 06:58 PM, Marcelo Tosatti wrote:
>>> On Fri, Jul 20, 2012 at 10:17:36AM +0800, Xiao Guangrong wrote:
On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
>> -}
>> +rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end,
>> size,
>> + &ctxt->exception);
>> +if (rc != X86EMUL_CONTINUE)
>> +return rc;
>> +
>> +mc->end += size;
>> +
>> +read_cached:
>> +memcpy(dest, mc->data + mc->pos, size);
>
> What prevents read_emulated(size > 8) call, with
> mc->pos == (mc->end - 8) now?
Marcelo,
The splitting has been done in emulator_read_write_onepage:
while (bytes) {
unsigned now = min(bytes, 8U);
frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
frag->gpa = gpa;
frag->data = val;
frag->len = now;
frag->write_readonly_mem = (ret == -EPERM);
gpa += now;
val += now;
bytes -= now;
}
So i think it is safe to remove the splitting in read_emulated.
>>>
>>> Yes, it is fine to remove it.
>>>
>>> But splitting in emulate.c prevented the case of _cache read_ with size
8 beyond end of mc->data. Must handle that case in read_emulated.
>>>
>>> "What prevents read_emulated(size > 8) call, with mc->pos == (mc->end - 8)
>>> now?"
>>
>> You mean the mmio region is partly cached?
>>
>> I think it can not happen. Now, we pass the whole size to
>> emulator_read_write_onepage(),
>> after it is finished, it saves the whole data into mc->data[], so, the
>> cache-read
>> can always get the whole data from mc->data[].
>
> I mean that nothing prevents a caller from reading beyond the end of
> mc->data array (but then again this was the previous behavior).
1024 bytes should be enough for instructions, may be we can add a WARN_ON
to check buffer-overflow.
>
> ACK
>
Thank you, Marcelo!
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] KVM: x86: simplify read_emulated
On Fri, Jul 20, 2012 at 09:15:44PM +0800, Xiao Guangrong wrote:
> On 07/20/2012 06:58 PM, Marcelo Tosatti wrote:
> > On Fri, Jul 20, 2012 at 10:17:36AM +0800, Xiao Guangrong wrote:
> >> On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
> >>
> -}
> +rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end,
> size,
> + &ctxt->exception);
> +if (rc != X86EMUL_CONTINUE)
> +return rc;
> +
> +mc->end += size;
> +
> +read_cached:
> +memcpy(dest, mc->data + mc->pos, size);
> >>>
> >>> What prevents read_emulated(size > 8) call, with
> >>> mc->pos == (mc->end - 8) now?
> >>
> >> Marcelo,
> >>
> >> The splitting has been done in emulator_read_write_onepage:
> >>
> >>while (bytes) {
> >>unsigned now = min(bytes, 8U);
> >>
> >>frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
> >>frag->gpa = gpa;
> >>frag->data = val;
> >>frag->len = now;
> >>frag->write_readonly_mem = (ret == -EPERM);
> >>
> >>gpa += now;
> >>val += now;
> >>bytes -= now;
> >>}
> >>
> >> So i think it is safe to remove the splitting in read_emulated.
> >
> > Yes, it is fine to remove it.
> >
> > But splitting in emulate.c prevented the case of _cache read_ with size
> >> 8 beyond end of mc->data. Must handle that case in read_emulated.
> >
> > "What prevents read_emulated(size > 8) call, with mc->pos == (mc->end - 8)
> > now?"
>
> You mean the mmio region is partly cached?
>
> I think it can not happen. Now, we pass the whole size to
> emulator_read_write_onepage(),
> after it is finished, it saves the whole data into mc->data[], so, the
> cache-read
> can always get the whole data from mc->data[].
I mean that nothing prevents a caller from reading beyond the end of
mc->data array (but then again this was the previous behavior).
ACK
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] KVM: x86: simplify read_emulated
On 07/20/2012 06:58 PM, Marcelo Tosatti wrote:
> On Fri, Jul 20, 2012 at 10:17:36AM +0800, Xiao Guangrong wrote:
>> On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
>>
- }
+ rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+&ctxt->exception);
+ if (rc != X86EMUL_CONTINUE)
+ return rc;
+
+ mc->end += size;
+
+read_cached:
+ memcpy(dest, mc->data + mc->pos, size);
>>>
>>> What prevents read_emulated(size > 8) call, with
>>> mc->pos == (mc->end - 8) now?
>>
>> Marcelo,
>>
>> The splitting has been done in emulator_read_write_onepage:
>>
>> while (bytes) {
>> unsigned now = min(bytes, 8U);
>>
>> frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
>> frag->gpa = gpa;
>> frag->data = val;
>> frag->len = now;
>> frag->write_readonly_mem = (ret == -EPERM);
>>
>> gpa += now;
>> val += now;
>> bytes -= now;
>> }
>>
>> So i think it is safe to remove the splitting in read_emulated.
>
> Yes, it is fine to remove it.
>
> But splitting in emulate.c prevented the case of _cache read_ with size
>> 8 beyond end of mc->data. Must handle that case in read_emulated.
>
> "What prevents read_emulated(size > 8) call, with mc->pos == (mc->end - 8)
> now?"
You mean the mmio region is partly cached?
I think it can not happen. Now, we pass the whole size to
emulator_read_write_onepage(),
after it is finished, it saves the whole data into mc->data[], so, the
cache-read
can always get the whole data from mc->data[].
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] KVM: x86: simplify read_emulated
On Fri, Jul 20, 2012 at 10:17:36AM +0800, Xiao Guangrong wrote:
> On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
>
> >> - }
> >> + rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
> >> +&ctxt->exception);
> >> + if (rc != X86EMUL_CONTINUE)
> >> + return rc;
> >> +
> >> + mc->end += size;
> >> +
> >> +read_cached:
> >> + memcpy(dest, mc->data + mc->pos, size);
> >
> > What prevents read_emulated(size > 8) call, with
> > mc->pos == (mc->end - 8) now?
>
> Marcelo,
>
> The splitting has been done in emulator_read_write_onepage:
>
> while (bytes) {
> unsigned now = min(bytes, 8U);
>
> frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
> frag->gpa = gpa;
> frag->data = val;
> frag->len = now;
> frag->write_readonly_mem = (ret == -EPERM);
>
> gpa += now;
> val += now;
> bytes -= now;
> }
>
> So i think it is safe to remove the splitting in read_emulated.
Yes, it is fine to remove it.
But splitting in emulate.c prevented the case of _cache read_ with size
> 8 beyond end of mc->data. Must handle that case in read_emulated.
"What prevents read_emulated(size > 8) call, with mc->pos == (mc->end - 8) now?"
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] KVM: x86: simplify read_emulated
On 07/20/2012 07:58 AM, Marcelo Tosatti wrote:
>> -}
>> +rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
>> + &ctxt->exception);
>> +if (rc != X86EMUL_CONTINUE)
>> +return rc;
>> +
>> +mc->end += size;
>> +
>> +read_cached:
>> +memcpy(dest, mc->data + mc->pos, size);
>
> What prevents read_emulated(size > 8) call, with
> mc->pos == (mc->end - 8) now?
Marcelo,
The splitting has been done in emulator_read_write_onepage:
while (bytes) {
unsigned now = min(bytes, 8U);
frag = &vcpu->mmio_fragments[vcpu->mmio_nr_fragments++];
frag->gpa = gpa;
frag->data = val;
frag->len = now;
frag->write_readonly_mem = (ret == -EPERM);
gpa += now;
val += now;
bytes -= now;
}
So i think it is safe to remove the splitting in read_emulated.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] KVM: x86: simplify read_emulated
On Tue, Jul 17, 2012 at 09:51:34PM +0800, Xiao Guangrong wrote:
> No need split mmio read region into 8-bits pieces since we do it in
> emulator_read_write_onepage
>
> Signed-off-by: Xiao Guangrong
> ---
> arch/x86/kvm/emulate.c | 29 -
> 1 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 97d9a99..2d1916b 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1166,24 +1166,19 @@ static int read_emulated(struct x86_emulate_ctxt
> *ctxt,
> int rc;
> struct read_cache *mc = &ctxt->mem_read;
>
> - while (size) {
> - int n = min(size, 8u);
> - size -= n;
> - if (mc->pos < mc->end)
> - goto read_cached;
> -
> - rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, n,
> - &ctxt->exception);
> - if (rc != X86EMUL_CONTINUE)
> - return rc;
> - mc->end += n;
> + if (mc->pos < mc->end)
> + goto read_cached;
>
> - read_cached:
> - memcpy(dest, mc->data + mc->pos, n);
> - mc->pos += n;
> - dest += n;
> - addr += n;
> - }
> + rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
> + &ctxt->exception);
> + if (rc != X86EMUL_CONTINUE)
> + return rc;
> +
> + mc->end += size;
> +
> +read_cached:
> + memcpy(dest, mc->data + mc->pos, size);
What prevents read_emulated(size > 8) call, with
mc->pos == (mc->end - 8) now?
> + mc->pos += size;
> return X86EMUL_CONTINUE;
> }
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/9] KVM: x86: simplify read_emulated
No need split mmio read region into 8-bits pieces since we do it in
emulator_read_write_onepage
Signed-off-by: Xiao Guangrong
---
arch/x86/kvm/emulate.c | 29 -
1 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 97d9a99..2d1916b 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1166,24 +1166,19 @@ static int read_emulated(struct x86_emulate_ctxt *ctxt,
int rc;
struct read_cache *mc = &ctxt->mem_read;
- while (size) {
- int n = min(size, 8u);
- size -= n;
- if (mc->pos < mc->end)
- goto read_cached;
-
- rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, n,
- &ctxt->exception);
- if (rc != X86EMUL_CONTINUE)
- return rc;
- mc->end += n;
+ if (mc->pos < mc->end)
+ goto read_cached;
- read_cached:
- memcpy(dest, mc->data + mc->pos, n);
- mc->pos += n;
- dest += n;
- addr += n;
- }
+ rc = ctxt->ops->read_emulated(ctxt, addr, mc->data + mc->end, size,
+ &ctxt->exception);
+ if (rc != X86EMUL_CONTINUE)
+ return rc;
+
+ mc->end += size;
+
+read_cached:
+ memcpy(dest, mc->data + mc->pos, size);
+ mc->pos += size;
return X86EMUL_CONTINUE;
}
--
1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
