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

[Jan Beulich <jbeulich@xxxxxxxx>]

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