Re: [Xen-devel] [v3 11/15] Update IRTE according to guest interrupt config changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.06.15 at 07:18, <feng.wu@xxxxxxxxx> wrote:
> +static struct vcpu *pi_find_dest_vcpu(struct domain *d, uint8_t dest_id,

Looks like there's nothing modifying d, i.e. should be const.

But how can dest_id be uint8_t when we support x2APIC in the
guest?

> +                                      uint8_t dest_mode, uint8_t 
> delivery_mode,

According to vlapic_match_dest()'s parameter types dest_mode
ought to be bool_t.

> +                                      uint8_t gvec)
> +{
> +    unsigned long *dest_vcpu_bitmap = NULL;

Pointless initializer.

> +    unsigned int dest_vcpu_num = 0, idx = 0;
> +    int size = (d->max_vcpus + BITS_PER_LONG - 1) / BITS_PER_LONG;

unsigned int size = BITS_TO_LONGS(d->max_vcpus);

> +    struct vcpu *v, *dest = NULL;
> +    int i;

unsigned int

> +    dest_vcpu_bitmap = xzalloc_array(unsigned long, size);
> +    if ( !dest_vcpu_bitmap )
> +    {
> +        dprintk(XENLOG_G_INFO,
> +                "dom%d: failed to allocate memory\n", d->domain_id);
> +        return NULL;
> +    }
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        if ( !vlapic_match_dest(vcpu_vlapic(v), NULL, 0,
> +                                dest_id, dest_mode) )
> +            continue;
> +
> +        __set_bit(v->vcpu_id, dest_vcpu_bitmap);
> +        dest_vcpu_num++;
> +    }
> +
> +    if ( delivery_mode == dest_LowestPrio )
> +    {
> +        if (  dest_vcpu_num != 0 )
> +        {
> +            for ( i = 0; i <= gvec % dest_vcpu_num; i++)

Unless the compiler recognizes this, you do a divide on each iteration.
Make this the initialized of i instead.

> +                idx = find_next_bit(dest_vcpu_bitmap, d->max_vcpus, idx) + 1;
> +            idx--;
> +
> +            BUG_ON(idx >= d->max_vcpus || idx < 0);

If you really think you need this, then this is too late: find_next_bit()
in undefined for an out of range last argument.

> +            dest = d->vcpu[idx];
> +        }
> +    }
> +    else if (  dest_vcpu_num == 1 )
> +    {
> +        idx = find_first_bit(dest_vcpu_bitmap, d->max_vcpus);
> +        BUG_ON(idx >= d->max_vcpus || idx < 0);

This seems pretty pointless considering how dest_vcpu_num
ends up being non-zero.

> +        dest = d->vcpu[idx];
> +    }

else BUG()? Or at least some gdprintk()? Oh, I see you have one in
the caller - I guess that's fine then.

> @@ -330,11 +403,32 @@ 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 = pi_find_dest_vcpu(d, dest, dest_mode,
> +                                        delivery_mode, pirq_dpci->gmsi.gvec);
> +
> +            if ( !vcpu )
> +                dprintk(XENLOG_G_WARNING,
> +                        "dom%u: failed to find the dest vCPU for PI, guest "
> +                        "vector:0x%x use software way to deliver the "
> +                        " interrupts.\n", d->domain_id, 
> pirq_dpci->gmsi.gvec);
> +            else if ( pi_update_irte( vcpu, info, pirq_dpci->gmsi.gvec ) != 
> 0 )
> +                dprintk(XENLOG_G_WARNING,
> +                        "%pv: failed to update PI IRTE, guest vector:0x%x "
> +                        "use software way to deliver the interrupts.\n",
> +                        vcpu, pirq_dpci->gmsi.gvec);

%#x instead of 0x%x generally please. For vectors, however,
%02x please. Also please don't break long format strings, so one
can grep for them. I'd suggest shortening them as much as possible
anyway (namely drop "use software way to deliver the interrupts.").

Jan

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