All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/9] igb: add missing feature set from
@ 2023-01-28 13:46 Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 1/9] MAINTAINERS: Add Sriram Yagnaraman as a igb reviewer Sriram Yagnaraman
                   ` (8 more replies)
  0 siblings, 9 replies; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

Based-on: <20230126111943.38695-1-akihiko.odaki@daynix.com>
([PATCH v4 00/13] Introduce igb)

Rebased on latest changes from Akihiko, and merged changes from my
original patchset:
https://lists.gnu.org/archive/html/qemu-devel/2022-12/msg04670.html

Sriram Yagnaraman (9):
  MAINTAINERS: Add Sriram Yagnaraman as a igb reviewer
  igb: handle PF/VF reset properly
  igb: implement VFRE and VFTE registers
  igb: check oversized packets for VMDq
  igb: respect E1000_VMOLR_RSSE
  igb: add ICR_RXDW
  igb: implement VF Tx and Rx stats
  igb: respect VT_CTL ignore MAC field
  igb: respect VMVIR and VMOLR for VLAN

 MAINTAINERS          |   1 +
 hw/net/e1000x_regs.h |   5 +
 hw/net/igb_core.c    | 255 ++++++++++++++++++++++++++++++++-----------
 hw/net/igb_regs.h    |   3 +-
 hw/net/trace-events  |   4 +
 5 files changed, 205 insertions(+), 63 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/9] MAINTAINERS: Add Sriram Yagnaraman as a igb reviewer
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 2/9] igb: handle PF/VF reset properly Sriram Yagnaraman
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

I would like to review and be informed on changes to igb device

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ece23b2b15..7d0e84ce37 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2224,6 +2224,7 @@ F: tests/qtest/libqos/e1000e.*
 
 igb
 M: Akihiko Odaki <akihiko.odaki@daynix.com>
+R: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
 S: Maintained
 F: docs/system/devices/igb.rst
 F: hw/net/igb*
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/9] igb: handle PF/VF reset properly
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 1/9] MAINTAINERS: Add Sriram Yagnaraman as a igb reviewer Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-29  5:58   ` Akihiko Odaki
  2023-01-28 13:46 ` [PATCH 3/9] igb: implement VFRE and VFTE registers Sriram Yagnaraman
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

Use PFRSTD to reset RSTI bit for VFs, and raise VFLRE interrupt when VF
is reset.

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 hw/net/e1000x_regs.h |  1 +
 hw/net/igb_core.c    | 33 +++++++++++++++++++++------------
 hw/net/trace-events  |  2 ++
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/hw/net/e1000x_regs.h b/hw/net/e1000x_regs.h
index fb5b861135..bb3fb36b8d 100644
--- a/hw/net/e1000x_regs.h
+++ b/hw/net/e1000x_regs.h
@@ -548,6 +548,7 @@
 
 #define E1000_CTRL_EXT_ASDCHK  0x00001000 /* auto speed detection check */
 #define E1000_CTRL_EXT_EE_RST  0x00002000 /* EEPROM reset */
+#define E1000_CTRL_EXT_PFRSTD  0x00004000 /* PF reset done indication */
 #define E1000_CTRL_EXT_LINK_EN 0x00010000 /* enable link status from external LINK_0 and LINK_1 pins */
 #define E1000_CTRL_EXT_DRV_LOAD 0x10000000 /* Driver loaded bit for FW */
 #define E1000_CTRL_EXT_EIAME   0x01000000
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index abeb9c7889..9bd53cc25f 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -1902,14 +1902,6 @@ static void igb_set_eims(IGBCore *core, int index, uint32_t val)
     igb_update_interrupt_state(core);
 }
 
-static void igb_vf_reset(IGBCore *core, uint16_t vfn)
-{
-    /* TODO: Reset of the queue enable and the interrupt registers of the VF. */
-
-    core->mac[V2PMAILBOX0 + vfn] &= ~E1000_V2PMAILBOX_RSTI;
-    core->mac[V2PMAILBOX0 + vfn] = E1000_V2PMAILBOX_RSTD;
-}
-
 static void mailbox_interrupt_to_vf(IGBCore *core, uint16_t vfn)
 {
     uint32_t ent = core->mac[VTIVAR_MISC + vfn];
@@ -1987,6 +1979,17 @@ static void igb_set_vfmailbox(IGBCore *core, int index, uint32_t val)
     }
 }
 
+static void igb_vf_reset(IGBCore *core, uint16_t vfn)
+{
+    /* disable Rx and Tx for the VF*/
+    core->mac[VFTE] &= ~BIT(vfn);
+    core->mac[VFRE] &= ~BIT(vfn);
+    /* indicate VF reset to PF */
+    core->mac[VFLRE] |= BIT(vfn);
+    /* VFLRE and mailbox use the same interrupt cause */
+    mailbox_interrupt_to_pf(core);
+}
+
 static void igb_w1c(IGBCore *core, int index, uint32_t val)
 {
     core->mac[index] &= ~val;
@@ -2241,14 +2244,20 @@ igb_set_status(IGBCore *core, int index, uint32_t val)
 static void
 igb_set_ctrlext(IGBCore *core, int index, uint32_t val)
 {
-    trace_e1000e_link_set_ext_params(!!(val & E1000_CTRL_EXT_ASDCHK),
-                                     !!(val & E1000_CTRL_EXT_SPD_BYPS));
-
-    /* TODO: PFRSTD */
+    trace_igb_link_set_ext_params(!!(val & E1000_CTRL_EXT_ASDCHK),
+                                  !!(val & E1000_CTRL_EXT_SPD_BYPS),
+                                  !!(val & E1000_CTRL_EXT_PFRSTD));
 
     /* Zero self-clearing bits */
     val &= ~(E1000_CTRL_EXT_ASDCHK | E1000_CTRL_EXT_EE_RST);
     core->mac[CTRL_EXT] = val;
+
+    if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_PFRSTD) {
+        for (int vfn = 0; vfn < IGB_MAX_VF_FUNCTIONS; vfn++) {
+            core->mac[V2PMAILBOX0 + vfn] &= ~E1000_V2PMAILBOX_RSTI;
+            core->mac[V2PMAILBOX0 + vfn] |= E1000_V2PMAILBOX_RSTD;
+        }
+    }
 }
 
 static void
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 2f791b9b57..e94172e748 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -281,6 +281,8 @@ igb_core_mdic_read_unhandled(uint32_t addr) "MDIC READ: PHY[%u] UNHANDLED"
 igb_core_mdic_write(uint32_t addr, uint32_t data) "MDIC WRITE: PHY[%u] = 0x%x"
 igb_core_mdic_write_unhandled(uint32_t addr) "MDIC WRITE: PHY[%u] UNHANDLED"
 
+igb_link_set_ext_params(bool asd_check, bool speed_select_bypass, bool pfrstd) "Set extended link params: ASD check: %d, Speed select bypass: %d, PF reset done: %d"
+
 igb_rx_desc_buff_size(uint32_t b) "buffer size: %u"
 igb_rx_desc_buff_write(uint64_t addr, uint16_t offset, const void* source, uint32_t len) "addr: 0x%"PRIx64", offset: %u, from: %p, length: %u"
 
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/9] igb: implement VFRE and VFTE registers
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 1/9] MAINTAINERS: Add Sriram Yagnaraman as a igb reviewer Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 2/9] igb: handle PF/VF reset properly Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-29  9:15   ` Akihiko Odaki
  2023-01-28 13:46 ` [PATCH 4/9] igb: check oversized packets for VMDq Sriram Yagnaraman
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

Also add checks for RXDCTL/TXDCTL queue enable bits

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 hw/net/igb_core.c | 42 +++++++++++++++++++++++++++++++-----------
 hw/net/igb_regs.h |  3 ++-
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 9bd53cc25f..6bca5459b9 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -778,6 +778,19 @@ igb_txdesc_writeback(IGBCore *core, dma_addr_t base,
     return igb_tx_wb_eic(core, txi->idx);
 }
 
+static inline bool
+igb_tx_enabled(IGBCore *core, const E1000E_RingInfo *txi)
+{
+    bool vmdq = core->mac[MRQC] & 1;
+    uint16_t qn = txi->idx;
+    uint16_t vfn = (qn > IGB_MAX_VF_FUNCTIONS) ?
+                   (qn - IGB_MAX_VF_FUNCTIONS) : qn;
+
+    return (core->mac[TCTL] & E1000_TCTL_EN) &&
+        (vmdq ? (core->mac[VFTE] & BIT(vfn)) : true) &&
+        (core->mac[TXDCTL0 + (qn * 16)] & E1000_TXDCTL_QUEUE_ENABLE);
+}
+
 static void
 igb_start_xmit(IGBCore *core, const IGB_TxRing *txr)
 {
@@ -787,8 +800,7 @@ igb_start_xmit(IGBCore *core, const IGB_TxRing *txr)
     const E1000E_RingInfo *txi = txr->i;
     uint32_t eic = 0;
 
-    /* TODO: check if the queue itself is enabled too. */
-    if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
+    if (!igb_tx_enabled(core, txi)) {
         trace_e1000e_tx_disabled();
         return;
     }
@@ -1003,6 +1015,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
             queues = BIT(def_pl >> E1000_VT_CTL_DEFAULT_POOL_SHIFT);
         }
 
+        queues &= core->mac[VFRE];
         igb_rss_parse_packet(core, core->rx_pkt, external_tx != NULL, rss_info);
         if (rss_info->queue & 1) {
             queues <<= 8;
@@ -1486,7 +1499,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
     static const int maximum_ethernet_hdr_len = (ETH_HLEN + 4);
 
     uint16_t queues = 0;
-    uint32_t n;
+    uint32_t n = 0;
     uint8_t min_buf[ETH_ZLEN];
     struct iovec min_iov;
     struct eth_header *ehdr;
@@ -1566,26 +1579,22 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
         }
 
         igb_rx_ring_init(core, &rxr, i);
-
-        trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
-
         if (!igb_has_rxbufs(core, rxr.i, total_size)) {
             retval = 0;
         }
     }
 
     if (retval) {
-        n = E1000_ICR_RXT0;
-
         igb_rx_fix_l4_csum(core, core->rx_pkt);
 
         for (i = 0; i < IGB_NUM_QUEUES; i++) {
-            if (!(queues & BIT(i))) {
+            if (!(queues & BIT(i)) ||
+                !(core->mac[E1000_RXDCTL(i) >> 2] & E1000_RXDCTL_QUEUE_ENABLE)) {
                 continue;
             }
 
             igb_rx_ring_init(core, &rxr, i);
-
+            trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
             igb_write_packet_to_guest(core, core->rx_pkt, &rxr, &rss_info);
 
             /* Check if receive descriptor minimum threshold hit */
@@ -1594,6 +1603,9 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
             }
 
             core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
+
+            /* same as RXDW (rx descriptor written back)*/
+            n = E1000_ICR_RXT0;
         }
 
         trace_e1000e_rx_written_to_guest(n);
@@ -1981,9 +1993,16 @@ static void igb_set_vfmailbox(IGBCore *core, int index, uint32_t val)
 
 static void igb_vf_reset(IGBCore *core, uint16_t vfn)
 {
+    uint16_t qn0 = vfn;
+    uint16_t qn1 = vfn + IGB_MAX_VF_FUNCTIONS;
+
     /* disable Rx and Tx for the VF*/
-    core->mac[VFTE] &= ~BIT(vfn);
+    core->mac[RXDCTL0 + (qn0 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
+    core->mac[RXDCTL0 + (qn1 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
+    core->mac[TXDCTL0 + (qn0 * 16)] &= ~E1000_TXDCTL_QUEUE_ENABLE;
+    core->mac[TXDCTL0 + (qn1 * 16)] &= ~E1000_TXDCTL_QUEUE_ENABLE;
     core->mac[VFRE] &= ~BIT(vfn);
+    core->mac[VFTE] &= ~BIT(vfn);
     /* indicate VF reset to PF */
     core->mac[VFLRE] |= BIT(vfn);
     /* VFLRE and mailbox use the same interrupt cause */
@@ -3889,6 +3908,7 @@ igb_phy_reg_init[] = {
 static const uint32_t igb_mac_reg_init[] = {
     [LEDCTL]        = 2 | (3 << 8) | BIT(15) | (6 << 16) | (7 << 24),
     [EEMNGCTL]      = BIT(31),
+    [TXDCTL0]       = E1000_TXDCTL_QUEUE_ENABLE,
     [RXDCTL0]       = E1000_RXDCTL_QUEUE_ENABLE | (1 << 16),
     [RXDCTL1]       = 1 << 16,
     [RXDCTL2]       = 1 << 16,
diff --git a/hw/net/igb_regs.h b/hw/net/igb_regs.h
index ebf3e95023..084e751378 100644
--- a/hw/net/igb_regs.h
+++ b/hw/net/igb_regs.h
@@ -160,7 +160,8 @@ union e1000_adv_rx_desc {
 #define E1000_MRQC_RSS_FIELD_IPV6_UDP       0x00800000
 #define E1000_MRQC_RSS_FIELD_IPV6_UDP_EX    0x01000000
 
-/* Additional Receive Descriptor Control definitions */
+/* Additional RX/TX Descriptor Control definitions */
+#define E1000_TXDCTL_QUEUE_ENABLE  0x02000000 /* Enable specific Tx Queue */
 #define E1000_RXDCTL_QUEUE_ENABLE  0x02000000 /* Enable specific Rx Queue */
 
 /* Direct Cache Access (DCA) definitions */
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 4/9] igb: check oversized packets for VMDq
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
                   ` (2 preceding siblings ...)
  2023-01-28 13:46 ` [PATCH 3/9] igb: implement VFRE and VFTE registers Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-29  7:06   ` Akihiko Odaki
  2023-01-28 13:46 ` [PATCH 5/9] igb: respect E1000_VMOLR_RSSE Sriram Yagnaraman
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 hw/net/igb_core.c | 74 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 20 deletions(-)

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 6bca5459b9..1eb7ba168f 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -1476,6 +1476,30 @@ igb_write_packet_to_guest(IGBCore *core, struct NetRxPkt *pkt,
     igb_update_rx_stats(core, size, total_size);
 }
 
+static inline bool
+igb_is_oversized(IGBCore *core, const E1000E_RingInfo *rxi, size_t size)
+{
+    bool vmdq = core->mac[MRQC] & 1;
+    uint16_t qn = rxi->idx;
+    uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
+                   (qn - IGB_MAX_VF_FUNCTIONS) : qn;
+
+    bool lpe = (vmdq ? core->mac[VMOLR0 + pool] & E1000_VMOLR_LPE :
+                core->mac[RCTL] & E1000_RCTL_LPE);
+    bool sbp = core->mac[RCTL] & E1000_RCTL_SBP;
+    int maximum_ethernet_vlan_size = 1522;
+    int maximum_ethernet_lpe_size =
+        (vmdq ? core->mac[VMOLR0 + pool] & E1000_VMOLR_RLPML_MASK :
+         core->mac[RLPML] & E1000_VMOLR_RLPML_MASK);
+
+    if (size > maximum_ethernet_lpe_size ||
+        (size > maximum_ethernet_vlan_size && !lpe && !sbp)) {
+        return true;
+    }
+
+    return false;
+}
+
 static inline void
 igb_rx_fix_l4_csum(IGBCore *core, struct NetRxPkt *pkt)
 {
@@ -1499,7 +1523,8 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
     static const int maximum_ethernet_hdr_len = (ETH_HLEN + 4);
 
     uint16_t queues = 0;
-    uint32_t n = 0;
+    uint16_t oversized = 0;
+    uint32_t icr_bits = 0;
     uint8_t min_buf[ETH_ZLEN];
     struct iovec min_iov;
     struct eth_header *ehdr;
@@ -1509,7 +1534,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
     E1000E_RxRing rxr;
     E1000E_RSSInfo rss_info;
     size_t total_size;
-    ssize_t retval;
+    ssize_t retval = 0;
     int i;
 
     trace_e1000e_rx_receive_iov(iovcnt);
@@ -1550,11 +1575,6 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
         filter_buf = min_buf;
     }
 
-    /* Discard oversized packets if !LPE and !SBP. */
-    if (e1000x_is_oversized(core->mac, size)) {
-        return orig_size;
-    }
-
     ehdr = PKT_GET_ETH_HDR(filter_buf);
     net_rx_pkt_set_packet_type(core->rx_pkt, get_eth_packet_type(ehdr));
 
@@ -1571,8 +1591,6 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
     total_size = net_rx_pkt_get_total_len(core->rx_pkt) +
         e1000x_fcs_len(core->mac);
 
-    retval = orig_size;
-
     for (i = 0; i < IGB_NUM_QUEUES; i++) {
         if (!(queues & BIT(i))) {
             continue;
@@ -1580,42 +1598,58 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
 
         igb_rx_ring_init(core, &rxr, i);
         if (!igb_has_rxbufs(core, rxr.i, total_size)) {
-            retval = 0;
+            icr_bits |= E1000_ICS_RXO;
         }
     }
 
-    if (retval) {
+    if (!icr_bits) {
+        retval = orig_size;
         igb_rx_fix_l4_csum(core, core->rx_pkt);
 
         for (i = 0; i < IGB_NUM_QUEUES; i++) {
-            if (!(queues & BIT(i)) ||
-                !(core->mac[E1000_RXDCTL(i) >> 2] & E1000_RXDCTL_QUEUE_ENABLE)) {
+            if (!(queues & BIT(i))) {
                 continue;
             }
 
             igb_rx_ring_init(core, &rxr, i);
+            if (igb_is_oversized(core, rxr.i, size)) {
+                oversized |= BIT(i);
+                continue;
+            }
+
+            if (!(core->mac[RXDCTL0 + (i * 16)] & E1000_RXDCTL_QUEUE_ENABLE)) {
+                continue;
+            }
+
             trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
             igb_write_packet_to_guest(core, core->rx_pkt, &rxr, &rss_info);
 
             /* Check if receive descriptor minimum threshold hit */
             if (igb_rx_descr_threshold_hit(core, rxr.i)) {
-                n |= E1000_ICS_RXDMT0;
+                icr_bits |= E1000_ICS_RXDMT0;
             }
 
             core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
 
             /* same as RXDW (rx descriptor written back)*/
-            n = E1000_ICR_RXT0;
+            icr_bits |= E1000_ICR_RXT0;
         }
+    }
+
+    /* 8.19.37 increment ROC only if packet is oversized for all queues */
+    if (oversized == queues) {
+        trace_e1000x_rx_oversized(size);
+        e1000x_inc_reg_if_not_full(core->mac, ROC);
+    }
 
-        trace_e1000e_rx_written_to_guest(n);
+    if (icr_bits & E1000_ICR_RXT0) {
+        trace_e1000e_rx_written_to_guest(icr_bits);
     } else {
-        n = E1000_ICS_RXO;
-        trace_e1000e_rx_not_written_to_guest(n);
+        trace_e1000e_rx_not_written_to_guest(icr_bits);
     }
 
-    trace_e1000e_rx_interrupt_set(n);
-    igb_set_interrupt_cause(core, n);
+    trace_e1000e_rx_interrupt_set(icr_bits);
+    igb_set_interrupt_cause(core, icr_bits);
 
     return retval;
 }
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 5/9] igb: respect E1000_VMOLR_RSSE
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
                   ` (3 preceding siblings ...)
  2023-01-28 13:46 ` [PATCH 4/9] igb: check oversized packets for VMDq Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-29  7:24   ` Akihiko Odaki
  2023-01-28 13:46 ` [PATCH 6/9] igb: add ICR_RXDW Sriram Yagnaraman
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

RSS for VFs is only enabled if VMOLR[n].RSSE is set.

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 hw/net/igb_core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 1eb7ba168f..e4fd4a1a5f 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -69,7 +69,7 @@ typedef struct IGBTxPktVmdqCallbackContext {
 
 static ssize_t
 igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
-                     bool has_vnet, bool *assigned);
+                     bool has_vnet, bool *external_tx);
 
 static inline void
 igb_set_interrupt_cause(IGBCore *core, uint32_t val);
@@ -942,7 +942,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
 
     if (core->mac[MRQC] & 1) {
         if (is_broadcast_ether_addr(ehdr->h_dest)) {
-            for (i = 0; i < 8; i++) {
+            for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
                 if (core->mac[VMOLR0 + i] & E1000_VMOLR_BAM) {
                     queues |= BIT(i);
                 }
@@ -976,7 +976,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
                 f = ta_shift[(rctl >> E1000_RCTL_MO_SHIFT) & 3];
                 f = (((ehdr->h_dest[5] << 8) | ehdr->h_dest[4]) >> f) & 0xfff;
                 if (macp[f >> 5] & (1 << (f & 0x1f))) {
-                    for (i = 0; i < 8; i++) {
+                    for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
                         if (core->mac[VMOLR0 + i] & E1000_VMOLR_ROMPE) {
                             queues |= BIT(i);
                         }
@@ -999,7 +999,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
                     }
                 }
             } else {
-                for (i = 0; i < 8; i++) {
+                for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
                     if (core->mac[VMOLR0 + i] & E1000_VMOLR_AUPE) {
                         mask |= BIT(i);
                     }
@@ -1018,7 +1018,15 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
         queues &= core->mac[VFRE];
         igb_rss_parse_packet(core, core->rx_pkt, external_tx != NULL, rss_info);
         if (rss_info->queue & 1) {
-            queues <<= 8;
+            for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
+                if (!(queues & BIT(i))) {
+                    continue;
+                }
+                if (core->mac[VMOLR0 + i] & E1000_VMOLR_RSSE) {
+                    queues |= BIT(i + IGB_MAX_VF_FUNCTIONS);
+                    queues &= ~BIT(i);
+                }
+            }
         }
     } else {
         switch (net_rx_pkt_get_packet_type(core->rx_pkt)) {
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 6/9] igb: add ICR_RXDW
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
                   ` (4 preceding siblings ...)
  2023-01-28 13:46 ` [PATCH 5/9] igb: respect E1000_VMOLR_RSSE Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 7/9] igb: implement VF Tx and Rx stats Sriram Yagnaraman
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

IGB uses RXDW ICR bit to indicate that rx descriptor has been written
back. This is the same as RXT0 bit in older HW.

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 hw/net/e1000x_regs.h | 4 ++++
 hw/net/igb_core.c    | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000x_regs.h b/hw/net/e1000x_regs.h
index bb3fb36b8d..3a3431d878 100644
--- a/hw/net/e1000x_regs.h
+++ b/hw/net/e1000x_regs.h
@@ -335,6 +335,7 @@
 #define E1000_ICR_RXDMT0        0x00000010 /* rx desc min. threshold (0) */
 #define E1000_ICR_RXO           0x00000040 /* rx overrun */
 #define E1000_ICR_RXT0          0x00000080 /* rx timer intr (ring 0) */
+#define E1000_ICR_RXDW          0x00000080 /* rx desc written back */
 #define E1000_ICR_MDAC          0x00000200 /* MDIO access complete */
 #define E1000_ICR_RXCFG         0x00000400 /* RX /c/ ordered set */
 #define E1000_ICR_GPI_EN0       0x00000800 /* GP Int 0 */
@@ -378,6 +379,7 @@
 #define E1000_ICS_RXDMT0    E1000_ICR_RXDMT0    /* rx desc min. threshold */
 #define E1000_ICS_RXO       E1000_ICR_RXO       /* rx overrun */
 #define E1000_ICS_RXT0      E1000_ICR_RXT0      /* rx timer intr */
+#define E1000_ICS_RXDW      E1000_ICR_RXDW      /* rx desc written back */
 #define E1000_ICS_MDAC      E1000_ICR_MDAC      /* MDIO access complete */
 #define E1000_ICS_RXCFG     E1000_ICR_RXCFG     /* RX /c/ ordered set */
 #define E1000_ICS_GPI_EN0   E1000_ICR_GPI_EN0   /* GP Int 0 */
@@ -407,6 +409,7 @@
 #define E1000_IMS_RXDMT0    E1000_ICR_RXDMT0    /* rx desc min. threshold */
 #define E1000_IMS_RXO       E1000_ICR_RXO       /* rx overrun */
 #define E1000_IMS_RXT0      E1000_ICR_RXT0      /* rx timer intr */
+#define E1000_IMS_RXDW      E1000_ICR_RXDW      /* rx desc written back */
 #define E1000_IMS_MDAC      E1000_ICR_MDAC      /* MDIO access complete */
 #define E1000_IMS_RXCFG     E1000_ICR_RXCFG     /* RX /c/ ordered set */
 #define E1000_IMS_GPI_EN0   E1000_ICR_GPI_EN0   /* GP Int 0 */
@@ -441,6 +444,7 @@
 #define E1000_IMC_RXDMT0    E1000_ICR_RXDMT0    /* rx desc min. threshold */
 #define E1000_IMC_RXO       E1000_ICR_RXO       /* rx overrun */
 #define E1000_IMC_RXT0      E1000_ICR_RXT0      /* rx timer intr */
+#define E1000_IMC_RXDW      E1000_ICR_RXDW      /* rx desc written back */
 #define E1000_IMC_MDAC      E1000_ICR_MDAC      /* MDIO access complete */
 #define E1000_IMC_RXCFG     E1000_ICR_RXCFG     /* RX /c/ ordered set */
 #define E1000_IMC_GPI_EN0   E1000_ICR_GPI_EN0   /* GP Int 0 */
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index e4fd4a1a5f..43ff387b16 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -1640,7 +1640,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
             core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
 
             /* same as RXDW (rx descriptor written back)*/
-            icr_bits |= E1000_ICR_RXT0;
+            icr_bits |= E1000_ICR_RXDW;
         }
     }
 
@@ -1650,7 +1650,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
         e1000x_inc_reg_if_not_full(core->mac, ROC);
     }
 
-    if (icr_bits & E1000_ICR_RXT0) {
+    if (icr_bits & E1000_ICR_RXDW) {
         trace_e1000e_rx_written_to_guest(icr_bits);
     } else {
         trace_e1000e_rx_not_written_to_guest(icr_bits);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 7/9] igb: implement VF Tx and Rx stats
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
                   ` (5 preceding siblings ...)
  2023-01-28 13:46 ` [PATCH 6/9] igb: add ICR_RXDW Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 8/9] igb: respect VT_CTL ignore MAC field Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 9/9] igb: respect VMVIR and VMOLR for VLAN Sriram Yagnaraman
  8 siblings, 0 replies; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

Please note that loopback counters for VM to VM traffic is not
implemented yet: VFGOTLBC, VFGPTLBC, VFGORLBC and VFGPRLBC.

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 hw/net/igb_core.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 43ff387b16..375d9d5e34 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -488,7 +488,7 @@ igb_tx_pkt_send(IGBCore *core, struct igb_tx *tx, int queue_index)
 }
 
 static void
-igb_on_tx_done_update_stats(IGBCore *core, struct NetTxPkt *tx_pkt)
+igb_on_tx_done_update_stats(IGBCore *core, struct NetTxPkt *tx_pkt, int qn)
 {
     static const int PTCregs[6] = { PTC64, PTC127, PTC255, PTC511,
                                     PTC1023, PTC1522 };
@@ -515,6 +515,14 @@ igb_on_tx_done_update_stats(IGBCore *core, struct NetTxPkt *tx_pkt)
     core->mac[GPTC] = core->mac[TPT];
     core->mac[GOTCL] = core->mac[TOTL];
     core->mac[GOTCH] = core->mac[TOTH];
+
+    if (core->mac[MRQC] & 1) {
+        uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
+                        (qn - IGB_MAX_VF_FUNCTIONS) : qn;
+
+        core->mac[PVFGOTC0 + (pool * 64)] += tot_len;
+        core->mac[PVFGPTC0 + (pool * 64)]++;
+    }
 }
 
 static void
@@ -577,7 +585,7 @@ igb_process_tx_desc(IGBCore *core,
                     core->mac[VET] & 0xffff);
             }
             if (igb_tx_pkt_send(core, tx, queue_index)) {
-                igb_on_tx_done_update_stats(core, tx->tx_pkt);
+                igb_on_tx_done_update_stats(core, tx->tx_pkt, queue_index);
             }
         }
 
@@ -1364,7 +1372,8 @@ igb_write_to_rx_buffers(IGBCore *core,
 }
 
 static void
-igb_update_rx_stats(IGBCore *core, size_t data_size, size_t data_fcs_size)
+igb_update_rx_stats(IGBCore *core, const E1000E_RingInfo *rxi,
+                    size_t data_size, size_t data_fcs_size)
 {
     e1000x_update_rx_total_stats(core->mac, data_size, data_fcs_size);
 
@@ -1380,6 +1389,18 @@ igb_update_rx_stats(IGBCore *core, size_t data_size, size_t data_fcs_size)
     default:
         break;
     }
+
+    if (core->mac[MRQC] & 1) {
+        uint16_t qn = rxi->idx;
+        uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
+                        (qn - IGB_MAX_VF_FUNCTIONS) : qn;
+
+        core->mac[PVFGORC0 + (pool * 64)] += data_size + 4;
+        core->mac[PVFGPRC0 + (pool * 64)]++;
+        if (net_rx_pkt_get_packet_type(core->rx_pkt) == ETH_PKT_MCAST) {
+            core->mac[PVFMPRC0 + (pool * 64)]++;
+        }
+    }
 }
 
 static inline bool
@@ -1481,7 +1502,7 @@ igb_write_packet_to_guest(IGBCore *core, struct NetRxPkt *pkt,
 
     } while (desc_offset < total_size);
 
-    igb_update_rx_stats(core, size, total_size);
+    igb_update_rx_stats(core, rxi, size, total_size);
 }
 
 static inline bool
@@ -1490,7 +1511,7 @@ igb_is_oversized(IGBCore *core, const E1000E_RingInfo *rxi, size_t size)
     bool vmdq = core->mac[MRQC] & 1;
     uint16_t qn = rxi->idx;
     uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
-                   (qn - IGB_MAX_VF_FUNCTIONS) : qn;
+                    (qn - IGB_MAX_VF_FUNCTIONS) : qn;
 
     bool lpe = (vmdq ? core->mac[VMOLR0 + pool] & E1000_VMOLR_LPE :
                 core->mac[RCTL] & E1000_RCTL_LPE);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 8/9] igb: respect VT_CTL ignore MAC field
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
                   ` (6 preceding siblings ...)
  2023-01-28 13:46 ` [PATCH 7/9] igb: implement VF Tx and Rx stats Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-28 13:46 ` [PATCH 9/9] igb: respect VMVIR and VMOLR for VLAN Sriram Yagnaraman
  8 siblings, 0 replies; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

