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

[Avi Kivity <avi@redhat.com>]

  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?

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.

>>> All device callbacks should be based on DeviceState * pointers which 
>>> means if we want to share device code between multiple interfaces 
>>> (be it ISA, PCI, or a SysBus device), we need to have a bus in between.
>>
>> How can you do that?  Do you mean that a timer calls 
>> DeviceState::ops->timer(DeviceState *)?  How do you handle multiple 
>> timers then?
>
>
> No.  We have two types of timers today.  vm_clock based timers and 
> rt_clock based timers.  It's always a bug for a device model to use an 
> rt_clock based timer.  We ought to have a separate API for vm_clock 
> based timers and it makes sense to tie that API to DeviceState.  For 
> instance:
>
> 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.

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

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.