Remove the hard-coded page size and instead use EFI_SIZE_TO_PAGES() to convert a number of bytes into a number of pages.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 12 ++++++------ Drivers/Net/MarvellYukonDxe/if_mskreg.h | 2 -- 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 2f10240..7979bf3 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1467,7 +1467,7 @@ msk_status_dma_alloc ( UINTN Length;

Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, - BYTES_TO_PAGES (MSK_STAT_RING_SZ), (VOID**)&mSoftc->msk_stat_ring, 0); + EFI_SIZE_TO_PAGES (MSK_STAT_RING_SZ), (VOID**)&mSoftc->msk_stat_ring, 0);

if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to allocate DMA'able memory for status ring\n")); @@ -1496,7 +1496,7 @@ msk_status_dma_free ( if (mSoftc->msk_stat_map) { mPciIo->Unmap (mPciIo, mSoftc->msk_stat_map); if (mSoftc->msk_stat_ring) { - mPciIo->FreeBuffer (mPciIo, BYTES_TO_PAGES (MSK_STAT_RING_SZ), mSoftc->msk_stat_ring); + mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (MSK_STAT_RING_SZ), mSoftc->msk_stat_ring); mSoftc->msk_stat_ring = NULL; } mSoftc->msk_stat_map = NULL; @@ -1516,7 +1516,7 @@ msk_txrx_dma_alloc ( EFI_STATUS Status;

Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, - BYTES_TO_PAGES (MSK_TX_RING_SZ), (VOID**)&sc_if->msk_rdata.msk_tx_ring, 0); + EFI_SIZE_TO_PAGES (MSK_TX_RING_SZ), (VOID**)&sc_if->msk_rdata.msk_tx_ring, 0);

if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to allocate DMA'able memory for Tx ring\n")); @@ -1535,7 +1535,7 @@ msk_txrx_dma_alloc ( ASSERT (Length == MSK_TX_RING_SZ);

Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, - BYTES_TO_PAGES (MSK_RX_RING_SZ), (VOID**)&sc_if->msk_rdata.msk_rx_ring, 0); + EFI_SIZE_TO_PAGES (MSK_RX_RING_SZ), (VOID**)&sc_if->msk_rdata.msk_rx_ring, 0);

if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to allocate DMA'able memory for Rx ring\n")); @@ -1582,7 +1582,7 @@ msk_txrx_dma_free ( if (sc_if->msk_cdata.msk_tx_ring_map) { mPciIo->Unmap (mPciIo, sc_if->msk_cdata.msk_tx_ring_map); if (sc_if->msk_rdata.msk_tx_ring) { - mPciIo->FreeBuffer (mPciIo, BYTES_TO_PAGES (MSK_TX_RING_SZ), sc_if->msk_rdata.msk_tx_ring); + mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (MSK_TX_RING_SZ), sc_if->msk_rdata.msk_tx_ring); sc_if->msk_rdata.msk_tx_ring = NULL; } sc_if->msk_cdata.msk_tx_ring_map = NULL; @@ -1592,7 +1592,7 @@ msk_txrx_dma_free ( if (sc_if->msk_cdata.msk_rx_ring_map) { mPciIo->Unmap (mPciIo, sc_if->msk_cdata.msk_rx_ring_map); if (sc_if->msk_rdata.msk_rx_ring) { - mPciIo->FreeBuffer (mPciIo, BYTES_TO_PAGES (MSK_RX_RING_SZ), sc_if->msk_rdata.msk_rx_ring); + mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (MSK_RX_RING_SZ), sc_if->msk_rdata.msk_rx_ring); sc_if->msk_rdata.msk_rx_ring = NULL; } sc_if->msk_cdata.msk_rx_ring_map = NULL; diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index 64a30a2..f0dd05e 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2275,8 +2275,6 @@ struct msk_stat_desc { #define MSK_MIN_FRAMELEN (ETHER_MIN_LEN - ETHER_CRC_LEN) */

-#define PAGE_SIZE 0x1000 -#define BYTES_TO_PAGES(BYTES) ((((BYTES) - 1) / PAGE_SIZE) + 1) #define htole32(x) (x) // All UEFI platforms are little endian #define le32toh(x) (x) #define ACPI_SPECFLAG_PREFETCHABLE 0x06

On 08/26/2016 09:41 AM, Leif Lindholm wrote:

On Thu, Aug 25, 2016 at 06:39:41PM -0400, Alan Ott wrote:

...

Remove the hard-coded page size and instead use EFI_SIZE_TO_PAGES() to convert a number of bytes into a number of pages.

Clearly an improvement, but given the minor confusion in (the edk2) naming, may be worth an addition in the commit message to clarify that the EFI_PAGE_SIZE is not the same as the processor page size. (I had a minute or two of thinking this could be unusable for EBC.)

Or more succinctly - could this be modified to: '... into a number of 4K EFI pages.' ?

If so (please confirm): Reviewed-by: Leif Lindholm leif.lindholm@linaro.org No need to resubmit, I can fix on commit.

This commit message change is fine with me. Thanks for the clarification.

Alan.

...

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 12 ++++++------ Drivers/Net/MarvellYukonDxe/if_mskreg.h | 2 -- 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 2f10240..7979bf3 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1467,7 +1467,7 @@ msk_status_dma_alloc ( UINTN Length; Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData,

•                               BYTES_TO_PAGES (MSK_STAT_RING_SZ), (VOID**)&mSoftc->msk_stat_ring, 0);
  

•                               EFI_SIZE_TO_PAGES (MSK_STAT_RING_SZ), (VOID**)&mSoftc->msk_stat_ring, 0);
  

if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to allocate DMA'able memory for status ring\n")); @@ -1496,7 +1496,7 @@ msk_status_dma_free ( if (mSoftc->msk_stat_map) { mPciIo->Unmap (mPciIo, mSoftc->msk_stat_map); if (mSoftc->msk_stat_ring) {

•  mPciIo->FreeBuffer (mPciIo, BYTES_TO_PAGES (MSK_STAT_RING_SZ), mSoftc->msk_stat_ring);
  

•  mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (MSK_STAT_RING_SZ), mSoftc->msk_stat_ring);
    mSoftc->msk_stat_ring = NULL;
  }
  mSoftc->msk_stat_map = NULL;
  

@@ -1516,7 +1516,7 @@ msk_txrx_dma_alloc ( EFI_STATUS Status; Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData,

•                               BYTES_TO_PAGES (MSK_TX_RING_SZ), (VOID**)&sc_if->msk_rdata.msk_tx_ring, 0);
  

•                               EFI_SIZE_TO_PAGES (MSK_TX_RING_SZ), (VOID**)&sc_if->msk_rdata.msk_tx_ring, 0);
  

if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to allocate DMA'able memory for Tx ring\n")); @@ -1535,7 +1535,7 @@ msk_txrx_dma_alloc ( ASSERT (Length == MSK_TX_RING_SZ); Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData,

•                               BYTES_TO_PAGES (MSK_RX_RING_SZ), (VOID**)&sc_if->msk_rdata.msk_rx_ring, 0);
  

•                               EFI_SIZE_TO_PAGES (MSK_RX_RING_SZ), (VOID**)&sc_if->msk_rdata.msk_rx_ring, 0);
  

if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to allocate DMA'able memory for Rx ring\n")); @@ -1582,7 +1582,7 @@ msk_txrx_dma_free ( if (sc_if->msk_cdata.msk_tx_ring_map) { mPciIo->Unmap (mPciIo, sc_if->msk_cdata.msk_tx_ring_map); if (sc_if->msk_rdata.msk_tx_ring) {

•  mPciIo->FreeBuffer (mPciIo, BYTES_TO_PAGES (MSK_TX_RING_SZ), sc_if->msk_rdata.msk_tx_ring);
  

•  mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (MSK_TX_RING_SZ), sc_if->msk_rdata.msk_tx_ring);
    sc_if->msk_rdata.msk_tx_ring = NULL;
  }
  sc_if->msk_cdata.msk_tx_ring_map = NULL;
  

@@ -1592,7 +1592,7 @@ msk_txrx_dma_free ( if (sc_if->msk_cdata.msk_rx_ring_map) { mPciIo->Unmap (mPciIo, sc_if->msk_cdata.msk_rx_ring_map); if (sc_if->msk_rdata.msk_rx_ring) {

•  mPciIo->FreeBuffer (mPciIo, BYTES_TO_PAGES (MSK_RX_RING_SZ), sc_if->msk_rdata.msk_rx_ring);
  

•  mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (MSK_RX_RING_SZ), sc_if->msk_rdata.msk_rx_ring);
    sc_if->msk_rdata.msk_rx_ring = NULL;
  }
  sc_if->msk_cdata.msk_rx_ring_map = NULL;
  

diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index 64a30a2..f0dd05e 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2275,8 +2275,6 @@ struct msk_stat_desc { #define MSK_MIN_FRAMELEN (ETHER_MIN_LEN - ETHER_CRC_LEN) */ -#define PAGE_SIZE 0x1000 -#define BYTES_TO_PAGES(BYTES) ((((BYTES) - 1) / PAGE_SIZE) + 1) #define htole32(x) (x) // All UEFI platforms are little endian #define le32toh(x) (x)

#define ACPI_SPECFLAG_PREFETCHABLE 0x06

2.5.0

On Thu, Aug 25, 2016 at 06:39:42PM -0400, Alan Ott wrote:

Change the receive buffers to be single-use only. This involves a few changes that work together:

1. Change msk_newbuf() to not attempt to re-use or free a buffer,
2. Change msk_rxeof() free the buffer when it is done with it,
3. Store a temporary copy of the received data for passing to the Receive Queue,
4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons:

1. The optimization failed to work for 64-bit DMA transfers;
2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

This looks sensible to me, but I would appreciate a tested-by from Daniil or Ryan (on Juno) before I merge it.

Thanks!

/ Leif

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 7979bf3..573dea5 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf ( rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

• if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) {
• return EFI_ALREADY_STARTED;
• } else if (rxd->rx_m.Buf != NULL) {
• mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping);
• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length), rxd->rx_m.Buf);
• }
• Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status;

@@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m;

DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n")); @@ -1871,26 +1865,40 @@ msk_rxeof ( break; }

• m = rxd->rx_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);
  

•  break;
  

• }
• Status = mPciIo->Flush (mPciIo);
• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to Flush DMA\n"));
  

  }
• Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to Unmap DMA\n"));
  

• }
• Status = gBS->AllocatePool (EfiBootServicesData, sizeof (MSK_LINKED_DMA_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0);

•  rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
  

•  m_link->Signature = RX_MBUF_SIGNATURE;
   Status = gBS->AllocatePool (EfiBootServicesData,
                               len,
                               (VOID**) &m_link->DmaBuf.Buf);
   if(!EFI_ERROR (Status)) {
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len);
  

•    CopyMem (m_link->DmaBuf.Buf, m.Buf, len);
     m_link->DmaBuf.Length = len;
  

•    m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping;
  

•    m_link->DmaBuf.DmaMapping = m.DmaMapping;
  

•    mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf);
  

InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else { -- 2.5.0

On 08/26/2016 12:34 PM, Daniil Egranov wrote:

Hi Alan,

On 08/25/2016 05:39 PM, Alan Ott wrote:

...

Change the receive buffers to be single-use only. This involves a few changes that work together:

1. Change msk_newbuf() to not attempt to re-use or free a buffer,
2. Change msk_rxeof() free the buffer when it is done with it,
3. Store a temporary copy of the received data for passing to the Receive Queue,
4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons:

1. The optimization failed to work for 64-bit DMA transfers;
2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 7979bf3..573dea5 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf ( rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

• if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) {
• return EFI_ALREADY_STARTED;
• } else if (rxd->rx_m.Buf != NULL) {
• mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping);
• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES

(rxd->rx_m.Length), rxd->rx_m.Buf);

• }
• Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages,

EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status; @@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m; DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n")); @@ -1871,26 +1865,40 @@ msk_rxeof ( break; }
  • m = rxd->rx_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);
  

•  break;
  

• }
• Status = mPciIo->Flush (mPciIo);
• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to Flush DMA\n"));
  

This is network related error message, please change the debug level to EFI_D_NET.

...

  }

• Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to Unmap DMA\n"));
  

This is network related error message, please change the debug level to EFI_D_NET.

These are PCI/DMA-related. Other places in the driver, errors of this nature are reported as EFI_D_ERROR. It's up to you guys though.

See also the error messages: "failed to allocate DMA'able", and "failed to map DMA'able" in if_msk.c, which are EFI_D_ERROR. I just followed what was already there, which I still think is right.

Let me know.

...

• }

• Status = gBS->AllocatePool (EfiBootServicesData,
                              sizeof (MSK_LINKED_DMA_BUF),
                              (VOID**) &m_link);
  if (!EFI_ERROR (Status)) {
    gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0);
  

•  rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
  

•   m_link->Signature = RX_MBUF_SIGNATURE;
    Status = gBS->AllocatePool (EfiBootServicesData,
                                len,
                                (VOID**) &m_link->DmaBuf.Buf);
    if(!EFI_ERROR (Status)) {
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len);
  

•    CopyMem (m_link->DmaBuf.Buf, m.Buf, len);
  

Please use gBS->CopyMem() instead of CopyMem(). It's needed for EBC code optimization.

Ok.

Alan.

On 08/26/2016 02:36 PM, Daniil Egranov wrote:

On 08/26/2016 12:49 PM, Alan Ott wrote:

...

On 08/26/2016 12:34 PM, Daniil Egranov wrote:

...

Hi Alan,

On 08/25/2016 05:39 PM, Alan Ott wrote:

...

Change the receive buffers to be single-use only. This involves a few changes that work together:

1. Change msk_newbuf() to not attempt to re-use or free a buffer,
2. Change msk_rxeof() free the buffer when it is done with it,
3. Store a temporary copy of the received data for passing to the Receive Queue,
4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons:

1. The optimization failed to work for 64-bit DMA transfers;
2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the

CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 7979bf3..573dea5 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf ( rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

• if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) {
• return EFI_ALREADY_STARTED;
• } else if (rxd->rx_m.Buf != NULL) {
• mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping);
• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES

(rxd->rx_m.Length), rxd->rx_m.Buf);

• }
• Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages,

EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status; @@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m; DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n")); @@ -1871,26 +1865,40 @@ msk_rxeof ( break; }
  • m = rxd->rx_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);
  

•  break;
  

• }
• Status = mPciIo->Flush (mPciIo);
• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to Flush DMA\n"));
  

This is network related error message, please change the debug level to EFI_D_NET.

...

  }

• Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
• if (EFI_ERROR (Status)) {

•  DEBUG ((EFI_D_ERROR, "Marvell Yukon: failed to Unmap DMA\n"));
  

This is network related error message, please change the debug level to EFI_D_NET.

These are PCI/DMA-related. Other places in the driver, errors of this nature are reported as EFI_D_ERROR. It's up to you guys though.

See also the error messages: "failed to allocate DMA'able", and "failed to map DMA'able" in if_msk.c, which are EFI_D_ERROR. I just followed what was already there, which I still think is right.

Let me know.

I am trying to keep EFI_D_ERROR for the initialization code and for general sanity checking (i may missed some places). This code will be called constantly during the network operations so in case of error, it will spam console terminal with errors. That will be interesting for people who is debugging network functionality but not for others so i would like to limit it to EFI_D_NET so this type of messages have to be explicitly enabled.

Ok. No problem. Be on the lookout for v4.

Alan.

Hi Alan,

On 08/25/2016 05:39 PM, Alan Ott wrote:

Explicitly zero allocated memory for DMA receive buffers to help guard against security issues.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 573dea5..9bd0d12 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -585,6 +585,7 @@ msk_newbuf ( if (EFI_ERROR (Status)) { return Status; }

• ZeroMem (Buffer, Length);

Please use gBS->SetMem() instead of ZeroMem(). It's needed for EBC code optimization.

Status = mPciIo->Map (mPciIo, EfiPciIoOperationBusMasterWrite, Buffer, &Length, &PhysAddr, &Mapping); if (EFI_ERROR (Status)) {

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 57 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 13 ++++++++ 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 9bd0d12..3cab084 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod; + INTN nbuf; EFI_STATUS Status;

sc_if->msk_cdata.msk_rx_cons = 0; @@ -500,17 +501,22 @@ msk_init_rx_ring (

rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0); - prod = sc_if->msk_cdata.msk_rx_prod; - for (i = 0; i < MSK_RX_RING_CNT; i++) { + for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod]; - Status = msk_newbuf (sc_if, prod); - if (EFI_ERROR (Status)) { - return Status; - } MSK_INC (prod, MSK_RX_RING_CNT); } + nbuf = MSK_RX_BUF_CNT; + prod = 0; + + for (i = 0; i < nbuf; i++) { + Status = msk_newbuf (sc_if, prod); + if (EFI_ERROR (Status)) { + return Status; + } + MSK_RX_INC(prod, MSK_RX_RING_CNT); + }

// Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1; @@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0; + sc_if->msk_cdata.msk_tx_high_addr = 0;

rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) * MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf (

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

+#ifdef MSK_64BIT_DMA + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; + rx_le = rxd->rx_le; + rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + MSK_INC(idx, MSK_RX_RING_CNT); +#endif + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le; @@ -594,6 +608,14 @@ msk_newbuf ( return Status; }

+#ifdef MSK_64BIT_DMA + rx_le = rxd->rx_le; + rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr)); + rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + MSK_INC(idx, MSK_RX_RING_CNT); + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; +#endif + gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer; @@ -1649,6 +1671,19 @@ msk_encap (

control = 0;

+#ifdef MSK_64BIT_DMA + if (MSK_ADDR_HI(BusPhysAddr) != + sc_if->msk_cdata.msk_tx_high_addr) { + sc_if->msk_cdata.msk_tx_high_addr = + MSK_ADDR_HI(BusPhysAddr); + tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; + tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr)); + tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + sc_if->msk_cdata.msk_tx_cnt++; + MSK_INC(prod, MSK_TX_RING_CNT); + } +#endif + si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr)); @@ -1866,6 +1901,12 @@ msk_rxeof ( break; }

+#ifdef MSK_64BIT_DMA + rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT]; +#else + rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; +#endif + m = rxd->rx_m; Status = msk_newbuf (sc_if, cons); if (EFI_ERROR (Status)) { @@ -1910,8 +1951,8 @@ msk_rxeof ( } } while (0);

- MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT); - MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); + MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT); + MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); }

static diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..6e835f2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,9 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter

+#if MAX_ADDRESS > 0xffffffff +#define MSK_64BIT_DMA +#endif #define MSK_TX_RING_CNT 512 #define MSK_RX_RING_CNT 512 #define MSK_RX_BUF_ALIGN 8 @@ -2323,6 +2326,7 @@ struct msk_chain_data { void *msk_tx_ring_map; void *msk_rx_ring_map; // struct msk_rxdesc msk_jumbo_rxdesc[MSK_JUMBO_RX_RING_CNT]; + INTN msk_tx_high_addr; INTN msk_tx_prod; INTN msk_tx_cons; INTN msk_tx_cnt; @@ -2352,6 +2356,15 @@ struct msk_ring_data { #define MSK_STAT_RING_SZ (sizeof (struct msk_stat_desc) * MSK_STAT_RING_CNT)

#define MSK_INC(x, y) ((x) = (x + 1) % y) +#ifdef MSK_64BIT_DMA +#define MSK_RX_INC(x, y) (x) = (x + 2) % y +#define MSK_RX_BUF_CNT (MSK_RX_RING_CNT / 2) +#define MSK_JUMBO_RX_BUF_CNT (MSK_JUMBO_RX_RING_CNT / 2) +#else +#define MSK_RX_INC(x, y) (x) = (x + 1) % y +#define MSK_RX_BUF_CNT MSK_RX_RING_CNT +#define MSK_JUMBO_RX_BUF_CNT MSK_JUMBO_RX_RING_CNT +#endif

#define MSK_PCI_BUS 0 #define MSK_PCIX_BUS 1

On 08/26/2016 07:15 PM, Daniil Egranov wrote:

Hi Alan,

On 08/25/2016 05:39 PM, Alan Ott wrote:

...

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 57 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 13 ++++++++ 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 9bd0d12..3cab084 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod;

• INTN nbuf; EFI_STATUS Status; sc_if->msk_cdata.msk_rx_cons = 0;

@@ -500,17 +501,22 @@ msk_init_rx_ring ( rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0);

• prod = sc_if->msk_cdata.msk_rx_prod;
• for (i = 0; i < MSK_RX_RING_CNT; i++) {

• for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod];

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

•  return Status;
  

• } MSK_INC (prod, MSK_RX_RING_CNT); }

• nbuf = MSK_RX_BUF_CNT;
• prod = 0;
• for (i = 0; i < nbuf; i++) {

• Status = msk_newbuf (sc_if, prod);
  

• if (EFI_ERROR (Status)) {
  

•   return Status;
  

• }
  

• MSK_RX_INC(prod, MSK_RX_RING_CNT);
  

• } // Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1;

@@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0;

• sc_if->msk_cdata.msk_tx_high_addr = 0; rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) *

MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf ( DEBUG ((EFI_D_NET, "Marvell Yukon: discard rxbuf\n")); +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];
• rx_le = rxd->rx_le;
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);

+#endif

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le;

@@ -594,6 +608,14 @@ msk_newbuf ( return Status; } +#ifdef MSK_64BIT_DMA

• rx_le = rxd->rx_le;
• rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr));
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);
• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

+#endif

• gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer;

@@ -1649,6 +1671,19 @@ msk_encap ( control = 0; +#ifdef MSK_64BIT_DMA

• if (MSK_ADDR_HI(BusPhysAddr) !=
• sc_if->msk_cdata.msk_tx_high_addr) {

•  sc_if->msk_cdata.msk_tx_high_addr =
  

•      MSK_ADDR_HI(BusPhysAddr);
  

•  tx_le = &sc_if->msk_rdata.msk_tx_ring[prod];
  

•  tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr));
  

•  tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
  

•  sc_if->msk_cdata.msk_tx_cnt++;
  

•  MSK_INC(prod, MSK_TX_RING_CNT);
  

• }

+#endif

• si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr));

@@ -1866,6 +1901,12 @@ msk_rxeof ( break; } +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT];

+#else

• rxd = &sc_if->msk_cdata.msk_rxdesc[cons];

+#endif

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

@@ -1910,8 +1951,8 @@ msk_rxeof ( } } while (0);

• MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT);

• MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); } static

diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..6e835f2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,9 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter +#if MAX_ADDRESS > 0xffffffff +#define MSK_64BIT_DMA +#endif

This will be a problem with EBC architecture independent approach as it defines the architecture conditional build. To support EBC version of the driver, this condition have to be handled during the run time. Could you take a look on it and see if such build time conditions can be removed?

The 32-bit DMA case of this driver, as far as I can tell, can be treated as an optimization on 32-bit platforms. It saves items in the RX ring. On 32-bit platforms the high addresses will just be zero.[1]

Since the point of EBC drivers is that they don't know whether they are running on a 32- or 64-bit system (since the EBC environment provides a 64-bit interface to the driver), I don't see how this can be detected at runtime. It's part of the point of EBC to not know about the underlying platform. Maybe someone can show me wrong.

Back to your assertion that this will be a problem with EBC, are you sure that's true? Since EBC driver runs in a 64-bit environment, it seems to me that the code above, when built for EBC, will always set MSK_64BIT_DMA.

Failing that, my recommendation is to #define MSK_64BIT_DMA here in all cases (and not depend on the MAX_ADDRESS). I don't think we lose anything there (except loss of a slight optimization on 32-bit platforms).

Alan.

[1] It's unclear to me the effect of setting EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE on a 32-bit platform, because of course on a 32-bit platform, the dual address cycle will never happen (since the it is a requirement that the dual address cycle not happen if the high bits are all zero).

On 27 August 2016 at 16:26, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:

On 27 August 2016 at 15:30, Alan Ott alan@softiron.co.uk wrote:

...

On 08/26/2016 07:15 PM, Daniil Egranov wrote:

...

Hi Alan,

On 08/25/2016 05:39 PM, Alan Ott wrote:

...

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 57 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 13 ++++++++ 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 9bd0d12..3cab084 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod;

• INTN nbuf; EFI_STATUS Status; sc_if->msk_cdata.msk_rx_cons = 0;

@@ -500,17 +501,22 @@ msk_init_rx_ring ( rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0);

• prod = sc_if->msk_cdata.msk_rx_prod;
• for (i = 0; i < MSK_RX_RING_CNT; i++) {

• for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod];

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

•  return Status;
  

• } MSK_INC (prod, MSK_RX_RING_CNT); }

• nbuf = MSK_RX_BUF_CNT;
• prod = 0;
• for (i = 0; i < nbuf; i++) {

• Status = msk_newbuf (sc_if, prod);
  

• if (EFI_ERROR (Status)) {
  

•   return Status;
  

• }
  

• MSK_RX_INC(prod, MSK_RX_RING_CNT);
  

• } // Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1;

@@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0;

• sc_if->msk_cdata.msk_tx_high_addr = 0; rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) *

MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf ( DEBUG ((EFI_D_NET, "Marvell Yukon: discard rxbuf\n")); +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];
• rx_le = rxd->rx_le;
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);

+#endif

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le;

@@ -594,6 +608,14 @@ msk_newbuf ( return Status; } +#ifdef MSK_64BIT_DMA

