Re: [QEMU][PATCH v6 4/4] tests/qtest: Introduce tests for Xilinx VERSAL CANFD controller

[Peter Maydell]

On Tue, 30 May 2023 at 22:23, Vikram Garhwal <vikram.garhwal@amd.com> wrote:
>
> The QTests perform three tests on the Xilinx VERSAL CANFD controller:
>     Tests the CANFD controllers in loopback.
>     Tests the CANFD controllers in normal mode with CAN frame.
>     Tests the CANFD controllers in normal mode with CANFD frame.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Hi; Coverity has spotted some issues with this test code; could
you investigate and send followup patches, please ?


> +static void match_rx_tx_data(const uint32_t *buf_tx, const uint32_t *buf_rx,
> +                             bool is_canfd_frame)
> +{
> +    uint16_t size = 0;
> +    uint8_t len = CAN_FRAME_SIZE;
> +
> +    if (is_canfd_frame) {
> +        len = CANFD_FRAME_SIZE;
> +    }

Here len is either 4 (if !is_canfd_frame) or 18 (if is_canfd_frame)...

> +
> +    while (size < len) {

...and we loop with size always less than len...

> +        if (R_RX0_ID_OFFSET + 4 * size == R_RX0_DLC_OFFSET)  {
> +            g_assert_cmpint((buf_rx[size] & DLC_FD_BIT_MASK), ==,
> +                            (buf_tx[size] & DLC_FD_BIT_MASK));
> +        } else {
> +            if (!is_canfd_frame && size == 4) {

...so here this condition can never be true: if !is_canfd_frame
then we know size is less than 4.

What was the intention here ?

(CID 1512900)

> +                break;
> +            }
> +
> +            g_assert_cmpint(buf_rx[size], ==, buf_tx[size]);
> +        }
> +
> +        size++;
> +    }
> +}
> +/*
> + * Xilinx CANFD supports both CAN and CANFD frames. This test will be
> + * transferring CAN frame i.e. 8 bytes of data from CANFD0 and CANFD1 through
> + * canbus. CANFD0 initiate the data transfer to can-bus, CANFD1 receives the
> + * data. Test compares the can frame data sent from CANFD0 and received on
> + * CANFD1.
> + */
> +static void test_can_data_transfer(void)
> +{
> +    uint32_t buf_tx[CAN_FRAME_SIZE] = { 0x5a5bb9a4, 0x80000000,
> +                                        0x12345678, 0x87654321 };
> +    uint32_t buf_rx[CAN_FRAME_SIZE] = { 0x00, 0x00, 0x00, 0x00 };

The buf_rx[] array here is only 4 bytes long...

> +    uint32_t status = 0;
> +
> +    generate_random_data(buf_tx, false);
> +
> +    QTestState *qts = qtest_init("-machine xlnx-versal-virt"
> +                " -object can-bus,id=canbus"
> +                " -machine canbus0=canbus"
> +                " -machine canbus1=canbus"
> +                );
> +
> +    configure_canfd(qts, MSR_NORMAL_MODE);
> +
> +    /* Check if CANFD0 and CANFD1 are in Normal mode. */
> +    status = qtest_readl(qts, CANFD0_BASE_ADDR + R_SR_OFFSET);
> +    status = status & STATUS_REG_MASK;
> +    g_assert_cmpint(status, ==, STATUS_NORMAL_MODE);
> +
> +    status = qtest_readl(qts, CANFD1_BASE_ADDR + R_SR_OFFSET);
> +    status = status & STATUS_REG_MASK;
> +    g_assert_cmpint(status, ==, STATUS_NORMAL_MODE);
> +
> +    write_data(qts, CANFD0_BASE_ADDR, buf_tx, false);
> +
> +    send_data(qts, CANFD0_BASE_ADDR);
> +    read_data(qts, CANFD1_BASE_ADDR, buf_rx);

...but read_data() will write up to 17 bytes of data to the buffer,
if the incoming data from the device claims it to be a canfd frame.
The device shouldn't really do that, but the point of a test is
that the device might not be functioning correctly, so we should
size buf_rx[] large enough to fit whatever read_data() writes to it.

(CID 1512899)

> +    match_rx_tx_data(buf_tx, buf_rx, false);
> +
> +    qtest_quit(qts);
> +}

thanks
-- PMM