On Wed, Jun 07, 2023 at 04:35:03PM +0200, Igor Mammedov wrote:
> Date: Wed, 7 Jun 2023 16:35:03 +0200
> From: Igor Mammedov <imamm...@redhat.com>
> Subject: Re: [PATCH v2 1/3] hw/smbios: Fix smbios_smp_sockets caculation
> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu)
Hi Igor,
>
> On Thu, 1 Jun 2023 17:29:50 +0800
> Zhao Liu <zhao1....@linux.intel.com> wrote:
>
> > From: Zhao Liu <zhao1....@intel.com>
> >
> > Here're 2 mistakes:
> > 1. 003f230e37d7 ("machine: Tweak the order of topology members in struct
> > CpuTopology") changes the meaning of smp.cores but doesn't fix
> > original smp.cores uses. And because of the introduction of cluster,
> > now smp.cores means the number of cores in one cluster. So smp.cores
> > * smp.threads just means the cpus in a cluster not in a socket.
>
> > 2. smp.cpus means the number of initial online cpus, not the total
> > number of cpus. For such topology calculation, smp.max_cpus
> > should be considered.
> that's probably not relevant to the patch.
>
For the 2nd point, I mean the original calculation should use max_cpus
other than cpus to calculate sockets:
- smbios_smp_sockets = DIV_ROUND_UP(ms->smp.cpus,
+ smbios_smp_sockets = DIV_ROUND_UP(ms->smp.max_cpus,
ms->smp.cores * ms->smp.threads);
But since we already have smp.sockets, we can use it directly.
>
> >
> > Since the number of sockets has already been recorded in smp structure,
> > use smp.sockets directly.
>
>
> I'd rephrase commit message to something like this:
> ---
> CPU topology is calculated by ..., and trying to recalculate it here
> with another rules leads to an error, such as
>
> ... example follows ..
>
> So stop reinventing the another wheel and use topo values that ... has
> calculated.
Looks good for me. Thanks!
Regards,
Zhao
>
> >
> > Fixes: 003f230e37d7 ("machine: Tweak the order of topology members in
> > struct CpuTopology")
> > Signed-off-by: Zhao Liu <zhao1....@intel.com>
> > ---
> > hw/smbios/smbios.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index d2007e70fb05..d67415d44dd8 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -1088,8 +1088,7 @@ void smbios_get_tables(MachineState *ms,
> > smbios_build_type_2_table();
> > smbios_build_type_3_table();
> >
> > - smbios_smp_sockets = DIV_ROUND_UP(ms->smp.cpus,
> > - ms->smp.cores * ms->smp.threads);
> > + smbios_smp_sockets = ms->smp.sockets;
> > assert(smbios_smp_sockets >= 1);
> >
> > for (i = 0; i < smbios_smp_sockets; i++) {
>
