Gleb Natapov wrote on 2013-01-21:
> 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.
Maybe I misread your comments. :)
Yes, it is more reasonable to put it in lapic.
>
>>>> @@ -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.
Sure.
Best regards,
Yang
--
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
