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

[Gleb Natapov <gleb@redhat.com>]

On Sun, Jun 24, 2012 at 04:39:22PM +0300, Avi Kivity wrote:
> On 06/24/2012 04:27 PM, Gleb Natapov wrote:
> > On Sun, Jun 24, 2012 at 04:12:05PM +0300, Avi Kivity wrote:
> >> On 06/12/2012 03:01 PM, Gleb Natapov wrote:
> >> > The function will be used outside of the emulator.
> >> > 
> >> >  /*
> >> >   * x86_emulate_ops:
> >> >   *
> >> > @@ -194,6 +199,10 @@ struct x86_emulate_ops {
> >> >  
> >> >  	bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
> >> >  			 u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
> >> > +
> >> > +	int (*linearize)(struct x86_emulate_ctxt *ctxt,
> >> > +			struct segmented_address addr, unsigned size,
> >> > +			bool write, bool fetch, ulong *linear);
> >> >  };
> >> >  
> >> 
> >> linearize is defined in terms of the other ops; this means that if we
> >> get a second user they will have to replicate it.
> >> 
> > What do you mean? This patch series adds another user, so now there are two: one
> > inside the emulator another is outside.
> 
> I meant like task switching or real-mode interrupt emulation.
> 
You mean code outside of KVM if we ever will make emulator reusable? It will have to
have its own, much more simple version of the callback.

> > 
> >> Why not make the current linearize available to users?
> >>
> > Code outside of the emulator does not call the emulator except when
> > emulation is actually needed. To call linearize() from the emulator.c
> > almost fully functional emulation ctxt will have to be set up (including
> > fake instruction decoding, hacky and slower). 
> 
> ctxt->d use should be removed for the exported version and replaced by a
> parameter.  The internal version can still use it (calling the exported
> version after extracting the parameter).
>
IMO we should stick to the pattern we have now: calling generic code from
the emulator and not vice versa. Lets not create more spaghetti.

>  To not duplicate the logic
> > I moved linearize() to generic code and made it available to emulator
> > via callback. It actually saves a couple of callback invocations when
> > emulator calls linearize() IIRC.
> 
> It's not available to other emulator users (which don't exist yet
> anyway).  But having linearize() in the emulator is consistent with
> placing logic in emulate.c and accessors outside.
> 
It is the question of where we draw the line. For instance MMU details
are now hidden from the emulator behind a callback. One can argue that
emulator should have access to MMU directly via callbacks and
emulate memory access by itself.

> Regarding initialization, we should eventually initialize nothing and
> let the emulator bring in needed data via callbacks (including general
> registers).
> 
Some things will have to be initialized (or rather reset to initial value)
between emulator invocations. Access to registers can be done on demand,
but this is unrelated to this series optimization.

--
			Gleb.