[Qemu-devel] [PATCH for-2.0] cpu: do not use QOM casts in ENV_GET_CPU

[Qemu-devel] [PATCH for-2.0] cpu: do not use QOM casts in ENV_GET_CPU

[Paolo Bonzini]

QOM casts are only typesafe inasmuch as we know that the argument is
a QOM object.  If it is not, the accesses to fields in Object can
access invalid memory and thus cause a segfault.

Using a QOM cast in ENV_GET_CPU is useless and harmful.  Useless,
because the cast is applied to the result of container_of, which is
type safe.  So the QOM cast is nothing but typesafety theater.
Harmful, because ENV_GET_CPU *is* used in hot paths especially
now that, in 2.0, the movement of fields from CPU_COMMON to
CPUState was completed.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Cc: Andreas Faerber <afaerber@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-alpha/cpu-qom.h      | 2 +-
 target-arm/cpu-qom.h        | 2 +-
 target-cris/cpu-qom.h       | 2 +-
 target-i386/cpu-qom.h       | 2 +-
 target-lm32/cpu-qom.h       | 2 +-
 target-m68k/cpu-qom.h       | 2 +-
 target-microblaze/cpu-qom.h | 2 +-
 target-mips/cpu-qom.h       | 2 +-
 target-ppc/cpu-qom.h        | 2 +-
 target-s390x/cpu-qom.h      | 2 +-
 target-sh4/cpu-qom.h        | 2 +-
 target-sparc/cpu-qom.h      | 2 +-
 target-unicore32/cpu-qom.h  | 2 +-
 target-xtensa/cpu-qom.h     | 2 +-
 14 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/target-alpha/cpu-qom.h b/target-alpha/cpu-qom.h
