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

[Sriram Yagnaraman]

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Wednesday, 1 February 2023 14:03
> 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/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.

Okay, that makes sense. Anyhow, I think it is better to implement DTXSWC.LLE 
bit to keep/remove source pool before implementing this change. Otherwise, we 
might end up sending packets back to the originating VF when VLAN filtering 
allows it.

> 
> >
> >>
> >>>            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"