Re: [PATCH v3] net/imx_fec: Adding support for MAC filtering in the FEC IP implementation.

[Mark Cave-Ayland]

On 10/12/2019 14:06, address@hidden wrote:

> From: bwasim <address@hidden>
> 
> This addition ensures that the IP does NOT boot up in promiscuous mode
> by default, and so the software only receives the desired
> packets(Unicast, Broadcast, Unicast / Multicast hashed) by default.
> The software running on-top of QEMU can also modify these settings and
> disable reception of broadcast frames or make the IP receive all packets 
> (PROM mode).
> This patch greatly reduces the number of packets received by the
> software running on-top of the QEMU model. Tested with the armv7-a SABRE_LITE 
> machine.
> Testing included running a custom OS with IPv4 / IPv6 support. Hashing
> and filtering of packets is tested to work well. Skeleton taken from
> the CADENCE_GEM IP and hash generation algorithm from the Linux Kernel.
> 
> Signed-off-by: Bilal Wasim <address@hidden>
> ---
>  hw/net/imx_fec.c         | 109 ++++++++++++++++++++++++++++++++++++++-
>  include/hw/net/imx_fec.h |  10 ++++
>  2 files changed, 118 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index bd99236864..d248f39fb0 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -419,6 +419,79 @@ static void imx_enet_write_bd(IMXENETBufDesc *bd, 
> dma_addr_t addr)
>      dma_memory_write(&address_space_memory, addr, bd, sizeof(*bd));
>  }
>  
> +/*
> + * Calculate a FEC MAC Address hash index
> + */
> +static unsigned calc_mac_hash(const uint8_t *mac, uint8_t mac_length)
> +{
> +    uint32_t crc = net_crc32_le(mac, mac_length);
> +
> +    /*
> +     * only upper 6 bits (FEC_HASH_BITS) are used
> +     * which point to specific bit in the hash registers
> +     */
> +    return (crc >> (32 - FEC_HASH_BITS)) & 0x3f;
> +}

Is it worth keeping this in a separate function? This appears to be a standard 
hash
calculation and most other cards simply inline it like below:

