Re: [Qemu-devel] [PATCH] hw/ssi/xilinx_spips: add lqspi_write routine

[Philippe Mathieu-Daudé]

Cc'ing the Xilinx folks.

On 7/3/19 9:06 PM, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> Define skeleton lqspi_write routine. Avoid NULL dereference.
> 
> Reported-by: Lei Sun <address@hidden>

I wish reporters give more information about their findings, that would
help understanding the fix is correct, or how we could find/fix other
places with similar issues...

Does this come from static analysis?
Guest fuzzing code? If Linux guest, this range should not be ioremapped.

Sometimes this I/O range are not visible from the CPU address space but
only the DMA.

Anyway, if this comes from new access from the kernel driver, then we
should probably implement more registers in QEMU.

> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  hw/ssi/xilinx_spips.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 8115bb6d46..0836b8977a 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1221,8 +1221,15 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int 
> size)
>      }
>  }
>  
> +static void
> +lqspi_write(void *opaque, hwaddr addr, uint64_t data, unsigned size)
> +{
> +    return;

My first reflex was to ask you to replace this line by this (or equivalent):

  qemu_log_mask(LOG_UNIMP,
                "%s Unexpected %u-bit access to 0x%"PRIx64
                " (value: 0x%"PRIx64"\n",
                __func__, size << 3, offset, data);

So we can track guest accesses to this range.

However, looking at the datasheet 'UG1085 (v1.0) November 24, 2015',
Chapter 22: Quad-SPI Controller, I understand this region is only
accessible by the CPU in READ mode, as an AXI slave.

So, if we model this, even logging LOG_GUEST_ERROR is incorrect, we
should trap some AXI bus access error.

So the proper fix might be:

> +}
> +
>  static const MemoryRegionOps lqspi_ops = {
>      .read = lqspi_read,
> +    .write = lqspi_write,

       .write_with_attrs = lqspi_write,

>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> 

With:

static MemTxResult lqspi_write(void *opaque, hwaddr offset,
                               uint64_t value, unsigned size,
                               MemTxAttrs attrs)
{
    return MEMTX_DECODE_ERROR;
}

So arm_extabort_type() could return a "AXI bus Decode error".

Peter/Edgar, what do you think?

Regards,

Phil.