| Message ID | 50056DE6.6020801@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <xiaoguangrong@linux.vnet.ibm.com> > --- > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 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; }
No need split mmio read region into 8-bits pieces since we do it in emulator_read_write_onepage Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> --- arch/x86/kvm/emulate.c | 29 ++++++++++++----------------- 1 files changed, 12 insertions(+), 17 deletions(-)