Re: [PATCH 1/5] hw/smbios: add core_count2 to smbios table type 4

[Igor Mammedov]

On Mon, 6 Jun 2022 13:11:36 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> On Thu, Jun 2, 2022 at 4:35 PM Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 2 Jun 2022 16:31:25 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >  
> > > On Tue, 31 May 2022 14:40:15 +0200
> > > Julia Suvorova <jusual@redhat.com> wrote:
> > >  
> > > > On Sat, May 28, 2022 at 6:34 AM Ani Sinha <ani@anisinha.ca> wrote:  
> > > > >
> > > > >
> > > > >
> > > > > On Fri, 27 May 2022, Julia Suvorova wrote:
> > > > >  
> > > > > > In order to use the increased number of cpus, we need to bring 
> > > > > > smbios
> > > > > > tables in line with the SMBIOS 3.0 specification. This allows us to
> > > > > > introduce core_count2 which acts as a duplicate of core_count if we 
> > > > > > have
> > > > > > fewer cores than 256, and contains the actual core number per 
> > > > > > socket if
> > > > > > we have more.
> > > > > >
> > > > > > core_enabled2 and thread_count2 fields work the same way.
> > > > > >
> > > > > > Signed-off-by: Julia Suvorova <jusual@redhat.com>  
> > > > >
> > > > > Other than the comment below,
> > > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > >  
> > > > > > ---
> > > > > >  include/hw/firmware/smbios.h |  3 +++
> > > > > >  hw/smbios/smbios.c           | 11 +++++++++--
> > > > > >  2 files changed, 12 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/include/hw/firmware/smbios.h 
> > > > > > b/include/hw/firmware/smbios.h
> > > > > > index 4b7ad77a44..c427ae5558 100644
> > > > > > --- a/include/hw/firmware/smbios.h
> > > > > > +++ b/include/hw/firmware/smbios.h
> > > > > > @@ -187,6 +187,9 @@ struct smbios_type_4 {
> > > > > >      uint8_t thread_count;
> > > > > >      uint16_t processor_characteristics;
> > > > > >      uint16_t processor_family2;
> > > > > > +    uint16_t core_count2;
> > > > > > +    uint16_t core_enabled2;
> > > > > > +    uint16_t thread_count2;  
> >
> > on the other hand,
> > is it ok unconditionally extend type 4 and use v3 structure
> > if qemu was started with v2 smbios?  
> 
> We have a flag for the entry point type, not the smbios version per
> se. Additional fields added to structures were not previously tracked
> (for instance, processor_family2 is v2.6 and status is v2.0). AFAIU it
that's a bug, unless there is a very good reason to keep doing that,
I'd not do that.

> can affect only the total table length and the maximum structure size

table length is fixed depending on used version,
so if we follow it, we should set length and use part of structure
correctly if old version is specified (i.e.
   1. old structure + v30 structure,
   2. copy only a relevant part of v30 structure and
      fixup length when v2 version is asked for
though, I'd prefer #1

> (word). But if you like, I can raise an error (warning) if someone
> tries to start a vm with cpus > 255 and smbios v2.

it's a separate thing, I'd error out
(it will break users that use v2 with too may CPUs but then
they should fix their configuration to use v3)

> Best regards, Julia Suvorova.
> 
> > > > >
> > > > > I would add a comment along the lines of
> > > > > /* section 7.5, table 21 smbios spec version 3.0.0 */  
> > > >
> > > > Ok  
> > >
> > > With Ani's comment fixed
> > >
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > >  
> > > >  
> > > > > >  } QEMU_PACKED;
> > > > > >
> > > > > >  /* SMBIOS type 11 - OEM strings */
> > > > > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > > > > > index 60349ee402..45d7be6b30 100644
> > > > > > --- a/hw/smbios/smbios.c
> > > > > > +++ b/hw/smbios/smbios.c
> > > > > > @@ -709,8 +709,15 @@ static void 
> > > > > > smbios_build_type_4_table(MachineState *ms, unsigned instance)
> > > > > >      SMBIOS_TABLE_SET_STR(4, serial_number_str, type4.serial);
> > > > > >      SMBIOS_TABLE_SET_STR(4, asset_tag_number_str, type4.asset);
> > > > > >      SMBIOS_TABLE_SET_STR(4, part_number_str, type4.part);
> > > > > > -    t->core_count = t->core_enabled = ms->smp.cores;
> > > > > > -    t->thread_count = ms->smp.threads;
> > > > > > +
> > > > > > +    t->core_count = (ms->smp.cores > 255) ? 0xFF : ms->smp.cores;
> > > > > > +    t->core_enabled = t->core_count;
> > > > > > +
> > > > > > +    t->core_count2 = t->core_enabled2 = cpu_to_le16(ms->smp.cores);
> > > > > > +
> > > > > > +    t->thread_count = (ms->smp.threads > 255) ? 0xFF : 
> > > > > > ms->smp.threads;
> > > > > > +    t->thread_count2 = cpu_to_le16(ms->smp.threads);
> > > > > > +
> > > > > >      t->processor_characteristics = cpu_to_le16(0x02); /* Unknown */
> > > > > >      t->processor_family2 = cpu_to_le16(0x01); /* Other */
> > > > > >
> > > > > > --
> > > > > > 2.35.1
> > > > > >
> > > > > >  
> > > > >  
> > > >  
> > >  
> >  
>