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

[Francisco Iglesias <francisco.iglesias@amd.com>]

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);
>>> +    }
>>> +}
>