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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 27/11/2023 08:40, Jan Beulich wrote:
> On 23.11.2023 18:30, Alejandro Vallejo wrote:
> > @@ -1498,27 +1511,36 @@ 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;
> > +    uint32_t good_ldr = x2apic_ldr_from_id(vlapic->loaded.id);
> >   
> > -    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
> > -    {
> > +    /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */
> > +    if ( !vlapic_x2apic_mode(vlapic) ||
> > +         (vlapic->loaded.ldr == good_ldr) )
> > +        return;
> > +
> > +    if ( vlapic->loaded.ldr == 1 )
> > +       /*
> > +        * Xen <= 4.4 may have 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.
> > +        */
> Just one comment on top of Andrew's: Is the double "fix" really intended
> here? (I could see it might be, but then "fix the bug fix" would read
> slightly more smoothly to me as a non-native speaker.)
It's not intended indeed. s/fix fix/fix/

> Another aspect here is what exactly the comment states (and does not
> state). Original comments made explicit that LDR == 1 contradicts ID == 0.
> In the new comment you only emphasize that all CPUs cannot have that same
> LDR. But the value of 1 being bogus in the first place doesn't become clear
> anymore.
>
> Jan
1 is bogus for id!=0, but so would be 3, 7 or 42. In particular we have
ID==2 contradicting LDR=2, and we're allowing it. The reason why we must
fix this other case is because all LDRs are equal, otherwise it would
get the same treatment as the other bug.

Cheers,
Alejandro