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) and covers migrations from broken hypervisors, so LDRs are
preserved even for hotppluggable CPUs and across APIC resets.

While touching that area, I removed the existing printk statement in
vlapic_load_fixup() (as the checks it performed didn't make sense in x2APIC
mode and wouldn't affect the outcome) and put another printk as an else
branch so we get warnings trying to load nonsensical LDR values we don't
know about.

Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v3:
  * Removed underscores from (x2)APIC_ID in commit message
  * Added commit msg explaining the removal of the vlapic_load_fixup() printk
  * Restored lowercase to hex "F"s
  * Refactored a bit vlapic_load_fixup() so it has a trailing printk in
    case of spotting a nonsensical LDR it doesn't understand.
  * Moved field in domain.h with the other booleans
  * Trimmed down field name in domain.h
  * Trimmed down field comment in domain.h

If the field name in domain.h still seems too long I'm happy for it to be
trimmed further down, but I do want to preserve the "_bug_" part of it.
---
 xen/arch/x86/hvm/vlapic.c             | 62 +++++++++++++++++----------
 xen/arch/x86/include/asm/hvm/domain.h |  3 ++
 2 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 5cb87f8649..cd929c3716 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1061,13 +1061,23 @@ static const struct hvm_mmio_ops vlapic_mmio_ops = {
     .write = vlapic_mmio_write,
 };
 
+static uint32_t x2apic_ldr_from_id(uint32_t id)
+{
+    return ((id & ~0xf) << 12) | (1 << (id & 0xf));
+}
+
 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 vcpu_id = vlapic_vcpu(vlapic)->vcpu_id;
+    uint32_t apic_id = vcpu_id * 2;
+    uint32_t apic_ldr = x2apic_ldr_from_id(apic_id);
+
+    /* This is a migration bug workaround. See wall of text in lapic_load_fixup() */
+    if ( vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id )
+        apic_ldr = x2apic_ldr_from_id(vcpu_id);
 
-    vlapic_set_reg(vlapic, APIC_ID, id * 2);
-    vlapic_set_reg(vlapic, APIC_LDR, ldr);
+    vlapic_set_reg(vlapic, APIC_ID, apic_id);
+    vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
 }
 
 int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
@@ -1498,27 +1508,35 @@ static int cf_check lapic_save_regs(struct vcpu *v, hvm_domain_context_t *h)
  */
 static void lapic_load_fixup(struct vlapic *vlapic)
 {
-    uint32_t id = vlapic->loaded.id;
+    /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */
+    if ( !vlapic_x2apic_mode(vlapic) ||
+         (vlapic->loaded.ldr == x2apic_ldr_from_id(vlapic->loaded.id)) )
+        return;
 
-    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
-    {
+    if ( vlapic->loaded.ldr == 1 )
+       /*
+        * Xen <= 4.4 had a bug by which all the APICs configured in x2APIC
+        * mode got LDR = 1. We can't leave it as-is because it assigned the
+        * same LDR to every CPU.  We'll fix fix the bug now and assign an
+        * LDR value consistent with the APIC ID.
+        */
+        set_x2apic_id(vlapic);
+    else if ( vlapic->loaded.ldr ==
+              x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id) )
         /*
-         * This is optional: ID != 0 contradicts LDR == 1. It's being added
-         * to aid in eventual debugging of issues arising from the fixup done
-         * here, but can be dropped as soon as it is found to conflict with
-         * other (future) changes.
+         * This is a migration from a broken Xen between 4.4 and 4.18 and
+         * we must _PRESERVE_ LDRs so new vCPUs use consistent derivations.
+         * This is so existing running guests that may have already read
+         * the LDR at the source host aren't surprised when IPIs stop
+         * working as they did at the other end. To address this, we set
+         * this domain boolean so future CPU hotplugs derive an LDR
+         * consistent with the older Xen's broken idea of consistency.
          */
-        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
-             id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
-            printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
-                   vlapic_vcpu(vlapic), id);
-        set_x2apic_id(vlapic);
-    }
-    else /* Undo an eventual earlier fixup. */
-    {
-        vlapic_set_reg(vlapic, APIC_ID, id);
-        vlapic_set_reg(vlapic, APIC_LDR, vlapic->loaded.ldr);
-    }
+        vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id = true;
+    else
+        printk(XENLOG_G_WARNING
+               "%pv: bogus x2APIC loaded id=%#x ldr=%#x\n",
+               vlapic_vcpu(vlapic), vlapic->loaded.id, vlapic->loaded.ldr);
 }
 
 static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index 6e53ce4449..2fee3874cd 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -106,6 +106,9 @@ struct hvm_domain {
 
     bool                   is_s3_suspended;
 
+    /* Compat setting for a bug in x2APIC LDR. See vlapic_load_fixup() */
+    bool x2apic_ldr_bug_with_vcpu_id;
+
     /* hypervisor intercepted msix table */
     struct list_head       msixtbl_list;
 
-- 
2.34.1

On Wed, Nov 22, 2023 at 04:08:17PM +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)
> 
> This patch fixes the implementation so we follow the rules in the x2APIC
> spec(s) and covers migrations from broken hypervisors, so LDRs are
> preserved even for hotppluggable CPUs and across APIC resets.
> 
> While touching that area, I removed the existing printk statement in
> vlapic_load_fixup() (as the checks it performed didn't make sense in x2APIC
> mode and wouldn't affect the outcome) and put another printk as an else
> branch so we get warnings trying to load nonsensical LDR values we don't
> know about.
> 
> Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Mostly some style nits, and one comment adjustment.

If you are OK with those:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> v3:
>   * Removed underscores from (x2)APIC_ID in commit message
>   * Added commit msg explaining the removal of the vlapic_load_fixup() printk
>   * Restored lowercase to hex "F"s
>   * Refactored a bit vlapic_load_fixup() so it has a trailing printk in
>     case of spotting a nonsensical LDR it doesn't understand.
>   * Moved field in domain.h with the other booleans
>   * Trimmed down field name in domain.h
>   * Trimmed down field comment in domain.h
> 
> If the field name in domain.h still seems too long I'm happy for it to be
> trimmed further down, but I do want to preserve the "_bug_" part of it.

I think the _with_ part is redundant, but it's certainly shorter than
previously :).

