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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, May 08, 2019 at 07:10:29AM -0600, Jan Beulich wrote:
> Mixed meaning was implied so far by different pieces of code -
> disagreement was in particular about whether to expect offline CPUs'
> bits to possibly be set. Switch to a mostly consistent meaning
> (exception being high priority interrupts, which would perhaps better
> be switched to the same model as well in due course). Use the field to
> record the vector allocation mask, i.e. potentially including bits of
> offline (parked) CPUs. This implies that before passing the mask to
> certain functions (most notably cpu_mask_to_apicid()) it needs to be
> further reduced to the online subset.
> 
> The exception of high priority interrupts is also why for the moment
> _bind_irq_vector() is left as is, despite looking wrong: It's used
> exclusively for IRQ0, which isn't supposed to move off CPU0 at any time.
> 
> The prior lack of restricting to online CPUs in set_desc_affinity()
> before calling cpu_mask_to_apicid() in particular allowed (in x2APIC
> clustered mode) offlined CPUs to end up enabled in an IRQ's destination
> field. (I wonder whether vector_allocation_cpumask_flat() shouldn't
> follow a similar model, using cpu_present_map in favor of
> cpu_online_map.)
> 
> For IO-APIC code it was definitely wrong to potentially store, as a
> fallback, TARGET_CPUS (i.e. all online ones) into the field, as that
> would have caused problems when determining on which CPUs to release
> vectors when they've gone out of use. Disable interrupts instead when
> no valid target CPU can be established (which code elsewhere should
> guarantee to never happen), and log a message in such an unlikely event.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Thanks.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Some comments below.

> ---
> v2: New.
> 
> --- 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.

>              set_ioapic_affinity_irq(desc, desc->arch.cpu_mask);
>          }
>  
> @@ -2197,7 +2197,6 @@ int io_apic_set_pci_routing (int ioapic,
>  {
>      struct irq_desc *desc = irq_to_desc(irq);
>      struct IO_APIC_route_entry entry;
> -    cpumask_t mask;
>      unsigned long flags;
>      int vector;
>  
> @@ -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?

>  
>      apic_printk(APIC_DEBUG, KERN_DEBUG "IOAPIC[%d]: Set PCI routing entry "
>  		"(%d-%d -> %#x -> IRQ %d Mode:%i Active:%i)\n", ioapic,
> @@ -2422,7 +2427,21 @@ int ioapic_guest_write(unsigned long phy
>      /* Set the vector field to the real vector! */
>      rte.vector = desc->arch.vector;
>  
> -    SET_DEST(rte, logical, cpu_mask_to_apicid(desc->arch.cpu_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(rte, logical, cpu_mask_to_apicid(mask));
> +    }
> +    else
> +    {
> +        gprintk(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;
> +        rte.mask = 1;
> +    }
>  
>      __ioapic_write_entry(apic, pin, 0, rte);
>      
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -471,11 +471,13 @@ static int __assign_irq_vector(
>       */
>      static int current_vector = FIRST_DYNAMIC_VECTOR, current_offset = 0;
>      int cpu, err, old_vector;
> -    cpumask_t tmp_mask;
>      vmask_t *irq_used_vectors = NULL;
>  
>      old_vector = irq_to_vector(irq);
> -    if (old_vector > 0) {
> +    if ( old_vector > 0 )

Another candidate to switch to valid_irq_vector or at least make an
explicit comparison with IRQ_VECTOR_UNASSIGNED.

Seeing your reply to my comment in that direction on a previous patch
this can be done as a follow up.

Roger.

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