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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

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.
> 
> Jan
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.

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.

Cheers,
Alejandro