[v1,3/5] cadence_gem: Add queue support

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

There is a indentation error in this patch in the gem_transmit function.
I have written it like that to make it easier to see the changes. It is
fixed in the next patch.

 hw/net/cadence_gem.c         | 157 ++++++++++++++++++++++++++++++++-----------
 include/hw/net/cadence_gem.h |   2 +-
 2 files changed, 118 insertions(+), 41 deletions(-)

On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> There is a indentation error in this patch in the gem_transmit function.
> I have written it like that to make it easier to see the changes. It is
> fixed in the next patch.
>
>  hw/net/cadence_gem.c         | 157 ++++++++++++++++++++++++++++++++-----------
>  include/hw/net/cadence_gem.h |   2 +-
>  2 files changed, 118 insertions(+), 41 deletions(-)
>
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 2dabad5..c80e833 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -141,6 +141,29 @@
>  #define GEM_DESCONF6      (0x00000294/4)
>  #define GEM_DESCONF7      (0x00000298/4)
>
> +#define GEM_INT_Q1_STATUS               (0x00000400 / 4)
> +
> +#define GEM_TRANSMIT_Q1_PTR             (0x00000440 / 4)
> +#define GEM_TRANSMIT_Q15_PTR            (GEM_TRANSMIT_Q1_PTR + 14)
> +
> +#define GEM_RECEIVE_Q1_PTR              (0x00000480 / 4)
> +#define GEM_RECEIVE_Q15_PTR             (GEM_RECEIVE_Q1_PTR + 14)
> +
> +#define GEM_INT_Q1_ENABLE               (0x00000600 / 4)
> +#define GEM_INT_Q7_ENABLE               (GEM_INT_Q1_ENABLE + 6)
> +#define GEM_INT_Q8_ENABLE               (0x00000660 / 4)
> +#define GEM_INT_Q15_ENABLE              (GEM_INT_Q8_ENABLE + 7)
> +
> +#define GEM_INT_Q1_DISABLE              (0x00000620 / 4)
> +#define GEM_INT_Q7_DISABLE              (GEM_INT_Q1_DISABLE + 6)
> +#define GEM_INT_Q8_DISABLE              (0x00000680 / 4)
> +#define GEM_INT_Q15_DISABLE             (GEM_INT_Q8_DISABLE + 7)
> +
> +#define GEM_INT_Q1_MASK                 (0x00000640 / 4)
> +#define GEM_INT_Q7_MASK                 (GEM_INT_Q1_MASK + 6)
> +#define GEM_INT_Q8_MASK                 (0x000006A0 / 4)
> +#define GEM_INT_Q15_MASK                (GEM_INT_Q8_MASK + 7)
> +
>  /*****************************************/
>  #define GEM_NWCTRL_TXSTART     0x00000200 /* Transmit Enable */
>  #define GEM_NWCTRL_TXENA       0x00000008 /* Transmit Enable */
> @@ -284,9 +307,9 @@ static inline unsigned tx_desc_get_length(unsigned *desc)
>      return desc[1] & DESC_1_LENGTH;
>  }
>
> -static inline void print_gem_tx_desc(unsigned *desc)
> +static inline void print_gem_tx_desc(unsigned *desc, uint8_t queue)
>  {
> -    DB_PRINT("TXDESC:\n");
> +    DB_PRINT("TXDESC (queue %" PRId8 "):\n", queue);
>      DB_PRINT("bufaddr: 0x%08x\n", *desc);
>      DB_PRINT("used_hw: %d\n", tx_desc_get_used(desc));
>      DB_PRINT("wrap:    %d\n", tx_desc_get_wrap(desc));
> @@ -416,6 +439,7 @@ static void phy_update_link(CadenceGEMState *s)
>  static int gem_can_receive(NetClientState *nc)
>  {
>      CadenceGEMState *s;
> +    int i;
>
>      s = qemu_get_nic_opaque(nc);
>
> @@ -428,18 +452,20 @@ static int gem_can_receive(NetClientState *nc)
>          return 0;
>      }
>
> -    if (rx_desc_get_ownership(s->rx_desc[0]) == 1) {
> -        if (s->can_rx_state != 2) {
> -            s->can_rx_state = 2;
> -            DB_PRINT("can't receive - busy buffer descriptor 0x%x\n",
> -                     s->rx_desc_addr[0]);
> +    for (i = 0; i < s->num_priority_queues; i++) {
> +        if (rx_desc_get_ownership(s->rx_desc[i]) == 1) {
> +            if (s->can_rx_state != 2) {
> +                s->can_rx_state = 2;
> +                DB_PRINT("can't receive - busy buffer descriptor (q%d) 0x%x\n",
> +                         i, s->rx_desc_addr[i]);
> +             }
> +            return 0;
>          }
> -        return 0;
>      }
>
>      if (s->can_rx_state != 0) {
>          s->can_rx_state = 0;
> -        DB_PRINT("can receive 0x%x\n", s->rx_desc_addr[0]);
> +        DB_PRINT("can receive\n");
>      }
>      return 1;
>  }
> @@ -450,9 +476,16 @@ static int gem_can_receive(NetClientState *nc)
>   */
>  static void gem_update_int_status(CadenceGEMState *s)
>  {
> -    if (s->regs[GEM_ISR]) {
> -        DB_PRINT("asserting int. (0x%08x)\n", s->regs[GEM_ISR]);
> -        qemu_set_irq(s->irq[0], 1);
> +    int i;
> +
> +    for (i = 0; i < s->num_priority_queues; ++i) {
> +        /* There seems to be no sane way to see which queue triggered the
> +         * interrupt.
> +         */
> +        if (s->regs[GEM_ISR]) {
> +            DB_PRINT("asserting int. q=%d)\n", i);
> +            qemu_set_irq(s->irq[i], 1);

I don't understand this. The hardware surely can't trigger
every IRQ line simultaneously and identically ?

> +        }
>      }
>  }
>
> @@ -601,17 +634,18 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
>      return GEM_RX_REJECT;
>  }
>
> -static void gem_get_rx_desc(CadenceGEMState *s)
> +static void gem_get_rx_desc(CadenceGEMState *s, int q)
>  {
> -    DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[0]);
> +    DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]);
>      /* read current descriptor */
>      cpu_physical_memory_read(s->rx_desc_addr[0],
>                               (uint8_t *)s->rx_desc[0], sizeof(s->rx_desc[0]));
>
>      /* Descriptor owned by software ? */
> -    if (rx_desc_get_ownership(s->rx_desc[0]) == 1) {
> +    if (rx_desc_get_ownership(s->rx_desc[q]) == 1 &&
> +            s->num_priority_queues == 1) {

Why only if num_priority_queues is 1? (This is one of those "looks
a bit odd, is the h/w really like this?" questions; it could be right.)

>          DB_PRINT("descriptor 0x%x owned by sw.\n",
> -                 (unsigned)s->rx_desc_addr[0]);
> +                 (unsigned)s->rx_desc_addr[q]);
>          s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
>          s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
>          /* Handle interrupt consequences */
> @@ -632,6 +666,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>      uint8_t   *rxbuf_ptr;
>      bool first_desc = true;
>      int maf;
> +    int q = 0;

Shouldn't we be doing something for each queue, rather than just
having a variable that's always 0?

>
>      s = qemu_get_nic_opaque(nc);
>
> @@ -729,31 +764,31 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>
>          /* Update the descriptor.  */
>          if (first_desc) {
> -            rx_desc_set_sof(s->rx_desc[0]);
> +            rx_desc_set_sof(s->rx_desc[q]);
>              first_desc = false;
>          }
>          if (bytes_to_copy == 0) {
> -            rx_desc_set_eof(s->rx_desc[0]);
> -            rx_desc_set_length(s->rx_desc[0], size);
> +            rx_desc_set_eof(s->rx_desc[q]);
> +            rx_desc_set_length(s->rx_desc[q], size);
>          }
> -        rx_desc_set_ownership(s->rx_desc[0]);
> +        rx_desc_set_ownership(s->rx_desc[q]);
>
>          switch (maf) {
>          case GEM_RX_PROMISCUOUS_ACCEPT:
>              break;
>          case GEM_RX_BROADCAST_ACCEPT:
> -            rx_desc_set_broadcast(s->rx_desc[0]);
> +            rx_desc_set_broadcast(s->rx_desc[q]);
>              break;
>          case GEM_RX_UNICAST_HASH_ACCEPT:
> -            rx_desc_set_unicast_hash(s->rx_desc[0]);
> +            rx_desc_set_unicast_hash(s->rx_desc[q]);
>              break;
>          case GEM_RX_MULTICAST_HASH_ACCEPT:
> -            rx_desc_set_multicast_hash(s->rx_desc[0]);
> +            rx_desc_set_multicast_hash(s->rx_desc[q]);
>              break;
>          case GEM_RX_REJECT:
>              abort();
>          default: /* SAR */
> -            rx_desc_set_sar(s->rx_desc[0], maf);
> +            rx_desc_set_sar(s->rx_desc[q], maf);
>          }
>
>          /* Descriptor write-back.  */
> @@ -762,14 +797,15 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>                                    sizeof(s->rx_desc[0]));
>
>          /* Next descriptor */
> -        if (rx_desc_get_wrap(s->rx_desc[0])) {
> +        if (rx_desc_get_wrap(s->rx_desc[q])) {
>              DB_PRINT("wrapping RX descriptor list\n");
>              s->rx_desc_addr[0] = s->regs[GEM_RXQBASE];
>          } else {
>              DB_PRINT("incrementing RX descriptor list\n");
>              s->rx_desc_addr[0] += 8;
>          }
> -        gem_get_rx_desc(s);
> +
> +        gem_get_rx_desc(s, q);
>      }
>
>      /* Count it */
> @@ -841,6 +877,7 @@ static void gem_transmit(CadenceGEMState *s)
>      uint8_t     tx_packet[2048];
>      uint8_t     *p;
>      unsigned    total_bytes;
> +    int8_t q;

Why int8_t here but int everywhere else?

>
>      /* Do nothing if transmit is not enabled. */
>      if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_TXENA)) {
> @@ -856,8 +893,9 @@ static void gem_transmit(CadenceGEMState *s)
>      p = tx_packet;
>      total_bytes = 0;
>
> +    for (q = s->num_priority_queues - 1; q >= 0; q--) {
>      /* read current descriptor */
> -    packet_desc_addr = s->tx_desc_addr[0];
> +    packet_desc_addr = s->tx_desc_addr[q];
>
>      DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr);
>      cpu_physical_memory_read(packet_desc_addr,
> @@ -869,7 +907,7 @@ static void gem_transmit(CadenceGEMState *s)
>          if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_TXENA)) {
>              return;
>          }
> -        print_gem_tx_desc(desc);
> +        print_gem_tx_desc(desc, q);
>
>          /* The real hardware would eat this (and possibly crash).
>           * For QEMU let's lend a helping hand.
> @@ -904,18 +942,18 @@ static void gem_transmit(CadenceGEMState *s)
>              /* Modify the 1st descriptor of this packet to be owned by
>               * the processor.
>               */
> -            cpu_physical_memory_read(s->tx_desc_addr[0], (uint8_t *)desc_first,
> +            cpu_physical_memory_read(s->tx_desc_addr[q], (uint8_t *)desc_first,
>                                       sizeof(desc_first));
>              tx_desc_set_used(desc_first);
> -            cpu_physical_memory_write(s->tx_desc_addr[0], (uint8_t *)desc_first,
> +            cpu_physical_memory_write(s->tx_desc_addr[q], (uint8_t *)desc_first,
>                                        sizeof(desc_first));
>              /* Advance the hardware current descriptor past this packet */
>              if (tx_desc_get_wrap(desc)) {
> -                s->tx_desc_addr[0] = s->regs[GEM_TXQBASE];
> +                s->tx_desc_addr[q] = s->regs[GEM_TXQBASE];
>              } else {
> -                s->tx_desc_addr[0] = packet_desc_addr + 8;
> +                s->tx_desc_addr[q] = packet_desc_addr + 8;
>              }
> -            DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[0]);
> +            DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[q]);
>
>              s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL;
>              s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
> @@ -961,6 +999,7 @@ static void gem_transmit(CadenceGEMState *s)
>          s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
>          gem_update_int_status(s);
>      }
> +    }
>  }
>
>  static void gem_phy_reset(CadenceGEMState *s)
> @@ -1067,7 +1106,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      CadenceGEMState *s;
>      uint32_t retval;
> -
> +    int i;
>      s = (CadenceGEMState *)opaque;
>
>      offset >>= 2;
> @@ -1077,8 +1116,10 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>
>      switch (offset) {
>      case GEM_ISR:
> -        DB_PRINT("lowering irq on ISR read\n");
> -        qemu_set_irq(s->irq[0], 0);
> +        DB_PRINT("lowering irqs on ISR read\n");
> +        for (i = 0; i < s->num_priority_queues; ++i) {
> +            qemu_set_irq(s->irq[i], 0);
> +        }
>          break;
>      case GEM_PHYMNTNC:
>          if (retval & GEM_PHYMNTNC_OP_R) {
> @@ -1103,6 +1144,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>      retval &= ~(s->regs_wo[offset]);
>
>      DB_PRINT("0x%08x\n", retval);
> +    gem_update_int_status(s);
>      return retval;
>  }
>
> @@ -1115,6 +1157,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>  {
>      CadenceGEMState *s = (CadenceGEMState *)opaque;
>      uint32_t readonly;
> +    int i;
>
>      DB_PRINT("offset: 0x%04x write: 0x%08x ", (unsigned)offset, (unsigned)val);
>      offset >>= 2;
> @@ -1134,14 +1177,18 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>      switch (offset) {
>      case GEM_NWCTRL:
>          if (val & GEM_NWCTRL_RXENA) {
> -            gem_get_rx_desc(s);
> +            for (i = 0; i < s->num_priority_queues; ++i) {
> +                gem_get_rx_desc(s, i);
> +            }
>          }
>          if (val & GEM_NWCTRL_TXSTART) {
>              gem_transmit(s);
>          }
>          if (!(val & GEM_NWCTRL_TXENA)) {
>              /* Reset to start of Q when transmit disabled. */
> -            s->tx_desc_addr[0] = s->regs[GEM_TXQBASE];
> +            for (i = 0; i < s->num_priority_queues; i++) {
> +                s->tx_desc_addr[i] = s->regs[GEM_TXQBASE];
> +            }
>          }
>          if (gem_can_receive(qemu_get_queue(s->nic))) {
>              qemu_flush_queued_packets(qemu_get_queue(s->nic));
> @@ -1154,9 +1201,15 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>      case GEM_RXQBASE:
>          s->rx_desc_addr[0] = val;
>          break;
> +    case GEM_RECEIVE_Q1_PTR ... GEM_RECEIVE_Q15_PTR:
> +        s->rx_desc_addr[offset - GEM_RECEIVE_Q1_PTR + 1] = val;
> +        break;
>      case GEM_TXQBASE:
>          s->tx_desc_addr[0] = val;
>          break;
> +    case GEM_TRANSMIT_Q1_PTR ... GEM_TRANSMIT_Q15_PTR:
> +        s->tx_desc_addr[offset - GEM_TRANSMIT_Q1_PTR + 1] = val;
> +        break;
>      case GEM_RXSTATUS:
>          gem_update_int_status(s);
>          break;
> @@ -1164,10 +1217,26 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>          s->regs[GEM_IMR] &= ~val;
>          gem_update_int_status(s);
>          break;
> +    case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE:
> +        s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= ~val;
> +        gem_update_int_status(s);
> +        break;
> +    case GEM_INT_Q8_ENABLE ... GEM_INT_Q15_ENABLE:
> +        s->regs[GEM_INT_Q8_MASK + offset - GEM_INT_Q8_ENABLE] &= ~val;
> +        gem_update_int_status(s);
> +        break;
>      case GEM_IDR:
>          s->regs[GEM_IMR] |= val;
>          gem_update_int_status(s);
>          break;
> +    case GEM_INT_Q1_DISABLE ... GEM_INT_Q7_DISABLE:
> +        s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_DISABLE] |= val;
> +        gem_update_int_status(s);
> +        break;
> +    case GEM_INT_Q8_DISABLE ... GEM_INT_Q15_DISABLE:
> +        s->regs[GEM_INT_Q8_MASK + offset - GEM_INT_Q8_DISABLE] |= val;
> +        gem_update_int_status(s);
> +        break;
>      case GEM_SPADDR1LO:
>      case GEM_SPADDR2LO:
>      case GEM_SPADDR3LO:
> @@ -1204,8 +1273,11 @@ static const MemoryRegionOps gem_ops = {
>
>  static void gem_set_link(NetClientState *nc)
>  {
> +    CadenceGEMState *s = qemu_get_nic_opaque(nc);
> +
>      DB_PRINT("\n");
> -    phy_update_link(qemu_get_nic_opaque(nc));
> +    phy_update_link(s);
> +    gem_update_int_status(s);
>  }
>
>  static NetClientInfo net_gem_info = {
> @@ -1219,8 +1291,11 @@ static NetClientInfo net_gem_info = {
>  static void gem_realize(DeviceState *dev, Error **errp)
>  {
>      CadenceGEMState *s = CADENCE_GEM(dev);
> +    int i;
>
> -    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq[0]);
> +    for (i = 0; i < s->num_priority_queues; ++i) {
> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq[i]);
> +    }
>
>      qemu_macaddr_default_if_unset(&s->conf.macaddr);
>
> @@ -1260,6 +1335,8 @@ static const VMStateDescription vmstate_cadence_gem = {
>
>  static Property gem_properties[] = {
>      DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
> +    DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
> +                      num_priority_queues, 1),

This should go in the same patch as adding the field to the
CadenceGEMState struct (don't care whether you move the prop
define or the field addition).

>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
> index 77e0582..60b3ab0 100644
> --- a/include/hw/net/cadence_gem.h
> +++ b/include/hw/net/cadence_gem.h
> @@ -30,7 +30,7 @@
>  #include "net/net.h"
>  #include "hw/sysbus.h"
>
> -#define CADENCE_GEM_MAXREG        (0x00000640/4) /* Last valid GEM address */
> +#define CADENCE_GEM_MAXREG        (0x00000800 / 4) /* Last valid GEM address */

Changing this define changes the migration format (because it
has an array of this size in it) so you need a version bump.

>  #define MAX_PRIORITY_QUEUES             8
>
> --
> 2.7.4

thanks
-- PMM

On Mon, Jul 25, 2016 at 8:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 July 2016 at 00:20, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>> There is a indentation error in this patch in the gem_transmit function.
>> I have written it like that to make it easier to see the changes. It is
>> fixed in the next patch.
>>
>>  hw/net/cadence_gem.c         | 157 ++++++++++++++++++++++++++++++++-----------
>>  include/hw/net/cadence_gem.h |   2 +-
>>  2 files changed, 118 insertions(+), 41 deletions(-)
>>
>> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
>> index 2dabad5..c80e833 100644
>> --- a/hw/net/cadence_gem.c
>> +++ b/hw/net/cadence_gem.c
>> @@ -141,6 +141,29 @@
>>  #define GEM_DESCONF6      (0x00000294/4)
>>  #define GEM_DESCONF7      (0x00000298/4)
>>
>> +#define GEM_INT_Q1_STATUS               (0x00000400 / 4)
>> +
>> +#define GEM_TRANSMIT_Q1_PTR             (0x00000440 / 4)
>> +#define GEM_TRANSMIT_Q15_PTR            (GEM_TRANSMIT_Q1_PTR + 14)
>> +
>> +#define GEM_RECEIVE_Q1_PTR              (0x00000480 / 4)
>> +#define GEM_RECEIVE_Q15_PTR             (GEM_RECEIVE_Q1_PTR + 14)
>> +
>> +#define GEM_INT_Q1_ENABLE               (0x00000600 / 4)
>> +#define GEM_INT_Q7_ENABLE               (GEM_INT_Q1_ENABLE + 6)
>> +#define GEM_INT_Q8_ENABLE               (0x00000660 / 4)
>> +#define GEM_INT_Q15_ENABLE              (GEM_INT_Q8_ENABLE + 7)
>> +
>> +#define GEM_INT_Q1_DISABLE              (0x00000620 / 4)
>> +#define GEM_INT_Q7_DISABLE              (GEM_INT_Q1_DISABLE + 6)
>> +#define GEM_INT_Q8_DISABLE              (0x00000680 / 4)
>> +#define GEM_INT_Q15_DISABLE             (GEM_INT_Q8_DISABLE + 7)
>> +
>> +#define GEM_INT_Q1_MASK                 (0x00000640 / 4)
>> +#define GEM_INT_Q7_MASK                 (GEM_INT_Q1_MASK + 6)
>> +#define GEM_INT_Q8_MASK                 (0x000006A0 / 4)
>> +#define GEM_INT_Q15_MASK                (GEM_INT_Q8_MASK + 7)
>> +
>>  /*****************************************/
>>  #define GEM_NWCTRL_TXSTART     0x00000200 /* Transmit Enable */
>>  #define GEM_NWCTRL_TXENA       0x00000008 /* Transmit Enable */
>> @@ -284,9 +307,9 @@ static inline unsigned tx_desc_get_length(unsigned *desc)
>>      return desc[1] & DESC_1_LENGTH;
>>  }
>>
>> -static inline void print_gem_tx_desc(unsigned *desc)
>> +static inline void print_gem_tx_desc(unsigned *desc, uint8_t queue)
>>  {
>> -    DB_PRINT("TXDESC:\n");
>> +    DB_PRINT("TXDESC (queue %" PRId8 "):\n", queue);
>>      DB_PRINT("bufaddr: 0x%08x\n", *desc);
>>      DB_PRINT("used_hw: %d\n", tx_desc_get_used(desc));
>>      DB_PRINT("wrap:    %d\n", tx_desc_get_wrap(desc));
>> @@ -416,6 +439,7 @@ static void phy_update_link(CadenceGEMState *s)
>>  static int gem_can_receive(NetClientState *nc)
>>  {
>>      CadenceGEMState *s;
>> +    int i;
>>
>>      s = qemu_get_nic_opaque(nc);
>>
>> @@ -428,18 +452,20 @@ static int gem_can_receive(NetClientState *nc)
>>          return 0;
>>      }
>>
>> -    if (rx_desc_get_ownership(s->rx_desc[0]) == 1) {
>> -        if (s->can_rx_state != 2) {
>> -            s->can_rx_state = 2;
>> -            DB_PRINT("can't receive - busy buffer descriptor 0x%x\n",
>> -                     s->rx_desc_addr[0]);
>> +    for (i = 0; i < s->num_priority_queues; i++) {
>> +        if (rx_desc_get_ownership(s->rx_desc[i]) == 1) {
>> +            if (s->can_rx_state != 2) {
>> +                s->can_rx_state = 2;
>> +                DB_PRINT("can't receive - busy buffer descriptor (q%d) 0x%x\n",
>> +                         i, s->rx_desc_addr[i]);
>> +             }
>> +            return 0;
>>          }
>> -        return 0;
>>      }
>>
>>      if (s->can_rx_state != 0) {
>>          s->can_rx_state = 0;
>> -        DB_PRINT("can receive 0x%x\n", s->rx_desc_addr[0]);
>> +        DB_PRINT("can receive\n");
>>      }
>>      return 1;
>>  }
>> @@ -450,9 +476,16 @@ static int gem_can_receive(NetClientState *nc)
>>   */
>>  static void gem_update_int_status(CadenceGEMState *s)
>>  {
>> -    if (s->regs[GEM_ISR]) {
>> -        DB_PRINT("asserting int. (0x%08x)\n", s->regs[GEM_ISR]);
>> -        qemu_set_irq(s->irq[0], 1);
>> +    int i;
>> +
>> +    for (i = 0; i < s->num_priority_queues; ++i) {
>> +        /* There seems to be no sane way to see which queue triggered the
>> +         * interrupt.
>> +         */
>> +        if (s->regs[GEM_ISR]) {
>> +            DB_PRINT("asserting int. q=%d)\n", i);
>> +            qemu_set_irq(s->irq[i], 1);
>
> I don't understand this. The hardware surely can't trigger
> every IRQ line simultaneously and identically ?

Yeah, this is wrong, I have updated it.

>
>> +        }
>>      }
>>  }
>>
>> @@ -601,17 +634,18 @@ static int gem_mac_address_filter(CadenceGEMState *s, const uint8_t *packet)
>>      return GEM_RX_REJECT;
>>  }
>>
>> -static void gem_get_rx_desc(CadenceGEMState *s)
>> +static void gem_get_rx_desc(CadenceGEMState *s, int q)
>>  {
>> -    DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[0]);
>> +    DB_PRINT("read descriptor 0x%x\n", (unsigned)s->rx_desc_addr[q]);
>>      /* read current descriptor */
>>      cpu_physical_memory_read(s->rx_desc_addr[0],
>>                               (uint8_t *)s->rx_desc[0], sizeof(s->rx_desc[0]));
>>
>>      /* Descriptor owned by software ? */
>> -    if (rx_desc_get_ownership(s->rx_desc[0]) == 1) {
>> +    if (rx_desc_get_ownership(s->rx_desc[q]) == 1 &&
>> +            s->num_priority_queues == 1) {
>
> Why only if num_priority_queues is 1? (This is one of those "looks
> a bit odd, is the h/w really like this?" questions; it could be right.)

Another weird thing, fixed.

>
>>          DB_PRINT("descriptor 0x%x owned by sw.\n",
>> -                 (unsigned)s->rx_desc_addr[0]);
>> +                 (unsigned)s->rx_desc_addr[q]);
>>          s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
>>          s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
>>          /* Handle interrupt consequences */
>> @@ -632,6 +666,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>>      uint8_t   *rxbuf_ptr;
>>      bool first_desc = true;
>>      int maf;
>> +    int q = 0;
>
> Shouldn't we be doing something for each queue, rather than just
> having a variable that's always 0?

Woops, I missed out on the screening logic. Added it in.

>
>>
>>      s = qemu_get_nic_opaque(nc);
>>
>> @@ -729,31 +764,31 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>>
>>          /* Update the descriptor.  */
>>          if (first_desc) {
>> -            rx_desc_set_sof(s->rx_desc[0]);
>> +            rx_desc_set_sof(s->rx_desc[q]);
>>              first_desc = false;
>>          }
>>          if (bytes_to_copy == 0) {
>> -            rx_desc_set_eof(s->rx_desc[0]);
>> -            rx_desc_set_length(s->rx_desc[0], size);
>> +            rx_desc_set_eof(s->rx_desc[q]);
>> +            rx_desc_set_length(s->rx_desc[q], size);
>>          }
>> -        rx_desc_set_ownership(s->rx_desc[0]);
>> +        rx_desc_set_ownership(s->rx_desc[q]);
>>
>>          switch (maf) {
>>          case GEM_RX_PROMISCUOUS_ACCEPT:
>>              break;
>>          case GEM_RX_BROADCAST_ACCEPT:
>> -            rx_desc_set_broadcast(s->rx_desc[0]);
>> +            rx_desc_set_broadcast(s->rx_desc[q]);
>>              break;
>>          case GEM_RX_UNICAST_HASH_ACCEPT:
>> -            rx_desc_set_unicast_hash(s->rx_desc[0]);
>> +            rx_desc_set_unicast_hash(s->rx_desc[q]);
>>              break;
>>          case GEM_RX_MULTICAST_HASH_ACCEPT:
>> -            rx_desc_set_multicast_hash(s->rx_desc[0]);
>> +            rx_desc_set_multicast_hash(s->rx_desc[q]);
>>              break;
>>          case GEM_RX_REJECT:
>>              abort();
>>          default: /* SAR */
>> -            rx_desc_set_sar(s->rx_desc[0], maf);
>> +            rx_desc_set_sar(s->rx_desc[q], maf);
>>          }
>>
>>          /* Descriptor write-back.  */
>> @@ -762,14 +797,15 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>>                                    sizeof(s->rx_desc[0]));
>>
>>          /* Next descriptor */
>> -        if (rx_desc_get_wrap(s->rx_desc[0])) {
>> +        if (rx_desc_get_wrap(s->rx_desc[q])) {
>>              DB_PRINT("wrapping RX descriptor list\n");
>>              s->rx_desc_addr[0] = s->regs[GEM_RXQBASE];
>>          } else {
>>              DB_PRINT("incrementing RX descriptor list\n");
>>              s->rx_desc_addr[0] += 8;
>>          }
>> -        gem_get_rx_desc(s);
>> +
>> +        gem_get_rx_desc(s, q);
>>      }
>>
>>      /* Count it */
>> @@ -841,6 +877,7 @@ static void gem_transmit(CadenceGEMState *s)
>>      uint8_t     tx_packet[2048];
>>      uint8_t     *p;
>>      unsigned    total_bytes;
>> +    int8_t q;
>
> Why int8_t here but int everywhere else?

No reason, it's an int now.

>
>>
>>      /* Do nothing if transmit is not enabled. */
>>      if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_TXENA)) {
>> @@ -856,8 +893,9 @@ static void gem_transmit(CadenceGEMState *s)
>>      p = tx_packet;
>>      total_bytes = 0;
>>
>> +    for (q = s->num_priority_queues - 1; q >= 0; q--) {
>>      /* read current descriptor */
>> -    packet_desc_addr = s->tx_desc_addr[0];
>> +    packet_desc_addr = s->tx_desc_addr[q];
>>
>>      DB_PRINT("read descriptor 0x%" HWADDR_PRIx "\n", packet_desc_addr);
>>      cpu_physical_memory_read(packet_desc_addr,
>> @@ -869,7 +907,7 @@ static void gem_transmit(CadenceGEMState *s)
>>          if (!(s->regs[GEM_NWCTRL] & GEM_NWCTRL_TXENA)) {
>>              return;
>>          }
>> -        print_gem_tx_desc(desc);
>> +        print_gem_tx_desc(desc, q);
>>
>>          /* The real hardware would eat this (and possibly crash).
>>           * For QEMU let's lend a helping hand.
>> @@ -904,18 +942,18 @@ static void gem_transmit(CadenceGEMState *s)
>>              /* Modify the 1st descriptor of this packet to be owned by
>>               * the processor.
>>               */
>> -            cpu_physical_memory_read(s->tx_desc_addr[0], (uint8_t *)desc_first,
>> +            cpu_physical_memory_read(s->tx_desc_addr[q], (uint8_t *)desc_first,
>>                                       sizeof(desc_first));
>>              tx_desc_set_used(desc_first);
>> -            cpu_physical_memory_write(s->tx_desc_addr[0], (uint8_t *)desc_first,
>> +            cpu_physical_memory_write(s->tx_desc_addr[q], (uint8_t *)desc_first,
>>                                        sizeof(desc_first));
>>              /* Advance the hardware current descriptor past this packet */
>>              if (tx_desc_get_wrap(desc)) {
>> -                s->tx_desc_addr[0] = s->regs[GEM_TXQBASE];
>> +                s->tx_desc_addr[q] = s->regs[GEM_TXQBASE];
>>              } else {
>> -                s->tx_desc_addr[0] = packet_desc_addr + 8;
>> +                s->tx_desc_addr[q] = packet_desc_addr + 8;
>>              }
>> -            DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[0]);
>> +            DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[q]);
>>
>>              s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL;
>>              s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
>> @@ -961,6 +999,7 @@ static void gem_transmit(CadenceGEMState *s)
>>          s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
>>          gem_update_int_status(s);
>>      }
>> +    }
>>  }
>>
>>  static void gem_phy_reset(CadenceGEMState *s)
>> @@ -1067,7 +1106,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>>  {
>>      CadenceGEMState *s;
>>      uint32_t retval;
>> -
>> +    int i;
>>      s = (CadenceGEMState *)opaque;
>>
>>      offset >>= 2;
>> @@ -1077,8 +1116,10 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>>
>>      switch (offset) {
>>      case GEM_ISR:
>> -        DB_PRINT("lowering irq on ISR read\n");
>> -        qemu_set_irq(s->irq[0], 0);
>> +        DB_PRINT("lowering irqs on ISR read\n");
>> +        for (i = 0; i < s->num_priority_queues; ++i) {
>> +            qemu_set_irq(s->irq[i], 0);
>> +        }
>>          break;
>>      case GEM_PHYMNTNC:
>>          if (retval & GEM_PHYMNTNC_OP_R) {
>> @@ -1103,6 +1144,7 @@ static uint64_t gem_read(void *opaque, hwaddr offset, unsigned size)
>>      retval &= ~(s->regs_wo[offset]);
>>
>>      DB_PRINT("0x%08x\n", retval);
>> +    gem_update_int_status(s);
>>      return retval;
>>  }
>>
>> @@ -1115,6 +1157,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>>  {
>>      CadenceGEMState *s = (CadenceGEMState *)opaque;
>>      uint32_t readonly;
>> +    int i;
>>
>>      DB_PRINT("offset: 0x%04x write: 0x%08x ", (unsigned)offset, (unsigned)val);
>>      offset >>= 2;
>> @@ -1134,14 +1177,18 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>>      switch (offset) {
>>      case GEM_NWCTRL:
>>          if (val & GEM_NWCTRL_RXENA) {
>> -            gem_get_rx_desc(s);
>> +            for (i = 0; i < s->num_priority_queues; ++i) {
>> +                gem_get_rx_desc(s, i);
>> +            }
>>          }
>>          if (val & GEM_NWCTRL_TXSTART) {
>>              gem_transmit(s);
>>          }
>>          if (!(val & GEM_NWCTRL_TXENA)) {
>>              /* Reset to start of Q when transmit disabled. */
>> -            s->tx_desc_addr[0] = s->regs[GEM_TXQBASE];
>> +            for (i = 0; i < s->num_priority_queues; i++) {
>> +                s->tx_desc_addr[i] = s->regs[GEM_TXQBASE];
>> +            }
>>          }
>>          if (gem_can_receive(qemu_get_queue(s->nic))) {
>>              qemu_flush_queued_packets(qemu_get_queue(s->nic));
>> @@ -1154,9 +1201,15 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>>      case GEM_RXQBASE:
>>          s->rx_desc_addr[0] = val;
>>          break;
>> +    case GEM_RECEIVE_Q1_PTR ... GEM_RECEIVE_Q15_PTR:
>> +        s->rx_desc_addr[offset - GEM_RECEIVE_Q1_PTR + 1] = val;
>> +        break;
>>      case GEM_TXQBASE:
>>          s->tx_desc_addr[0] = val;
>>          break;
>> +    case GEM_TRANSMIT_Q1_PTR ... GEM_TRANSMIT_Q15_PTR:
>> +        s->tx_desc_addr[offset - GEM_TRANSMIT_Q1_PTR + 1] = val;
>> +        break;
>>      case GEM_RXSTATUS:
>>          gem_update_int_status(s);
>>          break;
>> @@ -1164,10 +1217,26 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>>          s->regs[GEM_IMR] &= ~val;
>>          gem_update_int_status(s);
>>          break;
>> +    case GEM_INT_Q1_ENABLE ... GEM_INT_Q7_ENABLE:
>> +        s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_ENABLE] &= ~val;
>> +        gem_update_int_status(s);
>> +        break;
>> +    case GEM_INT_Q8_ENABLE ... GEM_INT_Q15_ENABLE:
>> +        s->regs[GEM_INT_Q8_MASK + offset - GEM_INT_Q8_ENABLE] &= ~val;
>> +        gem_update_int_status(s);
>> +        break;
>>      case GEM_IDR:
>>          s->regs[GEM_IMR] |= val;
>>          gem_update_int_status(s);
>>          break;
>> +    case GEM_INT_Q1_DISABLE ... GEM_INT_Q7_DISABLE:
>> +        s->regs[GEM_INT_Q1_MASK + offset - GEM_INT_Q1_DISABLE] |= val;
>> +        gem_update_int_status(s);
>> +        break;
>> +    case GEM_INT_Q8_DISABLE ... GEM_INT_Q15_DISABLE:
>> +        s->regs[GEM_INT_Q8_MASK + offset - GEM_INT_Q8_DISABLE] |= val;
>> +        gem_update_int_status(s);
>> +        break;
>>      case GEM_SPADDR1LO:
>>      case GEM_SPADDR2LO:
>>      case GEM_SPADDR3LO:
>> @@ -1204,8 +1273,11 @@ static const MemoryRegionOps gem_ops = {
>>
>>  static void gem_set_link(NetClientState *nc)
>>  {
>> +    CadenceGEMState *s = qemu_get_nic_opaque(nc);
>> +
>>      DB_PRINT("\n");
>> -    phy_update_link(qemu_get_nic_opaque(nc));
>> +    phy_update_link(s);
>> +    gem_update_int_status(s);
>>  }
>>
>>  static NetClientInfo net_gem_info = {
>> @@ -1219,8 +1291,11 @@ static NetClientInfo net_gem_info = {
>>  static void gem_realize(DeviceState *dev, Error **errp)
>>  {
>>      CadenceGEMState *s = CADENCE_GEM(dev);
>> +    int i;
>>
>> -    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq[0]);
>> +    for (i = 0; i < s->num_priority_queues; ++i) {
>> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq[i]);
>> +    }
>>
>>      qemu_macaddr_default_if_unset(&s->conf.macaddr);
>>
>> @@ -1260,6 +1335,8 @@ static const VMStateDescription vmstate_cadence_gem = {
>>
>>  static Property gem_properties[] = {
>>      DEFINE_NIC_PROPERTIES(CadenceGEMState, conf),
>> +    DEFINE_PROP_UINT8("num-priority-queues", CadenceGEMState,
>> +                      num_priority_queues, 1),
>
> This should go in the same patch as adding the field to the
> CadenceGEMState struct (don't care whether you move the prop
> define or the field addition).

Agreed, I have moved it to an earlier patch.

>
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h
>> index 77e0582..60b3ab0 100644
>> --- a/include/hw/net/cadence_gem.h
>> +++ b/include/hw/net/cadence_gem.h
>> @@ -30,7 +30,7 @@
>>  #include "net/net.h"
>>  #include "hw/sysbus.h"
>>
>> -#define CADENCE_GEM_MAXREG        (0x00000640/4) /* Last valid GEM address */
>> +#define CADENCE_GEM_MAXREG        (0x00000800 / 4) /* Last valid GEM address */
>
> Changing this define changes the migration format (because it
> has an array of this size in it) so you need a version bump.

Done

Thanks,

Alistair

>
>>  #define MAX_PRIORITY_QUEUES             8
>>
>> --
>> 2.7.4
>
> thanks
> -- PMM
>