From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 12/13] KVM: x86: add KVM_MEM_X86_SMRAM memory slot flag Date: Wed, 6 May 2015 18:24:41 +0200 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-Transfer-Encoding: QUOTED-PRINTABLE 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 To: Paolo Bonzini Return-path: Content-Disposition: inline In-Reply-To: <5549E337.1090606@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2015-05-06 11:47+0200, Paolo Bonzini: > On 05/05/2015 19:17, Radim Kr=C4=8Dm=C3=A1=C5=99 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 =3D gfn_to_memslot(vcpu->kvm, gfn); >>> + bool found; >>> + struct kvm_memslots *memslots =3D kvm_memslots(vcpu->kvm); >>> + struct kvm_memory_slot *slot =3D search_memslots(memslots, gfn, &= found); >>> + >>> + if (found && unlikely(slot->flags & KVM_MEM_X86_SMRAM) && !is_smm= (vcpu)) >>> + return NULL; >>=20 >> Patch [10/13] made me sad and IIUIC, the line above is the only reas= on >> for it ... >=20 > Yes, all the differences trickle down to using x86_gfn_to_memslot. >=20 > On the other hand, there are already cut-and-pasted loops for guest=20 > memory access, see kvm_write_guest_virt_system or=20 > kvm_read_guest_virt_helper. (Yeah ... not introducing new problem is a good first step to fixing th= e 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 >=20 > #define __kvm_write_guest(fn_page, gpa, data, len, args...) \ > ({ \ > gpa_t _gpa =3D (gpa); \ > void *_data =3D (data); \ > int _len =3D (len); \ > gfn_t _gfn =3D _gpa >> PAGE_SHIFT; \ > int _offset =3D offset_in_page(_gpa); \ > int _seg, _ret; \ > while ((_seg =3D next_segment(_len, _offset)) !=3D 0) { \ > _ret =3D (fn_page)(args##, _gfn, _data, _offset, _se= g); \ > if (_ret < 0) \ > break; \ > _offset =3D 0; \ > _len -=3D _seg; \ > _data +=3D _seg; \ > ++_gfn; \ > } \ > _ret; \ > }) >=20 > ... >=20 > int x86_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *dat= a, > unsigned long len) > { > return __kvm_write_guest(x86_write_guest_page, gpa, data, len, vcpu)= ; > } >=20 > 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=20 >> bool kvm_arch_vcpu_can_access_slot(vcpu, slot) >> which could also be inline in arch/*/include/asm/kvm_host.h thanks t= o >> 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 operat= ions >> with little code duplication and the compiler could also optimize th= e >> case where vcpu is NULL. >=20 > 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 archi= tecture > 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 reinden= tation. 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 wo= uld >> always succeed. (Might not be generic enough.) >=20 > That's ugly... Yes. (And I still prefer it.) > The question is also how often the copied code is changed, and the an= swer is > that most of it was never changed since it was introduced in 2007 > (commit 195aefde9cc2, "KVM: Add general accessors to read and write g= uest > memory"). Before then, KVM used kmap_atomic directly! >=20 > Only the cache code is more recent, but that also has only been chang= ed a > couple of times after introducing it in 2010 (commit 49c7754ce570, "K= VM: > Add memory slot versioning and use it to provide fast guest write int= erface"). > 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.)