From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:50189)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <ehabkost@redhat.com>) id 1bNnJw-0006lp-0W
	for qemu-devel@nongnu.org; Thu, 14 Jul 2016 16:33:23 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <ehabkost@redhat.com>) id 1bNnJr-0003mq-K3
	for qemu-devel@nongnu.org; Thu, 14 Jul 2016 16:33:19 -0400
Date: Thu, 14 Jul 2016 17:33:05 -0300
From: Eduardo Habkost <ehabkost@redhat.com>
Message-ID: <20160714203305.GF31865@thinpad.lan.raisama.net>
References: <1465580427-13596-1-git-send-email-drjones@redhat.com>
	<1465580427-13596-10-git-send-email-drjones@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1465580427-13596-10-git-send-email-drjones@redhat.com>
Subject: Re: [Qemu-devel] [PATCH RFC 09/16] hw/i386/pc: don't use smp_cores,
 smp_threads
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Andrew Jones <drjones@redhat.com>
Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, qemu-arm@nongnu.org, peter.maydell@linaro.org, agraf@suse.de, pbonzini@redhat.com, dgibson@redhat.com, imammedo@redhat.com, david@gibson.dropbear.id.au

On Fri, Jun 10, 2016 at 07:40:20PM +0200, Andrew Jones wrote:
[...]
> @@ -1940,9 +1943,10 @@ static void pc_machine_reset(void)
>  
>  static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
>  {
> +    CPUState *cs = first_cpu;

Eww.

>      X86CPUTopoInfo topo;
> -    x86_topo_ids_from_idx(smp_cores, smp_threads, cpu_index,
> -                          &topo);
> +
> +    x86_topo_ids_from_idx(cs->nr_cores, cs->nr_threads, cpu_index, &topo);

If first_cpu is already available[1], then current_machine would
also be available. Better to use the new MachineState fields you
added, than the CPUState::nr_{cores,threads} fields that
shouldn't have existed in the first place.

[1] Is it really available? This function is called very early,
    from parse_numa_opts(). I think current_machine is already
    available, though. But we need to ensure parse_numa_opts()
    will be called only after SMP option parsing is done and SMP
    topology data in MachineState is already populated.

    Later, we could also make the NUMA option parsing not depend
    on SMP parsing to be done first. The cpu_index_to_socket_id()
    magic can be moved to numa_post_machine_init(), probably.

-- 
Eduardo