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
