Re: [Qemu-devel] [PATCH v6] hw/ssi/imx_spi.c: fix CS handling during SPI access.

[Peter Maydell]

On 11 January 2017 at 20:00, Jean-Christophe Dubois <address@hidden> wrote:
> The i.MX SPI device was not de-asserting the CS line at the end of
> memory access.
>
> This triggered a SIGSEGV in Qemu when the sabrelite emulator was acessing
> a SPI flash memory.
>
> Whith this path the CS signal is correctly asserted and deasserted arround
> memory access.
>
> Assertion level is now based on SPI device configuration.
>
> This was tested by:
> * booting linux on Sabrelite Qemu emulator.
> * booting xvisor on Sabrelite Qemu emultor.
>
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> Acked-by: Marcin Krzemiński <address@hidden>

>  hw/ssi/imx_spi.c         | 148 
> ++++++++++++++++++++++++++++++++++++-----------
>  include/hw/ssi/imx_spi.h |   4 ++
>  2 files changed, 117 insertions(+), 35 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index e4e395f..d54c145 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -56,19 +56,6 @@ static const char *imx_spi_reg_name(uint32_t reg)
>      }
>  }
>
> -static const VMStateDescription vmstate_imx_spi = {
> -    .name = TYPE_IMX_SPI,
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> -    .fields = (VMStateField[]) {
> -        VMSTATE_FIFO32(tx_fifo, IMXSPIState),
> -        VMSTATE_FIFO32(rx_fifo, IMXSPIState),
> -        VMSTATE_INT16(burst_length, IMXSPIState),
> -        VMSTATE_UINT32_ARRAY(regs, IMXSPIState, ECSPI_MAX),
> -        VMSTATE_END_OF_LIST()
> -    },
> -};
> -
>  static void imx_spi_txfifo_reset(IMXSPIState *s)
>  {
>      fifo32_reset(&s->tx_fifo);
> @@ -119,6 +106,32 @@ static void imx_spi_update_irq(IMXSPIState *s)
>      DPRINTF("IRQ level is %d\n", level);
>  }
>
> +static int imx_spi_post_load(void *opaque, int version_id)
> +{
> +    IMXSPIState *s = (IMXSPIState *)opaque;
> +
> +    imx_spi_update_irq(s);
> +
> +    /* TODO: We should also restore the CS line level */

I don't think you should need to do anything here -- if
this device's state has been restored and the state at
the other end of the line has been restored, there's nothing
else to do. It's like reset, you don't want to be asserting
irq lines in post_load.

> +    if (imx_spi_is_multiple_master_burst(s)) {
> +        /*
> +         * If we are in multi master burst mode we need to call this
> +         * function at least 2 times before deselecting the CS line.
> +         * In practice the transfert ends (and the CS is deselected)
> +         * when the guest write 0 to the INT reg (according to linux
> +         * driver code).
> +         */
> +        s->txfifo_burst_count++;
> +    }

> @@ -380,6 +433,30 @@ static void imx_spi_write(void *opaque, hwaddr offset, 
> uint64_t value,
>          }
>
>          break;
> +    case ECSPI_INTREG:
> +        s->regs[index] = value;
> +
> +        /* Disable CS lines */
> +        if ((value == 0) && (s->txfifo_burst_count > 1)) {
> +            /*
> +             * When 0 is writen to the INT register (disabling all
> +             * interrupts) we need to deselect the device CS line.
> +             * But only if the txfifo function has been called at
> +             * least twice.
> +             * Note: This seems a bit hacky and I would preffer to
> +             * rely on something else to deselect the CS line. But it
> +             * works correctly with the linux driver.
> +             */
> +
> +            /* Deactivate the current CS line */
> +            qemu_set_irq(s->cs_lines[imx_spi_selected_channel(s)],
> +                         !imx_spi_current_channel_pol(s));
> +
> +            /* reset the txfifo count to 0 */
> +            s->txfifo_burst_count = 0;
> +        }

I think this is a bad idea. We have one or two devices which have
similar kinds of hacky code in them that knows what the Linux
driver does. The problem is then twofold:
 (1) if the Linux driver changes then the hacks break
 (2) it's almost impossible to remove the hacks later, because
     the reasoning behind them (which guest kernels are affected,
     etc) gets lost and you don't know whether you're going to
     break previously-working setups if you try to remove the
     hack code

I think we really should try to get this right from the start
(ie "behave like the hardware does").

thanks
-- PMM