RE: [PATCH v3 8/9] igb: respect VT_CTL ignore MAC field
> -Original Message-
> From: Akihiko Odaki
> Sent: Wednesday, 1 February 2023 14:03
> To: Sriram Yagnaraman
> Cc: qemu-devel@nongnu.org; Jason Wang ; Dmitry
> Fleytman ; Michael S . Tsirkin
> ; Marcel Apfelbaum
> 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
> >> Sent: Wednesday, 1 February 2023 05:58
> >> To: Sriram Yagnaraman
> >> Cc: qemu-devel@nongnu.org; Jason Wang ;
> Dmitry
> >> Fleytman ; Michael S . Tsirkin
> >> ; Marcel Apfelbaum
> >> 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
> >>> ---
> >>>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"
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
Sent: Wednesday, 1 February 2023 05:58
To: Sriram Yagnaraman
Cc: qemu-devel@nongnu.org; Jason Wang ; Dmitry
Fleytman ; Michael S . Tsirkin
; Marcel Apfelbaum
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
---
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"
RE: [PATCH v3 8/9] igb: respect VT_CTL ignore MAC field
> -Original Message-
> From: Akihiko Odaki
> Sent: Wednesday, 1 February 2023 05:58
> To: Sriram Yagnaraman
> Cc: qemu-devel@nongnu.org; Jason Wang ; Dmitry
> Fleytman ; Michael S . Tsirkin
> ; Marcel Apfelbaum
> 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
> > ---
> > 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"
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
---
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"
[PATCH v3 8/9] igb: respect VT_CTL ignore MAC field
Also trace out a warning if replication mode is disabled, since we only
support replication mode enabled.
Signed-off-by: Sriram Yagnaraman
---
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;
+}
+
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"
--
2.34.1
