Re: [PATCH 30/40] igb: Implement igb-specific oversize check

2023-04-21 Thread Akihiko Odaki 

On 2023/04/16 20:22, Sriram Yagnaraman wrote:




-Original Message-
From: Akihiko Odaki 
Sent: Friday, 14 April 2023 13:37
Cc: Sriram Yagnaraman ; Jason Wang
; Dmitry Fleytman ;
Michael S. Tsirkin ; Alex Bennée ;
Philippe Mathieu-Daudé ; Thomas Huth
; Wainer dos Santos Moschetta
; Beraldo Leal ; Cleber Rosa
; Laurent Vivier ; Paolo Bonzini
; qemu-devel@nongnu.org; Akihiko Odaki

Subject: [PATCH 30/40] igb: Implement igb-specific oversize check

igb has a configurable size limit for LPE, and uses different limits depending 
on
whether the packet is treated as a VLAN packet.

Signed-off-by: Akihiko Odaki 
---
  hw/net/igb_core.c | 41 +++--
  1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
2013a9a53d..569897fb99 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -954,16 +954,21 @@ igb_rx_l4_cso_enabled(IGBCore *core)
  return !!(core->mac[RXCSUM] & E1000_RXCSUM_TUOFLD);  }

-static bool


The convention in seems to be to declare return value in first line and then 
the function name in the next line.


There are already functions not following the convention, and it is more 
like exceptional in the entire QEMU code base. This patch prioritize the 
QEMU's common practice over e1000e's old convention.





-igb_rx_is_oversized(IGBCore *core, uint16_t qn, size_t size)
+static bool igb_rx_is_oversized(IGBCore *core, const struct eth_header *ehdr,
+size_t size, bool lpe, uint16_t rlpml)
  {
-uint16_t pool = qn % IGB_NUM_VM_POOLS;
-bool lpe = !!(core->mac[VMOLR0 + pool] & E1000_VMOLR_LPE);
-int max_ethernet_lpe_size =
-core->mac[VMOLR0 + pool] & E1000_VMOLR_RLPML_MASK;
-int max_ethernet_vlan_size = 1522;
+size += 4;


Is the above 4 CRC bytes?


Yes. In v2, a new constant ETH_FCS_LEN is used to explictly state that.




+
+if (lpe) {
+return size > rlpml;
+}
+
+if (e1000x_is_vlan_packet(ehdr, core->mac[VET] & 0x) &&
+e1000x_vlan_rx_filter_enabled(core->mac)) {
+return size > 1522;
+}


Should a check for 1526 bytes if extended VLAN is present be added?
Maybe in "igb: Strip the second VLAN tag for extended VLAN"?


In v2, I placed "igb: Strip the second VLAN tag for extended VLAN" 
earlier than this patch, and this patch is rewritten so it can handle 
the second VLAN tag too.






-return size > (lpe ? max_ethernet_lpe_size : max_ethernet_vlan_size);
+return size > 1518;
  }

  static uint16_t igb_receive_assign(IGBCore *core, const L2Header *l2_header,
@@ -976,6 +981,8 @@ static uint16_t igb_receive_assign(IGBCore *core,
const L2Header *l2_header,
  uint16_t queues = 0;
  uint16_t oversized = 0;
  uint16_t vid = be16_to_cpu(l2_header->vlan[0].h_tci) & VLAN_VID_MASK;
+bool lpe;
+uint16_t rlpml;
  int i;

  memset(rss_info, 0, sizeof(E1000E_RSSInfo)); @@ -984,6 +991,14 @@
static uint16_t igb_receive_assign(IGBCore *core, const L2Header *l2_header,
  *external_tx = true;
  }

+lpe = !!(core->mac[RCTL] & E1000_RCTL_LPE);
+rlpml = core->mac[RLPML];
+if (!(core->mac[RCTL] & E1000_RCTL_SBP) &&
+igb_rx_is_oversized(core, ehdr, size, lpe, rlpml)) {
+trace_e1000x_rx_oversized(size);
+return queues;
+}
+
  if (e1000x_is_vlan_packet(ehdr, core->mac[VET] & 0x) &&
  !e1000x_rx_vlan_filter(core->mac, PKT_GET_VLAN_HDR(ehdr))) {
  return queues;
@@ -1067,7 +1082,10 @@ static uint16_t igb_receive_assign(IGBCore *core,
const L2Header *l2_header,
  queues &= core->mac[VFRE];
  if (queues) {
  for (i = 0; i < IGB_NUM_VM_POOLS; i++) {
-if ((queues & BIT(i)) && igb_rx_is_oversized(core, i, size)) {
+lpe = !!(core->mac[VMOLR0 + i] & E1000_VMOLR_LPE);
+rlpml = core->mac[VMOLR0 + i] & E1000_VMOLR_RLPML_MASK;
+if ((queues & BIT(i)) &&
+igb_rx_is_oversized(core, ehdr, size, lpe, rlpml))
+ {
  oversized |= BIT(i);
  }
  }
@@ -1609,11 +1627,6 @@ igb_receive_internal(IGBCore *core, const struct
iovec *iov, int iovcnt,
  iov_to_buf(iov, iovcnt, iov_ofs, _buf, sizeof(min_buf.l2_header));
  }

-/* Discard oversized packets if !LPE and !SBP. */
-if (e1000x_is_oversized(core->mac, size)) {
-return orig_size;
-}
-
  net_rx_pkt_set_packet_type(core->rx_pkt,
 get_eth_packet_type(_buf.l2_header.eth));
  net_rx_pkt_set_protocols(core->rx_pkt, iov, iovcnt, iov_ofs);
--
2.40.0

RE: [PATCH 30/40] igb: Implement igb-specific oversize check

2023-04-16 Thread Sriram Yagnaraman 


> -Original Message-
> From: Akihiko Odaki 
> Sent: Friday, 14 April 2023 13:37
> Cc: Sriram Yagnaraman ; Jason Wang
> ; Dmitry Fleytman ;
> Michael S. Tsirkin ; Alex Bennée ;
> Philippe Mathieu-Daudé ; Thomas Huth
> ; Wainer dos Santos Moschetta
> ; Beraldo Leal ; Cleber Rosa
> ; Laurent Vivier ; Paolo Bonzini
> ; qemu-devel@nongnu.org; Akihiko Odaki
> 
> Subject: [PATCH 30/40] igb: Implement igb-specific oversize check
> 
> igb has a configurable size limit for LPE, and uses different limits 
> depending on
> whether the packet is treated as a VLAN packet.
> 
> Signed-off-by: Akihiko Odaki 
> ---
>  hw/net/igb_core.c | 41 +++--
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> 2013a9a53d..569897fb99 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -954,16 +954,21 @@ igb_rx_l4_cso_enabled(IGBCore *core)
>  return !!(core->mac[RXCSUM] & E1000_RXCSUM_TUOFLD);  }
> 
> -static bool

The convention in seems to be to declare return value in first line and then 
the function name in the next line. 

> -igb_rx_is_oversized(IGBCore *core, uint16_t qn, size_t size)
> +static bool igb_rx_is_oversized(IGBCore *core, const struct eth_header *ehdr,
> +size_t size, bool lpe, uint16_t rlpml)
>  {
> -uint16_t pool = qn % IGB_NUM_VM_POOLS;
> -bool lpe = !!(core->mac[VMOLR0 + pool] & E1000_VMOLR_LPE);
> -int max_ethernet_lpe_size =
> -core->mac[VMOLR0 + pool] & E1000_VMOLR_RLPML_MASK;
> -int max_ethernet_vlan_size = 1522;
> +size += 4;

Is the above 4 CRC bytes?

> +
> +if (lpe) {
> +return size > rlpml;
> +}
> +
> +if (e1000x_is_vlan_packet(ehdr, core->mac[VET] & 0x) &&
> +e1000x_vlan_rx_filter_enabled(core->mac)) {
> +return size > 1522;
> +}

Should a check for 1526 bytes if extended VLAN is present be added?
Maybe in "igb: Strip the second VLAN tag for extended VLAN"?

> 
> -return size > (lpe ? max_ethernet_lpe_size : max_ethernet_vlan_size);
> +return size > 1518;
>  }
> 
>  static uint16_t igb_receive_assign(IGBCore *core, const L2Header *l2_header,
> @@ -976,6 +981,8 @@ static uint16_t igb_receive_assign(IGBCore *core,
> const L2Header *l2_header,
>  uint16_t queues = 0;
>  uint16_t oversized = 0;
>  uint16_t vid = be16_to_cpu(l2_header->vlan[0].h_tci) & VLAN_VID_MASK;
> +bool lpe;
> +uint16_t rlpml;
>  int i;
> 
>  memset(rss_info, 0, sizeof(E1000E_RSSInfo)); @@ -984,6 +991,14 @@
> static uint16_t igb_receive_assign(IGBCore *core, const L2Header *l2_header,
>  *external_tx = true;
>  }
> 
> +lpe = !!(core->mac[RCTL] & E1000_RCTL_LPE);
> +rlpml = core->mac[RLPML];
> +if (!(core->mac[RCTL] & E1000_RCTL_SBP) &&
> +igb_rx_is_oversized(core, ehdr, size, lpe, rlpml)) {
> +trace_e1000x_rx_oversized(size);
> +return queues;
> +}
> +
>  if (e1000x_is_vlan_packet(ehdr, core->mac[VET] & 0x) &&
>  !e1000x_rx_vlan_filter(core->mac, PKT_GET_VLAN_HDR(ehdr))) {
>  return queues;
> @@ -1067,7 +1082,10 @@ static uint16_t igb_receive_assign(IGBCore *core,
> const L2Header *l2_header,
>  queues &= core->mac[VFRE];
>  if (queues) {
>  for (i = 0; i < IGB_NUM_VM_POOLS; i++) {
> -if ((queues & BIT(i)) && igb_rx_is_oversized(core, i, size)) 
> {
> +lpe = !!(core->mac[VMOLR0 + i] & E1000_VMOLR_LPE);
> +rlpml = core->mac[VMOLR0 + i] & E1000_VMOLR_RLPML_MASK;
> +if ((queues & BIT(i)) &&
> +igb_rx_is_oversized(core, ehdr, size, lpe, rlpml))
> + {
>  oversized |= BIT(i);
>  }
>  }
> @@ -1609,11 +1627,6 @@ igb_receive_internal(IGBCore *core, const struct
> iovec *iov, int iovcnt,
>  iov_to_buf(iov, iovcnt, iov_ofs, _buf, 
> sizeof(min_buf.l2_header));
>  }
> 
> -/* Discard oversized packets if !LPE and !SBP. */
> -if (e1000x_is_oversized(core->mac, size)) {
> -return orig_size;
> -}
> -
>  net_rx_pkt_set_packet_type(core->rx_pkt,
> get_eth_packet_type(_buf.l2_header.eth));
>  net_rx_pkt_set_protocols(core->rx_pkt, iov, iovcnt, iov_ofs);
> --
> 2.40.0

[PATCH 30/40] igb: Implement igb-specific oversize check

2023-04-14 Thread Akihiko Odaki 
igb has a configurable size limit for LPE, and uses different limits
depending on whether the packet is treated as a VLAN packet.

Signed-off-by: Akihiko Odaki 
---
 hw/net/igb_core.c | 41 +++--
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 2013a9a53d..569897fb99 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -954,16 +954,21 @@ igb_rx_l4_cso_enabled(IGBCore *core)
 return !!(core->mac[RXCSUM] & E1000_RXCSUM_TUOFLD);
 }
 
-static bool
-igb_rx_is_oversized(IGBCore *core, uint16_t qn, size_t size)
+static bool igb_rx_is_oversized(IGBCore *core, const struct eth_header *ehdr,
+size_t size, bool lpe, uint16_t rlpml)
 {
-uint16_t pool = qn % IGB_NUM_VM_POOLS;
-bool lpe = !!(core->mac[VMOLR0 + pool] & E1000_VMOLR_LPE);
-int max_ethernet_lpe_size =
-core->mac[VMOLR0 + pool] & E1000_VMOLR_RLPML_MASK;
-int max_ethernet_vlan_size = 1522;
+size += 4;
+
+if (lpe) {
+return size > rlpml;
+}
+
+if (e1000x_is_vlan_packet(ehdr, core->mac[VET] & 0x) &&
+e1000x_vlan_rx_filter_enabled(core->mac)) {
+return size > 1522;
+}
 
-return size > (lpe ? max_ethernet_lpe_size : max_ethernet_vlan_size);
+return size > 1518;
 }
 
 static uint16_t igb_receive_assign(IGBCore *core, const L2Header *l2_header,
@@ -976,6 +981,8 @@ static uint16_t igb_receive_assign(IGBCore *core, const 
L2Header *l2_header,
 uint16_t queues = 0;
 uint16_t oversized = 0;
 uint16_t vid = be16_to_cpu(l2_header->vlan[0].h_tci) & VLAN_VID_MASK;
+bool lpe;
+uint16_t rlpml;
 int i;
 
 memset(rss_info, 0, sizeof(E1000E_RSSInfo));
@@ -984,6 +991,14 @@ static uint16_t igb_receive_assign(IGBCore *core, const 
L2Header *l2_header,
 *external_tx = true;
 }
 
+lpe = !!(core->mac[RCTL] & E1000_RCTL_LPE);
+rlpml = core->mac[RLPML];
+if (!(core->mac[RCTL] & E1000_RCTL_SBP) &&
+igb_rx_is_oversized(core, ehdr, size, lpe, rlpml)) {
+trace_e1000x_rx_oversized(size);
+return queues;
+}
+
 if (e1000x_is_vlan_packet(ehdr, core->mac[VET] & 0x) &&
 !e1000x_rx_vlan_filter(core->mac, PKT_GET_VLAN_HDR(ehdr))) {
 return queues;
@@ -1067,7 +1082,10 @@ static uint16_t igb_receive_assign(IGBCore *core, const 
L2Header *l2_header,
 queues &= core->mac[VFRE];
 if (queues) {
 for (i = 0; i < IGB_NUM_VM_POOLS; i++) {
-if ((queues & BIT(i)) && igb_rx_is_oversized(core, i, size)) {
+lpe = !!(core->mac[VMOLR0 + i] & E1000_VMOLR_LPE);
+rlpml = core->mac[VMOLR0 + i] & E1000_VMOLR_RLPML_MASK;
+if ((queues & BIT(i)) &&
+igb_rx_is_oversized(core, ehdr, size, lpe, rlpml)) {
 oversized |= BIT(i);
 }
 }
@@ -1609,11 +1627,6 @@ igb_receive_internal(IGBCore *core, const struct iovec 
*iov, int iovcnt,
 iov_to_buf(iov, iovcnt, iov_ofs, _buf, sizeof(min_buf.l2_header));
 }
 
-/* Discard oversized packets if !LPE and !SBP. */
-if (e1000x_is_oversized(core->mac, size)) {
-return orig_size;
-}
-
 net_rx_pkt_set_packet_type(core->rx_pkt,
get_eth_packet_type(_buf.l2_header.eth));
 net_rx_pkt_set_protocols(core->rx_pkt, iov, iovcnt, iov_ofs);
-- 
2.40.0