Re: [PATCH v3] hw/smbios: add options for type 4 max-speed and current-speed

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 3 Mar 2020 11:18:56 +0800
Heyi Guo <guoheyi@huawei.com> wrote:

> One comment from myself after going through the code...
> 
> On 2020/3/3 9:01, Heyi Guo wrote:
> > Common VM users sometimes care about CPU speed, so we add two new
> > options to allow VM vendors to present CPU speed to their users.
> > Normally these information can be fetched from host smbios.
> >
> > Strictly speaking, the "max speed" and "current speed" in type 4
> > are not really for the max speed and current speed of processor, for
> > "max speed" identifies a capability of the system, and "current speed"
> > identifies the processor's speed at boot (see smbios spec), but some
> > applications do not tell the differences.
> >
> > Signed-off-by: Heyi Guo <guoheyi@huawei.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >
> > ---
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >
> > v2 -> v3:
> > - Refine comments per Igor's suggestion.
> >
> > v1 -> v2:
> > - change "_" in option names to "-"
> > - check if option value is too large to fit in SMBIOS type 4 speed
> > fields.
> > ---
> >   hw/smbios/smbios.c | 29 ++++++++++++++++++++++++++---
> >   qemu-options.hx    |  3 ++-
> >   2 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index ffd98727ee..4c5992241c 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -94,6 +94,8 @@ static struct {
> >   
> >   static struct {
> >       const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
> > +    uint32_t max_speed;
> > +    uint32_t current_speed;  
> 
> How about defining these two fields as uint16_t, just like "speed" in 
> type17? Then we can also drop the range check against UIN16_MAX.
Well,
someone should check if values provided by user make sense anyway
so check should be there but in some other form.

> 
> Please advise.
> 
> Thanks,
> 
> Heyi
> 
> 
> >   } type4;
> >   
> >   static struct {
> > @@ -272,6 +274,14 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
> >           .name = "version",
> >           .type = QEMU_OPT_STRING,
> >           .help = "version number",
> > +    },{
> > +        .name = "max-speed",
> > +        .type = QEMU_OPT_NUMBER,
> > +        .help = "max speed in MHz",
> > +    },{
> > +        .name = "current-speed",
> > +        .type = QEMU_OPT_NUMBER,
> > +        .help = "speed at system boot in MHz",
> >       },{
> >           .name = "serial",
> >           .type = QEMU_OPT_STRING,
> > @@ -586,9 +596,8 @@ static void smbios_build_type_4_table(MachineState *ms, unsigned instance)
> >       SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
> >       t->voltage = 0;
> >       t->external_clock = cpu_to_le16(0); /* Unknown */
> > -    /* SVVP requires max_speed and current_speed to not be unknown. */
> > -    t->max_speed = cpu_to_le16(2000); /* 2000 MHz */
> > -    t->current_speed = cpu_to_le16(2000); /* 2000 MHz */
> > +    t->max_speed = cpu_to_le16(type4.max_speed);
> > +    t->current_speed = cpu_to_le16(type4.current_speed);
> >       t->status = 0x41; /* Socket populated, CPU enabled */
> >       t->processor_upgrade = 0x01; /* Other */
> >       t->l1_cache_handle = cpu_to_le16(0xFFFF); /* N/A */
> > @@ -1129,6 +1138,20 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> >               save_opt(&type4.serial, opts, "serial");
> >               save_opt(&type4.asset, opts, "asset");
> >               save_opt(&type4.part, opts, "part");
> > +            /*
> > +             * SVVP requires max_speed and current_speed to be set and not being
> > +             * 0 which counts as unknown (SMBIOS 3.1.0/Table 21). Set the
> > +             * default value to 2000MHz as we did before.
> > +             */
> > +            type4.max_speed = qemu_opt_get_number(opts, "max-speed", 2000);
> > +            type4.current_speed = qemu_opt_get_number(opts, "current-speed",
> > +                                                      2000);
> > +            if (type4.max_speed > UINT16_MAX ||
> > +                type4.current_speed > UINT16_MAX) {
> > +                error_setg(errp, "SMBIOS CPU speed is too large (> %d)",
> > +                           UINT16_MAX);
> > +            }
> > +
> >               return;
> >           case 11:
> >               qemu_opts_validate(opts, qemu_smbios_type11_opts, &err);
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index ac315c1ac4..7a2f7c1f66 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -2233,6 +2233,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
> >       "                specify SMBIOS type 3 fields\n"
> >       "-smbios type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
> >       "              [,asset=str][,part=str]\n"
> > +    "              [,max-speed=%d][,current-speed=%d]\n"
> >       "                specify SMBIOS type 4 fields\n"
> >       "-smbios type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str]\n"
> >       "               [,asset=str][,part=str][,speed=%d]\n"
> > @@ -2255,7 +2256,7 @@ Specify SMBIOS type 2 fields
> >   @item -smbios type=3[,manufacturer=@var{str}][,version=@var{str}][,serial=@var{str}][,asset=@var{str}][,sku=@var{str}]
> >   Specify SMBIOS type 3 fields
> >   
> > -@item -smbios type=4[,sock_pfx=@var{str}][,manufacturer=@var{str}][,version=@var{str}][,serial=@var{str}][,asset=@var{str}][,part=@var{str}]
> > +@item -smbios type=4[,sock_pfx=@var{str}][,manufacturer=@var{str}][,version=@var{str}][,serial=@var{str}][,asset=@var{str}][,part=@var{str}][,max-speed=@var{%d}][,current-speed=@var{%d}]
> >   Specify SMBIOS type 4 fields
> >   
> >   @item -smbios type=17[,loc_pfx=@var{str}][,bank=@var{str}][,manufacturer=@var{str}][,serial=@var{str}][,asset=@var{str}][,part=@var{str}][,speed=@var{%d}]  
>