On Mon, Sep 10, 2012 at 06:09:01PM +0300, Avi Kivity wrote:
> On 09/10/2012 04:09 PM, Gleb Natapov wrote:
> > Most interrupt are delivered to only one vcpu. Use pre-build tables to
> > find interrupt destination instead of looping through all vcpus.
> >
> > +
> > +static inline int recalculate_apic_map(struct kvm *kvm)
> > +{
> > + struct kvm_apic_map *new, *old = NULL;
> > + struct kvm_vcpu *vcpu;
> > + int i;
> > +
> > + new = kzalloc(sizeof(struct kvm_apic_map), GFP_KERNEL);
> > +
> > + if (!new)
> > + return -ENOMEM;
> > +
> > + new->ldr_bits = 8;
> > + new->flat = true;
> > + kvm_for_each_vcpu(i, vcpu, kvm) {
> > + u16 cid, lid;
> > + struct kvm_lapic *apic = vcpu->arch.apic;
> > +
> > + if (!kvm_apic_present(vcpu))
> > + continue;
> > +
> > + if (apic_x2apic_mode(apic)) {
> > + new->ldr_bits = 32;
> > + new->flat = false;
> > + } else if (kvm_apic_sw_enabled(apic) && new->flat &&
> > + kvm_apic_get_reg(apic, APIC_DFR) ==
> > APIC_DFR_CLUSTER)
> > + new->flat = false;
> > +
> > + new->phys_map[kvm_apic_id(apic)] = apic;
> > + kvm_apic_get_logical_id(kvm_apic_get_reg(apic, APIC_LDR),
> > + new->flat, new->ldr_bits, &cid, &lid);
> > +
> > + if (lid)
> > + new->logical_map[cid][ffs(lid) - 1] = apic;
> > + }
> > + mutex_lock(&kvm->arch.apic_map_lock);
> > + old = kvm->arch.apic_map;
> > + rcu_assign_pointer(kvm->arch.apic_map, new);
> > + mutex_unlock(&kvm->arch.apic_map_lock);
>
> is this not racy?
>
> cpu 0 cpu 1
> ---------------- -----------------
> new apic id
> recalculate
>
> new apic id
> recalculate
>
> take lock
> install map
> drop lock
>
> take lock
> install map
> drop lock
>
Hmm, yes. Will have to hold lock during recalculation. Not a big deal.
>
> > +
> > + if (old)
> > + call_rcu(&old->rcu, rcu_free_apic_map);
> > +
> > + return 0;
> > +}
> > +
>
> > -
> > static const unsigned int apic_lvt_mask[APIC_LVT_NUM] = {
> > LVT_MASK , /* part LVTT mask, timer mode mask added at runtime */
> > LVT_MASK | APIC_MODE_MASK, /* LVTTHMR */
> > @@ -477,6 +562,67 @@ int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct
> > kvm_lapic *source,
> > return result;
> > }
> >
> > +bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> > + struct kvm_lapic_irq *irq, int *r)
> > +{
> > + unsigned long bitmap = 1;
> > + struct kvm_lapic **dst;
> > + int i;
> > +
> > + if (irq->shorthand == APIC_DEST_SELF) {
> > + *r = kvm_apic_set_irq(src->vcpu, irq);
> > + return true;
> > + }
> > +
> > + if (irq->shorthand)
> > + return false;
> > +
> > + if (irq->dest_mode == 0) { /* physical mode */
> > + if (irq->delivery_mode == APIC_DM_LOWEST ||
> > + irq->dest_id == 0xff)
> > + return false;
> > + rcu_read_lock();
>
> Unmatched rcu_read_lock() inside a block is ugly, please start it earlier.
>
Doing rcu_read_unlock() ugly too, but if you prefer it this way...
> > + dst = &kvm->arch.apic_map->phys_map[irq->dest_id & 0xff];
>
> Like mst says, rcu_dereference().
>
Ugh.
> > + } else {
> > + u16 cid, lid;
> > + u32 mda = irq->dest_id;
> > +
> > + rcu_read_lock();
> > + if (kvm->arch.apic_map->ldr_bits == 8)
> > + mda <<= 24;
> > +
> > + kvm_apic_get_logical_id(mda, kvm->arch.apic_map->flat,
> > + kvm->arch.apic_map->ldr_bits, &cid, &lid);
> > + dst = kvm->arch.apic_map->logical_map[cid];
> > +
> > + bitmap = lid;
> > + if (irq->delivery_mode == APIC_DM_LOWEST) {
> > + int l = -1;
> > + for_each_set_bit(i, &bitmap, 16) {
> > + if (!dst[i])
> > + continue;
> > + if (l < 0)
> > + l = i;
> > + else if (kvm_apic_compare_prio(dst[i]->vcpu,
> > dst[l]->vcpu) < 0)
> > + l = i;
> > + }
> > +
> > + bitmap = (l >= 0) ? 1 << l : 0;
> > + }
> > + }
> > +
> > + for_each_set_bit(i, &bitmap, 16) {
> > + if (!dst[i])
> > + continue;
> > + if (*r < 0)
> > + *r = 0;
> > + *r += kvm_apic_set_irq(dst[i]->vcpu, irq);
> > + }
> > +
> > + rcu_read_unlock();
> > + return true;
> > +}
> > +
> > /*
> > * Add a pending IRQ into lapic.
> > * Return 1 if successfully added and 0 if discarded.
> > @@ -880,7 +1026,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32
> > reg, u32 val)
> > switch (reg) {
> > case APIC_ID: /* Local APIC ID */
> > if (!apic_x2apic_mode(apic))
> > - apic_set_reg(apic, APIC_ID, val);
> > + kvm_apic_set_id(apic, val >> 24);
> > else
> > ret = 1;
> > break;
> > @@ -896,15 +1042,16 @@ static int apic_reg_write(struct kvm_lapic *apic,
> > u32 reg, u32 val)
> >
> > case APIC_LDR:
> > if (!apic_x2apic_mode(apic))
> > - apic_set_reg(apic, APIC_LDR, val & APIC_LDR_MASK);
> > + kvm_apic_set_ldr(apic, val & APIC_LDR_MASK);
> > else
> > ret = 1;
> > break;
> >
> > case APIC_DFR:
> > - if (!apic_x2apic_mode(apic))
> > + if (!apic_x2apic_mode(apic)) {
> > apic_set_reg(apic, APIC_DFR, val | 0x0FFFFFFF);
> > - else
> > + recalculate_apic_map(apic->vcpu->kvm);
> > + } else
> > ret = 1;
>
> What if these have different settings on different vcpus?
>
Then table approach will not work and we need to loop. Luckily for us
spec says all CPUs have to have same DFR settings.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
