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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.11.2023 14:41, Roger Pau Monné wrote:
> 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.

Oh, right, silly me. Perhaps the function could have a better name
(reflecting its purpose, and making more clear that the hook is merely
leveraged for the purpose).

Jan