Re: [PATCH-for-8.2 v2 1/2] hw/net/can/xlnx-zynqmp: Avoid underflow while popping TX FIFOs

[Vikram Garhwal <vikram.garhwal@amd.com>]

Hi,
On Sun, Nov 19, 2023 at 11:51:01PM +0100, Philippe Mathieu-Daudé wrote:
> Per https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm/Message-Format
> 
>   Message Format
> 
>   The same message format is used for RXFIFO, TXFIFO, and TXHPB.
>   Each message includes four words (16 bytes). Software must read
>   and write all four words regardless of the actual number of data
>   bytes and valid fields in the message.
> 
> There is no mention in this reference manual about what the
> hardware does when not all four words are written. To fix the
> reported underflow behavior when DATA2 register is written,
> I choose to fill the data with the previous content of the
> ID / DLC / DATA1 registers, which is how I expect hardware
> would do.
> 
> Note there is no hardware flag raised under such condition.
> 
> Reported-by: Qiang Liu <cyruscyliu@gmail.com>
> Fixes: 98e5d7a2b7 ("hw/net/can: Introduce Xilinx ZynqMP CAN controller")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1425
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Francisco Iglesias <francisco.iglesias@amd.com>
> ---
>  hw/net/can/xlnx-zynqmp-can.c | 49 +++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
> index e93e6c5e19..58938b574e 100644
> --- a/hw/net/can/xlnx-zynqmp-can.c
> +++ b/hw/net/can/xlnx-zynqmp-can.c
> @@ -434,6 +434,51 @@ static bool tx_ready_check(XlnxZynqMPCANState *s)
>      return true;
>  }
>  
> +static void read_tx_frame(XlnxZynqMPCANState *s, Fifo32 *fifo, uint32_t *data)
> +{
> +    unsigned used = fifo32_num_used(fifo);
> +    bool is_txhpb = fifo == &s->txhpb_fifo;
> +
> +    assert(used > 0);
> +    used %= CAN_FRAME_SIZE;
> +
> +    /*
> +     * Frame Message Format
> +     *
> +     * Each frame includes four words (16 bytes). Software must read and write
> +     * all four words regardless of the actual number of data bytes and valid
> +     * fields in the message.
> +     * If software misbehave (not writting all four words), we use the previous
%s/writting/writing
> +     * registers content to initialize each missing word.
> +     */
> +    if (used > 0) {
> +        /* ID, DLC, DATA1 missing */
Code is correct but i feel This comment is confusing. ID is missing for sure
if user > 0 but same is not the case for DLC and DATA1.

> +        data[0] = s->regs[is_txhpb ? R_TXHPB_ID : R_TXFIFO_ID];
> +    } else {
> +        data[0] = fifo32_pop(fifo);
> +    }
> +    if (used == 1 || used == 2) {
> +        /* DLC, DATA1 missing */
Same here DLC is missing for sure but DATA1 is not for used == 2.
> +        data[1] = s->regs[is_txhpb ? R_TXHPB_DLC : R_TXFIFO_DLC];
> +    } else {
> +        data[1] = fifo32_pop(fifo);
> +    }
> +    if (used == 1) {
> +        /* DATA1 missing */
May be we remove all these individual comments and write a common comment before
the first check(if(used > 0)). Something like this:
/*
 * If used is 1 then ID, DLC and DATA1 are missing.
 *
 * if used is 2 then ID and DLC are missing.
 *
 * if used is 3 then only ID is missing.
 */

Code looks correct to me. So, With above minor changes in code comments:

Reviewed-by: Vikram Garhwal <vikram.garhwal@amd.com>

> +        data[2] = s->regs[is_txhpb ? R_TXHPB_DATA1 : R_TXFIFO_DATA1];
> +    } else {
> +        data[2] = fifo32_pop(fifo);
> +    }
> +    /* DATA2 triggered the transfer thus is always available */
> +    data[3] = fifo32_pop(fifo);
> +
> +    if (used) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Incomplete CAN frame (only %u/%u slots used)\n",
> +                      TYPE_XLNX_ZYNQMP_CAN, used, CAN_FRAME_SIZE);
> +    }
> +}
> +
>  static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32 *fifo)
>  {
>      qemu_can_frame frame;
> @@ -451,9 +496,7 @@ static void transfer_fifo(XlnxZynqMPCANState *s, Fifo32 *fifo)
>      }
>  
>      while (!fifo32_is_empty(fifo)) {
> -        for (i = 0; i < CAN_FRAME_SIZE; i++) {
> -            data[i] = fifo32_pop(fifo);
> -        }
> +        read_tx_frame(s, fifo, data);
>  
>          if (ARRAY_FIELD_EX32(s->regs, STATUS_REGISTER, LBACK)) {
>              /*
> -- 
> 2.41.0
>