[Qemu-devel] Re: [PATCH 26/35] kvm: Eliminate KVMState arguments

[Anthony Liguori]

On 01/07/2011 03:03 AM, Jan Kiszka wrote:
> Am 06.01.2011 20:24, Anthony Liguori wrote:
>    
> > On 01/06/2011 11:56 AM, Marcelo Tosatti wrote:
> >      
> > > From: Jan Kiszka<address@hidden>
> > >
> > > QEMU supports only one VM, so there is only one kvm_state per process,
> > > and we gain nothing passing a reference to it around. Eliminate any need
> > > to refer to it outside of kvm-all.c.
> > >
> > > Signed-off-by: Jan Kiszka<address@hidden>
> > > CC: Alexander Graf<address@hidden>
> > > Signed-off-by: Marcelo Tosatti<address@hidden>
> > >
> > >        
> > I think this is a big mistake.
> >      
> Obviously, I don't share your concerns. :)
>
>    
> > Having to manage kvm_state keeps the abstraction lines well defined.
> >      
> How does it help?
>
>    
> > Otherwise, it's far too easy for portions of code to call into KVM
> > functions that really shouldn't.
> >      
> I can't imagine we gain anything from requiring kvm_check_extension
> callers to hold a kvm_state "capability". Yes, it's now much easier to
> call kvm_[vm_]ioctl, but that's the key point of this change:
>
> So far we primarily complicated the internal interface between generic
> and arch-dependent kvm parts by requiring kvm_state joggling. But
> external users already find interfaces without this restriction
> (kvm_log_*, kvm_ioeventfd_*, ...). That's because it's at least
> complicated to _cleanly_ pass kvm_state references to all users that
> need it - e.g. sysbus devices like kvmclock or upcoming in-kernel irqchips.
>    

I think you're basically making my point for me.

ioeventfd is a broken interface.  It shouldn't be a VM ioctl but rather 
a VCPU ioctl because PIO events are dispatched on a per-VCPU basis.

kvm_state is available as part of CPU state so it's quite easy to get at 
if these interfaces just took a CPUState argument (and they should).

> Let's just stop this artificial abstraction that has no practical use
> and focus on detecting layering violations via code review. That's more
> reliable IMHO.
>    

Documenting relationships between devices and the CPU is a very 
important task.  Being able to grep for cpu_single_env to see what 
devices models are interacting with the CPU is a very good thing.

When you've got these interactions hidden in a spaghetti of function 
calls, things become impossible to understand.

Regards,

Anthony Liguori

> Jan
>
>