Re: [Qemu-devel] [RFC 3/8] pc: prepare PC for custom machine state

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Mon, 2014-03-24 at 11:34 +0100, Igor Mammedov wrote:
> On Sun, 23 Mar 2014 17:13:37 +0200
> Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> 
> > On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/pc.c         |   26 ++++++++++++++++++++++++++
> > >  hw/i386/pc_piix.c    |   34 +++++++++++++++++-----------------
> > >  hw/i386/pc_q35.c     |   10 +++++-----
> > >  include/hw/i386/pc.h |   14 ++++++++++++++
> > >  4 files changed, 62 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index e715a33..e0bc3a2 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1413,3 +1413,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> > >          gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
> > >      }
> > >  }
> > > +
> > > +void qemu_register_pc_machine(QEMUMachine *m)
> > I am not so comfortable with this approach because
> > every subsystem (e.g pc) will have to duplicate the
> > "register machine" code until the conversion from
> > QEMUMachine to MachineClass is over. (which I hope
> > it will not take too much time)
> There is no much one can do once leaf type is not directly
> inherited from TYPE_MACHINE. This function would eventually
> go away leaf machine types are converted to static type
> registration.
> 
> > 
> > I propose a patch already in the list which does that
> > automatically by moving this logic into hw/core/machine.c .
> > In this way it will be much less code "touched" during conversion. 
> > 
> > Andreas, did you have anything against the usage of 'class_base_init' ?
> > 
> > The patch is:
> > [PATCH 1/3] hw/machine: move QEMUMachine assignment into the core machine
> > http://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02151.html
> Yep, this allows to drop 2/8 and works as expected.
However Andreas does not like it, because the base class init depends
on the derived class data. Anyway, I hope I'll have a version soon without QemuMachine.

Thanks,
Marcel

> 
> > 
> > > +{
> > > +    TypeInfo ti = {
> > > +        .name       = g_strconcat(m->name, TYPE_MACHINE_SUFFIX, NULL),
> > This leads to a small memory leak, I missed that myself :(
> > Here is a fixed version:
> > [PATCHv2] vl.c: Fix memory leak in qemu_register_machine
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg222800.html
> Thanks
> 
> > 
> > Thanks,
> > Marcel
> > 
> > > +        .parent     = TYPE_PC_MACHINE,
> > > +        .class_init = machine_class_init,
> > > +        .class_data = (void *)m,
> > > +    };
> > > +
> > > +    type_register(&ti);
> > > +}
> > > +
> > > +static const TypeInfo pc_machine_info = {
> > > +    .name = TYPE_PC_MACHINE,
> > > +    .parent = TYPE_MACHINE,
> > > +    .abstract = true,
> > > +    .instance_size = sizeof(PCMachineState),
> > > +};
> > > +
> > > +static void pc_machine_register_types(void)
> > > +{
> > > +    type_register_static(&pc_machine_info);
> > > +}
> > > +
> > > +type_init(pc_machine_register_types)
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index 7930a26..97df43e 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -817,24 +817,24 @@ static QEMUMachine xenfv_machine = {
> > >  
> > >  static void pc_machine_init(void)
> > >  {
> > > -    qemu_register_machine(&pc_i440fx_machine_v2_0);
> > > -    qemu_register_machine(&pc_i440fx_machine_v1_7);
> > > -    qemu_register_machine(&pc_i440fx_machine_v1_6);
> > > -    qemu_register_machine(&pc_i440fx_machine_v1_5);
> > > -    qemu_register_machine(&pc_i440fx_machine_v1_4);
> > > -    qemu_register_machine(&pc_machine_v1_3);
> > > -    qemu_register_machine(&pc_machine_v1_2);
> > > -    qemu_register_machine(&pc_machine_v1_1);
> > > -    qemu_register_machine(&pc_machine_v1_0);
> > > -    qemu_register_machine(&pc_machine_v0_15);
> > > -    qemu_register_machine(&pc_machine_v0_14);
> > > -    qemu_register_machine(&pc_machine_v0_13);
> > > -    qemu_register_machine(&pc_machine_v0_12);
> > > -    qemu_register_machine(&pc_machine_v0_11);
> > > -    qemu_register_machine(&pc_machine_v0_10);
> > > -    qemu_register_machine(&isapc_machine);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v2_0);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_7);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_6);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_5);
> > > +    qemu_register_pc_machine(&pc_i440fx_machine_v1_4);
> > > +    qemu_register_pc_machine(&pc_machine_v1_3);
> > > +    qemu_register_pc_machine(&pc_machine_v1_2);
> > > +    qemu_register_pc_machine(&pc_machine_v1_1);
> > > +    qemu_register_pc_machine(&pc_machine_v1_0);
> > > +    qemu_register_pc_machine(&pc_machine_v0_15);
> > > +    qemu_register_pc_machine(&pc_machine_v0_14);
> > > +    qemu_register_pc_machine(&pc_machine_v0_13);
> > > +    qemu_register_pc_machine(&pc_machine_v0_12);
> > > +    qemu_register_pc_machine(&pc_machine_v0_11);
> > > +    qemu_register_pc_machine(&pc_machine_v0_10);
> > > +    qemu_register_pc_machine(&isapc_machine);
> > >  #ifdef CONFIG_XEN
> > > -    qemu_register_machine(&xenfv_machine);
> > > +    qemu_register_pc_machine(&xenfv_machine);
> > >  #endif
> > >  }
> > >  
> > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > index c844dc2..16b4daa 100644
> > > --- a/hw/i386/pc_q35.c
> > > +++ b/hw/i386/pc_q35.c
> > > @@ -358,11 +358,11 @@ static QEMUMachine pc_q35_machine_v1_4 = {
> > >  
> > >  static void pc_q35_machine_init(void)
> > >  {
> > > -    qemu_register_machine(&pc_q35_machine_v2_0);
> > > -    qemu_register_machine(&pc_q35_machine_v1_7);
> > > -    qemu_register_machine(&pc_q35_machine_v1_6);
> > > -    qemu_register_machine(&pc_q35_machine_v1_5);
> > > -    qemu_register_machine(&pc_q35_machine_v1_4);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v2_0);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v1_7);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v1_6);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v1_5);
> > > +    qemu_register_pc_machine(&pc_q35_machine_v1_4);
> > >  }
> > >  
> > >  machine_init(pc_q35_machine_init);
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 9010246..a01a220 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -12,9 +12,23 @@
> > >  #include "qemu/bitmap.h"
> > >  #include "sysemu/sysemu.h"
> > >  #include "hw/pci/pci.h"
> > > +#include "hw/boards.h"
> > >  
> > >  #define HPET_INTCAP "hpet-intcap"
> > >  
> > > +struct PCMachineState {
> > > +    /*< private >*/
> > > +    MachineState parent_obj;
> > > +};
> > > +
> > > +typedef struct PCMachineState PCMachineState;
> > > +
> > > +#define TYPE_PC_MACHINE "abstract-pc-machine"
> > > +#define PC_MACHINE(obj) \
> > > +     OBJECT_CHECK(PCMachineState, (obj), TYPE_PC_MACHINE)
> > > +
> > > +void qemu_register_pc_machine(QEMUMachine *m);
> > > +
> > >  /* PC-style peripherals (also used by other machines).  */
> > >  
> > >  typedef struct PcPciInfo {
> > 
> > 
> > 
>