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

[Anthony Liguori <anthony@codemonkey.ws>]

On 08/23/2010 12:46 AM, Avi Kivity wrote:
>  On 08/23/2010 12:02 AM, Anthony Liguori wrote:
>> On 08/22/2010 03:28 PM, Avi Kivity wrote:
>>>  On 08/20/2010 09:38 PM, Anthony Liguori wrote:
>>>>> While that might be useful, I don't quite see what makes CPUs so 
>>>>> special
>>>>> that they need to be kept out of qdev.  Could be just my 
>>>>> ignorance, of
>>>>> course.
>>>>
>>>> CPUs have special relationships with things like memory in QEMU.  
>>>> You can argue that a device is anything with pins and that CPUs are 
>>>> just like any other chip. 
>>>
>>> We're not modelling chips!  If we declare something a device, we do 
>>> it because it's functionally a device.  It could be part of a chip, 
>>> or spread along multiple chips.
>>
>> 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.

> 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 :-)

>> 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.

>> 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.

Regards,

Anthony Liguori