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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

> +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 ?

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.

> +		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 ?

>  	trace_kvm_book3s_exit(exit_nr, vcpu);
>  	preempt_enable();
>  	kvm_resched(vcpu);
> diff --git a/arch/powerpc/kvm/book3s_segment.S b/arch/powerpc/kvm/book3s_segment.S
> index 6bae0a9..3a78b46 100644
> --- a/arch/powerpc/kvm/book3s_segment.S
> +++ b/arch/powerpc/kvm/book3s_segment.S
> @@ -308,28 +308,6 @@ no_dcbz32_off:
>  
>  #endif /* CONFIG_PPC_BOOK3S_64 */
>  
> -	/*
> -	 * For some interrupts, we need to call the real Linux
> -	 * handler, so it can do work for us. This has to happen
> -	 * as if the interrupt arrived from the kernel though,
> -	 * so let's fake it here where most state is restored.
> -	 *
> -	 * Having set up SRR0/1 with the address where we want
> -	 * to continue with relocation on (potentially in module
> -	 * space), we either just go straight there with rfi[d],
> -	 * or we jump to an interrupt handler with bctr if there
> -	 * is an interrupt to be handled first.  In the latter
> -	 * case, the rfi[d] at the end of the interrupt handler
> -	 * will get us back to where we want to continue.
> -	 */
> -
> -	cmpwi	r12, BOOK3S_INTERRUPT_EXTERNAL
> -	beq	1f
> -	cmpwi	r12, BOOK3S_INTERRUPT_DECREMENTER
> -	beq	1f
> -	cmpwi	r12, BOOK3S_INTERRUPT_PERFMON
> -1:	mtctr	r12
> -
>  	/* Register usage at this point:
>  	 *
>  	 * R1       = host R1
> @@ -348,7 +326,6 @@ no_dcbz32_off:
>  	/* Load highmem handler address */
>  	mtsrr0	r8
>  
> -	/* RFI into the highmem handler, or jump to interrupt handler */
> -	beqctr
> +	/* RFI into the highmem handler */
>  	RFI
>  kvmppc_handler_trampoline_exit_end:

Cheers,
Ben.