> ---
>  xen/arch/x86/hvm/vlapic.c             | 62 +++++++++++++++++----------
>  xen/arch/x86/include/asm/hvm/domain.h |  3 ++
>  2 files changed, 43 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 5cb87f8649..cd929c3716 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1061,13 +1061,23 @@ static const struct hvm_mmio_ops vlapic_mmio_ops = {
>      .write = vlapic_mmio_write,
>  };
>  
> +static uint32_t x2apic_ldr_from_id(uint32_t id)
> +{
> +    return ((id & ~0xf) << 12) | (1 << (id & 0xf));
> +}
> +
>  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 vcpu_id = vlapic_vcpu(vlapic)->vcpu_id;
> +    uint32_t apic_id = vcpu_id * 2;
> +    uint32_t apic_ldr = x2apic_ldr_from_id(apic_id);
> +
> +    /* This is a migration bug workaround. See wall of text in lapic_load_fixup() */

Line length > 80 cols.

I try to avoid referencing function names in comments, as they tend to
get out of sync without noticing.  It's much easier to use cscope to
grep for x2apic_ldr_bug_with_vcpu_id and find the comment itself.

> +    if ( vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id )
> +        apic_ldr = x2apic_ldr_from_id(vcpu_id);
>  
> -    vlapic_set_reg(vlapic, APIC_ID, id * 2);
> -    vlapic_set_reg(vlapic, APIC_LDR, ldr);
> +    vlapic_set_reg(vlapic, APIC_ID, apic_id);
> +    vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
>  }
>  
>  int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
> @@ -1498,27 +1508,35 @@ static int cf_check lapic_save_regs(struct vcpu *v, hvm_domain_context_t *h)
>   */
>  static void lapic_load_fixup(struct vlapic *vlapic)
>  {
> -    uint32_t id = vlapic->loaded.id;
> +    /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */
> +    if ( !vlapic_x2apic_mode(vlapic) ||
> +         (vlapic->loaded.ldr == x2apic_ldr_from_id(vlapic->loaded.id)) )
> +        return;
>  
> -    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
> -    {
> +    if ( vlapic->loaded.ldr == 1 )
> +       /*
> +        * Xen <= 4.4 had a bug by which all the APICs configured in x2APIC
> +        * mode got LDR = 1. We can't leave it as-is because it assigned the
> +        * same LDR to every CPU.  We'll fix fix the bug now and assign an
> +        * LDR value consistent with the APIC ID.
> +        */
> +        set_x2apic_id(vlapic);
> +    else if ( vlapic->loaded.ldr ==
> +              x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id) )
>          /*
> -         * This is optional: ID != 0 contradicts LDR == 1. It's being added
> -         * to aid in eventual debugging of issues arising from the fixup done
> -         * here, but can be dropped as soon as it is found to conflict with
> -         * other (future) changes.
> +         * This is a migration from a broken Xen between 4.4 and 4.18 and
> +         * we must _PRESERVE_ LDRs so new vCPUs use consistent derivations.

Not sure if we should try to avoid mentioning specific versions in the
comments, as I this fix will be backported to stable branches (I hope),
and hence those will no longer be affected.

> +         * This is so existing running guests that may have already read
> +         * the LDR at the source host aren't surprised when IPIs stop
> +         * working as they did at the other end. To address this, we set
> +         * this domain boolean so future CPU hotplugs derive an LDR
> +         * consistent with the older Xen's broken idea of consistency.

I think this is possibly too verbose, I would be fine with just the
first sentence TBH.  If we want the full comment however, the wording
should be slightly addressed: it's not just IPIs that would possibly
fail to be delivered, but any interrupt attempting to target the APIC
using the previous LDR addressing (either an IPI or an external
interrupt).

>           */
> -        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
> -             id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
> -            printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
> -                   vlapic_vcpu(vlapic), id);
> -        set_x2apic_id(vlapic);
> -    }
> -    else /* Undo an eventual earlier fixup. */
> -    {
> -        vlapic_set_reg(vlapic, APIC_ID, id);
> -        vlapic_set_reg(vlapic, APIC_LDR, vlapic->loaded.ldr);
> -    }
> +        vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id = true;
> +    else
> +        printk(XENLOG_G_WARNING
> +               "%pv: bogus x2APIC loaded id=%#x ldr=%#x\n",
> +               vlapic_vcpu(vlapic), vlapic->loaded.id, vlapic->loaded.ldr);

Could you write the expected values while at it:

"%pv: bogus x2APIC loaded id=%#x ldr=%#x (expected id=%#x ldr=%#x)\n"

>  }
>  
>  static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
> diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
> index 6e53ce4449..2fee3874cd 100644
> --- a/xen/arch/x86/include/asm/hvm/domain.h
> +++ b/xen/arch/x86/include/asm/hvm/domain.h
> @@ -106,6 +106,9 @@ struct hvm_domain {
>  
>      bool                   is_s3_suspended;
>  
> +    /* Compat setting for a bug in x2APIC LDR. See vlapic_load_fixup() */
> +    bool x2apic_ldr_bug_with_vcpu_id;

I think you already mentioned in a followup reply that using
bug_x2apic_ldr_vcpu_id would be fine.

Thanks, Roger.

On Thu, Nov 23, 2023 at 10:03:12AM +0100, Roger Pau Monné wrote:
> On Wed, Nov 22, 2023 at 04:08:17PM +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)
> > 
> > This patch fixes the implementation so we follow the rules in the x2APIC
> > spec(s) and covers migrations from broken hypervisors, so LDRs are
> > preserved even for hotppluggable CPUs and across APIC resets.
> > 
> > While touching that area, I removed the existing printk statement in
> > vlapic_load_fixup() (as the checks it performed didn't make sense in x2APIC
> > mode and wouldn't affect the outcome) and put another printk as an else
> > branch so we get warnings trying to load nonsensical LDR values we don't
> > know about.
> > 
> > Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
> > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> 
> Mostly some style nits, and one comment adjustment.
> 
> If you are OK with those:
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> > ---
> > v3:
> >   * Removed underscores from (x2)APIC_ID in commit message
> >   * Added commit msg explaining the removal of the vlapic_load_fixup() printk
> >   * Restored lowercase to hex "F"s
> >   * Refactored a bit vlapic_load_fixup() so it has a trailing printk in
> >     case of spotting a nonsensical LDR it doesn't understand.
> >   * Moved field in domain.h with the other booleans
> >   * Trimmed down field name in domain.h
> >   * Trimmed down field comment in domain.h
> > 
> > If the field name in domain.h still seems too long I'm happy for it to be
> > trimmed further down, but I do want to preserve the "_bug_" part of it.
> 
> I think the _with_ part is redundant, but it's certainly shorter than
> previously :).
Sure
> 
> > ---
> >  xen/arch/x86/hvm/vlapic.c             | 62 +++++++++++++++++----------
> >  xen/arch/x86/include/asm/hvm/domain.h |  3 ++
> >  2 files changed, 43 insertions(+), 22 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > index 5cb87f8649..cd929c3716 100644
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -1061,13 +1061,23 @@ static const struct hvm_mmio_ops vlapic_mmio_ops = {
> >      .write = vlapic_mmio_write,
> >  };
> >  
> > +static uint32_t x2apic_ldr_from_id(uint32_t id)
> > +{
> > +    return ((id & ~0xf) << 12) | (1 << (id & 0xf));
> > +}
> > +
> >  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 vcpu_id = vlapic_vcpu(vlapic)->vcpu_id;
> > +    uint32_t apic_id = vcpu_id * 2;
> > +    uint32_t apic_ldr = x2apic_ldr_from_id(apic_id);
> > +
> > +    /* This is a migration bug workaround. See wall of text in lapic_load_fixup() */
> 
> Line length > 80 cols.
> 
> I try to avoid referencing function names in comments, as they tend to
> get out of sync without noticing.  It's much easier to use cscope to
> grep for x2apic_ldr_bug_with_vcpu_id and find the comment itself.
In my experience that's less of a problem than it's usually made out to be,
and helps new readers know about the real context something is in the place
it is.

