| Message ID | 20231115151711.89170-1-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATCH-for-8.2] hw/net/can/xlnx-zynqmp: Avoid underflow while popping TX FIFO | expand |
Hi Philippe, good catch! On 2023-11-15 16:17, 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> > --- > Tested with the CAN tests from 'make check-qtest-aarch64' > --- > 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); For the case when there are multiple frames in the fifo we need to swap above to: unsigned used = fifo32_num_used(fifo) > CAN_FRAME_SIZE ? 0 : fifo32_num_used(fifo); With above minor modification: Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com> Best regards, Francisco > + 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 > + * registers content to initialize each missing word. > + */ > + if (used > 0) { > + /* ID, DLC, DATA1 missing */ > + 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 */ > + data[1] = s->regs[is_txhpb ? R_TXHPB_DLC : R_TXFIFO_DLC]; > + } else { > + data[1] = fifo32_pop(fifo); > + } > + if (used == 1) { > + /* DATA1 missing */ > + 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)) { > /*
Hi Francisco, On 16/11/23 15:17, Francisco Iglesias wrote: > Hi Philippe, good catch! Well this was fuzzed by Qiang Liu. > On 2023-11-15 16:17, 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> >> --- >> Tested with the CAN tests from 'make check-qtest-aarch64' >> --- >> 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); > > For the case when there are multiple frames in the fifo we need to swap > above to: > > unsigned used = fifo32_num_used(fifo) > CAN_FRAME_SIZE ? 0 : > fifo32_num_used(fifo); Isn't this ... > With above minor modification: > > Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com> > > Best regards, > Francisco > >> + bool is_txhpb = fifo == &s->txhpb_fifo; >> + >> + assert(used > 0); >> + used %= CAN_FRAME_SIZE; ... done here? >> + /* >> + * 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 >> + * registers content to initialize each missing word. >> + */ >> + if (used > 0) { >> + /* ID, DLC, DATA1 missing */ >> + 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 */ >> + data[1] = s->regs[is_txhpb ? R_TXHPB_DLC : R_TXFIFO_DLC]; >> + } else { >> + data[1] = fifo32_pop(fifo); >> + } >> + if (used == 1) { >> + /* DATA1 missing */ >> + 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); >> + } >> +}
Hi Philippe, On 2023-11-16 16:44, Philippe Mathieu-Daudé wrote: > Hi Francisco, > > On 16/11/23 15:17, Francisco Iglesias wrote: >> Hi Philippe, good catch! > > Well this was fuzzed by Qiang Liu. > >> On 2023-11-15 16:17, 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> >>> --- >>> Tested with the CAN tests from 'make check-qtest-aarch64' >>> --- >>> 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); >> >> For the case when there are multiple frames in the fifo we need to >> swap above to: >> >> unsigned used = fifo32_num_used(fifo) > CAN_FRAME_SIZE ? 0 : >> fifo32_num_used(fifo); > > Isn't this ... > >> With above minor modification: >> >> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com> >> >> Best regards, >> Francisco >> >>> + bool is_txhpb = fifo == &s->txhpb_fifo; >>> + >>> + assert(used > 0); >>> + used %= CAN_FRAME_SIZE; > > ... done here? Ah yes, I was thinking that the first frame would be correct if for example used == 6, but yes that is not possible to know (can be the second frame that is ok). Feel free to add my reviewed-by to patch. Thanks, Best regards, Francisco > >>> + /* >>> + * 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 >>> + * registers content to initialize each missing word. >>> + */ >>> + if (used > 0) { >>> + /* ID, DLC, DATA1 missing */ >>> + 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 */ >>> + data[1] = s->regs[is_txhpb ? R_TXHPB_DLC : R_TXFIFO_DLC]; >>> + } else { >>> + data[1] = fifo32_pop(fifo); >>> + } >>> + if (used == 1) { >>> + /* DATA1 missing */ >>> + 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); >>> + } >>> +} >
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 + * registers content to initialize each missing word. + */ + if (used > 0) { + /* ID, DLC, DATA1 missing */ + 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 */ + data[1] = s->regs[is_txhpb ? R_TXHPB_DLC : R_TXFIFO_DLC]; + } else { + data[1] = fifo32_pop(fifo); + } + if (used == 1) { + /* DATA1 missing */ + 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)) { /*
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> --- Tested with the CAN tests from 'make check-qtest-aarch64' --- hw/net/can/xlnx-zynqmp-can.c | 49 +++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-)