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 <[email protected]> > Cc: Nikolay Nikolaev <[email protected]> > --- > 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 _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
