From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60912 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PcNuA-0004dd-0V for qemu-devel@nongnu.org; Mon, 10 Jan 2011 14:59:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PcNu6-0000GV-6B for qemu-devel@nongnu.org; Mon, 10 Jan 2011 14:59:49 -0500 Received: from e4.ny.us.ibm.com ([32.97.182.144]:58764) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PcNu6-0000GG-3S for qemu-devel@nongnu.org; Mon, 10 Jan 2011 14:59:46 -0500 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e4.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p0AJfrnb016531 for ; Mon, 10 Jan 2011 14:41:53 -0500 Received: from d01relay07.pok.ibm.com (d01relay07.pok.ibm.com [9.56.227.147]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id D084872805F for ; Mon, 10 Jan 2011 14:59:43 -0500 (EST) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p0AJxhO92371804 for ; Mon, 10 Jan 2011 14:59:43 -0500 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p0AJxgWX007633 for ; Mon, 10 Jan 2011 14:59:42 -0500 Message-ID: <4D2B6506.6070907@linux.vnet.ibm.com> Date: Mon, 10 Jan 2011 13:59:02 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <4D2616D6.4080309@linux.vnet.ibm.com> <4D26D6CF.5070405@web.de> <4D27A16F.9030809@linux.vnet.ibm.com> <4D282489.90506@web.de> In-Reply-To: <4D282489.90506@web.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 26/35] kvm: Eliminate KVMState arguments List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Marcelo Tosatti , qemu-devel@nongnu.org, kvm@vger.kernel.org, Alexander Graf 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 >>>>> >>>>> 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 >>>>> CC: Alexander Graf >>>>> Signed-off-by: Marcelo Tosatti >>>>> >>>>> >>>>> >>>> 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 > >