Also trace out a warning if replication mode is disabled, since we only
support replication mode enabled.

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 hw/net/igb_core.c   | 9 +++++++++
 hw/net/trace-events | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 375d9d5e34..8e33e15505 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -949,6 +949,10 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
     }
 
     if (core->mac[MRQC] & 1) {
+        if (!(core->mac[VT_CTL] & E1000_VT_CTL_VM_REPL_EN)) {
+            trace_igb_rx_vmdq_replication_mode_disabled();
+        }
+
         if (is_broadcast_ether_addr(ehdr->h_dest)) {
             for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
                 if (core->mac[VMOLR0 + i] & E1000_VMOLR_BAM) {
@@ -995,6 +999,11 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
             }
         }
 
+        /* assume a full pool list if IGMAC is set */
+        if (core->mac[VT_CTL] & E1000_VT_CTL_IGNORE_MAC) {
+            queues = BIT(IGB_MAX_VF_FUNCTIONS) - 1;
+        }
+
         if (e1000x_vlan_rx_filter_enabled(core->mac)) {
             uint16_t mask = 0;
 
diff --git a/hw/net/trace-events b/hw/net/trace-events
index e94172e748..9bc7658692 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -288,6 +288,8 @@ igb_rx_desc_buff_write(uint64_t addr, uint16_t offset, const void* source, uint3
 
 igb_rx_metadata_rss(uint32_t rss) "RSS data: 0x%X"
 
+igb_rx_vmdq_replication_mode_disabled(void) "WARN: Only replication mode enabled is supported"
+
 igb_irq_icr_clear_gpie_nsicr(void) "Clearing ICR on read due to GPIE.NSICR enabled"
 igb_irq_icr_write(uint32_t bits, uint32_t old_icr, uint32_t new_icr) "Clearing ICR bits 0x%x: 0x%x --> 0x%x"
 igb_irq_set_iam(uint32_t icr) "Update IAM: 0x%x"
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 9/9] igb: respect VMVIR and VMOLR for VLAN
  2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
                   ` (7 preceding siblings ...)
  2023-01-28 13:46 ` [PATCH 8/9] igb: respect VT_CTL ignore MAC field Sriram Yagnaraman
@ 2023-01-28 13:46 ` Sriram Yagnaraman
  2023-01-29  8:13   ` Akihiko Odaki
  8 siblings, 1 reply; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-28 13:46 UTC (permalink / raw)
  Cc: qemu-devel, Akihiko Odaki, Jason Wang, Dmitry Fleytman,
	Michael S . Tsirkin, Marcel Apfelbaum, Sriram Yagnaraman

Add support for stripping/inserting VLAN for VFs.

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 hw/net/igb_core.c | 100 ++++++++++++++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 35 deletions(-)

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 8e33e15505..96a5c5eca3 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -384,6 +384,26 @@ igb_rss_parse_packet(IGBCore *core, struct NetRxPkt *pkt, bool tx,
     info->queue = E1000_RSS_QUEUE(&core->mac[RETA], info->hash);
 }
 
+static inline bool
+igb_tx_insert_vlan(IGBCore *core, uint16_t qn,
+                   struct igb_tx *tx, bool desc_vle)
+{
+    if (core->mac[MRQC] & 1) {
+        uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
+                        (qn - IGB_MAX_VF_FUNCTIONS) : qn;
+
+        if (core->mac[VMVIR0 + pool] & E1000_VMVIR_VLANA_DEFAULT) {
+            /* always insert default VLAN */
+            desc_vle = true;
+            tx->vlan = core->mac[VMVIR0 + pool] & 0xfff;
+        } else if (core->mac[VMVIR0 + pool] & E1000_VMVIR_VLANA_NEVER) {
+            return false;
+        }
+    }
+
+    return desc_vle && e1000x_vlan_enabled(core->mac);
+}
+
 static bool
 igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)
 {
@@ -580,7 +600,8 @@ igb_process_tx_desc(IGBCore *core,
 
     if (cmd_type_len & E1000_TXD_CMD_EOP) {
         if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
-            if (cmd_type_len & E1000_TXD_CMD_VLE) {
+            if (igb_tx_insert_vlan(core, queue_index, tx,
+                (cmd_type_len & E1000_TXD_CMD_VLE))) {
                 net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
                     core->mac[VET] & 0xffff);
             }
@@ -1514,6 +1535,22 @@ igb_write_packet_to_guest(IGBCore *core, struct NetRxPkt *pkt,
     igb_update_rx_stats(core, rxi, size, total_size);
 }
 
+static inline bool
+igb_rx_strip_vlan(IGBCore *core, const E1000E_RingInfo *rxi,
+    eth_pkt_types_e pkt_type)
+{
+    if (core->mac[MRQC] & 1) {
+        uint16_t qn = rxi->idx;
+        uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
+                        (qn - IGB_MAX_VF_FUNCTIONS) : qn;
+        return (pkt_type == ETH_PKT_MCAST) ?
+                core->mac[RPLOLR] & E1000_RPLOLR_STRVLAN :
+                core->mac[VMOLR0 + pool] & E1000_VMOLR_STRVLAN;
+    }
+
+    return e1000x_vlan_enabled(core->mac);
+}
+
 static inline bool
 igb_is_oversized(IGBCore *core, const E1000E_RingInfo *rxi, size_t size)
 {
@@ -1574,6 +1611,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
     size_t total_size;
     ssize_t retval = 0;
     int i;
+    bool strip_vlan = false;
 
     trace_e1000e_rx_receive_iov(iovcnt);
 
@@ -1615,10 +1653,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
 
     ehdr = PKT_GET_ETH_HDR(filter_buf);
     net_rx_pkt_set_packet_type(core->rx_pkt, get_eth_packet_type(ehdr));
-
-    net_rx_pkt_attach_iovec_ex(core->rx_pkt, iov, iovcnt, iov_ofs,
-                               e1000x_vlan_enabled(core->mac),
-                               core->mac[VET] & 0xffff);
+    net_rx_pkt_set_protocols(core->rx_pkt, filter_buf, size);
 
     queues = igb_receive_assign(core, ehdr, &rss_info, external_tx);
     if (!queues) {
@@ -1626,8 +1661,8 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
         return orig_size;
     }
 
-    total_size = net_rx_pkt_get_total_len(core->rx_pkt) +
-        e1000x_fcs_len(core->mac);
+    retval = orig_size;
+    total_size = size + e1000x_fcs_len(core->mac);
 
     for (i = 0; i < IGB_NUM_QUEUES; i++) {
         if (!(queues & BIT(i))) {
@@ -1635,43 +1670,38 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
         }
 
         igb_rx_ring_init(core, &rxr, i);
+        strip_vlan = igb_rx_strip_vlan(core, rxr.i,
+            get_eth_packet_type(ehdr));
+        net_rx_pkt_attach_iovec_ex(core->rx_pkt, iov, iovcnt, iov_ofs,
+            strip_vlan, core->mac[VET] & 0xffff);
+        igb_rx_fix_l4_csum(core, core->rx_pkt);
+
         if (!igb_has_rxbufs(core, rxr.i, total_size)) {
             icr_bits |= E1000_ICS_RXO;
+            continue;
         }
-    }
-
-    if (!icr_bits) {
-        retval = orig_size;
-        igb_rx_fix_l4_csum(core, core->rx_pkt);
-
-        for (i = 0; i < IGB_NUM_QUEUES; i++) {
-            if (!(queues & BIT(i))) {
-                continue;
-            }
 
-            igb_rx_ring_init(core, &rxr, i);
-            if (igb_is_oversized(core, rxr.i, size)) {
-                oversized |= BIT(i);
-                continue;
-            }
+        if (igb_is_oversized(core, rxr.i, total_size)) {
+            oversized |= BIT(i);
+            continue;
+        }
 
-            if (!(core->mac[RXDCTL0 + (i * 16)] & E1000_RXDCTL_QUEUE_ENABLE)) {
-                continue;
-            }
+        if (!(core->mac[RXDCTL0 + (i * 16)] & E1000_RXDCTL_QUEUE_ENABLE)) {
+            continue;
+        }
 
-            trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
-            igb_write_packet_to_guest(core, core->rx_pkt, &rxr, &rss_info);
+        trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
+        igb_write_packet_to_guest(core, core->rx_pkt, &rxr, &rss_info);
 
-            /* Check if receive descriptor minimum threshold hit */
-            if (igb_rx_descr_threshold_hit(core, rxr.i)) {
-                icr_bits |= E1000_ICS_RXDMT0;
-            }
+        /* Check if receive descriptor minimum threshold hit */
+        if (igb_rx_descr_threshold_hit(core, rxr.i)) {
+            icr_bits |= E1000_ICS_RXDMT0;
+        }
 
-            core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
+        core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
 
-            /* same as RXDW (rx descriptor written back)*/
-            icr_bits |= E1000_ICR_RXDW;
-        }
+        /* same as RXDW (rx descriptor written back)*/
+        icr_bits |= E1000_ICR_RXDW;
     }
 
     /* 8.19.37 increment ROC only if packet is oversized for all queues */
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/9] igb: handle PF/VF reset properly
  2023-01-28 13:46 ` [PATCH 2/9] igb: handle PF/VF reset properly Sriram Yagnaraman
@ 2023-01-29  5:58   ` Akihiko Odaki
  2023-01-29  6:15     ` Akihiko Odaki
  0 siblings, 1 reply; 20+ messages in thread
From: Akihiko Odaki @ 2023-01-29  5:58 UTC (permalink / raw)
  To: Sriram Yagnaraman
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

On 2023/01/28 22:46, Sriram Yagnaraman wrote:
> Use PFRSTD to reset RSTI bit for VFs, and raise VFLRE interrupt when VF
> is reset.
> 
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> ---
>   hw/net/e1000x_regs.h |  1 +
>   hw/net/igb_core.c    | 33 +++++++++++++++++++++------------
>   hw/net/trace-events  |  2 ++
>   3 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/net/e1000x_regs.h b/hw/net/e1000x_regs.h
> index fb5b861135..bb3fb36b8d 100644
> --- a/hw/net/e1000x_regs.h
> +++ b/hw/net/e1000x_regs.h
> @@ -548,6 +548,7 @@
>   
>   #define E1000_CTRL_EXT_ASDCHK  0x00001000 /* auto speed detection check */
>   #define E1000_CTRL_EXT_EE_RST  0x00002000 /* EEPROM reset */
> +#define E1000_CTRL_EXT_PFRSTD  0x00004000 /* PF reset done indication */
>   #define E1000_CTRL_EXT_LINK_EN 0x00010000 /* enable link status from external LINK_0 and LINK_1 pins */
>   #define E1000_CTRL_EXT_DRV_LOAD 0x10000000 /* Driver loaded bit for FW */
>   #define E1000_CTRL_EXT_EIAME   0x01000000
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index abeb9c7889..9bd53cc25f 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -1902,14 +1902,6 @@ static void igb_set_eims(IGBCore *core, int index, uint32_t val)
>       igb_update_interrupt_state(core);
>   }
>   
> -static void igb_vf_reset(IGBCore *core, uint16_t vfn)
> -{
> -    /* TODO: Reset of the queue enable and the interrupt registers of the VF. */
> -
> -    core->mac[V2PMAILBOX0 + vfn] &= ~E1000_V2PMAILBOX_RSTI;
> -    core->mac[V2PMAILBOX0 + vfn] = E1000_V2PMAILBOX_RSTD;
> -}
> -
>   static void mailbox_interrupt_to_vf(IGBCore *core, uint16_t vfn)
>   {
>       uint32_t ent = core->mac[VTIVAR_MISC + vfn];
> @@ -1987,6 +1979,17 @@ static void igb_set_vfmailbox(IGBCore *core, int index, uint32_t val)
>       }
>   }
>   
> +static void igb_vf_reset(IGBCore *core, uint16_t vfn)
> +{
> +    /* disable Rx and Tx for the VF*/
> +    core->mac[VFTE] &= ~BIT(vfn);
> +    core->mac[VFRE] &= ~BIT(vfn);
> +    /* indicate VF reset to PF */
> +    core->mac[VFLRE] |= BIT(vfn);
> +    /* VFLRE and mailbox use the same interrupt cause */
> +    mailbox_interrupt_to_pf(core);
> +}
> +

Please do not move the function unless you have a legitimate reason for 
that.

>   static void igb_w1c(IGBCore *core, int index, uint32_t val)
>   {
>       core->mac[index] &= ~val;
> @@ -2241,14 +2244,20 @@ igb_set_status(IGBCore *core, int index, uint32_t val)
>   static void
>   igb_set_ctrlext(IGBCore *core, int index, uint32_t val)
>   {
> -    trace_e1000e_link_set_ext_params(!!(val & E1000_CTRL_EXT_ASDCHK),
> -                                     !!(val & E1000_CTRL_EXT_SPD_BYPS));
> -
> -    /* TODO: PFRSTD */
> +    trace_igb_link_set_ext_params(!!(val & E1000_CTRL_EXT_ASDCHK),
> +                                  !!(val & E1000_CTRL_EXT_SPD_BYPS),
> +                                  !!(val & E1000_CTRL_EXT_PFRSTD));
>   
>       /* Zero self-clearing bits */
>       val &= ~(E1000_CTRL_EXT_ASDCHK | E1000_CTRL_EXT_EE_RST);
>       core->mac[CTRL_EXT] = val;
> +
> +    if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_PFRSTD) {
> +        for (int vfn = 0; vfn < IGB_MAX_VF_FUNCTIONS; vfn++) {
> +            core->mac[V2PMAILBOX0 + vfn] &= ~E1000_V2PMAILBOX_RSTI;
> +            core->mac[V2PMAILBOX0 + vfn] |= E1000_V2PMAILBOX_RSTD;
> +        }
> +    }
>   }
>   
>   static void
> diff --git a/hw/net/trace-events b/hw/net/trace-events
> index 2f791b9b57..e94172e748 100644
> --- a/hw/net/trace-events
> +++ b/hw/net/trace-events
> @@ -281,6 +281,8 @@ igb_core_mdic_read_unhandled(uint32_t addr) "MDIC READ: PHY[%u] UNHANDLED"
>   igb_core_mdic_write(uint32_t addr, uint32_t data) "MDIC WRITE: PHY[%u] = 0x%x"
>   igb_core_mdic_write_unhandled(uint32_t addr) "MDIC WRITE: PHY[%u] UNHANDLED"
>   
> +igb_link_set_ext_params(bool asd_check, bool speed_select_bypass, bool pfrstd) "Set extended link params: ASD check: %d, Speed select bypass: %d, PF reset done: %d"
> +
>   igb_rx_desc_buff_size(uint32_t b) "buffer size: %u"
>   igb_rx_desc_buff_write(uint64_t addr, uint16_t offset, const void* source, uint32_t len) "addr: 0x%"PRIx64", offset: %u, from: %p, length: %u"
>   


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/9] igb: handle PF/VF reset properly
  2023-01-29  5:58   ` Akihiko Odaki
@ 2023-01-29  6:15     ` Akihiko Odaki
  0 siblings, 0 replies; 20+ messages in thread
From: Akihiko Odaki @ 2023-01-29  6:15 UTC (permalink / raw)
  To: Sriram Yagnaraman
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

On 2023/01/29 14:58, Akihiko Odaki wrote:
> On 2023/01/28 22:46, Sriram Yagnaraman wrote:
>> Use PFRSTD to reset RSTI bit for VFs, and raise VFLRE interrupt when VF
>> is reset.
>>
>> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>> ---
>>   hw/net/e1000x_regs.h |  1 +
>>   hw/net/igb_core.c    | 33 +++++++++++++++++++++------------
>>   hw/net/trace-events  |  2 ++
>>   3 files changed, 24 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/net/e1000x_regs.h b/hw/net/e1000x_regs.h
>> index fb5b861135..bb3fb36b8d 100644
>> --- a/hw/net/e1000x_regs.h
>> +++ b/hw/net/e1000x_regs.h
>> @@ -548,6 +548,7 @@
>>   #define E1000_CTRL_EXT_ASDCHK  0x00001000 /* auto speed detection 
>> check */
>>   #define E1000_CTRL_EXT_EE_RST  0x00002000 /* EEPROM reset */
>> +#define E1000_CTRL_EXT_PFRSTD  0x00004000 /* PF reset done indication */
>>   #define E1000_CTRL_EXT_LINK_EN 0x00010000 /* enable link status from 
>> external LINK_0 and LINK_1 pins */
>>   #define E1000_CTRL_EXT_DRV_LOAD 0x10000000 /* Driver loaded bit for 
>> FW */
>>   #define E1000_CTRL_EXT_EIAME   0x01000000
>> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
>> index abeb9c7889..9bd53cc25f 100644
>> --- a/hw/net/igb_core.c
>> +++ b/hw/net/igb_core.c
>> @@ -1902,14 +1902,6 @@ static void igb_set_eims(IGBCore *core, int 
>> index, uint32_t val)
>>       igb_update_interrupt_state(core);
>>   }
>> -static void igb_vf_reset(IGBCore *core, uint16_t vfn)
>> -{
>> -    /* TODO: Reset of the queue enable and the interrupt registers of 
>> the VF. */
>> -
>> -    core->mac[V2PMAILBOX0 + vfn] &= ~E1000_V2PMAILBOX_RSTI;
>> -    core->mac[V2PMAILBOX0 + vfn] = E1000_V2PMAILBOX_RSTD;
>> -}
>> -
>>   static void mailbox_interrupt_to_vf(IGBCore *core, uint16_t vfn)
>>   {
>>       uint32_t ent = core->mac[VTIVAR_MISC + vfn];
>> @@ -1987,6 +1979,17 @@ static void igb_set_vfmailbox(IGBCore *core, 
>> int index, uint32_t val)
>>       }
>>   }
>> +static void igb_vf_reset(IGBCore *core, uint16_t vfn)
>> +{
>> +    /* disable Rx and Tx for the VF*/
>> +    core->mac[VFTE] &= ~BIT(vfn);
>> +    core->mac[VFRE] &= ~BIT(vfn);
>> +    /* indicate VF reset to PF */
>> +    core->mac[VFLRE] |= BIT(vfn);
>> +    /* VFLRE and mailbox use the same interrupt cause */
>> +    mailbox_interrupt_to_pf(core);
>> +}
>> +
> 
> Please do not move the function unless you have a legitimate reason for 
> that.

I got it. It is necessary to refer mailbox_interrupt_to_pf().

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

> 
>>   static void igb_w1c(IGBCore *core, int index, uint32_t val)
>>   {
>>       core->mac[index] &= ~val;
>> @@ -2241,14 +2244,20 @@ igb_set_status(IGBCore *core, int index, 
>> uint32_t val)
>>   static void
>>   igb_set_ctrlext(IGBCore *core, int index, uint32_t val)
>>   {
>> -    trace_e1000e_link_set_ext_params(!!(val & E1000_CTRL_EXT_ASDCHK),
>> -                                     !!(val & E1000_CTRL_EXT_SPD_BYPS));
>> -
>> -    /* TODO: PFRSTD */
>> +    trace_igb_link_set_ext_params(!!(val & E1000_CTRL_EXT_ASDCHK),
>> +                                  !!(val & E1000_CTRL_EXT_SPD_BYPS),
>> +                                  !!(val & E1000_CTRL_EXT_PFRSTD));
>>       /* Zero self-clearing bits */
>>       val &= ~(E1000_CTRL_EXT_ASDCHK | E1000_CTRL_EXT_EE_RST);
>>       core->mac[CTRL_EXT] = val;
>> +
>> +    if (core->mac[CTRL_EXT] & E1000_CTRL_EXT_PFRSTD) {
>> +        for (int vfn = 0; vfn < IGB_MAX_VF_FUNCTIONS; vfn++) {
>> +            core->mac[V2PMAILBOX0 + vfn] &= ~E1000_V2PMAILBOX_RSTI;
>> +            core->mac[V2PMAILBOX0 + vfn] |= E1000_V2PMAILBOX_RSTD;
>> +        }
>> +    }
>>   }
>>   static void
>> diff --git a/hw/net/trace-events b/hw/net/trace-events
>> index 2f791b9b57..e94172e748 100644
>> --- a/hw/net/trace-events
>> +++ b/hw/net/trace-events
>> @@ -281,6 +281,8 @@ igb_core_mdic_read_unhandled(uint32_t addr) "MDIC 
>> READ: PHY[%u] UNHANDLED"
>>   igb_core_mdic_write(uint32_t addr, uint32_t data) "MDIC WRITE: 
>> PHY[%u] = 0x%x"
>>   igb_core_mdic_write_unhandled(uint32_t addr) "MDIC WRITE: PHY[%u] 
>> UNHANDLED"
>> +igb_link_set_ext_params(bool asd_check, bool speed_select_bypass, 
>> bool pfrstd) "Set extended link params: ASD check: %d, Speed select 
>> bypass: %d, PF reset done: %d"
>> +
>>   igb_rx_desc_buff_size(uint32_t b) "buffer size: %u"
>>   igb_rx_desc_buff_write(uint64_t addr, uint16_t offset, const void* 
>> source, uint32_t len) "addr: 0x%"PRIx64", offset: %u, from: %p, 
>> length: %u"


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/9] igb: check oversized packets for VMDq
  2023-01-28 13:46 ` [PATCH 4/9] igb: check oversized packets for VMDq Sriram Yagnaraman
@ 2023-01-29  7:06   ` Akihiko Odaki
  0 siblings, 0 replies; 20+ messages in thread
From: Akihiko Odaki @ 2023-01-29  7:06 UTC (permalink / raw)
  To: Sriram Yagnaraman
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

On 2023/01/28 22:46, Sriram Yagnaraman wrote:
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> ---
>   hw/net/igb_core.c | 74 ++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index 6bca5459b9..1eb7ba168f 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -1476,6 +1476,30 @@ igb_write_packet_to_guest(IGBCore *core, struct NetRxPkt *pkt,
>       igb_update_rx_stats(core, size, total_size);
>   }
>   
> +static inline bool

Please remove inline qualifier. inline qualifier has some adverse effects:
- It suppresses GCC warnings for unused functions. This behavior is 
useful when you write a function in a header file, but it is not in this 
case.
- It confuses the compiler if the function later grows and becomes 
unsuitable for inlining.
- It is noise in source code.

In this case, the compiler should be able to decide to inline the 
function or not by its own.

> +igb_is_oversized(IGBCore *core, const E1000E_RingInfo *rxi, size_t size)
> +{
> +    bool vmdq = core->mac[MRQC] & 1;
> +    uint16_t qn = rxi->idx;
> +    uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
> +                   (qn - IGB_MAX_VF_FUNCTIONS) : qn;

Write as qn % 8; this pattern is already prevalent.

> +
> +    bool lpe = (vmdq ? core->mac[VMOLR0 + pool] & E1000_VMOLR_LPE :
> +                core->mac[RCTL] & E1000_RCTL_LPE);

RCTL.LPE should be checked even if VMDq is enabled; In section 7.10.3.4, 
Size Filtering is defined to check RCTL.LPE, and it is part of packet 
switching procedure for virtualized environment. Linux also ensures it 
sets the maximum value to RLPML.RLPML if VMDq is enabled:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cfbc871c2174f352542053d25659920d6841ed41

> +    bool sbp = core->mac[RCTL] & E1000_RCTL_SBP;
> +    int maximum_ethernet_vlan_size = 1522;
> +    int maximum_ethernet_lpe_size =
> +        (vmdq ? core->mac[VMOLR0 + pool] & E1000_VMOLR_RLPML_MASK :
> +         core->mac[RLPML] & E1000_VMOLR_RLPML_MASK);
> +
> +    if (size > maximum_ethernet_lpe_size ||
> +        (size > maximum_ethernet_vlan_size && !lpe && !sbp)) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>   static inline void
>   igb_rx_fix_l4_csum(IGBCore *core, struct NetRxPkt *pkt)
>   {
> @@ -1499,7 +1523,8 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>       static const int maximum_ethernet_hdr_len = (ETH_HLEN + 4);
>   
>       uint16_t queues = 0;
> -    uint32_t n = 0;
> +    uint16_t oversized = 0;
> +    uint32_t icr_bits = 0;
>       uint8_t min_buf[ETH_ZLEN];
>       struct iovec min_iov;
>       struct eth_header *ehdr;
> @@ -1509,7 +1534,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>       E1000E_RxRing rxr;
>       E1000E_RSSInfo rss_info;
>       size_t total_size;
> -    ssize_t retval;
> +    ssize_t retval = 0;
>       int i;
>   
>       trace_e1000e_rx_receive_iov(iovcnt);
> @@ -1550,11 +1575,6 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>           filter_buf = min_buf;
>       }
>   
> -    /* Discard oversized packets if !LPE and !SBP. */
> -    if (e1000x_is_oversized(core->mac, size)) {
> -        return orig_size;
> -    }
> -
>       ehdr = PKT_GET_ETH_HDR(filter_buf);
>       net_rx_pkt_set_packet_type(core->rx_pkt, get_eth_packet_type(ehdr));
>   
> @@ -1571,8 +1591,6 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>       total_size = net_rx_pkt_get_total_len(core->rx_pkt) +
>           e1000x_fcs_len(core->mac);
>   
> -    retval = orig_size;
> -
>       for (i = 0; i < IGB_NUM_QUEUES; i++) {
>           if (!(queues & BIT(i))) {
>               continue;
> @@ -1580,42 +1598,58 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>   
>           igb_rx_ring_init(core, &rxr, i);
>           if (!igb_has_rxbufs(core, rxr.i, total_size)) {
> -            retval = 0;
> +            icr_bits |= E1000_ICS_RXO;
>           }
>       }
>   
> -    if (retval) {
> +    if (!icr_bits) {
> +        retval = orig_size;
>           igb_rx_fix_l4_csum(core, core->rx_pkt);
>   
>           for (i = 0; i < IGB_NUM_QUEUES; i++) {
> -            if (!(queues & BIT(i)) ||
> -                !(core->mac[E1000_RXDCTL(i) >> 2] & E1000_RXDCTL_QUEUE_ENABLE)) {
> +            if (!(queues & BIT(i))) {
>                   continue;
>               }
>   
>               igb_rx_ring_init(core, &rxr, i);
> +            if (igb_is_oversized(core, rxr.i, size)) {
> +                oversized |= BIT(i);
> +                continue;
> +            }

VMOLR.RLPML is checked during Rx queue assignment, which is implemented 
in igb_receive_assign(). The oversize check should be moved to the function.

> +
> +            if (!(core->mac[RXDCTL0 + (i * 16)] & E1000_RXDCTL_QUEUE_ENABLE)) {
> +                continue;
> +            }
> +
>               trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
>               igb_write_packet_to_guest(core, core->rx_pkt, &rxr, &rss_info);
>   
>               /* Check if receive descriptor minimum threshold hit */
>               if (igb_rx_descr_threshold_hit(core, rxr.i)) {
> -                n |= E1000_ICS_RXDMT0;
> +                icr_bits |= E1000_ICS_RXDMT0;
>               }
>   
>               core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
>   
>               /* same as RXDW (rx descriptor written back)*/
> -            n = E1000_ICR_RXT0;
> +            icr_bits |= E1000_ICR_RXT0;
>           }
> +    }
> +
> +    /* 8.19.37 increment ROC only if packet is oversized for all queues */
> +    if (oversized == queues) {
> +        trace_e1000x_rx_oversized(size);
> +        e1000x_inc_reg_if_not_full(core->mac, ROC);
> +    }
>   
> -        trace_e1000e_rx_written_to_guest(n);
> +    if (icr_bits & E1000_ICR_RXT0) {
> +        trace_e1000e_rx_written_to_guest(icr_bits);
>       } else {
> -        n = E1000_ICS_RXO;
> -        trace_e1000e_rx_not_written_to_guest(n);
> +        trace_e1000e_rx_not_written_to_guest(icr_bits);
>       }
>   
> -    trace_e1000e_rx_interrupt_set(n);
> -    igb_set_interrupt_cause(core, n);
> +    trace_e1000e_rx_interrupt_set(icr_bits);
> +    igb_set_interrupt_cause(core, icr_bits);
>   
>       return retval;
>   }


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/9] igb: respect E1000_VMOLR_RSSE
  2023-01-28 13:46 ` [PATCH 5/9] igb: respect E1000_VMOLR_RSSE Sriram Yagnaraman
@ 2023-01-29  7:24   ` Akihiko Odaki
  2023-01-30 12:07     ` Sriram Yagnaraman
  0 siblings, 1 reply; 20+ messages in thread
From: Akihiko Odaki @ 2023-01-29  7:24 UTC (permalink / raw)
  To: Sriram Yagnaraman
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

On 2023/01/28 22:46, Sriram Yagnaraman wrote:
> RSS for VFs is only enabled if VMOLR[n].RSSE is set.
> 
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> ---
>   hw/net/igb_core.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index 1eb7ba168f..e4fd4a1a5f 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -69,7 +69,7 @@ typedef struct IGBTxPktVmdqCallbackContext {
>   
>   static ssize_t
>   igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
> -                     bool has_vnet, bool *assigned);
> +                     bool has_vnet, bool *external_tx);

I admit external_tx is somewhat confusing, but it is more than just 
telling if it is assigned to Rx queue or not. If external_tx is not 
NULL, it indicates it is part of Tx packet switching. In that case, a 
bool value which describes whether the packet should be routed to 
external LAN must be assigned. The value can be false even if the packet 
is assigned to Rx queues; it will be always false if it is multicast, 
for example.

>   
>   static inline void
>   igb_set_interrupt_cause(IGBCore *core, uint32_t val);
> @@ -942,7 +942,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
>   
>       if (core->mac[MRQC] & 1) {
>           if (is_broadcast_ether_addr(ehdr->h_dest)) {
> -            for (i = 0; i < 8; i++) {
> +            for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {

I just left it as 8 because VMDq is not specific to VF. Perhaps it is 
better to have another macro to denote the number of VMDq pools, but it 
is not done yet.

>                   if (core->mac[VMOLR0 + i] & E1000_VMOLR_BAM) {
>                       queues |= BIT(i);
>                   }
> @@ -976,7 +976,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
>                   f = ta_shift[(rctl >> E1000_RCTL_MO_SHIFT) & 3];
>                   f = (((ehdr->h_dest[5] << 8) | ehdr->h_dest[4]) >> f) & 0xfff;
>                   if (macp[f >> 5] & (1 << (f & 0x1f))) {
> -                    for (i = 0; i < 8; i++) {
> +                    for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
>                           if (core->mac[VMOLR0 + i] & E1000_VMOLR_ROMPE) {
>                               queues |= BIT(i);
>                           }
> @@ -999,7 +999,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
>                       }
>                   }
>               } else {
> -                for (i = 0; i < 8; i++) {
> +                for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
>                       if (core->mac[VMOLR0 + i] & E1000_VMOLR_AUPE) {
>                           mask |= BIT(i);
>                       }
> @@ -1018,7 +1018,15 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
>           queues &= core->mac[VFRE];
>           igb_rss_parse_packet(core, core->rx_pkt, external_tx != NULL, rss_info);
>           if (rss_info->queue & 1) {
> -            queues <<= 8;
> +            for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
> +                if (!(queues & BIT(i))) {
> +                    continue;
> +                }
> +                if (core->mac[VMOLR0 + i] & E1000_VMOLR_RSSE) {
> +                    queues |= BIT(i + IGB_MAX_VF_FUNCTIONS);
> +                    queues &= ~BIT(i);
> +                }
> +            }
>           }
>       } else {
>           switch (net_rx_pkt_get_packet_type(core->rx_pkt)) {


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 9/9] igb: respect VMVIR and VMOLR for VLAN
  2023-01-28 13:46 ` [PATCH 9/9] igb: respect VMVIR and VMOLR for VLAN Sriram Yagnaraman
@ 2023-01-29  8:13   ` Akihiko Odaki
  0 siblings, 0 replies; 20+ messages in thread
From: Akihiko Odaki @ 2023-01-29  8:13 UTC (permalink / raw)
  To: Sriram Yagnaraman
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

On 2023/01/28 22:46, Sriram Yagnaraman wrote:
> Add support for stripping/inserting VLAN for VFs.
> 
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> ---
>   hw/net/igb_core.c | 100 ++++++++++++++++++++++++++++++----------------
>   1 file changed, 65 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index 8e33e15505..96a5c5eca3 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -384,6 +384,26 @@ igb_rss_parse_packet(IGBCore *core, struct NetRxPkt *pkt, bool tx,
>       info->queue = E1000_RSS_QUEUE(&core->mac[RETA], info->hash);
>   }
>   
> +static inline bool
> +igb_tx_insert_vlan(IGBCore *core, uint16_t qn,
> +                   struct igb_tx *tx, bool desc_vle)
> +{
> +    if (core->mac[MRQC] & 1) {
> +        uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
> +                        (qn - IGB_MAX_VF_FUNCTIONS) : qn;
> +
> +        if (core->mac[VMVIR0 + pool] & E1000_VMVIR_VLANA_DEFAULT) {
> +            /* always insert default VLAN */
> +            desc_vle = true;
> +            tx->vlan = core->mac[VMVIR0 + pool] & 0xfff;

This should be masked with 0xffff; "Port VLAN ID" field is defined as 
16-bit.

> +        } else if (core->mac[VMVIR0 + pool] & E1000_VMVIR_VLANA_NEVER) {
> +            return false;
> +        }
> +    }
> +
> +    return desc_vle && e1000x_vlan_enabled(core->mac);
> +}
> +
>   static bool
>   igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)
>   {
> @@ -580,7 +600,8 @@ igb_process_tx_desc(IGBCore *core,
>   
>       if (cmd_type_len & E1000_TXD_CMD_EOP) {
>           if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
> -            if (cmd_type_len & E1000_TXD_CMD_VLE) {
> +            if (igb_tx_insert_vlan(core, queue_index, tx,
> +                (cmd_type_len & E1000_TXD_CMD_VLE))) {

The fourth parameter of igb_tx_insert_vlan() is bool, which is defined 
as it only ensures it has storage of 1-bit, but this passes a value 
greater than that.

>                   net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
>                       core->mac[VET] & 0xffff);
>               }
> @@ -1514,6 +1535,22 @@ igb_write_packet_to_guest(IGBCore *core, struct NetRxPkt *pkt,
>       igb_update_rx_stats(core, rxi, size, total_size);
>   }
>   
> +static inline bool
> +igb_rx_strip_vlan(IGBCore *core, const E1000E_RingInfo *rxi,
> +    eth_pkt_types_e pkt_type)
> +{
> +    if (core->mac[MRQC] & 1) {
> +        uint16_t qn = rxi->idx;
> +        uint16_t pool = (qn > IGB_MAX_VF_FUNCTIONS) ?
> +                        (qn - IGB_MAX_VF_FUNCTIONS) : qn;
> +        return (pkt_type == ETH_PKT_MCAST) ?
> +                core->mac[RPLOLR] & E1000_RPLOLR_STRVLAN :
> +                core->mac[VMOLR0 + pool] & E1000_VMOLR_STRVLAN;
> +    }
> +
> +    return e1000x_vlan_enabled(core->mac);
> +}
> +
>   static inline bool
>   igb_is_oversized(IGBCore *core, const E1000E_RingInfo *rxi, size_t size)
>   {
> @@ -1574,6 +1611,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>       size_t total_size;
>       ssize_t retval = 0;
>       int i;
> +    bool strip_vlan = false;

strip_vlan does not need a default value. Having a default value will 
suppresss compiler warnings when you actually need to compute a valid value.

>   
>       trace_e1000e_rx_receive_iov(iovcnt);
>   
> @@ -1615,10 +1653,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>   
>       ehdr = PKT_GET_ETH_HDR(filter_buf);
>       net_rx_pkt_set_packet_type(core->rx_pkt, get_eth_packet_type(ehdr));
> -
> -    net_rx_pkt_attach_iovec_ex(core->rx_pkt, iov, iovcnt, iov_ofs,
> -                               e1000x_vlan_enabled(core->mac),
> -                               core->mac[VET] & 0xffff);
> +    net_rx_pkt_set_protocols(core->rx_pkt, filter_buf, size);
>   
>       queues = igb_receive_assign(core, ehdr, &rss_info, external_tx);
>       if (!queues) {
> @@ -1626,8 +1661,8 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>           return orig_size;
>       }
>   
> -    total_size = net_rx_pkt_get_total_len(core->rx_pkt) +
> -        e1000x_fcs_len(core->mac);
> +    retval = orig_size;
> +    total_size = size + e1000x_fcs_len(core->mac);
>   
>       for (i = 0; i < IGB_NUM_QUEUES; i++) {
>           if (!(queues & BIT(i))) {
> @@ -1635,43 +1670,38 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>           }
>   
>           igb_rx_ring_init(core, &rxr, i);
> +        strip_vlan = igb_rx_strip_vlan(core, rxr.i,
> +            get_eth_packet_type(ehdr));
> +        net_rx_pkt_attach_iovec_ex(core->rx_pkt, iov, iovcnt, iov_ofs,
> +            strip_vlan, core->mac[VET] & 0xffff);
> +        igb_rx_fix_l4_csum(core, core->rx_pkt);
> +
>           if (!igb_has_rxbufs(core, rxr.i, total_size)) {
>               icr_bits |= E1000_ICS_RXO;
> +            continue;
>           }
> -    }
> -
> -    if (!icr_bits) {
> -        retval = orig_size;
> -        igb_rx_fix_l4_csum(core, core->rx_pkt);
> -
> -        for (i = 0; i < IGB_NUM_QUEUES; i++) {
> -            if (!(queues & BIT(i))) {
> -                continue;
> -            }
>   
> -            igb_rx_ring_init(core, &rxr, i);
> -            if (igb_is_oversized(core, rxr.i, size)) {
> -                oversized |= BIT(i);
> -                continue;
> -            }
> +        if (igb_is_oversized(core, rxr.i, total_size)) {
> +            oversized |= BIT(i);
> +            continue;
> +        }
>   
> -            if (!(core->mac[RXDCTL0 + (i * 16)] & E1000_RXDCTL_QUEUE_ENABLE)) {
> -                continue;
> -            }
> +        if (!(core->mac[RXDCTL0 + (i * 16)] & E1000_RXDCTL_QUEUE_ENABLE)) {
> +            continue;
> +        }
>   
> -            trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
> -            igb_write_packet_to_guest(core, core->rx_pkt, &rxr, &rss_info);
> +        trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
> +        igb_write_packet_to_guest(core, core->rx_pkt, &rxr, &rss_info);
>   
> -            /* Check if receive descriptor minimum threshold hit */
> -            if (igb_rx_descr_threshold_hit(core, rxr.i)) {
> -                icr_bits |= E1000_ICS_RXDMT0;
> -            }
> +        /* Check if receive descriptor minimum threshold hit */
> +        if (igb_rx_descr_threshold_hit(core, rxr.i)) {
> +            icr_bits |= E1000_ICS_RXDMT0;
> +        }
>   
> -            core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
> +        core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
>   
> -            /* same as RXDW (rx descriptor written back)*/
> -            icr_bits |= E1000_ICR_RXDW;
> -        }
> +        /* same as RXDW (rx descriptor written back)*/
> +        icr_bits |= E1000_ICR_RXDW;
>       }
>   
>       /* 8.19.37 increment ROC only if packet is oversized for all queues */


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/9] igb: implement VFRE and VFTE registers
  2023-01-28 13:46 ` [PATCH 3/9] igb: implement VFRE and VFTE registers Sriram Yagnaraman
@ 2023-01-29  9:15   ` Akihiko Odaki
  2023-01-30 10:16     ` Sriram Yagnaraman
  0 siblings, 1 reply; 20+ messages in thread
From: Akihiko Odaki @ 2023-01-29  9:15 UTC (permalink / raw)
  To: Sriram Yagnaraman
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

On 2023/01/28 22:46, Sriram Yagnaraman wrote:
> Also add checks for RXDCTL/TXDCTL queue enable bits
> 
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> ---
>   hw/net/igb_core.c | 42 +++++++++++++++++++++++++++++++-----------
>   hw/net/igb_regs.h |  3 ++-
>   2 files changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index 9bd53cc25f..6bca5459b9 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -778,6 +778,19 @@ igb_txdesc_writeback(IGBCore *core, dma_addr_t base,
>       return igb_tx_wb_eic(core, txi->idx);
>   }
>   
> +static inline bool
> +igb_tx_enabled(IGBCore *core, const E1000E_RingInfo *txi)
> +{
> +    bool vmdq = core->mac[MRQC] & 1;
> +    uint16_t qn = txi->idx;
> +    uint16_t vfn = (qn > IGB_MAX_VF_FUNCTIONS) ?
> +                   (qn - IGB_MAX_VF_FUNCTIONS) : qn;
> +
> +    return (core->mac[TCTL] & E1000_TCTL_EN) &&
> +        (vmdq ? (core->mac[VFTE] & BIT(vfn)) : true) &&

Instead, do: (!vmdq || core->mac[VFTE] & BIT(vfn))

> +        (core->mac[TXDCTL0 + (qn * 16)] & E1000_TXDCTL_QUEUE_ENABLE);
> +}
> +
>   static void
>   igb_start_xmit(IGBCore *core, const IGB_TxRing *txr)
>   {
> @@ -787,8 +800,7 @@ igb_start_xmit(IGBCore *core, const IGB_TxRing *txr)
>       const E1000E_RingInfo *txi = txr->i;
>       uint32_t eic = 0;
>   
> -    /* TODO: check if the queue itself is enabled too. */
> -    if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
> +    if (!igb_tx_enabled(core, txi)) {
>           trace_e1000e_tx_disabled();
>           return;
>       }
> @@ -1003,6 +1015,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header *ehdr,
>               queues = BIT(def_pl >> E1000_VT_CTL_DEFAULT_POOL_SHIFT);
>           }
>   
> +        queues &= core->mac[VFRE];
>           igb_rss_parse_packet(core, core->rx_pkt, external_tx != NULL, rss_info);
>           if (rss_info->queue & 1) {
>               queues <<= 8;
> @@ -1486,7 +1499,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>       static const int maximum_ethernet_hdr_len = (ETH_HLEN + 4);
>   
>       uint16_t queues = 0;
> -    uint32_t n;
> +    uint32_t n = 0;
>       uint8_t min_buf[ETH_ZLEN];
>       struct iovec min_iov;
>       struct eth_header *ehdr;
> @@ -1566,26 +1579,22 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>           }
>   
>           igb_rx_ring_init(core, &rxr, i);
> -
> -        trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
> -
>           if (!igb_has_rxbufs(core, rxr.i, total_size)) {
>               retval = 0;
>           }

This stops sending packet when a disabled queue has no space.

>       }
>   
>       if (retval) {
> -        n = E1000_ICR_RXT0;
> -
>           igb_rx_fix_l4_csum(core, core->rx_pkt);
>   
>           for (i = 0; i < IGB_NUM_QUEUES; i++) {
> -            if (!(queues & BIT(i))) {
> +            if (!(queues & BIT(i)) ||
> +                !(core->mac[E1000_RXDCTL(i) >> 2] & E1000_RXDCTL_QUEUE_ENABLE)) {
>                   continue;
>               }
>   
>               igb_rx_ring_init(core, &rxr, i);
> -
> +            trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
>               igb_write_packet_to_guest(core, core->rx_pkt, &rxr, &rss_info);
>   
>               /* Check if receive descriptor minimum threshold hit */
> @@ -1594,6 +1603,9 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>               }
>   
>               core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
> +
> +            /* same as RXDW (rx descriptor written back)*/
> +            n = E1000_ICR_RXT0;
>           }
>   
>           trace_e1000e_rx_written_to_guest(n);
> @@ -1981,9 +1993,16 @@ static void igb_set_vfmailbox(IGBCore *core, int index, uint32_t val)
>   
>   static void igb_vf_reset(IGBCore *core, uint16_t vfn)
>   {
> +    uint16_t qn0 = vfn;
> +    uint16_t qn1 = vfn + IGB_MAX_VF_FUNCTIONS;
> +
>       /* disable Rx and Tx for the VF*/
> -    core->mac[VFTE] &= ~BIT(vfn);
> +    core->mac[RXDCTL0 + (qn0 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
> +    core->mac[RXDCTL0 + (qn1 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
> +    core->mac[TXDCTL0 + (qn0 * 16)] &= ~E1000_TXDCTL_QUEUE_ENABLE;
> +    core->mac[TXDCTL0 + (qn1 * 16)] &= ~E1000_TXDCTL_QUEUE_ENABLE;
>       core->mac[VFRE] &= ~BIT(vfn);
> +    core->mac[VFTE] &= ~BIT(vfn);
>       /* indicate VF reset to PF */
>       core->mac[VFLRE] |= BIT(vfn);
>       /* VFLRE and mailbox use the same interrupt cause */
> @@ -3889,6 +3908,7 @@ igb_phy_reg_init[] = {
>   static const uint32_t igb_mac_reg_init[] = {
>       [LEDCTL]        = 2 | (3 << 8) | BIT(15) | (6 << 16) | (7 << 24),
>       [EEMNGCTL]      = BIT(31),
> +    [TXDCTL0]       = E1000_TXDCTL_QUEUE_ENABLE,
>       [RXDCTL0]       = E1000_RXDCTL_QUEUE_ENABLE | (1 << 16),
>       [RXDCTL1]       = 1 << 16,
>       [RXDCTL2]       = 1 << 16,
> diff --git a/hw/net/igb_regs.h b/hw/net/igb_regs.h
> index ebf3e95023..084e751378 100644
> --- a/hw/net/igb_regs.h
> +++ b/hw/net/igb_regs.h
> @@ -160,7 +160,8 @@ union e1000_adv_rx_desc {
>   #define E1000_MRQC_RSS_FIELD_IPV6_UDP       0x00800000
>   #define E1000_MRQC_RSS_FIELD_IPV6_UDP_EX    0x01000000
>   
> -/* Additional Receive Descriptor Control definitions */
> +/* Additional RX/TX Descriptor Control definitions */
> +#define E1000_TXDCTL_QUEUE_ENABLE  0x02000000 /* Enable specific Tx Queue */
>   #define E1000_RXDCTL_QUEUE_ENABLE  0x02000000 /* Enable specific Rx Queue */
>   
>   /* Direct Cache Access (DCA) definitions */


^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH 3/9] igb: implement VFRE and VFTE registers
  2023-01-29  9:15   ` Akihiko Odaki
@ 2023-01-30 10:16     ` Sriram Yagnaraman
  2023-01-30 13:20       ` Akihiko Odaki
  0 siblings, 1 reply; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-30 10:16 UTC (permalink / raw)
  To: Akihiko Odaki
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum



> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Sunday, 29 January 2023 10:16
> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
> Fleytman <dmitry.fleytman@gmail.com>; Michael S . Tsirkin
> <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Subject: Re: [PATCH 3/9] igb: implement VFRE and VFTE registers
> 
> On 2023/01/28 22:46, Sriram Yagnaraman wrote:
> > Also add checks for RXDCTL/TXDCTL queue enable bits
> >
> > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> > ---
> >   hw/net/igb_core.c | 42 +++++++++++++++++++++++++++++++-----------
> >   hw/net/igb_regs.h |  3 ++-
> >   2 files changed, 33 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> > 9bd53cc25f..6bca5459b9 100644
> > --- a/hw/net/igb_core.c
> > +++ b/hw/net/igb_core.c
> > @@ -778,6 +778,19 @@ igb_txdesc_writeback(IGBCore *core, dma_addr_t
> base,
> >       return igb_tx_wb_eic(core, txi->idx);
> >   }
> >
> > +static inline bool
> > +igb_tx_enabled(IGBCore *core, const E1000E_RingInfo *txi) {
> > +    bool vmdq = core->mac[MRQC] & 1;
> > +    uint16_t qn = txi->idx;
> > +    uint16_t vfn = (qn > IGB_MAX_VF_FUNCTIONS) ?
> > +                   (qn - IGB_MAX_VF_FUNCTIONS) : qn;
> > +
> > +    return (core->mac[TCTL] & E1000_TCTL_EN) &&
> > +        (vmdq ? (core->mac[VFTE] & BIT(vfn)) : true) &&
> 
> Instead, do: (!vmdq || core->mac[VFTE] & BIT(vfn))
> 
> > +        (core->mac[TXDCTL0 + (qn * 16)] & E1000_TXDCTL_QUEUE_ENABLE);
> > +}
> > +
> >   static void
> >   igb_start_xmit(IGBCore *core, const IGB_TxRing *txr)
> >   {
> > @@ -787,8 +800,7 @@ igb_start_xmit(IGBCore *core, const IGB_TxRing
> *txr)
> >       const E1000E_RingInfo *txi = txr->i;
> >       uint32_t eic = 0;
> >
> > -    /* TODO: check if the queue itself is enabled too. */
> > -    if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
> > +    if (!igb_tx_enabled(core, txi)) {
> >           trace_e1000e_tx_disabled();
> >           return;
> >       }
> > @@ -1003,6 +1015,7 @@ static uint16_t igb_receive_assign(IGBCore *core,
> const struct eth_header *ehdr,
> >               queues = BIT(def_pl >> E1000_VT_CTL_DEFAULT_POOL_SHIFT);
> >           }
> >
> > +        queues &= core->mac[VFRE];
> >           igb_rss_parse_packet(core, core->rx_pkt, external_tx != NULL,
> rss_info);
> >           if (rss_info->queue & 1) {
> >               queues <<= 8;
> > @@ -1486,7 +1499,7 @@ igb_receive_internal(IGBCore *core, const struct
> iovec *iov, int iovcnt,
> >       static const int maximum_ethernet_hdr_len = (ETH_HLEN + 4);
> >
> >       uint16_t queues = 0;
> > -    uint32_t n;
> > +    uint32_t n = 0;
> >       uint8_t min_buf[ETH_ZLEN];
> >       struct iovec min_iov;
> >       struct eth_header *ehdr;
> > @@ -1566,26 +1579,22 @@ igb_receive_internal(IGBCore *core, const
> struct iovec *iov, int iovcnt,
> >           }
> >
> >           igb_rx_ring_init(core, &rxr, i);
> > -
> > -        trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
> > -
> >           if (!igb_has_rxbufs(core, rxr.i, total_size)) {
> >               retval = 0;
> >           }
> 
> This stops sending packet when a disabled queue has no space.

Yes, that is true, but I have refactored this part a bit in patchset 9 that fixes this.

> 
> >       }
> >
> >       if (retval) {
> > -        n = E1000_ICR_RXT0;
> > -
> >           igb_rx_fix_l4_csum(core, core->rx_pkt);
> >
> >           for (i = 0; i < IGB_NUM_QUEUES; i++) {
> > -            if (!(queues & BIT(i))) {
> > +            if (!(queues & BIT(i)) ||
> > +                !(core->mac[E1000_RXDCTL(i) >> 2] &
> > + E1000_RXDCTL_QUEUE_ENABLE)) {
> >                   continue;
> >               }
> >
> >               igb_rx_ring_init(core, &rxr, i);
> > -
> > +            trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
> >               igb_write_packet_to_guest(core, core->rx_pkt, &rxr,
> > &rss_info);
> >
> >               /* Check if receive descriptor minimum threshold hit */
> > @@ -1594,6 +1603,9 @@ igb_receive_internal(IGBCore *core, const struct
> iovec *iov, int iovcnt,
> >               }
> >
> >               core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
> > +
> > +            /* same as RXDW (rx descriptor written back)*/
> > +            n = E1000_ICR_RXT0;
> >           }
> >
> >           trace_e1000e_rx_written_to_guest(n);
> > @@ -1981,9 +1993,16 @@ static void igb_set_vfmailbox(IGBCore *core,
> > int index, uint32_t val)
> >
> >   static void igb_vf_reset(IGBCore *core, uint16_t vfn)
> >   {
> > +    uint16_t qn0 = vfn;
> > +    uint16_t qn1 = vfn + IGB_MAX_VF_FUNCTIONS;
> > +
> >       /* disable Rx and Tx for the VF*/
> > -    core->mac[VFTE] &= ~BIT(vfn);
> > +    core->mac[RXDCTL0 + (qn0 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
> > +    core->mac[RXDCTL0 + (qn1 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
> > +    core->mac[TXDCTL0 + (qn0 * 16)] &= ~E1000_TXDCTL_QUEUE_ENABLE;
> > +    core->mac[TXDCTL0 + (qn1 * 16)] &= ~E1000_TXDCTL_QUEUE_ENABLE;
> >       core->mac[VFRE] &= ~BIT(vfn);
> > +    core->mac[VFTE] &= ~BIT(vfn);
> >       /* indicate VF reset to PF */
> >       core->mac[VFLRE] |= BIT(vfn);
> >       /* VFLRE and mailbox use the same interrupt cause */ @@ -3889,6
> > +3908,7 @@ igb_phy_reg_init[] = {
> >   static const uint32_t igb_mac_reg_init[] = {
> >       [LEDCTL]        = 2 | (3 << 8) | BIT(15) | (6 << 16) | (7 << 24),
> >       [EEMNGCTL]      = BIT(31),
> > +    [TXDCTL0]       = E1000_TXDCTL_QUEUE_ENABLE,
> >       [RXDCTL0]       = E1000_RXDCTL_QUEUE_ENABLE | (1 << 16),
> >       [RXDCTL1]       = 1 << 16,
> >       [RXDCTL2]       = 1 << 16,
> > diff --git a/hw/net/igb_regs.h b/hw/net/igb_regs.h index
> > ebf3e95023..084e751378 100644
> > --- a/hw/net/igb_regs.h
> > +++ b/hw/net/igb_regs.h
> > @@ -160,7 +160,8 @@ union e1000_adv_rx_desc {
> >   #define E1000_MRQC_RSS_FIELD_IPV6_UDP       0x00800000
> >   #define E1000_MRQC_RSS_FIELD_IPV6_UDP_EX    0x01000000
> >
> > -/* Additional Receive Descriptor Control definitions */
> > +/* Additional RX/TX Descriptor Control definitions */ #define
> > +E1000_TXDCTL_QUEUE_ENABLE  0x02000000 /* Enable specific Tx Queue
> */
> >   #define E1000_RXDCTL_QUEUE_ENABLE  0x02000000 /* Enable specific Rx
> > Queue */
> >
> >   /* Direct Cache Access (DCA) definitions */

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH 5/9] igb: respect E1000_VMOLR_RSSE
  2023-01-29  7:24   ` Akihiko Odaki
@ 2023-01-30 12:07     ` Sriram Yagnaraman
  2023-01-30 13:20       ` Akihiko Odaki
  0 siblings, 1 reply; 20+ messages in thread
From: Sriram Yagnaraman @ 2023-01-30 12:07 UTC (permalink / raw)
  To: Akihiko Odaki
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Sunday, 29 January 2023 08:25
> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
> Fleytman <dmitry.fleytman@gmail.com>; Michael S . Tsirkin
> <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Subject: Re: [PATCH 5/9] igb: respect E1000_VMOLR_RSSE
> 
> On 2023/01/28 22:46, Sriram Yagnaraman wrote:
> > RSS for VFs is only enabled if VMOLR[n].RSSE is set.
> >
> > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> > ---
> >   hw/net/igb_core.c | 18 +++++++++++++-----
> >   1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> > 1eb7ba168f..e4fd4a1a5f 100644
> > --- a/hw/net/igb_core.c
> > +++ b/hw/net/igb_core.c
> > @@ -69,7 +69,7 @@ typedef struct IGBTxPktVmdqCallbackContext {
> >
> >   static ssize_t
> >   igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
> > -                     bool has_vnet, bool *assigned);
> > +                     bool has_vnet, bool *external_tx);
> 
> I admit external_tx is somewhat confusing, but it is more than just telling if it
> is assigned to Rx queue or not. If external_tx is not NULL, it indicates it is part
> of Tx packet switching. In that case, a bool value which describes whether the
> packet should be routed to external LAN must be assigned. The value can be
> false even if the packet is assigned to Rx queues; it will be always false if it is
> multicast, for example.

Yes, I undestand the purpose of external_tx. I merely changed the parameter name in the function declaration to match it's defintion. Anyhow, I will remove this change since it was purely comsetic.

> 
> >
> >   static inline void
> >   igb_set_interrupt_cause(IGBCore *core, uint32_t val); @@ -942,7
> > +942,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const
> > struct eth_header *ehdr,
> >
> >       if (core->mac[MRQC] & 1) {
> >           if (is_broadcast_ether_addr(ehdr->h_dest)) {
> > -            for (i = 0; i < 8; i++) {
> > +            for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
> 
> I just left it as 8 because VMDq is not specific to VF. Perhaps it is better to
> have another macro to denote the number of VMDq pools, but it is not done
> yet.

Ok, I agree. I will introduce a new IGB_MAX_VM_POOLS macro instead.

> 
> >                   if (core->mac[VMOLR0 + i] & E1000_VMOLR_BAM) {
> >                       queues |= BIT(i);
> >                   }
> > @@ -976,7 +976,7 @@ static uint16_t igb_receive_assign(IGBCore *core,
> const struct eth_header *ehdr,
> >                   f = ta_shift[(rctl >> E1000_RCTL_MO_SHIFT) & 3];
> >                   f = (((ehdr->h_dest[5] << 8) | ehdr->h_dest[4]) >> f) & 0xfff;
> >                   if (macp[f >> 5] & (1 << (f & 0x1f))) {
> > -                    for (i = 0; i < 8; i++) {
> > +                    for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
> >                           if (core->mac[VMOLR0 + i] & E1000_VMOLR_ROMPE) {
> >                               queues |= BIT(i);
> >                           }
> > @@ -999,7 +999,7 @@ static uint16_t igb_receive_assign(IGBCore *core,
> const struct eth_header *ehdr,
> >                       }
> >                   }
> >               } else {
> > -                for (i = 0; i < 8; i++) {
> > +                for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
> >                       if (core->mac[VMOLR0 + i] & E1000_VMOLR_AUPE) {
> >                           mask |= BIT(i);
> >                       }
> > @@ -1018,7 +1018,15 @@ static uint16_t igb_receive_assign(IGBCore
> *core, const struct eth_header *ehdr,
> >           queues &= core->mac[VFRE];
> >           igb_rss_parse_packet(core, core->rx_pkt, external_tx != NULL,
> rss_info);
> >           if (rss_info->queue & 1) {
> > -            queues <<= 8;
> > +            for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
> > +                if (!(queues & BIT(i))) {
> > +                    continue;
> > +                }
> > +                if (core->mac[VMOLR0 + i] & E1000_VMOLR_RSSE) {
> > +                    queues |= BIT(i + IGB_MAX_VF_FUNCTIONS);
> > +                    queues &= ~BIT(i);
> > +                }
> > +            }
> >           }
> >       } else {
> >           switch (net_rx_pkt_get_packet_type(core->rx_pkt)) {

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/9] igb: implement VFRE and VFTE registers
  2023-01-30 10:16     ` Sriram Yagnaraman
@ 2023-01-30 13:20       ` Akihiko Odaki
  0 siblings, 0 replies; 20+ messages in thread
From: Akihiko Odaki @ 2023-01-30 13:20 UTC (permalink / raw)
  To: Sriram Yagnaraman
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

On 2023/01/30 19:16, Sriram Yagnaraman wrote:
> 
> 
>> -----Original Message-----
>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Sent: Sunday, 29 January 2023 10:16
>> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
>> Fleytman <dmitry.fleytman@gmail.com>; Michael S . Tsirkin
>> <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Subject: Re: [PATCH 3/9] igb: implement VFRE and VFTE registers
>>
>> On 2023/01/28 22:46, Sriram Yagnaraman wrote:
>>> Also add checks for RXDCTL/TXDCTL queue enable bits
>>>
>>> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>>> ---
>>>    hw/net/igb_core.c | 42 +++++++++++++++++++++++++++++++-----------
>>>    hw/net/igb_regs.h |  3 ++-
>>>    2 files changed, 33 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
>>> 9bd53cc25f..6bca5459b9 100644
>>> --- a/hw/net/igb_core.c
>>> +++ b/hw/net/igb_core.c
>>> @@ -778,6 +778,19 @@ igb_txdesc_writeback(IGBCore *core, dma_addr_t
>> base,
>>>        return igb_tx_wb_eic(core, txi->idx);
>>>    }
>>>
>>> +static inline bool
>>> +igb_tx_enabled(IGBCore *core, const E1000E_RingInfo *txi) {
>>> +    bool vmdq = core->mac[MRQC] & 1;
>>> +    uint16_t qn = txi->idx;
>>> +    uint16_t vfn = (qn > IGB_MAX_VF_FUNCTIONS) ?
>>> +                   (qn - IGB_MAX_VF_FUNCTIONS) : qn;
>>> +
>>> +    return (core->mac[TCTL] & E1000_TCTL_EN) &&
>>> +        (vmdq ? (core->mac[VFTE] & BIT(vfn)) : true) &&
>>
>> Instead, do: (!vmdq || core->mac[VFTE] & BIT(vfn))
>>
>>> +        (core->mac[TXDCTL0 + (qn * 16)] & E1000_TXDCTL_QUEUE_ENABLE);
>>> +}
>>> +
>>>    static void
>>>    igb_start_xmit(IGBCore *core, const IGB_TxRing *txr)
>>>    {
>>> @@ -787,8 +800,7 @@ igb_start_xmit(IGBCore *core, const IGB_TxRing
>> *txr)
>>>        const E1000E_RingInfo *txi = txr->i;
>>>        uint32_t eic = 0;
>>>
>>> -    /* TODO: check if the queue itself is enabled too. */
>>> -    if (!(core->mac[TCTL] & E1000_TCTL_EN)) {
>>> +    if (!igb_tx_enabled(core, txi)) {
>>>            trace_e1000e_tx_disabled();
>>>            return;
>>>        }
>>> @@ -1003,6 +1015,7 @@ static uint16_t igb_receive_assign(IGBCore *core,
>> const struct eth_header *ehdr,
>>>                queues = BIT(def_pl >> E1000_VT_CTL_DEFAULT_POOL_SHIFT);
>>>            }
>>>
>>> +        queues &= core->mac[VFRE];
>>>            igb_rss_parse_packet(core, core->rx_pkt, external_tx != NULL,
>> rss_info);
>>>            if (rss_info->queue & 1) {
>>>                queues <<= 8;
>>> @@ -1486,7 +1499,7 @@ igb_receive_internal(IGBCore *core, const struct
>> iovec *iov, int iovcnt,
>>>        static const int maximum_ethernet_hdr_len = (ETH_HLEN + 4);
>>>
>>>        uint16_t queues = 0;
>>> -    uint32_t n;
>>> +    uint32_t n = 0;
>>>        uint8_t min_buf[ETH_ZLEN];
>>>        struct iovec min_iov;
>>>        struct eth_header *ehdr;
>>> @@ -1566,26 +1579,22 @@ igb_receive_internal(IGBCore *core, const
>> struct iovec *iov, int iovcnt,
>>>            }
>>>
>>>            igb_rx_ring_init(core, &rxr, i);
>>> -
>>> -        trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
>>> -
>>>            if (!igb_has_rxbufs(core, rxr.i, total_size)) {
>>>                retval = 0;
>>>            }
>>
>> This stops sending packet when a disabled queue has no space.
> 
> Yes, that is true, but I have refactored this part a bit in patchset 9 that fixes this.

I see. Please include the fix in this patch so it will be easier to 
review and it won't prevent from bisecting.

> 
>>
>>>        }
>>>
>>>        if (retval) {
>>> -        n = E1000_ICR_RXT0;
>>> -
>>>            igb_rx_fix_l4_csum(core, core->rx_pkt);
>>>
>>>            for (i = 0; i < IGB_NUM_QUEUES; i++) {
>>> -            if (!(queues & BIT(i))) {
>>> +            if (!(queues & BIT(i)) ||
>>> +                !(core->mac[E1000_RXDCTL(i) >> 2] &
>>> + E1000_RXDCTL_QUEUE_ENABLE)) {
>>>                    continue;
>>>                }
>>>
>>>                igb_rx_ring_init(core, &rxr, i);
>>> -
>>> +            trace_e1000e_rx_rss_dispatched_to_queue(rxr.i->idx);
>>>                igb_write_packet_to_guest(core, core->rx_pkt, &rxr,
>>> &rss_info);
>>>
>>>                /* Check if receive descriptor minimum threshold hit */
>>> @@ -1594,6 +1603,9 @@ igb_receive_internal(IGBCore *core, const struct
>> iovec *iov, int iovcnt,
>>>                }
>>>
>>>                core->mac[EICR] |= igb_rx_wb_eic(core, rxr.i->idx);
>>> +
>>> +            /* same as RXDW (rx descriptor written back)*/
>>> +            n = E1000_ICR_RXT0;
>>>            }
>>>
>>>            trace_e1000e_rx_written_to_guest(n);
>>> @@ -1981,9 +1993,16 @@ static void igb_set_vfmailbox(IGBCore *core,
>>> int index, uint32_t val)
>>>
>>>    static void igb_vf_reset(IGBCore *core, uint16_t vfn)
>>>    {
>>> +    uint16_t qn0 = vfn;
>>> +    uint16_t qn1 = vfn + IGB_MAX_VF_FUNCTIONS;
>>> +
>>>        /* disable Rx and Tx for the VF*/
>>> -    core->mac[VFTE] &= ~BIT(vfn);
>>> +    core->mac[RXDCTL0 + (qn0 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
>>> +    core->mac[RXDCTL0 + (qn1 * 16)] &= ~E1000_RXDCTL_QUEUE_ENABLE;
>>> +    core->mac[TXDCTL0 + (qn0 * 16)] &= ~E1000_TXDCTL_QUEUE_ENABLE;
>>> +    core->mac[TXDCTL0 + (qn1 * 16)] &= ~E1000_TXDCTL_QUEUE_ENABLE;
>>>        core->mac[VFRE] &= ~BIT(vfn);
>>> +    core->mac[VFTE] &= ~BIT(vfn);
>>>        /* indicate VF reset to PF */
>>>        core->mac[VFLRE] |= BIT(vfn);
>>>        /* VFLRE and mailbox use the same interrupt cause */ @@ -3889,6
>>> +3908,7 @@ igb_phy_reg_init[] = {
>>>    static const uint32_t igb_mac_reg_init[] = {
>>>        [LEDCTL]        = 2 | (3 << 8) | BIT(15) | (6 << 16) | (7 << 24),
>>>        [EEMNGCTL]      = BIT(31),
>>> +    [TXDCTL0]       = E1000_TXDCTL_QUEUE_ENABLE,
>>>        [RXDCTL0]       = E1000_RXDCTL_QUEUE_ENABLE | (1 << 16),
>>>        [RXDCTL1]       = 1 << 16,
>>>        [RXDCTL2]       = 1 << 16,
>>> diff --git a/hw/net/igb_regs.h b/hw/net/igb_regs.h index
>>> ebf3e95023..084e751378 100644
>>> --- a/hw/net/igb_regs.h
>>> +++ b/hw/net/igb_regs.h
>>> @@ -160,7 +160,8 @@ union e1000_adv_rx_desc {
>>>    #define E1000_MRQC_RSS_FIELD_IPV6_UDP       0x00800000
>>>    #define E1000_MRQC_RSS_FIELD_IPV6_UDP_EX    0x01000000
>>>
>>> -/* Additional Receive Descriptor Control definitions */
>>> +/* Additional RX/TX Descriptor Control definitions */ #define
>>> +E1000_TXDCTL_QUEUE_ENABLE  0x02000000 /* Enable specific Tx Queue
>> */
>>>    #define E1000_RXDCTL_QUEUE_ENABLE  0x02000000 /* Enable specific Rx
>>> Queue */
>>>
>>>    /* Direct Cache Access (DCA) definitions */


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/9] igb: respect E1000_VMOLR_RSSE
  2023-01-30 12:07     ` Sriram Yagnaraman
@ 2023-01-30 13:20       ` Akihiko Odaki
  0 siblings, 0 replies; 20+ messages in thread
From: Akihiko Odaki @ 2023-01-30 13:20 UTC (permalink / raw)
  To: Sriram Yagnaraman
  Cc: qemu-devel, Jason Wang, Dmitry Fleytman, Michael S . Tsirkin,
	Marcel Apfelbaum

On 2023/01/30 21:07, Sriram Yagnaraman wrote:
>> -----Original Message-----
>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Sent: Sunday, 29 January 2023 08:25
>> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
>> Fleytman <dmitry.fleytman@gmail.com>; Michael S . Tsirkin
>> <mst@redhat.com>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Subject: Re: [PATCH 5/9] igb: respect E1000_VMOLR_RSSE
>>
>> On 2023/01/28 22:46, Sriram Yagnaraman wrote:
>>> RSS for VFs is only enabled if VMOLR[n].RSSE is set.
>>>
>>> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>>> ---
>>>    hw/net/igb_core.c | 18 +++++++++++++-----
>>>    1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
>>> 1eb7ba168f..e4fd4a1a5f 100644
>>> --- a/hw/net/igb_core.c
>>> +++ b/hw/net/igb_core.c
>>> @@ -69,7 +69,7 @@ typedef struct IGBTxPktVmdqCallbackContext {
>>>
>>>    static ssize_t
>>>    igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>>> -                     bool has_vnet, bool *assigned);
>>> +                     bool has_vnet, bool *external_tx);
>>
>> I admit external_tx is somewhat confusing, but it is more than just telling if it
>> is assigned to Rx queue or not. If external_tx is not NULL, it indicates it is part
>> of Tx packet switching. In that case, a bool value which describes whether the
>> packet should be routed to external LAN must be assigned. The value can be
>> false even if the packet is assigned to Rx queues; it will be always false if it is
>> multicast, for example.
> 
> Yes, I undestand the purpose of external_tx. I merely changed the parameter name in the function declaration to match it's defintion. Anyhow, I will remove this change since it was purely comsetic.

The definition is wrong then. I'll submit the new version with it fixed.

> 
>>
>>>
>>>    static inline void
>>>    igb_set_interrupt_cause(IGBCore *core, uint32_t val); @@ -942,7
>>> +942,7 @@ static uint16_t igb_receive_assign(IGBCore *core, const
>>> struct eth_header *ehdr,
>>>
>>>        if (core->mac[MRQC] & 1) {
>>>            if (is_broadcast_ether_addr(ehdr->h_dest)) {
>>> -            for (i = 0; i < 8; i++) {
>>> +            for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
>>
>> I just left it as 8 because VMDq is not specific to VF. Perhaps it is better to
>> have another macro to denote the number of VMDq pools, but it is not done
>> yet.
> 
> Ok, I agree. I will introduce a new IGB_MAX_VM_POOLS macro instead.

You don't need "MAX" as the number of pools is fixed if I understand it 
correctly.

> 
>>
>>>                    if (core->mac[VMOLR0 + i] & E1000_VMOLR_BAM) {
>>>                        queues |= BIT(i);
>>>                    }
>>> @@ -976,7 +976,7 @@ static uint16_t igb_receive_assign(IGBCore *core,
>> const struct eth_header *ehdr,
>>>                    f = ta_shift[(rctl >> E1000_RCTL_MO_SHIFT) & 3];
>>>                    f = (((ehdr->h_dest[5] << 8) | ehdr->h_dest[4]) >> f) & 0xfff;
>>>                    if (macp[f >> 5] & (1 << (f & 0x1f))) {
>>> -                    for (i = 0; i < 8; i++) {
>>> +                    for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
>>>                            if (core->mac[VMOLR0 + i] & E1000_VMOLR_ROMPE) {
>>>                                queues |= BIT(i);
>>>                            }
>>> @@ -999,7 +999,7 @@ static uint16_t igb_receive_assign(IGBCore *core,
>> const struct eth_header *ehdr,
>>>                        }
>>>                    }
>>>                } else {
>>> -                for (i = 0; i < 8; i++) {
>>> +                for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
>>>                        if (core->mac[VMOLR0 + i] & E1000_VMOLR_AUPE) {
>>>                            mask |= BIT(i);
>>>                        }
>>> @@ -1018,7 +1018,15 @@ static uint16_t igb_receive_assign(IGBCore
>> *core, const struct eth_header *ehdr,
>>>            queues &= core->mac[VFRE];
>>>            igb_rss_parse_packet(core, core->rx_pkt, external_tx != NULL,
>> rss_info);
>>>            if (rss_info->queue & 1) {
>>> -            queues <<= 8;
>>> +            for (i = 0; i < IGB_MAX_VF_FUNCTIONS; i++) {
>>> +                if (!(queues & BIT(i))) {
>>> +                    continue;
>>> +                }
>>> +                if (core->mac[VMOLR0 + i] & E1000_VMOLR_RSSE) {
>>> +                    queues |= BIT(i + IGB_MAX_VF_FUNCTIONS);
>>> +                    queues &= ~BIT(i);
>>> +                }
>>> +            }
>>>            }
>>>        } else {
>>>            switch (net_rx_pkt_get_packet_type(core->rx_pkt)) {


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2023-01-30 13:21 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-28 13:46 [PATCH 0/9] igb: add missing feature set from Sriram Yagnaraman
2023-01-28 13:46 ` [PATCH 1/9] MAINTAINERS: Add Sriram Yagnaraman as a igb reviewer Sriram Yagnaraman
2023-01-28 13:46 ` [PATCH 2/9] igb: handle PF/VF reset properly Sriram Yagnaraman
2023-01-29  5:58   ` Akihiko Odaki
2023-01-29  6:15     ` Akihiko Odaki
2023-01-28 13:46 ` [PATCH 3/9] igb: implement VFRE and VFTE registers Sriram Yagnaraman
2023-01-29  9:15   ` Akihiko Odaki
2023-01-30 10:16     ` Sriram Yagnaraman
2023-01-30 13:20       ` Akihiko Odaki
2023-01-28 13:46 ` [PATCH 4/9] igb: check oversized packets for VMDq Sriram Yagnaraman
2023-01-29  7:06   ` Akihiko Odaki
2023-01-28 13:46 ` [PATCH 5/9] igb: respect E1000_VMOLR_RSSE Sriram Yagnaraman
2023-01-29  7:24   ` Akihiko Odaki
2023-01-30 12:07     ` Sriram Yagnaraman
2023-01-30 13:20       ` Akihiko Odaki
2023-01-28 13:46 ` [PATCH 6/9] igb: add ICR_RXDW Sriram Yagnaraman
2023-01-28 13:46 ` [PATCH 7/9] igb: implement VF Tx and Rx stats Sriram Yagnaraman
2023-01-28 13:46 ` [PATCH 8/9] igb: respect VT_CTL ignore MAC field Sriram Yagnaraman
2023-01-28 13:46 ` [PATCH 9/9] igb: respect VMVIR and VMOLR for VLAN Sriram Yagnaraman
2023-01-29  8:13   ` Akihiko Odaki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.