From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [PATCH 26/35] kvm: Eliminate KVMState arguments Date: Fri, 07 Jan 2011 17:27:43 -0600 Message-ID: <4D27A16F.9030809@linux.vnet.ibm.com> References: <4D2616D6.4080309@linux.vnet.ibm.com> <4D26D6CF.5070405@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , qemu-devel@nongnu.org, kvm@vger.kernel.org, Alexander Graf To: Jan Kiszka Return-path: Received: from e36.co.us.ibm.com ([32.97.110.154]:46449 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751693Ab1AGX1x (ORCPT ); Fri, 7 Jan 2011 18:27:53 -0500 Received: from d03relay01.boulder.ibm.com (d03relay01.boulder.ibm.com [9.17.195.226]) by e36.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p07NMwtd031734 for ; Fri, 7 Jan 2011 16:22:58 -0700 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay01.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p07NRlF1154056 for ; Fri, 7 Jan 2011 16:27:47 -0700 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p07NRkqH030719 for ; Fri, 7 Jan 2011 16:27:46 -0700 In-Reply-To: <4D26D6CF.5070405@web.de> Sender: kvm-owner@vger.kernel.org List-ID: 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. 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 > > From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51176 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PbLj4-0002vd-Ai for qemu-devel@nongnu.org; Fri, 07 Jan 2011 18:28:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PbLj2-0004YQ-V3 for qemu-devel@nongnu.org; Fri, 07 Jan 2011 18:28:06 -0500 Received: from e31.co.us.ibm.com ([32.97.110.149]:49952) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PbLj2-0004Wc-LE for qemu-devel@nongnu.org; Fri, 07 Jan 2011 18:28:04 -0500 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by e31.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p07NDgIS017677 for ; Fri, 7 Jan 2011 16:13:42 -0700 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p07NRkUr160286 for ; Fri, 7 Jan 2011 16:27:46 -0700 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p07NRkqB030719 for ; Fri, 7 Jan 2011 16:27:46 -0700 Message-ID: <4D27A16F.9030809@linux.vnet.ibm.com> Date: Fri, 07 Jan 2011 17:27:43 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <4D2616D6.4080309@linux.vnet.ibm.com> <4D26D6CF.5070405@web.de> In-Reply-To: <4D26D6CF.5070405@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/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. 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 > >