On Tue, 3 Mar 2020 11:18:56 +0800 Heyi Guo <[email protected]> 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 <[email protected]> > > Reviewed-by: Igor Mammedov <[email protected]> > > > > --- > > Cc: "Michael S. Tsirkin" <[email protected]> > > Cc: Igor Mammedov <[email protected]> > > Cc: Philippe Mathieu-Daudé <[email protected]> > > > > 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}] > > >
