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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, September 04, 2015 11:59 PM
> To: Wu, Feng
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v6 13/18] Update IRTE according to guest interrupt config
> changes
> 
> >>> 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"?

> 
> > +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?

> 
> > +    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.

> 
> > @@ -329,11 +433,30 @@ int pt_irq_create_bind(
> >          /* Calculate dest_vcpu_id for MSI-type pirq migration. */
> >          dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK;
> >          dest_mode = !!(pirq_dpci->gmsi.gflags & VMSI_DM_MASK);
> > +        delivery_mode = (pirq_dpci->gmsi.gflags >>
> GFLAGS_SHIFT_DELIV_MODE) &
> > +                        VMSI_DELIV_MASK;
> 
> In numbers (gflags >> 12) & 0x7000, which is likely not what you
> want.
> 
> >          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.

> 
> > +            }
> 
> (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);
            }

Thanks,
Feng

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel