Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.

[Gleb Natapov <gleb@redhat.com>]

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.