1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v3 0/2] hw/ssi/xilinx_spips: Avoid NULL pointer deference
  4. View patch

[Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

Philippe Mathieu-Daudé posted 2 patches 6 years, 7 months ago
Maintainers: Alistair Francis <alistair@alistair23.me>
There is a newer version of this series
[Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
Posted by Philippe Mathieu-Daudé 6 years, 7 months ago 
In the next commit we will implement the write_with_attrs()
handler. To avoid using different APIs, convert the read()
handler first.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/ssi/xilinx_spips.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 8115bb6d46..e80619aece 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1202,27 +1202,27 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
     }
 }
 
-static uint64_t
-lqspi_read(void *opaque, hwaddr addr, unsigned int size)
+static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
+                              unsigned size, MemTxAttrs attrs)
 {
-    XilinxQSPIPS *q = opaque;
-    uint32_t ret;
+    XilinxQSPIPS *q = XILINX_QSPIPS(opaque);
 
     if (addr >= q->lqspi_cached_addr &&
             addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
         uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
-        ret = cpu_to_le32(*(uint32_t *)retp);
-        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
-                   (unsigned)ret);
-        return ret;
+        *value = cpu_to_le32(*(uint32_t *)retp);
+        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
+                   addr, *value);
     } else {
         lqspi_load_cache(opaque, addr);
-        return lqspi_read(opaque, addr, size);
+        lqspi_read(opaque, addr, value, size, attrs);
     }
