Re: [Xen-devel] [PATCH v4 06/13] x86/IOMMU: don't restrict IRQ affinities to online CPUs

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jul 16, 2019 at 10:20:10AM +0000, Jan Beulich wrote:
> On 16.07.2019 11:12, Roger Pau Monné  wrote:
> > On Tue, Jul 16, 2019 at 07:40:57AM +0000, Jan Beulich wrote:
> >> In line with "x86/IRQ: desc->affinity should strictly represent the
> >> requested value" the internally used IRQ(s) also shouldn't be restricted
> >> to online ones. Make set_desc_affinity() (set_msi_affinity() then does
> >> by implication) cope with a NULL mask being passed (just like
> >> assign_irq_vector() does), and have IOMMU code pass NULL instead of
> >> &cpu_online_map (when, for VT-d, there's no NUMA node information
> >> available).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > 
> > LGTM, just one patch style comment and one code comment:
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Thanks.
> 
> >> --- a/xen/arch/x86/irq.c
> >> +++ b/xen/arch/x86/irq.c
> >> @@ -796,18 +796,26 @@ unsigned int set_desc_affinity(struct ir
> >>        unsigned long flags;
> >>        cpumask_t dest_mask;
> >>    
> >> -    if (!cpumask_intersects(mask, &cpu_online_map))
> >> +    if ( mask && !cpumask_intersects(mask, &cpu_online_map) )
> >>            return BAD_APICID;
> >>    
> >>        spin_lock_irqsave(&vector_lock, flags);
> >> -    ret = _assign_irq_vector(desc, mask);
> >> +    ret = _assign_irq_vector(desc, mask ?: TARGET_CPUS);
> >>        spin_unlock_irqrestore(&vector_lock, flags);
> > 
> > I think the patch is somehow mangled at least on my end, there's one
> > prepended extra space in the non-modified lines AFAICT.
> 
> Well, yes, hence the last sentence in the cover letter and the attached
> patches there. It is the mail system (more likely server than client)
> over here which causes this issue (everywhere for me).

Oh, sorry to hear that. Hope you get that sorted out, I guess it's
causing quite a lot of pain for more people at SUSE also.

> >>    
> >> -    if (ret < 0)
> >> +    if ( ret < 0 )
> >>            return BAD_APICID;
> >>    
> >> -    cpumask_copy(desc->affinity, mask);
> > 
> > AFAICT you could also avoid the if and just do the same as in the
> > assign_irq_vector call above and pass TARGET_CPUS if mask is NULL?
> 
> Are you talking about the if() in context above, or the one you've
> stripped (immediately following the last quoted line of the patch)?
> For the one in context I don't see how the rest of your remark is
> related. For the other one - no, strictly not, as that would be
> against the purpose of this change: We specifically want to _not_
> restrict desc->affinity to online CPUs only (yet that's what
> TARGET_CPUS resolves to).

Yes, that was my remark - which is wrong as you pointed out. I guess
you could use cpu_possible_map, but anyway the current approach is OK
IMO.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel