From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code. Date: Tue, 26 Jun 2012 11:30:41 +0300 Message-ID: <20120626083041.GY6533@redhat.com> References: <20120624142753.GX6533@redhat.com> <4FE86046.6090702@redhat.com> <20120625131253.GA16583@redhat.com> <4FE86A53.2060108@redhat.com> <20120625141750.GA2596@redhat.com> <4FE8767F.1020108@redhat.com> <20120625145501.GB2596@redhat.com> <4FE87DB7.1000905@redhat.com> <20120625153544.GC2596@redhat.com> <4FE888AE.80801@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, mtosatti@redhat.com To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:63079 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758476Ab2FZIan (ORCPT ); Tue, 26 Jun 2012 04:30:43 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q5Q8UgWE031867 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 26 Jun 2012 04:30:43 -0400 Content-Disposition: inline In-Reply-To: <4FE888AE.80801@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Jun 25, 2012 at 06:50:06PM +0300, Avi Kivity wrote: > >> >> Later we can extend x86_decode_insn() and the other > >> >> functions to follow the same rule. > >> >> > >> > What rule? We cannot not initialize a context. You can reduce things > >> > that should be initialized to minimum (getting GP registers on demand, > >> > etc), but still some initialization is needed since ctxt holds emulation > >> > state and it needs to be reset before each emulation. > >> > >> An alternative is to use two contexts, the base context only holds ops > >> and is the parameter to all the callbacks on the non-state APIs, the > >> derived context holds the state: > >> > >> struct x86_emulation_ctxt { > >> struct x86_ops *ops; > >> /* state that always needs to be initialized, preferablt none */ > >> }; > >> > >> struct x86_insn_ctxt { > >> struct x86_emulation_ctxt em; > >> /* instruction state */ > >> } > >> > >> and so we have a compile-time split between users of the state and > >> non-users. > >> > > I do not understand how you will divide current ctxt structure between > > those two. > > > > Where will you put those for instance: interruptibility, have_exception, > > perm_ok, only_vendor_specific_insn and how can they not be initialized > > before each instruction emulation? > > x86_emulate_ops::get_interruptibility() > x86_emulate_ops::set_interruptibility() > x86_emulate_ops::exception() > They do not remove the need for initialization before instruction execution, they just move things that need to be initialized somewhere else (to kvm_arch_vcpu likely). > x86_decode_insn(struct x86_insn_ctxt *ctxt, unsigned flags) > { > ctxt->flags = flags; > ctxt->perm_ok = false; > } > > In short, instruction emulation state is only seen by instruction > emulation functions, the others don't get to see it. > So you want to divide emulator.c to two types of function: those without side effect, that do some kind of calculations on vcpu state according to weird x86 rules, and those that change vcpu state and write it back eventually. I do not see the justification for that complication really. emulator.c is complicated enough already and the line between two may be blurred. If you dislike linearize() callback so much I can make kvm_linearize_address() to do calculation base on its parameters only. It is almost there, only cpl and seg base/desc are missing from parameter list. I can put it into header and x86.c/emulator.c will both be able to use it. -- Gleb.