[Qemu-devel] [PATCH-for-4.1 v5 0/3] hw/ssi/xilinx_spips: Avoid NULL dereference and out-of-bound access

[Qemu-devel] [PATCH-for-4.1 v5 0/3] hw/ssi/xilinx_spips: Avoid NULL dereference and out-of-bound access

[Philippe Mathieu-Daudé]

v1: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01238.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01586.html
v3: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01657.html
v4: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01777.html
v5:
- address Francisco comments from v4

Philippe Mathieu-Daudé (3):
  hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs
  hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory
  hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

 hw/ssi/xilinx_spips.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1 v5 1/3] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Philippe Mathieu-Daudé]

In the next commit we will implement the write_with_attrs()
handler. To avoid using different APIs, convert the read()
handler first.

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v4: Do not ignore lqspi_read() return value (Francisco)
---
 hw/ssi/xilinx_spips.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 8115bb6d46..b7c7275dbe 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1202,27 +1202,26 @@ 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;
-    } else {
-        lqspi_load_cache(opaque, addr);
-        return lqspi_read(opaque, addr, size);
+        *value = cpu_to_le32(*(uint32_t *)retp);
+        DB_PRINT_L(1, "addr: %08" HWADDR_PRIx ", data: %08" PRIx64 "\n",
+                   addr, *value);
+        return MEMTX_OK;
     }
+
+    lqspi_load_cache(opaque, addr);
+    return lqspi_read(opaque, addr, value, size, attrs);
 }
 
 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

[Qemu-devel] [PATCH-for-4.1 v5 2/3] hw/ssi/xilinx_spips: Avoid AXI writes to the LQSPI linear memory

[Philippe Mathieu-Daudé]

Lei Sun found while auditing the code that a CPU write would
trigger a NULL pointer dereference.

From UG1085 datasheet [*] AXI writes in this region are ignored
and generates an AXI Slave Error (SLVERR).

Fix by implementing the write_with_attrs() handler.
Return MEMTX_ERROR when the region is accessed (this error maps
to an AXI slave error).

