From d4e5be3b1bbc0d532c6374a222fd563c0970e070 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 13 Jul 2022 00:59:11 +0000 Subject: [PATCH 1/7] ena(4): Use MI bus_space_barrier for mem BAR vs doorbell ordering. --- sys/dev/pci/if_ena.c | 3 --- sys/external/bsd/ena-com/ena_eth_com.h | 3 +++ sys/external/bsd/ena-com/ena_plat.h | 8 ++++++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/sys/dev/pci/if_ena.c b/sys/dev/pci/if_ena.c index cc59fd9d6b39..653973bd7758 100644 --- a/sys/dev/pci/if_ena.c +++ b/sys/dev/pci/if_ena.c @@ -1187,7 +1187,6 @@ ena_refill_rx_bufs(struct ena_ring *rx_ring, uint32_t num) } if (likely(i != 0)) { - wmb(); ena_com_write_sq_doorbell(rx_ring->ena_com_io_sq); } rx_ring->next_to_use = next_to_use; @@ -3006,7 +3005,6 @@ ena_start_xmit(struct ena_ring *tx_ring) if (unlikely(acum_pkts == DB_THRESHOLD)) { acum_pkts = 0; - wmb(); /* Trigger the dma engine */ ena_com_write_sq_doorbell(io_sq); counter_u64_add(tx_ring->tx_stats.doorbells, 1); @@ -3017,7 +3015,6 @@ ena_start_xmit(struct ena_ring *tx_ring) IF_STAT_PUTREF(adapter->ifp); if (likely(acum_pkts != 0)) { - wmb(); /* Trigger the dma engine */ ena_com_write_sq_doorbell(io_sq); counter_u64_add(tx_ring->tx_stats.doorbells, 1); diff --git a/sys/external/bsd/ena-com/ena_eth_com.h b/sys/external/bsd/ena-com/ena_eth_com.h index d0c8b9080589..09de0806ae47 100644 --- a/sys/external/bsd/ena-com/ena_eth_com.h +++ b/sys/external/bsd/ena-com/ena_eth_com.h @@ -137,6 +137,9 @@ static inline int ena_com_write_sq_doorbell(struct ena_com_io_sq *io_sq) ena_trc_dbg("write submission queue doorbell for queue: %d tail: %d\n", io_sq->qid, tail); + if (io_sq->mem_queue_type == ENA_ADMIN_PLACEMENT_POLICY_DEV) + ENA_MEM_BARRIER_WRITE(io_sq->bus); + ENA_REG_WRITE32(io_sq->bus, tail, io_sq->db_addr); return 0; diff --git a/sys/external/bsd/ena-com/ena_plat.h b/sys/external/bsd/ena-com/ena_plat.h index 305f371b27c7..b8de672f7f31 100644 --- a/sys/external/bsd/ena-com/ena_plat.h +++ b/sys/external/bsd/ena-com/ena_plat.h @@ -321,6 +321,14 @@ int ena_dma_alloc(device_t dmadev, bus_size_t size, ena_mem_handle_t *dma, (virt) = NULL; \ } while (0) +#define ENA_MEM_BARRIER_WRITE(bus) \ + bus_space_barrier( \ + ((struct ena_bus*)bus)->mem_bar_t, \ + ((struct ena_bus*)bus)->mem_bar_h, \ + 0, \ + 1 /* XXX */, \ + BUS_SPACE_BARRIER_WRITE) + /* Register R/W methods */ #define ENA_REG_WRITE32(bus, value, offset) \ bus_space_write_4( \ From 7dd6ea7dbf5f5ffe6a9acfe53bd5c2e76e42c565 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 13 Jul 2022 00:59:11 +0000 Subject: [PATCH 2/7] ena(4): Remove nonsensical wmb(). The wmb() was placed essentially to order: memcpy(bouncebuf, ..., n) wmb() memcpy(iomem, bouncebuf, n) However, this is already ordered by the data dependency through bouncebuf. So there is no need for wmb() here. There may be a need for wmb() before ringing the doorbell -- but that has to happen _between_ memcpy(iomem, ...) and ringing the doorbell, not before it. --- sys/external/bsd/ena-com/ena_eth_com.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/sys/external/bsd/ena-com/ena_eth_com.c b/sys/external/bsd/ena-com/ena_eth_com.c index 190de1f0ea42..a6581933e031 100644 --- a/sys/external/bsd/ena-com/ena_eth_com.c +++ b/sys/external/bsd/ena-com/ena_eth_com.c @@ -87,11 +87,6 @@ static inline void ena_com_write_bounce_buffer_to_dev(struct ena_com_io_sq *io_s dst_tail_mask = io_sq->tail & (io_sq->q_depth - 1); dst_offset = dst_tail_mask * llq_info->desc_list_entry_size; - /* Make sure everything was written into the bounce buffer before - * writing the bounce buffer to the device - */ - wmb(); - /* The line is completed. Copy it to dev */ ENA_MEMCPY_TO_DEVICE_64(io_sq->desc_addr.pbuf_dev_addr + dst_offset, bounce_buffer, From eccfa83e7285402b4d2be7bdc7af9798de1190b3 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 13 Jul 2022 01:00:07 +0000 Subject: [PATCH 3/7] ena(4): Use appropriately paired and ordered bus_dmamap_syncs. --- sys/external/bsd/ena-com/ena_com.c | 77 ++++++++++++++++++++++++++--- sys/external/bsd/ena-com/ena_plat.h | 14 ++++-- 2 files changed, 80 insertions(+), 11 deletions(-) diff --git a/sys/external/bsd/ena-com/ena_com.c b/sys/external/bsd/ena-com/ena_com.c index 4314b31cbb59..96f252d63a8f 100644 --- a/sys/external/bsd/ena-com/ena_com.c +++ b/sys/external/bsd/ena-com/ena_com.c @@ -286,7 +286,21 @@ static struct ena_comp_ctx *__ena_com_submit_admin_cmd(struct ena_com_admin_queu if (unlikely((admin_queue->sq.tail & queue_size_mask) == 0)) admin_queue->sq.phase = !admin_queue->sq.phase; - ENA_DB_SYNC(&admin_queue->sq.mem_handle); + /* + * cq preread matches postread in ena_com_handle_admin_completion + * (not sure of final size or where it'll go, so sync whole queue) + */ + ena_dma_sync(&admin_queue->cq.mem_handle, + 0, ADMIN_CQ_SIZE(admin_queue->q_depth), + ENA_DMASYNC_PREREAD); + + /* sq prewrite matches postwrite in ena_com_handle_admin_completion */ + ena_dma_sync(&admin_queue->sq.mem_handle, + ((char *)&admin_queue->sq.entries[tail_masked] - + (char *)admin_queue->sq.entries), + cmd_size_in_bytes, + ENA_DMASYNC_PREWRITE); + ENA_REG_WRITE32(admin_queue->bus, admin_queue->sq.tail, admin_queue->sq.db_addr); @@ -500,14 +514,32 @@ static void ena_com_handle_admin_completion(struct ena_com_admin_queue *admin_qu cqe = &admin_queue->cq.entries[head_masked]; /* Go over all the completions */ - while ((cqe->acq_common_descriptor.flags & - ENA_ADMIN_ACQ_COMMON_DESC_PHASE_MASK) == phase) { + for (;;) { + /* cq postread matches preread in __ena_com_submit_admin_cmd */ + ena_dma_sync(&admin_queue->cq.mem_handle, + (char *)cqe - (char *)admin_queue->cq.entries, + sizeof(cqe->acq_common_descriptor.flags), + ENA_DMASYNC_POSTREAD); + if ((cqe->acq_common_descriptor.flags & + ENA_ADMIN_ACQ_COMMON_DESC_PHASE_MASK) != phase) + break; /* Do not read the rest of the completion entry before the * phase bit was validated */ - rmb(); + ena_dma_sync(&admin_queue->cq.mem_handle, + (char *)cqe - (char *)admin_queue->cq.entries, + sizeof(*cqe), + ENA_DMASYNC_POSTREAD); ena_com_handle_single_admin_completion(admin_queue, cqe); + /* + * sq postwrite matches prewrite in __ena_com_submit_admin_cmd + * (not sure where it came from, so sync whole queue) + */ + ena_dma_sync(&admin_queue->sq.mem_handle, + 0, ADMIN_SQ_SIZE(admin_queue->q_depth), + ENA_DMASYNC_POSTWRITE); + head_masked++; comp_num++; if (unlikely(head_masked == admin_queue->q_depth)) { @@ -739,11 +771,17 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset) /* make sure read_resp->req_id get updated before the hw can write * there */ - wmb(); + ena_dma_sync(&mmio_read->read_resp_mem_handle, 0, sizeof(*read_resp), + ENA_DMASYNC_PREREAD|ENA_DMASYNC_PREWRITE); ENA_REG_WRITE32(ena_dev->bus, mmio_read_reg, ena_dev->reg_bar + ENA_REGS_MMIO_REG_READ_OFF); for (i = 0; i < timeout; i++) { + ena_dma_sync(&mmio_read->read_resp_mem_handle, + offsetof(struct ena_admin_ena_mmio_req_read_less_resp, + req_id), + sizeof(read_resp->req_id), + ENA_DMASYNC_POSTREAD); if (read_resp->req_id == mmio_read->seq_num) break; @@ -760,6 +798,8 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset) goto err; } + ena_dma_sync(&mmio_read->read_resp_mem_handle, 0, sizeof(*read_resp), + ENA_DMASYNC_POSTREAD|ENA_DMASYNC_POSTWRITE); if (read_resp->reg_off != offset) { ena_trc_err("Read failure: wrong offset provided"); ret = ENA_MMIO_READ_TIMEOUT; @@ -1449,6 +1489,10 @@ void ena_com_admin_aenq_enable(struct ena_com_dev *ena_dev) /* Init head_db to mark that all entries in the queue * are initially available */ + /* preread matches postread in ena_com_aenq_intr_handler */ + ena_dma_sync(&ena_dev->aenq.mem_handle, + 0, ADMIN_AENQ_SIZE(ena_dev->aenq.q_depth), + ENA_DMASYNC_PREREAD); ENA_REG_WRITE32(ena_dev->bus, depth, ena_dev->reg_bar + ENA_REGS_AENQ_HEAD_DB_OFF); } @@ -1955,8 +1999,23 @@ void ena_com_aenq_intr_handler(struct ena_com_dev *dev, void *data) aenq_common = &aenq_e->aenq_common_desc; /* Go over all the events */ - while ((aenq_common->flags & ENA_ADMIN_AENQ_COMMON_DESC_PHASE_MASK) == - phase) { + for (;;) { + /* + * postread matches preread either in + * ena_com_admin_aenq_enable or at the end of this + * function + */ + ena_dma_sync(&aenq->mem_handle, + (char *)&aenq_common->flags - (char *)aenq->entries, + sizeof(aenq_common->flags), + ENA_DMASYNC_POSTREAD); + if ((aenq_common->flags & + ENA_ADMIN_AENQ_COMMON_DESC_PHASE_MASK) != phase) + break; + ena_dma_sync(&aenq->mem_handle, + (char *)aenq_common - (char *)aenq->entries, + sizeof(aenq_common), + ENA_DMASYNC_POSTREAD); timestamp = (unsigned long long)aenq_common->timestamp_low | ((unsigned long long)aenq_common->timestamp_high << 32); ena_trc_dbg("AENQ! Group[%x] Syndrom[%x] timestamp: [%llus]\n", @@ -1989,7 +2048,9 @@ void ena_com_aenq_intr_handler(struct ena_com_dev *dev, void *data) return; /* write the aenq doorbell after all AENQ descriptors were read */ - mb(); + /* preread matches postread earlier in this function when run again */ + ena_dma_sync(&aenq->mem_handle, 0, ADMIN_AENQ_SIZE(aenq->q_depth), + ENA_DMASYNC_PREREAD); ENA_REG_WRITE32(dev->bus, (u32)aenq->head, dev->reg_bar + ENA_REGS_AENQ_HEAD_DB_OFF); } #ifdef ENA_EXTENDED_STATS diff --git a/sys/external/bsd/ena-com/ena_plat.h b/sys/external/bsd/ena-com/ena_plat.h index b8de672f7f31..4b598354ae29 100644 --- a/sys/external/bsd/ena-com/ena_plat.h +++ b/sys/external/bsd/ena-com/ena_plat.h @@ -342,9 +342,17 @@ int ena_dma_alloc(device_t dmadev, bus_size_t size, ena_mem_handle_t *dma, ((struct ena_bus*)bus)->reg_bar_h, \ (bus_size_t)(offset)) -#define ENA_DB_SYNC(mem_handle) bus_dmamap_sync((mem_handle)->tag, \ - (mem_handle)->map, 0, (mem_handle)->map->dm_mapsize, \ - BUS_DMASYNC_PREREAD) +#define ENA_DMASYNC_PREREAD BUS_DMASYNC_PREREAD +#define ENA_DMASYNC_POSTREAD BUS_DMASYNC_POSTREAD +#define ENA_DMASYNC_PREWRITE BUS_DMASYNC_PREWRITE +#define ENA_DMASYNC_POSTWRITE BUS_DMASYNC_POSTWRITE + +static inline void +ena_dma_sync(ena_mem_handle_t *h, bus_addr_t offset, bus_size_t len, int ops) +{ + + bus_dmamap_sync(h->tag, h->map, offset, len, ops); +} #define time_after(a,b) ((long)((unsigned long)(b) - (unsigned long)(a)) < 0) From 30e47071dd07aac70a1ab25029d96341a1ef0d64 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 13 Jul 2022 01:00:08 +0000 Subject: [PATCH 4/7] ena(4): Order access to reset_reason and ENA_FLAG_TRIGGER_RESET. --- sys/dev/pci/if_ena.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sys/dev/pci/if_ena.c b/sys/dev/pci/if_ena.c index 653973bd7758..97696d1cd98a 100644 --- a/sys/dev/pci/if_ena.c +++ b/sys/dev/pci/if_ena.c @@ -40,6 +40,8 @@ __KERNEL_RCSID(0, "$NetBSD: if_ena.c,v 1.33 2022/05/23 13:53:37 rin Exp $"); #include #include + +#include #include #include #include @@ -865,6 +867,7 @@ validate_rx_req_id(struct ena_ring *rx_ring, uint16_t req_id) /* Trigger device reset */ rx_ring->adapter->reset_reason = ENA_REGS_RESET_INV_RX_REQ_ID; + membar_release(); ENA_FLAG_SET_ATOMIC(ENA_FLAG_TRIGGER_RESET, rx_ring->adapter); return (EFAULT); @@ -2677,6 +2680,7 @@ ena_down(struct ena_adapter *adapter) } if (ENA_FLAG_ISSET(ENA_FLAG_TRIGGER_RESET, adapter)) { + membar_acquire(); rc = ena_com_dev_reset(adapter->ena_dev, adapter->reset_reason); if (unlikely(rc != 0)) @@ -3474,6 +3478,7 @@ static void check_for_missing_keep_alive(struct ena_adapter *adapter) "Keep alive watchdog timeout.\n"); counter_u64_add(adapter->dev_stats.wd_expired, 1); adapter->reset_reason = ENA_REGS_RESET_KEEP_ALIVE_TO; + membar_release(); ENA_FLAG_SET_ATOMIC(ENA_FLAG_TRIGGER_RESET, adapter); } } @@ -3487,6 +3492,7 @@ static void check_for_admin_com_state(struct ena_adapter *adapter) "ENA admin queue is not in running state!\n"); counter_u64_add(adapter->dev_stats.admin_q_pause, 1); adapter->reset_reason = ENA_REGS_RESET_ADMIN_TO; + membar_release(); ENA_FLAG_SET_ATOMIC(ENA_FLAG_TRIGGER_RESET, adapter); } } @@ -3532,6 +3538,7 @@ check_missing_comp_in_queue(struct ena_adapter *adapter, missed_tx, adapter->missing_tx_threshold); adapter->reset_reason = ENA_REGS_RESET_MISS_TX_CMPL; + membar_release(); ENA_FLAG_SET_ATOMIC(ENA_FLAG_TRIGGER_RESET, adapter); return (EIO); From c161b9a76eae9df92c7fa723559c9deb31e34ee6 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 13 Jul 2022 01:00:08 +0000 Subject: [PATCH 5/7] ena(4): Remove nonsensical rmb(). 1. In Linux this is smp_rmb(), so it's only about software coordination between CPUs -- no need for DMA or MMIO barriers or anything. If anything, this should be membar_acquire (or, less likely, membar_consumer). 2. I can't find what the corresponding smp_wmb() (i.e., membar_release, or membar_producer) is supposed to be. --- sys/dev/pci/if_ena.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/sys/dev/pci/if_ena.c b/sys/dev/pci/if_ena.c index 97696d1cd98a..df4c11ec6cb3 100644 --- a/sys/dev/pci/if_ena.c +++ b/sys/dev/pci/if_ena.c @@ -3561,9 +3561,6 @@ check_for_missing_tx_completions(struct ena_adapter *adapter) struct ena_ring *tx_ring; int i, budget, rc; - /* Make sure the driver doesn't turn the device in other process */ - rmb(); - if (!ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter)) return; From f9dd877f6a36f68cdde43f084cb6a2c35d37b0e0 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 13 Jul 2022 01:00:09 +0000 Subject: [PATCH 6/7] ena(4): Remove compat definitions of rmb/wmb/mb. Driver has been converted to use bus_space_barrier and bus_dmamap_sync as appropriate. --- sys/external/bsd/ena-com/ena_plat.h | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/sys/external/bsd/ena-com/ena_plat.h b/sys/external/bsd/ena-com/ena_plat.h index 4b598354ae29..0823b462d08f 100644 --- a/sys/external/bsd/ena-com/ena_plat.h +++ b/sys/external/bsd/ena-com/ena_plat.h @@ -394,23 +394,4 @@ void prefetch(void *x) #include "ena_defs/ena_includes.h" -/* - * XXX This is not really right. Need to adjust the driver to use - * bus_space_barrier or bus_dmamap_sync. - */ -#if defined(__i386__) || defined(__x86_64__) -#include -#define rmb() x86_lfence() -#define wmb() x86_sfence() -#define mb() x86_mfence() -#elif defined(__aarch64__) -#define rmb() __asm __volatile("dsb ld" ::: "memory") -#define wmb() __asm __volatile("dsb st" ::: "memory") -#define mb() __asm __volatile("dsb sy" ::: "memory") -#else -#define rmb() membar_acquire() -#define wmb() membar_release() -#define mb() membar_sync() -#endif - #endif /* ENA_PLAT_H_ */ From 5ac89cd7dfbcbfde69a87ed30d2067bb631dc3af Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 13 Jul 2022 01:00:10 +0000 Subject: [PATCH 7/7] ena(4): Add missing bus_dmamap_syncs to if_ena.c. --- sys/dev/pci/if_ena.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sys/dev/pci/if_ena.c b/sys/dev/pci/if_ena.c index df4c11ec6cb3..8e41fcc5aba6 100644 --- a/sys/dev/pci/if_ena.c +++ b/sys/dev/pci/if_ena.c @@ -1126,6 +1126,8 @@ ena_free_rx_mbuf(struct ena_adapter *adapter, struct ena_ring *rx_ring, return; } + bus_dmamap_sync(adapter->sc_dmat, rx_info->map, 0, + rx_info->map->dm_mapsize, BUS_DMASYNC_POSTREAD); bus_dmamap_unload(adapter->sc_dmat, rx_info->map); m_freem(rx_info->mbuf); rx_info->mbuf = NULL; @@ -1269,6 +1271,8 @@ ena_free_tx_bufs(struct ena_adapter *adapter, unsigned int qid) qid, i); } + bus_dmamap_sync(adapter->sc_dmat, tx_info->map, 0, + tx_info->map->dm_mapsize, BUS_DMASYNC_POSTWRITE); bus_dmamap_unload(adapter->sc_dmat, tx_info->map); m_free(tx_info->mbuf); tx_info->mbuf = NULL; @@ -1471,6 +1475,8 @@ ena_tx_cleanup(struct ena_ring *tx_ring) if (likely(tx_info->num_of_bufs != 0)) { /* Map is no longer required */ + bus_dmamap_sync(adapter->sc_dmat, tx_info->map, 0, + tx_info->map->dm_mapsize, BUS_DMASYNC_POSTWRITE); bus_dmamap_unload(adapter->sc_dmat, tx_info->map); } @@ -1617,6 +1623,8 @@ ena_rx_mbuf(struct ena_ring *rx_ring, struct ena_com_rx_buf_info *ena_bufs, mbuf, mbuf->m_flags, mbuf->m_pkthdr.len); /* DMA address is not needed anymore, unmap it */ + bus_dmamap_sync(rx_ring->adapter->sc_dmat, rx_info->map, 0, + rx_info->map->dm_mapsize, BUS_DMASYNC_POSTREAD); bus_dmamap_unload(rx_ring->adapter->sc_dmat, rx_info->map); rx_info->mbuf = NULL; @@ -1659,6 +1667,8 @@ ena_rx_mbuf(struct ena_ring *rx_ring, struct ena_com_rx_buf_info *ena_bufs, "rx mbuf updated. len %d", mbuf->m_pkthdr.len); /* Free already appended mbuf, it won't be useful anymore */ + bus_dmamap_sync(rx_ring->adapter->sc_dmat, rx_info->map, 0, + rx_info->map->dm_mapsize, BUS_DMASYNC_POSTREAD); bus_dmamap_unload(rx_ring->adapter->sc_dmat, rx_info->map); m_freem(rx_info->mbuf); rx_info->mbuf = NULL;