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

[Alexander Graf]

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.
> 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)
> 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.
> 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
> Regards.
>
>   

Regards,

Alex