[*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

Reported-by: Lei Sun <slei.casper@gmail.com>
Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v4: Fix typos (Francisco)
---
 hw/ssi/xilinx_spips.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index b7c7275dbe..3c4e8365ee 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1220,8 +1220,24 @@ static MemTxResult lqspi_read(void *opaque, hwaddr addr, uint64_t *value,
     return lqspi_read(opaque, addr, value, size, attrs);
 }
 
+static MemTxResult lqspi_write(void *opaque, hwaddr offset, uint64_t value,
+                               unsigned size, MemTxAttrs attrs)
+{
+    /*
+     * From UG1085, Chapter 24 (Quad-SPI controllers):
+     * - Writes are ignored
+     * - AXI writes generate an external AXI slave error (SLVERR)
+     */
+    qemu_log_mask(LOG_GUEST_ERROR, "%s Unexpected %u-bit access to 0x%" PRIx64
+                                   " (value: 0x%" PRIx64 "\n",
+                  __func__, size << 3, offset, value);
+
+    return MEMTX_ERROR;
+}
+
 static const MemoryRegionOps lqspi_ops = {
     .read_with_attrs = lqspi_read,
+    .write_with_attrs = lqspi_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
         .min_access_size = 1,
-- 
2.20.1

[Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Philippe Mathieu-Daudé]

Both lqspi_read() and lqspi_load_cache() expect a 32-bit
aligned address.

From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:

  Transfer Size Limitations

    Because of the 32-bit wide TX, RX, and generic FIFO, all
    APB/AXI transfers must be an integer multiple of 4-bytes.
    Shorter transfers are not possible.

Set MemoryRegionOps.impl values to force 32-bit accesses,
this way we are sure we do not access the lqspi_buf[] array
out of bound.

[*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
---
 hw/ssi/xilinx_spips.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index 3c4e8365ee..b29e0a4a89 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
     .read_with_attrs = lqspi_read,
     .write_with_attrs = lqspi_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
     .valid = {
         .min_access_size = 1,
         .max_access_size = 4
-- 
2.20.1

Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Francisco Iglesias]

Hi Philippe,

On [2019 Jul 08] Mon 12:47:50, Philippe Mathieu-Daudé wrote:
> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
> aligned address.
> 
> From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:

s/22/24/

After above change:

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

(I tested all three patches so the Tested-by tag can also be appended on the
other two)

Best regards,
Francisco Iglesias

> 
>   Transfer Size Limitations
> 
>     Because of the 32-bit wide TX, RX, and generic FIFO, all
>     APB/AXI transfers must be an integer multiple of 4-bytes.
>     Shorter transfers are not possible.
> 
> Set MemoryRegionOps.impl values to force 32-bit accesses,
> this way we are sure we do not access the lqspi_buf[] array
> out of bound.
> 
> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
> ---
>  hw/ssi/xilinx_spips.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 3c4e8365ee..b29e0a4a89 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
>      .read_with_attrs = lqspi_read,
>      .write_with_attrs = lqspi_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
>      .valid = {
>          .min_access_size = 1,
>          .max_access_size = 4
> -- 
> 2.20.1
> 

Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Philippe Mathieu-Daudé]

Hi Francisco,

On 7/8/19 4:26 PM, Francisco Iglesias wrote:
> Hi Philippe,
> 
> On [2019 Jul 08] Mon 12:47:50, Philippe Mathieu-Daudé wrote:
>> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
>> aligned address.
>>
>> From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:
> 
> s/22/24/

I'm confuse, Chapter 24 is "SD/SDIO Controller", so it seems this Xilinx
datasheet is not stable to refer to. Maybe I should simply use:

  From UG1085 datasheet [*] Chapter on 'Quad-SPI Controller':

> 
> After above change:
> 
> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> 
> Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> 
> (I tested all three patches so the Tested-by tag can also be appended on the
> other two)

Thanks!

> 
> Best regards,
> Francisco Iglesias
> 
>>
>>   Transfer Size Limitations
>>
>>     Because of the 32-bit wide TX, RX, and generic FIFO, all
>>     APB/AXI transfers must be an integer multiple of 4-bytes.
>>     Shorter transfers are not possible.
>>
>> Set MemoryRegionOps.impl values to force 32-bit accesses,
>> this way we are sure we do not access the lqspi_buf[] array
>> out of bound.
>>
>> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
>> ---
>>  hw/ssi/xilinx_spips.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>> index 3c4e8365ee..b29e0a4a89 100644
>> --- a/hw/ssi/xilinx_spips.c
>> +++ b/hw/ssi/xilinx_spips.c
>> @@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
>>      .read_with_attrs = lqspi_read,
>>      .write_with_attrs = lqspi_write,
>>      .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>>      .valid = {
>>          .min_access_size = 1,
>>          .max_access_size = 4
>> -- 
>> 2.20.1
>>

Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Francisco Iglesias]

Hi Philippe,

On [2019 Jul 08] Mon 16:58:29, Philippe Mathieu-Daudé wrote:
> Hi Francisco,
> 
> On 7/8/19 4:26 PM, Francisco Iglesias wrote:
> > Hi Philippe,
> > 
> > On [2019 Jul 08] Mon 12:47:50, Philippe Mathieu-Daudé wrote:
> >> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
> >> aligned address.
> >>
> >> From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:
> > 
> > s/22/24/
> 
> I'm confuse, Chapter 24 is "SD/SDIO Controller", so it seems this Xilinx
> datasheet is not stable to refer to. Maybe I should simply use:
> 
>   From UG1085 datasheet [*] Chapter on 'Quad-SPI Controller':

I just clicked on the link and ended up into this version of the UG1085:

'UG1085 (v1.9) January 17, 2019'

But your right its probably better to refer to a specific version of the
UG1085 instead? Then both should be ok, either to write the way you
suggest above or refer to the chapter number in that UG1085 version (as it
was before).

Best regards,
Francisco

> 
> > 
> > After above change:
> > 
> > Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> 
> > Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> > 
> > (I tested all three patches so the Tested-by tag can also be appended on the
> > other two)
> 
> Thanks!
> 
> > 
> > Best regards,
> > Francisco Iglesias
> > 
> >>
> >>   Transfer Size Limitations
> >>
> >>     Because of the 32-bit wide TX, RX, and generic FIFO, all
> >>     APB/AXI transfers must be an integer multiple of 4-bytes.
> >>     Shorter transfers are not possible.
> >>
> >> Set MemoryRegionOps.impl values to force 32-bit accesses,
> >> this way we are sure we do not access the lqspi_buf[] array
> >> out of bound.
> >>
> >> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >> v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
> >> ---
> >>  hw/ssi/xilinx_spips.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> >> index 3c4e8365ee..b29e0a4a89 100644
> >> --- a/hw/ssi/xilinx_spips.c
> >> +++ b/hw/ssi/xilinx_spips.c
> >> @@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
> >>      .read_with_attrs = lqspi_read,
> >>      .write_with_attrs = lqspi_write,
> >>      .endianness = DEVICE_NATIVE_ENDIAN,
> >> +    .impl = {
> >> +        .min_access_size = 4,
> >> +        .max_access_size = 4,
> >> +    },
> >>      .valid = {
> >>          .min_access_size = 1,
> >>          .max_access_size = 4
> >> -- 
> >> 2.20.1
> >>

Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Philippe Mathieu-Daudé]

Hi Peter,

On 7/8/19 6:03 PM, Francisco Iglesias wrote:
> Hi Philippe,
> 
> On [2019 Jul 08] Mon 16:58:29, Philippe Mathieu-Daudé wrote:
>> Hi Francisco,
>>
>> On 7/8/19 4:26 PM, Francisco Iglesias wrote:
>>> Hi Philippe,
>>>
>>> On [2019 Jul 08] Mon 12:47:50, Philippe Mathieu-Daudé wrote:
>>>> Both lqspi_read() and lqspi_load_cache() expect a 32-bit
>>>> aligned address.
>>>>
>>>> From UG1085 datasheet [*] Chapter 22: Quad-SPI Controller:
>>>
>>> s/22/24/
>>
>> I'm confuse, Chapter 24 is "SD/SDIO Controller", so it seems this Xilinx
>> datasheet is not stable to refer to. Maybe I should simply use:
>>
>>   From UG1085 datasheet [*] Chapter on 'Quad-SPI Controller':
> 
> I just clicked on the link and ended up into this version of the UG1085:
> 
> 'UG1085 (v1.9) January 17, 2019'
> 
> But your right its probably better to refer to a specific version of the
> UG1085 instead? Then both should be ok, either to write the way you
> suggest above or refer to the chapter number in that UG1085 version (as it
> was before).
> 
> Best regards,
> Francisco
> 
>>
>>>
>>> After above change:
>>>
>>> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> 
>>> Tested-by: Francisco Iglesias <frasse.iglesias@gmail.com>
>>>
>>> (I tested all three patches so the Tested-by tag can also be appended on the
>>> other two)

Are you OK to take this series via your ARM tree?

If so, do you want me to respin fixing the comment and adding Francisco
tags?

Thanks,

Phil.

>>
>> Thanks!
>>
>>>
>>> Best regards,
>>> Francisco Iglesias
>>>
>>>>
>>>>   Transfer Size Limitations
>>>>
>>>>     Because of the 32-bit wide TX, RX, and generic FIFO, all
>>>>     APB/AXI transfers must be an integer multiple of 4-bytes.
>>>>     Shorter transfers are not possible.
>>>>
>>>> Set MemoryRegionOps.impl values to force 32-bit accesses,
>>>> this way we are sure we do not access the lqspi_buf[] array
>>>> out of bound.
>>>>
>>>> [*] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>> v5: add datasheet reference, drop RFC prefix, fix build (Francisco)
>>>> ---
>>>>  hw/ssi/xilinx_spips.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>>>> index 3c4e8365ee..b29e0a4a89 100644
>>>> --- a/hw/ssi/xilinx_spips.c
>>>> +++ b/hw/ssi/xilinx_spips.c
>>>> @@ -1239,6 +1239,10 @@ static const MemoryRegionOps lqspi_ops = {
>>>>      .read_with_attrs = lqspi_read,
>>>>      .write_with_attrs = lqspi_write,
>>>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>>> +    .impl = {
>>>> +        .min_access_size = 4,
>>>> +        .max_access_size = 4,
>>>> +    },
>>>>      .valid = {
>>>>          .min_access_size = 1,
>>>>          .max_access_size = 4
>>>> -- 
>>>> 2.20.1
>>>>

Re: [Qemu-devel] [PATCH-for-4.1 v5 1/3] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Peter Maydell]

On Mon, 8 Jul 2019 at 11:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> In the next commit we will implement the write_with_attrs()
> handler. To avoid using different APIs, convert the read()
> handler first.
>
> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v4: Do not ignore lqspi_read() return value (Francisco)
> ---
>  hw/ssi/xilinx_spips.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 8115bb6d46..b7c7275dbe 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1202,27 +1202,26 @@ 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;
> -    } else {
> -        lqspi_load_cache(opaque, addr);
> -        return lqspi_read(opaque, addr, size);
> +        *value = cpu_to_le32(*(uint32_t *)retp);

If you find yourself casting a uint8_t* to uint32_t* in
order to pass it to cpu_to_le32(), it's a sign that you
should instead be using one of the "load/store value in
appropriate endianness" operations. In this case I think
you want
    *value = ldl_le_p(retp);

That looks like it was an issue already present in this code,
though, (we do it several times in various places in the source file)
so we can fix it later.

thanks
-- PMM

Re: [Qemu-devel] [PATCH-for-4.1 v5 3/3] hw/ssi/xilinx_spips: Avoid out-of-bound access to lqspi_buf[]

[Peter Maydell]

On Tue, 9 Jul 2019 at 11:58, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Are you OK to take this series via your ARM tree?
>
> If so, do you want me to respin fixing the comment and adding Francisco
> tags?

Yes, and yes please.

thanks
-- PMM

Re: [Qemu-devel] [PATCH-for-4.1 v5 1/3] hw/ssi/xilinx_spips: Convert lqspi_read() to read_with_attrs

[Philippe Mathieu-Daudé]

On 7/9/19 1:11 PM, Peter Maydell wrote:
> On Mon, 8 Jul 2019 at 11:48, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> In the next commit we will implement the write_with_attrs()
>> handler. To avoid using different APIs, convert the read()
>> handler first.
>>
>> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v4: Do not ignore lqspi_read() return value (Francisco)
>> ---
>>  hw/ssi/xilinx_spips.c | 23 +++++++++++------------
>>  1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>> index 8115bb6d46..b7c7275dbe 100644
>> --- a/hw/ssi/xilinx_spips.c
>> +++ b/hw/ssi/xilinx_spips.c
>> @@ -1202,27 +1202,26 @@ 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;
>> -    } else {
>> -        lqspi_load_cache(opaque, addr);
>> -        return lqspi_read(opaque, addr, size);
>> +        *value = cpu_to_le32(*(uint32_t *)retp);
> 
> If you find yourself casting a uint8_t* to uint32_t* in
> order to pass it to cpu_to_le32(), it's a sign that you
> should instead be using one of the "load/store value in
> appropriate endianness" operations. In this case I think
> you want
>     *value = ldl_le_p(retp);
> 
> That looks like it was an issue already present in this code,
> though, (we do it several times in various places in the source file)
> so we can fix it later.

Well, other places check GQSPI_CFG.ENDIAN bit for switching endianess,
here we don't... Dubious code? Per the code DMA accesses seems
little-endian. However tx_data_bytes() handles endian swaping, while
rx_data_bytes() doesn't.

It seems wise to postpone this after the current release, indeed.

Thanks,

Phil.