[Qemu-devel] [PATCH 0/2] net: Fix lan9118 multi-buffer transmit

[Qemu-devel] [PATCH 0/2] net: Fix lan9118 multi-buffer transmit

[Roy Franz]

This patchset fixes some bugs in the lan9118 emulation that were found
while debugging UEFI network support on the emulated VExpress platform.

The 9118 ethernet controller supports transmission of multi-buffer packets 
with arbitrary byte alignment of the start and end bytes of each buffer.  
All writes to the packet fifo are 32 bits, so the controller discards 
bytes at the beginning and end of each buffer based on the 'Data start 
offset' and 'Buffer size' of the TX command 'A' word.  

The UEFI network driver uses 1 buffer for the packet header, and one for 
the payload.  The mishandling of the start offset and buffer length 
resulted in extra bytes being inserted in the packet.  

With these changes the UEFI network driver that works with the real 9118 
chip on the VExpress evaluation board works unmodified on QEMU.  The Linux 
kernel driver continues to work on QEMU as well.  Since the Linux driver 
was unaffected by these bugs I am inferring that it is using a single 
buffer for each packet.  



Roy Franz (2):
  Fix lan9118 TX "CMD A" handling
  Fix lan9118 buffer length handling

 hw/net/lan9118.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/2] net: Fix lan9118 TX "CMD A" handling

[Roy Franz]

The 9118 ethernet controller supports transmission of multi-buffer packets
with arbitrary byte alignment of the start and end bytes.  All writes to
the packet fifo are 32 bits, so the controller discards bytes at the beginning
and end of each buffer based on the 'Data start offset' and 'Buffer size'
of the TX command 'A' format.

This patch changes the buffer size and offset internal state variables to be
updated on every "TX command A" write.  Previously they were only updated for
the first segment, which resulted incorrect behavior for packets with more
than one segment. Each segment of the packet has its own CMD A command, with
its own buffer size and start offset.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 hw/net/lan9118.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 2315f99..c5d6f14 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -727,14 +727,14 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
         s->txp->cmd_a = val & 0x831f37ff;
         s->txp->fifo_used++;
         s->txp->state = TX_B;
+        s->txp->buffer_size = s->txp->cmd_a & 0x7ff;
+        s->txp->offset = (s->txp->cmd_a >> 16) & 0x1f;
         break;
     case TX_B:
         if (s->txp->cmd_a & 0x2000) {
             /* First segment */
             s->txp->cmd_b = val;
             s->txp->fifo_used++;
-            s->txp->buffer_size = s->txp->cmd_a & 0x7ff;
-            s->txp->offset = (s->txp->cmd_a >> 16) & 0x1f;
             /* End alignment does not include command words.  */
             n = (s->txp->buffer_size + s->txp->offset + 3) >> 2;
             switch ((n >> 24) & 3) {
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/2] net: Fix lan9118 buffer length handling

[Roy Franz]

The 9118 ethernet controller supports transmission of multi-buffer packets
with arbitrary byte alignment of the start and end bytes.  All writes to
the packet fifo are 32 bits, so the controller discards bytes at the beginning
and end of each buffer based on the 'Data start offset' and 'Buffer size'
of the TX command 'A' format.

This patch uses the provided buffer length to limit the bytes transmitted.
Previously all the bytes of the last 32-bit word written to the TX fifo
were added to the internal transmit buffer structure resulting in more bytes
being transmitted than were submitted to the hardware in the command.  This
resulted in extra bytes being inserted into the middle of multi-buffer
packets when the non-final buffers had non-32bit aligned ending addresses.

Signed-off-by: Roy Franz <roy.franz@linaro.org>
---
 hw/net/lan9118.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index c5d6f14..712bb41 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -773,11 +773,10 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
                in FIFO words.  Empirical results show it to be little-endian.
                */
             /* TODO: FIFO overflow checking.  */
-            while (n--) {
+            while (n-- && s->txp->buffer_size--) {
                 s->txp->data[s->txp->len] = val & 0xff;
                 s->txp->len++;
                 val >>= 8;
-                s->txp->buffer_size--;
             }
             s->txp->fifo_used++;
         }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/2] net: Fix lan9118 buffer length handling

[Peter Crosthwaite]

On Sat, Dec 21, 2013 at 4:26 AM, Roy Franz <roy.franz@linaro.org> wrote:
> The 9118 ethernet controller supports transmission of multi-buffer packets
> with arbitrary byte alignment of the start and end bytes.  All writes to
> the packet fifo are 32 bits, so the controller discards bytes at the beginning
> and end of each buffer based on the 'Data start offset' and 'Buffer size'
> of the TX command 'A' format.
>
> This patch uses the provided buffer length to limit the bytes transmitted.
> Previously all the bytes of the last 32-bit word written to the TX fifo
> were added to the internal transmit buffer structure resulting in more bytes
> being transmitted than were submitted to the hardware in the command.  This
> resulted in extra bytes being inserted into the middle of multi-buffer
> packets when the non-final buffers had non-32bit aligned ending addresses.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  hw/net/lan9118.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index c5d6f14..712bb41 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -773,11 +773,10 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
>                 in FIFO words.  Empirical results show it to be little-endian.
>                 */
>              /* TODO: FIFO overflow checking.  */
> -            while (n--) {
> +            while (n-- && s->txp->buffer_size--) {

can you just get n right in the first place (line 766):

n = MIN(4, s->txp_buffer_size);

rather than doing min calculation on two parallel iteration variables?

Regards,
Peter

>                  s->txp->data[s->txp->len] = val & 0xff;
>                  s->txp->len++;
>                  val >>= 8;
> -                s->txp->buffer_size--;
>              }
>              s->txp->fifo_used++;
>          }
> --
> 1.7.10.4
>
>

