diff mbox series

[5/9] igb: respect E1000_VMOLR_RSSE

Message ID 20230128134633.22730-6-sriram.yagnaraman@est.tech
State New
Headers show
Series igb: add missing feature set from | expand

Commit Message

Sriram Yagnaraman Jan. 28, 2023, 1:46 p.m. UTC 
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(-)

Comments

Akihiko Odaki Jan. 29, 2023, 7:24 a.m. UTC | #1 
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)) {
Sriram Yagnaraman Jan. 30, 2023, 12:07 p.m. UTC | #2 
> -----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)) {
Akihiko Odaki Jan. 30, 2023, 1:20 p.m. UTC | #3 
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 mbox series

Patch

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)) {