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

[Avi Kivity]

 On 08/23/2010 04:23 PM, Anthony Liguori wrote:
> > > 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.

The pci device has num_gpio_out == num_gpio_in == 0, so it all works 
(trivially).  Again I agree except for the sense of impending doom.  
It's silly but not insane - it assumes most devices have >0 gpio pins, 
which isn't the case for the devices we care about (so it's only 
subjectively silly).

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

Sure, it's a worthy cleanup.  But it's not a reason to go to DEFCON 1.

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

Right.  In any case, I don't see how passing a DeviceState helps.

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

Is a "state reference" allowed to have a pointer to the state, or reach 
it in some other way (for example, static storage for singleton devices)?

Isn't "save/restore works" an equivalent statement to "device state is 
reachable from the DeviceState"?

--
error compiling committee.c: too many arguments to function