On Thu, Mar 26, 2015 at 02:39:38PM +0000, Andre Przywara wrote:
> Currently we have struct kvm_exit_mmio for encapsulating MMIO abort
> data to be passed on from syndrome decoding all the way down to the
> VGIC register handlers. Now as we switch the MMIO handling to be
> routed through the KVM MMIO bus, it does not make sense anymore to
> use that structure already from the beginning. So we put the data into
> kvm_run very early and use that encapsulation till the MMIO bus call.
> Then we fill kvm_exit_mmio in the VGIC only, making it a VGIC private
> structure. On that way we replace the data buffer in that structure
> with a pointer pointing to a single location in kvm_run, so we get
> rid of some copying on the way.
> With all of the virtual GIC emulation code now being registered with
> the kvm_io_bus, we can remove all of the old MMIO handling code and
> its dispatching functionality.
>
> I didn't bother to rename kvm_exit_mmio (to vgic_mmio or something),
> because that touches a lot of code lines without any good reason.
>
> This is based on an original patch by Nikolay.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> Cc: Nikolay Nikolaev <n.nikolaev@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> arch/arm/include/asm/kvm_mmio.h | 22 ---------
> arch/arm/kvm/mmio.c | 60 ++++++++++++++++++-------
> arch/arm64/include/asm/kvm_mmio.h | 22 ---------
> include/kvm/arm_vgic.h | 6 ---
> virt/kvm/arm/vgic-v2-emul.c | 21 +--------
> virt/kvm/arm/vgic-v3-emul.c | 35 ---------------
> virt/kvm/arm/vgic.c | 89 ++-----------------------------------
> virt/kvm/arm/vgic.h | 13 +++---
> 8 files changed, 57 insertions(+), 211 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_mmio.h b/arch/arm/include/asm/kvm_mmio.h
> index 3f83db2..d8e90c8 100644
> --- a/arch/arm/include/asm/kvm_mmio.h
> +++ b/arch/arm/include/asm/kvm_mmio.h
> @@ -28,28 +28,6 @@ struct kvm_decode {
> bool sign_extend;
> };
>
> -/*
> - * The in-kernel MMIO emulation code wants to use a copy of run->mmio,
> - * which is an anonymous type. Use our own type instead.
> - */
> -struct kvm_exit_mmio {
> - phys_addr_t phys_addr;
> - u8 data[8];
> - u32 len;
> - bool is_write;
> - void *private;
> -};
> -
> -static inline void kvm_prepare_mmio(struct kvm_run *run,
> - struct kvm_exit_mmio *mmio)
> -{
> - run->mmio.phys_addr = mmio->phys_addr;
> - run->mmio.len = mmio->len;
> - run->mmio.is_write = mmio->is_write;
> - memcpy(run->mmio.data, mmio->data, mmio->len);
> - run->exit_reason = KVM_EXIT_MMIO;
> -}
> -
> int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run);
> int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> phys_addr_t fault_ipa);
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index 5d3bfc0..bb2ab44 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -122,7 +122,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> }
>
> static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> - struct kvm_exit_mmio *mmio)
> + struct kvm_run *run)
> {
> unsigned long rt;
> int len;
> @@ -148,9 +148,9 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> sign_extend = kvm_vcpu_dabt_issext(vcpu);
> rt = kvm_vcpu_dabt_get_rd(vcpu);
>
> - mmio->is_write = is_write;
> - mmio->phys_addr = fault_ipa;
> - mmio->len = len;
> + run->mmio.is_write = is_write;
> + run->mmio.phys_addr = fault_ipa;
> + run->mmio.len = len;
> vcpu->arch.mmio_decode.sign_extend = sign_extend;
> vcpu->arch.mmio_decode.rt = rt;
>
> @@ -162,23 +162,49 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> return 0;
> }
>
> +/**
> + * handle_kernel_mmio - handle an in-kernel MMIO access
> + * @vcpu: pointer to the vcpu performing the access
> + * @run: pointer to the kvm_run structure
> + *
> + * returns true if the MMIO access has been performed in kernel space,
> + * and false if it needs to be emulated in user space.
> + */
> +static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> + int ret;
> +
> + if (run->mmio.is_write) {
> + ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
> + run->mmio.len, run->mmio.data);
> +
> + } else {
> + ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
> + run->mmio.len, run->mmio.data);
> + }
> + if (!ret) {
> + kvm_handle_mmio_return(vcpu, run);
> + return true;
> + }
> +
> + return false;
> +}
> +
> int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> phys_addr_t fault_ipa)
> {
> - struct kvm_exit_mmio mmio;
> unsigned long data;
> unsigned long rt;
> int ret;
>
> /*
> - * Prepare MMIO operation. First stash it in a private
> - * structure that we can use for in-kernel emulation. If the
> - * kernel can't handle it, copy it into run->mmio and let user
> - * space do its magic.
> + * Prepare MMIO operation. First put the MMIO data into run->mmio.
> + * Then try if some in-kernel emulation feels responsible, otherwise
> + * let user space do its magic.
> */
>
> if (kvm_vcpu_dabt_isvalid(vcpu)) {
> - ret = decode_hsr(vcpu, fault_ipa, &mmio);
> + ret = decode_hsr(vcpu, fault_ipa, run);
hmm, this does feel rather obtrusive, because as Marc pointed out the
run structure can be modified from userspace. So a patch that comes
along later and assumes that run->mmio.len is something sane from the
HSR will break catastrophically. So I think it'd be better to use a
privat kernel struct for this.
Otherwise, this looks good to me!
Thanks,
-Christoffer
--
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
