Re: [Qemu-devel] [PATCH v3 3/3] edu: uses uint64_t in dma operation

[Philippe Mathieu-Daudé]

On 5/3/19 11:12 AM, Jiri Slaby wrote:
> On 22. 04. 19, 16:11, Li Qiang wrote:
>> The dma related variable dma.dst/src/cnt is dma_addr_t, it is
>> uint64_t in x64 platform. Change these usage from uint32_to
>> uint64_t to avoid trancation in edu_dma_timer.

Here I suggested fix the typo with "to avoid address truncation".

>>
>> Signed-off-by: Li Qiang <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daude <address@hidden>
>> ---
>>  hw/misc/edu.c | 15 ++++++++-------
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
>> index 33de05141f..401ada74af 100644
>> --- a/hw/misc/edu.c
>> +++ b/hw/misc/edu.c
>> @@ -98,23 +98,24 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
>>      }
>>  }
>>  
>> -static bool within(uint32_t addr, uint32_t start, uint32_t end)
>> +static bool within(uint64_t addr, uint64_t start, uint64_t end)
>>  {
>>      return start <= addr && addr < end;
>>  }
>>  
>> -static void edu_check_range(uint32_t addr, uint32_t size1, uint32_t start,
>> +static void edu_check_range(uint64_t addr, uint64_t size1, uint64_t start,
>>                  uint32_t size2)
> 
> So in this version you don't change size2's type, but you still change
> size1's one :)?

On my previous review I suggested Li to keep u32 since the MMIO region
is 1 MB wide:

https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03505.html

I agree to be coherent we should use one or another, but not a mix.

> 
> Other than that, looks good to me.
> 
>>  {
>> -    uint32_t end1 = addr + size1;
>> -    uint32_t end2 = start + size2;
>> +    uint64_t end1 = addr + size1;
>> +    uint64_t end2 = start + size2;
>>  
>>      if (within(addr, start, end2) &&
>>              end1 > addr && within(end1, start, end2)) {
>>          return;
>>      }
>>  
>> -    hw_error("EDU: DMA range 0x%.8x-0x%.8x out of bounds (0x%.8x-0x%.8x)!",
>> +    hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
>> +             " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
>>              addr, end1 - 1, start, end2 - 1);
>>  }
> 
> 
>