>>> On 06.09.15 at 06:54, <feng...@intel.com> wrote: >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: Friday, September 04, 2015 11:59 PM >> >>> On 25.08.15 at 03:57, <feng...@intel.com> wrote: >> >> First of all - an empty Cc list on a patch is suspicious. > > I did Cc you for this patch. Why do you say "an empty Cc list"?
Oops, sorry - this really belongs to patch 12. >> > +static struct vcpu *pi_find_dest_vcpu(const struct domain *d, uint32_t >> dest_id, >> > + bool_t dest_mode, uint8_t >> delivery_mode, >> > + uint8_t gvec) >> > +{ >> > + unsigned int dest_vcpus = 0; >> > + struct vcpu *v, *dest = NULL; >> > + >> > + if ( delivery_mode == dest_LowestPrio ) >> > + return vector_hashing_dest(d, dest_id, dest_mode, gvec); >> >> So at this point delivery_mode == dest_Fixed, right? > > It won't be dest_LowestPrio here, so it might be proper to add > else if (delivery_mode == dest_Fixed) for the code below. So the > question is: Can deliver modes other than lowest priority and fixed, > such as NMI, SMI, etc. hit here? Any ideas? That's for you to validate. If it can't, add an ASSERT(). If it can, use an if() as you suggest. >> > + for_each_vcpu ( d, v ) >> > + { >> > + if ( !vlapic_match_dest(vcpu_vlapic(v), NULL, >> APIC_DEST_NOSHORT, >> > + dest_id, dest_mode) ) >> > + continue; >> > + >> > + dest_vcpus++; >> > + dest = v; >> > + } >> > + >> > + /* >> > + * For fixed destination, we only handle single-destination >> > + * interrupts. >> > + */ >> > + if ( dest_vcpus == 1 ) >> > + return dest; >> >> Is it thus even possible for the if() condition to be false? > > It can be false if the interrupts has multiple destination with Fixed > deliver mode. > >> If it isn't, >> returning early from the loop would seem the better option. And >> even if it is, I would question whether delivering the interrupt to >> the first match isn't going to be better than dropping it. > > Here, if we deliver all the interrupts to the first match, only this > vCPU will receive all the interrupts, even though the irq affinity > shows it has multiple destination. I don't think this is correct. > Furthermore, is there any performance issues if the interrupt frequency > is high and the matched vCPU cannot handle them in time? So here, I > just leave these kind of interrupts to legacy interrupt remapping > mechanism. Oh, okay, if they're not getting lost, that's fine then. >> > dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode); >> > pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id; >> > spin_unlock(&d->event_lock); >> > if ( dest_vcpu_id >= 0 ) >> > hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]); >> > + >> > + /* Use interrupt posting if it is supported. */ >> > + if ( iommu_intpost ) >> > + { >> > + const struct vcpu *vcpu = pi_find_dest_vcpu(d, dest, >> dest_mode, >> > + delivery_mode, >> pirq_dpci->gmsi.gvec); >> > + >> > + if ( vcpu ) >> > + { >> > + rc = pi_update_irte( vcpu, info, pirq_dpci->gmsi.gvec ); >> > + if ( unlikely(rc) ) >> > + dprintk(XENLOG_G_INFO, >> > + "%pv: failed to update PI IRTE, >> gvec:%02x\n", >> > + vcpu, pirq_dpci->gmsi.gvec); >> >> Even if only a debug build printk() - aren't you afraid that if this >> ever triggers, it will trigger a lot? And hence be pretty useless? > > I think it reaches this debug printk rarely, but a least, when it is really > failed, it > can give people some hints about why we are using interrupt remapping > instead > of interrupt posing for the external interrupts. I understand your motivation, but you don't really answer my question. (And btw., if you really mean "rarely", then there's a bug somewhere that you need to fix. It should _never_ trigger if everything is working correctly.) >> > + } >> >> (Not only) depending on the answer, I'd consider adding an "else >> printk()" here. > > So do you mean something like this: > > if ( vcpu ) > { > rc = pi_update_irte( vcpu, info, pirq_dpci->gmsi.gvec ); > if ( unlikely(rc) ) > dprintk(XENLOG_G_INFO, > "%pv: failed to update PI IRTE, gvec:%02x, use > interrupt remapping\n", > vcpu, pirq_dpci->gmsi.gvec); > else > dprintk(XENLOG_G_INFO, > "%pv: Succeed to update PI IRTE, gvec:%02x, use > interrupt posting\n", > vcpu, pirq_dpci->gmsi.gvec); > } No. I intentionally placed my question _after_ the closing brace, i.e. I suggested an "else" to the outer "if". Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel