Re: [Qemu-devel] [PATCH 2/2 (RFC)] QEMU: SMBIOS: Build full smbios tables

[Gerd Hoffmann]

On Mi, 2014-03-05 at 09:48 -0500, Gabriel L. Somlo wrote:
> On Wed, Mar 05, 2014 at 11:59:44AM +0100, Gerd Hoffmann wrote:
> > > -static bool smbios_type1_defaults = true;
> > > +static bool smbios_defaults = true;
> > >  /* Make sure that guest addresses aligned at 1Gbyte boundaries get 
> > > mapped to
> > >   * host addresses aligned at 1Gbyte boundaries.  This way we can use 
> > > 1GByte
> > >   * pages in the host.
> > > @@ -143,9 +143,9 @@ static void pc_init1(QEMUMachineInitArgs *args,
> > >      guest_info->has_pci_info = has_pci_info;
> > >      guest_info->isapc_ram_fw = !pci_enabled;
> > >  
> > > -    if (smbios_type1_defaults) {
> > > +    if (smbios_defaults) {
> > >          /* These values are guest ABI, do not change */
> > > -        smbios_set_type1_defaults("QEMU", "Standard PC (i440FX + PIIX, 
> > > 1996)",
> > > +        smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
> > >                                    args->machine->name);
> > >      }
> > 
> > Need to be careful here.  smbios_type1_defaults handles machine type
> > compatibility.  qemu version 1.7 (and older) don't set any smbios fields
> > by default.  Current qemu behaves simliar with -M pc-i440fx-1.7 (+older)
> > for compatibility reasons, and smbios_type1_defaults controls that.
> 
> I was just reusing the type_1 defaults for more than just
> type_1 tables, so I felt I should rename it to reflect this
> new, more general purpose. It's still setting type_1 defaults
> if/when necessary.

Ok, let me try explain again with some more background info.  Qemu tries
to enable guest visible changes and sometimes new features (depends on
the feature) only in new machine types.  If you install a guest with
(for example) virt-manager, libvirt will store the current machine type
in the guest xml (which is pc-i440fx-1.7 on the stable branch).  When
qemu is upgrated to the upcoming 2.0 release, the guest will continue to
be started with -M pc-i440fx-1.7 and the expectation is that the guest
will not see any changes.

There are two reasons for that:
  * First, we want avoid confusing the guest and triggering unwanted
    actions such as windows wanting be reactivated due to hardware
    changes.
  * Second, for live migration compatibility.  New features might need
    additional state to be stored, so we turn them off for
    compatibility.

You don't need to worry about live migration, but the smbios stuff
clearly goes into the guest-visible change category.

smbios_type1_defaults is one of the compatibility controls.  It is false
for 1.7 (+older) machine types and true otherwise.  So when mucking with
it we need to be careful to not break the compatibility stuff.

So, if we manage to get the patches into shape in time for qemu 2.0 your
way to do that is fine.  We are pretty close to the 2.0 freeze though,
so maybe we should better plan for post-2.0 anyway, especially as you
plan to add more tables.

If the patches get merged after 2.0, for 2.1, then we'll need a separate
variable as we'll have two compatibility modes (for 1.7+older and 2.0)
then.

> > Maybe we should continue to use the individual fields interface for 1.7
> > machine types, for best backward compatibility.
> 
> I'm trying to wrap my head around how I'd distinguish between fields
> on the command line that are to be passed on as individual fields, vs.
> fields that should be used to build a full table...

Assuming the patches get in after 2.0.  Then we'll have some new compat
variable, say smbios_generate_tables (false for 2.0+older, true
otherwise).  And in the code:

   if (smbios_generate_tables) {
       /* generate complete type1 table and pass it on */
   } else {
       /* use existing code to pass individual type1 fields */
   }

The compat stuff is a bit messy at times, but I hope it is more clear
now ...

cheers,
  Gerd