Re: [PATCH v4] xen/x86: On x2APIC mode, derive LDR from APIC ID

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 24/11/2023 22:05, Andrew Cooper wrote:
> > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> > index 5cb87f8649..cd4701c5a2 100644
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -1061,13 +1061,26 @@ 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)
> >   {
> const struct vcpu *v = vlapic_vcpu(vlapic);
>
> seeing as you've got the expression used twice already.
>
> With that, the following logic is shorter too; you can get away with
> dropping the vcpu_id variable as v->vcpu_id is the more common form to
> use in Xen.
Twice? I can see a vague raison-d'etre in lapic_load_fixup(), but
there's a single occurence here.

> >   We must preserve LDRs so new vCPUs use consistent
> > +         * derivations and existing guests, which may have already read the
> > +         * LDR at the source host, aren't surprised when interrupts stop
> > +         * working the way they did at the other end.
> >            */
> > -        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.bug_x2apic_ldr_vcpu_id = true;
> > +    else
> > +        printk(XENLOG_G_WARNING
> > +               "%pv: bogus x2APIC loaded id=%#x ldr=%#x (expected ldr=%#x)\n",
> %pv: bogus loaded x2APIC ID %#x, LDR %#x, expected LDR %#x\n
>
> If you properly capitalise x2APIC, you should capitalise ID and LDR.
> The other changes are matter of taste, but make for a less cluttered log
> message IMO.
"bogus x2APIC loaded" is meant to be a sentence followed by key-value 
pairs. Uppercasing the keys is fine (albeit unnecessary, IMO), but you 
choice of word order feels backwards.
> This is a long list of minor niggles, but they're all style/comment
> issues, and nothing to do with the logic itself.  I'm happy to fix them
> all up on commit, and here is the result with them merged:
>
> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=commitdiff;h=953dcb0317d20959ffee14e404595cfbb66c607a
>
> for you to glance over.
>
> ~Andrew
Except for the 2 bits pointed out, the others are cosmetic changes
I don't particularly mind. Jan spotted a typo in the second comment 
block in lapic_load_fixup() that should be corrected too.
(s/fix fix/fix/)

Cheers,
Alejandro