Re: [PATCH v3 8/9] igb: respect VT_CTL ignore MAC field

[Akihiko Odaki]

On 2023/02/01 19:29, Sriram Yagnaraman wrote:
> > -----Original Message-----
> > From: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Sent: Wednesday, 1 February 2023 05:58
> > 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 v3 8/9] igb: respect VT_CTL ignore MAC field
> >
> > On 2023/01/31 18:42, Sriram Yagnaraman wrote:
> > > Also trace out a warning if replication mode is disabled, since we
> > > only support replication mode enabled.
> > >
> > > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> > > ---
> > >    hw/net/igb_core.c   | 9 +++++++++
> > >    hw/net/trace-events | 2 ++
> > >    2 files changed, 11 insertions(+)
> > >
> > > diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> > > c5f9c14f47..8115be2d76 100644
> > > --- a/hw/net/igb_core.c
> > > +++ b/hw/net/igb_core.c
> > > @@ -964,6 +964,10 @@ static uint16_t igb_receive_assign(IGBCore *core,
> > const struct eth_header *ehdr,
> > >        }
> > >
> > >        if (core->mac[MRQC] & 1) {
> > > +        if (!(core->mac[VT_CTL] & E1000_VT_CTL_VM_REPL_EN)) {
> > > +            trace_igb_rx_vmdq_replication_mode_disabled();
> > > +        }
> > > +
> > >            if (is_broadcast_ether_addr(ehdr->h_dest)) {
> > >                for (i = 0; i < IGB_NUM_VM_POOLS; i++) {
> > >                    if (core->mac[VMOLR0 + i] & E1000_VMOLR_BAM) { @@
> > > -1010,6 +1014,11 @@ static uint16_t igb_receive_assign(IGBCore *core,
> > const struct eth_header *ehdr,
> > >                }
> > >            }
> > >
> > > +        /* assume a full pool list if IGMAC is set */
> > > +        if (core->mac[VT_CTL] & E1000_VT_CTL_IGNORE_MAC) {
> > > +            queues = BIT(IGB_MAX_VF_FUNCTIONS) - 1;
> > > +        }
> > > +
> >
> > This overwrites "queues", but "external_tx" is not overwritten.
>
> Description in section 7.10.3.6 is a bit confusing, I interpreted that packet 
> is not sent to network if it matches an exact filter regardless of VT_CTL.IGMAC 
> setting.
> I think that VT_CTL.IGMAC setting is only for MAC filtering and pool selection, 
> and not to determine if a packet must go to external LAN or not.

It says nothing about VT_CTL.IGMAC so we need to make the best guess.

The rule saying "a unicast packet that matches an exact filter is not 
sent to the LAN" aligns with the common expectation of driver authors 
that a packet directed to a VF won't be sent to someone else.

However, when VT_CTL.IGMAC is set, the exact filter does not tell if the 
destination of the packet is a VF. In such a case, that rule does not do 
anything good, but can do some harm; if you have used igb for normal MAC 
routing and later switched to VLAN routing with VT_CTL.IGMAC, the exact 
filter may be left as is, the exact filter can prevent irrelevant 
packets from being routed to the external LAN.

> > >            if (e1000x_vlan_rx_filter_enabled(core->mac)) {
> > >                uint16_t mask = 0;
> > >
> > > diff --git a/hw/net/trace-events b/hw/net/trace-events index
> > > e94172e748..9bc7658692 100644
> > > --- a/hw/net/trace-events
> > > +++ b/hw/net/trace-events
> > > @@ -288,6 +288,8 @@ igb_rx_desc_buff_write(uint64_t addr, uint16_t
> > > offset, const void* source, uint3
> > >
> > >    igb_rx_metadata_rss(uint32_t rss) "RSS data: 0x%X"
> > >
> > > +igb_rx_vmdq_replication_mode_disabled(void) "WARN: Only replication
> > mode enabled is supported"
> > > +
> > >    igb_irq_icr_clear_gpie_nsicr(void) "Clearing ICR on read due to GPIE.NSICR
> > enabled"
> > >    igb_irq_icr_write(uint32_t bits, uint32_t old_icr, uint32_t new_icr)
> > "Clearing ICR bits 0x%x: 0x%x --> 0x%x"
> > >    igb_irq_set_iam(uint32_t icr) "Update IAM: 0x%x"