[v1] i386 + x86_64 mttcg

[Qemu-devel] [PATCH 5/6] target/i386/translate: use thread-local storage in !user-mode

Posted by Emilio G. Cota 7 years, 2 months ago

Needed for MTTCG.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 target/i386/translate.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/target/i386/translate.c b/target/i386/translate.c
index 1f9d1d9b24..9a6a72e205 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -71,26 +71,34 @@
 
 //#define MACRO_TEST   1
 
+/* we need thread-local storage for mttcg */
+#ifdef CONFIG_USER_ONLY
+#define I386_THREAD
+#else
+#define I386_THREAD __thread
+#endif
+
 /* global register indexes */
-static TCGv cpu_A0;
-static TCGv cpu_cc_dst, cpu_cc_src, cpu_cc_src2, cpu_cc_srcT;
+static TCGv cpu_cc_dst, cpu_cc_src, cpu_cc_src2;
 static TCGv_i32 cpu_cc_op;
 static TCGv cpu_regs[CPU_NB_REGS];
 static TCGv cpu_seg_base[6];
 static TCGv_i64 cpu_bndl[4];
 static TCGv_i64 cpu_bndu[4];
 /* local temps */
-static TCGv cpu_T0, cpu_T1;
+static I386_THREAD TCGv cpu_cc_srcT;
+static I386_THREAD TCGv cpu_A0;
+static I386_THREAD TCGv cpu_T0, cpu_T1;
 /* local register indexes (only used inside old micro ops) */
-static TCGv cpu_tmp0, cpu_tmp4;
-static TCGv_ptr cpu_ptr0, cpu_ptr1;
-static TCGv_i32 cpu_tmp2_i32, cpu_tmp3_i32;
-static TCGv_i64 cpu_tmp1_i64;
+static I386_THREAD TCGv cpu_tmp0, cpu_tmp4;
+static I386_THREAD TCGv_ptr cpu_ptr0, cpu_ptr1;
+static I386_THREAD TCGv_i32 cpu_tmp2_i32, cpu_tmp3_i32;
+static I386_THREAD TCGv_i64 cpu_tmp1_i64;
 
 #include "exec/gen-icount.h"
 
 #ifdef TARGET_X86_64
-static int x86_64_hregs;
+static I386_THREAD int x86_64_hregs;
 #endif
 
 typedef struct DisasContext {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 5/6] target/i386/translate: use thread-local storage in !user-mode

Posted by Alex Bennée 7 years, 1 month ago

Emilio G. Cota <cota@braap.org> writes:

> Needed for MTTCG.
>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  target/i386/translate.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index 1f9d1d9b24..9a6a72e205 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -71,26 +71,34 @@
>  
>  //#define MACRO_TEST   1
>  
> +/* we need thread-local storage for mttcg */
> +#ifdef CONFIG_USER_ONLY
> +#define I386_THREAD
> +#else
> +#define I386_THREAD __thread
> +#endif
> +

I'm confused - as we can have multi-threaded user space don't the same
requirements apply?

>  /* global register indexes */
> -static TCGv cpu_A0;
> -static TCGv cpu_cc_dst, cpu_cc_src, cpu_cc_src2, cpu_cc_srcT;
> +static TCGv cpu_cc_dst, cpu_cc_src, cpu_cc_src2;
>  static TCGv_i32 cpu_cc_op;
>  static TCGv cpu_regs[CPU_NB_REGS];
>  static TCGv cpu_seg_base[6];
>  static TCGv_i64 cpu_bndl[4];
>  static TCGv_i64 cpu_bndu[4];
>  /* local temps */
> -static TCGv cpu_T0, cpu_T1;
> +static I386_THREAD TCGv cpu_cc_srcT;
> +static I386_THREAD TCGv cpu_A0;
> +static I386_THREAD TCGv cpu_T0, cpu_T1;
>  /* local register indexes (only used inside old micro ops) */
> -static TCGv cpu_tmp0, cpu_tmp4;
> -static TCGv_ptr cpu_ptr0, cpu_ptr1;
> -static TCGv_i32 cpu_tmp2_i32, cpu_tmp3_i32;
> -static TCGv_i64 cpu_tmp1_i64;
> +static I386_THREAD TCGv cpu_tmp0, cpu_tmp4;
> +static I386_THREAD TCGv_ptr cpu_ptr0, cpu_ptr1;
> +static I386_THREAD TCGv_i32 cpu_tmp2_i32, cpu_tmp3_i32;
> +static I386_THREAD TCGv_i64 cpu_tmp1_i64;
>  
>  #include "exec/gen-icount.h"
>  
>  #ifdef TARGET_X86_64
> -static int x86_64_hregs;
> +static I386_THREAD int x86_64_hregs;
>  #endif
>  
>  typedef struct DisasContext {


-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 5/6] target/i386/translate: use thread-local storage in !user-mode

