[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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |