From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v7 4/4] KVM: x86: Add support for local interrupt requests
 from userspace
Date: Thu, 30 Jul 2015 11:33:47 +0200
Message-ID: <55B9EF7B.9080806@redhat.com>
References: <1438237303-19124-1-git-send-email-srutherford@google.com>
 <1438237303-19124-4-git-send-email-srutherford@google.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
To: Steve Rutherford <srutherford@google.com>,
	KVM list <kvm@vger.kernel.org>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-wi0-f172.google.com ([209.85.212.172]:37258 "EHLO
	mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750884AbbG3Jdu (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 30 Jul 2015 05:33:50 -0400
Received: by wibud3 with SMTP id ud3so59974006wib.0
        for <kvm@vger.kernel.org>; Thu, 30 Jul 2015 02:33:49 -0700 (PDT)
In-Reply-To: <1438237303-19124-4-git-send-email-srutherford@google.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 30/07/2015 08:21, Steve Rutherford wrote:
>   */
>  int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
>  {
> -	if (!irqchip_in_kernel(v->kvm))
> +	if (!pic_in_kernel(v->kvm))
>  		return v->arch.interrupt.pending;
>  
>  	if (kvm_cpu_has_extint(v))
> @@ -75,7 +88,7 @@ int kvm_cpu_has_injectable_intr(struct kvm_vcpu *v)
>   */
>  int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
>  {
> -	if (!irqchip_in_kernel(v->kvm))
> +	if (!pic_in_kernel(v->kvm))
>  		return v->arch.interrupt.pending;
>  
>  	if (kvm_cpu_has_extint(v))
> @@ -103,7 +123,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
>  {
>  	int vector;
>  
> -	if (!irqchip_in_kernel(v->kvm))
> +	if (!pic_in_kernel(v->kvm) && v->arch.interrupt.pending)
>  		return v->arch.interrupt.nr;
>  
>  	vector = kvm_cpu_get_extint(v);

I have one more doubt about these three hunks.

v->arch.interrupt should not be used at all with split irqchip.  In 
particular:

- kvm_cpu_has_injectable_intr should go through kvm_cpu_has_extint and 
query pending_userspace_extint

- same for kvm_cpu_has_interrupt

- kvm_cpu_get_interrupt should go through kvm_cpu_get_extint and 
return/clear v->arch.pending_external_vector.

So I think !irqchip_in_kernel(v->kvm) is the right test.  In 
particular, with pic_in_kernel, kvm_cpu_has_extint's irqchip_split case 
is dead.  I am then not sure how you reach this code in x86.c:

	/* kvm_cpu_has_injectable_intr doesn't take extints into account? */
        } else if (kvm_cpu_has_injectable_intr(vcpu)) {
                /*
                 * Because interrupts can be injected asynchronously, we are
                 * calling check_nested_events again here to avoid a race condition.
                 * See https://lkml.org/lkml/2014/7/2/60 for discussion about this
                 * proposal and current concerns.  Perhaps we should be setting
                 * KVM_REQ_EVENT only on certain events and not unconditionally?
                 */
                if (is_guest_mode(vcpu) && kvm_x86_ops->check_nested_events) {
                        r = kvm_x86_ops->check_nested_events(vcpu, req_int_win);
                        if (r != 0)
                                return r;
                }
                if (kvm_x86_ops->interrupt_allowed(vcpu)) {
			/*
			 * kvm_cpu_get_interrupt does take extints into account
			 * because of the " && v->arch.interrupt.pending", but
			 * you won't get here unless you have an APIC interrupt
			 * pending!
			 */
                        kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu),
                                            false);
                        kvm_x86_ops->set_irq(vcpu);
                }
        }

Paolo