Re: [PATCH 2/2] KVM: PPC: Book3S: Call into C interrupt handlers

[Alexander Graf]

On 26.04.2012, at 23:45, Benjamin Herrenschmidt wrote:

> 
>> +static void kvmppc_fill_pt_regs(struct pt_regs *regs)
>> +{
>> +    ulong r1, ip, msr, lr;
>> +
>> +    asm("mr %0, 1" : "=r"(r1));
>> +    asm("mflr %0" : "=r"(lr));
>> +    asm("mfmsr %0" : "=r"(msr));
>> +    asm("bl 1f; 1: mflr %0" : "=r"(ip));
>> +
>> +    memset(regs, 0, sizeof(*regs));
>> +    regs->gpr[1] = r1;
>> +    regs->nip = ip;
>> +    regs->msr = msr;
>> +    regs->link = lr;
>> +}
> 
> That is -very- gross ... I suppose it works but yuck :-)
> 
>> +static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu,
>> +                                 unsigned int exit_nr)
>> +{
>> +    struct pt_regs regs;
>> +
>> +    switch (exit_nr) {
>> +    case BOOK3S_INTERRUPT_EXTERNAL:
>> +    case BOOK3S_INTERRUPT_EXTERNAL_LEVEL:
>> +    case BOOK3S_INTERRUPT_EXTERNAL_HV:
>> +            kvmppc_fill_pt_regs(&regs);
>> +            soft_irq_disable();
>> +            do_IRQ(&regs);
>> +            soft_irq_enable();
> 
> What are those soft_irq_disable/enable ? They look like sometimes
> local_irq_disable/enable and sometimes something else ?

Yeah, on ppc64 these are local_irq_en/disable. On ppc32, they are nops, because 
there we don't distinguish between soft and hard irq enabled.

> 
> If you are hard disabled already (which you should be) and want to
> "mark" things as soft disabled, I suppose that will work except that
> you'll be missing PACA_IRQ_HARD_DIS in irq_happened, so
> local_irq_enable() will not hard-enable.

I jump through this hoop because in kernel/posix-cpu-timers.c:1288 the BUG_ON 
triggers, since irqs_disabled() is false, because they are hard, but not soft 
disabled (which is a combination that iiuc doesn't happen in normal Linux).

> 
>> +            break;
>> +    case BOOK3S_INTERRUPT_DECREMENTER:
>> +    case BOOK3S_INTERRUPT_HV_DECREMENTER:
>> +            kvmppc_fill_pt_regs(&regs);
>> +            soft_irq_disable();
>> +            timer_interrupt(&regs);
>> +            soft_irq_enable();
>> +            break;
> 
> Same.
> 
>> +    case BOOK3S_INTERRUPT_MACHINE_CHECK:
>> +            /* FIXME */
>> +            break;
>> +    case BOOK3S_INTERRUPT_PERFMON:
>> +            kvmppc_fill_pt_regs(&regs);
>> +            soft_irq_disable();
>> +            performance_monitor_exception(&regs);
>> +            soft_irq_enable();
>> +            break;
> 
> Same.
> 
>> +    }
>> +}
>> +
>> int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>                        unsigned int exit_nr)
>> {
>> @@ -548,6 +602,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 
>> kvm_vcpu *vcpu,
>>      run->exit_reason = KVM_EXIT_UNKNOWN;
>>      run->ready_for_interrupt_injection = 1;
>> 
>> +    /* restart interrupts if they were meant for the host */
>> +    kvmppc_restart_interrupt(vcpu, exit_nr);
>> +    __hard_irq_enable();
>> +
> 
> I suppose that's to work around the above comment about missing
> PACA_IRQ_HARD_DIS ?

This is to actually enable interrupts for real, regardless of ppc64 and ppc32. 
In fact, the previous code was pretty buggy - it was running the handlers with 
interrupts disabled ;).


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html