On 01/08/2011 02:47 AM, Jan Kiszka wrote:
Am 08.01.2011 00:27, Anthony Liguori wrote:
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<jan.kiszka@xxxxxxxxxxx>
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<jan.kiszka@xxxxxxxxxxx>
CC: Alexander Graf<agraf@xxxxxxx>
Signed-off-by: Marcelo Tosatti<mtosatti@xxxxxxxxxx>
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.
OK, but I don't want to argue about the ioeventfd API. So let's put this
case aside. :)
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).
My point is definitely NOT about cpu-bound devices. That case is clear
and is not touched at all by this patch.
My point is about devices that have clear system scope like kvmclock,
ioapic, pit, pic,
I don't see how ioapic, pit, or pic have a system scope.
I don't know enough about kvmclock.
whatever-the-future-will-bring. And about KVM services
that have global scope like capability checks and other feature
explorations or VM configurations done by the KVM arch code. You still
didn't explain what we gain in these concrete scenarios by handing the
technically redundant abstraction kvm_state around, especially _inside_
the KVM core.
If you have to pass around a KVMState pointer, you establish an explicit
relationship and communication between subsystems. Any place where the
global KVMState is used is a red flag that something is wrong.
I don't see what the advantage to making all of the KVMState global and
implicit. It seems like a big step backwards to me. Can you give a
very concrete example of where you think it results in easier to
understand code as I don't see how making relationships implicit ever
makes code easier to understand?
Regards,
Anthony Liguori
Jan
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
