From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v11 3/3] x86, apicv: add virtual interrupt delivery support Date: Mon, 21 Jan 2013 07:03:00 +0200 Message-ID: <20130121050300.GA25818@redhat.com> References: <1358331672-32384-1-git-send-email-yang.z.zhang@intel.com> <1358331672-32384-4-git-send-email-yang.z.zhang@intel.com> <20130120125136.GA5119@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "kvm@vger.kernel.org" , "Shan, Haitao" , "mtosatti@redhat.com" , "Zhang, Xiantao" , "Tian, Kevin" To: "Zhang, Yang Z" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:2614 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750699Ab3AUFDD (ORCPT ); Mon, 21 Jan 2013 00:03:03 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jan 21, 2013 at 12:49:01AM +0000, Zhang, Yang Z wrote: > Gleb Natapov wrote on 2013-01-20: > > On Thu, Jan 17, 2013 at 01:26:03AM +0000, Zhang, Yang Z wrote: > >> Previous patch is stale. Resend the new patch. > >> The only change is clear EOI and SELF-IPI reg in msr bitmap when vid is enabled. > >> > >> ------------------------ > >> @@ -340,6 +325,8 @@ static inline int apic_find_highest_irr(struct kvm_lapic > > *apic) > >> { > >> int result; > >> + /* Note that irr_pending is just a hint. It will be always > >> + * true with virtual interrupt delivery enabled. */ > > This is not correct format for multi-line comments. > Sure, will correct it here and below. > > >> +static 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(); > >> +} > > The logic in this function belongs to lapic code. The only thing > > that is specific to vmx in the function is setting of the bit in > > vmx->eoi_exit_bitmap, but since eoi_exit_bitmap is calculated and > > loaded during same vcpu entry we do not need vmx->eoi_exit_bitmap at > > all. Declare it on a stack in vmx_update_eoi_exitmap() and pass it to > > set_eoi_exitmap() and vmx_load_eoi_exitmap(). > IIRC, this logic is in lapic before v7. And you suggested to move the whole function into vmx code. > So, it better to move back to lapic file? > IIRC I suggested to call it only from vmx, not move it there. Before that the calculation was done even with vid disabled and only result was ignored. With current logic KVM_REQ_EOIBITMAP will be set only with vid enabled so the calculation will not be done needlessly. > >> @@ -115,6 +116,42 @@ static void update_handled_vectors(struct kvm_ioapic > > *ioapic) > >> smp_wmb(); > >> } > >> +void set_eoi_exitmap(struct kvm_vcpu *vcpu) > >> +{ > > This function is exported from the file and need to have more unique > > name. kvm_ioapic_calculate_eoi_exitmap() for instance. > Ok. > > >> @@ -156,6 +193,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); > > ioapic_write_indirect() is called under ioapic->lock, > > ioapic_update_eoi_exitmap() takes the same lock. Have you tested the > > code? > ioapic_update_eoi_exitmap doesn't take any lock. > Sorry. You are correct. Confused between different functions. > I will do a full testing for every patch before sending out. It covers both windows and Linux guest. > We are getting close so please test with userspace irq chip too. -- Gleb.