+
+    return MEMTX_OK;
 }
 
 static const MemoryRegionOps lqspi_ops = {
-    .read = lqspi_read,
+    .read_with_attrs = lqspi_read,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
         .min_access_size = 1,
-- 
2.20.1
Re: [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
Posted by Francisco Iglesias 6 years, 7 months ago 
Hi Philippe,

On [2019 Jul 05] Fri 17:08:49, Philippe Mathieu-Daudé wrote:
> In the next commit we will implement the write_with_attrs()
> handler. To avoid using different APIs, convert the read()
> handler first.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/ssi/xilinx_spips.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 8115bb6d46..e80619aece 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1202,27 +1202,27 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
>      }
>  }
>  
> -static uint64_t
> -lqspi_read(void *opaque, hwaddr addr, unsigned int size)
> +static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
> +                              unsigned size, MemTxAttrs attrs)
>  {
> -    XilinxQSPIPS *q = opaque;
> -    uint32_t ret;
> +    XilinxQSPIPS *q = XILINX_QSPIPS(opaque);
>  
>      if (addr >= q->lqspi_cached_addr &&
>              addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
>          uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
> -        ret = cpu_to_le32(*(uint32_t *)retp);
> -        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
> -                   (unsigned)ret);
> -        return ret;
> +        *value = cpu_to_le32(*(uint32_t *)retp);
> +        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
> +                   addr, *value);
>      } else {
>          lqspi_load_cache(opaque, addr);
> -        return lqspi_read(opaque, addr, size);
> +        lqspi_read(opaque, addr, value, size, attrs);

If you don't want to leave the return value floating you can always keep the
'return' (I'm unsure if coverity will complain about that).

Either way:

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>


>      }
> +
> +    return MEMTX_OK;
>  }
>  
>  static const MemoryRegionOps lqspi_ops = {
> -    .read = lqspi_read,
> +    .read_with_attrs = lqspi_read,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> -- 
> 2.20.1
>
Re: [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
Posted by Philippe Mathieu-Daudé 6 years, 7 months ago 
On 7/5/19 5:53 PM, Francisco Iglesias wrote:
> Hi Philippe,
> 
> On [2019 Jul 05] Fri 17:08:49, Philippe Mathieu-Daudé wrote:
>> In the next commit we will implement the write_with_attrs()
>> handler. To avoid using different APIs, convert the read()
>> handler first.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/ssi/xilinx_spips.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>> index 8115bb6d46..e80619aece 100644
>> --- a/hw/ssi/xilinx_spips.c
>> +++ b/hw/ssi/xilinx_spips.c
>> @@ -1202,27 +1202,27 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
>>      }
>>  }
>>  
>> -static uint64_t
>> -lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>> +static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
>> +                              unsigned size, MemTxAttrs attrs)
>>  {
>> -    XilinxQSPIPS *q = opaque;
>> -    uint32_t ret;
>> +    XilinxQSPIPS *q = XILINX_QSPIPS(opaque);
>>  
>>      if (addr >= q->lqspi_cached_addr &&
>>              addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
>>          uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
>> -        ret = cpu_to_le32(*(uint32_t *)retp);
>> -        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
>> -                   (unsigned)ret);
>> -        return ret;
>> +        *value = cpu_to_le32(*(uint32_t *)retp);
>> +        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
>> +                   addr, *value);
>>      } else {
>>          lqspi_load_cache(opaque, addr);
>> -        return lqspi_read(opaque, addr, size);
>> +        lqspi_read(opaque, addr, value, size, attrs);
> 
> If you don't want to leave the return value floating you can always keep the
> 'return' (I'm unsure if coverity will complain about that).

Ah, I missed that, I'll fix.

> 
> Either way:
> 
> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

Thanks!

I'll wait some more time of other want to review, then I'll respin with
the typo you corrected in the 2nd patch fixed.

> 
>>      }
>> +
>> +    return MEMTX_OK;
>>  }
>>  
>>  static const MemoryRegionOps lqspi_ops = {
>> -    .read = lqspi_read,
>> +    .read_with_attrs = lqspi_read,
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>      .valid = {
>>          .min_access_size = 1,
>> -- 
>> 2.20.1
>>
Re: [Qemu-devel] [PATCH v3 1/2] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
Posted by Philippe Mathieu-Daudé 6 years, 7 months ago 
On 7/5/19 7:06 PM, Philippe Mathieu-Daudé wrote:
> On 7/5/19 5:53 PM, Francisco Iglesias wrote:
>> Hi Philippe,
>>
>> On [2019 Jul 05] Fri 17:08:49, Philippe Mathieu-Daudé wrote:
>>> In the next commit we will implement the write_with_attrs()
>>> handler. To avoid using different APIs, convert the read()
>>> handler first.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  hw/ssi/xilinx_spips.c | 20 ++++++++++----------
>>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>>> index 8115bb6d46..e80619aece 100644
>>> --- a/hw/ssi/xilinx_spips.c
>>> +++ b/hw/ssi/xilinx_spips.c
>>> @@ -1202,27 +1202,27 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
>>>      }
>>>  }
>>>  
>>> -static uint64_t
>>> -lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>>> +static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
>>> +                              unsigned size, MemTxAttrs attrs)
>>>  {
>>> -    XilinxQSPIPS *q = opaque;
>>> -    uint32_t ret;
>>> +    XilinxQSPIPS *q = XILINX_QSPIPS(opaque);
>>>  
>>>      if (addr >= q->lqspi_cached_addr &&
>>>              addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {

Looking at Frederic's commit 252b99baeb9, it seems this "4" has to be
replaced by "size", or cleaner, use .impl.min_access_size = 4.

Currently we have:

static const MemoryRegionOps lqspi_ops = {
    .valid = {
        .min_access_size = 1,
        .max_access_size = 4
    }
};

If we use:

- size = 1
- addr = LQSPI_CACHE_SIZE - 1

We have

'addr >= q->lqspi_cached_addr': true
'addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4': true

>>>          uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
>>> -        ret = cpu_to_le32(*(uint32_t *)retp);

Are we reading 3 extra bytes?

>>> -        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
>>> -                   (unsigned)ret);
>>> -        return ret;
>>> +        *value = cpu_to_le32(*(uint32_t *)retp);
>>> +        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
>>> +                   addr, *value);
>>>      } else {
>>>          lqspi_load_cache(opaque, addr);
>>> -        return lqspi_read(opaque, addr, size);
>>> +        lqspi_read(opaque, addr, value, size, attrs);
>>
>> If you don't want to leave the return value floating you can always keep the
>> 'return' (I'm unsure if coverity will complain about that).
> 
> Ah, I missed that, I'll fix.
> 
>>
>> Either way:
>>
>> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> 
> Thanks!
> 
> I'll wait some more time of other want to review, then I'll respin with
> the typo you corrected in the 2nd patch fixed.
> 
>>
>>>      }
>>> +
>>> +    return MEMTX_OK;
>>>  }
>>>  
>>>  static const MemoryRegionOps lqspi_ops = {
>>> -    .read = lqspi_read,
>>> +    .read_with_attrs = lqspi_read,
>>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>>      .valid = {
>>>          .min_access_size = 1,
>>> -- 
>>> 2.20.1
>>>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.