On Tue, Apr 05, 2016 at 01:12:04PM +0100, Andre Przywara wrote:
> On 29/03/16 13:33, Christoffer Dall wrote:
> > On Fri, Mar 25, 2016 at 02:04:24AM +0000, Andre Przywara wrote:
> >> When the kernel was handling a guest MMIO access internally, we need
> >> to copy the emulation result into the run->mmio structure in order
> >> for the kvm_handle_mmio_return() function to pick it up and inject
> >> the result back into the guest.
> >> Currently the only user of kvm_io_bus for ARM is the VGIC, which did
> >> this copying itself, so this was not causing issues so far.
> >> But with upcoming kvm_io_bus users we need to do the copying here.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> >> ---
> >> arch/arm/kvm/mmio.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> >> index 0f6600f..d5c2727 100644
> >> --- a/arch/arm/kvm/mmio.c
> >> +++ b/arch/arm/kvm/mmio.c
> >> @@ -206,7 +206,7 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >> run->mmio.is_write = is_write;
> >> run->mmio.phys_addr = fault_ipa;
> >> run->mmio.len = len;
> >> - if (is_write)
> >> + if (is_write || !ret)
> >> memcpy(run->mmio.data, data_buf, len);
> >
> > I had a really hard time understanding this, how about this instead:
>
> Admittedly this is the shortest possible fix. I also had a rework closer
> to your version, which also avoided copying the arguments in some cases,
> but I thought a smaller diff would be more suitable, since this is
> actually a fix.
>
> Shall I add a comment here or post my version of the fix instead?
>
Let me test what I have below more thoroughly and send that as a patch.
-Christoffer
>
> > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> > index 0f6600f..0158e9e 100644
> > --- a/arch/arm/kvm/mmio.c
> > +++ b/arch/arm/kvm/mmio.c
> > @@ -87,11 +87,10 @@ static unsigned long mmio_read_buf(char *buf, unsigned int len)
> >
> > /**
> > * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
> > + * or in-kernel IO emulation
> > + *
> > * @vcpu: The VCPU pointer
> > * @run: The VCPU run struct containing the mmio data
> > - *
> > - * This should only be called after returning from userspace for MMIO load
> > - * emulation.
> > */
> > int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> > {
> > @@ -206,18 +205,19 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > run->mmio.is_write = is_write;
> > run->mmio.phys_addr = fault_ipa;
> > run->mmio.len = len;
> > - if (is_write)
> > - memcpy(run->mmio.data, data_buf, len);
> >
> > if (!ret) {
> > /* We handled the access successfully in the kernel. */
> > + if (!is_write)
> > + memcpy(run->mmio.data, data_buf, len);
> > vcpu->stat.mmio_exit_kernel++;
> > kvm_handle_mmio_return(vcpu, run);
> > return 1;
> > - } else {
> > - vcpu->stat.mmio_exit_user++;
> > }
> >
> > + if (is_write)
> > + memcpy(run->mmio.data, data_buf, len);
> > + vcpu->stat.mmio_exit_user++;
> > run->exit_reason = KVM_EXIT_MMIO;
> > return 0;
> > }
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 00429b3..63e99cb 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -850,12 +850,6 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
> > updated_state = false;
> > }
> > spin_unlock(&dist->lock);
> > - run->mmio.is_write = is_write;
> > - run->mmio.len = len;
> > - run->mmio.phys_addr = addr;
> > - memcpy(run->mmio.data, val, len);
> > -
> > - kvm_handle_mmio_return(vcpu, run);
> >
> > if (updated_state)
> > vgic_kick_vcpus(vcpu->kvm);
> >
>
--
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
