| Message ID | 20250318213209.2579218-12-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg, codebase: Build once patches | expand |
On Wed, Mar 19, 2025 at 7:36 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > Perform aligned atomic reads in translator_ld, if possible. > According to > > https://lore.kernel.org/qemu-devel/20240607101403.1109-1-jim.shu@sifive.com/ > > this is required for RISC-V Ziccif. Thanks Richard!! > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > accel/tcg/translator.c | 42 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 38 insertions(+), 4 deletions(-) > > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > index ef1538b4fc..157be33bf6 100644 > --- a/accel/tcg/translator.c > +++ b/accel/tcg/translator.c > @@ -265,12 +265,14 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, > > if (likely(((base ^ last) & TARGET_PAGE_MASK) == 0)) { > /* Entire read is from the first page. */ > - memcpy(dest, host + (pc - base), len); > - return true; > + goto do_read; > } > > if (unlikely(((base ^ pc) & TARGET_PAGE_MASK) == 0)) { > - /* Read begins on the first page and extends to the second. */ > + /* > + * Read begins on the first page and extends to the second. > + * The unaligned read is never atomic. > + */ > size_t len0 = -(pc | TARGET_PAGE_MASK); > memcpy(dest, host + (pc - base), len0); > pc += len0; > @@ -329,7 +331,39 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, > host = db->host_addr[1]; > } > > - memcpy(dest, host + (pc - base), len); > + do_read: > + /* > + * Assume aligned reads should be atomic, if possible. > + * We're not in a position to jump out with EXCP_ATOMIC. > + */ > + host += pc - base; > + switch (len) { > + case 2: > + if (QEMU_IS_ALIGNED(pc, 2)) { > + uint16_t t = qatomic_read((uint16_t *)host); > + stw_he_p(dest, t); > + return true; > + } > + break; > + case 4: > + if (QEMU_IS_ALIGNED(pc, 4)) { > + uint32_t t = qatomic_read((uint32_t *)host); > + stl_he_p(dest, t); > + return true; > + } > + break; > +#ifdef CONFIG_ATOMIC64 > + case 8: > + if (QEMU_IS_ALIGNED(pc, 8)) { > + uint64_t t = qatomic_read__nocheck((uint64_t *)host); > + stq_he_p(dest, t); > + return true; > + } > + break; > +#endif > + } > + /* Unaligned or partial read from the second page is not atomic. */ > + memcpy(dest, host, len); > return true; > } > > -- > 2.43.0 > >
On 3/18/25 14:31, Richard Henderson wrote: > Perform aligned atomic reads in translator_ld, if possible. > According to > > https://lore.kernel.org/qemu-devel/20240607101403.1109-1-jim.shu@sifive.com/ > > this is required for RISC-V Ziccif. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/translator.c | 42 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 38 insertions(+), 4 deletions(-) > > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > index ef1538b4fc..157be33bf6 100644 > --- a/accel/tcg/translator.c > +++ b/accel/tcg/translator.c > @@ -265,12 +265,14 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, > > if (likely(((base ^ last) & TARGET_PAGE_MASK) == 0)) { > /* Entire read is from the first page. */ > - memcpy(dest, host + (pc - base), len); > - return true; > + goto do_read; > } > > if (unlikely(((base ^ pc) & TARGET_PAGE_MASK) == 0)) { > - /* Read begins on the first page and extends to the second. */ > + /* > + * Read begins on the first page and extends to the second. > + * The unaligned read is never atomic. > + */ > size_t len0 = -(pc | TARGET_PAGE_MASK); > memcpy(dest, host + (pc - base), len0); > pc += len0; > @@ -329,7 +331,39 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, > host = db->host_addr[1]; > } > > - memcpy(dest, host + (pc - base), len); > + do_read: > + /* > + * Assume aligned reads should be atomic, if possible. > + * We're not in a position to jump out with EXCP_ATOMIC. > + */ > + host += pc - base; > + switch (len) { Should we have a case for: case 1: uint8_t t = *(uint8_t *)host; stb_he_p(dest, t); return true; To skip the memcpy for a single byte? > + case 2: > + if (QEMU_IS_ALIGNED(pc, 2)) { > + uint16_t t = qatomic_read((uint16_t *)host); > + stw_he_p(dest, t); > + return true; > + } > + break; > + case 4: > + if (QEMU_IS_ALIGNED(pc, 4)) { > + uint32_t t = qatomic_read((uint32_t *)host); > + stl_he_p(dest, t); > + return true; > + } > + break; > +#ifdef CONFIG_ATOMIC64 > + case 8: > + if (QEMU_IS_ALIGNED(pc, 8)) { > + uint64_t t = qatomic_read__nocheck((uint64_t *)host); > + stq_he_p(dest, t); > + return true; > + } > + break; > +#endif > + } > + /* Unaligned or partial read from the second page is not atomic. */ > + memcpy(dest, host, len); > return true; > } >
On 3/18/25 17:15, Pierrick Bouvier wrote: >> @@ -329,7 +331,39 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, >> host = db->host_addr[1]; >> } >> - memcpy(dest, host + (pc - base), len); >> + do_read: >> + /* >> + * Assume aligned reads should be atomic, if possible. >> + * We're not in a position to jump out with EXCP_ATOMIC. >> + */ >> + host += pc - base; >> + switch (len) { > > Should we have a case for: > case 1: > uint8_t t = *(uint8_t *)host; > stb_he_p(dest, t); > return true; > > To skip the memcpy for a single byte? I guess only the i386 translator is going to be issuing byte code loads. I wonder if it's measurable? My gut reaction is that it isn't. r~
On 3/18/25 18:28, Richard Henderson wrote: > On 3/18/25 17:15, Pierrick Bouvier wrote: >>> @@ -329,7 +331,39 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, >>> host = db->host_addr[1]; >>> } >>> - memcpy(dest, host + (pc - base), len); >>> + do_read: >>> + /* >>> + * Assume aligned reads should be atomic, if possible. >>> + * We're not in a position to jump out with EXCP_ATOMIC. >>> + */ >>> + host += pc - base; >>> + switch (len) { >> >> Should we have a case for: >> case 1: >> uint8_t t = *(uint8_t *)host; >> stb_he_p(dest, t); >> return true; >> >> To skip the memcpy for a single byte? > > I guess only the i386 translator is going to be issuing byte code loads. I wonder if it's > measurable? My gut reaction is that it isn't. > If we see a memcpy at this spot in a profiling one day, it will always be time to revisit. For now, it's ok for me. Just wonder why the case for len == 1 was not explicitely added. > > r~
On 3/18/25 14:31, Richard Henderson wrote: > Perform aligned atomic reads in translator_ld, if possible. > According to > > https://lore.kernel.org/qemu-devel/20240607101403.1109-1-jim.shu@sifive.com/ > > this is required for RISC-V Ziccif. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/translator.c | 42 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 38 insertions(+), 4 deletions(-) > > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > index ef1538b4fc..157be33bf6 100644 > --- a/accel/tcg/translator.c > +++ b/accel/tcg/translator.c > @@ -265,12 +265,14 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, > > if (likely(((base ^ last) & TARGET_PAGE_MASK) == 0)) { > /* Entire read is from the first page. */ > - memcpy(dest, host + (pc - base), len); > - return true; > + goto do_read; > } > > if (unlikely(((base ^ pc) & TARGET_PAGE_MASK) == 0)) { > - /* Read begins on the first page and extends to the second. */ > + /* > + * Read begins on the first page and extends to the second. > + * The unaligned read is never atomic. > + */ > size_t len0 = -(pc | TARGET_PAGE_MASK); > memcpy(dest, host + (pc - base), len0); > pc += len0; > @@ -329,7 +331,39 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, > host = db->host_addr[1]; > } > > - memcpy(dest, host + (pc - base), len); > + do_read: > + /* > + * Assume aligned reads should be atomic, if possible. > + * We're not in a position to jump out with EXCP_ATOMIC. > + */ > + host += pc - base; > + switch (len) { > + case 2: > + if (QEMU_IS_ALIGNED(pc, 2)) { > + uint16_t t = qatomic_read((uint16_t *)host); > + stw_he_p(dest, t); > + return true; > + } > + break; > + case 4: > + if (QEMU_IS_ALIGNED(pc, 4)) { > + uint32_t t = qatomic_read((uint32_t *)host); > + stl_he_p(dest, t); > + return true; > + } > + break; > +#ifdef CONFIG_ATOMIC64 > + case 8: > + if (QEMU_IS_ALIGNED(pc, 8)) { > + uint64_t t = qatomic_read__nocheck((uint64_t *)host); > + stq_he_p(dest, t); > + return true; > + } > + break; > +#endif > + } > + /* Unaligned or partial read from the second page is not atomic. */ > + memcpy(dest, host, len); > return true; > } > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index ef1538b4fc..157be33bf6 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -265,12 +265,14 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, if (likely(((base ^ last) & TARGET_PAGE_MASK) == 0)) { /* Entire read is from the first page. */ - memcpy(dest, host + (pc - base), len); - return true; + goto do_read; } if (unlikely(((base ^ pc) & TARGET_PAGE_MASK) == 0)) { - /* Read begins on the first page and extends to the second. */ + /* + * Read begins on the first page and extends to the second. + * The unaligned read is never atomic. + */ size_t len0 = -(pc | TARGET_PAGE_MASK); memcpy(dest, host + (pc - base), len0); pc += len0; @@ -329,7 +331,39 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db, host = db->host_addr[1]; } - memcpy(dest, host + (pc - base), len); + do_read: + /* + * Assume aligned reads should be atomic, if possible. + * We're not in a position to jump out with EXCP_ATOMIC. + */ + host += pc - base; + switch (len) { + case 2: + if (QEMU_IS_ALIGNED(pc, 2)) { + uint16_t t = qatomic_read((uint16_t *)host); + stw_he_p(dest, t); + return true; + } + break; + case 4: + if (QEMU_IS_ALIGNED(pc, 4)) { + uint32_t t = qatomic_read((uint32_t *)host); + stl_he_p(dest, t); + return true; + } + break; +#ifdef CONFIG_ATOMIC64 + case 8: + if (QEMU_IS_ALIGNED(pc, 8)) { + uint64_t t = qatomic_read__nocheck((uint64_t *)host); + stq_he_p(dest, t); + return true; + } + break; +#endif + } + /* Unaligned or partial read from the second page is not atomic. */ + memcpy(dest, host, len); return true; }
Perform aligned atomic reads in translator_ld, if possible. According to https://lore.kernel.org/qemu-devel/20240607101403.1109-1-jim.shu@sifive.com/ this is required for RISC-V Ziccif. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- accel/tcg/translator.c | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-)