Re: [PATCH v3 06/11] net: cadence_gem: Move tx/rx packet buffert to CadenceGEMState

[Edgar E. Iglesias]

On Fri, May 08, 2020 at 04:30:40PM +0530, Sai Pavan Boddu wrote:
> Moving this buffers to CadenceGEMState, as their size will be increased
> more when JUMBO frames support is added.
> 
> Signed-off-by: Sai Pavan Boddu <address@hidden>
> ---
>  hw/net/cadence_gem.c         | 52 
> ++++++++++++++++++++++++++------------------
>  include/hw/net/cadence_gem.h |  2 ++
>  2 files changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 77a0588..5ccec1a 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -314,6 +314,8 @@
>  
>  #define GEM_MODID_VALUE 0x00020118
>  
> +#define MAX_FRAME_SIZE 2048
> +
>  static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s, uint32_t *desc)
>  {
>      uint64_t ret = desc[0];
> @@ -928,17 +930,14 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
>   */
>  static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t 
> size)
>  {
> -    CadenceGEMState *s;
> +    CadenceGEMState *s = qemu_get_nic_opaque(nc);
>      unsigned   rxbufsize, bytes_to_copy;
>      unsigned   rxbuf_offset;
> -    uint8_t    rxbuf[2048];
>      uint8_t   *rxbuf_ptr;
>      bool first_desc = true;
>      int maf;
>      int q = 0;
>  
> -    s = qemu_get_nic_opaque(nc);
> -
>      /* Is this destination MAC address "for us" ? */
>      maf = gem_mac_address_filter(s, buf);
>      if (maf == GEM_RX_REJECT) {
> @@ -994,19 +993,19 @@ static ssize_t gem_receive(NetClientState *nc, const 
> uint8_t *buf, size_t size)
>      } else {
>          unsigned crc_val;
>  
> -        if (size > sizeof(rxbuf) - sizeof(crc_val)) {
> -            size = sizeof(rxbuf) - sizeof(crc_val);
> +        if (size > MAX_FRAME_SIZE - sizeof(crc_val)) {
> +            size = MAX_FRAME_SIZE - sizeof(crc_val);
>          }
>          bytes_to_copy = size;
>          /* The application wants the FCS field, which QEMU does not provide.
>           * We must try and calculate one.
>           */
>  
> -        memcpy(rxbuf, buf, size);
> -        memset(rxbuf + size, 0, sizeof(rxbuf) - size);
> -        rxbuf_ptr = rxbuf;
> -        crc_val = cpu_to_le32(crc32(0, rxbuf, MAX(size, 60)));
> -        memcpy(rxbuf + size, &crc_val, sizeof(crc_val));
> +        memcpy(s->rx_packet, buf, size);
> +        memset(s->rx_packet + size, 0, MAX_FRAME_SIZE - size);
> +        rxbuf_ptr = s->rx_packet;
> +        crc_val = cpu_to_le32(crc32(0, s->rx_packet, MAX(size, 60)));
> +        memcpy(s->rx_packet + size, &crc_val, sizeof(crc_val));
>  
>          bytes_to_copy += 4;
>          size += 4;
> @@ -1152,7 +1151,6 @@ static void gem_transmit(CadenceGEMState *s)
>  {
>      uint32_t desc[DESC_MAX_NUM_WORDS];
>      hwaddr packet_desc_addr;
> -    uint8_t     tx_packet[2048];
>      uint8_t     *p;
>      unsigned    total_bytes;
>      int q = 0;
> @@ -1168,7 +1166,7 @@ static void gem_transmit(CadenceGEMState *s)
>       * Packets scattered across multiple descriptors are gathered to this
>       * one contiguous buffer first.
>       */
> -    p = tx_packet;
> +    p = s->tx_packet;
>      total_bytes = 0;
>  
>      for (q = s->num_priority_queues - 1; q >= 0; q--) {
> @@ -1198,12 +1196,12 @@ static void gem_transmit(CadenceGEMState *s)
>                  break;
>              }
>  
> -            if (tx_desc_get_length(desc) > sizeof(tx_packet) -
> -                                               (p - tx_packet)) {
> +            if (tx_desc_get_length(desc) > MAX_FRAME_SIZE -
> +                                               (p - s->tx_packet)) {
>                  DB_PRINT("TX descriptor @ 0x%" HWADDR_PRIx \
>                           " too large: size 0x%x space 0x%zx\n",
>                           packet_desc_addr, tx_desc_get_length(desc),
> -                         sizeof(tx_packet) - (p - tx_packet));
> +                         MAX_FRAME_SIZE - (p - s->tx_packet));
>                  break;
>              }
>  
> @@ -1248,24 +1246,24 @@ static void gem_transmit(CadenceGEMState *s)
>  
>                  /* Is checksum offload enabled? */
>                  if (s->regs[GEM_DMACFG] & GEM_DMACFG_TXCSUM_OFFL) {
> -                    net_checksum_calculate(tx_packet, total_bytes);
> +                    net_checksum_calculate(s->tx_packet, total_bytes);
>                  }
>  
>                  /* Update MAC statistics */
> -                gem_transmit_updatestats(s, tx_packet, total_bytes);
> +                gem_transmit_updatestats(s, s->tx_packet, total_bytes);
>  
>                  /* Send the packet somewhere */
>                  if (s->phy_loop || (s->regs[GEM_NWCTRL] &
>                                      GEM_NWCTRL_LOCALLOOP)) {
> -                    gem_receive(qemu_get_queue(s->nic), tx_packet,
> +                    gem_receive(qemu_get_queue(s->nic), s->tx_packet,
>                                  total_bytes);
>                  } else {
> -                    qemu_send_packet(qemu_get_queue(s->nic), tx_packet,
> +                    qemu_send_packet(qemu_get_queue(s->nic), s->tx_packet,
>                                       total_bytes);
>                  }
>  
>                  /* Prepare for next packet */
> -                p = tx_packet;
> +                p = s->tx_packet;
>                  total_bytes = 0;
>              }
>  
> @@ -1612,6 +1610,17 @@ static void gem_realize(DeviceState *dev, Error **errp)
>  
>      s->nic = qemu_new_nic(&net_gem_info, &s->conf,
>                            object_get_typename(OBJECT(dev)), dev->id, s);
> +
> +    s->tx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);
> +    s->rx_packet = g_new0(uint8_t, MAX_FRAME_SIZE);

Hi Sai,

Since you're only using MAX_FRAME_SIZE these could be arrays
in CadenceGEMState.

With that change:
Reviewed-by: Edgar E. Iglesias <address@hidden>