Re: [Qemu-devel] Machine description as data prototype, take 6

[Markus Armbruster]

Paul Brook <address@hidden> writes:

>> * The memory driver is PC-specific.  It should be generic and
>>   data-driven, but getting there isn't quite as easy as it sounds.
>>   Memory (and sometimes even holes) need to be allocated in just the
>>   right order to ensure guest physical address equals host offset for
>>   certain memory ranges.
>
> This is a bug elsewhere and should be fixed there.

Yes, the proper fix is a better guest memory allocation interface.

>> +/*
>> + * Device life cycle:
>> + *
>> + * 1. Configuration: config() method runs after parent's.  It should
>> + * initialize the device's private data from its configuration
>> + * sub-tree.  It may edit the configuration sub-tree, and may declare
>> + * initialization ordering constraints with tree_require_named().
>> + * 2. Initialization: init() method runs after parent's and after that
[...]
>> + * 3. Start: start() method runs, order is unspecified.
>
> Feels like there's at least one too many callbacks here.

I didn't have start() initially.  But then I realized I need to run
i440fx_init() before any PCI device's init(), and
i440fx_init_memory_mappings() after other devices initialized.  If
that's not the case, please tell me what I'm doing wrong.

> The "may edit the configuration sub-tree" also sounds wrong. Devices 
> shouldn't 
> be interacting with the config tree directly, they should just be 
> requesting/exposing features.

No config() method currently edits the tree.  It is, however, safe for
them to do so.  Whether they should is a separate question.

>                               This should also mean we shouldn't need manual 
> dependencies because all device interaction is explicit.

I fear I can't quite follow.  Could you elaborate?

When we extend the tree to cover interrupts, edges describing them could
perhaps replace the manual dependencies.

> Possibly this is a bit confused because you've still got all the device code 
> lumped in the same file. It's hard to identify hacks for PC bits you've not 
> implemented yet,

Such hacks don't exist.  Command line options asking for unimplemented
devices are simply ignored.

>                  machine/device independent code,

That's elsewhere: dt.c.

>                                                   per-device code, and 

First three quarters of hw/pcdt.c, one device after the other.

> hardcoded tree generation in lieu of an actual config file reader.

Twelve lines of code in hw/pcdt.c.

There's also code to edit the tree according to the command line.
Required feature at this stage, in my opinion.  The device-independent
part is in dt.c (section "Dynamic Devices").  A few device-dependent
lines are left in pcdt.c, function dt_customize_config().

>                                                                    Using ugly 
> wrappers round the legacy interface doesn't help,

Nobody likes wrappers.  Yet I feel they are a necessity at this stage.
Before we cam replace existing interfaces wholesale, we need to figure
out what to replace them with.  That's the purpose of this prototype.
Moreover, we have quite a few devices, most of them I can't test.
Making *me* change their interfaces is a recipe for unnecessary breakage
and churn.

Once we've convinced ourselves that the new interface is satisfactory,
we can merge the wrappers into the wrappees.  I don't think we've
reached that point already.

>                                                   especially for PCI devices 
> where we already have an abstraction layer.

If there is an abstraction layer covering initialization of PCI devices,
I must have missed it.  Of the ones I implemented so far, every single
one wants to be initialized in its own idiosyncratic way.