Re: [Qemu-devel] [RFC] in-kernel irqchip : split devices

[Anthony Liguori]

Avi Kivity wrote:
> On 10/14/2009 04:30 PM, Glauber Costa wrote:
> > Hello people,
> >
> > As I promised, I am sending a very brief PoC wrt split devices and 
> > in-kernel irqchip.
> > In this mail, I am including only the ioapic version for apreciation. 
> > I also have i8259,
> > and apic will take me a little bit more. This is just to try to bind 
> > the discussion to real
> > code.
> >
> >    
> I still can't say I like it.  The reset function is duplicated, the 
> state representation (which is an ABI) is gratuitously forked.
> You can't save/restore in-kernel irqchip and userspace irqchip, even 
> though where the code is located is an implementation detail.  While 
> we may not care much for the ioapic, it sets a bad precedent for 
> vhost-net, where we'd like to migrate from non-vhost-net hosts to 
> vhost-net hosts without the user noticing anything.
> > Note that we end up with a very slim representation of the device, 
> > and the code is much less
> > confusing, IMHO.
> >    
> You can always remove if statements by duplicating the code and 
> pushing the if one level upwards.  In total, there is more code, and 
> it is more confusing (since you need to deal with implementation 
> details at a higher level).
I'm surprised you feel this way.  Maybe this is an issue of having the 
model in your head vs. not having it because the current in-kernel code 
is extremely confusing IMHO.
When you look at ioapic.c in qemu-kvm, the first question I ask is, 
"what parts of this code is used when using in-kernel apic?".  The 
answer is not at all obvious.
To understand it, you have to first search for kvm_enabled() and you'll 
see that during save/restore the state is synced with the in-kernel 
state.  However, it's not clear whether pio/mmio operations still get 
processed and certainly not clear whether ioapic_set_irq() is not called 
anymore.
In fact, I think you to start with the assumption that it is which leads 
you to wonder why it doesn't do kvm_set_irq().
The answers are all subtle and have to do with weird things about how 
the isa irqs are allocated.  It's extremely confusing to someone who 
doesn't know exactly what's going on.
OTOH, the split model makes this all very obvious.  Sure there's some 
duplication but at the end of the day, you don't have to understand very 
much to see what's going on.  We just use userspace for device 
save/restore and reset support.  Code readability wins in my mind over 
reducing a couple dozen lines of code.
Regards,

Anthony Liguori