Re: [PATCH] x86/APIC: Remove x2APIC pure cluster mode

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

On Tue, Sep 24, 2024 at 07:20:44PM +0100, Andrew Cooper wrote:
> On 24/09/2024 6:10 pm, Roger Pau Monné wrote:
> > On Tue, Sep 24, 2024 at 06:21:47PM +0200, Jan Beulich wrote:
> >> On 24.09.2024 18:14, Roger Pau Monné wrote:
> >>> On Tue, Sep 24, 2024 at 04:27:36PM +0100, Andrew Cooper wrote:
> >>>> On 24/09/2024 4:10 pm, Roger Pau Monné wrote:
> >>>>> On Mon, Sep 23, 2024 at 03:35:59PM +0100, Matthew Barnes wrote:
> >>>>>> With the introduction of mixed x2APIC mode (using cluster addressing for
> >>>>>> IPIs and physical for external interrupts) the use of pure cluster mode
> >>>>>> doesn't have any benefit.
> >>>>>>
> >>>>>> Remove the mode itself, leaving only the code required for logical
> >>>>>> addressing when sending IPIs.
> >>>>>>
> >>>>>> Implements: https://gitlab.com/xen-project/xen/-/issues/189
> >>>> We use the Resolves: tag for this.  Can fix on commit.
> >>>>
> >>>>> There's at least one extra bit which I would also like to see removed,
> >>>>> either in this patch, or as following patch.
> >>>>>
> >>>>> In struct arch_irq_desc we have 3 cpumasks: cpu_mask, old_cpu_mask and
> >>>>> pending_mask.  After dropping cluster mode for external interrupts,
> >>>>> those fields could become integers AFACT, as now interrupts can only
> >>>>> target a single CPU opposed to a logical CPU set.
> >>>> A separate patch for sure, but that sounds like a great improvement.
> >>> Oh, there are quite some fields of struct genapic that are not needed
> >>> anymore, since both implementations set it to the same function.  It
> >>> would be helpful to prune them.
> >> Pruning where possible - yes. But "both" won't cover it, as we have 4
> >> instances of the struct (not just the two x2apic ones).
> > Yeah, realized that afterwards, we still have the xAPIC flat mode,
> > which is using logical delivery mode, but target a single CPU.  So
> > getting rid of the cpumask in arch_irq_desc seem possible, however
> > there might be nothing to prune in struct genapic.
> 
> Logical delivery mode for external interrupts in xAPIC is just as
> broken/useless as the code we've just deleted.
> 
> If that's the only thing in the way of more cleanup, we delete it too.

Bah, xAPIC flat delivery mode needs to be adjusted to use physical
delivery for external interrupts, as the vector space is shared
between all CPUs in that mode.  This must be done ahead of turning the
arch_irq_desc cpumasks into integers.

Sorry, this is turning into a more work that I originally expected,
mostly because I wasn't taking into account that xAPIC was still using
logical mode for external interrupts.

Thanks, Roger.