Re: [Xen-devel] [RFC v2 08/15] Update IRTE according to guest interrupt config changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.15 at 11:07, <feng.wu@xxxxxxxxx> wrote:
> +static bool_t pi_find_dest_vcpu(struct domain *d, uint8_t dest_id,
> +                                uint8_t dest_mode, uint8_t delivery_mode,
> +                                uint8_t gvec, struct vcpu **dest_vcpu)
> +{
> +    struct vcpu *v, **dest_vcpu_array;
> +    unsigned int dest_vcpu_num = 0;
> +    int ret;

This, being given as operand to "return", should match in type with
the function's return type.

> +    dest_vcpu_array = xzalloc_array(struct vcpu *, d->max_vcpus);

You realize that this can be quite big an allocation? (You could at
least halve it by storing vCPU IDs instead of pointers, but if I'm
not mistaken this could even be a simple bitmap.)

> +    if ( !dest_vcpu_array )
> +    {
> +        dprintk(XENLOG_G_INFO,
> +                "dom%d: failed to allocate memeory.\n", d->domain_id);

Please fix the typo and remove the stop.

> +        return 0;
> +    }
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        if ( !vlapic_match_dest(vcpu_vlapic(v), NULL, 0,
> +                                dest_id, dest_mode) )
> +            continue;
> +
> +        dest_vcpu_array[dest_vcpu_num++] = v;
> +    }
> +
> +    if ( delivery_mode == dest_LowestPrio )
> +    {
> +        if (  dest_vcpu_num != 0 )
> +        {
> +            *dest_vcpu = dest_vcpu_array[gvec % dest_vcpu_num];
> +            ret = 1;
> +        }
> +        else
> +            ret = 0;
> +    }
> +    else if (  dest_vcpu_num == 1 )
> +    {
> +        *dest_vcpu = dest_vcpu_array[0];
> +        ret = 1;
> +    }
> +    else
> +        ret = 0;
> +
> +    xfree(dest_vcpu_array);
> +    return ret;
> +}

Blank line before final return statement please.

> @@ -330,11 +398,40 @@ 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;
>          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 )
> +        {
> +            struct vcpu *vcpu = NULL;
> +
> +            if ( !pi_find_dest_vcpu(d, dest, dest_mode, delivery_mode,
> +                                    pirq_dpci->gmsi.gvec, &vcpu) )

Why not have the function return the vCPU instead of passing it
via indirection?

> +            {
> +                dprintk(XENLOG_G_WARNING,
> +                        "%pv: failed to find the dest vCPU for PI, guest "
> +                        "vector:%u use software way to deliver the "
> +                        " interrupts.\n", vcpu, pirq_dpci->gmsi.gvec);

You shouldn't be printing the (NULL) vCPU here. And please print
vectors as hex values.

> +                break;
> +            }
> +
> +            if ( pi_update_irte( vcpu, info, pirq_dpci->gmsi.gvec ) != 0 )
> +            {
> +                dprintk(XENLOG_G_WARNING,
> +                        "%pv: failed to update PI IRTE, guest vector:%u "
> +                        "use software way to deliver the interrupts.\n",
> +                        vcpu, pirq_dpci->gmsi.gvec);
> +
> +                break;
> +            }
> +        }
> +
>          break;

By using if() / else if() you could drop _both_ break-s you add.

Jan


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