[0/2] target/riscv: Bugfixes found in decodetree conversion

Message

Bastian Koppelmann Nov. 8, 2018, 12:06 p.m. UTC

Hi,

while going through the reviews of the riscv-decodetree patches, two bugs came
up that I fix here. There is one more problem [1] mentioned by Richard but 
I don't have the time to investigate it further.

[1] https://patchwork.kernel.org/patch/10650293/

Bastian Koppelmann (2):
  target/riscv: Fix FCLASS_D being treated as RV64 only
  target/riscv: Fix sfence.vm/a both available in any priv version

 target/riscv/translate.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Richard Henderson Nov. 8, 2018, 3:53 p.m. UTC | #1

On 11/8/18 1:06 PM, Bastian Koppelmann wrote:
> while going through the reviews of the riscv-decodetree patches, two bugs came
> up that I fix here. There is one more problem [1] mentioned by Richard but 
> I don't have the time to investigate it further.
> 
> [1] https://patchwork.kernel.org/patch/10650293/

That one's not a bug, but an optimization.

The other bug mentioned is shrw and shaw not sign-extending the result.


r~

Bastian Koppelmann Nov. 8, 2018, 5:29 p.m. UTC | #2

On 11/8/18 4:53 PM, Richard Henderson wrote:
> On 11/8/18 1:06 PM, Bastian Koppelmann wrote:
>> while going through the reviews of the riscv-decodetree patches, two bugs came
>> up that I fix here. There is one more problem [1] mentioned by Richard but
>> I don't have the time to investigate it further.
>>
>> [1] https://patchwork.kernel.org/patch/10650293/
> That one's not a bug, but an optimization.
>
> The other bug mentioned is shrw and shaw not sign-extending the result.


That was a bug I introduced during the conversion to decodetree.


Cheers,

Bastian

Palmer Dabbelt Nov. 8, 2018, 7:12 p.m. UTC | #3

On Thu, 08 Nov 2018 09:29:26 PST (-0800), Bastian Koppelmann wrote:
>
> On 11/8/18 4:53 PM, Richard Henderson wrote:
>> On 11/8/18 1:06 PM, Bastian Koppelmann wrote:
>>> while going through the reviews of the riscv-decodetree patches, two bugs came
>>> up that I fix here. There is one more problem [1] mentioned by Richard but
>>> I don't have the time to investigate it further.
>>>
>>> [1] https://patchwork.kernel.org/patch/10650293/
>> That one's not a bug, but an optimization.
>>
>> The other bug mentioned is shrw and shaw not sign-extending the result.
>
>
> That was a bug I introduced during the conversion to decodetree.

My understand of that patch feedback is that there are two issues:

* We don't take advantage of the ordering bits on fences, which could allow us 
  to emit less conservative fences.  This would presumably increase 
  performance.
* We treat fences as NOPs under "#ifndef CONFIG_USER_ONLY", which is a bug.

Am I wrong about that second one?  I think the fix should look something like 
this, which I haven't even tried to compile

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 18d7b6d1471d..624d1c679a84 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1766,7 +1766,6 @@ static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx)
                      GET_RM(ctx->opcode));
         break;
     case OPC_RISC_FENCE:
-#ifndef CONFIG_USER_ONLY
         if (ctx->opcode & 0x1000) {
             /* FENCE_I is a no-op in QEMU,
              * however we need to end the translation block */
@@ -1777,7 +1776,6 @@ static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx)
             /* FENCE is a full memory barrier. */
             tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
         }
-#endif
         break;
     case OPC_RISC_SYSTEM:
         gen_system(env, ctx, MASK_OP_SYSTEM(ctx->opcode), rd, rs1,

Richard Henderson Nov. 9, 2018, 6:14 a.m. UTC | #4

On 11/8/18 8:12 PM, Palmer Dabbelt wrote:
> * We don't take advantage of the ordering bits on fences, which could allow us
>  to emit less conservative fences.  This would presumably increase  performance.

Yes.

> * We treat fences as NOPs under "#ifndef CONFIG_USER_ONLY", which is a bug.

Ah yes, I'd forgotten this one.  This is a bug, in that multi-threaded user
programs do not get the memory ordering that they requested.

Hard to see, obviously, since the emulator has its own memory operations,
barriers, and locks.  But once we have a lot of the hot path of the user
program compiled, there's a lot less of that.


>     case OPC_RISC_FENCE:
> -#ifndef CONFIG_USER_ONLY
>         if (ctx->opcode & 0x1000) {
>             /* FENCE_I is a no-op in QEMU,
>              * however we need to end the translation block */
> @@ -1777,7 +1776,6 @@ static void decode_RV32_64G(CPURISCVState *env,
> DisasContext *ctx)
>             /* FENCE is a full memory barrier. */
>             tcg_gen_mb(TCG_MO_ALL | TCG_BAR_SC);
>         }
> -#endif
>         break;

Yes, this is minimally correct.


r~