> -----Original Message-----
> From: Akihiko Odaki <akihiko.od...@daynix.com>
> Sent: Wednesday, 1 February 2023 14:03
> 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 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.od...@daynix.com>
> >> Sent: Wednesday, 1 February 2023 05:58
> >> 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 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.yagnara...@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"