But I do hold the atypical belief that an out of date pointer to context is
preferrable to no context.

> 
> > +    if ( vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id )
> > +        apic_ldr = x2apic_ldr_from_id(vcpu_id);
> >  
> > -    vlapic_set_reg(vlapic, APIC_ID, id * 2);
> > -    vlapic_set_reg(vlapic, APIC_LDR, ldr);
> > +    vlapic_set_reg(vlapic, APIC_ID, apic_id);
> > +    vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
> >  }
> >  
> >  int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
> > @@ -1498,27 +1508,35 @@ static int cf_check lapic_save_regs(struct vcpu *v, hvm_domain_context_t *h)
> >   */
> >  static void lapic_load_fixup(struct vlapic *vlapic)
> >  {
> > -    uint32_t id = vlapic->loaded.id;
> > +    /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */
> > +    if ( !vlapic_x2apic_mode(vlapic) ||
> > +         (vlapic->loaded.ldr == x2apic_ldr_from_id(vlapic->loaded.id)) )
> > +        return;
> >  
> > -    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
> > -    {
> > +    if ( vlapic->loaded.ldr == 1 )
> > +       /*
> > +        * Xen <= 4.4 had a bug by which all the APICs configured in x2APIC
> > +        * mode got LDR = 1. We can't leave it as-is because it assigned the
> > +        * same LDR to every CPU.  We'll fix fix the bug now and assign an
> > +        * LDR value consistent with the APIC ID.
> > +        */
> > +        set_x2apic_id(vlapic);
> > +    else if ( vlapic->loaded.ldr ==
> > +              x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id) )
> >          /*
> > -         * This is optional: ID != 0 contradicts LDR == 1. It's being added
> > -         * to aid in eventual debugging of issues arising from the fixup done
> > -         * here, but can be dropped as soon as it is found to conflict with
> > -         * other (future) changes.
> > +         * This is a migration from a broken Xen between 4.4 and 4.18 and
> > +         * we must _PRESERVE_ LDRs so new vCPUs use consistent derivations.
> 
> Not sure if we should try to avoid mentioning specific versions in the
> comments, as I this fix will be backported to stable branches (I hope),
> and hence those will no longer be affected.
Hence the "broken Xen" part of the paragraphs. Not every 4.18 will have the
problem, but it shouldn't be seen in 4.19 onwards. I think there's value in
stating the versions that "may" exhibit problems, but this is all
subjective 

> 
> > +         * This is so existing running guests that may have already read
> > +         * the LDR at the source host aren't surprised when IPIs stop
> > +         * working as they did at the other end. To address this, we set
> > +         * this domain boolean so future CPU hotplugs derive an LDR
> > +         * consistent with the older Xen's broken idea of consistency.
> 
> I think this is possibly too verbose, I would be fine with just the
> first sentence TBH.  If we want the full comment however, the wording
> should be slightly addressed: it's not just IPIs that would possibly
> fail to be delivered, but any interrupt attempting to target the APIC
> using the previous LDR addressing (either an IPI or an external
> interrupt).
I can s/IPIs/targetted interrupts/ and remove the second sentence.

> 
> >           */
> > -        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
> > -             id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
> > -            printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
> > -                   vlapic_vcpu(vlapic), id);
> > -        set_x2apic_id(vlapic);
> > -    }
> > -    else /* Undo an eventual earlier fixup. */
> > -    {
> > -        vlapic_set_reg(vlapic, APIC_ID, id);
> > -        vlapic_set_reg(vlapic, APIC_LDR, vlapic->loaded.ldr);
> > -    }
> > +        vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id = true;
> > +    else
> > +        printk(XENLOG_G_WARNING
> > +               "%pv: bogus x2APIC loaded id=%#x ldr=%#x\n",
> > +               vlapic_vcpu(vlapic), vlapic->loaded.id, vlapic->loaded.ldr);
> 
> Could you write the expected values while at it:
> 
> "%pv: bogus x2APIC loaded id=%#x ldr=%#x (expected id=%#x ldr=%#x)\n"
x2APIC ID is current strictly related to the vcpu ID, but it won't be after
I'm done with topology. I can print the expected LDR though.
> 
> >  }
> >  
> >  static int cf_check lapic_load_hidden(struct domain *d, hvm_domain_context_t *h)
> > diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
> > index 6e53ce4449..2fee3874cd 100644
> > --- a/xen/arch/x86/include/asm/hvm/domain.h
> > +++ b/xen/arch/x86/include/asm/hvm/domain.h
> > @@ -106,6 +106,9 @@ struct hvm_domain {
> >  
> >      bool                   is_s3_suspended;
> >  
> > +    /* Compat setting for a bug in x2APIC LDR. See vlapic_load_fixup() */
> > +    bool x2apic_ldr_bug_with_vcpu_id;
> 
> I think you already mentioned in a followup reply that using
> bug_x2apic_ldr_vcpu_id would be fine.
> 
> Thanks, Roger.

Cheers,
Alejandro

On Thu, Nov 23, 2023 at 12:21:36PM +0000, Alejandro Vallejo wrote:
> On Thu, Nov 23, 2023 at 10:03:12AM +0100, Roger Pau Monné wrote:
> > On Wed, Nov 22, 2023 at 04:08:17PM +0000, Alejandro Vallejo wrote:
> > > +static uint32_t x2apic_ldr_from_id(uint32_t id)
> > > +{
> > > +    return ((id & ~0xf) << 12) | (1 << (id & 0xf));
> > > +}
> > > +
> > >  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 vcpu_id = vlapic_vcpu(vlapic)->vcpu_id;
> > > +    uint32_t apic_id = vcpu_id * 2;
> > > +    uint32_t apic_ldr = x2apic_ldr_from_id(apic_id);
> > > +
> > > +    /* This is a migration bug workaround. See wall of text in lapic_load_fixup() */
> > 
> > Line length > 80 cols.
> > 
> > I try to avoid referencing function names in comments, as they tend to
> > get out of sync without noticing.  It's much easier to use cscope to
> > grep for x2apic_ldr_bug_with_vcpu_id and find the comment itself.
> In my experience that's less of a problem than it's usually made out to be,
> and helps new readers know about the real context something is in the place
> it is.
> 
> But I do hold the atypical belief that an out of date pointer to context is
> preferrable to no context.

It's a question of taste TBH, I'm certainly not going to insist.

Since you have to wrap the line to fit in 80 cols anyway, I think I
would rather write: "This is a workaround for migrated domains. ...".
Current text reads to me as it's a migration bug, but that's not the
case, the bug is in the previous Xen versions.  I'm not a native
speaker anyway, so maybe it's just me reading it wrong.

> > 
> > > +    if ( vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id )
> > > +        apic_ldr = x2apic_ldr_from_id(vcpu_id);
> > >  
> > > -    vlapic_set_reg(vlapic, APIC_ID, id * 2);
> > > -    vlapic_set_reg(vlapic, APIC_LDR, ldr);
> > > +    vlapic_set_reg(vlapic, APIC_ID, apic_id);
> > > +    vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
> > >  }
> > >  
> > >  int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
> > > @@ -1498,27 +1508,35 @@ static int cf_check lapic_save_regs(struct vcpu *v, hvm_domain_context_t *h)
> > >   */
> > >  static void lapic_load_fixup(struct vlapic *vlapic)
> > >  {
> > > -    uint32_t id = vlapic->loaded.id;
> > > +    /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */
> > > +    if ( !vlapic_x2apic_mode(vlapic) ||
> > > +         (vlapic->loaded.ldr == x2apic_ldr_from_id(vlapic->loaded.id)) )
> > > +        return;
> > >  
> > > -    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
> > > -    {
> > > +    if ( vlapic->loaded.ldr == 1 )
> > > +       /*
> > > +        * Xen <= 4.4 had a bug by which all the APICs configured in x2APIC
> > > +        * mode got LDR = 1. We can't leave it as-is because it assigned the
> > > +        * same LDR to every CPU.  We'll fix fix the bug now and assign an
> > > +        * LDR value consistent with the APIC ID.
> > > +        */
> > > +        set_x2apic_id(vlapic);
> > > +    else if ( vlapic->loaded.ldr ==
> > > +              x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id) )
> > >          /*
> > > -         * This is optional: ID != 0 contradicts LDR == 1. It's being added
> > > -         * to aid in eventual debugging of issues arising from the fixup done
> > > -         * here, but can be dropped as soon as it is found to conflict with
> > > -         * other (future) changes.
> > > +         * This is a migration from a broken Xen between 4.4 and 4.18 and
> > > +         * we must _PRESERVE_ LDRs so new vCPUs use consistent derivations.
> > 
> > Not sure if we should try to avoid mentioning specific versions in the
> > comments, as I this fix will be backported to stable branches (I hope),
> > and hence those will no longer be affected.
> Hence the "broken Xen" part of the paragraphs. Not every 4.18 will have the
> problem, but it shouldn't be seen in 4.19 onwards. I think there's value in
> stating the versions that "may" exhibit problems, but this is all
> subjective 

FE.

> > 
> > > +         * This is so existing running guests that may have already read
> > > +         * the LDR at the source host aren't surprised when IPIs stop
> > > +         * working as they did at the other end. To address this, we set
> > > +         * this domain boolean so future CPU hotplugs derive an LDR
> > > +         * consistent with the older Xen's broken idea of consistency.
> > 
> > I think this is possibly too verbose, I would be fine with just the
> > first sentence TBH.  If we want the full comment however, the wording
> > should be slightly addressed: it's not just IPIs that would possibly
> > fail to be delivered, but any interrupt attempting to target the APIC
> > using the previous LDR addressing (either an IPI or an external
> > interrupt).
> I can s/IPIs/targetted interrupts/ and remove the second sentence.

I would just use interrupts FWIW.

> > 
> > >           */
> > > -        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
> > > -             id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
> > > -            printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
> > > -                   vlapic_vcpu(vlapic), id);
> > > -        set_x2apic_id(vlapic);
> > > -    }
> > > -    else /* Undo an eventual earlier fixup. */
> > > -    {
> > > -        vlapic_set_reg(vlapic, APIC_ID, id);
> > > -        vlapic_set_reg(vlapic, APIC_LDR, vlapic->loaded.ldr);
> > > -    }
> > > +        vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id = true;
> > > +    else
> > > +        printk(XENLOG_G_WARNING
> > > +               "%pv: bogus x2APIC loaded id=%#x ldr=%#x\n",
> > > +               vlapic_vcpu(vlapic), vlapic->loaded.id, vlapic->loaded.ldr);
> > 
> > Could you write the expected values while at it:
> > 
> > "%pv: bogus x2APIC loaded id=%#x ldr=%#x (expected id=%#x ldr=%#x)\n"
> x2APIC ID is current strictly related to the vcpu ID, but it won't be after
> I'm done with topology. I can print the expected LDR though.

