[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [RFC v2 2/3] Add units-per-idebus property |
Date: |
Mon, 22 Sep 2014 09:51:26 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Marcel Apfelbaum <address@hidden> writes:
> On Fri, 2014-09-19 at 11:39 +0200, Markus Armbruster wrote:
>> John Snow <address@hidden> writes:
>>
>> > Signed-off-by: John Snow <address@hidden>
[...]
>> > @@ -1583,6 +1584,7 @@ static void machine_class_init(ObjectClass *oc, void
>> > *data)
>> > mc->hot_add_cpu = qm->hot_add_cpu;
>> > mc->kvm_type = qm->kvm_type;
>> > mc->block_default_type = qm->block_default_type;
>> > + mc->units_per_idebus = qm->units_per_idebus;
>> > mc->max_cpus = qm->max_cpus;
>> > mc->no_serial = qm->no_serial;
>> > mc->no_parallel = qm->no_parallel;
>>
>> Not sufficient! You missed the duplicated code in
>> pc_generic_machine_class_init(). That something was missing was quite
>> obvious in my testing, although what was missing was not. This is the
>> fix I mentioned above.
>>
>> Marcel, you touched both copies recently. Do you know why we need two
>> of them? And why do we copy from QEMUMachine to MachineClass member by
>> member? Why can't we just point from MachineClass to QEMUMachine? Or
>> at least embed the struct so we can copy it wholesale?
> Hi Markus,
>
> I'll try to explain the design:
> 1. MachineClass should not be aware of QEMUMachine. The objective here is to
> eliminate QEMUMachine and it should not be part of MachineClass at any
> cost.
> 2. The plan is like this:
> - The machine types that are not yet QOMified will be QOMified on the fly
> by qemu_register_machine (vl.c) that calls machine_class_init and matches
> QEMUMachine fields with MachineClass fields.
> - This mechanism will be removed when all machines are QOMified. (never?
> :) )
Okay %-)
> - Machines that are QOMified will not reach this code at all, but follow
> the normal QOM path.
> As you can see, by design there is no duplication.
>
> Now let's get to PC machines case:
> - This is a "weird" use case of hybrid QOMifying.
> - All that the author wanted was to have all the PC machines
> derive from type TYPE_PC_MACHINE, but didn't have the time/resources/will
> to go over every PC machine and QOMify it. But he did need the common class.
> - His implementation:
> - He couldn't use the generic qemu_register_machine because the PC machines
> would not have derived from MACHINE_PC_TYPE.
> - So he used the following logic:
> - from the vl.c point of view, the PC machines are QOMified, so the
> PC machines registration *does not reach vl.c".
> - from the PC machines point of view, they remained not QOMified.
> - qemu_register_pc_machine mimics vl.c registration *only for pc machines*
> and has to copy the fields by itself. Plus, it gives the PC machines a
> common
> base class, the thing he was interested in in the first place.
Artifact of this hackery: two identical static functions: vl.c's
machine_class_init() and pc.c's pc_generic_machine_class_init(). Trap
for the unwary, and it caught John.
Unless there are plans to get rid of pc_generic_machine_class_init()
fairly soon, I'd recommend to give machine_class_init() external linkage
with a suitable comment, and drop pc_generic_machine_class_init().
> I hope it helped,
Sure did!
I checked the CodeTransitions Wiki page. It covers this work, and
points to http://wiki.qemu.org/Features/QOM/Machine for more
information. Good.