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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

> 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.