Re: [PATCHv4 3/5] KVM: emulator: move some address manipulation function out of emulator code.

[Gleb Natapov <gleb@redhat.com>]

On Thu, Jul 19, 2012 at 01:42:31PM +0300, Avi Kivity wrote:
> On 07/19/2012 10:40 AM, Gleb Natapov wrote:
> > The functions will be used outside of the emulator.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |   25 +++++++++++++++++++++++++
> >  arch/x86/kvm/emulate.c          |   15 ++-------------
> >  2 files changed, 27 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index de2aff8..6212575 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -980,4 +980,29 @@ int kvm_pmu_read_pmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
> >  void kvm_handle_pmu_event(struct kvm_vcpu *vcpu);
> >  void kvm_deliver_pmi(struct kvm_vcpu *vcpu);
> >  
> > +static inline unsigned long kvm_ad_mask(u8 ad_bytes)
> > +{
> > +	return (1UL << (ad_bytes << 3)) - 1;
> > +}
> > +
> > +/* Access/update address held in a register, based on addressing mode. */
> > +static inline unsigned long
> > +kvm_address_mask(u8 ad_bytes, unsigned long reg)
> > +{
> > +	if (ad_bytes == sizeof(unsigned long))
> > +		return reg;
> > +	else
> > +		return reg & kvm_ad_mask(ad_bytes);
> > +}
> > +
> > +static inline void
> > +kvm_register_address_increment(u8 ad_bytes, unsigned long *reg, int inc)
> > +{
> > +	if (ad_bytes == sizeof(unsigned long))
> > +		*reg += inc;
> > +	else
> > +		*reg = (*reg & ~kvm_ad_mask(ad_bytes)) |
> > +			((*reg + inc) & kvm_ad_mask(ad_bytes));
> > +}
> > +
> >  #endif /* _ASM_X86_KVM_HOST_H */
> > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > index 79899df..e317588 100644
> > --- a/arch/x86/kvm/emulate.c
> > +++ b/arch/x86/kvm/emulate.c
> > @@ -433,19 +433,11 @@ static int emulator_check_intercept(struct x86_emulate_ctxt *ctxt,
> >  	return ctxt->ops->intercept(ctxt, &info, stage);
> >  }
> >  
> > -static inline unsigned long ad_mask(struct x86_emulate_ctxt *ctxt)
> > -{
> > -	return (1UL << (ctxt->ad_bytes << 3)) - 1;
> > -}
> > -
> >  /* Access/update address held in a register, based on addressing mode. */
> >  static inline unsigned long
> >  address_mask(struct x86_emulate_ctxt *ctxt, unsigned long reg)
> >  {
> > -	if (ctxt->ad_bytes == sizeof(unsigned long))
> > -		return reg;
> > -	else
> > -		return reg & ad_mask(ctxt);
> > +	return kvm_address_mask(ctxt->ad_bytes, reg);
> >  }
> >  
> >  static inline unsigned long
> > @@ -457,10 +449,7 @@ register_address(struct x86_emulate_ctxt *ctxt, unsigned long reg)
> >  static inline void
> >  register_address_increment(struct x86_emulate_ctxt *ctxt, unsigned long *reg, int inc)
> >  {
> > -	if (ctxt->ad_bytes == sizeof(unsigned long))
> > -		*reg += inc;
> > -	else
> > -		*reg = (*reg & ~ad_mask(ctxt)) | ((*reg + inc) & ad_mask(ctxt));
> > +	return kvm_register_address_increment(ctxt->ad_bytes, reg, inc);
> >  }
> 
> All those exports suggest it's better to move the fast path into the
> emulator. 

We've already being through that. Putting the code into emulator gives
us nothing unless it also works on emulator context and working on
partially initialized emulator context is first dangerous and second
slower.

>  Suppose we change register access to use callbacks instead of
> bulk load/save, could we reuse the exising code?
> 
I do not see that problem. This helper function do basic arithmetics
on three values. The value itself will be fetched on demand by the emulator.

--
			Gleb.