Re: [Qemu-devel] [PATCH for-3.0] pc: Use "3.0+" constant as default SMBIOS version
On Tue, Jul 10, 2018 at 10:07:31AM +0100, Daniel P. Berrangé wrote: > On Mon, Jul 09, 2018 at 05:37:31PM -0300, Eduardo Habkost wrote: > > Every time we create new PC machine-types in QEMU, the defaults > > for SMBIOS fields change unnecessarily because the version field > > defaults to MachineClass::name. > > > > This can cause unexpected side-effects, like triggering license > > reactivation on guest software, or changing the VM memory layout > > because of BIOS table size changes. > > Does that really matter though ? By its very nature the 'Version' > field in SMBIOS is expected to change if you alter something about > the hardware. If guests OS don't want to be exposed to changes in > SMBIOS they would be using a fixed machine type, not the variable > "pc" type that continually changes. > > We could put padding in the string if we want to avoid BIOS table > layout changes. > > Having version change though feels like it is working as intended > for the semantics of these Version: fields in BIOS. Michael, do you have additional info on the original motivation for suggesting this change and why do you consider it a bug? (I don't have any concrete examples to justify the change) -- Eduardo
Re: [Qemu-devel] [PATCH for-3.0] pc: Use "3.0+" constant as default SMBIOS version
On Mon, Jul 09, 2018 at 05:37:31PM -0300, Eduardo Habkost wrote: > Every time we create new PC machine-types in QEMU, the defaults > for SMBIOS fields change unnecessarily because the version field > defaults to MachineClass::name. > > This can cause unexpected side-effects, like triggering license > reactivation on guest software, or changing the VM memory layout > because of BIOS table size changes. Does that really matter though ? By its very nature the 'Version' field in SMBIOS is expected to change if you alter something about the hardware. If guests OS don't want to be exposed to changes in SMBIOS they would be using a fixed machine type, not the variable "pc" type that continually changes. We could put padding in the string if we want to avoid BIOS table layout changes. Having version change though feels like it is working as intended for the semantics of these Version: fields in BIOS. > Change the SMBIOS version string for pc-*-3.0 to "3.0+" to avoid > doing this on every QEMU release, and keep compatible version > strings on older machine-types using a new > MachineClass::smbios_version field. 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 for-3.0] pc: Use "3.0+" constant as default SMBIOS version
On Tue, Jul 10, 2018 at 02:54:12AM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 09, 2018 at 05:37:31PM -0300, Eduardo Habkost wrote:
> > Every time we create new PC machine-types in QEMU, the defaults
> > for SMBIOS fields change unnecessarily because the version field
> > defaults to MachineClass::name.
> >
> > This can cause unexpected side-effects, like triggering license
> > reactivation on guest software, or changing the VM memory layout
> > because of BIOS table size changes.
> >
> > Change the SMBIOS version string for pc-*-3.0 to "3.0+" to avoid
> > doing this on every QEMU release, and keep compatible version
> > strings on older machine-types using a new
> > MachineClass::smbios_version field.
> >
> > Signed-off-by: Eduardo Habkost
> > ---
> > I just noticed that we started using mc->name on arm/virt since
> > commit dfadc3bfb458efefb72e13a57b62f138c464a577.
> > Should arm/virt start using "3.0+" too?
> > ---
> > include/hw/i386/pc.h | 3 +++
> > hw/i386/pc.c | 1 +
> > hw/i386/pc_piix.c| 5 +++--
> > hw/i386/pc_q35.c | 3 ++-
> > 4 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 4d99d69681..aea0fcaadb 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -134,6 +134,9 @@ struct PCMachineClass {
> >
> > /* use DMA capable linuxboot option rom */
> > bool linuxboot_dma_enabled;
> > +
> > +/* Version field for SMBIOS Type 1, Type 2, Type 3, and Type 4 structs
> > */
> > +const char *smbios_version;
> > };
> >
> > #define TYPE_PC_MACHINE "generic-pc-machine"
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 50d5553991..47877e7071 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -2379,6 +2379,7 @@ static void pc_machine_class_init(ObjectClass *oc,
> > void *data)
> > pcmc->acpi_data_size = 0x2 + 0x8000;
> > pcmc->save_tsc_khz = true;
> > pcmc->linuxboot_dma_enabled = true;
> > +pcmc->smbios_version = "3.0+";
> > assert(!mc->get_hotplug_handler);
> > mc->get_hotplug_handler = pc_get_hotpug_handler;
> > mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
>
> I suspect 3.00 is cleaner for tools that happen to
> parse the version as a numeral as it always was in the past,
> even if it's not exact.
It was never a numeral. It was "pc-q35-X.Y" or "pc-i440fx-X.Y".
--
Eduardo
Re: [Qemu-devel] [PATCH for-3.0] pc: Use "3.0+" constant as default SMBIOS version
On Mon, Jul 09, 2018 at 05:37:31PM -0300, Eduardo Habkost wrote:
> Every time we create new PC machine-types in QEMU, the defaults
> for SMBIOS fields change unnecessarily because the version field
> defaults to MachineClass::name.
>
> This can cause unexpected side-effects, like triggering license
> reactivation on guest software, or changing the VM memory layout
> because of BIOS table size changes.
>
> Change the SMBIOS version string for pc-*-3.0 to "3.0+" to avoid
> doing this on every QEMU release, and keep compatible version
> strings on older machine-types using a new
> MachineClass::smbios_version field.
>
> Signed-off-by: Eduardo Habkost
> ---
> I just noticed that we started using mc->name on arm/virt since
> commit dfadc3bfb458efefb72e13a57b62f138c464a577.
> Should arm/virt start using "3.0+" too?
> ---
> include/hw/i386/pc.h | 3 +++
> hw/i386/pc.c | 1 +
> hw/i386/pc_piix.c| 5 +++--
> hw/i386/pc_q35.c | 3 ++-
> 4 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 4d99d69681..aea0fcaadb 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -134,6 +134,9 @@ struct PCMachineClass {
>
> /* use DMA capable linuxboot option rom */
> bool linuxboot_dma_enabled;
> +
> +/* Version field for SMBIOS Type 1, Type 2, Type 3, and Type 4 structs */
> +const char *smbios_version;
> };
>
> #define TYPE_PC_MACHINE "generic-pc-machine"
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 50d5553991..47877e7071 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2379,6 +2379,7 @@ static void pc_machine_class_init(ObjectClass *oc, void
> *data)
> pcmc->acpi_data_size = 0x2 + 0x8000;
> pcmc->save_tsc_khz = true;
> pcmc->linuxboot_dma_enabled = true;
> +pcmc->smbios_version = "3.0+";
> assert(!mc->get_hotplug_handler);
> mc->get_hotplug_handler = pc_get_hotpug_handler;
> mc->cpu_index_to_instance_props = pc_cpu_index_to_props;
I suspect 3.00 is cleaner for tools that happen to
parse the version as a numeral as it always was in the past,
even if it's not exact.
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index dc09466b3e..f19c8b82d0 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -168,10 +168,9 @@ static void pc_init1(MachineState *machine,
> pc_guest_info_init(pcms);
>
> if (pcmc->smbios_defaults) {
> -MachineClass *mc = MACHINE_GET_CLASS(machine);
> /* These values are guest ABI, do not change */
> smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
> -mc->name, pcmc->smbios_legacy_mode,
> +pcmc->smbios_version, pcmc->smbios_legacy_mode,
> pcmc->smbios_uuid_encoded,
> SMBIOS_ENTRY_POINT_21);
> }
> @@ -440,9 +439,11 @@ DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL,
>
> static void pc_i440fx_2_12_machine_options(MachineClass *m)
> {
> +PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> pc_i440fx_3_0_machine_options(m);
> m->is_default = 0;
> m->alias = NULL;
> +pcmc->smbios_version = m->name;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_12);
> }
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 532241e3f8..d6551ed8e9 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -146,7 +146,7 @@ static void pc_q35_init(MachineState *machine)
> if (pcmc->smbios_defaults) {
> /* These values are guest ABI, do not change */
> smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
> -mc->name, pcmc->smbios_legacy_mode,
> +pcmc->smbios_version, pcmc->smbios_legacy_mode,
> pcmc->smbios_uuid_encoded,
> SMBIOS_ENTRY_POINT_21);
> }
> @@ -336,6 +336,7 @@ static void pc_q35_2_11_machine_options(MachineClass *m)
>
> pc_q35_2_12_machine_options(m);
> pcmc->default_nic_model = "e1000";
> +pcmc->smbios_version = m->name;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_11);
> }
>
> --
> 2.18.0.rc1.1.g3f1ff2140
