diff mbox series

[v3,3/3] edu: uses uint64_t in dma operation

Message ID 20190422141156.15746-4-liq3ea@163.com
State New
Headers show
Series hw: edu: some fixes | expand

Commit Message

Li Qiang April 22, 2019, 2:11 p.m. UTC 
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.

Signed-off-by: Li Qiang <liq3ea@163.com>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 hw/misc/edu.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Jiri Slaby May 3, 2019, 9:12 a.m. UTC | #1 
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.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
>  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 :)?

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);
>  }
Philippe Mathieu-Daudé May 3, 2019, 9:19 a.m. UTC | #2 
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 <liq3ea@163.com>
>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>> ---
>>  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);
>>  }
> 
> 
>
diff mbox series

Patch

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)
 {
-    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);
 }
 
@@ -139,13 +140,13 @@  static void edu_dma_timer(void *opaque)
     }
 
     if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
-        uint32_t dst = edu->dma.dst;
+        uint64_t dst = edu->dma.dst;
         edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
         dst -= DMA_START;
         pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
                 edu->dma_buf + dst, edu->dma.cnt);
     } else {
-        uint32_t src = edu->dma.src;
+        uint64_t src = edu->dma.src;
         edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
         src -= DMA_START;
         pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),