Re: [PATCH-for-9.0? 2/2] hw/net/lan9118: Fix overflow in TX FIFO

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 8/4/24 16:24, Peter Maydell wrote:
> On Mon, 8 Apr 2024 at 11:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> When the TX FIFO is full, raise the TX Status FIFO Overflow (TXSO)
>> flag, "Generated when the TX Status FIFO overflows" [*].
> 
> This doesn't sound right. The TX Status FIFO and the
> TX Data FIFO are separate FIFOs, and the TX FIFO has its own
> overflow bit, TDFO. And I think the overflow here is of
> a third FIFO, the MIL's transmit FIFO...
> 
>> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
>> index 7be0430ac5..7a1367b0bb 100644
>> --- a/hw/net/lan9118.c
>> +++ b/hw/net/lan9118.c
>> @@ -795,8 +795,11 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
>>               /* Documentation is somewhat unclear on the ordering of bytes
>>                  in FIFO words.  Empirical results show it to be little-endian.
>>                  */
>> -            /* TODO: FIFO overflow checking.  */
>>               while (n--) {
>> +                if (s->txp->len == PKT_SIZE) {
>> +                    s->int_sts |= TXSO_INT;
>> +                    break;
>> +                }
> 
> While I was looking at this bug, I realised that we have serious
> confusion about whether any of the variables we use to track FIFO
> size and FIFO usage are word counts or byte counts.
> 
> Looking at table 5-3 in the data sheet, the size of these
> FIFOs is actually software-configurable in the HW_CFG register,
> but we don't implement that and (attempt to) only provide
> the default configuration setting of TX_FIF_SZ == 5. That
> should mean:
>   TX data FIFO size == 4608 bytes == 1152 words
>   RX data FIFO size == 10560 bytes == 2640 words
>   TX status FIFO size == 512 bytes == 128 words
>   RX status FIFO size == 704 bytes == 176 words
> 
> But we don't consistently use either word or byte units for the
> variables we use to track FIFO size and FIFO usage. For instance:
>   * we initialise s->tx_fifo_size to 4608, which is a byte count
>   * we initialise s->rx_status_fifo_size to 704, which is a byte count...
>   * ...and then three lines later override that to 176, which is a word
>     count!
>   * we generally simply increment the various fifo_used fields
>     when we push a word into the FIFOs, implying word counts
>   * we mostly do calculations assuming word counts
>   * calculations of the RX_FIFO_INF and TX_FIFO_INF fields
>     (which report the used space in words and the free space
>     in bytes) are confused about units too
>   * the tx_status_fifo[] array is 512 words long and the bounds
>     checks assume 512 is a word count, but it is a byte count
>   * the rx_status_fifo[] array is 896 words long, but the worst
>     case RX status FIFO size is 896 bytes, even if we allowed
>     runtime adjustable FIFO sizes
>   * the rx_fifo[] array, on the other hand, is 3360 words long,
>     which really is the max possible size in words
> 
> Anyway, I think that txp->data[] is effectively modelling
> the "2K Byte transmit FIFO" within the MIL, not the TX FIFO.
> (We don't need to model the TX FIFO itself, because we don't
> do asynchronous sending of data packets: as soon as we've
> accumulated a complete packet into the MIL TX FIFO, we
> send it out. In real hardware the guest can put multiple
> packets into the TX data FIFO, which is why it makes sense to be
> able to configure a TX data FIFO size larger than the largest
> possible packet and larger than the MIL TX FIFO.)
> 
> So the limit that we are enforcing here is similar to the one
> described in the "Calculating Worst-Case TX FIFO (MIL) usage",
> except that we don't actually use data space for the gaps
> caused by unaligned buffers. So this can only overflow if the
> packet is greater than what the data sheet says is the
> maximum size of 1514 bytes. The datasheet unfortunately doesn't
> describe the behaviour if this maximum is exceeded, and our
> current code doesn't try to check it (it's in the "command B"
> words, which are all supposed to match in the case of a
> fragmented packet, and which we also don't check).
> 
> The most plausible behaviour to take I think is to raise
> TXE when we would overflow the s->txp_data[] buffer; there are
> various conditions described for when TXE is raised that seem
> like this would fit in reasonably with them.
> (There is a status bit TDFO for "TX Data FIFO Overrun", which
> I think is probably only for overruns of the TX data FIFO,
> not the MIL's TX FIFO.)
> 
> Since the datasheet doesn't say if the packet should be
> dropped or truncated if it's invalid like this, I guess
> we can do whatever's easiest.
> 
>>                   s->txp->data[s->txp->len] = val & 0xff;
>>                   s->txp->len++;
>>                   val >>= 8;
> 
> Conclusion:
>   * we should raise TXE, not TXSO
>   * add a comment about what exactly is going on here
>   * we should try to clean up the confusion between words and
>     bytes, as a separate patch that isn't -stable/-9.0
>     material...

Thanks a lot for this very detailed analysis! v2 on the way.