• rx_le = rxd->rx_le;
• rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr));
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);
• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

+#endif

• gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer;

@@ -1649,6 +1671,19 @@ msk_encap ( control = 0; +#ifdef MSK_64BIT_DMA

• if (MSK_ADDR_HI(BusPhysAddr) !=
• sc_if->msk_cdata.msk_tx_high_addr) {

•  sc_if->msk_cdata.msk_tx_high_addr =
  

•      MSK_ADDR_HI(BusPhysAddr);
  

•  tx_le = &sc_if->msk_rdata.msk_tx_ring[prod];
  

•  tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr));
  

•  tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
  

•  sc_if->msk_cdata.msk_tx_cnt++;
  

•  MSK_INC(prod, MSK_TX_RING_CNT);
  

• }

+#endif

• si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr));

@@ -1866,6 +1901,12 @@ msk_rxeof ( break; } +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT];

+#else

• rxd = &sc_if->msk_cdata.msk_rxdesc[cons];

+#endif

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

@@ -1910,8 +1951,8 @@ msk_rxeof ( } } while (0);

• MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT);

• MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); } static

diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..6e835f2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,9 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter +#if MAX_ADDRESS > 0xffffffff +#define MSK_64BIT_DMA +#endif

This will be a problem with EBC architecture independent approach as it defines the architecture conditional build. To support EBC version of the driver, this condition have to be handled during the run time. Could you take a look on it and see if such build time conditions can be removed?

The 32-bit DMA case of this driver, as far as I can tell, can be treated as an optimization on 32-bit platforms. It saves items in the RX ring. On 32-bit platforms the high addresses will just be zero.[1]

You are assuming that the CPU and the PCI host bridge have the same view of memory, which is not generally true on all architectures (although it is on x86). This means that even on a 32-bit platform, the PCI addresses returned by PciIo->Map() could have high bits set, and so 64bitness is fundamentally a property of the device (what the driver writer's guide calls the PCI controller), and of the driver, not of the architecture we happen to be running on.

...

Since the point of EBC drivers is that they don't know whether they are running on a 32- or 64-bit system (since the EBC environment provides a 64-bit interface to the driver), I don't see how this can be detected at runtime. It's part of the point of EBC to not know about the underlying platform. Maybe someone can show me wrong.

It could easily be detected at runtime, since sizeof() is not a compile time constant on EBC. But we shouldn't, given the above.

...

Back to your assertion that this will be a problem with EBC, are you sure that's true? Since EBC driver runs in a 64-bit environment, it seems to me that the code above, when built for EBC, will always set MSK_64BIT_DMA.

Failing that, my recommendation is to #define MSK_64BIT_DMA here in all cases (and not depend on the MAX_ADDRESS). I don't think we lose anything there (except loss of a slight optimization on 32-bit platforms).

Indeed. As an optimization, I think it should be possible to query the PCI layer whether it supports DMA above 4 GB, and enable the optimization in that case (which will also enable it for X64, since

s/in that case/if it doesn't/

UEFI on that platform typically does not support DMA >4 GB, and maps CPU:PCI 1:1)

-- Ard.

On 08/27/2016 12:43 PM, Ard Biesheuvel wrote:

On 27 August 2016 at 17:24, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:

...

On 27 August 2016 at 16:26, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:

...

On 27 August 2016 at 15:30, Alan Ott alan@softiron.co.uk wrote:

...

On 08/26/2016 07:15 PM, Daniil Egranov wrote:

...

Hi Alan,

On 08/25/2016 05:39 PM, Alan Ott wrote:

...

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 57 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 13 ++++++++ 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 9bd0d12..3cab084 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod;

• INTN nbuf; EFI_STATUS Status; sc_if->msk_cdata.msk_rx_cons = 0;

@@ -500,17 +501,22 @@ msk_init_rx_ring ( rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0);

• prod = sc_if->msk_cdata.msk_rx_prod;
• for (i = 0; i < MSK_RX_RING_CNT; i++) {

• for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod];

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

•  return Status;
  

• } MSK_INC (prod, MSK_RX_RING_CNT); }

• nbuf = MSK_RX_BUF_CNT;
• prod = 0;
• for (i = 0; i < nbuf; i++) {

• Status = msk_newbuf (sc_if, prod);
  

• if (EFI_ERROR (Status)) {
  

•   return Status;
  

• }
  

• MSK_RX_INC(prod, MSK_RX_RING_CNT);
  

• } // Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1;

@@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0;

• sc_if->msk_cdata.msk_tx_high_addr = 0; rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) *

MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf ( DEBUG ((EFI_D_NET, "Marvell Yukon: discard rxbuf\n")); +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];
• rx_le = rxd->rx_le;
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);

+#endif

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le;

@@ -594,6 +608,14 @@ msk_newbuf ( return Status; } +#ifdef MSK_64BIT_DMA

• rx_le = rxd->rx_le;
• rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr));
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);
• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

+#endif

• gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer;

@@ -1649,6 +1671,19 @@ msk_encap ( control = 0; +#ifdef MSK_64BIT_DMA

• if (MSK_ADDR_HI(BusPhysAddr) !=
• sc_if->msk_cdata.msk_tx_high_addr) {

•  sc_if->msk_cdata.msk_tx_high_addr =
  

•      MSK_ADDR_HI(BusPhysAddr);
  

•  tx_le = &sc_if->msk_rdata.msk_tx_ring[prod];
  

•  tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr));
  

•  tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
  

•  sc_if->msk_cdata.msk_tx_cnt++;
  

•  MSK_INC(prod, MSK_TX_RING_CNT);
  

• }

+#endif

• si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr));

@@ -1866,6 +1901,12 @@ msk_rxeof ( break; } +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT];

+#else

• rxd = &sc_if->msk_cdata.msk_rxdesc[cons];

+#endif

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

@@ -1910,8 +1951,8 @@ msk_rxeof ( } } while (0);

• MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT);

• MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); } static

diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..6e835f2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,9 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter +#if MAX_ADDRESS > 0xffffffff +#define MSK_64BIT_DMA +#endif

This will be a problem with EBC architecture independent approach as it defines the architecture conditional build. To support EBC version of the driver, this condition have to be handled during the run time. Could you take a look on it and see if such build time conditions can be removed?

The 32-bit DMA case of this driver, as far as I can tell, can be treated as an optimization on 32-bit platforms. It saves items in the RX ring. On 32-bit platforms the high addresses will just be zero.[1]

You are assuming that the CPU and the PCI host bridge have the same view of memory, which is not generally true on all architectures (although it is on x86). This means that even on a 32-bit platform, the PCI addresses returned by PciIo->Map() could have high bits set, and so 64bitness is fundamentally a property of the device (what the driver writer's guide calls the PCI controller), and of the driver, not of the architecture we happen to be running on.

...

Since the point of EBC drivers is that they don't know whether they are running on a 32- or 64-bit system (since the EBC environment provides a 64-bit interface to the driver), I don't see how this can be detected at runtime. It's part of the point of EBC to not know about the underlying platform. Maybe someone can show me wrong.

It could easily be detected at runtime, since sizeof() is not a compile time constant on EBC. But we shouldn't, given the above.

...

Back to your assertion that this will be a problem with EBC, are you sure that's true? Since EBC driver runs in a 64-bit environment, it seems to me that the code above, when built for EBC, will always set MSK_64BIT_DMA.

Failing that, my recommendation is to #define MSK_64BIT_DMA here in all cases (and not depend on the MAX_ADDRESS). I don't think we lose anything there (except loss of a slight optimization on 32-bit platforms).

Indeed. As an optimization, I think it should be possible to query the PCI layer whether it supports DMA above 4 GB, and enable the optimization in that case (which will also enable it for X64, since

s/in that case/if it doesn't/

...

UEFI on that platform typically does not support DMA >4 GB, and maps CPU:PCI 1:1)

Apologies for the repeated reply to self, but it seems the way to implement this according to the spec is to *not* set the attribute when enabling the device, but pass it into AllocateBuffer() when performing the consistent DMA mappings (and per my other email, the instance used for streaming DMA needs to go), and also use EfiPciOperationBusMasterRead64, EfiPciOperationBusMasterWrite64 etc explicitly when calling Map().

Hi Ard,

Make sure you are not confusing the flags for the PCI Root Bridge and the PCI Driver (often called PCI_IO*). It's a bit confusing on the naming, but PciIo->AllocateBuffer() will not take an attribute of EfiPciOperationBusMasterWrite64, because EfiPciOperationBusMasterWrite64 is a EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_OPERATION, and not a EFI_PCI_IO_PROTOCOL_OPERATION, which is what PciIo->AllocateBuffer() expects.

See sections 13.4 vs 13.2 of version 2.6 of the UEFI specification.

Alan.

But let's wait and see if anyone from the Tianocore community has any advice (I have cc'ed all of you on the email to edk2-devel)

In the mean time, I will ack either approach as long as it does not assume 32-bit PCI on 32-bit architectures.

Great! This whole thing is made worse by the lack of examples in the edk2 source tree. Linux has spoiled us in that respect :)

Alan.

On 28 August 2016 at 00:40, Alan Ott alan@softiron.co.uk wrote:

On 08/27/2016 11:26 AM, Ard Biesheuvel wrote:

...

On 27 August 2016 at 15:30, Alan Ott alan@softiron.co.uk wrote:

...

On 08/26/2016 07:15 PM, Daniil Egranov wrote:

...

Hi Alan,

On 08/25/2016 05:39 PM, Alan Ott wrote:

...

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 57 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 13 ++++++++ 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 9bd0d12..3cab084 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod;

• INTN nbuf; EFI_STATUS Status; sc_if->msk_cdata.msk_rx_cons = 0;

@@ -500,17 +501,22 @@ msk_init_rx_ring ( rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0);

• prod = sc_if->msk_cdata.msk_rx_prod;
• for (i = 0; i < MSK_RX_RING_CNT; i++) {

• for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod];

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

•  return Status;
  

• } MSK_INC (prod, MSK_RX_RING_CNT); }

• nbuf = MSK_RX_BUF_CNT;
• prod = 0;
• for (i = 0; i < nbuf; i++) {

• Status = msk_newbuf (sc_if, prod);
  

• if (EFI_ERROR (Status)) {
  

•   return Status;
  

• }
  

• MSK_RX_INC(prod, MSK_RX_RING_CNT);
  

• } // Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1;

@@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0;

• sc_if->msk_cdata.msk_tx_high_addr = 0; rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) *

MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf ( DEBUG ((EFI_D_NET, "Marvell Yukon: discard rxbuf\n")); +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];
• rx_le = rxd->rx_le;
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);

+#endif

• rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le;

@@ -594,6 +608,14 @@ msk_newbuf ( return Status; } +#ifdef MSK_64BIT_DMA

• rx_le = rxd->rx_le;
• rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr));
• rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
• MSK_INC(idx, MSK_RX_RING_CNT);
• rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

+#endif

• gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer;

@@ -1649,6 +1671,19 @@ msk_encap ( control = 0; +#ifdef MSK_64BIT_DMA

• if (MSK_ADDR_HI(BusPhysAddr) !=
• sc_if->msk_cdata.msk_tx_high_addr) {

•  sc_if->msk_cdata.msk_tx_high_addr =
  

•      MSK_ADDR_HI(BusPhysAddr);
  

•  tx_le = &sc_if->msk_rdata.msk_tx_ring[prod];
  

•  tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr));
  

•  tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER);
  

•  sc_if->msk_cdata.msk_tx_cnt++;
  

•  MSK_INC(prod, MSK_TX_RING_CNT);
  

• }

+#endif

• si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr));

@@ -1866,6 +1901,12 @@ msk_rxeof ( break; } +#ifdef MSK_64BIT_DMA

• rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT];

+#else

• rxd = &sc_if->msk_cdata.msk_rxdesc[cons];

+#endif

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

@@ -1910,8 +1951,8 @@ msk_rxeof ( } } while (0);

• MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT);

• MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
• MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); } static

diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..6e835f2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,9 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter +#if MAX_ADDRESS > 0xffffffff +#define MSK_64BIT_DMA +#endif

This will be a problem with EBC architecture independent approach as it defines the architecture conditional build. To support EBC version of the driver, this condition have to be handled during the run time. Could you take a look on it and see if such build time conditions can be removed?

The 32-bit DMA case of this driver, as far as I can tell, can be treated as an optimization on 32-bit platforms. It saves items in the RX ring. On 32-bit platforms the high addresses will just be zero.[1]

You are assuming that the CPU and the PCI host bridge have the same view of memory, which is not generally true on all architectures (although it is on x86). This means that even on a 32-bit platform, the PCI addresses returned by PciIo->Map() could have high bits set,

