Re: [Xen-devel] [PATCH v6 13/18] Update IRTE according to guest interrupt config changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.09.15 at 06:54, <feng.wu@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Friday, September 04, 2015 11:59 PM
>> >>> On 25.08.15 at 03:57, <feng.wu@xxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel