Re: [Qemu-devel] [PATCH v2 6/7] hw/net/stellaris_enet: Get rid of rx_fifo pointer

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> The rx_fifo pointer is awkward to migrate, and is actually
> redundant since it is always possible to determine it from
> the current rx[].len/.data and rx_fifo_len. Remove both
> rx_fifo and rx_fifo_len from the state, replacing them with
> a simple rx_fifo_offset which points at the current location
> in the RX fifo.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/stellaris_enet.c | 42 ++++++++++++++++++++----------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index e818b9d..af1c3ef 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -67,8 +67,7 @@ typedef struct {
>          uint8_t data[2048];
>          int len;
>      } rx[31];
> -    uint8_t *rx_fifo;
> -    int rx_fifo_len;
> +    int rx_fifo_offset;
>      int next_packet;
>      NICState *nic;
>      NICConf conf;
> @@ -216,21 +215,21 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
>      case 0x0c: /* TCTL */
>          return s->tctl;
>      case 0x10: /* DATA */
> -        if (s->rx_fifo_len == 0) {
> -            if (s->np == 0) {
> -                BADF("RX underflow\n");
> -                return 0;
> -            }
> -            s->rx_fifo_len = s->rx[s->next_packet].len;
> -            s->rx_fifo = s->rx[s->next_packet].data;
> -            DPRINTF("RX FIFO start packet len=%d\n", s->rx_fifo_len);
> +    {
> +        uint8_t *rx_fifo;
> +
> +        if (s->np == 0) {
> +            BADF("RX underflow\n");
> +            return 0;
>          }
> -        val = s->rx_fifo[0] | (s->rx_fifo[1] << 8) | (s->rx_fifo[2] << 16)
> -              | (s->rx_fifo[3] << 24);
> -        s->rx_fifo += 4;
> -        s->rx_fifo_len -= 4;
> -        if (s->rx_fifo_len <= 0) {
> -            s->rx_fifo_len = 0;
> +
> +        rx_fifo = s->rx[s->next_packet].data + s->rx_fifo_offset;
> +
> +        val = rx_fifo[0] | (rx_fifo[1] << 8) | (rx_fifo[2] << 16)
> +              | (rx_fifo[3] << 24);
> +        s->rx_fifo_offset += 4;
> +        if (s->rx_fifo_offset >= s->rx[s->next_packet].len) {
> +            s->rx_fifo_offset = 0;
>              s->next_packet++;
>              if (s->next_packet >= 31)
>                  s->next_packet = 0;
> @@ -238,6 +237,7 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
>              DPRINTF("RX done np=%d\n", s->np);
>          }
>          return val;
> +    }
>      case 0x14: /* IA0 */
>          return s->conf.macaddr.a[0] | (s->conf.macaddr.a[1] << 8)
>              | (s->conf.macaddr.a[2] << 16)
> @@ -291,8 +291,8 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>      case 0x08: /* RCTL */
>          s->rctl = value;
>          if (value & SE_RCTL_RSTFIFO) {
> -            s->rx_fifo_len = 0;
>              s->np = 0;
> +            s->rx_fifo_offset = 0;
>              stellaris_enet_update(s);
>          }
>          break;
> @@ -402,8 +402,7 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
>  
>      }
>      qemu_put_be32(f, s->next_packet);
> -    qemu_put_be32(f, s->rx_fifo - s->rx[s->next_packet].data);
> -    qemu_put_be32(f, s->rx_fifo_len);
> +    qemu_put_be32(f, s->rx_fifo_offset);
>  }
>  
>  static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> @@ -432,8 +431,7 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
>  
>      }
>      s->next_packet = qemu_get_be32(f);
> -    s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> -    s->rx_fifo_len = qemu_get_be32(f);
> +    s->rx_fifo_offset = qemu_get_be32(f);

Hmm, yes with the proviso that this of course is still as insecure as the original,
but you're about to fix that.

>      return 0;
>  }
> @@ -469,7 +467,7 @@ static int stellaris_enet_init(SysBusDevice *sbd)
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>  
>      stellaris_enet_reset(s);
> -    register_savevm(dev, "stellaris_enet", -1, 2,
> +    register_savevm(dev, "stellaris_enet", -1, 3,
>                      stellaris_enet_save, stellaris_enet_load, s);
>      return 0;
>  }

Same comment as the tx patch, need to change the matching value in the _load(
However, now that you've got the VMState conversion in there I think it's perfectly
reasonable to skip these version inc's; it doesn't seem to make sense to inc a
version by 3 in one small patchset.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> -- 
> 1.9.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK