Re: [PATCH] xen/x86: On x2APIC mode, derive LDR from APIC_ID

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/11/2023 12:32 pm, Jan Beulich wrote:
> On 14.11.2023 13:18, Alejandro Vallejo wrote:
>> On Tue, Nov 14, 2023 at 11:14:22AM +0100, Jan Beulich wrote:
>>> On 13.11.2023 18:53, Roger Pau Monné wrote:
>>>> On Mon, Nov 13, 2023 at 04:50:23PM +0000, Alejandro Vallejo wrote:
>>>>> Both Intel and AMD manuals agree that on x2APIC mode, the APIC LDR and ID
>>>>> registers are derivable from each other through a fixed formula.
>>>>>
>>>>> Xen uses that formula, but applies it to vCPU IDs (which are sequential)
>>>>> rather than x2APIC_IDs (which are not, at the moment). As I understand it,
>>>>> this is an attempt to tightly pack vCPUs into clusters so each cluster has
>>>>> 16 vCPUs rather than 8, but this is problematic for OSs that might read 
>>>>> the
>>>>> x2APIC_ID and internally derive LDR (or the other way around)
>>>> I would replace the underscore from x2APIC ID with a space instead.
>>>>
>>>> Seeing the commit that introduced the bogus LDR value, I'm not sure it
>>>> was intentional,
>>> Hard to reconstruct over 9 years later. It feels like Alejandro may be right
>>> with his derivation.
>>>
>>>> as previous Xen code had:
>>>>
>>>> u32 id = vlapic_get_reg(vlapic, APIC_ID);
>>>> u32 ldr = ((id & ~0xf) << 16) | (1 << (id & 0xf));
>>>> vlapic_set_reg(vlapic, APIC_LDR, ldr);
>>>>
>>>> Which was correct, as the LDR was derived from the APIC ID and not the
>>>> vCPU ID.
>>> Well, it gave the appearance of deriving from the APIC ID. Just that it was
>>> missing GET_xAPIC_ID() around the vlapic_get_reg() (hence why LDR was
>>> uniformly 1 on all CPUs).
>>>
>>>>> This patch fixes the implementation so we follow the rules in the x2APIC
>>>>> spec(s).
>>>>>
>>>>> While in the neighborhood, replace the u32 type with the standard uint32_t
>>>> Likely wants:
>>>>
>>>> Fixes: f9e0cccf7b35 ('x86/HVM: fix ID handling of x2APIC emulation')
>>> +1
>>>
>>>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
>>>> I do wonder whether we need to take any precautions with guests being
>>>> able to trigger an APIC reset, and thus seeing a changed LDR register
>>>> if the guest happens to be migrated from an older hypervisor version
>>>> that doesn't have this fix.  IOW: I wonder whether Xen should keep the
>>>> previous bogus LDR value across APIC resets for guests that have
>>>> already seen it.
>>> That earlier change deliberately fixed up any bogus values. I wonder
>>> whether what you suggest will do more good or more harm than going
>>> even farther and once again fixing up bad values in lapic_load_fixup().
>>> After all LDR being wrong affects vlapic_match_logical_addr()'s outcome.
>>> I think one of the two wants adding to the change, though.
>>>
>> You mean changing the LDR of a vCPU to the correct value on migrate? That
>> feels like playing with fire. A migrated VM is presumably a VM that is
>> running without issues (or it would have been rebooted). Letting it run
>> as it did seems safer.
> See Andrew's reply.
>
>> I don't think vlapic_match_logical_addr() is affected. The LDR's are still
>> unique in the bogus case so the matching ought to work. Problem would arise
>> if the guest makes assumptions about APIC_ID and LDR relationships.
> The LDRs still being unique (or not) isn't what I'm concerned about. It is
> the function's return value which would be wrong, as the incoming "mda"
> presumably was set in its respective field on the assumption that the LDRs
> are set in a spec-compliant way. There not having been problem reports
> makes me wonder whether any guests actually use logical delivery mode in a
> wider fashion.

They likely don't.

Logical delivery for xAPIC only works in a tiny fraction of cases
(assuming correct topology information, which we don't give), and
persuading a VM to turn on x2APIC without a vIOMMU is not something
we've managed to do in Xen.

Also (as I learn talking to people just last night) it turns out that
Logical Destination Mode for external interrupts is entirely broken
anyway.  It always hits the lowest ID in the cluster, unless the LAPIC
in question is already servicing a same-or-higher priority interrupt at
which point the next ID in the cluster is tried.

~Andrew