| Message ID | 1588935645-20351-7-git-send-email-sai.pavan.boddu@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cadence GEM Fixes | expand |
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 <sai.pavan.boddu@xilinx.com> > --- > 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 <edgar.iglesias@xilinx.com>
Hi Edgar, > -----Original Message----- > From: Edgar E. Iglesias <edgar.iglesias@gmail.com> > Sent: Friday, May 8, 2020 5:00 PM > To: Sai Pavan Boddu <saipava@xilinx.com> > Cc: Alistair Francis <Alistair.Francis@wdc.com>; Peter Maydell > <peter.maydell@linaro.org>; Jason Wang <jasowang@redhat.com>; Markus > Armbruster <armbru@redhat.com>; Philippe Mathieu-Daudé > <philmd@redhat.com>; Tong Ho <tongh@xilinx.com>; Ramon Fried > <rfried.dev@gmail.com>; qemu-arm@nongnu.org; qemu- > devel@nongnu.org > 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 <sai.pavan.boddu@xilinx.com> > > --- > > 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 <edgar.iglesias@xilinx.com>
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); +} + +static void gem_unrealize(DeviceState *dev, Error **errp) +{ + CadenceGEMState *s = CADENCE_GEM(dev); + + g_free(s->tx_packet); + g_free(s->rx_packet); } static void gem_init(Object *obj) @@ -1669,6 +1678,7 @@ static void gem_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); dc->realize = gem_realize; + dc->unrealize = gem_unrealize; device_class_set_props(dc, gem_properties); dc->vmsd = &vmstate_cadence_gem; dc->reset = gem_reset; diff --git a/include/hw/net/cadence_gem.h b/include/hw/net/cadence_gem.h index 5c83036..8dbbaa3 100644 --- a/include/hw/net/cadence_gem.h +++ b/include/hw/net/cadence_gem.h @@ -80,6 +80,8 @@ typedef struct CadenceGEMState { uint8_t can_rx_state; /* Debug only */ + uint8_t *tx_packet; + uint8_t *rx_packet; uint32_t rx_desc[MAX_PRIORITY_QUEUES][DESC_MAX_NUM_WORDS]; bool sar_active[4];
Moving this buffers to CadenceGEMState, as their size will be increased more when JUMBO frames support is added. Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> --- hw/net/cadence_gem.c | 52 ++++++++++++++++++++++++++------------------ include/hw/net/cadence_gem.h | 2 ++ 2 files changed, 33 insertions(+), 21 deletions(-)