| Message ID | 20230131094232.28863-9-sriram.yagnaraman@est.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | igb: merge changes from <20221229190817.25500-1-sriram.yagnaraman@est.tech> | expand |
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. > 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"
> -----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. > > > 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"
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"
> -----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"
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; + } + 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"
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(+)