| Message ID | 20180322031216.14569-1-wei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 21, 2018 at 10:12:16PM -0500, Wei Huang wrote: > Instead of using "1.0" as the system version of SMBIOS, we should use > mc->desc for mach-virt machine type. With this patch, "dmidecode -t 1" > (e.g., "-M virt-2.12,accel=kvm") will show: > > Handle 0x0100, DMI type 1, 27 bytes > System Information > Manufacturer: QEMU > Product Name: KVM Virtual Machine > Version: QEMU 2.12 ARM Virtual Machine > Serial Number: Not Specified > ... > > instead of: > Handle 0x0100, DMI type 1, 27 bytes > System Information > Manufacturer: QEMU > Product Name: KVM Virtual Machine > Version: 1.0 > Serial Number: Not Specified > ... > > Signed-off-by: Wei Huang <wei@redhat.com> > --- > hw/arm/virt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 2c07245047..da7228b297 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1132,6 +1132,7 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) > > static void virt_build_smbios(VirtMachineState *vms) > { > + MachineClass *mc = MACHINE_GET_CLASS(vms); > uint8_t *smbios_tables, *smbios_anchor; > size_t smbios_tables_len, smbios_anchor_len; > const char *product = "QEMU Virtual Machine"; > @@ -1145,7 +1146,7 @@ static void virt_build_smbios(VirtMachineState *vms) > } > > smbios_set_defaults("QEMU", product, > - "1.0", false, true, SMBIOS_ENTRY_POINT_30); > + mc->desc, false, true, SMBIOS_ENTRY_POINT_30); > > smbios_get_tables(NULL, 0, &smbios_tables, &smbios_tables_len, > &smbios_anchor, &smbios_anchor_len); > -- > 2.14.3 I agree we should change the useless 1.0, but shouldn't we use mc->name instead of mc->desc? mc->name would make it consistent with pc-piix and q35 and also be a less verbose "version". We'd get e.g. System Information Manufacturer: QEMU Product Name: KVM Virtual Machine Version: virt-2.12 Serial Number: Not Specified Thanks, drew
On 22 March 2018 at 08:01, Andrew Jones <drjones@redhat.com> wrote: > I agree we should change the useless 1.0, but shouldn't we use > mc->name instead of mc->desc? mc->name would make it consistent > with pc-piix and q35 and also be a less verbose "version". We'd > get e.g. > > System Information > Manufacturer: QEMU > Product Name: KVM Virtual Machine > Version: virt-2.12 > Serial Number: Not Specified I would vote for consistency with x86, I think. thanks -- PMM
On Wed, Mar 21, 2018 at 10:12:16PM -0500, Wei Huang wrote: > Instead of using "1.0" as the system version of SMBIOS, we should use > mc->desc for mach-virt machine type. With this patch, "dmidecode -t 1" > (e.g., "-M virt-2.12,accel=kvm") will show: > > Handle 0x0100, DMI type 1, 27 bytes > System Information > Manufacturer: QEMU > Product Name: KVM Virtual Machine > Version: QEMU 2.12 ARM Virtual Machine > Serial Number: Not Specified > ... > > instead of: > Handle 0x0100, DMI type 1, 27 bytes > System Information > Manufacturer: QEMU > Product Name: KVM Virtual Machine > Version: 1.0 > Serial Number: Not Specified > ... > > Signed-off-by: Wei Huang <wei@redhat.com> > --- > hw/arm/virt.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 2c07245047..da7228b297 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1132,6 +1132,7 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) > > static void virt_build_smbios(VirtMachineState *vms) > { > + MachineClass *mc = MACHINE_GET_CLASS(vms); > uint8_t *smbios_tables, *smbios_anchor; > size_t smbios_tables_len, smbios_anchor_len; > const char *product = "QEMU Virtual Machine"; > @@ -1145,7 +1146,7 @@ static void virt_build_smbios(VirtMachineState *vms) > } > > smbios_set_defaults("QEMU", product, > - "1.0", false, true, SMBIOS_ENTRY_POINT_30); > + mc->desc, false, true, SMBIOS_ENTRY_POINT_30); Surely this needs to be handled in back compatible manner so that existing machine types are not changed - only the new virt-2.12 machine type should get new format. Regards, Daniel
On 03/22/2018 05:53 AM, Peter Maydell wrote: > On 22 March 2018 at 08:01, Andrew Jones <drjones@redhat.com> wrote: >> I agree we should change the useless 1.0, but shouldn't we use >> mc->name instead of mc->desc? mc->name would make it consistent >> with pc-piix and q35 and also be a less verbose "version". We'd >> get e.g. >> >> System Information >> Manufacturer: QEMU >> Product Name: KVM Virtual Machine >> Version: virt-2.12 >> Serial Number: Not Specified > > I would vote for consistency with x86, I think. Drew and you like ->name better because of its conciseness consistency with x86. In my view virt-2.xx is not very informative. "Version" info in real machine could be verbose. Using my Thinkpad as example: Manufacturer: LENOVO Product Name: 20FAS1EG0N Version: ThinkPad T460s With that said, I will change it to mc->name. > > thanks > -- PMM >
On Thu, Mar 22, 2018 at 11:07:12AM +0000, Daniel P. Berrangé wrote: > On Wed, Mar 21, 2018 at 10:12:16PM -0500, Wei Huang wrote: > > Instead of using "1.0" as the system version of SMBIOS, we should use > > mc->desc for mach-virt machine type. With this patch, "dmidecode -t 1" > > (e.g., "-M virt-2.12,accel=kvm") will show: > > > > Handle 0x0100, DMI type 1, 27 bytes > > System Information > > Manufacturer: QEMU > > Product Name: KVM Virtual Machine > > Version: QEMU 2.12 ARM Virtual Machine > > Serial Number: Not Specified > > ... > > > > instead of: > > Handle 0x0100, DMI type 1, 27 bytes > > System Information > > Manufacturer: QEMU > > Product Name: KVM Virtual Machine > > Version: 1.0 > > Serial Number: Not Specified > > ... > > > > Signed-off-by: Wei Huang <wei@redhat.com> > > --- > > hw/arm/virt.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index 2c07245047..da7228b297 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1132,6 +1132,7 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) > > > > static void virt_build_smbios(VirtMachineState *vms) > > { > > + MachineClass *mc = MACHINE_GET_CLASS(vms); > > uint8_t *smbios_tables, *smbios_anchor; > > size_t smbios_tables_len, smbios_anchor_len; > > const char *product = "QEMU Virtual Machine"; > > @@ -1145,7 +1146,7 @@ static void virt_build_smbios(VirtMachineState *vms) > > } > > > > smbios_set_defaults("QEMU", product, > > - "1.0", false, true, SMBIOS_ENTRY_POINT_30); > > + mc->desc, false, true, SMBIOS_ENTRY_POINT_30); > > Surely this needs to be handled in back compatible manner so that existing > machine types are not changed - only the new virt-2.12 machine type should > get new format. On real hardware this table can change when there's a firmware update, so I don't think any OS would expect it to stay the same across reboots. I also can't imagine any applications caring at all about this, particularly because it's currently the useless "1.0" string. I think we're safe to neglect compat code in this case, but I may be missing something. Is there a particular issue you're aware of that we'd risk hitting? (Well, besides Windows detecting its installation machine has "changed", invalidating its license or whatever. We don't yet have any Windows guests to worry about.) Thanks, drew
On Thu, Mar 22, 2018 at 02:49:35PM +0100, Andrew Jones wrote: > On Thu, Mar 22, 2018 at 11:07:12AM +0000, Daniel P. Berrangé wrote: > > On Wed, Mar 21, 2018 at 10:12:16PM -0500, Wei Huang wrote: > > > Instead of using "1.0" as the system version of SMBIOS, we should use > > > mc->desc for mach-virt machine type. With this patch, "dmidecode -t 1" > > > (e.g., "-M virt-2.12,accel=kvm") will show: > > > > > > Handle 0x0100, DMI type 1, 27 bytes > > > System Information > > > Manufacturer: QEMU > > > Product Name: KVM Virtual Machine > > > Version: QEMU 2.12 ARM Virtual Machine > > > Serial Number: Not Specified > > > ... > > > > > > instead of: > > > Handle 0x0100, DMI type 1, 27 bytes > > > System Information > > > Manufacturer: QEMU > > > Product Name: KVM Virtual Machine > > > Version: 1.0 > > > Serial Number: Not Specified > > > ... > > > > > > Signed-off-by: Wei Huang <wei@redhat.com> > > > --- > > > hw/arm/virt.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > > index 2c07245047..da7228b297 100644 > > > --- a/hw/arm/virt.c > > > +++ b/hw/arm/virt.c > > > @@ -1132,6 +1132,7 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) > > > > > > static void virt_build_smbios(VirtMachineState *vms) > > > { > > > + MachineClass *mc = MACHINE_GET_CLASS(vms); > > > uint8_t *smbios_tables, *smbios_anchor; > > > size_t smbios_tables_len, smbios_anchor_len; > > > const char *product = "QEMU Virtual Machine"; > > > @@ -1145,7 +1146,7 @@ static void virt_build_smbios(VirtMachineState *vms) > > > } > > > > > > smbios_set_defaults("QEMU", product, > > > - "1.0", false, true, SMBIOS_ENTRY_POINT_30); > > > + mc->desc, false, true, SMBIOS_ENTRY_POINT_30); > > > > Surely this needs to be handled in back compatible manner so that existing > > machine types are not changed - only the new virt-2.12 machine type should > > get new format. > > On real hardware this table can change when there's a firmware update, > so I don't think any OS would expect it to stay the same across reboots. > I also can't imagine any applications caring at all about this, > particularly because it's currently the useless "1.0" string. I think > we're safe to neglect compat code in this case, but I may be missing > something. Is there a particular issue you're aware of that we'd risk > hitting? (Well, besides Windows detecting its installation machine has > "changed", invalidating its license or whatever. We don't yet have any > Windows guests to worry about.) I don't have a particular scenario in mind - just the general rule that we don't change something which is guest visible without tieing it to machine type. I guess we've not tied machine types to specific SeaBIOS / UEFI snapshot builds though, so in terms of firmware changing we've already got that scenario. Regards, Daniel
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 2c07245047..da7228b297 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1132,6 +1132,7 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size) static void virt_build_smbios(VirtMachineState *vms) { + MachineClass *mc = MACHINE_GET_CLASS(vms); uint8_t *smbios_tables, *smbios_anchor; size_t smbios_tables_len, smbios_anchor_len; const char *product = "QEMU Virtual Machine"; @@ -1145,7 +1146,7 @@ static void virt_build_smbios(VirtMachineState *vms) } smbios_set_defaults("QEMU", product, - "1.0", false, true, SMBIOS_ENTRY_POINT_30); + mc->desc, false, true, SMBIOS_ENTRY_POINT_30); smbios_get_tables(NULL, 0, &smbios_tables, &smbios_tables_len, &smbios_anchor, &smbios_anchor_len);
Instead of using "1.0" as the system version of SMBIOS, we should use mc->desc for mach-virt machine type. With this patch, "dmidecode -t 1" (e.g., "-M virt-2.12,accel=kvm") will show: Handle 0x0100, DMI type 1, 27 bytes System Information Manufacturer: QEMU Product Name: KVM Virtual Machine Version: QEMU 2.12 ARM Virtual Machine Serial Number: Not Specified ... instead of: Handle 0x0100, DMI type 1, 27 bytes System Information Manufacturer: QEMU Product Name: KVM Virtual Machine Version: 1.0 Serial Number: Not Specified ... Signed-off-by: Wei Huang <wei@redhat.com> --- hw/arm/virt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)