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

* Re: [RFC v2 08/15] Update IRTE according to guest interrupt config changes
@ 2015-06-12  9:40 Wu, Feng
  2015-06-12 10:40 ` Jan Beulich
  0 siblings, 1 reply; 7+ messages in thread
From: Wu, Feng @ 2015-06-12  9:40 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tian, Kevin, keir, george.dunlap, andrew.cooper3, xen-devel,
	Zhang, Yang Z, Wu, Feng

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, June 09, 2015 11:06 PM
> To: Wu, Feng
> Cc: andrew.cooper3@citrix.com; george.dunlap@eu.citrix.com; Tian, Kevin;
> Zhang, Yang Z; xen-devel@lists.xen.org; keir@xen.org
> Subject: Re: [RFC v2 08/15] Update IRTE according to guest interrupt config
> changes
> 
> >>> On 08.05.15 at 11:07, <feng.wu@intel.com> 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 we use vCPU IDs or bitmap, we need to iterate all the vCPUs in the
domain to find the right vCPU from the vcpu_id, right?

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

Good suggestion!

Thanks,
Feng

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

^ permalink raw reply	[flat|nested] 7+ messages in thread