Re: [PATCH] sim: riscv: Fix some issues with class-a instructions

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 4/22/24 17:01, Andrew Burgess wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
>>      {
>> +    case MATCH_LR_D:
>>      case MATCH_LR_W:
>>        TRACE_INSN (cpu, "%s %s, (%s);", op->name, rd_name, rs1_name);
>> -      store_rd (cpu, rd,
>> -	sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map,
>> -				   riscv_cpu->regs[rs1]));
>> +      if (op->xlen_requirement == 64)
>> +	tmp = sim_core_read_aligned_8 (cpu, riscv_cpu->pc, read_map,
>> +				       riscv_cpu->regs[rs1]);
>> +      else
>> +	tmp = EXTEND32 (sim_core_read_aligned_4 (cpu, riscv_cpu->pc, read_map,
>> +						 riscv_cpu->regs[rs1]));
> 
> I might be completely off the mark here, but I'm unsure about this
> change from using 'unaligned' to 'aligned' here.
> 
> From my reading of common/sim-n-core.h, I believe the aligned/unaligned
> refers to the callers knowledge of the address.  So calling
> sim_core_read_aligned_* means that the common code should assume that
> the address is correctly aligned.  While sim_core_read_unaligned_*
> handles the case that the address might not be aligned, and raises the
> exception.
> 
> Please remember: I might be completely wrong here, and you just need to
> explain to me what's going on better.
> 
> 

Ah, you are right, the sim_core_read_aligned does hit an assertion:

sim-core.c:432: assertion failed - (addr & (nr_bytes - 1)) == 0
Aborted

That is of course not what I wanted. I think I should better tempoarily
change the value of "current_alignment".

> 
>> +      store_rd (cpu, rd, tmp);
>>  
>>        /* Walk the reservation list to find an existing match.  */
>>        amo_curr = state->amo_reserved_list;
>> @@ -878,6 +886,7 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>        amo_curr->next = state->amo_reserved_list;
>>        state->amo_reserved_list = amo_curr;
>>        goto done;
>> +    case MATCH_SC_D:
>>      case MATCH_SC_W:
>>        TRACE_INSN (cpu, "%s %s, %s, (%s);", op->name, rd_name, rs2_name,
>>  		  rs1_name);
>> @@ -889,7 +898,12 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>  	  if (amo_curr->addr == riscv_cpu->regs[rs1])
>>  	    {
>>  	      /* We found a reservation, so operate it.  */
>> -	      sim_core_write_unaligned_4 (cpu, riscv_cpu->pc, write_map,
>> +	      if (op->xlen_requirement == 64)
>> +		sim_core_write_aligned_8 (cpu, riscv_cpu->pc, write_map,
>> +					  riscv_cpu->regs[rs1],
>> +					  riscv_cpu->regs[rs2]);
>> +	      else
>> +		sim_core_write_aligned_4 (cpu, riscv_cpu->pc, write_map,
>>  					  riscv_cpu->regs[rs1],
>>  					  riscv_cpu->regs[rs2]);
>>  	      store_rd (cpu, rd, 0);
>> @@ -913,14 +927,14 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>    TRACE_INSN (cpu, "%s %s, %s, (%s);",
>>  	      op->name, rd_name, rs2_name, rs1_name);
>>    if (op->xlen_requirement == 64)
>> -    tmp = sim_core_read_unaligned_8 (cpu, riscv_cpu->pc, read_map,
>> -				     riscv_cpu->regs[rs1]);
>> +    tmp = sim_core_read_aligned_8 (cpu, riscv_cpu->pc, read_map,
>> +				   riscv_cpu->regs[rs1]);
>>    else
>> -    tmp = EXTEND32 (sim_core_read_unaligned_4 (cpu, riscv_cpu->pc, read_map,
>> -					       riscv_cpu->regs[rs1]));
>> +    tmp = EXTEND32 (sim_core_read_aligned_4 (cpu, riscv_cpu->pc, read_map,
>> +					     riscv_cpu->regs[rs1]));
>>    store_rd (cpu, rd, tmp);
>>  
>> -  switch (op->match)
>> +  switch (op->match & ~mask_aqrl)
>>      {
>>      case MATCH_AMOADD_D:
>>      case MATCH_AMOADD_W:
>> @@ -931,25 +945,37 @@ execute_a (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op)
>>        tmp = riscv_cpu->regs[rd] & riscv_cpu->regs[rs2];
>>        break;
>>      case MATCH_AMOMAX_D:
>> -    case MATCH_AMOMAX_W:
>>        tmp = max ((signed_word) riscv_cpu->regs[rd],
>>  		 (signed_word) riscv_cpu->regs[rs2]);
>>        break;
>> +    case MATCH_AMOMAX_W:
>> +      tmp = max ((int32_t) riscv_cpu->regs[rd],
>> +		 (int32_t) riscv_cpu->regs[rs2]);
> 
> I wonder if we should use EXTEND32 here?  There seems to be a mix of
> styles throughout the file.
> 

No that is not necessary, because the value is just used to write the
result back to memory using sim_core_write_unaligned_4/8 and that should
ignore the high word.  But I will better try that out before I send
the next version of this patch.
Other places pass the value to store_rd so the high word will be visible in
an riscv64 architecture register.

>> +      break;
>>      case MATCH_AMOMAXU_D:
>> -    case MATCH_AMOMAXU_W:
>>        tmp = max ((unsigned_word) riscv_cpu->regs[rd],
>>  		 (unsigned_word) riscv_cpu->regs[rs2]);
>>        break;
>> +    case MATCH_AMOMAXU_W:
>> +      tmp = max ((uint32_t) riscv_cpu->regs[rd],
>> +		 (uint32_t) riscv_cpu->regs[rs2]);
> 
> And maybe EXTEND32 again here, but I guess we'd still need the cast to
> unsigned_word.
> 

Same here, some kind of sign extension happens, but the high word should be
ignored and there is no ugly compiler warning either.

But I can try that out if it behaves correctly...


Thanks
Bernd.