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

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

On Fri, Nov 03, 2023 at 01:51:13PM +0100, Jan Beulich wrote:
> On 03.11.2023 13:48, Roger Pau Monné wrote:
> > 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,
> 
> Not quite correct, I think: This is setting up the receive side of the IPIs
> (iirc LDR needs to be set for logical delivery mode to be usable). Beyond
> that lgtm, fwiw.

No, LDR is read-only in x2APIC mode (it's rw in xAPIC mode).
init_apic_ldr_x2apic_cluster() just reads LDR, but doesn't set it.

Thanks, Roger.