Re: [XEN PATCH 3/9] x86/smp: drop x86_cpu_to_apicid, use cpu_data[cpu].apicid instead

From: Jan Beulich <jbeulich@suse.com>
To: Krystian Hebel <krystian.hebel@3mdeb.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	xen-devel@lists.xenproject.org, "Wei Liu" <wl@xen.org>
Subject: Re: [XEN PATCH 3/9] x86/smp: drop x86_cpu_to_apicid, use cpu_data[cpu].apicid instead
Date: Thu, 8 Feb 2024 08:29:07 +0100	[thread overview]
Message-ID: <0d776dcc-af14-436a-bf8b-9bfb39b787b8@suse.com> (raw)
In-Reply-To: <8121d9b472b305be751158aa3af3fed98ff0572e.1699982111.git.krystian.hebel@3mdeb.com>

On 14.11.2023 18:50, Krystian Hebel wrote:
> Both fields held the same data.

Supposedly the same data only. They come from different origins, and you're
hiding this quite well by leaving all sites in place where the field is
written. Both items are also used for entirely separate purposes. So you
need to
- explain why both sources of information necessarily provide the same
  data,
- especially if there's remaining concern from the above explanation that
  the two values might end up different in corner cases (running
  virtualized ourselves comes to mind as a possible example), explain why
  nevertheless it is fine (risk free) to use the consolidated item for
  all of the originally separate purposes,
- either explain or do away with the multiple places setting this single
  remaining field.

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -61,10 +61,8 @@ unsigned int __read_mostly nr_sockets;
>  cpumask_t **__read_mostly socket_cpumask;
>  static cpumask_t *secondary_socket_cpumask;
>  
> -struct cpuinfo_x86 cpu_data[NR_CPUS];
> -
> -u32 x86_cpu_to_apicid[NR_CPUS] __read_mostly =
> -	{ [0 ... NR_CPUS-1] = BAD_APICID };
> +struct cpuinfo_x86 cpu_data[NR_CPUS] =
> +        { [0 ... NR_CPUS-1] .apicid = BAD_APICID };

Nit: Stray blank after closing square bracket.

Jan