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(-)

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);
 }
 
-- 
2.34.1

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

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.

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