Re: [Qemu-devel] [PATCH v2 0/7] APIC/IOAPIC cleanup

[Avi Kivity <avi@redhat.com>]

  On 08/23/2010 04:23 PM, Anthony Liguori wrote:
>>> This is really a fundamental discussion.  If you look closely at 
>>> qdev in it's current form, what it actually models is a device with 
>>> GPIO input and output whereas the GPIO input and output correspond 
>>> to qemu_irqs which really model pins that can be raised and lowered.
>>>
>>> To me, this is insane and I'm looking to move the GPIO stuff out of 
>>> qdev.  There are some devices where it makes sense to model the 
>>> interactions between pins but not for the vast majority of devices.
>>
>> I agree, but I don't see the burning need or why it's "insane".  
>> Seems like a minor design issue, can't you just ignore GPIO when you 
>> don't need it?
>
>
> In a sane object model, the expectation is that you can meaningfully 
> interact with base classes using the interfaces provided by the base 
> class.
>
> If DeviceState has a GPIO interface, you should be able to use that 
> interface without knowledge of the subclasses.  This implies that all 
> subclasses implement a GPIO interface and that it can be the primary 
> interface to interact and connect with devices.  Modelling a PCI 
> device based on a GPIO interface is what I was referring to as insane.

The pci device has num_gpio_out == num_gpio_in == 0, so it all works 
(trivially).  Again I agree except for the sense of impending doom.  
It's silly but not insane - it assumes most devices have >0 gpio pins, 
which isn't the case for the devices we care about (so it's only 
subjectively silly).

>> GPIO is just one way for a device to talk, same as 
>> (*bus)_phys_memory_rw() or its netdev or its chardev or its timers.  
>> It doesn't need to have special status within DeviceState, but it 
>> doesn't hurt so much that I can tell.
>
> Everything extra hurts when you're trying to move code in to a library 
> with unit tests covering the functionality :-)

Sure, it's a worthy cleanup.  But it's not a reason to go to DEFCON 1.

>
>>> typedef struct Timer Timer;
>>>
>>> void timer_init(DeviceState *, void (*fn)(Timer *));
>>> void timer_update_rel_ns(Timer *);
>>> void timer_cancel(Timer *);
>>> void timer_release(Timer *);
>>>
>>> Timer objects get embedded into the device's state and container_of 
>>> can be used to get to the original device state.  We could also pass 
>>> DeviceState.  It's not clear to me which is better.
>>
>> Not embedding the DeviceState is more generic.  For example, a device 
>> with a variable number of timers wouldn't be able to embed them in 
>> DeviceState.
>
> Where would they put them?  Everything a device does has to be stored 
> in a DeviceState.  It may put them in a container of some form if the 
> timers are dynamic.

Right.  In any case, I don't see how passing a DeviceState helps.

>>> But being able to associate timers with devices seems like a very 
>>> good idea to me because it means that you can see which devices are 
>>> registering timers.
>>
>> You might also have the timers auto-cancelled and auto-destroyed on 
>> device removal.  But the whole thing seems like a minor coding issue 
>> rather than something fundamental.
>
> The fundamental issue is: every function (minus trivial ones) in the 
> device models code should have a state reference.  That state 
> reference should inherit from a DeviceState.  If this statement isn't 
> true, then the device has been modelled in qdev incorrectly.
>
> Using this test, quite a lot of the "converted" devices are being 
> modelled incorrectly.

Is a "state reference" allowed to have a pointer to the state, or reach 
it in some other way (for example, static storage for singleton devices)?

Isn't "save/restore works" an equivalent statement to "device state is 
reachable from the DeviceState"?

-- 
error compiling committee.c: too many arguments to function