| Message ID | 20231113165023.5824-1-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/x86: On x2APIC mode, derive LDR from APIC_ID | expand |
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, 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. > 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') > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> 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.
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@cloud.com> > > 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
On 14/11/2023 10:14 am, 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: >>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> >> 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. OS software doesn't reset the APIC at runtime. Most OS software doesn't reset the APIC ever because it was impossible to recover from on the first two generations of APIC. (It required a full platform reset and model-specific logic.) On capable APIC versions you still lose interrupts by resetting, which is why a reset will only ever occur prior to setting up IRQs on boot. Linux does now reset the APIC (where possible) on boot in order to clear out any prior state. This also translates to resetting on kexec/etc. A guest which does reset the APIC Is never going to care what the old value was. Only a test case is liable to notice, and we're clearly not running one of those or we'd have found this bug already. So no, I really don't think we need to keep a breakage around in the APIC emulation forevermore to work around a theoretical-only problem. We should however discuss this in the commit message, just in case someone in another 9y is trying to figure out a bug... ~Andrew
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@cloud.com> > > 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
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@cloud.com> > > > > 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
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@cloud.com> >>> >>> 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. Jan
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@cloud.com> >>>> 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
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@cloud.com> > >>>> 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.
Hi, On Tue, Nov 14, 2023 at 03:11:28PM +0100, Roger Pau Monné wrote: > 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@cloud.com> > > >>>> 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). Ack. I'll just integrate this into my ongoing series, with... > > 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. ... this in mind. > > > 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. Cheers, Alejandro
On 14/11/2023 2:11 pm, Roger Pau Monné wrote: > 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: >>>> 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 Yeah that's never actually been tested with 256 vCPUs. A VM *must* have either a vIOMMU, or know (via whatever means) that there are no IO-APICs, or (via whatever means) that all IO-APICs can use reserved bits for 32k destination APIC ID support. As it stands, this is just something which will explode on us in the future. Hopefully the worst that will happen is a panic on boot. > 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). Of course. That would be data corruption otherwise. > 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. Yes. We can just set it properly on reset and the problem ought to disappear. ~Andrew
On Tue, Nov 14, 2023 at 02:44:09PM +0000, Andrew Cooper wrote: > On 14/11/2023 2:11 pm, Roger Pau Monné wrote: > > 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: > >>>> 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 > > Yeah that's never actually been tested with 256 vCPUs. > > A VM *must* have either a vIOMMU, or know (via whatever means) that > there are no IO-APICs, or (via whatever means) that all IO-APICs can use > reserved bits for 32k destination APIC ID support. > > As it stands, this is just something which will explode on us in the > future. Hopefully the worst that will happen is a panic on boot. Linux already accounts for this, vCPUs with APIC IDs > 255 won't be used if there's no IOMMU or extended destination ID support: https://elixir.bootlin.com/linux/latest/source/arch/x86/kernel/apic/apic.c#L1844 We should see how that goes once we allow APIC IDs > 255 for guests. That makes me remember I should add an item to get extended destination ID functional for Xen. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index c7ce82d064..5a74e84445 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -1063,10 +1063,10 @@ static const struct hvm_mmio_ops vlapic_mmio_ops = { static void set_x2apic_id(struct vlapic *vlapic) { - u32 id = vlapic_vcpu(vlapic)->vcpu_id; - u32 ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf)); + uint32_t id = vlapic_vcpu(vlapic)->vcpu_id * 2; + uint32_t ldr = ((id & ~0xf) << 12) | (1 << (id & 0xf)); - vlapic_set_reg(vlapic, APIC_ID, id * 2); + vlapic_set_reg(vlapic, APIC_ID, id); vlapic_set_reg(vlapic, APIC_LDR, ldr); }
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) 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 Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- xen/arch/x86/hvm/vlapic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)