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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
