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

[Anthony Liguori]

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