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

[Jan Beulich <JBeulich@xxxxxxxx>]

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).

>>    
>> -    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).

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