Re: [PULL 00/32] AVR port

[Philippe Mathieu-Daudé]

On 7/10/20 5:17 PM, Philippe Mathieu-Daudé wrote:
> On 7/10/20 5:12 PM, Peter Maydell wrote:
>> On Fri, 10 Jul 2020 at 16:03, Thomas Huth <thuth@redhat.com> wrote:
>>> Endianess bug ... this should fix it:
>>>
>>> diff --git a/target/avr/helper.c b/target/avr/helper.c
>>> --- a/target/avr/helper.c
>>> +++ b/target/avr/helper.c
>>> @@ -337,6 +337,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data,
>>> uint32_t addr)
>>>          helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data);
>>>      } else {
>>>          /* memory */
>>> -        cpu_physical_memory_write(OFFSET_DATA + addr, &data, 1);
>>> +        uint8_t data8 = data;
>>> +        cpu_physical_memory_write(OFFSET_DATA + addr, &data8, 1);
>>>      }
>>
>> Or equivalently
>>   address_space_stb(&address_space_memory, data, MEMTXATTRS_UNSPECIFIED, 
>> NULL);
>>
>> (better choices of address space may be available, but this is
>> the exact-same-behaviour one).
> 
> Ah, this is my stashed fix:
> 
> -- >8 --
> @@ -320,8 +320,10 @@ target_ulong helper_fullrd(CPUAVRState *env,
> uint32_t addr)
>   *  this function implements ST instruction when there is a posibility
> to write
>   *  into a CPU register
>   */
> -void helper_fullwr(CPUAVRState *env, uint32_t data, uint32_t addr)
> +void helper_fullwr(CPUAVRState *env, uint32_t data32, uint32_t addr)
>  {
> +    uint8_t data = data32;
> +    assert(data == data32);
> +
>      env->fullacc = false;
> 
> ---
> 
> 3 ways to do the same :) The assert is probably superfluous.
> 
> I don't like the fact that env->r[addr] (which is u8) is silently casted
> from u32.

I'll squash Peter suggested fix:

-- >8 --
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -232,7 +232,9 @@ target_ulong helper_inb(CPUAVRState *env, uint32_t port)
         break;
     default:
         /* not a special register, pass to normal memory access */
-        cpu_physical_memory_read(OFFSET_IO_REGISTERS + port, &data, 1);
+        data = address_space_ldub(&address_space_memory,
+                                  OFFSET_IO_REGISTERS + port,
+                                  MEMTXATTRS_UNSPECIFIED, NULL);
     }

     return data;
@@ -289,7 +291,8 @@ void helper_outb(CPUAVRState *env, uint32_t port,
uint32_t data)
         break;
     default:
         /* not a special register, pass to normal memory access */
-        cpu_physical_memory_write(OFFSET_IO_REGISTERS + port, &data, 1);
+        address_space_stb(&address_space_memory, OFFSET_IO_REGISTERS +
port,
+                          data, MEMTXATTRS_UNSPECIFIED, NULL);
     }
 }

@@ -305,13 +308,14 @@ target_ulong helper_fullrd(CPUAVRState *env,
uint32_t addr)

     if (addr < NUMBER_OF_CPU_REGISTERS) {
         /* CPU registers */
-        data = env->r[addr];
+        data = cpu_to_le32(env->r[addr]);
     } else if (addr < NUMBER_OF_CPU_REGISTERS + NUMBER_OF_IO_REGISTERS) {
         /* IO registers */
         data = helper_inb(env, addr - NUMBER_OF_CPU_REGISTERS);
     } else {
         /* memory */
-        cpu_physical_memory_read(OFFSET_DATA + addr, &data, 1);
+        data = address_space_ldub(&address_space_memory, OFFSET_DATA +
addr,
+                                  MEMTXATTRS_UNSPECIFIED, NULL);
     }
     return data;
 }
@@ -337,6 +341,7 @@ void helper_fullwr(CPUAVRState *env, uint32_t data,
uint32_t addr)
         helper_outb(env, addr - NUMBER_OF_CPU_REGISTERS, data);
     } else {
         /* memory */
-        cpu_physical_memory_write(OFFSET_DATA + addr, &data, 1);
+        address_space_stb(&address_space_memory, OFFSET_DATA + addr, data,
+                          MEMTXATTRS_UNSPECIFIED, NULL);
     }
 }
---