The linux driver for this part checks sizeof(dma_addr_t) against sizeof(u32), so I assume for Linux, your example case would be handled by a 32-bit platform having a large dma_addr_t.

Some 32-bit architectures support a larger physical address space (e.g., LPAE on ARM supports up to 40 bits, iirc). However, the UEFI spec mandates a 1:1 mapping for the CPU's view of memory, and so this high memory is not actually usable in UEFI. So no, this is not really the same thing.

It is simply that PCIe is naturally 64 bits (and so the choice of DUAL_ADDRESS_CYCLE for the name is a bit unfortunate, since it is tightly coupled to legacy 32-bits PCI, which needs two address cycles to put both halves of the address on the bus, while PCIe is not even a bus anymore), which means that a platform could be configured in a way that puts the memory that the CPU maps 1:1 higher up in the PCI controllers's address space. So this is more rare than LPAE, but still something we need to take into account.

...

and so 64bitness is fundamentally a property of the device (what the driver writer's guide calls the PCI controller), and of the driver, not of the architecture we happen to be running on.

In that case, it seems to make sense to leave the 64-bit support on all the time.

Agreed.

...

...

Since the point of EBC drivers is that they don't know whether they are running on a 32- or 64-bit system (since the EBC environment provides a 64-bit interface to the driver), I don't see how this can be detected at runtime. It's part of the point of EBC to not know about the underlying platform. Maybe someone can show me wrong.

It could easily be detected at runtime, since sizeof() is not a compile time constant on EBC. But we shouldn't, given the above.

...

Back to your assertion that this will be a problem with EBC, are you sure that's true? Since EBC driver runs in a 64-bit environment, it seems to me that the code above, when built for EBC, will always set MSK_64BIT_DMA.

Failing that, my recommendation is to #define MSK_64BIT_DMA here in all cases (and not depend on the MAX_ADDRESS). I don't think we lose anything there (except loss of a slight optimization on 32-bit platforms).

Indeed. As an optimization, I think it should be possible to query the PCI layer whether it supports DMA above 4 GB, and enable the optimization in that case (which will also enable it for X64, since UEFI on that platform typically does not support DMA >4 GB, and maps CPU:PCI 1:1)

I agree that could be an optimization, but believe it to be a separate development effort, of course.

Yes, and please don't bother. You are not going to be able to test it anyway on Seattle.

Thanks, Ard.

The Dual Address Cycle Attribute is necessary for bus mastering PCI devices which are capable of generating 64-bit addresses. See the "Driver Writer's Guide for UEFI 2.3.1" version 1.01, section 18.3.2. --- Drivers/Net/MarvellYukonDxe/if_msk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 3cab084..c05e51e 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1160,7 +1160,7 @@ mskc_attach ( Status = mPciIo->Attributes ( mPciIo, EfiPciIoAttributeOperationEnable, - Supports, + Supports | EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE, NULL ); }

Explicitly zero allocated memory for DMA receive buffers to help guard against security issues.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index b5bb034..3d4e960 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -585,6 +585,7 @@ msk_newbuf ( if (EFI_ERROR (Status)) { return Status; } + gBS->SetMem (Buffer, Length, 0);

Status = mPciIo->Map (mPciIo, EfiPciIoOperationBusMasterWrite, Buffer, &Length, &PhysAddr, &Mapping); if (EFI_ERROR (Status)) {

The Dual Address Cycle Attribute is necessary for bus mastering PCI devices which are capable of generating 64-bit addresses. See the "Driver Writer's Guide for UEFI 2.3.1" version 1.01, section 18.3.2.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 795c5f4..7e6c03b 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1160,7 +1160,7 @@ mskc_attach ( Status = mPciIo->Attributes ( mPciIo, EfiPciIoAttributeOperationEnable, - Supports, + Supports | EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE, NULL ); }

Change the receive buffers to be single-use only. This involves a few changes that work together: 1. Change msk_newbuf() to not attempt to re-use or free a buffer, 2. Change msk_rxeof() free the buffer when it is done with it, 3. Store a temporary copy of the received data for passing to the Receive Queue, 4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons: 1. The optimization failed to work for 64-bit DMA transfers; 2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index d2102dc..b2cf401 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf (

rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

- if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) { - return EFI_ALREADY_STARTED; - } else if (rxd->rx_m.Buf != NULL) { - mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping); - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length), rxd->rx_m.Buf); - } - Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status; @@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen; + MSK_DMA_BUF m;

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

@@ -1871,26 +1865,40 @@ msk_rxeof ( break; }

+ m = rxd->rx_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); + break; + } + + Status = mPciIo->Flush (mPciIo); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n")); } + + Status = mPciIo->Unmap (mPciIo, m.DmaMapping); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Unmap DMA\n")); + } + Status = gBS->AllocatePool (EfiBootServicesData, sizeof (MSK_LINKED_DMA_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0); - rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; - m_link->Signature = RX_MBUF_SIGNATURE; Status = gBS->AllocatePool (EfiBootServicesData, len, (VOID**) &m_link->DmaBuf.Buf); if(!EFI_ERROR (Status)) { - gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len); + gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); m_link->DmaBuf.Length = len; - m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping; + m_link->DmaBuf.DmaMapping = m.DmaMapping; + + mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf);

InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else {

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 57 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 13 ++++++++ 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 92c31a3..37847e0 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod; + INTN nbuf; EFI_STATUS Status;

sc_if->msk_cdata.msk_rx_cons = 0; @@ -500,17 +501,22 @@ msk_init_rx_ring (

rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0); - prod = sc_if->msk_cdata.msk_rx_prod; - for (i = 0; i < MSK_RX_RING_CNT; i++) { + for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod]; - Status = msk_newbuf (sc_if, prod); - if (EFI_ERROR (Status)) { - return Status; - } MSK_INC (prod, MSK_RX_RING_CNT); } + nbuf = MSK_RX_BUF_CNT; + prod = 0; + + for (i = 0; i < nbuf; i++) { + Status = msk_newbuf (sc_if, prod); + if (EFI_ERROR (Status)) { + return Status; + } + MSK_RX_INC(prod, MSK_RX_RING_CNT); + }

// Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1; @@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0; + sc_if->msk_cdata.msk_tx_high_addr = 0;

rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) * MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf (

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

+#ifdef MSK_64BIT_DMA + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; + rx_le = rxd->rx_le; + rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + MSK_INC(idx, MSK_RX_RING_CNT); +#endif + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le; @@ -594,6 +608,14 @@ msk_newbuf ( return Status; }

+#ifdef MSK_64BIT_DMA + rx_le = rxd->rx_le; + rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr)); + rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + MSK_INC(idx, MSK_RX_RING_CNT); + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; +#endif + gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer; @@ -1649,6 +1671,19 @@ msk_encap (

control = 0;

+#ifdef MSK_64BIT_DMA + if (MSK_ADDR_HI(BusPhysAddr) != + sc_if->msk_cdata.msk_tx_high_addr) { + sc_if->msk_cdata.msk_tx_high_addr = + MSK_ADDR_HI(BusPhysAddr); + tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; + tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr)); + tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + sc_if->msk_cdata.msk_tx_cnt++; + MSK_INC(prod, MSK_TX_RING_CNT); + } +#endif + si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr)); @@ -1866,6 +1901,12 @@ msk_rxeof ( break; }

+#ifdef MSK_64BIT_DMA + rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT]; +#else + rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; +#endif + m = rxd->rx_m; Status = msk_newbuf (sc_if, cons); if (EFI_ERROR (Status)) { @@ -1910,8 +1951,8 @@ msk_rxeof ( } } while (0);

- MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT); - MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); + MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT); + MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); }

static diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..6e835f2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,9 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter

+#if MAX_ADDRESS > 0xffffffff +#define MSK_64BIT_DMA +#endif #define MSK_TX_RING_CNT 512 #define MSK_RX_RING_CNT 512 #define MSK_RX_BUF_ALIGN 8 @@ -2323,6 +2326,7 @@ struct msk_chain_data { void *msk_tx_ring_map; void *msk_rx_ring_map; // struct msk_rxdesc msk_jumbo_rxdesc[MSK_JUMBO_RX_RING_CNT]; + INTN msk_tx_high_addr; INTN msk_tx_prod; INTN msk_tx_cons; INTN msk_tx_cnt; @@ -2352,6 +2356,15 @@ struct msk_ring_data { #define MSK_STAT_RING_SZ (sizeof (struct msk_stat_desc) * MSK_STAT_RING_CNT)

#define MSK_INC(x, y) ((x) = (x + 1) % y) +#ifdef MSK_64BIT_DMA +#define MSK_RX_INC(x, y) (x) = (x + 2) % y +#define MSK_RX_BUF_CNT (MSK_RX_RING_CNT / 2) +#define MSK_JUMBO_RX_BUF_CNT (MSK_JUMBO_RX_RING_CNT / 2) +#else +#define MSK_RX_INC(x, y) (x) = (x + 1) % y +#define MSK_RX_BUF_CNT MSK_RX_RING_CNT +#define MSK_JUMBO_RX_BUF_CNT MSK_JUMBO_RX_RING_CNT +#endif

#define MSK_PCI_BUS 0 #define MSK_PCIX_BUS 1

On 26 August 2016 at 23:57, Alan Ott alan@softiron.co.uk wrote:

Change the receive buffers to be single-use only. This involves a few changes that work together:

1. Change msk_newbuf() to not attempt to re-use or free a buffer,
2. Change msk_rxeof() free the buffer when it is done with it,
3. Store a temporary copy of the received data for passing to the Receive Queue,
4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons:

1. The optimization failed to work for 64-bit DMA transfers;
2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index d2102dc..1cc8964 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf (

rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

• if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) {
• return EFI_ALREADY_STARTED;
• } else if (rxd->rx_m.Buf != NULL) {
• mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping);
• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length), rxd->rx_m.Buf);
• }
• Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status;

@@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m;

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

@@ -1871,26 +1865,38 @@ msk_rxeof ( break; }

• m = rxd->rx_m;

This assignment results in

../Drivers/Net/MarvellYukonDxe/if_msk.c: In function ‘msk_rxeof’: ../Drivers/Net/MarvellYukonDxe/if_msk.c:1868:7: error: ‘rxd’ may be used uninitialized in this function [-Werror=maybe-uninitialized] m = rxd->rx_m; ^ cc1: all warnings being treated as errors

and actually, I don't see *any* assignment of rxd anywhere in the function, so there is clearly something dodgy going on here. Could you please confirm that this version of the patch works as expected?

 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;
  

• }
• Status = mPciIo->Flush (mPciIo);
• if (EFI_ERROR (Status)) {

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

  }
• Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
• if (EFI_ERROR (Status)) {

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

• }
• Status = gBS->AllocatePool (EfiBootServicesData, sizeof (MSK_LINKED_DMA_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0);

•  rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
  

This is the original assignment, but it is being removed.

   m_link->Signature = RX_MBUF_SIGNATURE;
   Status = gBS->AllocatePool (EfiBootServicesData,
                               len,
                               (VOID**) &m_link->DmaBuf.Buf);
   if(!EFI_ERROR (Status)) {

•    gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len);
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len);
     m_link->DmaBuf.Length = len;
  

•    m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping;
  

•    m_link->DmaBuf.DmaMapping = m.DmaMapping;
  
     InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link);
   } else {
  

@@ -1899,6 +1905,8 @@ msk_rxeof ( } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n")); }

• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf); } while (0);

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

-- 2.5.0

