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

[Andreas Färber]

Am 24.03.2014 12:20, schrieb Michael S. Tsirkin:
> On Mon, Mar 24, 2014 at 11:52:45AM +0100, Andreas Färber wrote:
>> Am 23.03.2014 16:13, schrieb Marcel Apfelbaum:
>>> On Thu, 2014-03-20 at 16:01 +0100, Igor Mammedov wrote:
>>>> Signed-off-by: Igor Mammedov <address@hidden>
>>>> ---
>>>>  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)
>>>
>>> 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' ?
>>
>> Yes, I do, .class_base_init is wrong for this, it would be .class_init;
>> but please avoid making a base class' .class_init (or .class_base_init)
>> depend on a subtype specifying .class_data.
>>
>> I believe I asked you to post patches that finish your conversion so
>> that MachineClass no longer needs this pointer to QEMUMachine and
>> actually uses the fields you already prepared. In my mind the next
>> logical step of QOM'ification is to have each machine specify what is
>> now in a QEMUMachine struct in its own type's class_init, then there is
>> no duplication of such a general assignment any more.
>>
>> Since this patch is for one machine only, I would much prefer to have
>> the PC duplicate the class_init like we did for sPAPR machine
>> (hw/ppc/spapr.c) over exposing the class_init across files - if this
>> series cannot wait to be ordered after the machine series.
>>
>> Regards,
>> Andreas
> 
> Ok IIUC this is exactly what Igor did so you ack the original patch?

No, it doesn't? It is a nack for 2/8. Nothing wrong with 3/8 in general
though, it can be rebased, given that this seems clearly 2.1 material.
Idea is to minimize changes to code outside of PC to not collide with
the refactoring. If Marcel (re)sends those changes soon, I can queue
them on qom-next and Igor can rebase on that for convenience.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg