(+ Alan)

On 23 September 2016 at 00:49, Daniil Egranov daniil.egranov@arm.com wrote:

1. Corrected Ethernet frame size to 1538.
2. Performance optimization. Removed extra CopyMem() call.

Could you split this in 2 patches please?

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com

---

NOTE: The driver requires [edk2] [PATCH 0/2] Juno PCI fixes

Drivers/Net/MarvellYukonDxe/if_msk.c | 22 +++++++++------------- Drivers/Net/MarvellYukonDxe/if_msk.h | 2 +- 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 6fb8af4..8e9b726 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1876,7 +1876,6 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m;

  DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n"));

@@ -1906,16 +1905,6 @@ msk_rxeof ( rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; #endif

• gBS->CopyMem (&m, &rxd->rx_m, sizeof(m));
• Status = msk_newbuf (sc_if, cons);
• if (EFI_ERROR (Status)) {

•  // This is a dropped packet, but we aren't counting drops
  

•  // Reuse old buffer
  

•  msk_discard_rxbuf (sc_if, cons);
  

What happened to dropping the packet in this case?

If you want to get rid of the gBS->CopyMem(), why not simply assign 'rxd->rx_m' to 'm' field by field? That should help the compiler optimize away the in-memory version of 'm' entirely

•  break;
  

• }
• Status = mPciIo->Flush (mPciIo); if (EFI_ERROR (Status)) { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n"));

@@ -1932,14 +1921,21 @@ msk_rxeof ( if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_SYSTEM_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE;

•  m_link->SystemBuf.Buf = m.Buf;
  

•  m_link->SystemBuf.Buf = rxd->rx_m.Buf;
   m_link->SystemBuf.Length = len;
  
   InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link);
  

  } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n"));

•  gBS->FreePool (m.Buf);
  

•  gBS->FreePool (rxd->rx_m.Buf);
  

  }

• Status = msk_newbuf (sc_if, cons);

• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMA buffer\n"));
  

•  break;
  

• }

• } while (0);

  MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.h b/Drivers/Net/MarvellYukonDxe/if_msk.h index 66513d3..64ddeac 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.h +++ b/Drivers/Net/MarvellYukonDxe/if_msk.h @@ -15,7 +15,7 @@

#include <Uefi.h>

-#define MAX_SUPPORTED_PACKET_SIZE (1500) +#define MAX_SUPPORTED_PACKET_SIZE (1538)

This looks reasonable, but could you still explain why you need to make this change?

EFI_STATUS mskc_probe (EFI_PCI_IO_PROTOCOL *PciIo);

-- 2.7.4

On 24 September 2016 at 02:00, Daniil Egranov daniil.egranov@arm.com wrote:

On 09/23/2016 03:10 AM, Ard Biesheuvel wrote:

...

(+ Alan)

On 23 September 2016 at 00:49, Daniil Egranov daniil.egranov@arm.com wrote:

...

1. Corrected Ethernet frame size to 1538.
2. Performance optimization. Removed extra CopyMem() call.

Could you split this in 2 patches please?

Will do.

...

...

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com

---

NOTE: The driver requires [edk2] [PATCH 0/2] Juno PCI fixes

Drivers/Net/MarvellYukonDxe/if_msk.c | 22 +++++++++------------- Drivers/Net/MarvellYukonDxe/if_msk.h | 2 +- 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 6fb8af4..8e9b726 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1876,7 +1876,6 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m;

  DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n"));

@@ -1906,16 +1905,6 @@ msk_rxeof ( rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; #endif

• gBS->CopyMem (&m, &rxd->rx_m, sizeof(m));
• Status = msk_newbuf (sc_if, cons);
• if (EFI_ERROR (Status)) {

•  // This is a dropped packet, but we aren't counting drops
  

•  // Reuse old buffer
  

•  msk_discard_rxbuf (sc_if, cons);
  

What happened to dropping the packet in this case?

I am going to review this part again. I guess we have an assumption that the driver will drop received messages in case of memory allocation issue but keep going.

No, it allocates a new buffer (and hands it to the device) before handling the contents of the old buffer. The old buffer is simply reused by the device, and not processed any further, if the packet needs to be dropped for any reason.

...

If you want to get rid of the gBS->CopyMem(), why not simply assign 'rxd->rx_m' to 'm' field by field? That should help the compiler optimize away the in-memory version of 'm' entirely

Will copping the receive buffer pointer to the queue buffer pointer be a more optimal way?

No, you need 'm' to store the values between the time that you allocate a new buffer for the device and process the contents of the old one.

...

...

•  break;
  

• }

• Status = mPciIo->Flush (mPciIo);
  if (EFI_ERROR (Status)) {
    DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n"));
  

@@ -1932,14 +1921,21 @@ msk_rxeof ( if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_SYSTEM_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE;

•  m_link->SystemBuf.Buf = m.Buf;
  

•  m_link->SystemBuf.Buf = rxd->rx_m.Buf;
    m_link->SystemBuf.Length = len;
  
    InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link);
  } else {
    DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive
  

buffer link. Dropping Frame\n"));

•  gBS->FreePool (m.Buf);
  

•  gBS->FreePool (rxd->rx_m.Buf);
  }
  

• Status = msk_newbuf (sc_if, cons);
• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMA
  

buffer\n"));

•  break;
  

• }