Posted by Emilio G. Cota 7 years, 1 month ago

On Mon, Sep 10, 2018 at 10:17:53 +0100, Alex Bennée wrote:
> 
> Emilio G. Cota <cota@braap.org> writes:
> 
> > Needed for MTTCG.
> >
> > Signed-off-by: Emilio G. Cota <cota@braap.org>
> > ---
> >  target/i386/translate.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/i386/translate.c b/target/i386/translate.c
> > index 1f9d1d9b24..9a6a72e205 100644
> > --- a/target/i386/translate.c
> > +++ b/target/i386/translate.c
> > @@ -71,26 +71,34 @@
> >  
> >  //#define MACRO_TEST   1
> >  
> > +/* we need thread-local storage for mttcg */
> > +#ifdef CONFIG_USER_ONLY
> > +#define I386_THREAD
> > +#else
> > +#define I386_THREAD __thread
> > +#endif
> > +
> 
> I'm confused - as we can have multi-threaded user space don't the same
> requirements apply?

In user-mode, code generation is serialized by mmap_lock.
Making these per-thread would just waste TLS space.

		E.

Re: [Qemu-devel] [PATCH 5/6] target/i386/translate: use thread-local storage in !user-mode

Posted by Paolo Bonzini 7 years, 1 month ago

On 10/09/2018 14:30, Emilio G. Cota wrote:
>> I'm confused - as we can have multi-threaded user space don't the same
>> requirements apply?
> In user-mode, code generation is serialized by mmap_lock.
> Making these per-thread would just waste TLS space.

It's stupid question time!  How can the TLS work?  tcg_x86_init is only
called once, the first time cpu_exec_realizefn is called.

Either they can be kept in non-TLS, or you should move them to DisasContext.

Paolo

Re: [Qemu-devel] [PATCH 5/6] target/i386/translate: use thread-local storage in !user-mode

Posted by Emilio G. Cota 7 years, 1 month ago

On Tue, Sep 11, 2018 at 13:24:03 +0200, Paolo Bonzini wrote:
> On 10/09/2018 14:30, Emilio G. Cota wrote:
> >> I'm confused - as we can have multi-threaded user space don't the same
> >> requirements apply?
> > In user-mode, code generation is serialized by mmap_lock.
> > Making these per-thread would just waste TLS space.
> 
> It's stupid question time!  How can the TLS work?  tcg_x86_init is only
> called once, the first time cpu_exec_realizefn is called.
>
> Either they can be kept in non-TLS, or you should move them to DisasContext.

Yes, the latter is the Right Thing (tm), as both you and Richard
pointed out. I should have done that in the first place.

Will send a v3 with just this change + the configure patch.

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH 5/6] target/i386/translate: use thread-local storage in !user-mode

Posted by Alex Bennée 7 years, 1 month ago

Emilio G. Cota <cota@braap.org> writes:

> On Mon, Sep 10, 2018 at 10:17:53 +0100, Alex Bennée wrote:
>>
>> Emilio G. Cota <cota@braap.org> writes:
>>
>> > Needed for MTTCG.
>> >
>> > Signed-off-by: Emilio G. Cota <cota@braap.org>
>> > ---
>> >  target/i386/translate.c | 24 ++++++++++++++++--------
>> >  1 file changed, 16 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/target/i386/translate.c b/target/i386/translate.c
>> > index 1f9d1d9b24..9a6a72e205 100644
>> > --- a/target/i386/translate.c
>> > +++ b/target/i386/translate.c
>> > @@ -71,26 +71,34 @@
>> >
>> >  //#define MACRO_TEST   1
>> >
>> > +/* we need thread-local storage for mttcg */
>> > +#ifdef CONFIG_USER_ONLY
>> > +#define I386_THREAD
>> > +#else
>> > +#define I386_THREAD __thread
>> > +#endif
>> > +
>>
>> I'm confused - as we can have multi-threaded user space don't the same
>> requirements apply?
>
> In user-mode, code generation is serialized by mmap_lock.
> Making these per-thread would just waste TLS space.

Ahh this is still the case - ok.

Acked-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

[Qemu-devel] [PATCH 1/6] qsp: drop atomics when using the seqlock
[Qemu-devel] [PATCH 2/6] test-rcu-list: avoid torn accesses to n_reclaims and n_nodes_removed
[Qemu-devel] [PATCH 3/6] atomic: fix comment s/x64_64/x86_64/
[Qemu-devel] [PATCH 4/6] cpus: initialize timers_state.vm_clock_lock
[Qemu-devel] [PATCH 5/6] target/i386/translate: use thread-local storage in !user-mode
[Qemu-devel] [PATCH 6/6] configure: enable mttcg for i386 and x86_64