Re: [PATCH v7 2/3] x86, apicv: add virtual interrupt delivery support

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Dec 17, 2012 at 01:30:49PM +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Virtual interrupt delivery avoids KVM to inject vAPIC interrupts
> manually, which is fully taken care of by the hardware. This needs
> some special awareness into existing interrupr injection path:
> 
> - for pending interrupt, instead of direct injection, we may need
>   update architecture specific indicators before resuming to guest.
> 
> - A pending interrupt, which is masked by ISR, should be also
>   considered in above update action, since hardware will decide
>   when to inject it at right time. Current has_interrupt and
>   get_interrupt only returns a valid vector from injection p.o.v.
> 
> Signed-off-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yang Zhang <yang.z.zhang@intel.com>
> ---
>  arch/ia64/kvm/lapic.h           |    6 ++
>  arch/x86/include/asm/kvm_host.h |    6 ++
>  arch/x86/include/asm/vmx.h      |   11 +++
>  arch/x86/kvm/irq.c              |   56 +++++++++++++-
>  arch/x86/kvm/lapic.c            |   65 ++++++++++-------
>  arch/x86/kvm/lapic.h            |   28 ++++++-
>  arch/x86/kvm/svm.c              |   24 ++++++
>  arch/x86/kvm/vmx.c              |  154 ++++++++++++++++++++++++++++++++++++++-
>  arch/x86/kvm/x86.c              |   11 ++-
>  include/linux/kvm_host.h        |    2 +
>  virt/kvm/ioapic.c               |   36 +++++++++
>  virt/kvm/ioapic.h               |    1 +
>  virt/kvm/irq_comm.c             |   20 +++++
>  13 files changed, 379 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/ia64/kvm/lapic.h b/arch/ia64/kvm/lapic.h
> index c5f92a9..cb59eb4 100644
> --- a/arch/ia64/kvm/lapic.h
> +++ b/arch/ia64/kvm/lapic.h
> @@ -27,4 +27,10 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq);
>  #define kvm_apic_present(x) (true)
>  #define kvm_lapic_enabled(x) (true)
>  
> +static inline void kvm_update_eoi_exitmap(struct kvm *kvm,
> +					struct kvm_lapic_irq *irq)
> +{
> +	/* IA64 has no apicv supporting, do nothing here */
> +}
> +
>  #endif
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c431b33..b63a144 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -697,6 +697,11 @@ struct kvm_x86_ops {
>  	void (*enable_nmi_window)(struct kvm_vcpu *vcpu);
>  	void (*enable_irq_window)(struct kvm_vcpu *vcpu);
>  	void (*update_cr8_intercept)(struct kvm_vcpu *vcpu, int tpr, int irr);
> +	int (*has_virtual_interrupt_delivery)(struct kvm_vcpu *vcpu);
> +	void (*update_apic_irq)(struct kvm_vcpu *vcpu, int max_irr);
> +	void (*update_eoi_exitmap)(struct kvm *kvm, struct kvm_lapic_irq *irq);
> +	void (*reset_eoi_exitmap)(struct kvm_vcpu *vcpu);
> +	void (*load_eoi_exitmap)(struct kvm_vcpu *vcpu);
>  	int (*set_tss_addr)(struct kvm *kvm, unsigned int addr);
>  	int (*get_tdp_level)(void);
>  	u64 (*get_mt_mask)(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio);

EOI exit bitmap is problematic (its racy). Please do this:

1. Make a synchronous (1) KVM_REQ_EOIBITMAP request on IOAPIC register
modifications which require EOI exit bitmap updates.
2. On VM-entry, during KVM_REQ_EOIBITMAP processing, each checks IOAPIC map
and adjusts its own EOI exit bitmap VMCS registers.

1) that waits until remote executing VCPUs have acknowledge the request,
using make_all_cpus_request (see virt/kvm/kvm_main.c), similarly to
remote TLB flushes.

What is the problem now: there is no control over _when_ a VCPU updates
its EOI exit bitmap VMCS register from the (remotely updated) master EOI
exit bitmap. The VCPU can be processing a KVM_REQ_EOIBITMAP relative to
a precedence IOAPIC register write while the current IOAPIC register
write is updating the EOI exit bitmap. There is no way to fix that
without locking (which can be avoided if the IOAPIC->EOI exit bitmap
synchronization is vcpu local).

Questions below.

> @@ -236,12 +225,14 @@ static inline void kvm_apic_set_id(struct kvm_lapic *apic, u8 id)
>  {
>  	apic_set_reg(apic, APIC_ID, id << 24);
>  	recalculate_apic_map(apic->vcpu->kvm);
> +	ioapic_update_eoi_exitmap(apic->vcpu->kvm);
>  }

Why is it necessary to update EOI exit bitmap here?

>  static inline void kvm_apic_set_ldr(struct kvm_lapic *apic, u32 id)
>  {
>  	apic_set_reg(apic, APIC_LDR, id);
>  	recalculate_apic_map(apic->vcpu->kvm);
> +	ioapic_update_eoi_exitmap(apic->vcpu->kvm);
>  }

And here?

> +/*
> + * this interface assumes a trap-like exit, which has already finished
> + * desired side effect including vISR and vPPR update.
> + */
> +void kvm_apic_set_eoi_accelerated(struct kvm_vcpu *vcpu, int vector)
> +{
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	trace_kvm_eoi(apic, vector);
> +
> +	kvm_ioapic_send_eoi(apic, vector);
> +	kvm_make_request(KVM_REQ_EVENT, apic->vcpu);
> +}

Why is it necessary to set KVM_REQ_EVENT here?

>  			apic_set_reg(apic, APIC_DFR, val | 0x0FFFFFFF);
>  			recalculate_apic_map(apic->vcpu->kvm);
> +			ioapic_update_eoi_exitmap(apic->vcpu->kvm);
>  		} else
>  			ret = 1;
>  		break;
> @@ -1318,6 +1330,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
>  		else
>  			static_key_slow_inc(&apic_hw_disabled.key);
>  		recalculate_apic_map(vcpu->kvm);
> +		ioapic_update_eoi_exitmap(apic->vcpu->kvm);
>  	}

Again, why all this EOI exit bitmap updates?

> +static void svm_update_eoi_exitmap(struct kvm *kvm, struct kvm_lapic_irq *irq)
> +{
> +	return ;
> +}
> +
> +static void svm_reset_eoi_exitmap(struct kvm_vcpu *vcpu)
> +{
> +	return ;
> +}
> +
> +static void svm_load_eoi_exitmap(struct kvm_vcpu *vcpu)
> +{
> +	return ;
> +}

Please mind coding style.

> +static void vmx_update_apic_irq(struct kvm_vcpu *vcpu, int max_irr)
> +{
> +	if (max_irr == -1)
> +		return ;

Coding style.