| Message ID | 20200303010158.52994-1-guoheyi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] hw/smbios: add options for type 4 max-speed and current-speed | expand |
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. 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}]
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}] >
On 2020/3/3 16:33, Igor Mammedov wrote: > 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. Shall we define the temporal variables as uint64_t, and check the values no larger than UINT16_MAX after invoking qemu_opt_get_number()? So any value larger than uint64_max will be blocked by qemu option parser, and value in (uint16_max, uint64_max] will be blocked by ourselves. Thanks, Heyi > >> 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}] > > .
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; } 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}]