diff mbox series

[v3,3/9] igb: add ICR_RXDW

Message ID 20230131094232.28863-4-sriram.yagnaraman@est.tech (mailing list archive)
State New, archived
Headers show
Series igb: merge changes from <20221229190817.25500-1-sriram.yagnaraman@est.tech> | expand

Commit Message

Sriram Yagnaraman Jan. 31, 2023, 9:42 a.m. UTC 
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    | 46 +++++++++++++++++---------------------------
 2 files changed, 22 insertions(+), 28 deletions(-)

Comments

Akihiko Odaki Feb. 1, 2023, 4:35 a.m. UTC | #1 
On 2023/01/31 18:42, Sriram Yagnaraman wrote:
> 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    | 46 +++++++++++++++++---------------------------
>   2 files changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/net/e1000x_regs.h b/hw/net/e1000x_regs.h
> index fb5b861135..f509db73a7 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 9c32ad5e36..e78bc3611a 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -1488,7 +1488,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 icr_bits = 0;
>       uint8_t min_buf[ETH_ZLEN];
>       struct iovec min_iov;
>       struct eth_header *ehdr;
> @@ -1561,6 +1561,7 @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>           e1000x_fcs_len(core->mac);
>   
>       retval = orig_size;
> +    igb_rx_fix_l4_csum(core, core->rx_pkt);
>   
>       for (i = 0; i < IGB_NUM_QUEUES; i++) {
>           if (!(queues & BIT(i))) {
> @@ -1569,43 +1570,32 @@ 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;
> +            icr_bits |= E1000_ICS_RXO;
> +            continue;
>           }
> -    }
>   
> -    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))) {
> -                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);
>   
> -            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)) {
> -                n |= 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);
> -        }
> +        icr_bits |= E1000_ICR_RXDW;
> +    }
>   
> -        trace_e1000e_rx_written_to_guest(n);
> +    if (icr_bits & E1000_ICR_RXDW) {
> +        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;
>   }

The change for igb_receive_internal() actually fixes a bug; even if a 
pool doesn't have enough space to write back a packet, it shouldn't 
prevent other pools from receiving the packet.

I have fixed in v7 (well, I intended to do that in v6 but I made some 
mistakes) of "introduce igb" series, but there are some differences:
- "n" is not renamed to "icr_bits". Yes, "n" is a bad name, but if it's 
to be fixed, it should be done for e1000e too at the same time.
- "retval" variable is removed.
- tracepoints were updated so that we can see to which queue the Rx 
packet is written back.

E1000_ICR_RXDW is still not added to "introduce igb" series so please 
rebase this and submit again.
Sriram Yagnaraman Feb. 1, 2023, 10:29 a.m. UTC | #2 
> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Wednesday, 1 February 2023 05:36
> 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 v3 3/9] igb: add ICR_RXDW
> 
> On 2023/01/31 18:42, Sriram Yagnaraman wrote:
> > 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    | 46 +++++++++++++++++---------------------------
> >   2 files changed, 22 insertions(+), 28 deletions(-)
> >
> > diff --git a/hw/net/e1000x_regs.h b/hw/net/e1000x_regs.h index
> > fb5b861135..f509db73a7 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
> > 9c32ad5e36..e78bc3611a 100644
> > --- a/hw/net/igb_core.c
> > +++ b/hw/net/igb_core.c
> > @@ -1488,7 +1488,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 icr_bits = 0;
> >       uint8_t min_buf[ETH_ZLEN];
> >       struct iovec min_iov;
> >       struct eth_header *ehdr;
> > @@ -1561,6 +1561,7 @@ igb_receive_internal(IGBCore *core, const struct
> iovec *iov, int iovcnt,
> >           e1000x_fcs_len(core->mac);
> >
> >       retval = orig_size;
> > +    igb_rx_fix_l4_csum(core, core->rx_pkt);
> >
> >       for (i = 0; i < IGB_NUM_QUEUES; i++) {
> >           if (!(queues & BIT(i))) {
> > @@ -1569,43 +1570,32 @@ 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;
> > +            icr_bits |= E1000_ICS_RXO;
> > +            continue;
> >           }
> > -    }
> >
> > -    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))) {
> > -                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);
> >
> > -            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)) {
> > -                n |= 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);
> > -        }
> > +        icr_bits |= E1000_ICR_RXDW;
> > +    }
> >
> > -        trace_e1000e_rx_written_to_guest(n);
> > +    if (icr_bits & E1000_ICR_RXDW) {
> > +        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;
> >   }
> 
> The change for igb_receive_internal() actually fixes a bug; even if a pool
> doesn't have enough space to write back a packet, it shouldn't prevent other
> pools from receiving the packet.
> 
> I have fixed in v7 (well, I intended to do that in v6 but I made some
> mistakes) of "introduce igb" series, but there are some differences:
> - "n" is not renamed to "icr_bits". Yes, "n" is a bad name, but if it's to be fixed,
> it should be done for e1000e too at the same time.
> - "retval" variable is removed.
> - tracepoints were updated so that we can see to which queue the Rx packet is
> written back.
> 
> E1000_ICR_RXDW is still not added to "introduce igb" series so please rebase
> this and submit again.

Okay, I fix that.
diff mbox series

Patch

diff --git a/hw/net/e1000x_regs.h b/hw/net/e1000x_regs.h
index fb5b861135..f509db73a7 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 9c32ad5e36..e78bc3611a 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -1488,7 +1488,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 icr_bits = 0;
     uint8_t min_buf[ETH_ZLEN];
     struct iovec min_iov;
     struct eth_header *ehdr;
@@ -1561,6 +1561,7 @@  igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
         e1000x_fcs_len(core->mac);
 
     retval = orig_size;
+    igb_rx_fix_l4_csum(core, core->rx_pkt);
 
     for (i = 0; i < IGB_NUM_QUEUES; i++) {
         if (!(queues & BIT(i))) {
@@ -1569,43 +1570,32 @@  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;
+            icr_bits |= E1000_ICS_RXO;
+            continue;
         }
-    }
 
-    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))) {
-                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);
 
-            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)) {
-                n |= 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);
-        }
+        icr_bits |= E1000_ICR_RXDW;
+    }
 
-        trace_e1000e_rx_written_to_guest(n);
+    if (icr_bits & E1000_ICR_RXDW) {
+        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;
 }