From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH v5 07/13] ARM: KVM: Emulation framework and CP15 emulation Date: Mon, 12 Dec 2011 15:44:23 +0200 Message-ID: <4EE60537.9050805@redhat.com> References: <20111211102403.21693.6887.stgit@localhost> <20111211102502.21693.59537.stgit@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: android-virt@lists.cs.columbia.edu, kvm@vger.kernel.org, Marc.Zyngier@arm.com, catalin.marinas@arm.com, tech@virtualopensystems.com, peter.maydell@linaro.org To: Christoffer Dall Return-path: Received: from mx1.redhat.com ([209.132.183.28]:17892 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751131Ab1LLNom (ORCPT ); Mon, 12 Dec 2011 08:44:42 -0500 In-Reply-To: <20111211102502.21693.59537.stgit@localhost> Sender: kvm-owner@vger.kernel.org List-ID: On 12/11/2011 12:25 PM, Christoffer Dall wrote: > From: Christoffer Dall > > Adds a new important function in the main KVM/ARM code called > handle_exit() which is called from kvm_arch_vcpu_ioctl_run() on returns > from guest execution. This function examines the Hyp-Syndrome-Register > (HSR), which contains information telling KVM what caused the exit from > the guest. > > Some of the reasons for an exit are CP15 accesses, which are > not allowed from the guest and this commits handles these exits by commit > emulating the intented operation in software and skip the guest intended > instruction. > > @@ -306,6 +307,62 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) > return 0; > } > > +static inline int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, > + int exception_index) > +{ > + unsigned long hsr_ec; > + > + if (exception_index == ARM_EXCEPTION_IRQ) > + return 0; > + > + if (exception_index != ARM_EXCEPTION_HVC) { > + kvm_err(-EINVAL, "Unsupported exception type"); > + return -EINVAL; > + } > + > + hsr_ec = (vcpu->arch.hsr & HSR_EC) >> HSR_EC_SHIFT; > + switch (hsr_ec) { > + case HSR_EC_WFI: > + return kvm_handle_wfi(vcpu, run); > + case HSR_EC_CP15_32: > + case HSR_EC_CP15_64: > + return kvm_handle_cp15_access(vcpu, run); > + case HSR_EC_CP14_MR: > + return kvm_handle_cp14_access(vcpu, run); > + case HSR_EC_CP14_LS: > + return kvm_handle_cp14_load_store(vcpu, run); > + case HSR_EC_CP14_64: > + return kvm_handle_cp14_access(vcpu, run); > + case HSR_EC_CP_0_13: > + return kvm_handle_cp_0_13_access(vcpu, run); > + case HSR_EC_CP10_ID: > + return kvm_handle_cp10_id(vcpu, run); > + case HSR_EC_SVC_HYP: > + /* SVC called from Hyp mode should never get here */ > + kvm_msg("SVC called from Hyp mode shouldn't go here"); > + BUG(); > + case HSR_EC_HVC: > + kvm_msg("hvc: %x (at %08x)", vcpu->arch.hsr & ((1 << 16) - 1), > + vcpu->arch.regs.pc); > + kvm_msg(" HSR: %8x", vcpu->arch.hsr); > + break; > + case HSR_EC_IABT: > + case HSR_EC_DABT: > + return kvm_handle_guest_abort(vcpu, run); > + case HSR_EC_IABT_HYP: > + case HSR_EC_DABT_HYP: > + /* The hypervisor should never cause aborts */ > + kvm_msg("The hypervisor itself shouldn't cause aborts"); > + BUG(); > + default: > + kvm_msg("Unkown exception class: %08x (%08x)", hsr_ec, > + vcpu->arch.hsr); > + BUG(); > + } x86 uses a function table, which is slightly nicer. > + > + return 0; > +} > + > /** > * kvm_arch_vcpu_ioctl_run - the main VCPU run function to execute guest code > * @vcpu: The VCPU pointer > @@ -333,6 +390,26 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > local_irq_enable(); > > trace_kvm_exit(vcpu->arch.regs.pc); > + > + ret = handle_exit(vcpu, run, ret); > + if (ret) { > + kvm_err(ret, "Error in handle_exit"); > + break; > + } > + > + if (run->exit_reason == KVM_EXIT_MMIO) > + break; > + > + if (need_resched()) { > + vcpu_put(vcpu); > + schedule(); > + vcpu_load(vcpu); > + } I don't think you need the vcpu_put()/vcpu_load() here; you can replace the whole thing with cond_resched(). > + > + if (signal_pending(current) && !(run->exit_reason)) { > + run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN; This exit reason doesn't fit with KVM_IRQ_LINE. With KVM_INTERRUPT, userspace is responsible for determining when to inject interrupts, so it needs to know when guest interrupts become enabled (this is KVM_EXIT_IRQ_WINDOWS_OPEN). With KVM_IRQ_LINE, userspace just sets the line status, and the kernel takes care of everything. Oh, and you need to exit to userspace unconditionally if a signal is pending. > + break; > + } > } > > + > +/****************************************************************************** > + * Co-processor emulation > + */ > + > +struct coproc_params { > + unsigned long CRm; > + unsigned long CRn; > + unsigned long Op1; > + unsigned long Op2; > + unsigned long Rt1; > + unsigned long Rt2; > + bool is_64bit; > + bool is_write; > +}; > + > +#define CP15_OP(_vcpu, _params, _cp15_reg) \ > +do { \ > + if (_params->is_write) \ > + _vcpu->arch.cp15._cp15_reg = *vcpu_reg(_vcpu, _params->Rt1); \ > + else \ > + *vcpu_reg(_vcpu, _params->Rt1) = _vcpu->arch.cp15._cp15_reg; \ > +} while (0); Ugly. How about an array of registers instead? -- error compiling committee.c: too many arguments to function