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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Wed, Nov 22, 2023 at 04:08:17PM +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) and covers migrations from broken hypervisors, so LDRs are
> preserved even for hotppluggable CPUs and across APIC resets.
> 
> While touching that area, I removed the existing printk statement in
> vlapic_load_fixup() (as the checks it performed didn't make sense in x2APIC
> mode and wouldn't affect the outcome) and put another printk as an else
> branch so we get warnings trying to load nonsensical LDR values we don't
> know about.
> 
> Fixes: f9e0cccf7b35 ("x86/HVM: fix ID handling of x2APIC emulation")
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
> v3:
>   * Removed underscores from (x2)APIC_ID in commit message
>   * Added commit msg explaining the removal of the vlapic_load_fixup() printk
>   * Restored lowercase to hex "F"s
>   * Refactored a bit vlapic_load_fixup() so it has a trailing printk in
>     case of spotting a nonsensical LDR it doesn't understand.
>   * Moved field in domain.h with the other booleans
>   * Trimmed down field name in domain.h
>   * Trimmed down field comment in domain.h
> 
> If the field name in domain.h still seems too long I'm happy for it to be
> trimmed further down, but I do want to preserve the "_bug_" part of it.
I sent this one before Roger had a chance to reply to my reply on v2, which 
was...

> 
> 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.
... and I'm fine with that adjustment here.

Cheers,
Alejandro