Re: [Qemu-arm] [PATCH v2 3/6] cadence_gem: Add support for screening

[Peter Maydell]

On 26 July 2016 at 01:12, Alistair Francis <address@hidden> wrote:
> The Cadence GEM hardware allows incoming data to be 'screened' based on some
> register values. Add support for these screens.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V2:
>  - Initial commit
>
>  hw/net/cadence_gem.c         | 151 
> +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/net/cadence_gem.h |   2 +
>  2 files changed, 153 insertions(+)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index deae122..d38bc1e 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -26,6 +26,7 @@
>  #include <zlib.h> /* For crc32 */
>
>  #include "hw/net/cadence_gem.h"
> +#include "qemu/log.h"
>  #include "net/checksum.h"
>
>  #ifdef CADENCE_GEM_ERR_DEBUG
> @@ -141,6 +142,37 @@
>  #define GEM_DESCONF6      (0x00000294/4)
>  #define GEM_DESCONF7      (0x00000298/4)
>
> +#define GEM_SCREENING_TYPE1_REGISTER_0  (0x00000500 / 4)
> +
> +#define GEM_ST1R_UDP_PORT_MATCH_ENABLE  (1 << 29)
> +#define GEM_ST1R_DSTC_ENABLE            (1 << 28)
> +#define GEM_ST1R_UDP_PORT_MATCH_SHIFT   (12)
> +#define GEM_ST1R_UDP_PORT_MATCH_WIDTH   (27 - GEM_ST1R_UDP_PORT_MATCH_SHIFT 
> + 1)
> +#define GEM_ST1R_DSTC_MATCH_SHIFT       (4)
> +#define GEM_ST1R_DSTC_MATCH_WIDTH       (11 - GEM_ST1R_DSTC_MATCH_SHIFT + 1)
> +#define GEM_ST1R_QUEUE_SHIFT            (0)
> +#define GEM_ST1R_QUEUE_WIDTH            (3 - GEM_ST1R_QUEUE_SHIFT + 1)
> +
> +#define GEM_SCREENING_TYPE2_REGISTER_0  (0x00000540 / 4)
> +
> +#define GEM_ST2R_COMPARE_A_ENABLE       (1 << 18)
> +#define GEM_ST2R_COMPARE_A_SHIFT        (13)
> +#define GEM_ST2R_COMPARE_WIDTH          (17 - GEM_ST2R_COMPARE_A_SHIFT + 1)
> +#define GEM_ST2R_ETHERTYPE_ENABLE       (1 << 12)
> +#define GEM_ST2R_ETHERTYPE_INDEX_SHIFT  (9)
> +#define GEM_ST2R_ETHERTYPE_INDEX_WIDTH  (11 - GEM_ST2R_ETHERTYPE_INDEX_SHIFT 
> \
> +                                            + 1)
> +#define GEM_ST2R_QUEUE_SHIFT            (0)
> +#define GEM_ST2R_QUEUE_WIDTH            (3 - GEM_ST2R_QUEUE_SHIFT + 1)
> +
> +#define GEM_SCREENING_TYPE2_ETHERTYPE_REG_0     (0x000006e0 / 4)
> +#define GEM_TYPE2_COMPARE_0_WORD_0              (0x00000700 / 4)
> +
> +#define GEM_T2CW1_COMPARE_OFFSET_SHIFT  (7)
> +#define GEM_T2CW1_COMPARE_OFFSET_WIDTH  (8 - GEM_T2CW1_COMPARE_OFFSET_SHIFT 
> + 1)
> +#define GEM_T2CW1_OFFSET_VALUE_SHIFT    (0)
> +#define GEM_T2CW1_OFFSET_VALUE_WIDTH    (6 - GEM_T2CW1_OFFSET_VALUE_SHIFT + 
> 1)
> +
>  /*****************************************/
>  #define GEM_NWCTRL_TXSTART     0x00000200 /* Transmit Enable */
>  #define GEM_NWCTRL_TXENA       0x00000008 /* Transmit Enable */
> @@ -601,6 +633,121 @@ static int gem_mac_address_filter(CadenceGEMState *s, 
> const uint8_t *packet)
>      return GEM_RX_REJECT;
>  }
>
> +/* Figure out which queue the recieved data should be sent to */

"received"

> +static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr)

Nothing seems to call this -- this probably results in a complaint
about an unused function if you build at this point in the series
(possibly only with optimisation on).

Do we need to also pass in the length of the rxbuf to avoid
reading beyond the end of short packets?

