From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Hogan Subject: Re: [Qemu-devel] [PATCH v2 07/10] target-mips: kvm: Add main KVM support for MIPS Date: Tue, 11 Feb 2014 10:54:48 +0000 Message-ID: <52FA0178.10506@imgtec.com> References: <1387203165-5553-1-git-send-email-james.hogan@imgtec.com> <1387203165-5553-8-git-send-email-james.hogan@imgtec.com> <52F8DD08.4070500@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , Gleb Natapov , Sanjay Lal , Paolo Bonzini , Aurelien Jarno To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Return-path: Received: from multi.imgtec.com ([194.200.65.239]:65193 "EHLO multi.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750767AbaBKKyv (ORCPT ); Tue, 11 Feb 2014 05:54:51 -0500 In-Reply-To: <52F8DD08.4070500@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: Hi Andreas, On 10/02/14 14:07, Andreas F=C3=A4rber wrote: >> +#define dprintf(fmt, ...) \ >=20 > dprintf is the name of a stdio.h function, so DPRINTF may be a better= name. Okay. >> +int kvm_arch_init_vcpu(CPUState *env) >=20 > Please use "env" only for CPUMIPSState, use "cpu" or "cs" here. The > usual convention is "cs" for CPUState in target-*/ so that "cpu" can = be > used for MIPSCPU. Okay. >> +{ >> + dprintf("%s\n", __func__); >> + return 0; >> +} >> + >> +static inline int cpu_mips_io_interrupts_pending(CPUArchState *env) >=20 > Please don't use CPUArchState in MIPS-specific code, use CPUMIPSState= =2E > Although in this trivial case MIPSCPU would be more future-proof. True. >> +{ >> + dprintf("%s: %#x\n", __func__, env->CP0_Cause & (1 << (2 + CP0C= a_IP))); >> + return env->CP0_Cause & (0x1 << (2 + CP0Ca_IP)); >> +} >> + >> + >> +void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run) >> +{ >> + MIPSCPU *cpu =3D MIPS_CPU(cs); >> + CPUMIPSState *env =3D &cpu->env; >> + int r; >> + struct kvm_mips_interrupt intr; >> + >> + if ((cs->interrupt_request & CPU_INTERRUPT_HARD) && >> + (cpu_mips_io_interrupts_pending(env))) { >=20 > Parentheses around cpu_mips_io_interrupts_pending() seem unnecessary > here FWIW. Good spot >> + intr.cpu =3D -1; >> + intr.irq =3D 2; >> + r =3D kvm_vcpu_ioctl(cs, KVM_INTERRUPT, &intr); >> + if (r < 0) { >> + printf("cpu %d fail inject %x\n", cs->cpu_index, intr.i= rq); >=20 > Should this really be a printf() rather than error_report() or trace = point? It looks like error_report() would indeed be better, thanks >> +int kvm_mips_set_interrupt(CPUMIPSState *env, int irq, int level) >> +{ >> + CPUState *cs =3D ENV_GET_CPU(env); >=20 > CPU(mips_env_get_cpu(env)) please - ENV_GET_CPU() is for generic code > only and supposed to go away. >=20 > Any chance a MIPSCPU *cpu (or CPUState *cs) argument can be used inst= ead? Yep, MIPSCPU can happily be used here (I thought the same thing after fixing cpu_mips_io_interrupts_pending above). Thanks for taking the time to review! Cheers James From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40022) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDAzT-0000no-Jt for qemu-devel@nongnu.org; Tue, 11 Feb 2014 05:55:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WDAzN-00084q-Id for qemu-devel@nongnu.org; Tue, 11 Feb 2014 05:54:59 -0500 Received: from multi.imgtec.com ([194.200.65.239]:65190) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WDAzN-0007uI-Ct for qemu-devel@nongnu.org; Tue, 11 Feb 2014 05:54:53 -0500 Message-ID: <52FA0178.10506@imgtec.com> Date: Tue, 11 Feb 2014 10:54:48 +0000 From: James Hogan MIME-Version: 1.0 References: <1387203165-5553-1-git-send-email-james.hogan@imgtec.com> <1387203165-5553-8-git-send-email-james.hogan@imgtec.com> <52F8DD08.4070500@suse.de> In-Reply-To: <52F8DD08.4070500@suse.de> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 07/10] target-mips: kvm: Add main KVM support for MIPS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: kvm@vger.kernel.org, Gleb Natapov , qemu-devel@nongnu.org, Sanjay Lal , Paolo Bonzini , Aurelien Jarno Hi Andreas, On 10/02/14 14:07, Andreas Färber wrote: >> +#define dprintf(fmt, ...) \ > > dprintf is the name of a stdio.h function, so DPRINTF may be a better name. Okay. >> +int kvm_arch_init_vcpu(CPUState *env) > > Please use "env" only for CPUMIPSState, use "cpu" or "cs" here. The > usual convention is "cs" for CPUState in target-*/ so that "cpu" can be > used for MIPSCPU. Okay. >> +{ >> + dprintf("%s\n", __func__); >> + return 0; >> +} >> + >> +static inline int cpu_mips_io_interrupts_pending(CPUArchState *env) > > Please don't use CPUArchState in MIPS-specific code, use CPUMIPSState. > Although in this trivial case MIPSCPU would be more future-proof. True. >> +{ >> + dprintf("%s: %#x\n", __func__, env->CP0_Cause & (1 << (2 + CP0Ca_IP))); >> + return env->CP0_Cause & (0x1 << (2 + CP0Ca_IP)); >> +} >> + >> + >> +void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run) >> +{ >> + MIPSCPU *cpu = MIPS_CPU(cs); >> + CPUMIPSState *env = &cpu->env; >> + int r; >> + struct kvm_mips_interrupt intr; >> + >> + if ((cs->interrupt_request & CPU_INTERRUPT_HARD) && >> + (cpu_mips_io_interrupts_pending(env))) { > > Parentheses around cpu_mips_io_interrupts_pending() seem unnecessary > here FWIW. Good spot >> + intr.cpu = -1; >> + intr.irq = 2; >> + r = kvm_vcpu_ioctl(cs, KVM_INTERRUPT, &intr); >> + if (r < 0) { >> + printf("cpu %d fail inject %x\n", cs->cpu_index, intr.irq); > > Should this really be a printf() rather than error_report() or trace point? It looks like error_report() would indeed be better, thanks >> +int kvm_mips_set_interrupt(CPUMIPSState *env, int irq, int level) >> +{ >> + CPUState *cs = ENV_GET_CPU(env); > > CPU(mips_env_get_cpu(env)) please - ENV_GET_CPU() is for generic code > only and supposed to go away. > > Any chance a MIPSCPU *cpu (or CPUState *cs) argument can be used instead? Yep, MIPSCPU can happily be used here (I thought the same thing after fixing cpu_mips_io_interrupts_pending above). Thanks for taking the time to review! Cheers James