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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Nov 14, 2023 at 12:55:46PM +0000, Andrew Cooper wrote:
> 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.

We do, in fact the pvshim (or nested Xen) will run in x2APIC mode if
available.

Linux >= 5.17 will also use x2APIC mode if available when running as a
Xen HVM guest:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c8980fcb210851138cb34c9a8cb0cf0c09f07bf9

If a guest has been booted with the bogus LDR we need to keep it on
migrate, otherwise at least Xen will break (because it does read the
LDR from the hardware instead of building it based on the APIC ID).

Switching to the correct LDR on APIC reset can be sensible, any APIC
device reset should be done together with updating whatever registers
have been previously cached, and OSes don't do APIC resets anyway.

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

Yeah, I've heard similar things for lowpri mode.  It's also valid to
implement as a round-robin.

Thanks, Roger.