• } while (0);

  MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.h b/Drivers/Net/MarvellYukonDxe/if_msk.h index 66513d3..64ddeac 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.h +++ b/Drivers/Net/MarvellYukonDxe/if_msk.h @@ -15,7 +15,7 @@

#include <Uefi.h>

-#define MAX_SUPPORTED_PACKET_SIZE (1500) +#define MAX_SUPPORTED_PACKET_SIZE (1538)

This looks reasonable, but could you still explain why you need to make this change?

With 1500 size tftp transfer gets messed up. I missed UDP part here. The driver receives Layer 2 Ethernet frame with 18 (Ethernet) + 20 (IPv4) + 8 (UDP) + 1500 (max payload) = 1546. If driver will support jumbo frames, it should be changed accordingly. Do we care about frame size for IPv6 case?

IPv6 yes. Jumbo frames no, since, looking at the original driver, this is not trivial to implement.

On 09/24/2016 10:40 AM, Alan Ott wrote:

On 09/24/2016 08:02 AM, Ard Biesheuvel wrote:

...

On 24 September 2016 at 02:00, Daniil Egranov daniil.egranov@arm.com wrote:

...

On 09/23/2016 03:10 AM, Ard Biesheuvel wrote:

...

(+ Alan)

On 23 September 2016 at 00:49, Daniil Egranov daniil.egranov@arm.com wrote:

...

1. Corrected Ethernet frame size to 1538.
2. Performance optimization. Removed extra CopyMem() call.

Could you split this in 2 patches please?

Will do.

...

...

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com

---

NOTE: The driver requires [edk2] [PATCH 0/2] Juno PCI fixes

Drivers/Net/MarvellYukonDxe/if_msk.c | 22 +++++++++------------- Drivers/Net/MarvellYukonDxe/if_msk.h | 2 +- 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 6fb8af4..8e9b726 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1876,7 +1876,6 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m;

  DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n"));

@@ -1906,16 +1905,6 @@ msk_rxeof ( rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; #endif

• gBS->CopyMem (&m, &rxd->rx_m, sizeof(m));
• Status = msk_newbuf (sc_if, cons);
• if (EFI_ERROR (Status)) {

•  // This is a dropped packet, but we aren't counting drops
  

•  // Reuse old buffer
  

•  msk_discard_rxbuf (sc_if, cons);
  

What happened to dropping the packet in this case?

I am going to review this part again. I guess we have an assumption that the driver will drop received messages in case of memory allocation issue but keep going.

No, it allocates a new buffer (and hands it to the device) before handling the contents of the old buffer. The old buffer is simply reused by the device, and not processed any further, if the packet needs to be dropped for any reason.

The reason to drop a packet will be a new buffer memory allocation error. If msk_newbuf failed, it will brake out from the loop and will skip current packet.

Status = msk_newbuf (sc_if, cons); if (EFI_ERROR (Status)) { // This is a dropped packet, but we aren't counting drops // Reuse old buffer msk_discard_rxbuf (sc_if, cons); break; } If memory allocation will constantly failed, the driver will constantly drop packets. The original code did not have that issue as the receive ring buffer has been preallocated during the driver initialization and almost never changed.

Yes, that's true. msk_newbuf() overwrites rxd, so the m copy is necessary. It's the way the original FreeBSD driver does it.

I don't see much reason to deviate significantly from the original driver in this case. In cases where we do deviate, there should be a good reason. The original FreeBSD driver is well-proven across many different Marvell parts and architectures, and while it might be tempting to do some refactoring, I don't believe the benefits outweigh the risks. There are more Marvell parts than we have the ability to test, and the same is true with architectures.

There are a lot of places have been changed including dynamic receive ring buffer allocation and all UEFI changes so it's already pretty far deviated from the original FreeBSD driver. I do not see any reason why we should not add any improvements to the code.

...

...

...

If you want to get rid of the gBS->CopyMem(), why not simply assign 'rxd->rx_m' to 'm' field by field? That should help the compiler optimize away the in-memory version of 'm' entirely

Will copping the receive buffer pointer to the queue buffer pointer be a more optimal way?

No, you need 'm' to store the values between the time that you allocate a new buffer for the device and process the contents of the old one.

This is correct. The m is necessary. See the FreeBSD driver. It's even called "m".

https://github.com/freebsd/freebsd/blob/master/sys/dev/msk/if_msk.c#L3181

You can avoid extra copy by allocating the linked list structure as a first thing and assign buffer's pointer without using "m".

...

...

...

...

•  break;
  

• }

•  Status = mPciIo->Flush (mPciIo);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n"));
  

@@ -1932,14 +1921,21 @@ msk_rxeof ( if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_SYSTEM_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE;

•  m_link->SystemBuf.Buf = m.Buf;
  

•  m_link->SystemBuf.Buf = rxd->rx_m.Buf;
     m_link->SystemBuf.Length = len;
  
     InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link);
   } else {
     DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate 
  

receive buffer link. Dropping Frame\n"));

•  gBS->FreePool (m.Buf);
  

•  gBS->FreePool (rxd->rx_m.Buf);
   }
  

• Status = msk_newbuf (sc_if, cons);
• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMA
  

buffer\n"));

•  break;
  

• }

