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

[Francisco Iglesias]

On Sun, Nov 19, 2023 at 11:51:02PM +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 read. To fix the
> reported underflow behavior, I choose to fill the 4 frame data
> registers when the first register (ID) is accessed, which is how
> I expect hardware would do.
> 
> 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/1427
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>


> ---
>  hw/net/can/xlnx-zynqmp-can.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
> index 58938b574e..c63fb4a83c 100644
> --- a/hw/net/can/xlnx-zynqmp-can.c
> +++ b/hw/net/can/xlnx-zynqmp-can.c
> @@ -777,14 +777,18 @@ static void update_rx_fifo(XlnxZynqMPCANState *s, const 
> qemu_can_frame *frame)
>      }
>  }
>  
> -static uint64_t can_rxfifo_pre_read(RegisterInfo *reg, uint64_t val)
> +static uint64_t can_rxfifo_post_read_id(RegisterInfo *reg, uint64_t val)
>  {
>      XlnxZynqMPCANState *s = XLNX_ZYNQMP_CAN(reg->opaque);
> +    unsigned used = fifo32_num_used(&s->rx_fifo);
>  
> -    if (!fifo32_is_empty(&s->rx_fifo)) {
> -        val = fifo32_pop(&s->rx_fifo);
> -    } else {
> +    if (used < CAN_FRAME_SIZE) {
>          ARRAY_FIELD_DP32(s->regs, INTERRUPT_STATUS_REGISTER, RXUFLW, 1);
> +    } else {
> +        val = s->regs[R_RXFIFO_ID] = fifo32_pop(&s->rx_fifo);
> +        s->regs[R_RXFIFO_DLC] = fifo32_pop(&s->rx_fifo);
> +        s->regs[R_RXFIFO_DATA1] = fifo32_pop(&s->rx_fifo);
> +        s->regs[R_RXFIFO_DATA2] = fifo32_pop(&s->rx_fifo);
>      }
>  
>      can_update_irq(s);
> @@ -945,14 +949,11 @@ static const RegisterAccessInfo can_regs_info[] = {
>          .post_write = can_tx_post_write,
>      },{ .name = "RXFIFO_ID",  .addr = A_RXFIFO_ID,
>          .ro = 0xffffffff,
> -        .post_read = can_rxfifo_pre_read,
> +        .post_read = can_rxfifo_post_read_id,
>      },{ .name = "RXFIFO_DLC",  .addr = A_RXFIFO_DLC,
>          .rsvd = 0xfff0000,
> -        .post_read = can_rxfifo_pre_read,
>      },{ .name = "RXFIFO_DATA1",  .addr = A_RXFIFO_DATA1,
> -        .post_read = can_rxfifo_pre_read,
>      },{ .name = "RXFIFO_DATA2",  .addr = A_RXFIFO_DATA2,
> -        .post_read = can_rxfifo_pre_read,
>      },{ .name = "AFR",  .addr = A_AFR,
>          .rsvd = 0xfffffff0,
>          .post_write = can_filter_enable_post_write,
> -- 
> 2.41.0
>