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

[J. Mayer]

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

Regards.

-- 
J. Mayer <address@hidden>
Never organized