index 198f1b1..47b3130 100644
--- a/target-alpha/cpu-qom.h
+++ b/target-alpha/cpu-qom.h
@@ -70,7 +70,7 @@ static inline AlphaCPU *alpha_env_get_cpu(CPUAlphaState *env)
     return container_of(env, AlphaCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(alpha_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)alpha_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(AlphaCPU, env)
 
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 00234e1..34506b5 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -169,7 +169,7 @@ static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
     return container_of(env, ARMCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(arm_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)arm_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(ARMCPU, env)
 
diff --git a/target-cris/cpu-qom.h b/target-cris/cpu-qom.h
index 7559366..be7a3e4 100644
--- a/target-cris/cpu-qom.h
+++ b/target-cris/cpu-qom.h
@@ -69,7 +69,7 @@ static inline CRISCPU *cris_env_get_cpu(CPUCRISState *env)
     return container_of(env, CRISCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(cris_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)cris_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(CRISCPU, env)
 
diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index e9b3d57..dc68188 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -111,7 +111,7 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
     return container_of(env, X86CPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(x86_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)x86_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(X86CPU, env)
 
diff --git a/target-lm32/cpu-qom.h b/target-lm32/cpu-qom.h
index 9f15093..47b3d31 100644
--- a/target-lm32/cpu-qom.h
+++ b/target-lm32/cpu-qom.h
@@ -73,7 +73,7 @@ static inline LM32CPU *lm32_env_get_cpu(CPULM32State *env)
     return container_of(env, LM32CPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(lm32_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)lm32_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(LM32CPU, env)
 
diff --git a/target-m68k/cpu-qom.h b/target-m68k/cpu-qom.h
index 7f388ed..8410df8 100644
--- a/target-m68k/cpu-qom.h
+++ b/target-m68k/cpu-qom.h
@@ -66,7 +66,7 @@ static inline M68kCPU *m68k_env_get_cpu(CPUM68KState *env)
     return container_of(env, M68kCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(m68k_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)m68k_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(M68kCPU, env)
 
diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
index 35a12b4..0c9988a 100644
--- a/target-microblaze/cpu-qom.h
+++ b/target-microblaze/cpu-qom.h
@@ -67,7 +67,7 @@ static inline MicroBlazeCPU *mb_env_get_cpu(CPUMBState *env)
     return container_of(env, MicroBlazeCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(mb_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)mb_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(MicroBlazeCPU, env)
 
diff --git a/target-mips/cpu-qom.h b/target-mips/cpu-qom.h
index 8877f81..31ed2f2 100644
--- a/target-mips/cpu-qom.h
+++ b/target-mips/cpu-qom.h
@@ -70,7 +70,7 @@ static inline MIPSCPU *mips_env_get_cpu(CPUMIPSState *env)
     return container_of(env, MIPSCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(mips_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)mips_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(MIPSCPU, env)
 
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 47dc8e6..75704fd 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -99,7 +99,7 @@ static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
     return container_of(env, PowerPCCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(ppc_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)ppc_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(PowerPCCPU, env)
 
diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
index ac0460e..0f90d0b 100644
--- a/target-s390x/cpu-qom.h
+++ b/target-s390x/cpu-qom.h
@@ -73,7 +73,7 @@ static inline S390CPU *s390_env_get_cpu(CPUS390XState *env)
     return container_of(env, S390CPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(s390_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)s390_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(S390CPU, env)
 
diff --git a/target-sh4/cpu-qom.h b/target-sh4/cpu-qom.h
index c04e786..4f44b04 100644
--- a/target-sh4/cpu-qom.h
+++ b/target-sh4/cpu-qom.h
@@ -79,7 +79,7 @@ static inline SuperHCPU *sh_env_get_cpu(CPUSH4State *env)
     return container_of(env, SuperHCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(sh_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)sh_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(SuperHCPU, env)
 
diff --git a/target-sparc/cpu-qom.h b/target-sparc/cpu-qom.h
index 8e3e0de..8e4a8b5 100644
--- a/target-sparc/cpu-qom.h
+++ b/target-sparc/cpu-qom.h
@@ -71,7 +71,7 @@ static inline SPARCCPU *sparc_env_get_cpu(CPUSPARCState *env)
     return container_of(env, SPARCCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(sparc_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)sparc_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(SPARCCPU, env)
 
diff --git a/target-unicore32/cpu-qom.h b/target-unicore32/cpu-qom.h
index f727760..a572470 100644
--- a/target-unicore32/cpu-qom.h
+++ b/target-unicore32/cpu-qom.h
@@ -56,7 +56,7 @@ static inline UniCore32CPU *uc32_env_get_cpu(CPUUniCore32State *env)
     return container_of(env, UniCore32CPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(uc32_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)uc32_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(UniCore32CPU, env)
 
diff --git a/target-xtensa/cpu-qom.h b/target-xtensa/cpu-qom.h
index c6cc2d9..b27425e 100644
--- a/target-xtensa/cpu-qom.h
+++ b/target-xtensa/cpu-qom.h
@@ -79,7 +79,7 @@ static inline XtensaCPU *xtensa_env_get_cpu(const CPUXtensaState *env)
     return container_of(env, XtensaCPU, env);
 }
 
-#define ENV_GET_CPU(e) CPU(xtensa_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)xtensa_env_get_cpu(e))
 
 #define ENV_OFFSET offsetof(XtensaCPU, env)
 
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH for-2.0] cpu: do not use QOM casts in ENV_GET_CPU

[Paolo Bonzini]

Il 26/03/2014 14:42, Paolo Bonzini ha scritto:
> QOM casts are only typesafe inasmuch as we know that the argument is
> a QOM object.  If it is not, the accesses to fields in Object can
> access invalid memory and thus cause a segfault.
>
> Using a QOM cast in ENV_GET_CPU is useless and harmful.  Useless,
> because the cast is applied to the result of container_of, which is
> type safe.  So the QOM cast is nothing but typesafety theater.
> Harmful, because ENV_GET_CPU *is* used in hot paths especially
> now that, in 2.0, the movement of fields from CPU_COMMON to
> CPUState was completed.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Cc: Andreas Faerber <afaerber@suse.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Andreas pointed out on IRC that this is just the tip of the iceberg, due 
to code like this:

static void vmsa_ttbcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
{
     ARMCPU *cpu = arm_env_get_cpu(env);

     if (arm_feature(env, ARM_FEATURE_LPAE)) {
         /* With LPAE the TTBCR could result in a change of ASID
          * via the TTBCR.A1 bit, so do a TLB flush.
          */
         tlb_flush(CPU(cpu), 1);
     }
     vmsa_ttbcr_raw_write(env, ri, value);
}

(from target-arm/helper.c).  Given this, and the limited time to fix the 
problem in 2.0 where it got magnified, I think we should reconsider 
whether releases should ahve QOM cast debugging enabled.

Paolo

Re: [Qemu-devel] [PATCH for-2.0] cpu: do not use QOM casts in ENV_GET_CPU

[Laurent Desnogues]

On Wed, Mar 26, 2014 at 2:42 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> QOM casts are only typesafe inasmuch as we know that the argument is
> a QOM object.  If it is not, the accesses to fields in Object can
> access invalid memory and thus cause a segfault.
>
> Using a QOM cast in ENV_GET_CPU is useless and harmful.  Useless,
> because the cast is applied to the result of container_of, which is
> type safe.  So the QOM cast is nothing but typesafety theater.
> Harmful, because ENV_GET_CPU *is* used in hot paths especially
> now that, in 2.0, the movement of fields from CPU_COMMON to
> CPUState was completed.

I tested the ARM part of the patch;  on my test (kernel boot + run of
Sunspider with Google V8), I get the following user time results:

(average of 5 runs):
- before: 17.8s
- with your patch: 17s

configure was run with no option.

Tested-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Cc: Andreas Faerber <afaerber@suse.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-alpha/cpu-qom.h      | 2 +-
>  target-arm/cpu-qom.h        | 2 +-
>  target-cris/cpu-qom.h       | 2 +-
>  target-i386/cpu-qom.h       | 2 +-
>  target-lm32/cpu-qom.h       | 2 +-
>  target-m68k/cpu-qom.h       | 2 +-
>  target-microblaze/cpu-qom.h | 2 +-
>  target-mips/cpu-qom.h       | 2 +-
>  target-ppc/cpu-qom.h        | 2 +-
>  target-s390x/cpu-qom.h      | 2 +-
>  target-sh4/cpu-qom.h        | 2 +-
>  target-sparc/cpu-qom.h      | 2 +-
>  target-unicore32/cpu-qom.h  | 2 +-
>  target-xtensa/cpu-qom.h     | 2 +-
>  14 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/target-alpha/cpu-qom.h b/target-alpha/cpu-qom.h
> index 198f1b1..47b3130 100644
> --- a/target-alpha/cpu-qom.h
> +++ b/target-alpha/cpu-qom.h
> @@ -70,7 +70,7 @@ static inline AlphaCPU *alpha_env_get_cpu(CPUAlphaState *env)
>      return container_of(env, AlphaCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(alpha_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)alpha_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(AlphaCPU, env)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 00234e1..34506b5 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -169,7 +169,7 @@ static inline ARMCPU *arm_env_get_cpu(CPUARMState *env)
>      return container_of(env, ARMCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(arm_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)arm_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(ARMCPU, env)
>
> diff --git a/target-cris/cpu-qom.h b/target-cris/cpu-qom.h
> index 7559366..be7a3e4 100644
> --- a/target-cris/cpu-qom.h
> +++ b/target-cris/cpu-qom.h
> @@ -69,7 +69,7 @@ static inline CRISCPU *cris_env_get_cpu(CPUCRISState *env)
>      return container_of(env, CRISCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(cris_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)cris_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(CRISCPU, env)
>
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index e9b3d57..dc68188 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -111,7 +111,7 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
>      return container_of(env, X86CPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(x86_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)x86_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(X86CPU, env)
>
> diff --git a/target-lm32/cpu-qom.h b/target-lm32/cpu-qom.h
> index 9f15093..47b3d31 100644
> --- a/target-lm32/cpu-qom.h
> +++ b/target-lm32/cpu-qom.h
> @@ -73,7 +73,7 @@ static inline LM32CPU *lm32_env_get_cpu(CPULM32State *env)
>      return container_of(env, LM32CPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(lm32_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)lm32_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(LM32CPU, env)
>
> diff --git a/target-m68k/cpu-qom.h b/target-m68k/cpu-qom.h
> index 7f388ed..8410df8 100644
> --- a/target-m68k/cpu-qom.h
> +++ b/target-m68k/cpu-qom.h
> @@ -66,7 +66,7 @@ static inline M68kCPU *m68k_env_get_cpu(CPUM68KState *env)
>      return container_of(env, M68kCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(m68k_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)m68k_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(M68kCPU, env)
>
> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
> index 35a12b4..0c9988a 100644
> --- a/target-microblaze/cpu-qom.h
> +++ b/target-microblaze/cpu-qom.h
> @@ -67,7 +67,7 @@ static inline MicroBlazeCPU *mb_env_get_cpu(CPUMBState *env)
>      return container_of(env, MicroBlazeCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(mb_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)mb_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(MicroBlazeCPU, env)
>
> diff --git a/target-mips/cpu-qom.h b/target-mips/cpu-qom.h
> index 8877f81..31ed2f2 100644
> --- a/target-mips/cpu-qom.h
> +++ b/target-mips/cpu-qom.h
> @@ -70,7 +70,7 @@ static inline MIPSCPU *mips_env_get_cpu(CPUMIPSState *env)
>      return container_of(env, MIPSCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(mips_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)mips_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(MIPSCPU, env)
>
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 47dc8e6..75704fd 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -99,7 +99,7 @@ static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>      return container_of(env, PowerPCCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(ppc_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)ppc_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(PowerPCCPU, env)
>
> diff --git a/target-s390x/cpu-qom.h b/target-s390x/cpu-qom.h
> index ac0460e..0f90d0b 100644
> --- a/target-s390x/cpu-qom.h
> +++ b/target-s390x/cpu-qom.h
> @@ -73,7 +73,7 @@ static inline S390CPU *s390_env_get_cpu(CPUS390XState *env)
>      return container_of(env, S390CPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(s390_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)s390_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(S390CPU, env)
>
> diff --git a/target-sh4/cpu-qom.h b/target-sh4/cpu-qom.h
> index c04e786..4f44b04 100644
> --- a/target-sh4/cpu-qom.h
> +++ b/target-sh4/cpu-qom.h
> @@ -79,7 +79,7 @@ static inline SuperHCPU *sh_env_get_cpu(CPUSH4State *env)
>      return container_of(env, SuperHCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(sh_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)sh_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(SuperHCPU, env)
>
> diff --git a/target-sparc/cpu-qom.h b/target-sparc/cpu-qom.h
> index 8e3e0de..8e4a8b5 100644
> --- a/target-sparc/cpu-qom.h
> +++ b/target-sparc/cpu-qom.h
> @@ -71,7 +71,7 @@ static inline SPARCCPU *sparc_env_get_cpu(CPUSPARCState *env)
>      return container_of(env, SPARCCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(sparc_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)sparc_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(SPARCCPU, env)
>
> diff --git a/target-unicore32/cpu-qom.h b/target-unicore32/cpu-qom.h
> index f727760..a572470 100644
> --- a/target-unicore32/cpu-qom.h
> +++ b/target-unicore32/cpu-qom.h
> @@ -56,7 +56,7 @@ static inline UniCore32CPU *uc32_env_get_cpu(CPUUniCore32State *env)
>      return container_of(env, UniCore32CPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(uc32_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)uc32_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(UniCore32CPU, env)
>
> diff --git a/target-xtensa/cpu-qom.h b/target-xtensa/cpu-qom.h
> index c6cc2d9..b27425e 100644
> --- a/target-xtensa/cpu-qom.h
> +++ b/target-xtensa/cpu-qom.h
> @@ -79,7 +79,7 @@ static inline XtensaCPU *xtensa_env_get_cpu(const CPUXtensaState *env)
>      return container_of(env, XtensaCPU, env);
>  }
>
> -#define ENV_GET_CPU(e) CPU(xtensa_env_get_cpu(e))
> +#define ENV_GET_CPU(e) ((CPUState *)xtensa_env_get_cpu(e))
>
>  #define ENV_OFFSET offsetof(XtensaCPU, env)
>
> --
> 1.8.5.3
>

Re: [Qemu-devel] [PATCH for-2.0] cpu: do not use QOM casts in ENV_GET_CPU

[Paolo Bonzini]

> (average of 5 runs):
> - before: 17.8s
> - with your patch: 17s

What about 1.7?

Paolo

Re: [Qemu-devel] [PATCH for-2.0] cpu: do not use QOM casts in ENV_GET_CPU

[Laurent Desnogues]

On Wed, Mar 26, 2014 at 2:55 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 26/03/2014 14:42, Paolo Bonzini ha scritto:
>
>> QOM casts are only typesafe inasmuch as we know that the argument is
>> a QOM object.  If it is not, the accesses to fields in Object can
>> access invalid memory and thus cause a segfault.
>>
>> Using a QOM cast in ENV_GET_CPU is useless and harmful.  Useless,
>> because the cast is applied to the result of container_of, which is
>> type safe.  So the QOM cast is nothing but typesafety theater.
>> Harmful, because ENV_GET_CPU *is* used in hot paths especially
>> now that, in 2.0, the movement of fields from CPU_COMMON to
>> CPUState was completed.
>>
>> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
>> Cc: Andreas Faerber <afaerber@suse.de>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
>
> Andreas pointed out on IRC that this is just the tip of the iceberg, due to
> code like this:
>
> static void vmsa_ttbcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
> {
>     ARMCPU *cpu = arm_env_get_cpu(env);
>
>     if (arm_feature(env, ARM_FEATURE_LPAE)) {
>         /* With LPAE the TTBCR could result in a change of ASID
>          * via the TTBCR.A1 bit, so do a TLB flush.
>          */
>         tlb_flush(CPU(cpu), 1);
>     }
>     vmsa_ttbcr_raw_write(env, ri, value);
> }
>
> (from target-arm/helper.c).  Given this, and the limited time to fix the
> problem in 2.0 where it got magnified, I think we should reconsider whether
> releases should ahve QOM cast debugging enabled.

I agree, but I guess your patch is still useful.  Here are results
with/out your patch with/out --disable-qom-cast-debug

main               17.8
main+disable       17.3
main+patch         17.0
main+patch+disable 16.8

Mainline is at 9c5793c5036c7608ff2c87846c6d11f904c08b58.

For QEMU 1.7, I get 16.4s.


Laurent

Re: [Qemu-devel] [PATCH for-2.0] cpu: do not use QOM casts in ENV_GET_CPU

[Andreas Färber]

Am 26.03.2014 14:42, schrieb Paolo Bonzini:
> QOM casts are only typesafe inasmuch as we know that the argument is
> a QOM object.  If it is not, the accesses to fields in Object can
> access invalid memory and thus cause a segfault.
> 
> Using a QOM cast in ENV_GET_CPU is useless and harmful.  Useless,
> because the cast is applied to the result of container_of, which is
> type safe.  So the QOM cast is nothing but typesafety theater.
> Harmful, because ENV_GET_CPU *is* used in hot paths especially
> now that, in 2.0, the movement of fields from CPU_COMMON to
> CPUState was completed.
> 
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Cc: Andreas Faerber <afaerber@suse.de>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-alpha/cpu-qom.h      | 2 +-
>  target-arm/cpu-qom.h        | 2 +-
>  target-cris/cpu-qom.h       | 2 +-
>  target-i386/cpu-qom.h       | 2 +-
>  target-lm32/cpu-qom.h       | 2 +-
>  target-m68k/cpu-qom.h       | 2 +-
>  target-microblaze/cpu-qom.h | 2 +-
>  target-mips/cpu-qom.h       | 2 +-
>  target-ppc/cpu-qom.h        | 2 +-
>  target-s390x/cpu-qom.h      | 2 +-
>  target-sh4/cpu-qom.h        | 2 +-
>  target-sparc/cpu-qom.h      | 2 +-
>  target-unicore32/cpu-qom.h  | 2 +-
>  target-xtensa/cpu-qom.h     | 2 +-
>  14 files changed, 14 insertions(+), 14 deletions(-)

Thanks for the reminder, Laurent. :)

This patch is missing two targets:

diff --git a/target-moxie/cpu.h b/target-moxie/cpu.h
index c5b12a5..667d751 100644
--- a/target-moxie/cpu.h
+++ b/target-moxie/cpu.h
@@ -109,7 +109,7 @@ static inline MoxieCPU
*moxie_env_get_cpu(CPUMoxieState *env
)
     return container_of(env, MoxieCPU, env);
 }

-#define ENV_GET_CPU(e) CPU(moxie_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)moxie_env_get_cpu(e))

 #define ENV_OFFSET offsetof(MoxieCPU, env)

diff --git a/target-openrisc/cpu.h b/target-openrisc/cpu.h
index 4512f45..7d671a8 100644
--- a/target-openrisc/cpu.h
+++ b/target-openrisc/cpu.h
@@ -339,7 +339,7 @@ static inline OpenRISCCPU
*openrisc_env_get_cpu(CPUOpenRISCState *env)
     return container_of(env, OpenRISCCPU, env);
 }

-#define ENV_GET_CPU(e) CPU(openrisc_env_get_cpu(e))
+#define ENV_GET_CPU(e) ((CPUState *)openrisc_env_get_cpu(e))

 #define ENV_OFFSET offsetof(OpenRISCCPU, env)

I do wonder if it wouldn't make more sense to simply
#define CPU(obj) ((CPUState *)obj)
which would catch all uses of CPU() while still allowing to change it to
#define CPU(obj) dynamic_cast<CPUState>(obj)
or so in the future without touching dozens of places. We can obviously
also take this patch here for 2.0 and then revert and redo differently.

Opinions?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-2.0] cpu: do not use QOM casts in ENV_GET_CPU

[Paolo Bonzini]

Il 28/03/2014 15:52, Andreas Färber ha scritto:
> I do wonder if it wouldn't make more sense to simply
> #define CPU(obj) ((CPUState *)obj)
> which would catch all uses of CPU() while still allowing to change it to
> #define CPU(obj) dynamic_cast<CPUState>(obj)
> or so in the future without touching dozens of places. We can obviously
> also take this patch here for 2.0 and then revert and redo differently.

I suggest doing a C-cast patch for 2.0, and then possibly revert and 
redo differently in 2.1.