Re: [Qemu-devel] [PATCH] SVM support

[Jocelyn Mayer]

On Mon, 2007-09-17 at 12:02 +0200, Alexander Graf wrote:
> J. Mayer wrote:
> > On Thu, 2007-09-13 at 17:27 +0200, Alexander Graf wrote:
> >   
> >> Thiemo Seufer wrote:
> >>     
> >>> Alexander Graf wrote:
> >>>   
> >>>       
> >>>> Thanks to Michael Peter who tried the patch on an x86 host I found some
> >>>> compilation problems.
> >>>> This updated patch addresses these issues and thus should compile on
> >>>> every platform for every target available.
> >>>>     
> >>>>         
> >
> > [...]
> >   
> >>>   
> >>>       
> >> Wow sorry about that, looks like I missed these.
> >>     
> >
> > Index: qemu-0.9.0.cvs/exec-all.h
> > ===================================================================
> > --- qemu-0.9.0.cvs.orig/exec-all.h
> > +++ qemu-0.9.0.cvs/exec-all.h
> > @@ -166,6 +166,7 @@ static inline int tlb_set_page(CPUState 
> >  typedef struct TranslationBlock {
> >      target_ulong pc;   /* simulated PC corresponding to this block (EIP
> > + CS base) */
> >      target_ulong cs_base; /* CS base for this block */
> > +    uint64_t intercept; /* SVM intercept vector */
> >      unsigned int flags; /* flags defining in which context the code was
> > generated */
> >      uint16_t size;      /* size of target code for this block (1 <=
> >                             size <= TARGET_PAGE_SIZE) */
> > Index: qemu-0.9.0.cvs/cpu-all.h
> > ===================================================================
> > --- qemu-0.9.0.cvs.orig/cpu-all.h
> > +++ qemu-0.9.0.cvs/cpu-all.h
> > @@ -715,6 +715,7 @@ extern int code_copy_enabled;
> >  #define CPU_INTERRUPT_HALT   0x20 /* CPU halt wanted */
> >  #define CPU_INTERRUPT_SMI    0x40 /* (x86 only) SMI interrupt pending
> > */
> >  #define CPU_INTERRUPT_DEBUG  0x80 /* Debug event occured.  */
> > +#define CPU_INTERRUPT_VIRQ   0x100 /* virtual interrupt pending.  */
> >  
> >  void cpu_interrupt(CPUState *s, int mask);
> >  void cpu_reset_interrupt(CPUState *env, int mask);
> >
> > Those two patches look ugly to me as target specific features should
> > never go in generic code or structures.
> > The CPU_INTERRUPT flags should just contain information about the
> > emulator behavior, thus CPU_INTERRUPT_TIMER, CPU_INTERRUPT_SMI are to be
> > removed. Target specific informations about the exception nature should
> > go in the CPUState structure... Then, adding a CPU_INTERRUPT_VIRQ seems
> > not a good idea at all: it's outside of the generic emulator scope and
> > pointless for most targets.
> >   
> I don't see any practical reason not to do it this way. We could define
> a CPU_INTERRUPT_TARGET interrupt, that stores the information in a the
> target specific CPUState, but this would hit performance (marginally
> though) and does not improve the situation. I don't think that we should
> should forcefully try to seperate targets where we are not even close to
> running out of constants. If everyone on this list sees the situation as
> Jocelyn does, I would be fine with writing a patch that puts target
> specific interrupts to the specific targets.

I was to do the same to add some functionality to the PowerPC target,
long time ago, and Fabrice Bellard convinced me not to do and agreed it
has been a bad idea to add the target specific CPU_INTERRUPT_xxx flags.
Then I did manage the PowerPC target to use only generic
CPU_INTERRUPT_xxx and put all other target specific informations in the
CPUState structure. It absolutelly changes nothing in terms of
functionality nor performance. The only thing is that it makes the
generic parts simpler and could even allow the cpu_exec function to
contain almost no target specific code, which would really great imho.

> > For the same reason, the intercept field in the TB structure seems not
> > acceptable, as TB specific target informations are already to be stored
> > in the flags field. As intercept seems only to be a bit field, it should
> > go, in a way or another, in tb flags. And as it seems that some
> >   
> TB flags is a 32-bit bitfield, where several bits are already used.
> Currently SVM supports 45 intercepts and each combination can generate a
> different TB, as we do not want to reduce performance for the
> non-intercepted case. Think of the intercept field as flags2, that could
> be used by other virtualization implementations on other architectures
> too (though I do not know of any)

So, why not make it generic and extend the flag field to as many bits as
you need ? Intercept is x86 specific and has no meaning outside of this
scope. Having more bits for flags is generic and may be useful for some
other targets... The easy way is to convert flag to 64 bits, if you have
enough bits. Another way is to make it a table, but this may need
changes in all targets code...

> > interceptions are related with functions implemented in helpers (not
> > micro-ops), you'd better check the interception in the helper at
> > runtime, which would add no visible overhead (as calling a helper is
> > slow compared to direct micro-ops execution), then you would not need to
> > store those infos in the TB structure. This may even make the emulation
> >   
> This was the case in an earlier version of the patch. The current
> implementation you see is an optimization to make it running faster, as
> there is (nearly) no need for runtime detection.

For all interceptions related to operations in helpers, it's not a
actual optimisation. Things done in helpers are slow and not widely used
(compared to the translated code, I mean), then adding a single test for
interception in a helper may not hurt global performances. You may need
TB flags only for operations you want to intercept that would be inlined
in the translated code, then maybe reduce the number of bits needed
without decreasing performanced. But, not knowing a lot about x86 VM
specification I may be wrong...

> > run faster has you won't fill the TB cache with multiple translation of
> > the same code each time the env->intercept changes, thus have chance to
> > avoid many TB caches flushes.
> >   
> This is actually intended, as I believe the emulated machine should run
> as fast as before when not using any interceptions

It depends if those flags are to be changed often or not, which depends
on the hypervisor software in use I guess. If the flags never change,
there'll be no TB flush, I agree...

Regards.

-- 
Jocelyn Mayer <address@hidden>