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

[Sai Pavan Boddu]

Hi Edgar,

> -----Original Message-----
> From: Edgar E. Iglesias <address@hidden>
> Sent: Friday, May 8, 2020 5:00 PM
> To: Sai Pavan Boddu <address@hidden>
> Cc: Alistair Francis <address@hidden>; Peter Maydell
> <address@hidden>; Jason Wang <address@hidden>; Markus
> Armbruster <address@hidden>; Philippe Mathieu-Daudé
> <address@hidden>; Tong Ho <address@hidden>; Ramon Fried
> <address@hidden>; address@hidden; qemu-
> address@hidden
> Subject: Re: [PATCH v3 06/11] net: cadence_gem: Move tx/rx packet buffert
> to CadenceGEMState
> 
> 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.
[Sai Pavan Boddu] Ok, I will put this in v4.

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