[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 07/10] target-mips: kvm: Add main KVM support
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 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 |
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