Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752685AbbEFQZA (ORCPT ); Wed, 6 May 2015 12:25:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41051 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751403AbbEFQYz (ORCPT ); Wed, 6 May 2015 12:24:55 -0400 Date: Wed, 6 May 2015 18:24:41 +0200 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, bsd@redhat.com, guangrong.xiao@linux.intel.com, Yang Zhang , wanpeng.li@linux.intel.com Subject: Re: [PATCH 12/13] KVM: x86: add KVM_MEM_X86_SMRAM memory slot flag Message-ID: <20150506162437.GA27205@potion.brq.redhat.com> References: <1430393772-27208-1-git-send-email-pbonzini@redhat.com> <1430393772-27208-13-git-send-email-pbonzini@redhat.com> <20150505171747.GB17198@potion.brq.redhat.com> <5549E337.1090606@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5549E337.1090606@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5208 Lines: 124 2015-05-06 11:47+0200, Paolo Bonzini: > On 05/05/2015 19:17, Radim Krčmář wrote: >> 2015-04-30 13:36+0200, Paolo Bonzini: >>> struct kvm_memory_slot *x86_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn) >>> { >>> - struct kvm_memory_slot *slot = gfn_to_memslot(vcpu->kvm, gfn); >>> + bool found; >>> + struct kvm_memslots *memslots = kvm_memslots(vcpu->kvm); >>> + struct kvm_memory_slot *slot = search_memslots(memslots, gfn, &found); >>> + >>> + if (found && unlikely(slot->flags & KVM_MEM_X86_SMRAM) && !is_smm(vcpu)) >>> + return NULL; >> >> Patch [10/13] made me sad and IIUIC, the line above is the only reason >> for it ... > > Yes, all the differences trickle down to using x86_gfn_to_memslot. > > On the other hand, there are already cut-and-pasted loops for guest > memory access, see kvm_write_guest_virt_system or > kvm_read_guest_virt_helper. (Yeah ... not introducing new problem is a good first step to fixing the existing one. I can accept that both are okay -- the definition is up to us -- but not that we are adding an abomination on purpose.) > We could add __-prefixed macros like > > #define __kvm_write_guest(fn_page, gpa, data, len, args...) \ > ({ \ > gpa_t _gpa = (gpa); \ > void *_data = (data); \ > int _len = (len); \ > gfn_t _gfn = _gpa >> PAGE_SHIFT; \ > int _offset = offset_in_page(_gpa); \ > int _seg, _ret; \ > while ((_seg = next_segment(_len, _offset)) != 0) { \ > _ret = (fn_page)(args##, _gfn, _data, _offset, _seg); \ > if (_ret < 0) \ > break; \ > _offset = 0; \ > _len -= _seg; \ > _data += _seg; \ > ++_gfn; \ > } \ > _ret; \ > }) > > ... > > int x86_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data, > unsigned long len) > { > return __kvm_write_guest(x86_write_guest_page, gpa, data, len, vcpu); > } > > but frankly it seems worse than the disease. Well, it's a good approach, but the C language makes it awkward. (I like first class functions.) > what about renaming and changing kvm_* memory function to >> vcpu_* and create >> bool kvm_arch_vcpu_can_access_slot(vcpu, slot) >> which could also be inline in arch/*/include/asm/kvm_host.h thanks to >> the way we build. >> We could be passing both kvm and vcpu in internal memslot operations and >> not checking if vcpu is NULL. This should allow all possible operations >> with little code duplication and the compiler could also optimize the >> case where vcpu is NULL. > > That would be a huge patch, and most architectures do not (yet) need it. Not that huge ... trivial extension for passing extra argument around and adding few wrappers to keep compatibility and then a bunch of static inline bool .*(vcpu, slot) { return true; } for remaining arches. (We could have a default unless an arch #defines KVM_ARCH_VCPU_SLOT_CHECKING or some other hack to anger programmers.) The hard part is have the same object code and added flexibility in C. > I can change the functions to kvm_vcpu_read_* and when a second architecture > needs it, we move it from arch/x86/kvm/ to virt/kvm. I named it x86_ just > because it was the same length as kvm_ and thus hardly needed reindentation. That doesn't improve the main issue, so x86 is good. >> Another option is adding something like "vcpu kvm_arch_fake_vcpu(kvm)" >> for cases where the access doesn't have an associated vcpu, so it would >> always succeed. (Might not be generic enough.) > > That's ugly... Yes. (And I still prefer it.) > The question is also how often the copied code is changed, and the answer is > that most of it was never changed since it was introduced in 2007 > (commit 195aefde9cc2, "KVM: Add general accessors to read and write guest > memory"). Before then, KVM used kmap_atomic directly! > > Only the cache code is more recent, but that also has only been changed a > couple of times after introducing it in 2010 (commit 49c7754ce570, "KVM: > Add memory slot versioning and use it to provide fast guest write interface"). > It is very stable code. We have different views on code duplication :) The feature you wanted exposed a flaw in the code, so an extension was needed. Copying code is the last resort after all options of abstracting were exhausted ... I might be forcing common paths when writing it twice requires less brain power, but 200 lines of structurally identical code seem far from it. Reworking stable code is simpler, as we can just cover all features needed now and omit the hard thinking about future extensions. (For me, stable code is the first candidate for generalization ... and I wouldn't copy it, even though it's mostly fine in practice.) It's all nice in theory; I'll prepare a patch we can discuss. (And maybe agree with this one after understanding all challenges.) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/