Re: [Qemu-arm] [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 <address@hidden> 
> > Tested-by: Francisco Iglesias <address@hidden>
> > 
> > (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é <address@hidden>
> >> ---
> >> 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
> >>