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

[Gleb Natapov <gleb@redhat.com>]

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.