Re: [PULL 00/32] AVR port

[Thomas Huth]

On 10/07/2020 17.32, Philippe Mathieu-Daudé wrote:
> 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:
[...]
> @@ -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]);

That part looks wrong?

 Thomas