Explicitly zero allocated memory for DMA receive buffers to help guard against security issues.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 1cc8964..ab9fa2b 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -585,6 +585,7 @@ msk_newbuf ( if (EFI_ERROR (Status)) { return Status; } + gBS->SetMem (Buffer, Length, 0);

Status = mPciIo->Map (mPciIo, EfiPciIoOperationBusMasterWrite, Buffer, &Length, &PhysAddr, &Mapping); if (EFI_ERROR (Status)) {

The Dual Address Cycle Attribute is necessary for bus mastering PCI devices which are capable of generating 64-bit addresses. See the "Driver Writer's Guide for UEFI 2.3.1" version 1.01, section 18.3.2.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 8820f2a..4ca3c5f 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1160,7 +1160,7 @@ mskc_attach ( Status = mPciIo->Attributes ( mPciIo, EfiPciIoAttributeOperationEnable, - Supports, + Supports | EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE, NULL ); }

On 08/27/2016 11:20 AM, Ard Biesheuvel wrote:

On 26 August 2016 at 23:57, Alan Ott alan@softiron.co.uk wrote:

...

The Dual Address Cycle Attribute is necessary for bus mastering PCI devices which are capable of generating 64-bit addresses. See the "Driver Writer's Guide for UEFI 2.3.1" version 1.01, section 18.3.2.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 8820f2a..4ca3c5f 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1160,7 +1160,7 @@ mskc_attach ( Status = mPciIo->Attributes ( mPciIo, EfiPciIoAttributeOperationEnable,

•      Supports,
  

•      Supports | EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
        NULL
        );
  

  }

Interestingly, although this pattern occurs more often in EDK2 code, it actually violated the spec, since it states

""" EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE This bit may only be used in the Attributes parameter to AllocateBuffer(). If this bit is set, then the PCI controller that is requesting a buffer through AllocateBuffer() is capable of producing PCI Dual Address Cycles, so it is able to access a 64-bit address space. If this bit is not set, then the PCI controller that is requesting a buffer through AllocateBuffer() is not capable of producing PCI Dual Address Cycles, so it is only able to access a 32-bit address space. """

Like the EfiPciOperationBusMasterWrite64 attribute I spoke of in the other email, EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE is only an attribute for the PCI Root Bridge version of AllocateBuffer(), not for the PCI IO version of AllocateBuffer(). See the 2.6 UEFI Spec, sections 13.2 vs 13.4.

This is out of sync with the UEFI driver writer's guide, which suggests what you are doing here.

We should get this clarified, but in the mean time, this looks correct to me.

Reviewed-by: Ard Biesheuvel ard.biesheuvel@linaro.org

Thanks for reviewing. It is appreciated.

Alan.

msk_rxeof() allocates a link object which contains a buffer. If the buffer can't be allocated, make sure to free the link object.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 4ca3c5f..ca863f3 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1943,6 +1943,7 @@ msk_rxeof ( InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMA buffer. Dropping Frame\n")); + gBS->FreePool (m_link); } } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n"));

On 29 August 2016 at 20:02, Alan Ott alan@softiron.co.uk wrote:

Change the receive buffers to be single-use only. This involves a few changes that work together:

1. Change msk_newbuf() to not attempt to re-use or free a buffer,
2. Change msk_rxeof() free the buffer when it is done with it,
3. Store a temporary copy of the received data for passing to the Receive Queue,
4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons:

1. The optimization failed to work for 64-bit DMA transfers;
2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index d2102dc..ad7a1c2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf (

rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

• if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) {
• return EFI_ALREADY_STARTED;
• } else if (rxd->rx_m.Buf != NULL) {
• mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping);
• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length), rxd->rx_m.Buf);
• }
• Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status;

@@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m;

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

@@ -1871,26 +1865,40 @@ msk_rxeof ( break; }

• rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
• m = rxd->rx_m;

Struct assignment is not allowed in EDK2 (but I haven't checked the existing code).

I will change this to

gBS->CopyMem (&m, &rxd->rx_m, sizeof(MSK_DMA_BUF));

when merging the patch, unless there is a pressing reason not to.

 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;
  

• }
• Status = mPciIo->Flush (mPciIo);
• if (EFI_ERROR (Status)) {

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

  }
• Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
• if (EFI_ERROR (Status)) {

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

• }
• Status = gBS->AllocatePool (EfiBootServicesData, sizeof (MSK_LINKED_DMA_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0);

•  rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
  

•  m_link->Signature = RX_MBUF_SIGNATURE;
   Status = gBS->AllocatePool (EfiBootServicesData,
                               len,
                               (VOID**) &m_link->DmaBuf.Buf);
   if(!EFI_ERROR (Status)) {
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len);
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len);
     m_link->DmaBuf.Length = len;
  

•    m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping;
  

•    m_link->DmaBuf.DmaMapping = m.DmaMapping;
  
  

Do we still need DmaMapping after we unmapped the buffer?

     InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link);
   } else {

@@ -1899,6 +1907,8 @@ msk_rxeof ( } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n")); }

• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf); } while (0);

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

-- 2.5.0

On 30 August 2016 at 03:59, Alan Ott alan@softiron.co.uk wrote:

On 08/29/2016 03:34 PM, Ard Biesheuvel wrote:

...

On 29 August 2016 at 20:02, Alan Ott alan@softiron.co.uk wrote:

...

Change the receive buffers to be single-use only. This involves a few changes that work together:

1. Change msk_newbuf() to not attempt to re-use or free a buffer,
2. Change msk_rxeof() free the buffer when it is done with it,
3. Store a temporary copy of the received data for passing to the Receive Queue,
4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons:

1. The optimization failed to work for 64-bit DMA transfers;
2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index d2102dc..ad7a1c2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf (

rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

• if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) {
• return EFI_ALREADY_STARTED;
• } else if (rxd->rx_m.Buf != NULL) {
• mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping);
• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length),

rxd->rx_m.Buf);

• }
• Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages,

EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status; @@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m;

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

@@ -1871,26 +1865,40 @@ msk_rxeof ( break; }

• rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
• m = rxd->rx_m;

Struct assignment is not allowed in EDK2 (but I haven't checked the existing code).

I had a look in the EDKII C Coding Standards 2.1 Draft PDF here: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications and didn't see it, but maybe I'm looking at the wrong thing.

...

I will change this to

gBS->CopyMem (&m, &rxd->rx_m, sizeof(MSK_DMA_BUF));

when merging the patch, unless there is a pressing reason not to.

Feel free, but: gBS->CopyMem (&m, &rxd->rx_m, sizeof(m)); is a little safer (sizeof(m) instead of sizeof(MSK_DMA_BUF)), but of course the struct assignment method is the safest (with compile-time checks for equivalent types and automatic determination of size (which already helped me once today)).

Whatever you feel needs to be done is fine.

OK

...

...

  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;
  

• }
• Status = mPciIo->Flush (mPciIo);
• if (EFI_ERROR (Status)) {

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

• Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
• if (EFI_ERROR (Status)) {

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

• }

• Status = gBS->AllocatePool (EfiBootServicesData,
                              sizeof (MSK_LINKED_DMA_BUF),
                              (VOID**) &m_link);
  if (!EFI_ERROR (Status)) {
    gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0);
  

•  rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
  

•   m_link->Signature = RX_MBUF_SIGNATURE;
    Status = gBS->AllocatePool (EfiBootServicesData,
                                len,
                                (VOID**) &m_link->DmaBuf.Buf);
    if(!EFI_ERROR (Status)) {
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len);
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len);
      m_link->DmaBuf.Length = len;
  

•    m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping;
  

•    m_link->DmaBuf.DmaMapping = m.DmaMapping;
  
  

Do we still need DmaMapping after we unmapped the buffer?

Certainly not, but the consumer of that queue doesn't do anything with the DMA mapping, which is why it didn't cause an issue, even before my patches.

The Receive Queue should use a MSK_LINKED_SYSTEM_BUF instead of a MSK_LINKED_DMA_BUF. I made a patch for this, which I will send tomorrow (Tuesday) as part of v7.

OK, but before you do that, could we get rid of the PciIo->AllocateBuffer() and the redundant copy as well? Something like below, but in a way that it doesn't leak memory?

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 9095ccd1744d..bf0f6d091ccb 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,7 +581,8 @@ msk_newbuf (

rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

- Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); + Status = gBS->AllocatePool (EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), + &Buffer); if (EFI_ERROR (Status)) { return Status; } @@ -590,7 +591,7 @@ msk_newbuf ( Status = mPciIo->Map (mPciIo, EfiPciIoOperationBusMasterWrite, Buffer, &Length, &PhysAddr, &Mapping); if (EFI_ERROR (Status)) { Length = MAX_SUPPORTED_PACKET_SIZE; - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (Length), Buffer); + gBS->FreePool (Buffer); return Status; }

@@ -1608,7 +1609,7 @@ msk_txrx_dma_free ( mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping); // Free Rx buffers as we own these if(rxd->rx_m.Buf != NULL) { - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length), rxd->rx_m.Buf); + gBS->FreePool (rxd->rx_m.Buf); rxd->rx_m.Buf = NULL; } gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); @@ -1891,19 +1892,11 @@ msk_rxeof ( if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE; - Status = gBS->AllocatePool (EfiBootServicesData, - len, - (VOID**) &m_link->DmaBuf.Buf); - if(!EFI_ERROR (Status)) { - gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); - m_link->DmaBuf.Length = len; - m_link->DmaBuf.DmaMapping = m.DmaMapping; - - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf); + m_link->DmaBuf.Buf = m.Buf; + m_link->DmaBuf.Length = len; + m_link->DmaBuf.DmaMapping = m.DmaMapping;

- InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); - } else { - DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMA buffer. Dropping Frame\n")); + InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n")); diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.h b/Drivers/Net/MarvellYukonDxe/if_msk.h index a5025a39ecf3..66513d3c54c1 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 EFI_PAGE_SIZE +#define MAX_SUPPORTED_PACKET_SIZE (1500)

EFI_STATUS mskc_probe (EFI_PCI_IO_PROTOCOL *PciIo);

On 30 August 2016 at 14:14, Alan Ott alan@softiron.co.uk wrote:

On 08/30/2016 02:49 AM, Ard Biesheuvel wrote:

...

On 30 August 2016 at 03:59, Alan Ott alan@softiron.co.uk wrote:

...

On 08/29/2016 03:34 PM, Ard Biesheuvel wrote:

...

On 29 August 2016 at 20:02, Alan Ott alan@softiron.co.uk wrote:

...

Change the receive buffers to be single-use only. This involves a few changes that work together:

1. Change msk_newbuf() to not attempt to re-use or free a buffer,
2. Change msk_rxeof() free the buffer when it is done with it,
3. Store a temporary copy of the received data for passing to the Receive Queue,
4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons:

1. The optimization failed to work for 64-bit DMA transfers;
2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the

CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index d2102dc..ad7a1c2 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf (

 rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

• if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) {
• return EFI_ALREADY_STARTED;
• } else if (rxd->rx_m.Buf != NULL) {
• mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping);
• mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length),

rxd->rx_m.Buf);

• }
• Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages,

EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status; @@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

• MSK_DMA_BUF m;

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

@@ -1871,26 +1865,40 @@ msk_rxeof ( break; }

• rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
• m = rxd->rx_m;

Struct assignment is not allowed in EDK2 (but I haven't checked the existing code).

I had a look in the EDKII C Coding Standards 2.1 Draft PDF here:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications and didn't see it, but maybe I'm looking at the wrong thing.

...

I will change this to

gBS->CopyMem (&m, &rxd->rx_m, sizeof(MSK_DMA_BUF));

when merging the patch, unless there is a pressing reason not to.

Feel free, but: gBS->CopyMem (&m, &rxd->rx_m, sizeof(m)); is a little safer (sizeof(m) instead of sizeof(MSK_DMA_BUF)), but of course the struct assignment method is the safest (with compile-time checks for equivalent types and automatic determination of size (which already helped me once today)).

Whatever you feel needs to be done is fine.

OK

...

...

...

   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;
  

• }
• Status = mPciIo->Flush (mPciIo);
• if (EFI_ERROR (Status)) {

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

• Status = mPciIo->Unmap (mPciIo, m.DmaMapping);
• if (EFI_ERROR (Status)) {

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

• }

•  Status = gBS->AllocatePool (EfiBootServicesData,
                               sizeof (MSK_LINKED_DMA_BUF),
                               (VOID**) &m_link);
   if (!EFI_ERROR (Status)) {
     gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0);
  

•  rxd = &sc_if->msk_cdata.msk_rxdesc[cons];
  

•    m_link->Signature = RX_MBUF_SIGNATURE;
     Status = gBS->AllocatePool (EfiBootServicesData,
                                 len,
                                 (VOID**) &m_link->DmaBuf.Buf);
     if(!EFI_ERROR (Status)) {
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len);
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len);
       m_link->DmaBuf.Length = len;
  

•    m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping;
  

•    m_link->DmaBuf.DmaMapping = m.DmaMapping;
  
  

Do we still need DmaMapping after we unmapped the buffer?

Certainly not, but the consumer of that queue doesn't do anything with the DMA mapping, which is why it didn't cause an issue, even before my patches.

The Receive Queue should use a MSK_LINKED_SYSTEM_BUF instead of a MSK_LINKED_DMA_BUF. I made a patch for this, which I will send tomorrow (Tuesday) as part of v7.

OK, but before you do that, could we get rid of the PciIo->AllocateBuffer()

I've acutally been testing with something similar already (although using gBS->AllocatePages() as you originally suggested), but I'd like to do one thing at a time.

OK, fair enough.

Further, from my reading of the documents, I'm not even sure it's right to try to DMA to memory that's not allocated by the PciIo. I had partially typed up an email to this effect, but I was hoping for one thing at a time. But since you brought it up...

What do you think about section 5.1.1.2 of the Driver Writer's Guide where it says:

...

