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

[Akihiko Odaki]

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