On Sat, 29 Feb 2020 08:17:48 +0800 Heyi Guo <[email protected]> wrote:
> Hi Igor, > > On 2020/2/28 17:39, Igor Mammedov wrote: > > On Thu, 27 Feb 2020 17:12:21 +0800 > > Heyi Guo <[email protected]> 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 <[email protected]> > >>>> > >>>> --- > >>>> Cc: "Michael S. Tsirkin" <[email protected]> > >>>> Cc: Igor Mammedov <[email protected]> > >>>> --- > >>>> 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}] > >>>> > >>> > >>> . > > > > . > >
