Re: [PATCH v4] xen/x86: On x2APIC mode, derive LDR from APIC ID

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 27/11/2023 12:24, Jan Beulich wrote:
> On 27.11.2023 13:17, Alejandro Vallejo wrote:
>> On 27/11/2023 08:40, Jan Beulich wrote:
>>> On 23.11.2023 18:30, Alejandro Vallejo wrote:
>>>> @@ -1498,27 +1511,36 @@ static int cf_check lapic_save_regs(struct vcpu 
>>>> *v, hvm_domain_context_t *h)
>>>>    */
>>>>   static void lapic_load_fixup(struct vlapic *vlapic)
>>>>   {
>>>> -    uint32_t id = vlapic->loaded.id;
>>>> +    uint32_t good_ldr = x2apic_ldr_from_id(vlapic->loaded.id);
>>>>   
>>>> -    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
>>>> -    {
>>>> +    /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct 
>>>> */
>>>> +    if ( !vlapic_x2apic_mode(vlapic) ||
>>>> +         (vlapic->loaded.ldr == good_ldr) )
>>>> +        return;
>>>> +
>>>> +    if ( vlapic->loaded.ldr == 1 )
>>>> +       /*
>>>> +        * Xen <= 4.4 may have a bug by which all the APICs configured in
>>>> +        * x2APIC mode got LDR = 1. We can't leave it as-is because it
>>>> +        * assigned the same LDR to every CPU.  We'll fix fix the bug now
>>>> +        * and assign an LDR value consistent with the APIC ID.
>>>> +        */
>>>
>>> Just one comment on top of Andrew's: Is the double "fix" really intended
>>> here? (I could see it might be, but then "fix the bug fix" would read
>>> slightly more smoothly to me as a non-native speaker.)
>>
>> It's not intended indeed. s/fix fix/fix/
>>
>>>
>>> Another aspect here is what exactly the comment states (and does not
>>> state). Original comments made explicit that LDR == 1 contradicts ID == 0.
>>> In the new comment you only emphasize that all CPUs cannot have that same
>>> LDR. But the value of 1 being bogus in the first place doesn't become clear
>>> anymore.
>>
>> 1 is bogus for id!=0, but so would be 3, 7 or 42.
> 
> Yet 3, 7, and 42 aren't interesting in the context of that older bug.
> 
>> In particular we have
>> ID==2 contradicting LDR=2, and we're allowing it. The reason why we must
>> fix this other case is because all LDRs are equal, otherwise it would
>> get the same treatment as the other bug.
> 
> I understand all that; still there's loss of information in the comments,
> from my perspective.
> 
> Jan

v2 did have a "Note that "x2apic_id == 0" has always been correct and
can't be used to discriminate these cases." and another in front of the
early exit "No need to perform fixups in non-x2apic mode, and x2apic_id
== 0 has always been correct.". They were trimmed as versions went on.

As mentioned before this is all cosmetic, so I'm happy either way. I'll
reinstate something to this effect in a v5.

Cheers,
Alejandro