Re: [Qemu-devel] [PATCH v2 07/10] target-mips: kvm: Add main KVM support for MIPS

[James Hogan <james.hogan@imgtec.com>]

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