Re: [PATCH v2] x86/x2apic: introduce a mixed physical/cluster mode

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 02, 2023 at 02:38:09PM +0100, Jan Beulich wrote:
> On 31.10.2023 15:52, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/genapic/x2apic.c
> > +++ b/xen/arch/x86/genapic/x2apic.c
> > @@ -180,6 +180,29 @@ static const struct genapic __initconstrel 
> > apic_x2apic_cluster = {
> >      .send_IPI_self = send_IPI_self_x2apic
> >  };
> >  
> > +/*
> > + * Mixed x2APIC mode: use physical for external (device) interrupts, and
> > + * cluster for inter processor interrupts.  Such mode has the benefits of 
> > not
> > + * sharing the vector space with all CPUs on the cluster, while still 
> > allowing
> > + * IPIs to be more efficiently delivered by not having to perform an ICR 
> > write
> > + * for each target CPU.
> > + */
> > +static const struct genapic __initconstrel apic_x2apic_mixed = {
> > +    APIC_INIT("x2apic_mixed", NULL),
> > +    /*
> > +     * NB: IPIs use the send_IPI_{mask,self} hooks only, other fields are
> > +     * exclusively used by external interrupts and hence are set to use
> > +     * Physical destination mode handlers.
> > +     */
> > +    .int_delivery_mode = dest_Fixed,
> > +    .int_dest_mode = 0 /* physical delivery */,
> > +    .init_apic_ldr = init_apic_ldr_x2apic_cluster,
> > +    .vector_allocation_cpumask = vector_allocation_cpumask_phys,
> > +    .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
> > +    .send_IPI_mask = send_IPI_mask_x2apic_cluster,
> > +    .send_IPI_self = send_IPI_self_x2apic
> > +};
> 
> I'm afraid the comment is still misleading in one respect: The .init_apic_ldr
> hook is also set to its Clustered mode handler (and validly so). As before my
> suggestion would be to leverage that we're using dedicated initializers here
> and have a Physical mode portion and a Clustered mode one, each clarifying in
> a brief leading comment where/how the handlers are used.

I've split this as:

/*
 * Mixed x2APIC mode: use physical for external (device) interrupts, and
 * cluster for inter processor interrupts.  Such mode has the benefits of not
 * sharing the vector space with all CPUs on the cluster, while still allowing
 * IPIs to be more efficiently delivered by not having to perform an ICR write
 * for each target CPU.
 */
static const struct genapic __initconstrel apic_x2apic_mixed = {
    APIC_INIT("x2apic_mixed", NULL),
    /*
     * The following fields are exclusively used by external interrupts and
     * hence are set to use Physical destination mode handlers.
     */
    .int_delivery_mode = dest_Fixed,
    .int_dest_mode = 0 /* physical delivery */,
    .vector_allocation_cpumask = vector_allocation_cpumask_phys,
    .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,
    /*
     * The following fields are exclusively used by IPIs and hence are set to
     * use Cluster Logical destination mode handlers.  Note that init_apic_ldr
     * is not used by IPIs, but the per-CPU fields it initializes are only used
     * by the IPI hooks.
     */
    .init_apic_ldr = init_apic_ldr_x2apic_cluster,
    .send_IPI_mask = send_IPI_mask_x2apic_cluster,
    .send_IPI_self = send_IPI_self_x2apic
};

Pending whether the usage of some of the fields in connect_bsp_APIC()
can be removed.

Thanks, Roger.