> +{
> +    uint32_t reg;
> +    bool matched, mismatched;
> +    int i, j;
> +
> +    for (i = 0; i < s->num_type1_screeners; i++) {
> +        reg = s->regs[GEM_SCREENING_TYPE1_REGISTER_0 + i];
> +        matched = false;
> +        mismatched = false;
> +
> +        /* Screening is based on UDP Port */
> +        if (reg & GEM_ST1R_UDP_PORT_MATCH_ENABLE) {
> +            uint16_t udp_port = rxbuf_ptr[14 + 22] << 8 | rxbuf_ptr[14 + 23];
> +            if (udp_port == extract32(reg, GEM_ST1R_UDP_PORT_MATCH_SHIFT,
> +                                           GEM_ST1R_UDP_PORT_MATCH_WIDTH)) {
> +                matched = true;
> +            } else {
> +                mismatched = true;
> +            }
> +        }
> +
> +        /* Screening is based on DS/TC */
> +        if (reg & GEM_ST1R_DSTC_ENABLE) {
> +            uint16_t dscp = rxbuf_ptr[14 + 1];

Why uint16_t if we're only reading one byte?

> +            if (dscp == extract32(reg, GEM_ST1R_DSTC_MATCH_SHIFT,
> +                                       GEM_ST1R_DSTC_MATCH_WIDTH)) {
> +                matched = true;
> +            } else {
> +                mismatched = true;
> +            }
> +        }
> +
> +        if (matched && !mismatched) {
> +            return extract32(reg, GEM_ST1R_QUEUE_SHIFT, 
> GEM_ST1R_QUEUE_WIDTH);
> +        }
> +    }
> +
> +    for (i = 0; i < s->num_type2_screeners; i++) {
> +        reg = s->regs[GEM_SCREENING_TYPE2_REGISTER_0 + i];
> +        matched = false;
> +        mismatched = false;
> +
> +        if (reg & GEM_ST2R_ETHERTYPE_ENABLE) {
> +            uint16_t type = rxbuf_ptr[12] << 8 | rxbuf_ptr[13];
> +            int et_idx = extract32(reg, GEM_ST2R_ETHERTYPE_INDEX_SHIFT,
> +                                        GEM_ST2R_ETHERTYPE_INDEX_WIDTH);
> +
> +            if (et_idx > s->num_type2_screeners) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "Out of range ethertype "
> +                              "register index: %d\n", et_idx);
> +            }
> +            if (type == s->regs[GEM_SCREENING_TYPE2_ETHERTYPE_REG_0 +
> +                                et_idx]) {
> +                matched = true;
> +            } else {
> +                mismatched = true;
> +            }
> +        }
> +
> +        /* Compare A, B, C */
> +        for (j = 0; j < 3; j++) {
> +            uint32_t cr0, cr1, mask;
> +            uint16_t rx_cmp;
> +            int offset;
> +            int cr_idx = extract32(reg, GEM_ST2R_COMPARE_A_SHIFT + j * 6,
> +                                        GEM_ST2R_COMPARE_WIDTH);
> +
> +            if (!(reg & (GEM_ST2R_COMPARE_A_ENABLE << (j * 6)))) {
> +                continue;
> +            }
> +            if (cr_idx > s->num_type2_screeners) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "Out of range compare "
> +                              "register index: %d\n", cr_idx);
> +            }
> +
> +            cr0 = s->regs[GEM_TYPE2_COMPARE_0_WORD_0 + cr_idx * 2];
> +            cr1 = s->regs[GEM_TYPE2_COMPARE_0_WORD_0 + cr_idx * 2 + 1];
> +            offset = extract32(cr1, GEM_T2CW1_OFFSET_VALUE_SHIFT,
> +                                    GEM_T2CW1_OFFSET_VALUE_WIDTH);
> +
> +            switch (extract32(cr1, GEM_T2CW1_COMPARE_OFFSET_SHIFT,
> +                                   GEM_T2CW1_COMPARE_OFFSET_WIDTH)) {

You pulled this out into 'offset' so you can just switch (offset).

> +            case (3): /* Skip UDP header */

You don't need brackets around the constants in case labels.

> +                qemu_log_mask(LOG_UNIMP, "TCP compare offsets"
> +                              "unimplemented - assuming UDP\n");
> +                offset += 8;
> +                /* Fallthrough */
> +            case (2): /* skip the IP header */
> +                offset += 20;
> +                /* Fallthrough */
> +            case (1): /* Count from after the ethertype */
> +                offset += 14;

'break' here would be a good idea.

What should happen for case 0? Guest error?

> +            }
> +
> +            rx_cmp = rxbuf_ptr[offset] << 8 | rxbuf_ptr[offset];
> +            mask = extract32(cr0, 0, 16);
> +
> +            if ((rx_cmp & mask) == (extract32(cr0, 16, 16) & mask)) {
> +                matched = true;
> +            } else {
> +                mismatched = true;
> +            }
> +        }
> +
> +        if (matched && !mismatched) {
> +            return extract32(reg, GEM_ST2R_QUEUE_SHIFT, 
> GEM_ST2R_QUEUE_WIDTH);
> +        }
> +    }
> +
> +    /* We made it here, assume it's queue 0 */
> +    return 0;
> +}
> +
>  static void gem_get_rx_desc(CadenceGEMState *s)
>  {
>      DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[0]);
> @@ -1263,6 +1410,10 @@ static Property gem_properties[] = {
>      DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
>      DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
>                        num_priority_queues, 1),
> +    DEFINE_PROP_UINT8("num-type1-screeners", CadenceGEMState,
> +                      num_type1_screeners, 4),
> +    DEFINE_PROP_UINT8("num-type2-screeners", CadenceGEMState,
> +                      num_type2_screeners, 4),

realize() should sanity check the properties aren't set too large.

>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
> index 77e0582..f2f78c0 100644
> --- a/include/hw/net/cadence_gem.h
> +++ b/include/hw/net/cadence_gem.h
> @@ -46,6 +46,8 @@ typedef struct CadenceGEMState {
>
>      /* Static properties */
>      uint8_t num_priority_queues;
> +    uint8_t num_type1_screeners;
> +    uint8_t num_type2_screeners;
>
>      /* GEM registers backing store */
>      uint32_t regs[CADENCE_GEM_MAXREG];
> --
> 2.7.4

thanks
-- PMM