RE: [PATCH v3 07/11] net: cadence_gem: Add support for jumbo frames

[Sai Pavan Boddu]

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