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

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jun 25, 2012 at 03:57:42PM +0300, Avi Kivity wrote:
> On 06/24/2012 05:27 PM, Gleb Natapov wrote:
> > 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.
> 
> Right now the all segment related operations are behind the line; the
> line is linear | physical.  Having a ->linearize op will change that.
> 
> > 
> >> 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.
> 
> Right.  But I think we can have x86_linearize() that doesn't take a
> context parameter, only ops.
> 
All ops take context parameter though.

--
			Gleb.