Re: [PATCH 11/12] x86/irq: convert irq_desc old_cpu_mask field to integer

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2025 10:58, Roger Pau Monne wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -115,7 +115,7 @@ static void release_old_vec(struct irq_desc *desc)
>      unsigned int vector = desc->arch.old_vector;
>  
>      desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
> -    cpumask_clear(desc->arch.old_cpu_mask);
> +    desc->arch.old_cpu = CPU_INVALID;

With this, ...

> @@ -221,10 +220,10 @@ static void _clear_irq_vector(struct irq_desc *desc)
>      {
>          /* If we were in motion, also clear desc->arch.old_vector */
>          old_vector = desc->arch.old_vector;
> -        cpumask_and(tmp_mask, desc->arch.old_cpu_mask, &cpu_online_map);
>  
> -        for_each_cpu(cpu, tmp_mask)
> +        if ( cpu_online(desc->arch.old_cpu) )

... you pretty certainly want to guard against the value making it here (even
if just accidentally), and thus triggering the assertion in cpumask_check().
(Again possibly elsewhere as well.)

> @@ -581,16 +575,16 @@ static int _assign_irq_vector(struct irq_desc *desc, 
> const cpumask_t *mask)
>               * in the 'mask' parameter.
>               */
>              desc->arch.vector = desc->arch.old_vector;
> -            desc->arch.cpu = cpumask_any(desc->arch.old_cpu_mask);
> +            desc->arch.cpu = desc->arch.old_cpu;
>  
>              /* Undo any possibly done cleanup. */
>              per_cpu(vector_irq, desc->arch.cpu)[desc->arch.vector] = irq;
>  
>              /* Cancel the pending move and release the current vector. */
>              desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
> -            cpumask_clear(desc->arch.old_cpu_mask);
> +            desc->arch.old_cpu = CPU_INVALID;
>              desc->arch.move_in_progress = 0;
> -            desc->arch.move_cleanup_count = 0;
> +            desc->arch.move_cleanup =  false;

Nit: Excess blank.

> @@ -2003,7 +1994,7 @@ void do_IRQ(struct cpu_user_regs *regs)
>                             ~irq, CPUMASK_PR(desc->affinity),
>                             /* TODO: handle hipri vectors nicely. */
>                             CPUMASK_PR(get_cpumask(desc->arch.cpu)),
> -                           CPUMASK_PR(desc->arch.old_cpu_mask),
> +                           CPUMASK_PR(get_cpumask(desc->arch.old_cpu)),

I should have asked on the previous patch already: Does it actually make sense
to still print these in mask form? Without that you wouldn't need get_cpumask(),
and as a result you also wouldn't need cpumask_none.

> @@ -2685,12 +2664,9 @@ void fixup_irqs(void)
>               * per-cpu vector table will no longer have ->arch.old_vector
>               * setup, and hence ->arch.old_cpu_mask would be stale.
>               */
> -            cpumask_clear_cpu(cpu, desc->arch.old_cpu_mask);
> -            if ( cpumask_empty(desc->arch.old_cpu_mask) )
> -            {
> -                desc->arch.move_in_progress = 0;
> -                release_old_vec(desc);
> -            }
> +            desc->arch.old_cpu = CPU_INVALID;
> +            desc->arch.move_in_progress = 0;

As you touch the line anyway, switch to using "false"?

Jan