[Qemu-devel] [PATCH 1/1] mach-virt: Set VM's SMBIOS system version to mc->desc

[Qemu-devel] [PATCH 1/1] mach-virt: Set VM's SMBIOS system version to mc->desc

[Wei Huang]

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

Re: [Qemu-devel] [PATCH 1/1] mach-virt: Set VM's SMBIOS system version to mc->desc

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH 1/1] mach-virt: Set VM's SMBIOS system version to mc->desc

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 1/1] mach-virt: Set VM's SMBIOS system version to mc->desc

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/1] mach-virt: Set VM's SMBIOS system version to mc->desc

[Wei Huang]

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
> 

Re: [Qemu-devel] [PATCH 1/1] mach-virt: Set VM's SMBIOS system version to mc->desc

[Andrew Jones]

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

Re: [Qemu-devel] [PATCH 1/1] mach-virt: Set VM's SMBIOS system version to mc->desc

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|