RE: [PATCH 5/9] igb: respect E1000_VMOLR_RSSE

[Sriram Yagnaraman <sriram.yagnaraman@est.tech>]

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