Re: [PATCH 10/12] x86/irq: convert irq_desc cpu_mask field to integer

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2025 10:58, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -310,9 +310,9 @@ static void cf_check hpet_msi_set_affinity(
>      struct msi_msg msg = ch->msi.msg;
>  
>      /* This really is only for dump_irqs(). */
> -    cpumask_copy(desc->arch.cpu_mask, mask);
> +    desc->arch.cpu = cpumask_any(mask);

Going from the comment, couldn't you use CPU_INVALID here? Then again, see
"x86/HPET: drop .set_affinity hook", where the function goes away anyway.

> @@ -337,7 +337,8 @@ static int __hpet_setup_msi_irq(struct irq_desc *desc)
>  {
>      struct msi_msg msg;
>  
> -    msi_compose_msg(desc->arch.vector, desc->arch.cpu_mask, &msg);
> +    msg.dest32 = cpu_physical_id(desc->arch.cpu);
> +    msi_compose_msg(desc->arch.vector, &msg);
>      return hpet_msi_write(desc->action->dev_id, &msg);
>  }

Setting msg.dest32 ahead of calling msi_compose_msg() feels odd. It makes things
look as if this was an input to the function, when by its name it rather would
want to be an output. Furthermore this is dead code right now, as the function
clears the entire structure first thing. Imo it being the function to fill the
field should be retained; instead of the CPU mask you'd once again make it a
scalar parameter. For the case where NULL was passed before, ...

> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -69,13 +69,9 @@ struct irq_desc;
>  struct arch_irq_desc {
>          int16_t vector;                  /* vector itself is only 8 bits, */
>          int16_t old_vector;              /* but we use -1 for unassigned  */
> -        /*
> -         * Except for high priority interrupts @cpu_mask may have bits set 
> for
> -         * offline CPUs.  Consumers need to be careful to mask this down to
> -         * online ones as necessary.  There is supposed to always be a non-
> -         * empty intersection with cpu_online_map.
> -         */
> -        cpumask_var_t cpu_mask;
> +/* Special target CPU values. */
> +#define CPU_INVALID  ~0U

... you already make a suitable constant available. (Nit: The expansion wants
parenthesizing.)

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -1112,8 +1112,7 @@ static void __init setup_IO_APIC_irqs(void)
>              if (platform_legacy_irq(irq))
>                  disable_8259A_irq(irq_to_desc(irq));
>  
> -            set_entry_dest(&entry,
> -                           
> cpu_mask_to_apicid(irq_to_desc(irq)->arch.cpu_mask));
> +            set_entry_dest(&entry, 
> cpu_physical_id(irq_to_desc(irq)->arch.cpu));

I may as well mention this here: Looks like this patch removes all call sites
of cpu_mask_to_apicid(). That would leave the function unreachable, i.e. 
violating
a Misra rule, so I think the function needs dropping right here.

> @@ -2137,14 +2136,11 @@ int io_apic_set_pci_routing (int ioapic, int pin, int 
> irq, int edge_level, int a
>          return vector;
>      entry.vector = vector;
>  
> -    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_entry_dest(&entry, cpu_mask_to_apicid(mask));
> +    if (cpu_online(desc->arch.cpu)) {

Can CPU_INVALID make it here? If so, it needs guarding against. If not, an
assertion may be nice. (Same possibly elsewhere.)

> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -156,8 +156,7 @@ static int __init _bind_irq_vector(struct irq_desc *desc, 
> int vector,
>  
>      if ( !cpu_online(cpu) )
>          return -EINVAL;
> -    if ( (desc->arch.vector == vector) &&
> -         cpumask_test_cpu(cpu, desc->arch.cpu_mask) )
> +    if ( (desc->arch.vector == vector) && cpu == desc->arch.cpu )

Please can you be consistent with parentheses on both sides of the &&?
(I'd prefer the excess ones to be dropped, but the alternative is also
okay.)

> @@ -684,8 +673,9 @@ next:
>          }
>          else if ( valid_irq_vector(old_vector) )
>          {
> -            cpumask_and(desc->arch.old_cpu_mask, desc->arch.cpu_mask,
> -                        &cpu_online_map);
> +            cpumask_clear(desc->arch.old_cpu_mask);
> +            if ( cpu_online(desc->arch.cpu) )
> +                cpumask_set_cpu(desc->arch.cpu, desc->arch.old_cpu_mask);

As mentioned for an earlier patch, to avoid the LOCK-ed update
cpumask_copy() may be better to use. Yet iirc like there, likely this
goes away again later in the series (by the title right in the next patch),
so perhaps not a big deal.

> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -16,6 +16,7 @@ unsigned int __read_mostly nr_cpumask_bits
>  const cpumask_t cpumask_all = {
>      .bits[0 ... (BITS_TO_LONGS(NR_CPUS) - 1)] = ~0UL
>  };
> +const cpumask_t cpumask_none;

This feels wasteful at least for larger NR_CPUS. And it's likely going to
violate some Misra rule on non-x86, for having no user. On x86, as long as
NR_CPUS <= 8 * PAGE_SIZE, you could (re-)use zero_page[] instead.

> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -507,7 +507,7 @@ static void cf_check set_x2apic_affinity(
>      if ( dest == BAD_APICID )
>          return;
>  
> -    msi_compose_msg(desc->arch.vector, NULL, &iommu->msi.msg);
> +    msi_compose_msg(desc->arch.vector, &iommu->msi.msg);
>      iommu->msi.msg.dest32 = dest;

With the outlined adjustment above, it looks like the explicit setting
of .dest32 would then not be needed here (and perhaps elsewhere) anymore.

Jan