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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