• } while (0);

  MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.h b/Drivers/Net/MarvellYukonDxe/if_msk.h index 66513d3..64ddeac 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.h +++ b/Drivers/Net/MarvellYukonDxe/if_msk.h @@ -15,7 +15,7 @@

#include <Uefi.h>

-#define MAX_SUPPORTED_PACKET_SIZE (1500) +#define MAX_SUPPORTED_PACKET_SIZE (1538)

This looks reasonable, but could you still explain why you need to make this change?

With 1500 size tftp transfer gets messed up. I missed UDP part here. The driver receives Layer 2 Ethernet frame with 18 (Ethernet) + 20 (IPv4) + 8 (UDP) + 1500 (max payload) = 1546. If driver will support jumbo frames, it should be changed accordingly. Do we care about frame size for IPv6 case?

IPv6 yes. Jumbo frames no, since, looking at the original driver, this is not trivial to implement.

The 1500 is the max payload for ethernet, not for UDP. The UDP headers don't enter into it. The number you are looking for is 1518 (1500+6+6+4+2). https://en.wikipedia.org/wiki/Ethernet_frame#Structure

That's incorrect. It's Ethernet II frame type, not 802.3 http://support.netrounds.com/hc/en-us/articles/201911666-Layer-2-Ethernet-fr...

I verified communication between Linux and driver with Wireshark and 46 bytes header is used with 18 to 1500 payload so the maximum frame size for IPv4 will be 1546. For IPv6, the header will be 66 bytes with payload of 66 to 1500 bytes so the maximum frame size for IPv6 will be 1566.

Jumbo support is in the original BSD driver, but I agree that there's no reason to do it in UEFI. Let's not lose sight of the fact that this is firmware. Gigabit networking with 1500-byte frames should provide enough bandwidth.

Alan.

---

Linaro-uefi mailing list Linaro-uefi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-uefi

On 23 September 2016 at 00:49, Daniil Egranov daniil.egranov@arm.com wrote:

Added NullDmaLib needed for ArmJunoPkg/Drivers/PciHostBridgeDxe with the fix for PCI DMA issue on Juno.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com

---

Platforms/ARM/Juno/ArmJuno.dsc | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/Platforms/ARM/Juno/ArmJuno.dsc b/Platforms/ARM/Juno/ArmJuno.dsc index 751225a..4456ff6 100644 --- a/Platforms/ARM/Juno/ArmJuno.dsc +++ b/Platforms/ARM/Juno/ArmJuno.dsc @@ -52,6 +52,8 @@ # USB Requirements UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf

• NullDmaLib|EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf

Is the PCI host bridge driver the only user of DmaLib on Juno? If not, are all other PCI bus masters coherent as well?

On 09/23/2016 07:08 AM, Ard Biesheuvel wrote:

On 23 September 2016 at 09:02, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:

...

On 23 September 2016 at 00:49, Daniil Egranov daniil.egranov@arm.com wrote:

...

Added NullDmaLib needed for ArmJunoPkg/Drivers/PciHostBridgeDxe with the fix for PCI DMA issue on Juno.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com

---

Platforms/ARM/Juno/ArmJuno.dsc | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/Platforms/ARM/Juno/ArmJuno.dsc b/Platforms/ARM/Juno/ArmJuno.dsc index 751225a..4456ff6 100644 --- a/Platforms/ARM/Juno/ArmJuno.dsc +++ b/Platforms/ARM/Juno/ArmJuno.dsc @@ -52,6 +52,8 @@ # USB Requirements UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf

• NullDmaLib|EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf

Is the PCI host bridge driver the only user of DmaLib on Juno? If not, are all other PCI bus masters coherent as well?

Looking at ArmJunoDxe, it seems that the PCI emulation for USB also uses this library. Are those DMA coherent on Juno?

The USB/EHCI controller is not coherent on Juno (its not PCIe attached). Of course if someone plugs say a XHCI controller in, that should be coherent.