On Fri, Apr 08, 2016 at 11:43:50AM +0100, Andre Przywara wrote:
> Currently we call kvm_handle_mmio_return when we need to sync back
> register content into the guest after a trap.
> This function expects its arguments packaged into struct kvm_run,
> which we only naturally use when we emulate in userspace. With
> in-kernel emulation we have to copy the data into that strcut.
s/strcut/struct/
> This patch fixes this by explicitly passing the required variables,
> so we save the copying in two cases.
this patch fixes what?
> Also since this function is actually a NOP for an MMIO write, we
> rename it to kvm_writeback_mmio_data and move the !is_write check to
> the callers.
> This fixes a bug where we were missing a writeback for one in-kernel
> emulation case.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
> Hi Christoffer,
>
> this is my take on fixing the missing MMIO writeback call.
> This rework kind of hides the actual bug-fix, that's why I dumped
> it in favour of the one-liner in my series.
>
Hmmm, I thought I also sent a patch for this (off list), and I'm not
sure why your approach is better, but ok, I guess I can review your
patch...
>
> arch/arm/include/asm/kvm_mmio.h | 3 ++-
> arch/arm/kvm/arm.c | 10 +++++---
> arch/arm/kvm/mmio.c | 54 ++++++++++++++++++---------------------
> arch/arm64/include/asm/kvm_mmio.h | 3 ++-
> virt/kvm/arm/vgic.c | 8 ++----
> 5 files changed, 38 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
> index d8e90c8..21f97ac 100644
> --- a/arch/arm/include/asm/kvm_mmio.h
> +++ b/arch/arm/include/asm/kvm_mmio.h
> @@ -28,7 +28,8 @@ struct kvm_decode {
> bool sign_extend;
> };
>
> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,
I think this should be called kvm_write_back_mmio_data() instead.
> + gpa_t addr);
> int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> phys_addr_t fault_ipa);
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index b538431..e9f593c 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -555,9 +555,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> return ret;
>
> if (run->exit_reason == KVM_EXIT_MMIO) {
> - ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> - if (ret)
> - return ret;
> + if (!run->mmio.is_write) {
> + ret = kvm_writeback_mmio_data(vcpu, run->mmio.data,
> + run->mmio.len,
> + run->mmio.phys_addr);
> + if (ret)
> + return ret;
> + }
I preferred all the logic be encapsulated in the function instead of
extracting values in the caller, but it's purely a cosmetic comment.
> }
>
> if (vcpu->sigset_active)
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index 0f6600f..052aa02 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -86,38 +86,33 @@ static unsigned long mmio_read_buf(char *buf, unsigned int len)
> }
>
> /**
> - * kvm_handle_mmio_return -- Handle MMIO loads after user space emulation
> + * kvm_writeback_mmio_data -- Write back emulation data into the guest's
> + * target register after return from userspace.
> * @vcpu: The VCPU pointer
> - * @run: The VCPU run struct containing the mmio data
> + * @data: A pointer to the data to be written back
> + * @len: The size of the read access
> + * @addr: The original MMIO address (for the tracepoint only)
> *
> * 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)
> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data_ptr, int len,
> + gpa_t addr)
> {
> unsigned long data;
> - unsigned int len;
> int mask;
>
> - if (!run->mmio.is_write) {
> - len = run->mmio.len;
> - if (len > sizeof(unsigned long))
> - return -EINVAL;
> + data = mmio_read_buf(data_ptr, len);
>
> - data = mmio_read_buf(run->mmio.data, len);
> -
> - if (vcpu->arch.mmio_decode.sign_extend &&
> - len < sizeof(unsigned long)) {
> - mask = 1U << ((len * 8) - 1);
> - data = (data ^ mask) - mask;
> - }
> -
> - trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, run->mmio.phys_addr,
> - data);
> - data = vcpu_data_host_to_guest(vcpu, data, len);
> - vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
> + if (vcpu->arch.mmio_decode.sign_extend && len < sizeof(unsigned long)) {
> + mask = 1U << ((len * 8) - 1);
> + data = (data ^ mask) - mask;
> }
>
> + trace_kvm_mmio(KVM_TRACE_MMIO_READ, len, addr, data);
> + data = vcpu_data_host_to_guest(vcpu, data, len);
> + vcpu_set_reg(vcpu, vcpu->arch.mmio_decode.rt, data);
> +
> return 0;
> }
>
> @@ -202,22 +197,23 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> data_buf);
> }
>
> + if (!ret) {
> + /* We handled the access successfully in the kernel. */
> + vcpu->stat.mmio_exit_kernel++;
> + if (!is_write)
> + kvm_writeback_mmio_data(vcpu, data_buf, len, fault_ipa);
are you not doing this writeback twice? Once in the vgic and once here?
That was the very thing I tried to address in my patch.
> + return 1;
> + }
> +
> /* Now prepare kvm_run for the potential return to userland. */
> run->mmio.is_write = is_write;
> run->mmio.phys_addr = fault_ipa;
> run->mmio.len = len;
> + run->exit_reason = KVM_EXIT_MMIO;
> if (is_write)
> memcpy(run->mmio.data, data_buf, len);
>
> - if (!ret) {
> - /* We handled the access successfully in the kernel. */
> - vcpu->stat.mmio_exit_kernel++;
> - kvm_handle_mmio_return(vcpu, run);
> - return 1;
> - } else {
> - vcpu->stat.mmio_exit_user++;
> - }
> + vcpu->stat.mmio_exit_user++;
>
> - run->exit_reason = KVM_EXIT_MMIO;
> return 0;
> }
> diff --git a/arch/arm64/include/asm/kvm_mmio.h b/arch/arm64/include/asm/kvm_mmio.h
> index fe612a9..00a57bd 100644
> --- a/arch/arm64/include/asm/kvm_mmio.h
> +++ b/arch/arm64/include/asm/kvm_mmio.h
> @@ -30,7 +30,8 @@ struct kvm_decode {
> bool sign_extend;
> };
>
> -int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
> +int kvm_writeback_mmio_data(struct kvm_vcpu *vcpu, void *data, int len,
> + gpa_t addr);
> int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> phys_addr_t fault_ipa);
>
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 00429b3..7a9aa56 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -821,7 +821,6 @@ static int vgic_handle_mmio_access(struct kvm_vcpu *vcpu,
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> struct vgic_io_device *iodev = container_of(this,
> struct vgic_io_device, dev);
> - struct kvm_run *run = vcpu->run;
> const struct vgic_io_range *range;
> struct kvm_exit_mmio mmio;
> bool updated_state;
> @@ -850,12 +849,9 @@ 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 (!is_write)
> + kvm_writeback_mmio_data(vcpu, val, len, addr);
see above.
>
> if (updated_state)
> vgic_kick_vcpus(vcpu->kvm);
> --
> 2.7.3
>
--
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
