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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Mon, Nov 13, 2023 at 06:53:00PM +0100, 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.
Sure
> 
> Seeing the commit that introduced the bogus LDR value, I'm not sure it
> was intentional, 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.
I can rephrase the commit message to state the clustering difference in a
way that doesn't speculate about the previous code intent. It way many
years ago and it doesn't matter terribly.
> 
> > 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')
Sure

> 
> > 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.
> 
> Thanks, Roger.
I could do that, but the fix would not be trivial. It would have to wait
for another series I'm working on that extends the CPU policy, because we'd
have to stash the initial x2APIC LDR of each vCPU on the migrate stream.
The question becomes whether there's value in preserving those LDRs, and of
that I'm not very sure.

In particular, I'm not sure how the guests might behave here. Keeping the
broken LDR could both maake things better or worse. Or better for some and
worse for others.

>From a purely pragmatic point of view, in the absence of a clear advantage
I'd rather take the path of least resistence and let nature take its
course. Otherwise, I'll just stash this patch in my topology series as it
would need to be added after the migration logic is in place.

Cheers,
Alejandro