> -----Original Message-----
> From: Akihiko Odaki <akihiko.od...@daynix.com>
> Sent: Sunday, 29 January 2023 08:25
> To: Sriram Yagnaraman <sriram.yagnara...@est.tech>
> Cc: qemu-devel@nongnu.org; Jason Wang <jasow...@redhat.com>; Dmitry
> Fleytman <dmitry.fleyt...@gmail.com>; Michael S . Tsirkin
> <m...@redhat.com>; Marcel Apfelbaum <marcel.apfelb...@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.yagnara...@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)) {
