Re: [PATCH] hw/smbios: add options for type 4 max_speed and current_speed

[Heyi Guo <guoheyi@huawei.com>]

Hi Igor,

On 2020/2/28 17:39, Igor Mammedov wrote:
> On Thu, 27 Feb 2020 17:12:21 +0800
> Heyi Guo <guoheyi@huawei.com> wrote:
>
>> On 2020/2/25 17:24, Philippe Mathieu-Daudé wrote:
>>> On 2/25/20 8:50 AM, 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>
>>>>
>>>> ---
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>>    hw/smbios/smbios.c | 22 +++++++++++++++++++---
>>>>    qemu-options.hx    |  3 ++-
>>>>    2 files changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
>>>> index ffd98727ee..1d5439643d 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;
>>>>    } 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",
> I'd suggest to use - instead of _ in option name

Thanks for your comments. However I can see other options like 
"sock_pfx" and "loc_pfx" also use "_" in option names. Should we keep 
consistent with the context?

Thanks,

Heyi


>
>>>> +        .type = QEMU_OPT_NUMBER,
>>>> +        .help = "max speed in MHz",
>>>> +    },{
>>>> +        .name = "current_speed",
> ditto
>
>>>> +        .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,13 @@ 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 not be
>>>> unknown, and
>>>> +             * we 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);
>>> Maybe check speeds are <= UINT16_MAX else set errp?
>> OK; I can do that in the v2. But I would wait for the maintainers to
>> provide more comments :)
>>
>> Thanks,
>>
>> Heyi
>>
>>>   
>>>>                return;
>>>>            case 11:
>>>>                qemu_opts_validate(opts, qemu_smbios_type11_opts, &err);
>>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>>> index ac315c1ac4..bc9ef0fda8 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}]
>>>>   
>>>
>>> .
>
> .