From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH 0/4] KVM: Clean up and optimize __kvm_set_memory_region() - part1 Date: Thu, 17 Jan 2013 19:58:59 +0200 Message-ID: <20130117175859.GC10107@redhat.com> References: <20130111182518.6c5975d9.yoshikawa_takuya_b1@lab.ntt.co.jp> <20130116190757.GA28986@amt.cnet> <50F76E0B.7040605@linux.vnet.ibm.com> <20130117072914.GE11529@redhat.com> <1358442726.9252.5.camel@ul30vt.home> <20130117172037.GB10107@redhat.com> <1358443805.9252.14.camel@ul30vt.home> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Xiao Guangrong , Marcelo Tosatti , Takuya Yoshikawa , kvm@vger.kernel.org To: Alex Williamson Return-path: Received: from mx1.redhat.com ([209.132.183.28]:10782 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751677Ab3AQR7D (ORCPT ); Thu, 17 Jan 2013 12:59:03 -0500 Content-Disposition: inline In-Reply-To: <1358443805.9252.14.camel@ul30vt.home> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Jan 17, 2013 at 10:30:05AM -0700, Alex Williamson wrote: > On Thu, 2013-01-17 at 19:20 +0200, Gleb Natapov wrote: > > On Thu, Jan 17, 2013 at 10:12:06AM -0700, Alex Williamson wrote: > > > On Thu, 2013-01-17 at 09:29 +0200, Gleb Natapov wrote: > > > > On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote: > > > > > On 01/17/2013 03:07 AM, Marcelo Tosatti wrote: > > > > > > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote: > > > > > >> Patches 1 to 3 are trivial. > > > > > >> > > > > > >> Patch 4 is the main cause of the increased lines, but I think the new > > > > > >> code makes it easier to understand why each condition in > > > > > >> __kvm_set_memory_region() is there. > > > > > >> > > > > > >> If you don't agree with patch 4, please consider taking the rest of the > > > > > >> series at this time. > > > > > >> > > > > > >> Takuya Yoshikawa (4): > > > > > >> KVM: set_memory_region: Don't jump to out_free unnecessarily > > > > > >> KVM: set_memory_region: Don't check for overlaps unless we create or move a slot > > > > > >> KVM: set_memory_region: Remove unnecessary variable memslot > > > > > >> KVM: set_memory_region: Identify the requested change explicitly > > > > > >> > > > > > >> virt/kvm/kvm_main.c | 94 ++++++++++++++++++++++++++++++++------------------- > > > > > >> 1 files changed, 59 insertions(+), 35 deletions(-) > > > > > >> > > > > > >> -- > > > > > >> 1.7.5.4 > > > > > > > > > > > > Reviewed-by: Marcelo Tosatti > > > > > > > > > > > > BTW, while at it, its probably worthwhile to restrict flags > > > > > > modifications: change from flags = 0 to flags = read-only is > > > > > > incomplete. Xiao, should it be allowed only during creation? > > > > > > > > > > Will Readonly memory be used for VM-mem-sharing in the future? > > > > > I remember you mentioned about mem-sharing things before. ;) > > > > > > > > > > Actually, It is safe on KVM MMU because all the gfns in the slot > > > > > can become readonly after calling __kvm_set_memory_region. It is > > > > > unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU > > > > > when the flag is changed. > > > > > > > > Which means if we allow changing flags from 0 to read_only or back we > > > > need to call kvm_iommu_map_pages() when flags change. BTW as far as I > > > > see currently IOMMU maps everything as read/write not matter what flags > > > > say. Looks like a bug. Alex? > > > > > > That's correct, legacy device assignment is always r/w. vfio handles > > > this correctly. Thanks, > > > > > Does this mean that device can write into read only memory? > > For legacy assignment, I would expect yes. On bare metal, DMA from an > I/O device can obviously circumvent processor based memory attributes, > but here that might also include things like ROMs too. Thanks, > Why dropping IOMMU_WRITE from flags if slot is read only in kvm_iommu_map_pages() will not work? -- Gleb.