| Message ID | 1302600985-10704-4-git-send-email-gleb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/12/2011 12:36 PM, Gleb Natapov wrote: > mmio_index should be taken into account when copying data from > userspace. > > Signed-off-by: Gleb Natapov<gleb@redhat.com> > --- > arch/x86/kvm/x86.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index b568779..609c7ab 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu) > if (vcpu->mmio_needed) { > vcpu->mmio_needed = 0; > if (!vcpu->mmio_is_write) > - memcpy(vcpu->mmio_data, run->mmio.data, 8); > + memcpy(vcpu->mmio_data + vcpu->mmio_index, > + run->mmio.data, 8); > vcpu->mmio_index += 8; > if (vcpu->mmio_index< vcpu->mmio_size) { > run->exit_reason = KVM_EXIT_MMIO; Interesting, the code passed the emulator.flat sse test. Does it now?
On Tue, Apr 12, 2011 at 03:19:00PM +0300, Avi Kivity wrote: > On 04/12/2011 12:36 PM, Gleb Natapov wrote: > >mmio_index should be taken into account when copying data from > >userspace. > > > >Signed-off-by: Gleb Natapov<gleb@redhat.com> > >--- > > arch/x86/kvm/x86.c | 3 ++- > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >index b568779..609c7ab 100644 > >--- a/arch/x86/kvm/x86.c > >+++ b/arch/x86/kvm/x86.c > >@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu) > > if (vcpu->mmio_needed) { > > vcpu->mmio_needed = 0; > > if (!vcpu->mmio_is_write) > >- memcpy(vcpu->mmio_data, run->mmio.data, 8); > >+ memcpy(vcpu->mmio_data + vcpu->mmio_index, > >+ run->mmio.data, 8); > > vcpu->mmio_index += 8; > > if (vcpu->mmio_index< vcpu->mmio_size) { > > run->exit_reason = KVM_EXIT_MMIO; > > Interesting, the code passed the emulator.flat sse test. Does it now? > It pass now and before. Probably by chance. But if I change read_emulated() to do int n = min(size, (unsigned)KVM_MMIO_SIZE); instead of int n = min(size, 8u); emulator.flat fails to emulate far jump instruction. -- Gleb. -- 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 04/12/2011 03:22 PM, Gleb Natapov wrote: > On Tue, Apr 12, 2011 at 03:19:00PM +0300, Avi Kivity wrote: > > On 04/12/2011 12:36 PM, Gleb Natapov wrote: > > >mmio_index should be taken into account when copying data from > > >userspace. > > > > > >Signed-off-by: Gleb Natapov<gleb@redhat.com> > > >--- > > > arch/x86/kvm/x86.c | 3 ++- > > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > > >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > >index b568779..609c7ab 100644 > > >--- a/arch/x86/kvm/x86.c > > >+++ b/arch/x86/kvm/x86.c > > >@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu) > > > if (vcpu->mmio_needed) { > > > vcpu->mmio_needed = 0; > > > if (!vcpu->mmio_is_write) > > >- memcpy(vcpu->mmio_data, run->mmio.data, 8); > > >+ memcpy(vcpu->mmio_data + vcpu->mmio_index, > > >+ run->mmio.data, 8); > > > vcpu->mmio_index += 8; > > > if (vcpu->mmio_index< vcpu->mmio_size) { > > > run->exit_reason = KVM_EXIT_MMIO; > > > > Interesting, the code passed the emulator.flat sse test. Does it now? > > > It pass now and before. Probably by chance. I don't understand how. I explicitly set the values so that it would fail in that case. Can you patch the test to fail with the current code? > But if I change read_emulated() to do > > int n = min(size, (unsigned)KVM_MMIO_SIZE); > > instead of > > int n = min(size, 8u); > > emulator.flat fails to emulate far jump instruction. Ouch, looks like we have the multi-transaction support in two places. I guess this is what made sse mmio work. Not sure what we should do (patch is fine, question is how to resolve the duplication).
On Tue, Apr 12, 2011 at 03:27:40PM +0300, Avi Kivity wrote: > On 04/12/2011 03:22 PM, Gleb Natapov wrote: > >On Tue, Apr 12, 2011 at 03:19:00PM +0300, Avi Kivity wrote: > >> On 04/12/2011 12:36 PM, Gleb Natapov wrote: > >> >mmio_index should be taken into account when copying data from > >> >userspace. > >> > > >> >Signed-off-by: Gleb Natapov<gleb@redhat.com> > >> >--- > >> > arch/x86/kvm/x86.c | 3 ++- > >> > 1 files changed, 2 insertions(+), 1 deletions(-) > >> > > >> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >> >index b568779..609c7ab 100644 > >> >--- a/arch/x86/kvm/x86.c > >> >+++ b/arch/x86/kvm/x86.c > >> >@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu) > >> > if (vcpu->mmio_needed) { > >> > vcpu->mmio_needed = 0; > >> > if (!vcpu->mmio_is_write) > >> >- memcpy(vcpu->mmio_data, run->mmio.data, 8); > >> >+ memcpy(vcpu->mmio_data + vcpu->mmio_index, > >> >+ run->mmio.data, 8); > >> > vcpu->mmio_index += 8; > >> > if (vcpu->mmio_index< vcpu->mmio_size) { > >> > run->exit_reason = KVM_EXIT_MMIO; > >> > >> Interesting, the code passed the emulator.flat sse test. Does it now? > >> > >It pass now and before. Probably by chance. > > I don't understand how. I explicitly set the values so that it > would fail in that case. > > Can you patch the test to fail with the current code? > If I understand correctly you've already found the explanation why test case worked? > >But if I change read_emulated() to do > > > > int n = min(size, (unsigned)KVM_MMIO_SIZE); > > > >instead of > > > > int n = min(size, 8u); > > > >emulator.flat fails to emulate far jump instruction. > > Ouch, looks like we have the multi-transaction support in two > places. I guess this is what made sse mmio work. > > Not sure what we should do (patch is fine, question is how to > resolve the duplication). > Multi-transaction works faster in complete_mmio, so read_emulated() should use it by doing int n = min(size, (unsigned)KVM_MMIO_SIZE). Other code in read_emulated() is still needed since it provides read re-play for multiple mmio reads during instruction emulation. -- Gleb. -- 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 04/12/2011 03:34 PM, Gleb Natapov wrote: > On Tue, Apr 12, 2011 at 03:27:40PM +0300, Avi Kivity wrote: > > On 04/12/2011 03:22 PM, Gleb Natapov wrote: > > >On Tue, Apr 12, 2011 at 03:19:00PM +0300, Avi Kivity wrote: > > >> On 04/12/2011 12:36 PM, Gleb Natapov wrote: > > >> >mmio_index should be taken into account when copying data from > > >> >userspace. > > >> > > > >> >Signed-off-by: Gleb Natapov<gleb@redhat.com> > > >> >--- > > >> > arch/x86/kvm/x86.c | 3 ++- > > >> > 1 files changed, 2 insertions(+), 1 deletions(-) > > >> > > > >> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > >> >index b568779..609c7ab 100644 > > >> >--- a/arch/x86/kvm/x86.c > > >> >+++ b/arch/x86/kvm/x86.c > > >> >@@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu) > > >> > if (vcpu->mmio_needed) { > > >> > vcpu->mmio_needed = 0; > > >> > if (!vcpu->mmio_is_write) > > >> >- memcpy(vcpu->mmio_data, run->mmio.data, 8); > > >> >+ memcpy(vcpu->mmio_data + vcpu->mmio_index, > > >> >+ run->mmio.data, 8); > > >> > vcpu->mmio_index += 8; > > >> > if (vcpu->mmio_index< vcpu->mmio_size) { > > >> > run->exit_reason = KVM_EXIT_MMIO; > > >> > > >> Interesting, the code passed the emulator.flat sse test. Does it now? > > >> > > >It pass now and before. Probably by chance. > > > > I don't understand how. I explicitly set the values so that it > > would fail in that case. > > > > Can you patch the test to fail with the current code? > > > If I understand correctly you've already found the explanation why > test case worked? Yes, but I used O_APPEND when writing my message. > > >But if I change read_emulated() to do > > > > > > int n = min(size, (unsigned)KVM_MMIO_SIZE); > > > > > >instead of > > > > > > int n = min(size, 8u); > > > > > >emulator.flat fails to emulate far jump instruction. > > > > Ouch, looks like we have the multi-transaction support in two > > places. I guess this is what made sse mmio work. > > > > Not sure what we should do (patch is fine, question is how to > > resolve the duplication). > > > Multi-transaction works faster in complete_mmio, so read_emulated() > should use it by doing int n = min(size, (unsigned)KVM_MMIO_SIZE). > Other code in read_emulated() is still needed since it provides read > re-play for multiple mmio reads during instruction emulation. Not concerned about speed here. Core support for large mmio feels correct, but the duplication doesn't.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b568779..609c7ab 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -5518,7 +5518,8 @@ static int complete_mmio(struct kvm_vcpu *vcpu) if (vcpu->mmio_needed) { vcpu->mmio_needed = 0; if (!vcpu->mmio_is_write) - memcpy(vcpu->mmio_data, run->mmio.data, 8); + memcpy(vcpu->mmio_data + vcpu->mmio_index, + run->mmio.data, 8); vcpu->mmio_index += 8; if (vcpu->mmio_index < vcpu->mmio_size) { run->exit_reason = KVM_EXIT_MMIO;
mmio_index should be taken into account when copying data from userspace. Signed-off-by: Gleb Natapov <gleb@redhat.com> --- arch/x86/kvm/x86.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)