From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=58676 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OMG0Z-0003kU-MD for qemu-devel@nongnu.org; Wed, 09 Jun 2010 03:47:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OMG0Y-0005kw-4v for qemu-devel@nongnu.org; Wed, 09 Jun 2010 03:47:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13355) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OMG0X-0005km-TY for qemu-devel@nongnu.org; Wed, 09 Jun 2010 03:47:30 -0400 Message-ID: <4C0F470E.2020900@redhat.com> Date: Wed, 09 Jun 2010 09:47:26 +0200 From: Jes Sorensen MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 15/22] machine: make max_cpus a -machine option References: <1275954730-8196-1-git-send-email-aliguori@us.ibm.com> <1275954730-8196-16-git-send-email-aliguori@us.ibm.com> In-Reply-To: <1275954730-8196-16-git-send-email-aliguori@us.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Glauber Costa , qemu-devel@nongnu.org On 06/08/10 01:52, Anthony Liguori wrote: > max_cpus is a weird property today. On the one hand, it represents the maximum > CPUs a board can support and is used to validate the number of vcpus requested > by the user. > > On the other hand, max_cpus can be set by the user in which case it is taken > to mean the number of physical sockets that should be advertised by the > firmware. Furthermore, if max_cpus isn't explicitly set by the user, it's > defaulted to the number of smp_cpus. But there's actually a second copy of > max_cpus that still represents the maximum cpus spported by the platform. Sorry this is wrong, max_cpus never meant to advertise the number of sockets, it means the number of cores (ie. cpus) as the limit advertised by firmware. > Yes, it's confusing. So let's be a bit more obvious. This patch introduces > a sockets parameter that allows a user to explicitly set the number of > advertised sockets apart from the number of maximum cpus. > > This is something of a stop gap. We really ought to specify a more rich > NUMA topology as machine options but that will come later. > > Signed-off-by: Anthony Liguori > [snip] > diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c > index 22ebb50..de4454f 100644 > --- a/hw/fw_cfg.c > +++ b/hw/fw_cfg.c > @@ -321,7 +321,8 @@ int fw_cfg_add_file(FWCfgState *s, const char *dir, const char *filename, > } > > FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port, > - target_phys_addr_t ctl_addr, target_phys_addr_t data_addr) > + target_phys_addr_t ctl_addr, target_phys_addr_t data_addr, > + QemuOpts *opts) > { > FWCfgState *s; > int io_ctl_memory, io_data_memory; > @@ -349,7 +350,8 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port, > fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16); > fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC)); > fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); > - fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus); > + fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, > + (uint16_t)qemu_opt_get_number(opts, "sockets", smp_cpus)); > fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu); NACK this is just plain wrong! Sorry Anthony, but max_cpus never meant number of sockets. It is used by the BIOS to calculate table sizes for CPU entries, and that number is based on cores, not sockets. If you want to pass a number of sockets onto the BIOS, I will totally support that, but you need to introduce FW_CFG_MAX_SOCKETS for that instead, and tell the BIOS to use that for the SRAT. Cheers, Jes