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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Nov 22, 2023 at 03:11:52PM +0000, Alejandro Vallejo wrote:
> On Wed, Nov 22, 2023 at 02:40:02PM +0100, Roger Pau Monné wrote:
> > On Tue, Nov 21, 2023 at 04:26:04PM +0000, Alejandro Vallejo wrote:
> > > 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.
> I thought it was covered by a MISRA rule, but it seems to apply only to the
> literal suffixes. Fair enough, I'll revert to lowercase.

FWIW, I'm thinking that I want to move x2apic_ldr_from_id() to a
header file, so that it can also be used by the native APIC driver in
order to detect when the LDR field is not configured according to the
spec (so adding a consistency check in
init_apic_ldr_x2apic_cluster()).

Anyway, might look into doing that after this patch is in.

> > > +    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);
> ... these. But they _seem_ bogus for several reasons. And I just got rid of
> them on these grounds:
> 
>   * It's using the GET_xAPIC_ID() macro on the register, but the LAPIC is
>     not in xAPIC mode (due to the previous check), so the legacy APIC must
>     be derived from the lowest octet, not the highest. That macro is meant
>     to be used on the MMIO register, not the MSR one.
>   * The printk (wants to be) triggered when the ID field is not "canonical"
>     OR the x2APIC ID is not a pure xAPIC ID. Neither of these things are
>     problems per se, the former just happens to be true at the moment, but
>     the latter may very well not be, and that shouldn't trigger a scary
>     printk.
>   * The error message seems to imply the effective APIC ID eventually left
>     loaded is the bogus one, but that's not the case.
> 
> Actually, I might just move the printk into a separate else in line with
> your previous suggestion.

Sounds good.  And I agree that using {GET,SET}_xAPIC_ID() on the
x2APIC 32bit width IDs looks to be wrong.

> > >  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)
> Sure. I placed it somewhere with unused padding. (that u32 is sandwiched
> between an "unsigned long" and a pointer), but I'm happy to stash it with
> the other booleans.

Yes, there's plenty of padding but I feel it's better to place it with
the rest of the booleans, as there's also padding 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 made it intentionally long so it can't be missed that it's a workaround
> and not architectural behaviour. Would you consider
> "x2apic_ldr_bug_vcpu_id" acceptable? I'd rather keep at least the "bug"
> part of the identifier around so it's not lost to time that this is a
> workaround marker and nothing else

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.