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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.08.15 at 03:57, <feng.wu@xxxxxxxxx> wrote:

First of all - an empty Cc list on a patch is suspicious.

> @@ -198,6 +199,109 @@ void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci)
>      xfree(dpci);
>  }
>  
> +/*
> + * This routine handles lowest-priority interrupts using vector-hashing
> + * mechanism. As an example, modern Intel CPUs use this method to handle
> + * lowest-priority interrupts.
> + *
> + * Here is the details about the vector-hashing mechanism:
> + * 1. For lowest-priority interrupts, store all the possible destination
> + *    vCPUs in an array.
> + * 2. Use "gvec % max number of destination vCPUs" to find the right
> + *    destination vCPU in the array for the lowest-priority interrupt.
> + */
> +static struct vcpu *vector_hashing_dest(const struct domain *d,
> +                                        uint32_t dest_id,
> +                                        bool_t dest_mode,
> +                                        uint8_t gvec)
> +
> +{
> +    unsigned long *dest_vcpu_bitmap;
> +    unsigned int dest_vcpus = 0, idx;
> +    unsigned int bitmap_array_size = BITS_TO_LONGS(d->max_vcpus);
> +    struct vcpu *v, *dest = NULL;
> +    unsigned int i;
> +
> +    dest_vcpu_bitmap = xzalloc_array(unsigned long, bitmap_array_size);
> +    if ( !dest_vcpu_bitmap )
> +    {
> +        dprintk(XENLOG_G_INFO,
> +                "dom%d: failed to allocate memory\n", d->domain_id);

This dprintk() won't really help much. Please remove it.

> +        return NULL;
> +    }
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        if ( !vlapic_match_dest(vcpu_vlapic(v), NULL, APIC_DEST_NOSHORT,
> +                                dest_id, dest_mode) )
> +            continue;
> +
> +        __set_bit(v->vcpu_id, dest_vcpu_bitmap);
> +        dest_vcpus++;
> +    }
> +
> +    if ( dest_vcpus != 0 )
> +    {
> +        unsigned int mod = gvec % dest_vcpus;
> +        idx = 0;

You don't use idx before here. Declare it here, which also avoids
me telling you that you placed the blank line wrongly.

> +        for ( i = 0; i <= mod; i++ )
> +        {
> +            idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx) + 1;
> +            BUG_ON(idx >= d->max_vcpus);
> +        }
> +        idx--;
> +
> +        dest = d->vcpu[idx];

Just [idx - 1] please.

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

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

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

> +            }

(Not only) depending on the answer, I'd consider adding an "else
printk()" here.

Jan

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