From: Marcelo Tosatti <mtosatti@redhat.com>
To: Yang Zhang <yang.z.zhang@intel.com>
Cc: kvm@vger.kernel.org, gleb@redhat.com, haitao.shan@intel.com,
Kevin Tian <kevin.tian@intel.com>
Subject: Re: [PATCH v9 3/3] x86, apicv: add virtual interrupt delivery support
Date: Thu, 10 Jan 2013 19:36:28 -0200 [thread overview]
Message-ID: <20130110213628.GB9830@amt.cnet> (raw)
In-Reply-To: <1357802768-15816-4-git-send-email-yang.z.zhang@intel.com>
Hi,
Getting into good shape.
On Thu, Jan 10, 2013 at 03:26:08PM +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/x86/include/asm/kvm_host.h | 5 +
> arch/x86/include/asm/vmx.h | 11 +++
> arch/x86/kvm/irq.c | 56 +++++++++++-
> arch/x86/kvm/lapic.c | 72 +++++++++------
> arch/x86/kvm/lapic.h | 23 +++++
> arch/x86/kvm/svm.c | 18 ++++
> arch/x86/kvm/vmx.c | 191 +++++++++++++++++++++++++++++++++++++--
> arch/x86/kvm/x86.c | 14 +++-
> include/linux/kvm_host.h | 3 +
> virt/kvm/ioapic.c | 18 ++++
> virt/kvm/ioapic.h | 4 +
> virt/kvm/irq_comm.c | 22 +++++
> virt/kvm/kvm_main.c | 5 +
> 13 files changed, 399 insertions(+), 43 deletions(-)
>
> static void recalculate_apic_map(struct kvm *kvm)
> {
> struct kvm_apic_map *new, *old = NULL;
> @@ -236,12 +219,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);
> }
Move ioapic_update_eoi_exitmap into recalculate_apic_map.
> 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);
> }
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b203ce7..990409a 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -434,6 +434,7 @@ struct vcpu_vmx {
> bool rdtscp_enabled;
>
> bool virtual_x2apic_enabled;
> + unsigned long eoi_exit_bitmap[4];
Use DECLARE_BITMAP (unsigned long is 4 bytes on 32-bit host).
> /* Support for a guest hypervisor (nested VMX) */
> struct nested_vmx nested;
> @@ -783,7 +784,8 @@ static inline bool cpu_has_vmx_apic_register_virt(void)
>
> static inline bool cpu_has_vmx_virtual_intr_delivery(void)
> {
> - return false;
> + return vmcs_config.cpu_based_2nd_exec_ctrl &
> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;
> }
>
> static inline bool cpu_has_vmx_flexpriority(void)
> @@ -2565,7 +2567,8 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
> SECONDARY_EXEC_PAUSE_LOOP_EXITING |
> SECONDARY_EXEC_RDTSCP |
> SECONDARY_EXEC_ENABLE_INVPCID |
> - SECONDARY_EXEC_APIC_REGISTER_VIRT;
> + SECONDARY_EXEC_APIC_REGISTER_VIRT |
> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY;
> if (adjust_vmx_controls(min2, opt2,
> MSR_IA32_VMX_PROCBASED_CTLS2,
> &_cpu_based_2nd_exec_control) < 0)
> @@ -2579,7 +2582,8 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>
> if (!(_cpu_based_exec_control & CPU_BASED_TPR_SHADOW))
> _cpu_based_2nd_exec_control &= ~(
> - SECONDARY_EXEC_APIC_REGISTER_VIRT);
> + SECONDARY_EXEC_APIC_REGISTER_VIRT |
> + SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
Nevermind the earlier comment about ().
> +static void set_eoi_exitmap_one(struct kvm_vcpu *vcpu,
> + u32 vector)
> +{
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> + if (WARN_ONCE((vector > 255),
> + "KVM VMX: vector (%d) out of range\n", vector))
> + return;
> + __set_bit(vector, vmx->eoi_exit_bitmap);
> +}
> +
> +void vmx_check_ioapic_entry(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
> +{
> + struct kvm_lapic **dst;
> + struct kvm_apic_map *map;
> + unsigned long bitmap = 1;
> + int i;
> +
> + rcu_read_lock();
> + map = rcu_dereference(vcpu->kvm->arch.apic_map);
> +
> + if (unlikely(!map)) {
> + set_eoi_exitmap_one(vcpu, irq->vector);
> + goto out;
> + }
> +
> + if (irq->dest_mode == 0) { /* physical mode */
> + if (irq->delivery_mode == APIC_DM_LOWEST ||
> + irq->dest_id == 0xff) {
> + set_eoi_exitmap_one(vcpu, irq->vector);
> + goto out;
> + }
> + dst = &map->phys_map[irq->dest_id & 0xff];
> + } else {
> + u32 mda = irq->dest_id << (32 - map->ldr_bits);
> +
> + dst = map->logical_map[apic_cluster_id(map, mda)];
> +
> + bitmap = apic_logical_id(map, mda);
> + }
> +
> + for_each_set_bit(i, &bitmap, 16) {
> + if (!dst[i])
> + continue;
> + if (dst[i]->vcpu == vcpu) {
> + set_eoi_exitmap_one(vcpu, irq->vector);
> + break;
> + }
> + }
> +
> +out:
> + rcu_read_unlock();
> +}
> +
> +static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu)
> +{
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> + vmcs_write64(EOI_EXIT_BITMAP0, vmx->eoi_exit_bitmap[0]);
> + vmcs_write64(EOI_EXIT_BITMAP1, vmx->eoi_exit_bitmap[1]);
> + vmcs_write64(EOI_EXIT_BITMAP2, vmx->eoi_exit_bitmap[2]);
> + vmcs_write64(EOI_EXIT_BITMAP3, vmx->eoi_exit_bitmap[3]);
> +}
> +
> +static void vmx_update_eoi_exitmap(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
> + union kvm_ioapic_redirect_entry *e;
> + struct kvm_lapic_irq irqe;
> + int index;
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> + /* clear eoi exit bitmap */
> + memset(vmx->eoi_exit_bitmap, 0, 32);
> +
> + /* traverse ioapic entry to set eoi exit bitmap*/
> + for (index = 0; index < IOAPIC_NUM_PINS; index++) {
> + e = &ioapic->redirtbl[index];
> + if (!e->fields.mask &&
> + (e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
> + kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
> + index))) {
> + irqe.dest_id = e->fields.dest_id;
> + irqe.vector = e->fields.vector;
> + irqe.dest_mode = e->fields.dest_mode;
> + irqe.delivery_mode = e->fields.delivery_mode << 8;
> + vmx_check_ioapic_entry(vcpu, &irqe);
> +
> + }
> + }
> +
> + vmx_load_eoi_exitmap(vcpu);
> +}
> +
> static void vmx_complete_atomic_exit(struct vcpu_vmx *vmx)
> {
> u32 exit_intr_info;
> @@ -7553,6 +7726,10 @@ static struct kvm_x86_ops vmx_x86_ops = {
> .update_cr8_intercept = update_cr8_intercept,
> .enable_virtual_x2apic_mode = vmx_enable_virtual_x2apic_mode,
> .disable_virtual_x2apic_mode = vmx_disable_virtual_x2apic_mode,
> + .has_virtual_interrupt_delivery = vmx_has_virtual_interrupt_delivery,
> + .update_apic_irq = vmx_update_apic_irq,
> + .update_eoi_exitmap = vmx_update_eoi_exitmap,
> + .set_svi = vmx_set_svi,
>
> .set_tss_addr = vmx_set_tss_addr,
> .get_tdp_level = get_ept_level,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1c9c834..e6d8227 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5527,7 +5527,7 @@ static void inject_pending_event(struct kvm_vcpu *vcpu)
> vcpu->arch.nmi_injected = true;
> kvm_x86_ops->set_nmi(vcpu);
> }
> - } else if (kvm_cpu_has_interrupt(vcpu)) {
> + } else if (kvm_cpu_has_injectable_intr(vcpu)) {
> if (kvm_x86_ops->interrupt_allowed(vcpu)) {
> kvm_queue_interrupt(vcpu, kvm_cpu_get_interrupt(vcpu),
> false);
> @@ -5648,6 +5648,11 @@ 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_EOIBITMAP, vcpu)) {
> + mutex_lock(&vcpu->kvm->arch.vioapic->eoimap_lock);
> + kvm_x86_ops->update_eoi_exitmap(vcpu);
> + mutex_unlock(&vcpu->kvm->arch.vioapic->eoimap_lock);
> + }
Take ioapic lock and irq_lock mutex.
> +void ioapic_update_eoi_exitmap(struct kvm *kvm)
> +{
> +#ifdef CONFIG_X86
> + struct kvm_vcpu *vcpu = kvm->vcpus[0];
> + struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> +
> + /* If vid is enabled in one of vcpus, then other
> + * vcpus also enabled it. */
> + if (!kvm_apic_vid_enabled(vcpu) || !ioapic)
> + return;
Is it even possible to call ioapic_update_eoi_exitmap() if
kvm->arch.vioapic == NULL?
> + kvm_make_update_eoibitmap_request(kvm);
> +#endif
> +}
> +
> static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
> {
> unsigned index;
> @@ -156,6 +170,7 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
> if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG
> && ioapic->irr & (1 << index))
> ioapic_service(ioapic, index);
> + ioapic_update_eoi_exitmap(ioapic->kvm);
> break;
> }
> }
> @@ -415,6 +430,9 @@ int kvm_ioapic_init(struct kvm *kvm)
> ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS, ioapic->base_address,
> IOAPIC_MEM_LENGTH, &ioapic->dev);
> mutex_unlock(&kvm->slots_lock);
> +#ifdef CONFIG_X86
> + mutex_init(&ioapic->eoimap_lock);
> +#endif
> if (ret < 0) {
> kvm->arch.vioapic = NULL;
> kfree(ioapic);
> diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> index a30abfe..34544ce 100644
> --- a/virt/kvm/ioapic.h
> +++ b/virt/kvm/ioapic.h
> @@ -47,6 +47,9 @@ struct kvm_ioapic {
> void (*ack_notifier)(void *opaque, int irq);
> spinlock_t lock;
> DECLARE_BITMAP(handled_vectors, 256);
> +#ifdef CONFIG_X86
> + struct mutex eoimap_lock;
> +#endif
> };
>
> #ifdef DEBUG
> @@ -82,5 +85,6 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> struct kvm_lapic_irq *irq);
> int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
> +void ioapic_update_eoi_exitmap(struct kvm *kvm);
>
> #endif
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 656fa45..64aa1ab 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -22,6 +22,7 @@
>
> #include <linux/kvm_host.h>
> #include <linux/slab.h>
> +#include <linux/export.h>
Whats this for?
> #include <trace/events/kvm.h>
>
> #include <asm/msidef.h>
> @@ -237,6 +238,25 @@ int kvm_set_irq_inatomic(struct kvm *kvm, int irq_source_id, u32 irq, int level)
> return ret;
> }
>
> +bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin)
> +{
> + struct kvm_irq_ack_notifier *kian;
> + struct hlist_node *n;
> + int gsi;
> +
> + rcu_read_lock();
> + gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
> + if (gsi != -1)
> + hlist_for_each_entry_rcu(kian, n, &kvm->irq_ack_notifier_list,
> + link)
> + if (kian->gsi == gsi)
> + return true;
Forgot rcu_read_unlock();
> + rcu_read_unlock();
> +
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(kvm_irq_has_notifier);
> +
> void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
> {
> struct kvm_irq_ack_notifier *kian;
> @@ -261,6 +281,7 @@ void kvm_register_irq_ack_notifier(struct kvm *kvm,
> mutex_lock(&kvm->irq_lock);
> hlist_add_head_rcu(&kian->link, &kvm->irq_ack_notifier_list);
> mutex_unlock(&kvm->irq_lock);
> + ioapic_update_eoi_exitmap(kvm);
> }
Move inside irq_lock protection.
>
> void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
> @@ -270,6 +291,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
> hlist_del_init_rcu(&kian->link);
> mutex_unlock(&kvm->irq_lock);
> synchronize_rcu();
> + ioapic_update_eoi_exitmap(kvm);
Move both synchronize_rcu and ioapic_update_eoi_exitmap inside irq_lock
protection.
next prev parent reply other threads:[~2013-01-10 21:36 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-10 7:26 [PATCH v9 0/3] x86, apicv: Add APIC virtualization support Yang Zhang
2013-01-10 7:26 ` [PATCH v9 1/3] x86, apicv: add APICv register " Yang Zhang
2013-01-10 20:25 ` Marcelo Tosatti
2013-01-10 7:26 ` [PATCH v9 2/3] x86, apicv: add virtual x2apic support Yang Zhang
2013-01-10 7:55 ` Gleb Natapov
2013-01-10 8:32 ` Zhang, Yang Z
2013-01-10 8:52 ` Gleb Natapov
2013-01-10 11:54 ` Zhang, Yang Z
2013-01-10 12:16 ` Gleb Natapov
2013-01-10 12:22 ` Zhang, Yang Z
2013-01-10 12:34 ` Gleb Natapov
2013-01-11 7:36 ` Zhang, Yang Z
2013-01-11 16:54 ` Gleb Natapov
2013-01-14 1:03 ` Zhang, Yang Z
2013-01-14 1:14 ` Zhang, Yang Z
2013-01-11 2:37 ` Zhang, Yang Z
2013-01-11 13:51 ` Gleb Natapov
2013-01-10 8:25 ` Gleb Natapov
2013-01-10 8:31 ` Zhang, Yang Z
2013-01-10 8:53 ` Gleb Natapov
2013-01-10 7:26 ` [PATCH v9 3/3] x86, apicv: add virtual interrupt delivery support Yang Zhang
2013-01-10 8:23 ` Gleb Natapov
2013-01-10 12:04 ` Zhang, Yang Z
2013-01-10 21:36 ` Marcelo Tosatti [this message]
2013-01-11 14:09 ` Gleb Natapov
2013-01-11 17:58 ` Marcelo Tosatti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130110213628.GB9830@amt.cnet \
--to=mtosatti@redhat.com \
--cc=gleb@redhat.com \
--cc=haitao.shan@intel.com \
--cc=kevin.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=yang.z.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.