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

[Avi Kivity]

 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.