| Message ID | 1461341333-19646-11-git-send-email-sergey.fedorov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 2016-04-22 19:08, Sergey Fedorov wrote: > From: Sergey Fedorov <serge.fdrv@gmail.com> > > Ensure direct jump patching in MIPS is atomic by using > atomic_read()/atomic_set() for code patching. > > Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> > Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org> > --- > > Changes in v2: > * s/atomic_write/atomic_set/ > > tcg/mips/tcg-target.inc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c > index 682e19897db0..cefc0398018a 100644 > --- a/tcg/mips/tcg-target.inc.c > +++ b/tcg/mips/tcg-target.inc.c > @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s) > void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr) > { > uint32_t *ptr = (uint32_t *)jmp_addr; > - *ptr = deposit32(*ptr, 0, 26, addr >> 2); > + uint32_t insn = atomic_read(ptr); > + atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2)); > flush_icache_range(jmp_addr, jmp_addr + 4); Does it really make sense to read and write the value atomically? The resulting operation is still not atomic, something can happen in between. Aurelien
On 2016-04-22 18:47, Aurelien Jarno wrote: > On 2016-04-22 19:08, Sergey Fedorov wrote: > > From: Sergey Fedorov <serge.fdrv@gmail.com> > > > > Ensure direct jump patching in MIPS is atomic by using > > atomic_read()/atomic_set() for code patching. > > > > Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> > > Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org> > > --- > > > > Changes in v2: > > * s/atomic_write/atomic_set/ > > > > tcg/mips/tcg-target.inc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c > > index 682e19897db0..cefc0398018a 100644 > > --- a/tcg/mips/tcg-target.inc.c > > +++ b/tcg/mips/tcg-target.inc.c > > @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s) > > void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr) > > { > > uint32_t *ptr = (uint32_t *)jmp_addr; > > - *ptr = deposit32(*ptr, 0, 26, addr >> 2); > > + uint32_t insn = atomic_read(ptr); > > + atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2)); > > flush_icache_range(jmp_addr, jmp_addr + 4); > > Does it really make sense to read and write the value atomically? The > resulting operation is still not atomic, something can happen in > between. Hmm, thinking more about that, given the only instruction used is "J", we don't have to read the value, patch it and write it. We can directly use something like (untested): atomic_set(ptr, (0x02 << 26) | (addr >> 2)); Aurelien
On 22/04/16 19:47, Aurelien Jarno wrote: > On 2016-04-22 19:08, Sergey Fedorov wrote: >> From: Sergey Fedorov <serge.fdrv@gmail.com> >> >> Ensure direct jump patching in MIPS is atomic by using >> atomic_read()/atomic_set() for code patching. >> >> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> >> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org> >> --- >> >> Changes in v2: >> * s/atomic_write/atomic_set/ >> >> tcg/mips/tcg-target.inc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c >> index 682e19897db0..cefc0398018a 100644 >> --- a/tcg/mips/tcg-target.inc.c >> +++ b/tcg/mips/tcg-target.inc.c >> @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s) >> void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr) >> { >> uint32_t *ptr = (uint32_t *)jmp_addr; >> - *ptr = deposit32(*ptr, 0, 26, addr >> 2); >> + uint32_t insn = atomic_read(ptr); >> + atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2)); >> flush_icache_range(jmp_addr, jmp_addr + 4); > Does it really make sense to read and write the value atomically? The > resulting operation is still not atomic, something can happen in > between. Actually, it's not important to read it atomically because it's always the target address part of the instruction gets only changed. So whatever can happen in between is overwritten by subsequent deposit32(). Kind regards, Sergey
On 22/04/16 19:51, Aurelien Jarno wrote: > On 2016-04-22 18:47, Aurelien Jarno wrote: >> On 2016-04-22 19:08, Sergey Fedorov wrote: >>> From: Sergey Fedorov <serge.fdrv@gmail.com> >>> >>> Ensure direct jump patching in MIPS is atomic by using >>> atomic_read()/atomic_set() for code patching. >>> >>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> >>> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org> >>> --- >>> >>> Changes in v2: >>> * s/atomic_write/atomic_set/ >>> >>> tcg/mips/tcg-target.inc.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c >>> index 682e19897db0..cefc0398018a 100644 >>> --- a/tcg/mips/tcg-target.inc.c >>> +++ b/tcg/mips/tcg-target.inc.c >>> @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s) >>> void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr) >>> { >>> uint32_t *ptr = (uint32_t *)jmp_addr; >>> - *ptr = deposit32(*ptr, 0, 26, addr >> 2); >>> + uint32_t insn = atomic_read(ptr); >>> + atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2)); >>> flush_icache_range(jmp_addr, jmp_addr + 4); >> Does it really make sense to read and write the value atomically? The >> resulting operation is still not atomic, something can happen in >> between. > Hmm, thinking more about that, given the only instruction used is "J", > we don't have to read the value, patch it and write it. We can directly > use something like (untested): > > atomic_set(ptr, (0x02 << 26) | (addr >> 2)); Hmm, looking at "case INDEX_op_goto_tb:" in tcg_out_op() again I'm thinking about: atomic_set(ptr, deposit32(OPC_J, 0, 26, addr >> 2)); Kind regards, Sergey
On 2016-04-22 20:00, Sergey Fedorov wrote: > On 22/04/16 19:51, Aurelien Jarno wrote: > > On 2016-04-22 18:47, Aurelien Jarno wrote: > >> On 2016-04-22 19:08, Sergey Fedorov wrote: > >>> From: Sergey Fedorov <serge.fdrv@gmail.com> > >>> > >>> Ensure direct jump patching in MIPS is atomic by using > >>> atomic_read()/atomic_set() for code patching. > >>> > >>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com> > >>> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org> > >>> --- > >>> > >>> Changes in v2: > >>> * s/atomic_write/atomic_set/ > >>> > >>> tcg/mips/tcg-target.inc.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c > >>> index 682e19897db0..cefc0398018a 100644 > >>> --- a/tcg/mips/tcg-target.inc.c > >>> +++ b/tcg/mips/tcg-target.inc.c > >>> @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s) > >>> void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr) > >>> { > >>> uint32_t *ptr = (uint32_t *)jmp_addr; > >>> - *ptr = deposit32(*ptr, 0, 26, addr >> 2); > >>> + uint32_t insn = atomic_read(ptr); > >>> + atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2)); > >>> flush_icache_range(jmp_addr, jmp_addr + 4); > >> Does it really make sense to read and write the value atomically? The > >> resulting operation is still not atomic, something can happen in > >> between. > > Hmm, thinking more about that, given the only instruction used is "J", > > we don't have to read the value, patch it and write it. We can directly > > use something like (untested): > > > > atomic_set(ptr, (0x02 << 26) | (addr >> 2)); > > Hmm, looking at "case INDEX_op_goto_tb:" in tcg_out_op() again I'm > thinking about: > > atomic_set(ptr, deposit32(OPC_J, 0, 26, addr >> 2)); Oh right, I forgot when reviewing the patch that this code is actually in tcg-target.inc.c, and that the OPC_J constant is available. Your version looks good to me. Aurelien
diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c index 682e19897db0..cefc0398018a 100644 --- a/tcg/mips/tcg-target.inc.c +++ b/tcg/mips/tcg-target.inc.c @@ -1886,6 +1886,7 @@ static void tcg_target_init(TCGContext *s) void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr) { uint32_t *ptr = (uint32_t *)jmp_addr; - *ptr = deposit32(*ptr, 0, 26, addr >> 2); + uint32_t insn = atomic_read(ptr); + atomic_set(ptr, deposit32(insn, 0, 26, addr >> 2)); flush_icache_range(jmp_addr, jmp_addr + 4); }