[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC v2 08/15] Update IRTE according to guest interrupt config changes
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: Tuesday, June 09, 2015 11:06 PM > To: Wu, Feng > Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin; > Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx > Subject: Re: [RFC v2 08/15] Update IRTE according to guest interrupt config > changes > > >>> 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 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |