| Message ID | 20231119225102.49227-3-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/net/can/xlnx-zynqmp: Avoid underflow while popping FIFOs | expand |
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 >
On Wed, Nov 22, 2023 at 08:45:56PM +0100, Francisco Iglesias wrote: > 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> Reviewed-by: Vikram Garhwal <vikram.garhwal@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 > >
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,
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> --- hw/net/can/xlnx-zynqmp-can.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)