From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH] KVM: x86: Avoid zapping mmio sptes twice for generation wraparound Date: Wed, 3 Jul 2013 12:03:27 +0300 Message-ID: <20130703090327.GE18508@redhat.com> References: <20130703171804.89d6cc2c.yoshikawa_takuya_b1@lab.ntt.co.jp> <51D3E093.3020408@redhat.com> <51D3E33D.1090704@linux.vnet.ibm.com> <51D3E5DC.5020902@linux.vnet.ibm.com> <20130703085310.GD18508@redhat.com> <51D3E77F.2060805@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Xiao Guangrong , Takuya Yoshikawa , kvm@vger.kernel.org To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:7770 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754542Ab3GCJDf (ORCPT ); Wed, 3 Jul 2013 05:03:35 -0400 Content-Disposition: inline In-Reply-To: <51D3E77F.2060805@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Jul 03, 2013 at 10:57:35AM +0200, Paolo Bonzini wrote: > Il 03/07/2013 10:53, Gleb Natapov ha scritto: > >>> > > Please wait a while. I can not understand it very clearly. > >>> > > > >>> > > This conditional check will cause caching a overflow value into mmio spte. > >>> > > The simple case is that kvm adds new slots for many times, the mmio-gen is easily > >>> > > more than MMIO_MAX_GEN. > >>> > > > >> > > >> > Actually, the double zapping can be avoided by moving kvm_mmu_invalidate_mmio_sptes to > >> > the end of install_new_memslots(). > >> > > > Exactly. Why should we hide it in obscure functions? > > Because kvm_mmu_invalidate_mmio_sptes is an x86-specific function, and > we already have a good arch API to hook into __kvm_set_memory_region. > >>From this patch it can be seen that the API is not good enough. Generation update API is missing. > Another possible implementation is to cache the last memslot generation, > check it in kvm_arch_prepare/commit_memory_region, and call > kvm_mmu_invalidate_mmio_sptes if it changed. This would avoid the need > to keep "((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE))" in sync > between the two places. But I don't think it is a substantial improvement. > Why are you trying to overcome API shortcomings instead of fixing the API. -- Gleb.