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

[Anthony Liguori]

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.

But we really need to spend some more energy on how we model the device 
tree because it's important to work out the interface that we want to 
represent without going too low.

> > But do we really want to model memory chipsets, a north and south 
> > bridge, and long with cache hierarchies?
>
> We do model devices within the north and south bridges.  The 
> aggregation into two chips is largely meaningless from a functional 
> point of view, as are cache hierarchies.

Right, but it's an issue of how we want devices to interact.

This is the important thing to figure out.  Implementations languages 
don't matter compared to getting the object model right.  If we tried to 
model the device tree based on pin-outs being the interface between 
devices at the lowest level, we're going to fail in any language.

> > If we wanted to add per-device locking based on putting a lock in 
> > DeviceState that was acquired and released whenever you executed a 
> > PIO, how would you do that today?
> >
> > You would convert cpu_register_ioport_* to take a DeviceState in 
> > serial_init_core.  Sure, you could add a layer of indirection in 
> > ISASerialDevice, but what about timers?  We would want to implement 
> > device based timers to do the same thing but again, when we register 
> > the timers we don't have a DeviceState.
> >
> > 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.

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.

Regards,

Anthony Liguori

> Much better to call a traditional callback which then uses 
> container_of() to locate its state.