qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alistair Francis
Subject: Re: [Qemu-devel] [PATCH v2 3/6] cadence_gem: Add support for screening
Date: Tue, 26 Jul 2016 09:42:05 -0700

On Tue, Jul 26, 2016 at 5:01 AM, Peter Maydell <address@hidden> wrote:
> 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"

Fixed.

>
>> +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).

There is a warning about this. I wasn't sure what the position on
warnings in between patch a series was. I can squash this into the
next patch, I was just worried that the patch was getting a little
big.

What do you think?

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

That is probably a good idea.

>
>> +{
>> +    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?

Good point, fixed.

>
>> +            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).

They are actually different.

>
>> +            case (3): /* Skip UDP header */
>
> You don't need brackets around the constants in case labels.

Fixed

>
>> +                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?

No change to offset. I added a case for it.

>
>> +            }
>> +
>> +            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.

Ok, adding it.

Thanks,

Alistair

>
>>      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
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]