A UEFI driver must never directly allocate a memory buffer for DMA access. The UEFI driver cannot know enough about the system architecture to predict what system memory areas are available for DMA or if CPU caches are coherent with DMA operations. Instead, a UEFI driver must use the services provided by the I/O protocol for the bus to allocate and free buffers required for DMA operations. There should also be services to initiate and complete DMA transactions. For example, the PCI Root Bridge I/O Protocol and PCI I/O Protocol both provide services for PCI DMA operations. As additional I/O bus types with DMA capabilities are introduced, new protocols that abstract the DMA services must be provided.

Similar language is in section 5.3.11. It seems like it's stating you can't make the assumption that the bus is able to perform DMA operations to all of RAM, but that the bus driver knows these (platform- and bus-specific) limitations, so you should use the bus (in this case PciIo), to allocate the memory.

However, the examples of streaming PCI DMA in the DWG do _not_ do an allocation this way, and rely on a pointer passed in from outside the example code (DWG 18.5.7).

I've looked hard in the UEFI specification, but I can't find anything else which corroborates this. The UEFI spec only seems to mandate PciIo->AllocateBuffer() for common buffers (EfiPciOperationBusMasterCommonBuffer). This leads me to suspect the above quoted paragraph (DWG 5.1.1.2) may only be talking about common buffers and not for streaming buffers, but it's not clear.

No, it is not clear, but AllocateBuffer() is simply not required for streaming DMA. Note that the transmit path in this driver already performs streaming DMA on arbitrary buffers, so there is no reason for the RX path to be different.

In general, the directionality of streaming DMA combined with the restrictions on when the data is valid and accessible (before Map, after Unmap, etc) means that the implementation can reallocate the buffer if it is not located in suitable memory, which is exactly what the non-coherent DmaLib does.

There are two reasons I'd prefer not to use AllocateBuffer() here: - it gives you uncached memory on non-coherent platforms, which means that every single read performed by the CopyMem() after flush/unmap goes all the way to main memory - AllocateBuffer() changes the memory map (and so does AllocatePages(), which is why I suggested AllocatePool() instead in the second instance), which is costly

In my testing, your suggestion (AllocatePages()) works, but I'm not convinced it's correct.

Either way, I'm going to push for this being a separate patch than the current patch series. I'm happy to do it (if we decide ultimately that it is correct), but it's really a separate problem (as you've indicated before).

Let me know if this is ok with you.

Sure.

...

and the redundant copy as well? Something like below, but in a way that it doesn't leak memory?

Can this also be a separate from the series? Again, it's a separate problem. Like the above, I'm happy to do it, but rebasing this same set continuously is getting tiresome. v7 is nearly ready, and I'd like it to be the last one. I'm happy to submit a second series after this series is in, to optimize these issues you point out.

Let me know, and again, thanks for your review and interest in this series.

No, that is perfectly fine. I am very happy you are willing to spend more time on this, since I don't have access to the hardware (and am not that familiar with this version or the BSD version of the driver)

It just seemed that changing from MSK_DMA_BUF to MSK_SYSTEM_BUF etc would combine naturally with the changes above, but perhaps not. It's up to you.

Thanks, Ard.

On 30 August 2016 at 15:46, Alan Ott alan@softiron.co.uk wrote:

On 08/30/2016 10:15 AM, Ard Biesheuvel wrote:

...

On 30 August 2016 at 14:14, Alan Ott alan@softiron.co.uk wrote:

...

On 08/30/2016 02:49 AM, Ard Biesheuvel wrote:

...

On 30 August 2016 at 03:59, Alan Ott alan@softiron.co.uk wrote:

...

On 08/29/2016 03:34 PM, Ard Biesheuvel wrote:

...

On 29 August 2016 at 20:02, Alan Ott alan@softiron.co.uk wrote: > > Change the receive buffers to be single-use only. This involves a few > changes that work together: > 1. Change msk_newbuf() to not attempt to re-use or free a buffer, > 2. Change msk_rxeof() free the buffer when it is done with it, > 3. Store a temporary copy of the received data for passing to > the Receive Queue, > 4. Call the required Flush() and Unmap() on the DMA buffer. > > In addition this means failure to allocate a new buffer is a failure > of msk_rxeof(). > > Note that this change makes the driver work the way the FreeBSD > driver > (from which this driver was derived) works, and this simply removes > an > optimization (the code in msk_newbuf() which re-uses the buffers. > This > removal of the optimization is done for two reasons: > 1. The optimization failed to work for 64-bit DMA transfers; > 2. The UEFI specification, version 2.6, section 13.4 requires calls > to > Flush() and Unmap() before reading a DMA write buffer from the > CPU, > which doesn't fit with the optimization as it existed. > > Reverting back to the behavior as it was in the FreeBSD driver solves > number 1 and 2 above, and makes this driver more consistent with > something we know to be working. There is slightly more overhead, but > it is more consistent with the UEFI standard. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Alan Ott alan@softiron.co.uk > --- > Drivers/Net/MarvellYukonDxe/if_msk.c | 32 > +++++++++++++++++++++----------- > 1 file changed, 21 insertions(+), 11 deletions(-) > > diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c > b/Drivers/Net/MarvellYukonDxe/if_msk.c > index d2102dc..ad7a1c2 100644 > --- a/Drivers/Net/MarvellYukonDxe/if_msk.c > +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c > @@ -581,13 +581,6 @@ msk_newbuf ( > > rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; > > - if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) { > - return EFI_ALREADY_STARTED; > - } else if (rxd->rx_m.Buf != NULL) { > - mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping); > - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES > (rxd->rx_m.Length), > rxd->rx_m.Buf); > - } > - > Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, > EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); > if (EFI_ERROR (Status)) { > return Status; > @@ -1848,6 +1841,7 @@ msk_rxeof ( > struct msk_rxdesc *rxd; > INTN cons; > INTN rxlen; > + MSK_DMA_BUF m; > > DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n")); > > @@ -1871,26 +1865,40 @@ msk_rxeof ( > break; > } > > + rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; > + > + m = rxd->rx_m;

Struct assignment is not allowed in EDK2 (but I haven't checked the existing code).

I had a look in the EDKII C Coding Standards 2.1 Draft PDF here:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications and didn't see it, but maybe I'm looking at the wrong thing.

...

I will change this to

gBS->CopyMem (&m, &rxd->rx_m, sizeof(MSK_DMA_BUF));

when merging the patch, unless there is a pressing reason not to.

Feel free, but: gBS->CopyMem (&m, &rxd->rx_m, sizeof(m)); is a little safer (sizeof(m) instead of sizeof(MSK_DMA_BUF)), but of course the struct assignment method is the safest (with compile-time checks for equivalent types and automatic determination of size (which already helped me once today)).

Whatever you feel needs to be done is fine.

OK

...

...

> 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; > + } > + > + Status = mPciIo->Flush (mPciIo); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n")); > } > + > + Status = mPciIo->Unmap (mPciIo, m.DmaMapping); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Unmap DMA\n")); > + } > + > Status = gBS->AllocatePool (EfiBootServicesData, > sizeof (MSK_LINKED_DMA_BUF), > (VOID**) &m_link); > if (!EFI_ERROR (Status)) { > gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0); > - rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; > - > m_link->Signature = RX_MBUF_SIGNATURE; > Status = gBS->AllocatePool (EfiBootServicesData, > len, > (VOID**) &m_link->DmaBuf.Buf); > if(!EFI_ERROR (Status)) { > - gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len); > + gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); > m_link->DmaBuf.Length = len; > - m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping; > + m_link->DmaBuf.DmaMapping = m.DmaMapping; > Do we still need DmaMapping after we unmapped the buffer?

Certainly not, but the consumer of that queue doesn't do anything with the DMA mapping, which is why it didn't cause an issue, even before my patches.

The Receive Queue should use a MSK_LINKED_SYSTEM_BUF instead of a MSK_LINKED_DMA_BUF. I made a patch for this, which I will send tomorrow (Tuesday) as part of v7.

OK, but before you do that, could we get rid of the PciIo->AllocateBuffer()

I've acutally been testing with something similar already (although using gBS->AllocatePages() as you originally suggested), but I'd like to do one thing at a time.

OK, fair enough.

...

Further, from my reading of the documents, I'm not even sure it's right to try to DMA to memory that's not allocated by the PciIo. I had partially typed up an email to this effect, but I was hoping for one thing at a time. But since you brought it up...

What do you think about section 5.1.1.2 of the Driver Writer's Guide where it says:

...

A UEFI driver must never directly allocate a memory buffer for DMA access. The UEFI driver cannot know enough about the system architecture to predict what system memory areas are available for DMA or if CPU caches are coherent with DMA operations. Instead, a UEFI driver must use the services provided by the I/O protocol for the bus to allocate and free buffers required for DMA operations. There should also be services to initiate and complete DMA transactions. For example, the PCI Root Bridge I/O Protocol and PCI I/O Protocol both provide services for PCI DMA operations. As additional I/O bus types with DMA capabilities are introduced, new protocols that abstract the DMA services must be provided.

Similar language is in section 5.3.11. It seems like it's stating you can't make the assumption that the bus is able to perform DMA operations to all of RAM, but that the bus driver knows these (platform- and bus-specific) limitations, so you should use the bus (in this case PciIo), to allocate the memory.

However, the examples of streaming PCI DMA in the DWG do _not_ do an allocation this way, and rely on a pointer passed in from outside the example code (DWG 18.5.7).

I've looked hard in the UEFI specification, but I can't find anything else which corroborates this. The UEFI spec only seems to mandate PciIo->AllocateBuffer() for common buffers (EfiPciOperationBusMasterCommonBuffer). This leads me to suspect the above quoted paragraph (DWG 5.1.1.2) may only be talking about common buffers and not for streaming buffers, but it's not clear.

No, it is not clear, but AllocateBuffer() is simply not required for streaming DMA. Note that the transmit path in this driver already performs streaming DMA on arbitrary buffers, so there is no reason for the RX path to be different.

In general, the directionality of streaming DMA combined with the restrictions on when the data is valid and accessible (before Map, after Unmap, etc) means that the implementation can reallocate the buffer if it is not located in suitable memory, which is exactly what the non-coherent DmaLib does.

There are two reasons I'd prefer not to use AllocateBuffer() here:

• it gives you uncached memory on non-coherent platforms, which means

that every single read performed by the CopyMem() after flush/unmap goes all the way to main memory

• AllocateBuffer() changes the memory map (and so does

AllocatePages(), which is why I suggested AllocatePool() instead in the second instance), which is costly

Ok, that's fine with me. I'll patch this later today after sending v7.

...

...

In my testing, your suggestion (AllocatePages()) works, but I'm not convinced it's correct.

Either way, I'm going to push for this being a separate patch than the current patch series. I'm happy to do it (if we decide ultimately that it is correct), but it's really a separate problem (as you've indicated before).

Let me know if this is ok with you.

Sure.

...

...

and the redundant copy as well? Something like below, but in a way that it doesn't leak memory?

Can this also be a separate from the series? Again, it's a separate problem. Like the above, I'm happy to do it, but rebasing this same set continuously is getting tiresome. v7 is nearly ready, and I'd like it to be the last one. I'm happy to submit a second series after this series is in, to optimize these issues you point out.

Let me know, and again, thanks for your review and interest in this series.

No, that is perfectly fine. I am very happy you are willing to spend more time on this, since I don't have access to the hardware (and am not that familiar with this version or the BSD version of the driver)

It just seemed that changing from MSK_DMA_BUF to MSK_SYSTEM_BUF etc would combine naturally with the changes above, but perhaps not. It's up to you.

The MSK_DMA_BUF/MSK_SYSTEM_BUF change wasn't too bad as the structures are similar and the structure of the code flow is the same.

I'll take out the extra CopyMem() too. It really just moves the Free*() to a different place. Expect some patches later today.

Awesome, thanks!

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

This patch makes this driver use 64-bit DMA in all cases, because it was determined that there is no good way to test whether 64-bit DMA is required.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 55 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 16 ++++++++++ 2 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 31c7d95..246b73a 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod; + INTN nbuf; EFI_STATUS Status;

sc_if->msk_cdata.msk_rx_cons = 0; @@ -500,17 +501,22 @@ msk_init_rx_ring (

rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0); - prod = sc_if->msk_cdata.msk_rx_prod; - for (i = 0; i < MSK_RX_RING_CNT; i++) { + for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod]; - Status = msk_newbuf (sc_if, prod); - if (EFI_ERROR (Status)) { - return Status; - } MSK_INC (prod, MSK_RX_RING_CNT); } + nbuf = MSK_RX_BUF_CNT; + prod = 0; + + for (i = 0; i < nbuf; i++) { + Status = msk_newbuf (sc_if, prod); + if (EFI_ERROR (Status)) { + return Status; + } + MSK_RX_INC(prod, MSK_RX_RING_CNT); + }

// Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1; @@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0; + sc_if->msk_cdata.msk_tx_high_addr = 0;

rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) * MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf (

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

+#ifdef MSK_64BIT_DMA + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; + rx_le = rxd->rx_le; + rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + MSK_INC(idx, MSK_RX_RING_CNT); +#endif + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le; @@ -594,6 +608,14 @@ msk_newbuf ( return Status; }

+#ifdef MSK_64BIT_DMA + rx_le = rxd->rx_le; + rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr)); + rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + MSK_INC(idx, MSK_RX_RING_CNT); + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; +#endif + gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer; @@ -1649,6 +1671,19 @@ msk_encap (

control = 0;

+#ifdef MSK_64BIT_DMA + if (MSK_ADDR_HI(BusPhysAddr) != + sc_if->msk_cdata.msk_tx_high_addr) { + sc_if->msk_cdata.msk_tx_high_addr = + MSK_ADDR_HI(BusPhysAddr); + tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; + tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr)); + tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + sc_if->msk_cdata.msk_tx_cnt++; + MSK_INC(prod, MSK_TX_RING_CNT); + } +#endif + si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr)); @@ -1866,7 +1901,11 @@ msk_rxeof ( break; }

+#ifdef MSK_64BIT_DMA + rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT]; +#else rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; +#endif

m = rxd->rx_m; Status = msk_newbuf (sc_if, cons); @@ -1912,8 +1951,8 @@ msk_rxeof ( mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf); } while (0);

- MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT); - MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); + MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT); + MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); }

static diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..c95578d 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,12 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter

+/* Use 64-bit DMA in all cases in UEFI. After much discussion on the mailing + * list, it was determined that there is not currently a good way to detect + * whether 32-bit DMA should be used (if ever) or whether there are any + * supported platforms on which 64-bit DMA would not work */ +#define MSK_64BIT_DMA + #define MSK_TX_RING_CNT 512 #define MSK_RX_RING_CNT 512 #define MSK_RX_BUF_ALIGN 8 @@ -2323,6 +2329,7 @@ struct msk_chain_data { void *msk_tx_ring_map; void *msk_rx_ring_map; // struct msk_rxdesc msk_jumbo_rxdesc[MSK_JUMBO_RX_RING_CNT]; + INTN msk_tx_high_addr; INTN msk_tx_prod; INTN msk_tx_cons; INTN msk_tx_cnt; @@ -2352,6 +2359,15 @@ struct msk_ring_data { #define MSK_STAT_RING_SZ (sizeof (struct msk_stat_desc) * MSK_STAT_RING_CNT)

#define MSK_INC(x, y) ((x) = (x + 1) % y) +#ifdef MSK_64BIT_DMA +#define MSK_RX_INC(x, y) (x) = (x + 2) % y +#define MSK_RX_BUF_CNT (MSK_RX_RING_CNT / 2) +#define MSK_JUMBO_RX_BUF_CNT (MSK_JUMBO_RX_RING_CNT / 2) +#else +#define MSK_RX_INC(x, y) (x) = (x + 1) % y +#define MSK_RX_BUF_CNT MSK_RX_RING_CNT +#define MSK_JUMBO_RX_BUF_CNT MSK_JUMBO_RX_RING_CNT +#endif

#define MSK_PCI_BUS 0 #define MSK_PCIX_BUS 1

msk_rxeof() allocates a link object which contains a buffer. If the buffer can't be allocated, make sure to free the link object.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index c7eee0a..03081eb 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1943,6 +1943,7 @@ msk_rxeof ( InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMA buffer. Dropping Frame\n")); + gBS->FreePool (m_link); } } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n"));

Change the receive buffers to be single-use only. This involves a few changes that work together: 1. Change msk_newbuf() to not attempt to re-use or free a buffer, 2. Change msk_rxeof() free the buffer when it is done with it, 3. Store a temporary copy of the received data for passing to the Receive Queue, 4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons: 1. The optimization failed to work for 64-bit DMA transfers; 2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index d2102dc..2114d90 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf (

rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

- if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) { - return EFI_ALREADY_STARTED; - } else if (rxd->rx_m.Buf != NULL) { - mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping); - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length), rxd->rx_m.Buf); - } - Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status; @@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen; + MSK_DMA_BUF m;

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

@@ -1871,26 +1865,40 @@ msk_rxeof ( break; }

+ rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; + + m = rxd->rx_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); + break; + } + + Status = mPciIo->Flush (mPciIo); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n")); } + + Status = mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Unmap DMA\n")); + } + Status = gBS->AllocatePool (EfiBootServicesData, sizeof (MSK_LINKED_DMA_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0); - rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; - m_link->Signature = RX_MBUF_SIGNATURE; Status = gBS->AllocatePool (EfiBootServicesData, len, (VOID**) &m_link->DmaBuf.Buf); if(!EFI_ERROR (Status)) { - gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len); + gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); m_link->DmaBuf.Length = len; - m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping; + m_link->DmaBuf.DmaMapping = NULL;

InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else { @@ -1899,6 +1907,8 @@ msk_rxeof ( } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n")); } + + mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf); } while (0);

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

The Receive Queue uses system memory for each packet, not DMA memory, so use the MSK_LINKED_SYSTEM_BUF items for this queue instead of MSK_DMA_BUF items.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 2114d90..dc18379 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1804,7 +1804,7 @@ mskc_receive ( OUT VOID *Buffer ) { - MSK_LINKED_DMA_BUF *mBuf; + MSK_LINKED_SYSTEM_BUF *mBuf;

msk_intr (); // check the interrupt lines

@@ -1813,17 +1813,17 @@ mskc_receive ( return EFI_NOT_READY; }

- mBuf = CR (GetFirstNode (&mSoftc->ReceiveQueueHead), MSK_LINKED_DMA_BUF, Link, RX_MBUF_SIGNATURE); - if (mBuf->DmaBuf.Length > *BufferSize) { - *BufferSize = mBuf->DmaBuf.Length; + mBuf = CR (GetFirstNode (&mSoftc->ReceiveQueueHead), MSK_LINKED_SYSTEM_BUF, Link, RX_MBUF_SIGNATURE); + if (mBuf->SystemBuf.Length > *BufferSize) { + *BufferSize = mBuf->SystemBuf.Length; DEBUG ((EFI_D_NET, "Marvell Yukon: Receive buffer is too small: Provided = %d, Received = %d\n", - *BufferSize, mBuf->DmaBuf.Length)); + *BufferSize, mBuf->SystemBuf.Length)); return EFI_BUFFER_TOO_SMALL; } - *BufferSize = mBuf->DmaBuf.Length; + *BufferSize = mBuf->SystemBuf.Length; RemoveEntryList (&mBuf->Link); - gBS->CopyMem (Buffer, mBuf->DmaBuf.Buf, *BufferSize); - gBS->FreePool(mBuf->DmaBuf.Buf); + gBS->CopyMem (Buffer, mBuf->SystemBuf.Buf, *BufferSize); + gBS->FreePool(mBuf->SystemBuf.Buf); gBS->FreePool (mBuf); return EFI_SUCCESS; } @@ -1837,7 +1837,7 @@ msk_rxeof ( ) { EFI_STATUS Status; - MSK_LINKED_DMA_BUF *m_link; + MSK_LINKED_SYSTEM_BUF *m_link; struct msk_rxdesc *rxd; INTN cons; INTN rxlen; @@ -1887,18 +1887,17 @@ msk_rxeof ( }

Status = gBS->AllocatePool (EfiBootServicesData, - sizeof (MSK_LINKED_DMA_BUF), + sizeof (MSK_LINKED_SYSTEM_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { - gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0); + gBS->SetMem (m_link, sizeof (MSK_LINKED_SYSTEM_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE; Status = gBS->AllocatePool (EfiBootServicesData, len, - (VOID**) &m_link->DmaBuf.Buf); + (VOID**) &m_link->SystemBuf.Buf); if(!EFI_ERROR (Status)) { - gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); - m_link->DmaBuf.Length = len; - m_link->DmaBuf.DmaMapping = NULL; + gBS->CopyMem (m_link->SystemBuf.Buf, m.Buf, len); + m_link->SystemBuf.Length = len;

InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else {

The Receive Queue uses system memory for each packet, not DMA memory, so use the MSK_LINKED_SYSTEM_BUF items for this queue instead of MSK_DMA_BUF items.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 2114d90..dc18379 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1804,7 +1804,7 @@ mskc_receive ( OUT VOID *Buffer ) { - MSK_LINKED_DMA_BUF *mBuf; + MSK_LINKED_SYSTEM_BUF *mBuf;

msk_intr (); // check the interrupt lines

@@ -1813,17 +1813,17 @@ mskc_receive ( return EFI_NOT_READY; }

- mBuf = CR (GetFirstNode (&mSoftc->ReceiveQueueHead), MSK_LINKED_DMA_BUF, Link, RX_MBUF_SIGNATURE); - if (mBuf->DmaBuf.Length > *BufferSize) { - *BufferSize = mBuf->DmaBuf.Length; + mBuf = CR (GetFirstNode (&mSoftc->ReceiveQueueHead), MSK_LINKED_SYSTEM_BUF, Link, RX_MBUF_SIGNATURE); + if (mBuf->SystemBuf.Length > *BufferSize) { + *BufferSize = mBuf->SystemBuf.Length; DEBUG ((EFI_D_NET, "Marvell Yukon: Receive buffer is too small: Provided = %d, Received = %d\n", - *BufferSize, mBuf->DmaBuf.Length)); + *BufferSize, mBuf->SystemBuf.Length)); return EFI_BUFFER_TOO_SMALL; } - *BufferSize = mBuf->DmaBuf.Length; + *BufferSize = mBuf->SystemBuf.Length; RemoveEntryList (&mBuf->Link); - gBS->CopyMem (Buffer, mBuf->DmaBuf.Buf, *BufferSize); - gBS->FreePool(mBuf->DmaBuf.Buf); + gBS->CopyMem (Buffer, mBuf->SystemBuf.Buf, *BufferSize); + gBS->FreePool(mBuf->SystemBuf.Buf); gBS->FreePool (mBuf); return EFI_SUCCESS; } @@ -1837,7 +1837,7 @@ msk_rxeof ( ) { EFI_STATUS Status; - MSK_LINKED_DMA_BUF *m_link; + MSK_LINKED_SYSTEM_BUF *m_link; struct msk_rxdesc *rxd; INTN cons; INTN rxlen; @@ -1887,18 +1887,17 @@ msk_rxeof ( }

Status = gBS->AllocatePool (EfiBootServicesData, - sizeof (MSK_LINKED_DMA_BUF), + sizeof (MSK_LINKED_SYSTEM_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { - gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0); + gBS->SetMem (m_link, sizeof (MSK_LINKED_SYSTEM_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE; Status = gBS->AllocatePool (EfiBootServicesData, len, - (VOID**) &m_link->DmaBuf.Buf); + (VOID**) &m_link->SystemBuf.Buf); if(!EFI_ERROR (Status)) { - gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); - m_link->DmaBuf.Length = len; - m_link->DmaBuf.DmaMapping = NULL; + gBS->CopyMem (m_link->SystemBuf.Buf, m.Buf, len); + m_link->SystemBuf.Length = len;

InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else {

Add support for 64-bit DMA transfers, since some 64-bit platforms don't have the ability to generate DMA addresses which can fit in 32-bits.

This code came from the FreeBSD driver, the one from which this driver was derived.

This patch makes this driver use 64-bit DMA in all cases, because it was determined that there is no good way to test whether 64-bit DMA is required.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 55 ++++++++++++++++++++++++++++----- Drivers/Net/MarvellYukonDxe/if_mskreg.h | 16 ++++++++++ 2 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 6a395bd..eedf717 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -492,6 +492,7 @@ msk_init_rx_ring ( struct msk_rxdesc *rxd; INTN i; INTN prod; + INTN nbuf; EFI_STATUS Status;

sc_if->msk_cdata.msk_rx_cons = 0; @@ -500,17 +501,22 @@ msk_init_rx_ring (

rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_rx_ring, MSK_RX_RING_SZ, 0); - prod = sc_if->msk_cdata.msk_rx_prod; - for (i = 0; i < MSK_RX_RING_CNT; i++) { + for (i = prod = 0; i < MSK_RX_RING_CNT; i++) { rxd = &sc_if->msk_cdata.msk_rxdesc[prod]; gBS->SetMem (&rxd->rx_m, sizeof (MSK_DMA_BUF), 0); rxd->rx_le = &rd->msk_rx_ring[prod]; - Status = msk_newbuf (sc_if, prod); - if (EFI_ERROR (Status)) { - return Status; - } MSK_INC (prod, MSK_RX_RING_CNT); } + nbuf = MSK_RX_BUF_CNT; + prod = 0; + + for (i = 0; i < nbuf; i++) { + Status = msk_newbuf (sc_if, prod); + if (EFI_ERROR (Status)) { + return Status; + } + MSK_RX_INC(prod, MSK_RX_RING_CNT); + }

// Update prefetch unit. sc_if->msk_cdata.msk_rx_prod = MSK_RX_RING_CNT - 1; @@ -532,6 +538,7 @@ msk_init_tx_ring ( sc_if->msk_cdata.msk_tx_prod = 0; sc_if->msk_cdata.msk_tx_cons = 0; sc_if->msk_cdata.msk_tx_cnt = 0; + sc_if->msk_cdata.msk_tx_high_addr = 0;

rd = &sc_if->msk_rdata; gBS->SetMem (rd->msk_tx_ring, sizeof (struct msk_tx_desc) * MSK_TX_RING_CNT, 0); @@ -556,6 +563,13 @@ msk_discard_rxbuf (

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

+#ifdef MSK_64BIT_DMA + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; + rx_le = rxd->rx_le; + rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + MSK_INC(idx, MSK_RX_RING_CNT); +#endif + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; DmaBuffer = &rxd->rx_m; rx_le = rxd->rx_le; @@ -594,6 +608,14 @@ msk_newbuf ( return Status; }

+#ifdef MSK_64BIT_DMA + rx_le = rxd->rx_le; + rx_le->msk_addr = htole32(MSK_ADDR_HI(PhysAddr)); + rx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + MSK_INC(idx, MSK_RX_RING_CNT); + rxd = &sc_if->msk_cdata.msk_rxdesc[idx]; +#endif + gBS->SetMem (&(rxd->rx_m), sizeof (MSK_DMA_BUF), 0); rxd->rx_m.DmaMapping = Mapping; rxd->rx_m.Buf = Buffer; @@ -1649,6 +1671,19 @@ msk_encap (

control = 0;

+#ifdef MSK_64BIT_DMA + if (MSK_ADDR_HI(BusPhysAddr) != + sc_if->msk_cdata.msk_tx_high_addr) { + sc_if->msk_cdata.msk_tx_high_addr = + MSK_ADDR_HI(BusPhysAddr); + tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; + tx_le->msk_addr = htole32(MSK_ADDR_HI(BusPhysAddr)); + tx_le->msk_control = htole32(OP_ADDR64 | HW_OWNER); + sc_if->msk_cdata.msk_tx_cnt++; + MSK_INC(prod, MSK_TX_RING_CNT); + } +#endif + si = prod; tx_le = &sc_if->msk_rdata.msk_tx_ring[prod]; tx_le->msk_addr = htole32 (MSK_ADDR_LO (BusPhysAddr)); @@ -1866,7 +1901,11 @@ msk_rxeof ( break; }

+#ifdef MSK_64BIT_DMA + rxd = &sc_if->msk_cdata.msk_rxdesc[(cons + 1) % MSK_RX_RING_CNT]; +#else rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; +#endif

m = rxd->rx_m; Status = msk_newbuf (sc_if, cons); @@ -1911,8 +1950,8 @@ msk_rxeof ( mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf); } while (0);

- MSK_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT); - MSK_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); + MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT); + MSK_RX_INC (sc_if->msk_cdata.msk_rx_prod, MSK_RX_RING_CNT); }

static diff --git a/Drivers/Net/MarvellYukonDxe/if_mskreg.h b/Drivers/Net/MarvellYukonDxe/if_mskreg.h index f0dd05e..c95578d 100644 --- a/Drivers/Net/MarvellYukonDxe/if_mskreg.h +++ b/Drivers/Net/MarvellYukonDxe/if_mskreg.h @@ -2239,6 +2239,12 @@ struct msk_stat_desc { #define BMU_UDP_CHECK (0x57<<16) // Descr with UDP ext (YUKON only) #define BMU_BBC 0xffff // Bit 15.. 0: Buffer Byte Counter

+/* Use 64-bit DMA in all cases in UEFI. After much discussion on the mailing + * list, it was determined that there is not currently a good way to detect + * whether 32-bit DMA should be used (if ever) or whether there are any + * supported platforms on which 64-bit DMA would not work */ +#define MSK_64BIT_DMA + #define MSK_TX_RING_CNT 512 #define MSK_RX_RING_CNT 512 #define MSK_RX_BUF_ALIGN 8 @@ -2323,6 +2329,7 @@ struct msk_chain_data { void *msk_tx_ring_map; void *msk_rx_ring_map; // struct msk_rxdesc msk_jumbo_rxdesc[MSK_JUMBO_RX_RING_CNT]; + INTN msk_tx_high_addr; INTN msk_tx_prod; INTN msk_tx_cons; INTN msk_tx_cnt; @@ -2352,6 +2359,15 @@ struct msk_ring_data { #define MSK_STAT_RING_SZ (sizeof (struct msk_stat_desc) * MSK_STAT_RING_CNT)

#define MSK_INC(x, y) ((x) = (x + 1) % y) +#ifdef MSK_64BIT_DMA +#define MSK_RX_INC(x, y) (x) = (x + 2) % y +#define MSK_RX_BUF_CNT (MSK_RX_RING_CNT / 2) +#define MSK_JUMBO_RX_BUF_CNT (MSK_JUMBO_RX_RING_CNT / 2) +#else +#define MSK_RX_INC(x, y) (x) = (x + 1) % y +#define MSK_RX_BUF_CNT MSK_RX_RING_CNT +#define MSK_JUMBO_RX_BUF_CNT MSK_JUMBO_RX_RING_CNT +#endif

#define MSK_PCI_BUS 0 #define MSK_PCIX_BUS 1

msk_rxeof() allocates a link object which contains a buffer. If the buffer can't be allocated, make sure to free the link object.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 126cef5..2fbb096 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1942,6 +1942,7 @@ msk_rxeof ( InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMA buffer. Dropping Frame\n")); + gBS->FreePool (m_link); } } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n"));

Change the receive buffers to be single-use only. This involves a few changes that work together: 1. Change msk_newbuf() to not attempt to re-use or free a buffer, 2. Change msk_rxeof() free the buffer when it is done with it, 3. Store a temporary copy of the received data for passing to the Receive Queue, 4. Call the required Flush() and Unmap() on the DMA buffer.

In addition this means failure to allocate a new buffer is a failure of msk_rxeof().

Note that this change makes the driver work the way the FreeBSD driver (from which this driver was derived) works, and this simply removes an optimization (the code in msk_newbuf() which re-uses the buffers. This removal of the optimization is done for two reasons: 1. The optimization failed to work for 64-bit DMA transfers; 2. The UEFI specification, version 2.6, section 13.4 requires calls to Flush() and Unmap() before reading a DMA write buffer from the CPU, which doesn't fit with the optimization as it existed.

Reverting back to the behavior as it was in the FreeBSD driver solves number 1 and 2 above, and makes this driver more consistent with something we know to be working. There is slightly more overhead, but it is more consistent with the UEFI standard.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk --- Drivers/Net/MarvellYukonDxe/if_msk.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index d2102dc..2114d90 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -581,13 +581,6 @@ msk_newbuf (

rxd = &sc_if->msk_cdata.msk_rxdesc[idx];

- if ((rxd->rx_m.Buf != NULL) && (rxd->rx_m.Length >= Length)) { - return EFI_ALREADY_STARTED; - } else if (rxd->rx_m.Buf != NULL) { - mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping); - mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (rxd->rx_m.Length), rxd->rx_m.Buf); - } - Status = mPciIo->AllocateBuffer (mPciIo, AllocateAnyPages, EfiBootServicesData, EFI_SIZE_TO_PAGES (Length), &Buffer, 0); if (EFI_ERROR (Status)) { return Status; @@ -1848,6 +1841,7 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen; + MSK_DMA_BUF m;

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

@@ -1871,26 +1865,40 @@ msk_rxeof ( break; }

+ rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; + + m = rxd->rx_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); + break; + } + + Status = mPciIo->Flush (mPciIo); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n")); } + + Status = mPciIo->Unmap (mPciIo, rxd->rx_m.DmaMapping); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Unmap DMA\n")); + } + Status = gBS->AllocatePool (EfiBootServicesData, sizeof (MSK_LINKED_DMA_BUF), (VOID**) &m_link); if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0); - rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; - m_link->Signature = RX_MBUF_SIGNATURE; Status = gBS->AllocatePool (EfiBootServicesData, len, (VOID**) &m_link->DmaBuf.Buf); if(!EFI_ERROR (Status)) { - gBS->CopyMem (m_link->DmaBuf.Buf, rxd->rx_m.Buf, len); + gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len); m_link->DmaBuf.Length = len; - m_link->DmaBuf.DmaMapping = rxd->rx_m.DmaMapping; + m_link->DmaBuf.DmaMapping = NULL;

InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else { @@ -1899,6 +1907,8 @@ msk_rxeof ( } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receive buffer link. Dropping Frame\n")); } + + mPciIo->FreeBuffer (mPciIo, EFI_SIZE_TO_PAGES (m.Length), m.Buf); } while (0);

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

On 30 August 2016 at 17:04, Alan Ott alan@softiron.co.uk wrote:

The Receive Queue uses system memory for each packet, not DMA memory, so use the MSK_LINKED_SYSTEM_BUF items for this queue instead of MSK_DMA_BUF items.

Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alan Ott alan@softiron.co.uk

Reviewed-by: Ard Biesheuvel ard.biesheuvel@linaro.org

---

Drivers/Net/MarvellYukonDxe/if_msk.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 2114d90..dc18379 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1804,7 +1804,7 @@ mskc_receive ( OUT VOID *Buffer ) {

• MSK_LINKED_DMA_BUF *mBuf;

• MSK_LINKED_SYSTEM_BUF *mBuf;

  msk_intr (); // check the interrupt lines

@@ -1813,17 +1813,17 @@ mskc_receive ( return EFI_NOT_READY; }

• mBuf = CR (GetFirstNode (&mSoftc->ReceiveQueueHead), MSK_LINKED_DMA_BUF, Link, RX_MBUF_SIGNATURE);
• if (mBuf->DmaBuf.Length > *BufferSize) {
• *BufferSize = mBuf->DmaBuf.Length;

• mBuf = CR (GetFirstNode (&mSoftc->ReceiveQueueHead), MSK_LINKED_SYSTEM_BUF, Link, RX_MBUF_SIGNATURE);
• if (mBuf->SystemBuf.Length > *BufferSize) {
• *BufferSize = mBuf->SystemBuf.Length; DEBUG ((EFI_D_NET, "Marvell Yukon: Receive buffer is too small: Provided = %d, Received = %d\n",

•        *BufferSize, mBuf->DmaBuf.Length));
  

•        *BufferSize, mBuf->SystemBuf.Length));
  

  return EFI_BUFFER_TOO_SMALL; }

• *BufferSize = mBuf->DmaBuf.Length;

• *BufferSize = mBuf->SystemBuf.Length; RemoveEntryList (&mBuf->Link);

• gBS->CopyMem (Buffer, mBuf->DmaBuf.Buf, *BufferSize);
• gBS->FreePool(mBuf->DmaBuf.Buf);

• gBS->CopyMem (Buffer, mBuf->SystemBuf.Buf, *BufferSize);
• gBS->FreePool(mBuf->SystemBuf.Buf); gBS->FreePool (mBuf); return EFI_SUCCESS;

} @@ -1837,7 +1837,7 @@ msk_rxeof ( ) { EFI_STATUS Status;

• MSK_LINKED_DMA_BUF *m_link;

• MSK_LINKED_SYSTEM_BUF *m_link; struct msk_rxdesc *rxd; INTN cons; INTN rxlen;

@@ -1887,18 +1887,17 @@ msk_rxeof ( }

 Status = gBS->AllocatePool (EfiBootServicesData,

•                            sizeof (MSK_LINKED_DMA_BUF),
  

•                            sizeof (MSK_LINKED_SYSTEM_BUF),
                             (VOID**) &m_link);
  

  if (!EFI_ERROR (Status)) {

•  gBS->SetMem (m_link, sizeof (MSK_LINKED_DMA_BUF), 0);
  

•  gBS->SetMem (m_link, sizeof (MSK_LINKED_SYSTEM_BUF), 0);
   m_link->Signature = RX_MBUF_SIGNATURE;
   Status = gBS->AllocatePool (EfiBootServicesData,
                               len,
  

•                              (VOID**) &m_link->DmaBuf.Buf);
  

•                              (VOID**) &m_link->SystemBuf.Buf);
   if(!EFI_ERROR (Status)) {
  

•    gBS->CopyMem (m_link->DmaBuf.Buf, m.Buf, len);
  

•    m_link->DmaBuf.Length = len;
  

•    m_link->DmaBuf.DmaMapping = NULL;
  

•    gBS->CopyMem (m_link->SystemBuf.Buf, m.Buf, len);
  

•    m_link->SystemBuf.Length = len;
  
     InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link);
   } else {
  

-- 2.5.0