Re: [Qemu-devel] [PATCH 15/22] machine: make max_cpus a -machine option

[Jes Sorensen]

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 <address@hidden>
> 
[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