Re: [PATCH v3 2/4] KVM: x86: Add KVM exit for IOAPIC EOIs

[Paolo Bonzini <pbonzini@redhat.com>]

On 03/06/2015 01:51, Steve Rutherford wrote:
> Adds KVM_EXIT_IOAPIC_EOI which allows the kernel to EOI
> level-triggered IOAPIC interrupts.
> 
> Uses a per VCPU exit bitmap to decide whether or not the IOAPIC needs
> to be informed (which is identical to the EOI_EXIT_BITMAP field used
> by modern x86 processors, but can also be used to elide kvm IOAPIC EOI
> exits on older processors).
> 
> [Note: A prototype using ResampleFDs found that decoupling the EOI
> from the VCPU's thread made it possible for the VCPU to not see a
> recent EOI after reentering the guest. This does not match real
> hardware.]
> 
> Compile tested for Intel x86.
> 
> Signed-off-by: Steve Rutherford <srutherford@google.com>
> ---
>  Documentation/virtual/kvm/api.txt | 10 ++++++++++
>  arch/x86/include/asm/kvm_host.h   |  3 +++
>  arch/x86/kvm/lapic.c              |  9 +++++++++
>  arch/x86/kvm/x86.c                | 11 +++++++++++
>  include/linux/kvm_host.h          |  1 +
>  include/uapi/linux/kvm.h          |  5 +++++
>  6 files changed, 39 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 9a43d42..6ab2a3f7 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -3271,6 +3271,16 @@ Valid values for 'type' are:
>  	 */
>  	__u64 kvm_valid_regs;
>  	__u64 kvm_dirty_regs;
> +
> +	/* KVM_EXIT_IOAPIC_EOI */
> +        struct {
> +	       __u8 vector;
> +        } eoi;
> +
> +Indicates that an eoi of a level triggered IOAPIC interrupt on vector has
> +occurred, which should be handled by the userspace IOAPIC. Triggers when
> +the Irqchip has been split between userspace and the kernel.

I'm okay with making this a VCPU-level thing, but you should document
_why_ it is here.  For example,

"Indicates that the VCPU's in-kernel local APIC received an EOI for a
level-triggered IOAPIC interrupt.  This exit only triggers when the
IOAPIC is implemented in userspace; the userspace IOAPIC should process
the EOI and retrigger the interrupt if it is still asserted.  Vector is
the LAPIC interrupt vector for which the EOI was received."

Also, you have to move it inside the anonymous union just above.  It's
okay in the header, but wrong in the documentation.

Paolo

>  	union {
>  		struct kvm_sync_regs regs;
>  		char padding[1024];
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index af3225a..2778d36 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -540,6 +540,9 @@ struct kvm_vcpu_arch {
>  	struct {
>  		bool pv_unhalted;
>  	} pv;
> +
> +	u64 eoi_exit_bitmaps[4];
> +	int pending_ioapic_eoi;
>  };
>  
>  struct kvm_lpage_info {
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 92f4c98..28eb946 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -869,6 +869,15 @@ int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2)
>  
>  static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>  {
> +	if (irqchip_split(apic->vcpu->kvm)) {
> +		if (test_bit(vector,
> +			     (void *) apic->vcpu->arch.eoi_exit_bitmaps)) {
> +			apic->vcpu->arch.pending_ioapic_eoi = vector;
> +			kvm_make_request(KVM_REQ_IOAPIC_EOI_EXIT, apic->vcpu);
> +		}
> +		return;
> +	}
> +
>  	if (kvm_ioapic_handles_vector(apic->vcpu->kvm, vector)) {
>  		int trigger_mode;
>  		if (apic_test_vector(vector, apic->regs + APIC_TMR))
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 19c8980..5e01810 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6392,6 +6392,17 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  			kvm_handle_pmu_event(vcpu);
>  		if (kvm_check_request(KVM_REQ_PMI, vcpu))
>  			kvm_deliver_pmi(vcpu);
> +		if (kvm_check_request(KVM_REQ_IOAPIC_EOI_EXIT, vcpu)) {
> +			BUG_ON(vcpu->arch.pending_ioapic_eoi > 255);
> +			if (test_bit(vcpu->arch.pending_ioapic_eoi,
> +				     (void *) vcpu->arch.eoi_exit_bitmaps)) {
> +				vcpu->run->exit_reason = KVM_EXIT_IOAPIC_EOI;
> +				vcpu->run->eoi.vector =
> +						vcpu->arch.pending_ioapic_eoi;
> +				r = 0;
> +				goto out;
> +			}
> +		}
>  		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
>  			vcpu_scan_ioapic(vcpu);
>  		if (kvm_check_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu))
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7e2b41a..c6df36f 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -134,6 +134,7 @@ static inline bool is_error_page(struct page *page)
>  #define KVM_REQ_ENABLE_IBS        23
>  #define KVM_REQ_DISABLE_IBS       24
>  #define KVM_REQ_APIC_PAGE_RELOAD  25
> +#define KVM_REQ_IOAPIC_EOI_EXIT   26
>  
>  #define KVM_USERSPACE_IRQ_SOURCE_ID		0
>  #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 1e6f6c3..826a08d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -183,6 +183,7 @@ struct kvm_s390_skeys {
>  #define KVM_EXIT_EPR              23
>  #define KVM_EXIT_SYSTEM_EVENT     24
>  #define KVM_EXIT_S390_STSI        25
> +#define KVM_EXIT_IOAPIC_EOI       26
>  
>  /* For KVM_EXIT_INTERNAL_ERROR */
>  /* Emulate instruction failed. */
> @@ -329,6 +330,10 @@ struct kvm_run {
>  			__u8 sel1;
>  			__u16 sel2;
>  		} s390_stsi;
> +		/* KVM_EXIT_IOAPIC_EOI */
> +		struct {
> +			__u8 vector;
> +		} eoi;
>  		/* Fix the size of the union. */
>  		char padding[256];
>  	};
>