hw/net/cadence_gem.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
This was one of my first attempts, and so I was sure to miss something.. I've incorporated all the updates in this patch.. Let me know what you think about this..
net/cadence_gem: Updating the GEM MAC IP to properly filter out the multicast addresses.
The current code makes a bad assumption that the most-significant byte
of the MAC address is used to determine if the address is multicast or
unicast, but in reality only a single bit is used to determine this.
This caused IPv6 to not work.. Fix is now in place and has been tested
with ZCU102-A53 / IPv6 on a TAP interface. Works well..
Signed-off-by: Bilal Wasim <bilal_wasim@mentor.com>
---
hw/net/cadence_gem.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
index b8be73dc55..98efb93f8a 100644
--- a/hw/net/cadence_gem.c
+++ b/hw/net/cadence_gem.c
@@ -34,6 +34,7 @@
#include "qemu/module.h"
#include "sysemu/dma.h"
#include "net/checksum.h"
+#include "net/eth.h"
#ifdef CADENCE_GEM_ERR_DEBUG
#define DB_PRINT(...) do { \
@@ -315,6 +316,12 @@
#define GEM_MODID_VALUE 0x00020118
+/* IEEE has specified that the most significant bit of the most significant byte be used for
+ * distinguishing between Unicast and Multicast addresses.
+ * If its a 1, that means multicast, 0 means unicast. */
+#define IS_MULTICAST(address) is_multicast_ether_addr(address)
+#define IS_UNICAST(address) is_unicast_ether_addr(address)
+
static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc)
{
uint64_t ret = desc[0];
@@ -601,7 +608,7 @@ static void gem_receive_updatestats(CadenceGEMState *s, const uint8_t *packet,
}
/* Error-free Multicast Frames counter */
- if (packet[0] == 0x01) {
+ if (IS_MULTICAST(packet)) {
s->regs[GEM_RXMULTICNT]++;
}
@@ -690,21 +697,21 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
}
/* Accept packets -w- hash match? */
- if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
- (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
+ if ((IS_MULTICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
+ (IS_UNICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
unsigned hash_index;
hash_index = calc_mac_hash(packet);
if (hash_index < 32) {
if (s->regs[GEM_HASHLO] & (1<<hash_index)) {
- return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
- GEM_RX_UNICAST_HASH_ACCEPT;
+ return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT :
+ GEM_RX_UNICAST_HASH_ACCEPT;
}
} else {
hash_index -= 32;
if (s->regs[GEM_HASHHI] & (1<<hash_index)) {
- return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
- GEM_RX_UNICAST_HASH_ACCEPT;
+ return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT :
+ GEM_RX_UNICAST_HASH_ACCEPT;
}
}
}
--
2.19.1.windows.1
------------------------------------------------------------------------------------------------------------------------------
-----Original Message-----
From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com]
Sent: Thursday, November 28, 2019 9:00 PM
To: Wasim, Bilal <Bilal_Wasim@mentor.com>
Cc: qemu-devel@nongnu.org; alistair@alistair23.me; peter.maydell@linaro.org; qemu-arm@nongnu.org
Subject: Re: [PATCH] Updating the GEM MAC IP to properly filter out the multicast addresses
On Thu, Nov 28, 2019 at 03:10:16PM +0000, Wasim, Bilal wrote:
> [PATCH] Updating the GEM MAC IP to properly filter out the multicast
> addresses. The current code makes a bad assumption that the
> most-significant byte of the MAC address is used to determine if the
> address is multicast or unicast, but in reality only a single bit is
> used to determine this. This caused IPv6 to not work.. Fix is now in
> place and has been tested with
> ZCU102-A53 / IPv6 on a TAP interface. Works well..
Hi Bilal,
The fix looks right to me but I have a few comments.
* Your patch seems a little wrongly formated.
[PATCH] goes into the Subject line for example and you're missing path prefixes.
Do a git log -- hw/net/cadence_gem.c to see examples on how it should look.
* The patch will probably not pass checkpatch since you seem to have long lines.
* We also need to update gem_receive_updatestats() to use the corrected macros.
More inline:
>
> Signed-off-by: Bilal Wasim <bilal_wasim@mentor.com>
> ---
> hw/net/cadence_gem.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index
> b8be73d..f8bcbb3 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -315,6 +315,12 @@
> #define GEM_MODID_VALUE 0x00020118
> +/* IEEE has specified that the most significant bit of the most
> +significant byte be used for
> + * distinguishing between Unicast and Multicast addresses.
> + * If its a 1, that means multicast, 0 means unicast. */
> +#define IS_MULTICAST(address) (((address[0] & 0x01) == 0x01) ? 1 : 0)
This can be simplified:
#define IS_MULTICAST(address) (address[0] & 1)
Actually, looking closer, we already have functions to do these checks in:
include/net/eth.h
static inline int is_multicast_ether_addr(const uint8_t *addr) static inline int is_broadcast_ether_addr(const uint8_t *addr) static inline int is_unicast_ether_addr(const uint8_t *addr)
> +#define IS_UNICAST(address) (!IS_MULTICAST(address))
> +
> static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t
> *desc) {
> uint64_t ret = desc[0];
> @@ -690,21 +696,21 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
> }
> /* Accept packets -w- hash match? */
> - if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
> - (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
> + if ((IS_MULTICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) ||
> + (IS_UNICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) {
> unsigned hash_index;
> hash_index = calc_mac_hash(packet);
> if (hash_index < 32) {
> if (s->regs[GEM_HASHLO] & (1<<hash_index)) {
> - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
> - GEM_RX_UNICAST_HASH_ACCEPT;
> + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT :
> +
> + GEM_RX_UNICAST_HASH_ACCEPT;
> }
> } else {
> hash_index -= 32;
> if (s->regs[GEM_HASHHI] & (1<<hash_index)) {
> - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT :
> - GEM_RX_UNICAST_HASH_ACCEPT;
> + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT :
> +
> + GEM_RX_UNICAST_HASH_ACCEPT;
> }
> }
> }
> --
> 2.9.3
>
On Thu, Nov 28, 2019 at 05:02:00PM +0000, Wasim, Bilal wrote: > This was one of my first attempts, and so I was sure to miss something.. I've incorporated all the updates in this patch.. Let me know what you think about this.. > > net/cadence_gem: Updating the GEM MAC IP to properly filter out the multicast addresses. > > The current code makes a bad assumption that the most-significant byte > of the MAC address is used to determine if the address is multicast or > unicast, but in reality only a single bit is used to determine this. > This caused IPv6 to not work.. Fix is now in place and has been tested > with ZCU102-A53 / IPv6 on a TAP interface. Works well.. Thanks Bilal, This looks better but not quite there yet. * You don't seem to be using git-send-email to post patches, try it, it will make life easier wrt to formatting. The patch should be in a separate email. The subject line should be the subject of the email. git-format-patch and git-send-email will take care of that for you. * You don't need to define IS_MULTICAST, you can directly use is_multicast_ether_addr() and friends. * The patch still has long lines (longer than 80 chars) You can run scripts/checkpatch.pl on your commit before posting the patch. Cheers, Edgar > > Signed-off-by: Bilal Wasim <bilal_wasim@mentor.com> > --- > hw/net/cadence_gem.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c > index b8be73dc55..98efb93f8a 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -34,6 +34,7 @@ > #include "qemu/module.h" > #include "sysemu/dma.h" > #include "net/checksum.h" > +#include "net/eth.h" > > #ifdef CADENCE_GEM_ERR_DEBUG > #define DB_PRINT(...) do { \ > @@ -315,6 +316,12 @@ > > #define GEM_MODID_VALUE 0x00020118 > > +/* IEEE has specified that the most significant bit of the most significant byte be used for > + * distinguishing between Unicast and Multicast addresses. > + * If its a 1, that means multicast, 0 means unicast. */ > +#define IS_MULTICAST(address) is_multicast_ether_addr(address) > +#define IS_UNICAST(address) is_unicast_ether_addr(address) > + > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc) > { > uint64_t ret = desc[0]; > @@ -601,7 +608,7 @@ static void gem_receive_updatestats(CadenceGEMState *s, const uint8_t *packet, > } > > /* Error-free Multicast Frames counter */ > - if (packet[0] == 0x01) { > + if (IS_MULTICAST(packet)) { > s->regs[GEM_RXMULTICNT]++; > } > > @@ -690,21 +697,21 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet) > } > > /* Accept packets -w- hash match? */ > - if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > - (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > + if ((IS_MULTICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > + (IS_UNICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > unsigned hash_index; > > hash_index = calc_mac_hash(packet); > if (hash_index < 32) { > if (s->regs[GEM_HASHLO] & (1<<hash_index)) { > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > - GEM_RX_UNICAST_HASH_ACCEPT; > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > + GEM_RX_UNICAST_HASH_ACCEPT; > } > } else { > hash_index -= 32; > if (s->regs[GEM_HASHHI] & (1<<hash_index)) { > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > - GEM_RX_UNICAST_HASH_ACCEPT; > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > + GEM_RX_UNICAST_HASH_ACCEPT; > } > } > } > -- > 2.19.1.windows.1 > > ------------------------------------------------------------------------------------------------------------------------------ > -----Original Message----- > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] > Sent: Thursday, November 28, 2019 9:00 PM > To: Wasim, Bilal <Bilal_Wasim@mentor.com> > Cc: qemu-devel@nongnu.org; alistair@alistair23.me; peter.maydell@linaro.org; qemu-arm@nongnu.org > Subject: Re: [PATCH] Updating the GEM MAC IP to properly filter out the multicast addresses > > On Thu, Nov 28, 2019 at 03:10:16PM +0000, Wasim, Bilal wrote: > > [PATCH] Updating the GEM MAC IP to properly filter out the multicast > > addresses. The current code makes a bad assumption that the > > most-significant byte of the MAC address is used to determine if the > > address is multicast or unicast, but in reality only a single bit is > > used to determine this. This caused IPv6 to not work.. Fix is now in > > place and has been tested with > > ZCU102-A53 / IPv6 on a TAP interface. Works well.. > > Hi Bilal, > > The fix looks right to me but I have a few comments. > > * Your patch seems a little wrongly formated. > [PATCH] goes into the Subject line for example and you're missing path prefixes. > > Do a git log -- hw/net/cadence_gem.c to see examples on how it should look. > > * The patch will probably not pass checkpatch since you seem to have long lines. > > * We also need to update gem_receive_updatestats() to use the corrected macros. > > More inline: > > > > > Signed-off-by: Bilal Wasim <bilal_wasim@mentor.com> > > --- > > hw/net/cadence_gem.c | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index > > b8be73d..f8bcbb3 100644 > > --- a/hw/net/cadence_gem.c > > +++ b/hw/net/cadence_gem.c > > @@ -315,6 +315,12 @@ > > #define GEM_MODID_VALUE 0x00020118 > > +/* IEEE has specified that the most significant bit of the most > > +significant byte be used for > > + * distinguishing between Unicast and Multicast addresses. > > + * If its a 1, that means multicast, 0 means unicast. */ > > +#define IS_MULTICAST(address) (((address[0] & 0x01) == 0x01) ? 1 : 0) > > > > This can be simplified: > #define IS_MULTICAST(address) (address[0] & 1) > > Actually, looking closer, we already have functions to do these checks in: > include/net/eth.h > > static inline int is_multicast_ether_addr(const uint8_t *addr) static inline int is_broadcast_ether_addr(const uint8_t *addr) static inline int is_unicast_ether_addr(const uint8_t *addr) > > > > > +#define IS_UNICAST(address) (!IS_MULTICAST(address)) > > + > > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t > > *desc) { > > uint64_t ret = desc[0]; > > @@ -690,21 +696,21 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet) > > } > > /* Accept packets -w- hash match? */ > > - if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > > - (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > > + if ((IS_MULTICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > > + (IS_UNICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > > unsigned hash_index; > > hash_index = calc_mac_hash(packet); > > if (hash_index < 32) { > > if (s->regs[GEM_HASHLO] & (1<<hash_index)) { > > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > > - GEM_RX_UNICAST_HASH_ACCEPT; > > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > > + > > + GEM_RX_UNICAST_HASH_ACCEPT; > > } > > } else { > > hash_index -= 32; > > if (s->regs[GEM_HASHHI] & (1<<hash_index)) { > > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > > - GEM_RX_UNICAST_HASH_ACCEPT; > > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > > + > > + GEM_RX_UNICAST_HASH_ACCEPT; > > } > > } > > } > > -- > > 2.9.3 > >
Thanks for the pointers.. I will incorporate all these changes and post an updated thread asap.. -----Original Message----- From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] Sent: Thursday, November 28, 2019 10:32 PM To: Wasim, Bilal <Bilal_Wasim@mentor.com> Cc: qemu-devel@nongnu.org; alistair@alistair23.me; peter.maydell@linaro.org; qemu-arm@nongnu.org Subject: Re: [PATCH] Updating the GEM MAC IP to properly filter out the multicast addresses On Thu, Nov 28, 2019 at 05:02:00PM +0000, Wasim, Bilal wrote: > This was one of my first attempts, and so I was sure to miss something.. I've incorporated all the updates in this patch.. Let me know what you think about this.. > > net/cadence_gem: Updating the GEM MAC IP to properly filter out the multicast addresses. > > The current code makes a bad assumption that the most-significant byte > of the MAC address is used to determine if the address is multicast or > unicast, but in reality only a single bit is used to determine this. > This caused IPv6 to not work.. Fix is now in place and has been tested > with ZCU102-A53 / IPv6 on a TAP interface. Works well.. Thanks Bilal, This looks better but not quite there yet. * You don't seem to be using git-send-email to post patches, try it, it will make life easier wrt to formatting. The patch should be in a separate email. The subject line should be the subject of the email. git-format-patch and git-send-email will take care of that for you. * You don't need to define IS_MULTICAST, you can directly use is_multicast_ether_addr() and friends. * The patch still has long lines (longer than 80 chars) You can run scripts/checkpatch.pl on your commit before posting the patch. Cheers, Edgar > > Signed-off-by: Bilal Wasim <bilal_wasim@mentor.com> > --- > hw/net/cadence_gem.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index > b8be73dc55..98efb93f8a 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -34,6 +34,7 @@ > #include "qemu/module.h" > #include "sysemu/dma.h" > #include "net/checksum.h" > +#include "net/eth.h" > > #ifdef CADENCE_GEM_ERR_DEBUG > #define DB_PRINT(...) do { \ > @@ -315,6 +316,12 @@ > > #define GEM_MODID_VALUE 0x00020118 > > +/* IEEE has specified that the most significant bit of the most > +significant byte be used for > + * distinguishing between Unicast and Multicast addresses. > + * If its a 1, that means multicast, 0 means unicast. */ > +#define IS_MULTICAST(address) is_multicast_ether_addr(address) > +#define IS_UNICAST(address) is_unicast_ether_addr(address) > + > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, > uint32_t *desc) { > uint64_t ret = desc[0]; > @@ -601,7 +608,7 @@ static void gem_receive_updatestats(CadenceGEMState *s, const uint8_t *packet, > } > > /* Error-free Multicast Frames counter */ > - if (packet[0] == 0x01) { > + if (IS_MULTICAST(packet)) { > s->regs[GEM_RXMULTICNT]++; > } > > @@ -690,21 +697,21 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet) > } > > /* Accept packets -w- hash match? */ > - if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > - (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > + if ((IS_MULTICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > + (IS_UNICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > unsigned hash_index; > > hash_index = calc_mac_hash(packet); > if (hash_index < 32) { > if (s->regs[GEM_HASHLO] & (1<<hash_index)) { > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > - GEM_RX_UNICAST_HASH_ACCEPT; > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > + > + GEM_RX_UNICAST_HASH_ACCEPT; > } > } else { > hash_index -= 32; > if (s->regs[GEM_HASHHI] & (1<<hash_index)) { > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > - GEM_RX_UNICAST_HASH_ACCEPT; > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > + > + GEM_RX_UNICAST_HASH_ACCEPT; > } > } > } > -- > 2.19.1.windows.1 > > ---------------------------------------------------------------------- > -------------------------------------------------------- > -----Original Message----- > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] > Sent: Thursday, November 28, 2019 9:00 PM > To: Wasim, Bilal <Bilal_Wasim@mentor.com> > Cc: qemu-devel@nongnu.org; alistair@alistair23.me; > peter.maydell@linaro.org; qemu-arm@nongnu.org > Subject: Re: [PATCH] Updating the GEM MAC IP to properly filter out > the multicast addresses > > On Thu, Nov 28, 2019 at 03:10:16PM +0000, Wasim, Bilal wrote: > > [PATCH] Updating the GEM MAC IP to properly filter out the multicast > > addresses. The current code makes a bad assumption that the > > most-significant byte of the MAC address is used to determine if the > > address is multicast or unicast, but in reality only a single bit is > > used to determine this. This caused IPv6 to not work.. Fix is now in > > place and has been tested with > > ZCU102-A53 / IPv6 on a TAP interface. Works well.. > > Hi Bilal, > > The fix looks right to me but I have a few comments. > > * Your patch seems a little wrongly formated. > [PATCH] goes into the Subject line for example and you're missing path prefixes. > > Do a git log -- hw/net/cadence_gem.c to see examples on how it should look. > > * The patch will probably not pass checkpatch since you seem to have long lines. > > * We also need to update gem_receive_updatestats() to use the corrected macros. > > More inline: > > > > > Signed-off-by: Bilal Wasim <bilal_wasim@mentor.com> > > --- > > hw/net/cadence_gem.c | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index > > b8be73d..f8bcbb3 100644 > > --- a/hw/net/cadence_gem.c > > +++ b/hw/net/cadence_gem.c > > @@ -315,6 +315,12 @@ > > #define GEM_MODID_VALUE 0x00020118 > > +/* IEEE has specified that the most significant bit of the most > > +significant byte be used for > > + * distinguishing between Unicast and Multicast addresses. > > + * If its a 1, that means multicast, 0 means unicast. */ > > +#define IS_MULTICAST(address) (((address[0] & 0x01) == 0x01) ? 1 : 0) > > > > This can be simplified: > #define IS_MULTICAST(address) (address[0] & 1) > > Actually, looking closer, we already have functions to do these checks in: > include/net/eth.h > > static inline int is_multicast_ether_addr(const uint8_t *addr) static > inline int is_broadcast_ether_addr(const uint8_t *addr) static inline > int is_unicast_ether_addr(const uint8_t *addr) > > > > > +#define IS_UNICAST(address) (!IS_MULTICAST(address)) > > + > > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, > > uint32_t > > *desc) { > > uint64_t ret = desc[0]; > > @@ -690,21 +696,21 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet) > > } > > /* Accept packets -w- hash match? */ > > - if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > > - (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > > + if ((IS_MULTICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > > + (IS_UNICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > > unsigned hash_index; > > hash_index = calc_mac_hash(packet); > > if (hash_index < 32) { > > if (s->regs[GEM_HASHLO] & (1<<hash_index)) { > > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > > - GEM_RX_UNICAST_HASH_ACCEPT; > > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > > + > > + GEM_RX_UNICAST_HASH_ACCEPT; > > } > > } else { > > hash_index -= 32; > > if (s->regs[GEM_HASHHI] & (1<<hash_index)) { > > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > > - GEM_RX_UNICAST_HASH_ACCEPT; > > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > > + > > + GEM_RX_UNICAST_HASH_ACCEPT; > > } > > } > > } > > -- > > 2.9.3 > >
git "send-email" doesn't really work well with my work account because the server considers it "less secure" and does NOT provide a way around it.. I've had to re-submit the patch from my secondary email... -----Original Message----- From: Qemu-arm [mailto:qemu-arm-bounces+bilal_wasim=mentor.com@nongnu.org] On Behalf Of Wasim, Bilal Sent: Thursday, November 28, 2019 10:35 PM To: Edgar E. Iglesias <edgar.iglesias@gmail.com> Cc: peter.maydell@linaro.org; alistair@alistair23.me; qemu-devel@nongnu.org; qemu-arm@nongnu.org Subject: RE: [PATCH] Updating the GEM MAC IP to properly filter out the multicast addresses Thanks for the pointers.. I will incorporate all these changes and post an updated thread asap.. -----Original Message----- From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] Sent: Thursday, November 28, 2019 10:32 PM To: Wasim, Bilal <Bilal_Wasim@mentor.com> Cc: qemu-devel@nongnu.org; alistair@alistair23.me; peter.maydell@linaro.org; qemu-arm@nongnu.org Subject: Re: [PATCH] Updating the GEM MAC IP to properly filter out the multicast addresses On Thu, Nov 28, 2019 at 05:02:00PM +0000, Wasim, Bilal wrote: > This was one of my first attempts, and so I was sure to miss something.. I've incorporated all the updates in this patch.. Let me know what you think about this.. > > net/cadence_gem: Updating the GEM MAC IP to properly filter out the multicast addresses. > > The current code makes a bad assumption that the most-significant byte > of the MAC address is used to determine if the address is multicast or > unicast, but in reality only a single bit is used to determine this. > This caused IPv6 to not work.. Fix is now in place and has been tested > with ZCU102-A53 / IPv6 on a TAP interface. Works well.. Thanks Bilal, This looks better but not quite there yet. * You don't seem to be using git-send-email to post patches, try it, it will make life easier wrt to formatting. The patch should be in a separate email. The subject line should be the subject of the email. git-format-patch and git-send-email will take care of that for you. * You don't need to define IS_MULTICAST, you can directly use is_multicast_ether_addr() and friends. * The patch still has long lines (longer than 80 chars) You can run scripts/checkpatch.pl on your commit before posting the patch. Cheers, Edgar > > Signed-off-by: Bilal Wasim <bilal_wasim@mentor.com> > --- > hw/net/cadence_gem.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index > b8be73dc55..98efb93f8a 100644 > --- a/hw/net/cadence_gem.c > +++ b/hw/net/cadence_gem.c > @@ -34,6 +34,7 @@ > #include "qemu/module.h" > #include "sysemu/dma.h" > #include "net/checksum.h" > +#include "net/eth.h" > > #ifdef CADENCE_GEM_ERR_DEBUG > #define DB_PRINT(...) do { \ > @@ -315,6 +316,12 @@ > > #define GEM_MODID_VALUE 0x00020118 > > +/* IEEE has specified that the most significant bit of the most > +significant byte be used for > + * distinguishing between Unicast and Multicast addresses. > + * If its a 1, that means multicast, 0 means unicast. */ > +#define IS_MULTICAST(address) is_multicast_ether_addr(address) > +#define IS_UNICAST(address) is_unicast_ether_addr(address) > + > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, > uint32_t *desc) { > uint64_t ret = desc[0]; > @@ -601,7 +608,7 @@ static void gem_receive_updatestats(CadenceGEMState *s, const uint8_t *packet, > } > > /* Error-free Multicast Frames counter */ > - if (packet[0] == 0x01) { > + if (IS_MULTICAST(packet)) { > s->regs[GEM_RXMULTICNT]++; > } > > @@ -690,21 +697,21 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet) > } > > /* Accept packets -w- hash match? */ > - if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > - (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > + if ((IS_MULTICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > + (IS_UNICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > unsigned hash_index; > > hash_index = calc_mac_hash(packet); > if (hash_index < 32) { > if (s->regs[GEM_HASHLO] & (1<<hash_index)) { > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > - GEM_RX_UNICAST_HASH_ACCEPT; > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > + > + GEM_RX_UNICAST_HASH_ACCEPT; > } > } else { > hash_index -= 32; > if (s->regs[GEM_HASHHI] & (1<<hash_index)) { > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > - GEM_RX_UNICAST_HASH_ACCEPT; > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > + > + GEM_RX_UNICAST_HASH_ACCEPT; > } > } > } > -- > 2.19.1.windows.1 > > ---------------------------------------------------------------------- > -------------------------------------------------------- > -----Original Message----- > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] > Sent: Thursday, November 28, 2019 9:00 PM > To: Wasim, Bilal <Bilal_Wasim@mentor.com> > Cc: qemu-devel@nongnu.org; alistair@alistair23.me; > peter.maydell@linaro.org; qemu-arm@nongnu.org > Subject: Re: [PATCH] Updating the GEM MAC IP to properly filter out > the multicast addresses > > On Thu, Nov 28, 2019 at 03:10:16PM +0000, Wasim, Bilal wrote: > > [PATCH] Updating the GEM MAC IP to properly filter out the multicast > > addresses. The current code makes a bad assumption that the > > most-significant byte of the MAC address is used to determine if the > > address is multicast or unicast, but in reality only a single bit is > > used to determine this. This caused IPv6 to not work.. Fix is now in > > place and has been tested with > > ZCU102-A53 / IPv6 on a TAP interface. Works well.. > > Hi Bilal, > > The fix looks right to me but I have a few comments. > > * Your patch seems a little wrongly formated. > [PATCH] goes into the Subject line for example and you're missing path prefixes. > > Do a git log -- hw/net/cadence_gem.c to see examples on how it should look. > > * The patch will probably not pass checkpatch since you seem to have long lines. > > * We also need to update gem_receive_updatestats() to use the corrected macros. > > More inline: > > > > > Signed-off-by: Bilal Wasim <bilal_wasim@mentor.com> > > --- > > hw/net/cadence_gem.c | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index > > b8be73d..f8bcbb3 100644 > > --- a/hw/net/cadence_gem.c > > +++ b/hw/net/cadence_gem.c > > @@ -315,6 +315,12 @@ > > #define GEM_MODID_VALUE 0x00020118 > > +/* IEEE has specified that the most significant bit of the most > > +significant byte be used for > > + * distinguishing between Unicast and Multicast addresses. > > + * If its a 1, that means multicast, 0 means unicast. */ > > +#define IS_MULTICAST(address) (((address[0] & 0x01) == 0x01) ? 1 : 0) > > > > This can be simplified: > #define IS_MULTICAST(address) (address[0] & 1) > > Actually, looking closer, we already have functions to do these checks in: > include/net/eth.h > > static inline int is_multicast_ether_addr(const uint8_t *addr) static > inline int is_broadcast_ether_addr(const uint8_t *addr) static inline > int is_unicast_ether_addr(const uint8_t *addr) > > > > > +#define IS_UNICAST(address) (!IS_MULTICAST(address)) > > + > > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, > > uint32_t > > *desc) { > > uint64_t ret = desc[0]; > > @@ -690,21 +696,21 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet) > > } > > /* Accept packets -w- hash match? */ > > - if ((packet[0] == 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > > - (packet[0] != 0x01 && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > > + if ((IS_MULTICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_MCAST_HASH)) || > > + (IS_UNICAST(packet) && (s->regs[GEM_NWCFG] & GEM_NWCFG_UCAST_HASH))) { > > unsigned hash_index; > > hash_index = calc_mac_hash(packet); > > if (hash_index < 32) { > > if (s->regs[GEM_HASHLO] & (1<<hash_index)) { > > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > > - GEM_RX_UNICAST_HASH_ACCEPT; > > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > > + > > + GEM_RX_UNICAST_HASH_ACCEPT; > > } > > } else { > > hash_index -= 32; > > if (s->regs[GEM_HASHHI] & (1<<hash_index)) { > > - return packet[0] == 0x01 ? GEM_RX_MULTICAST_HASH_ACCEPT : > > - GEM_RX_UNICAST_HASH_ACCEPT; > > + return IS_MULTICAST(packet) ? GEM_RX_MULTICAST_HASH_ACCEPT : > > + > > + GEM_RX_UNICAST_HASH_ACCEPT; > > } > > } > > } > > -- > > 2.9.3 > >
Patchew URL: https://patchew.org/QEMU/2b7a42487a5d4a258062bd209a0d13fa@SVR-IES-MBX-03.mgc.mentorg.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: RE: [PATCH] Updating the GEM MAC IP to properly filter out the multicast addresses Type: series Message-id: 2b7a42487a5d4a258062bd209a0d13fa@SVR-IES-MBX-03.mgc.mentorg.com === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Switched to a new branch 'test' da627f2 Updating the GEM MAC IP to properly filter out the multicast addresses === OUTPUT BEGIN === ERROR: line over 90 characters #36: FILE: hw/net/cadence_gem.c:319: +/* IEEE has specified that the most significant bit of the most significant byte be used for WARNING: Block comments use a leading /* on a separate line #36: FILE: hw/net/cadence_gem.c:319: +/* IEEE has specified that the most significant bit of the most significant byte be used for WARNING: Block comments use a trailing */ on a separate line #38: FILE: hw/net/cadence_gem.c:321: + * If its a 1, that means multicast, 0 means unicast. */ total: 1 errors, 2 warnings, 54 lines checked Commit da627f2a2cb5 (Updating the GEM MAC IP to properly filter out the multicast addresses) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/2b7a42487a5d4a258062bd209a0d13fa@SVR-IES-MBX-03.mgc.mentorg.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
© 2016 - 2024 Red Hat, Inc.