> +/*
> + * fec_mac_address_filter:
> + * Accept or reject this destination address?
> + */
> +static int fec_mac_address_filter(IMXFECState *s, const uint8_t *packet)
> +{
> +    const uint8_t broadcast_addr[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
> +    uint32_t addr1, addr2;
> +    uint8_t  hash;
> +
> +    /* Promiscuous mode? */
> +    if (s->regs[ENET_RCR] & ENET_RCR_PROM) {
> +        /* Accept all packets in promiscuous mode (even if bc_rej is set). */
> +        return FEC_RX_PROMISCUOUS_ACCEPT;
> +    }
> +
> +    /* Broadcast packet? */
> +    if (!memcmp(packet, broadcast_addr, 6)) {

ETH_ALEN?

> +        /* Reject broadcast packets? */
> +        if (s->regs[ENET_RCR] & ENET_RCR_BC_REJ) {
> +            return FEC_RX_REJECT;
> +        }
> +        /* Accept packets from broadcast address. */
> +        return FEC_RX_BROADCAST_ACCEPT;
> +    }
> +
> +    /* Accept packets -w- hash match? */
> +    hash = calc_mac_hash(packet, 6);

       hash = net_crc32_le(buf, ETH_ALEN) >> 26

> +    /* Accept packets -w- multicast hash match? */
> +    if ((packet[0] & 0x01) == 0x01) {
> +        /* Computed hash matches GAUR / GALR register ? */
> +        if (((hash < 32) && (s->regs[ENET_GALR] & (1 << hash)))
> +                || ((hash > 31) && (s->regs[ENET_GAUR] & (1 << (hash - 
> 32))))) {
> +            /* Accept multicast hash enabled address. */
> +            return FEC_RX_MULTICAST_HASH_ACCEPT;
> +        }
> +    } else {
> +        /* Computed hash matches IAUR / IALR register ? */
> +        if (((hash < 32) && (s->regs[ENET_IALR] & (1 << hash)))
> +                || ((hash > 31) && (s->regs[ENET_IAUR] & (1 << (hash - 
> 32))))) {
> +            /* Accept multicast hash enabled address. */
> +            return FEC_RX_UNICAST_HASH_ACCEPT;
> +        }
> +    }
> +
> +    /* Match Unicast address. */
> +    addr1  = g_htonl(s->regs[ENET_PALR]);
> +    addr2  = g_htonl(s->regs[ENET_PAUR]);
> +    if (!(memcmp(packet, (uint8_t *) &addr1, 4) ||
> +          memcmp(packet + 4, (uint8_t *) &addr2, 2))) {
> +        /* Accept packet because it matches my unicast address. */
> +        return FEC_RX_UNICAST_ACCEPT;
> +    }
> +
> +    /* Return -1 because we do NOT support MAC address filtering.. */
> +    return FEC_RX_REJECT;
> +}
> +
>  static void imx_eth_update(IMXFECState *s)
>  {
>      /*
> @@ -984,7 +1057,7 @@ static void imx_eth_write(void *opaque, hwaddr offset, 
> uint64_t value,
>      case ENET_IALR:
>      case ENET_GAUR:
>      case ENET_GALR:
> -        /* TODO: implement MAC hash filtering.  */
> +        s->regs[index] |= value;
>          break;
>      case ENET_TFWR:
>          if (s->is_fec) {
> @@ -1066,8 +1139,15 @@ static ssize_t imx_fec_receive(NetClientState *nc, 
> const uint8_t *buf,
>      uint32_t buf_addr;
>      uint8_t *crc_ptr;
>      unsigned int buf_len;
> +    int maf;
>      size_t size = len;
>  
> +    /* Is this destination MAC address "for us" ? */
> +    maf = fec_mac_address_filter(s, buf);
> +    if (maf == FEC_RX_REJECT) {
> +        return FEC_RX_REJECT;
> +    }
> +
>      FEC_PRINTF("len %d\n", (int)size);
>  
>      if (!s->regs[ENET_RDAR]) {
> @@ -1133,6 +1213,16 @@ static ssize_t imx_fec_receive(NetClientState *nc, 
> const uint8_t *buf,
>          } else {
>              s->regs[ENET_EIR] |= ENET_INT_RXB;
>          }
> +
> +        /* Update descriptor based on the "maf" flag. */
> +        if (maf == FEC_RX_BROADCAST_ACCEPT) {
> +            /* The packet is destined for the "broadcast" address. */
> +            bd.flags |= ENET_BD_BC;
> +        } else if (maf == FEC_RX_MULTICAST_HASH_ACCEPT) {
> +            /* The packet is destined for a "multicast" address. */
> +            bd.flags |= ENET_BD_MC;
> +        }
> +
>          imx_fec_write_bd(&bd, addr);
>          /* Advance to the next descriptor.  */
>          if ((bd.flags & ENET_BD_W) != 0) {
> @@ -1159,8 +1249,15 @@ static ssize_t imx_enet_receive(NetClientState *nc, 
> const uint8_t *buf,
>      uint8_t *crc_ptr;
>      unsigned int buf_len;
>      size_t size = len;
> +    int maf;
>      bool shift16 = s->regs[ENET_RACC] & ENET_RACC_SHIFT16;
>  
> +    /* Is this destination MAC address "for us" ? */
> +    maf = fec_mac_address_filter(s, buf);
> +    if (maf == FEC_RX_REJECT) {
> +        return FEC_RX_REJECT;
> +    }
> +
>      FEC_PRINTF("len %d\n", (int)size);
>  
>      if (!s->regs[ENET_RDAR]) {
> @@ -1254,6 +1351,16 @@ static ssize_t imx_enet_receive(NetClientState *nc, 
> const uint8_t *buf,
>                  s->regs[ENET_EIR] |= ENET_INT_RXB;
>              }
>          }
> +
> +        /* Update descriptor based on the "maf" flag. */
> +        if (maf == FEC_RX_BROADCAST_ACCEPT) {
> +            /* The packet is destined for the "broadcast" address. */
> +            bd.flags |= ENET_BD_BC;
> +        } else if (maf == FEC_RX_MULTICAST_HASH_ACCEPT) {
> +            /* The packet is destined for a "multicast" address. */
> +            bd.flags |= ENET_BD_MC;
> +        }
> +
>          imx_enet_write_bd(&bd, addr);
>          /* Advance to the next descriptor.  */
>          if ((bd.flags & ENET_BD_W) != 0) {
> diff --git a/include/hw/net/imx_fec.h b/include/hw/net/imx_fec.h
> index 7b3faa4019..f9cfcf6af5 100644
> --- a/include/hw/net/imx_fec.h
> +++ b/include/hw/net/imx_fec.h
> @@ -275,4 +275,14 @@ typedef struct IMXFECState {
>      uint8_t frame[ENET_MAX_FRAME_SIZE];
>  } IMXFECState;
>  
> +/* FEC address filtering defines. */
> +#define FEC_RX_REJECT                   (-1)
> +#define FEC_RX_PROMISCUOUS_ACCEPT       (-2)
> +#define FEC_RX_BROADCAST_ACCEPT         (-3)
> +#define FEC_RX_MULTICAST_HASH_ACCEPT    (-4)
> +#define FEC_RX_UNICAST_HASH_ACCEPT      (-5)
> +#define FEC_RX_UNICAST_ACCEPT           (-6)
> +
> +#define FEC_HASH_BITS                    6    /* #bits in hash */

If you inline the hash calculation as above then this line is no longer 
required.

>  #endif

I've done some bits on QEMU networking however I'm not familiar with this 
particular
device which limits me somewhat for further review.


ATB,

Mark.