[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v3 07/11] net: cadence_gem: Add support for jumbo frames
From: |
Sai Pavan Boddu |
Subject: |
RE: [PATCH v3 07/11] net: cadence_gem: Add support for jumbo frames |
Date: |
Fri, 8 May 2020 19:22:51 +0000 |
Hi Edgar,
> -----Original Message-----
> From: Edgar E. Iglesias <address@hidden>
> Sent: Friday, May 8, 2020 5:14 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 07/11] net: cadence_gem: Add support for jumbo
> frames
>
> On Fri, May 08, 2020 at 04:30:41PM +0530, Sai Pavan Boddu wrote:
> > Add a property "jumbo-max-len", which can be configured for jumbo
> > frame size up to 16,383 bytes, and also introduce new register
> GEM_JUMBO_MAX_LEN.
> >
> > Signed-off-by: Sai Pavan Boddu <address@hidden>
> > ---
> > hw/net/cadence_gem.c | 21 +++++++++++++++++++--
> > include/hw/net/cadence_gem.h | 1 +
> > 2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index
> > 5ccec1a..45c50ab 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -61,6 +61,7 @@
> > #define GEM_TXPAUSE (0x0000003C/4) /* TX Pause Time reg */
> > #define GEM_TXPARTIALSF (0x00000040/4) /* TX Partial Store and
> Forward */
> > #define GEM_RXPARTIALSF (0x00000044/4) /* RX Partial Store and
> Forward */
> > +#define GEM_JUMBO_MAX_LEN (0x00000048 / 4) /* Max Jumbo Frame
> Size */
>
> Would be nice to align this in the same way as all the others...
>
>
>
> > #define GEM_HASHLO (0x00000080/4) /* Hash Low address reg */
> > #define GEM_HASHHI (0x00000084/4) /* Hash High address reg */
> > #define GEM_SPADDR1LO (0x00000088/4) /* Specific addr 1 low reg */
> > @@ -314,7 +315,8 @@
> >
> > #define GEM_MODID_VALUE 0x00020118
> >
> > -#define MAX_FRAME_SIZE 2048
> > +#define MAX_JUMBO_FRAME_SIZE_MASK 0x3FFF #define
> MAX_FRAME_SIZE
> > +MAX_JUMBO_FRAME_SIZE_MASK
> >
> > static inline uint64_t tx_desc_get_buffer(CadenceGEMState *s,
> > uint32_t *desc) { @@ -1343,9 +1345,10 @@ static void
> > gem_reset(DeviceState *d)
> > s->regs[GEM_RXPARTIALSF] = 0x000003ff;
> > s->regs[GEM_MODID] = s->revision;
> > s->regs[GEM_DESCONF] = 0x02500111;
> > - s->regs[GEM_DESCONF2] = 0x2ab13fff;
> > + s->regs[GEM_DESCONF2] = 0x2ab10000 | s->jumbo_max_len;
> > s->regs[GEM_DESCONF5] = 0x002f2045;
> > s->regs[GEM_DESCONF6] = GEM_DESCONF6_64B_MASK;
> > + s->regs[GEM_JUMBO_MAX_LEN] = s->jumbo_max_len;
> >
> > if (s->num_priority_queues > 1) {
> > queues_mask = MAKE_64BIT_MASK(1, s->num_priority_queues - 1);
> > @@ -1420,6 +1423,9 @@ static uint64_t gem_read(void *opaque, hwaddr
> offset, unsigned size)
> > DB_PRINT("lowering irqs on ISR read\n");
> > /* The interrupts get updated at the end of the function. */
> > break;
> > + case GEM_JUMBO_MAX_LEN:
> > + retval = s->jumbo_max_len;
> > + break;
> > case GEM_PHYMNTNC:
> > if (retval & GEM_PHYMNTNC_OP_R) {
> > uint32_t phy_addr, reg_num; @@ -1516,6 +1522,9 @@ static
> > void gem_write(void *opaque, hwaddr offset, uint64_t val,
> > s->regs[GEM_IMR] &= ~val;
> > gem_update_int_status(s);
> > break;
> > + case GEM_JUMBO_MAX_LEN:
> > + s->jumbo_max_len = val & MAX_JUMBO_FRAME_SIZE_MASK;
>
> I don't think writing to this register may increase the max len beyond the
> max-len selected at design time (the property).
> TBH I'm surprised this register is RW in the spec.
>
> We may need two variables here, one for the design-time configured max
> and another for the runtime configurable max.
[Sai Pavan Boddu] Better way is to, use new created property to set the reset
value of this register. Which can be overwritten by guest on runtime by
writing to regs[GEM_JUMBO_MAX_LEN].
I would add few checks, so that frames does not cross JUMBO max length
configured.
Thanks,
Sai Pavan
>
>
> > + 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);
> > @@ -1611,6 +1620,12 @@ 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);
> >
> > + if (s->jumbo_max_len > MAX_FRAME_SIZE) {
> > + g_warning("jumbo-max-len is grater than %d",
>
>
> You've got a typo here "grater".
>
> I also think we could error out here if wrong values are chosen.
>
> Best regards,
> Edgar
- [PATCH v3 02/11] net: cadence_gem: Fix the queue address update during wrap around, (continued)
- [PATCH v3 02/11] net: cadence_gem: Fix the queue address update during wrap around, Sai Pavan Boddu, 2020/05/08
- [PATCH v3 04/11] net: cadence_gem: Define access permission for interrupt registers, Sai Pavan Boddu, 2020/05/08
- [PATCH v3 05/11] net: cadence_gem: Set ISR according to queue in use, Sai Pavan Boddu, 2020/05/08
- [PATCH v3 06/11] net: cadence_gem: Move tx/rx packet buffert to CadenceGEMState, Sai Pavan Boddu, 2020/05/08
- [PATCH v3 09/11] net: cadence_gem: Update the reset value for interrupt mask register, Sai Pavan Boddu, 2020/05/08
- [PATCH v3 07/11] net: cadence_gem: Add support for jumbo frames, Sai Pavan Boddu, 2020/05/08
- [PATCH v3 10/11] net: cadence_gem: TX_LAST bit should be set by guest, Sai Pavan Boddu, 2020/05/08
- [PATCH v3 11/11] net: cadence_gem: Fix RX address filtering, Sai Pavan Boddu, 2020/05/08
- [PATCH v3 08/11] net: cadnece_gem: Update irq_read_clear field of designcfg_debug1 reg, Sai Pavan Boddu, 2020/05/08