Re: [Xen-devel] [PATCH v2 06/12] x86/IRQ: consolidate use of ->arch.cpu_mask

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.19 at 13:32, <roger.pau@xxxxxxxxxx> wrote:
> On Wed, May 08, 2019 at 07:10:29AM -0600, Jan Beulich wrote:
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -680,7 +680,7 @@ void /*__init*/ setup_ioapic_dest(void)
>>                  continue;
>>              irq = pin_2_irq(irq_entry, ioapic, pin);
>>              desc = irq_to_desc(irq);
>> -            BUG_ON(cpumask_empty(desc->arch.cpu_mask));
>> +            BUG_ON(!cpumask_intersects(desc->arch.cpu_mask, 
>> &cpu_online_map));
> 
> I wonder if maybe you could instead do:
> 
> if ( cpumask_intersects(desc->arch.cpu_mask, &cpu_online_map) )
>     set_ioapic_affinity_irq(desc, desc->arch.cpu_mask);
> else
>     ASSERT_UNREACHABLE();
> 
> I guess if the IRQ is in use by Xen itself the failure ought to be
> fatal.

And imo also when it's another one (used by Dom0). Iirc we get
here only during Dom0 boot (the commented out __init serving as
a hint). Hence I think BUG_ON() is better in this case than any
for of assertion.

>> @@ -2232,11 +2231,17 @@ int io_apic_set_pci_routing (int ioapic,
>>          return vector;
>>      entry.vector = vector;
>>  
>> -    cpumask_copy(&mask, TARGET_CPUS);
>> -    /* Don't chance ending up with an empty mask. */
>> -    if (cpumask_intersects(&mask, desc->arch.cpu_mask))
>> -        cpumask_and(&mask, &mask, desc->arch.cpu_mask);
>> -    SET_DEST(entry, logical, cpu_mask_to_apicid(&mask));
>> +    if (cpumask_intersects(desc->arch.cpu_mask, TARGET_CPUS)) {
>> +        cpumask_t *mask = this_cpu(scratch_cpumask);
>> +
>> +        cpumask_and(mask, desc->arch.cpu_mask, TARGET_CPUS);
>> +        SET_DEST(entry, logical, cpu_mask_to_apicid(mask));
>> +    } else {
>> +        printk(XENLOG_ERR "IRQ%d: no target CPU (%*pb vs %*pb)\n",
>> +               irq, nr_cpu_ids, cpumask_bits(desc->arch.cpu_mask),
>> +               nr_cpu_ids, cpumask_bits(TARGET_CPUS));
>> +        desc->status |= IRQ_DISABLED;
>> +    }
> 
> Hm, part of this file doesn't seem to use Xen coding style, but the
> chunk you add below does use it. And there are functions (like
> mask_and_ack_level_ioapic_irq that seem to use a mix of coding
> styles).
> 
> I'm not sure what's the policy here, should new chunks follow Xen's
> coding style?

Well, I've decided to match surrounding code's style, until the file
gets morphed into consistent shape.

Jan



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