Re: [PATCH 2/2] x86/xen: return a sane initial apic id when running as PV guest

[Andrew Cooper]

On 05/04/2024 1:34 pm, Juergen Gross wrote:
> With recent sanity checks for topology information added, there are now
> warnings issued for APs when running as a Xen PV guest:
>
>   [Firmware Bug]: CPU   1: APIC ID mismatch. CPUID: 0x0000 APIC: 0x0001
>
> This is due to the initial APIC ID obtained via CPUID for PV guests is
> always 0.

/sigh

>From Xen:

    switch ( leaf )
    {
    case 0x1:
        /* TODO: Rework topology logic. */
        res->b &= 0x00ffffffu;
        if ( is_hvm_domain(d) )
            res->b |= (v->vcpu_id * 2) << 24;


I think there's a very good chance it was random prior to Xen 4.6.  That
used to come straight out of a CPUID value, so would get the APIC ID of
whichever pCPU it was scheduled on.

> Avoid the warnings by synthesizing the CPUID data to contain the same
> initial APIC ID as xen_pv_smp_config() is using for registering the
> APIC IDs of all CPUs.
>
> Fixes: 52128a7a21f7 ("86/cpu/topology: Make the APIC mismatch warnings complete")
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  arch/x86/xen/enlighten_pv.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index ace2eb054053..965e4ca36024 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -219,13 +219,20 @@ static __read_mostly unsigned int cpuid_leaf5_edx_val;
>  static void xen_cpuid(unsigned int *ax, unsigned int *bx,
>  		      unsigned int *cx, unsigned int *dx)
>  {
> -	unsigned maskebx = ~0;
> +	unsigned int maskebx = ~0;
> +	unsigned int or_ebx = 0;
>  
>  	/*
>  	 * Mask out inconvenient features, to try and disable as many
>  	 * unsupported kernel subsystems as possible.
>  	 */
>  	switch (*ax) {
> +	case 0x1:
> +		/* Replace initial APIC ID in bits 24-31 of EBX. */
> +		maskebx = 0x00ffffff;
> +		or_ebx = smp_processor_id() << 24;

I think the comment wants to cross-reference explicitly with
xen_pv_smp_config(), because what we care about here is the two sources
of information matching.

Also while you're at it, the x2APIC ID in leaf 0xb.

~Andrew