Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

["Cédric Le Goater" <clg@kaod.org>]

Sorry for the late answer.

On 7/13/20 6:15 PM, Peter Maydell wrote:
> On Mon, 13 Jul 2020 at 15:19, Cédric Le Goater <clg@kaod.org> wrote:
>> On 7/10/20 1:33 PM, Peter Maydell wrote:
>>> Andrew, Cedric: do you have the datasheet for this device? Do you
>>> know if we should also be flagging the error back to the
>>> guest somehow?
>>
>> zero is the only invalid size of a transmit buffer and the specs does
>> not have any special information on which bit to raise in that case.
> 
> I found a datasheet which might or might not be the equivalent
> bit of hardware -- does your datasheet have a note on the
> TXBUF_SIZE field of a tx descriptor that says "When the size is 0,
> the descriptor would be discarded" ? (Though I found another
> random doc that just says it's illegal...)

I only have : 

  TXBUF SIZE: Transmit buffer size in byte
  The transmit buffer size can not be zero.

>> I think FTGMAC100_INT_NO_NPTXBUF (transmit buffer unavailable) is our
>> best option and we should add an extra 'len == 0' test in front of
>> the dma_memory_read() call to raise it. A zero length is not considered
>> bogus by dma_memory_read() it seems.
> 
> My best guess at "what the hardware does" here would be:
>  * TXBUF_SIZE in a tx descriptor can be anything: the h/w
>    would happily allow you to assemble a tx packet with a
>    whole series of 1-byte sized buffers, each with its own
>    tx descriptor

yes. 

>  * zero-byte tx descriptors might just be marked "done" and
>    skipped over since they have no actual data

yes.

>  * any checking on max/min lengths would be done
>    only on the accumulated total-packet length (we do this
>    this way already for the frame-too-big check)

yes.

>  * I suspect "transmit buffer unavailable" means "the ethernet
>    controller needs more data but the next tx descriptor
>    is still marked as owned by the guest" -- this is certainly
>    what we currently do with it, and that doesn't seem like
>    the best thing to signal for the "tx packet too small"
>    case. It's possible that the hardware simply sends out a
>    runt packet of some form if the software tells it to do
>    that. My vote would be for handling it with XPKT_LOST,
>    the same way we do for over-large frames. 

XPKT_LOST means that packets are lost due to late/excessive 
collision. I have used this status for large frames because
not other bits made sense.

>    This probably
>    is not what the hardware does but at least it's a
>    coherent thing that the guest might be expecting to have
>    happen for a tx attempt and it matches the fact that we
>    really are not going to put it on the 'wire'.

I agree.

> 
> Side note: I suspect that any failures from
> dma_memory_read() and dma_memory_write() should be
> reported as AHB_ERR (currently we have a mix of
> ignoring them or using NO_NPTXBUF).

AHB_ERR is not used in the Aspeed implementation. I checked with 
them. But I think it makes sense for other implementations, so we
can add this status for Linux.

NO_NPTXBUF means a lack a transmit descriptors.


XPKT_LOST is our best choice then.

Thanks,

C. 

> (It would in theory be possible to test some of these edge
> cases on real hardware, but that kind of bare-metal test
> case is usually a pain to put together and way overkill
> for this situation, so I don't think we should bother.)
> 
>> Is address zero considered bogus ?
>> If not, we need to check that also.
> 
> Writes to address 0 are fine, it is not a special physical address.
> 
> thanks
> -- PMM
>