Re: [Qemu-devel] [PATCH 1/2] net: Fix lan9118 TX "CMD A" handling

[Peter Crosthwaite]

On Sat, Dec 21, 2013 at 4:26 AM, Roy Franz <roy.franz@linaro.org> wrote:
> The 9118 ethernet controller supports transmission of multi-buffer packets
> with arbitrary byte alignment of the start and end bytes.  All writes to
> the packet fifo are 32 bits, so the controller discards bytes at the beginning
> and end of each buffer based on the 'Data start offset' and 'Buffer size'
> of the TX command 'A' format.
>
> This patch changes the buffer size and offset internal state variables to be
> updated on every "TX command A" write.  Previously they were only updated for
> the first segment, which resulted incorrect behavior for packets with more
> than one segment. Each segment of the packet has its own CMD A command, with
> its own buffer size and start offset.
>
> Signed-off-by: Roy Franz <roy.franz@linaro.org>
> ---
>  hw/net/lan9118.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index 2315f99..c5d6f14 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -727,14 +727,14 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
>          s->txp->cmd_a = val & 0x831f37ff;
>          s->txp->fifo_used++;
>          s->txp->state = TX_B;
> +        s->txp->buffer_size = s->txp->cmd_a & 0x7ff;
> +        s->txp->offset = (s->txp->cmd_a >> 16) & 0x1f;

While you are touching this, you could change it to use the preferable
extract32 macro rather than the & >> logic.

Regards,
Peter

>          break;
>      case TX_B:
>          if (s->txp->cmd_a & 0x2000) {
>              /* First segment */
>              s->txp->cmd_b = val;
>              s->txp->fifo_used++;
> -            s->txp->buffer_size = s->txp->cmd_a & 0x7ff;
> -            s->txp->offset = (s->txp->cmd_a >> 16) & 0x1f;
>              /* End alignment does not include command words.  */
>              n = (s->txp->buffer_size + s->txp->offset + 3) >> 2;
>              switch ((n >> 24) & 3) {
> --
> 1.7.10.4
>
>

Re: [Qemu-devel] [PATCH 2/2] net: Fix lan9118 buffer length handling

[Roy Franz]

On Fri, Dec 27, 2013 at 5:25 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Sat, Dec 21, 2013 at 4:26 AM, Roy Franz <roy.franz@linaro.org> wrote:
>> The 9118 ethernet controller supports transmission of multi-buffer packets
>> with arbitrary byte alignment of the start and end bytes.  All writes to
>> the packet fifo are 32 bits, so the controller discards bytes at the beginning
>> and end of each buffer based on the 'Data start offset' and 'Buffer size'
>> of the TX command 'A' format.
>>
>> This patch uses the provided buffer length to limit the bytes transmitted.
>> Previously all the bytes of the last 32-bit word written to the TX fifo
>> were added to the internal transmit buffer structure resulting in more bytes
>> being transmitted than were submitted to the hardware in the command.  This
>> resulted in extra bytes being inserted into the middle of multi-buffer
>> packets when the non-final buffers had non-32bit aligned ending addresses.
>>
>> Signed-off-by: Roy Franz <roy.franz@linaro.org>
>> ---
>>  hw/net/lan9118.c |    3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
>> index c5d6f14..712bb41 100644
>> --- a/hw/net/lan9118.c
>> +++ b/hw/net/lan9118.c
>> @@ -773,11 +773,10 @@ static void tx_fifo_push(lan9118_state *s, uint32_t val)
>>                 in FIFO words.  Empirical results show it to be little-endian.
>>                 */
>>              /* TODO: FIFO overflow checking.  */
>> -            while (n--) {
>> +            while (n-- && s->txp->buffer_size--) {
>
> can you just get n right in the first place (line 766):
>
> n = MIN(4, s->txp_buffer_size);
>
> rather than doing min calculation on two parallel iteration variables?
>
> Regards,
> Peter
>
>>                  s->txp->data[s->txp->len] = val & 0xff;
>>                  s->txp->len++;
>>                  val >>= 8;
>> -                s->txp->buffer_size--;
>>              }
>>              s->txp->fifo_used++;
>>          }
>> --
>> 1.7.10.4
>>
>>

Hi Peter,

   I'll take a look at this (and your other comments on the extract32)
when I am back at work next week.

Thanks,
Roy