[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Tue, Nov 21, 2023 at 04:26:04PM +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).
> 
> The patch also covers migrations from broken hypervisors, so LDRs are
> preserved even for hotppluggable CPUs and across APIC resets.
> 
> Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>

LGTM, just a couple of style comments.

> ---
> I tested this by creating 3 checkpoints.
>  1. One with pre-4.4 broken state (every LDR=1, by hacking save_regs)
>  2. One with 4.4 onwards broken state (LDRs packed in their clusters)
>  3. One with correct LDR values
> 
> (1) and (3) restores to the same thing. Consistent APIC_ID+LDR
> (2) restores to what it previously had and hotplugs follow the same logic
> ---
>  xen/arch/x86/hvm/vlapic.c             | 81 +++++++++++++++++++--------
>  xen/arch/x86/include/asm/hvm/domain.h | 13 +++++
>  2 files changed, 72 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index a8e87c4446..7f169f1e5f 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));

Seeing other usages in vlapic.c I think the preference is to use lower
case for hex numbers.

> +}
> +
>  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);
>  
> -    vlapic_set_reg(vlapic, APIC_ID, id * 2);
> -    vlapic_set_reg(vlapic, APIC_LDR, ldr);
> +    /* This is a migration bug workaround. See wall of text in 
> lapic_load_fixup() */
> +    if ( vlapic_domain(vlapic)->arch.hvm.has_inconsistent_x2apic_ldr_bug )
> +        apic_ldr = x2apic_ldr_from_id(vcpu_id);
> +
> +    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)
> @@ -1495,30 +1505,57 @@ static int cf_check lapic_save_regs(struct vcpu *v, 
> hvm_domain_context_t *h)
>  /*
>   * Following lapic_load_hidden()/lapic_load_regs() we may need to
>   * correct ID and LDR when they come from an old, broken hypervisor.
> + *
> + * Xen <= 4.4 had a bug by which all the APICs configured in x2APIC mode
> + * got LDR = 1. This was fixed back then, but another bug was introduced
> + * causing APIC ID and LDR to break the consistency they are meant to have
> + * according to the specs (LDR was derived from vCPU ID, rather than APIC
> + * ID)
> + *
> + * Long story short, we can detect both cases here. For the LDR=1 case we
> + * want to fix it up on migrate, as IPIs just don't work on non-physical
> + * mode otherwise. For the other case we actually want to preserve previous
> + * behaviour so that existing running instances 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.
> + *
> + * Note that "x2apic_id == 0" has always been correct and can't be used to
> + * discriminate these cases.
> + *

I think it's best if this big comment was split between the relevant
parts of the if below used to detect the broken states, as that makes
the comments more in-place with the code.

> + * Yuck!
>   */
>  static void lapic_load_fixup(struct vlapic *vlapic)
>  {
> -    uint32_t id = vlapic->loaded.id;
> +    /*
> +     * This LDR would be present in broken versions of Xen 4.4 through 4.18.
> +     * It's correct for the cpu with x2apic_id=0 (vcpu0), but invalid for
> +     * any other.
> +     */
> +    uint32_t bad_ldr = x2apic_ldr_from_id(vlapic_vcpu(vlapic)->vcpu_id);
>  
> -    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
> -    {
> +    /*
> +     * No need to perform fixups in non-x2apic mode, and x2apic_id == 0 has
> +     * always been correct.
> +     */
> +    if ( !vlapic_x2apic_mode(vlapic) || !vlapic->loaded.id )

You could replace the !vlapic->loaded.id check and instead use:

vlapic->loaded.ldr == x2apic_ldr_from_id(vlapic->loaded.id)

As that will allow returning early from the function if the LDR is
correct.  Then if none of the fixups below apply we could print a
warning message that the LDR is incorrect, but cannot be fixed up.

> +        return;
> +
> +    if ( vlapic->loaded.ldr == 1 )
> +       /*
> +        * Migration from a broken Xen 4.4 or earlier. We can't leave it
> +        * as-is because it assigned the same LDR to every CPU. We'll fix
> +        * the bug now and assign LDR values consistent with the APIC ID.
> +        */
> +        set_x2apic_id(vlapic);

Previous code also did some checks here related to APIC ID sanity,
which are now dropped?

Might be worth mentioning in the commit message, if that was intended.

> +    else if ( bad_ldr == vlapic->loaded.ldr )
>          /*
> -         * 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. In
> +         * this case 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.has_inconsistent_x2apic_ldr_bug = 
> true;
>  }
>  
>  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..a42a6e99bb 100644
> --- a/xen/arch/x86/include/asm/hvm/domain.h
> +++ b/xen/arch/x86/include/asm/hvm/domain.h
> @@ -61,6 +61,19 @@ struct hvm_domain {
>      /* Cached CF8 for guest PCI config cycles */
>      uint32_t                pci_cf8;
>  
> +    /*
> +     * Xen had a bug between 4.4 and 4.18 by which the x2APIC LDR was
> +     * derived from the vcpu_id rather than the x2APIC ID. This is wrong,
> +     * but changing this behaviour is tricky because guests might have
> +     * already read the LDR and used it accordingly. In the interest of not
> +     * breaking migrations from those hypervisors we track here whether
> +     * this domain suffers from this or not so a hotplugged vCPU or an APIC
> +     * reset can recover the same LDR it would've had on the older host.
> +     *
> +     * Yuck!
> +     */
> +    bool has_inconsistent_x2apic_ldr_bug;

Could you place the new field after the existing boolean fields in the
struct? (AFAICT there's plenty of padding left there)

I also think the field name is too long, I would rather use
x2apic_ldr_vcpu_id for example (to note that LDR is calculated from
vCPU ID rather than APIC ID).

I think it would be good if we could trim a bit the comments, as I get
the impression it's a bit repetitive.

So I would leave the big explanation in lapic_load_fixup(), and just
comment here:

/* Compatibility setting for a bug in x2APIC LDR format. */

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.