Oh, I see.  Just printing the expected LDR then would be fine.

Thanks, Roger.

On 23/11/2023 2:03 pm, Roger Pau Monné wrote:
> On Thu, Nov 23, 2023 at 12:21:36PM +0000, Alejandro Vallejo wrote:
>> On Thu, Nov 23, 2023 at 10:03:12AM +0100, Roger Pau Monné wrote:
>>> On Wed, Nov 22, 2023 at 04:08:17PM +0000, Alejandro Vallejo wrote:
>>>> +    if ( vlapic_domain(vlapic)->arch.hvm.x2apic_ldr_bug_with_vcpu_id )
>>>> +        apic_ldr = x2apic_ldr_from_id(vcpu_id);
>>>>  
>>>> -    vlapic_set_reg(vlapic, APIC_ID, id * 2);
>>>> -    vlapic_set_reg(vlapic, APIC_LDR, ldr);
>>>> +    vlapic_set_reg(vlapic, APIC_ID, apic_id);
>>>> +    vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
>>>>  }
>>>>  
>>>>  int guest_wrmsr_apic_base(struct vcpu *v, uint64_t val)
>>>> @@ -1498,27 +1508,35 @@ static int cf_check lapic_save_regs(struct vcpu *v, hvm_domain_context_t *h)
>>>>   */
>>>>  static void lapic_load_fixup(struct vlapic *vlapic)
>>>>  {
>>>> -    uint32_t id = vlapic->loaded.id;
>>>> +    /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */
>>>> +    if ( !vlapic_x2apic_mode(vlapic) ||
>>>> +         (vlapic->loaded.ldr == x2apic_ldr_from_id(vlapic->loaded.id)) )
>>>> +        return;
>>>>  
>>>> -    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
>>>> -    {
>>>> +    if ( vlapic->loaded.ldr == 1 )
>>>> +       /*
>>>> +        * Xen <= 4.4 had a bug by which all the APICs configured in x2APIC
>>>> +        * mode got LDR = 1. We can't leave it as-is because it assigned the
>>>> +        * same LDR to every CPU.  We'll fix fix the bug now and assign an
>>>> +        * LDR value consistent with the APIC ID.
>>>> +        */
>>>> +        set_x2apic_id(vlapic);
>>>> +    else if ( vlapic->loaded.ldr ==
>>>> +              x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id) )
>>>>          /*
>>>> -         * This is optional: ID != 0 contradicts LDR == 1. It's being added
>>>> -         * to aid in eventual debugging of issues arising from the fixup done
>>>> -         * here, but can be dropped as soon as it is found to conflict with
>>>> -         * other (future) changes.
>>>> +         * This is a migration from a broken Xen between 4.4 and 4.18 and
>>>> +         * we must _PRESERVE_ LDRs so new vCPUs use consistent derivations.
>>> Not sure if we should try to avoid mentioning specific versions in the
>>> comments, as I this fix will be backported to stable branches (I hope),
>>> and hence those will no longer be affected.
>> Hence the "broken Xen" part of the paragraphs. Not every 4.18 will have the
>> problem, but it shouldn't be seen in 4.19 onwards. I think there's value in
>> stating the versions that "may" exhibit problems, but this is all
>> subjective 
> FE.

This patch has a high chance of being backported.  Old version numbers
(4.4 in this case) are critical information.

For the new end, "to date (Nov 2023)" or similar tends to works better,
because it also doesn't go stale when backported.

If a version is necessary/preferred, then "4.19 dev window" also works well.

~Andrew

On Wed, Nov 22, 2023 at 04:08:17PM +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)
> 
> This patch fixes the implementation so we follow the rules in the x2APIC
> spec(s) and covers migrations from broken hypervisors, so LDRs are
> preserved even for hotppluggable CPUs and across APIC resets.
> 
> While touching that area, I removed the existing printk statement in
> vlapic_load_fixup() (as the checks it performed didn't make sense in x2APIC
> mode and wouldn't affect the outcome) and put another printk as an else
> branch so we get warnings trying to load nonsensical LDR values we don't
> know about.
> 
> Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> v3:
>   * Removed underscores from (x2)APIC_ID in commit message
>   * Added commit msg explaining the removal of the vlapic_load_fixup() printk
>   * Restored lowercase to hex "F"s
>   * Refactored a bit vlapic_load_fixup() so it has a trailing printk in
>     case of spotting a nonsensical LDR it doesn't understand.
>   * Moved field in domain.h with the other booleans
>   * Trimmed down field name in domain.h
>   * Trimmed down field comment in domain.h
> 
> If the field name in domain.h still seems too long I'm happy for it to be
> trimmed further down, but I do want to preserve the "_bug_" part of it.
I sent this one before Roger had a chance to reply to my reply on v2, which was...

> 
> OK, if you think mentioning `bug` is helpful, I think it would be best
> placed as a prefix: bug_x2apic_ldr_vcpu_id.  Having it in the middle
> of the field name makes it harder to spot.
> 
> Thanks, Roger.
... and I'm fine with that adjustment here.

Cheers,
Alejandro