[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/smbios: add options for type 4 max_speed and current_spee
From: |
Igor Mammedov |
Subject: |
Re: [PATCH] hw/smbios: add options for type 4 max_speed and current_speed |
Date: |
Mon, 2 Mar 2020 09:20:05 +0100 |
On Sat, 29 Feb 2020 08:17:48 +0800
Heyi Guo <address@hidden> wrote:
> Hi Igor,
>
> On 2020/2/28 17:39, Igor Mammedov wrote:
> > On Thu, 27 Feb 2020 17:12:21 +0800
> > Heyi Guo <address@hidden> 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 <address@hidden>
> >>>>
> >>>> ---
> >>>> Cc: "Michael S. Tsirkin" <address@hidden>
> >>>> Cc: Igor Mammedov <address@hidden>
> >>>> ---
> >>>> 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?
For new options one should use '-',
that way we won't have to build wrappers around it if it becomes
a qom property in the future.
>
> 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}]
> >>>>
> >>>
> >>> .
> >
> > .
>
>
- Re: [PATCH] hw/smbios: add options for type 4 max_speed and current_speed,
Igor Mammedov <=