| Message ID | 20230128134633.22730-6-sriram.yagnaraman@est.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | igb: add missing feature set from | expand |
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)) {
> -----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)) {
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)) {
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)) {
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(-)