[Qemu-devel] [PATCH v2 0/7] stellaris_enet: overhaul tx/rx, convert to vmstate

[Qemu-devel] [PATCH v2 0/7] stellaris_enet: overhaul tx/rx, convert to vmstate

[Peter Maydell]

This patchset overhauls the stellaris_enet TX and RX handling code,
and converts it to use vmstate.

The principal motivation is to fix the buffer overrun noted
in the first patch, and to reimplement things using simpler
state fields which are easier to migrate and to validate in
post_load. I also fixed a couple of other bugs I noticed while
I was there.

This isn't actually sufficient to get my test image to work:
that needs proper implementation of the MII registers in the PHY.
I tested this with a minor hack to make all MII registers return
0x24, which happens to satisfy the test image's setup code.
However implementing the PHY registers is more work than I want
to do on this device right now...

Changes v1->v2:
 * only transmit when 1 is written to TR, not on any write
 * new patches to get rid of rx_fifo, rx_fifo_len
 * vmstate conversion (includes migration sanitizing code)

Peter Maydell (7):
  hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer
    overrun
  hw/net/stellaris_enet: Correct handling of packet padding
  hw/net/stellaris_enet: Rewrite tx fifo handling code
  hw/net/stellaris_enet: Correctly implement the TR and THR registers
  hw/net/stellaris_enet: Fix debug format strings
  hw/net/stellaris_enet: Get rid of rx_fifo pointer
  hw/net/stellaris_enet: Convert to vmstate

 hw/net/stellaris_enet.c | 311 ++++++++++++++++++++++++++++--------------------
 1 file changed, 185 insertions(+), 126 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v2 1/7] hw/net/stellaris_enet: Restructure tx_fifo code to avoid buffer overrun

[Peter Maydell]

The current tx_fifo code has a corner case where the guest can overrun
the fifo buffer: if automatic CRCs are disabled we allow the guest to write
the CRC word even if there isn't actually space for it in the FIFO.
The datasheet is unclear about exactly how the hardware deals with this
situation; the most plausible answer seems to be that the CRC word is
just lost.

Implement this fix by separating the "can we stuff another word in the
FIFO" logic from the "should we transmit the packet now" check. This
also moves us closer to the real hardware, which has a number of ways
it can be configured to trigger sending the packet, some of which we
don't implement.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/net/stellaris_enet.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index d04e6a4..bd844cd 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -253,10 +253,12 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
                 s->tx_fifo[s->tx_fifo_len++] = value >> 24;
             }
         } else {
-            s->tx_fifo[s->tx_fifo_len++] = value;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
+                s->tx_fifo[s->tx_fifo_len++] = value;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
+                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+            }
             if (s->tx_fifo_len >= s->tx_frame_len) {
                 /* We don't implement explicit CRC, so just chop it off.  */
                 if ((s->tctl & SE_TCTL_CRC) == 0)
-- 
1.9.0

[Qemu-devel] [PATCH v2 2/7] hw/net/stellaris_enet: Correct handling of packet padding

[Peter Maydell]

The PADEN bit in the transmit control register enables padding of short
data packets out to the required minimum length. However a typo here
meant we were adjusting tx_fifo_len rather than tx_frame_len, so the
padding didn't actually happen. Fix this bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: qemu-stable@nongnu.org
---
 hw/net/stellaris_enet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index bd844cd..d0da819 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -265,7 +265,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
                     s->tx_frame_len -= 4;
                 if ((s->tctl & SE_TCTL_PADEN) && s->tx_frame_len < 60) {
                     memset(&s->tx_fifo[s->tx_frame_len], 0, 60 - s->tx_frame_len);
-                    s->tx_fifo_len = 60;
+                    s->tx_frame_len = 60;
                 }
                 qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo,
                                  s->tx_frame_len);
-- 
1.9.0

[Qemu-devel] [PATCH v2 3/7] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Peter Maydell]

The datasheet is clear that the frame length written to the DATA
register is actually stored in the TX FIFO; this means we don't
need to keep both tx_frame_len and tx_fifo_len state separately.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/stellaris_enet.c | 121 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 43 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index d0da819..47787fd 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -59,7 +59,6 @@ typedef struct {
     uint32_t mtxd;
     uint32_t mrxd;
     uint32_t np;
-    int tx_frame_len;
     int tx_fifo_len;
     uint8_t tx_fifo[2048];
     /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
@@ -82,6 +81,62 @@ static void stellaris_enet_update(stellaris_enet_state *s)
     qemu_set_irq(s->irq, (s->ris & s->im) != 0);
 }
 
+/* Return the data length of the packet currently being assembled
+ * in the TX fifo.
+ */
+static inline int stellaris_txpacket_datalen(stellaris_enet_state *s)
+{
+    return s->tx_fifo[0] | (s->tx_fifo[1] << 8);
+}
+
+/* Return true if the packet currently in the TX FIFO is complete,
+* ie the FIFO holds enough bytes for the data length, ethernet header,
+* payload and optionally CRC.
+*/
+static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
+{
+    int framelen = stellaris_txpacket_datalen(s);
+    framelen += 16;
+    if (!(s->tctl & SE_TCTL_CRC)) {
+        framelen += 4;
+    }
+    /* Cover the corner case of a 2032 byte payload with auto-CRC disabled:
+     * this requires more bytes than will fit in the FIFO. It's not totally
+     * clear how the h/w handles this, but if using threshold-based TX
+     * it will definitely try to transmit something.
+     */
+    framelen = MIN(framelen, ARRAY_SIZE(s->tx_fifo));
+    return s->tx_fifo_len >= framelen;
+}
+
+/* Send the packet currently in the TX FIFO */
+static void stellaris_enet_send(stellaris_enet_state *s)
+{
+    int framelen = stellaris_txpacket_datalen(s);
+
+    /* Ethernet header is in the FIFO but not in the datacount.
+     * We don't implement explicit CRC, so just ignore any
+     * CRC value in the FIFO.
+     */
+    framelen += 14;
+    if ((s->tctl & SE_TCTL_PADEN) && framelen < 60) {
+        memset(&s->tx_fifo[framelen + 2], 0, 60 - framelen);
+        framelen = 60;
+    }
+    /* This MIN will have no effect unless the FIFO data is corrupt
+     * (eg bad data from an incoming migration); otherwise the check
+     * on the datalen at the start of writing the data into the FIFO
+     * will have caught this. Silently write a corrupt half-packet,
+     * which is what the hardware does in FIFO underrun situations.
+     */
+    framelen = MIN(framelen, ARRAY_SIZE(s->tx_fifo) - 2);
+    qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo + 2, framelen);
+    s->tx_fifo_len = 0;
+    s->ris |= SE_INT_TXEMP;
+    stellaris_enet_update(s);
+    DPRINTF("Done TX\n");
+}
+
 /* TODO: Implement MAC address filtering.  */
 static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, size_t size)
 {
@@ -215,8 +270,9 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         DPRINTF("IRQ ack %02x/%02x\n", value, s->ris);
         stellaris_enet_update(s);
         /* Clearing TXER also resets the TX fifo.  */
-        if (value & SE_INT_TXER)
-            s->tx_frame_len = -1;
+        if (value & SE_INT_TXER) {
+            s->tx_fifo_len = 0;
+        }
         break;
     case 0x04: /* IM */
         DPRINTF("IRQ mask %02x/%02x\n", value, s->ris);
@@ -235,46 +291,27 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         s->tctl = value;
         break;
     case 0x10: /* DATA */
-        if (s->tx_frame_len == -1) {
-            s->tx_frame_len = value & 0xffff;
-            if (s->tx_frame_len > 2032) {
-                DPRINTF("TX frame too long (%d)\n", s->tx_frame_len);
-                s->tx_frame_len = 0;
+        if (s->tx_fifo_len == 0) {
+            /* The first word is special, it contains the data length */
+            int framelen = value & 0xffff;
+            if (framelen > 2032) {
+                DPRINTF("TX frame too long (%d)\n", framelen);
                 s->ris |= SE_INT_TXER;
                 stellaris_enet_update(s);
-            } else {
-                DPRINTF("Start TX frame len=%d\n", s->tx_frame_len);
-                /* The value written does not include the ethernet header.  */
-                s->tx_frame_len += 14;
-                if ((s->tctl & SE_TCTL_CRC) == 0)
-                    s->tx_frame_len += 4;
-                s->tx_fifo_len = 0;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
-            }
-        } else {
-            if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
-                s->tx_fifo[s->tx_fifo_len++] = value;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 8;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 16;
-                s->tx_fifo[s->tx_fifo_len++] = value >> 24;
-            }
-            if (s->tx_fifo_len >= s->tx_frame_len) {
-                /* We don't implement explicit CRC, so just chop it off.  */
-                if ((s->tctl & SE_TCTL_CRC) == 0)
-                    s->tx_frame_len -= 4;
-                if ((s->tctl & SE_TCTL_PADEN) && s->tx_frame_len < 60) {
-                    memset(&s->tx_fifo[s->tx_frame_len], 0, 60 - s->tx_frame_len);
-                    s->tx_frame_len = 60;
-                }
-                qemu_send_packet(qemu_get_queue(s->nic), s->tx_fifo,
-                                 s->tx_frame_len);
-                s->tx_frame_len = -1;
-                s->ris |= SE_INT_TXEMP;
-                stellaris_enet_update(s);
-                DPRINTF("Done TX\n");
+                break;
             }
         }
+
+        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {
+            s->tx_fifo[s->tx_fifo_len++] = value;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 8;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 16;
+            s->tx_fifo[s->tx_fifo_len++] = value >> 24;
+        }
+
+        if (stellaris_txpacket_complete(s)) {
+            stellaris_enet_send(s);
+        }
         break;
     case 0x14: /* IA0 */
         s->conf.macaddr.a[0] = value;
@@ -326,7 +363,7 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
     s->im = SE_INT_PHY | SE_INT_MD | SE_INT_RXER | SE_INT_FOV | SE_INT_TXEMP
             | SE_INT_TXER | SE_INT_RX;
     s->thr = 0x3f;
-    s->tx_frame_len = -1;
+    s->tx_fifo_len = 0;
 }
 
 static void stellaris_enet_save(QEMUFile *f, void *opaque)
@@ -344,7 +381,6 @@ static void stellaris_enet_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, s->mtxd);
     qemu_put_be32(f, s->mrxd);
     qemu_put_be32(f, s->np);
-    qemu_put_be32(f, s->tx_frame_len);
     qemu_put_be32(f, s->tx_fifo_len);
     qemu_put_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
     for (i = 0; i < 31; i++) {
@@ -375,7 +411,6 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
     s->mtxd = qemu_get_be32(f);
     s->mrxd = qemu_get_be32(f);
     s->np = qemu_get_be32(f);
-    s->tx_frame_len = qemu_get_be32(f);
     s->tx_fifo_len = qemu_get_be32(f);
     qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
     for (i = 0; i < 31; i++) {
@@ -421,7 +456,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, 1,
+    register_savevm(dev, "stellaris_enet", -1, 2,
                     stellaris_enet_save, stellaris_enet_load, s);
     return 0;
 }
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 3/7] hw/net/stellaris_enet: Rewrite tx fifo handling code

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> The datasheet is clear that the frame length written to the DATA
> register is actually stored in the TX FIFO; this means we don't
> need to keep both tx_frame_len and tx_fifo_len state separately.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---

> -    register_savevm(dev, "stellaris_enet", -1, 1,
> +    register_savevm(dev, "stellaris_enet", -1, 2,
>                      stellaris_enet_save, stellaris_enet_load, s);
>      return 0;

Note that as well as inc'ing that you would have to change the
    if (version_id != 1)

in stellaris_enet_load (if you weren't about to scrap the load/save
code anyway).

Dave

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

[Qemu-devel] [PATCH v2 4/7] hw/net/stellaris_enet: Correctly implement the TR and THR registers

[Peter Maydell]

Packet transmission for the stellaris ethernet controller can be triggered
in one of two ways:
 * by setting a threshold value in the THR register; when the FIFO
   fill level reaches the threshold, the h/w starts transmitting.
   Software has to finish filling the FIFO before the transmit
   process completes to avoid a (silent) underrun
 * by software writing to the TR register to explicitly trigger
   transmission

Since QEMU transmits packets instantaneously (from the guest's
point of view), implement "transmit based on threshold" with
our existing mechanism of "transmit as soon as we have the whole
packet", with the additional wrinkle that we don't transmit if
the packet size is below the specified threshold, and implement
"transmit by specific request" properly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/stellaris_enet.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 47787fd..db6e43e 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -109,6 +109,15 @@ static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
     return s->tx_fifo_len >= framelen;
 }
 
+/* Return true if the TX FIFO threshold is enabled and the FIFO
+ * has filled enough to reach it.
+ */
+static inline bool stellaris_tx_thr_reached(stellaris_enet_state *s)
+{
+    return (s->thr < 0x3f &&
+            (s->tx_fifo_len >= 4 * (s->thr * 8 + 1)));
+}
+
 /* Send the packet currently in the TX FIFO */
 static void stellaris_enet_send(stellaris_enet_state *s)
 {
@@ -309,7 +318,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
             s->tx_fifo[s->tx_fifo_len++] = value >> 24;
         }
 
-        if (stellaris_txpacket_complete(s)) {
+        if (stellaris_tx_thr_reached(s) && stellaris_txpacket_complete(s)) {
             stellaris_enet_send(s);
         }
         break;
@@ -338,9 +347,13 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
     case 0x2c: /* MTXD */
         s->mtxd = value & 0xff;
         break;
+    case 0x38: /* TR */
+        if (value & 1) {
+            stellaris_enet_send(s);
+        }
+        break;
     case 0x30: /* MRXD */
     case 0x34: /* NP */
-    case 0x38: /* TR */
         /* Ignored.  */
     case 0x3c: /* Undocuented: Timestamp? */
         /* Ignored.  */
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 4/7] hw/net/stellaris_enet: Correctly implement the TR and THR registers

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Packet transmission for the stellaris ethernet controller can be triggered
> in one of two ways:
>  * by setting a threshold value in the THR register; when the FIFO
>    fill level reaches the threshold, the h/w starts transmitting.
>    Software has to finish filling the FIFO before the transmit
>    process completes to avoid a (silent) underrun
>  * by software writing to the TR register to explicitly trigger
>    transmission
> 
> Since QEMU transmits packets instantaneously (from the guest's
> point of view), implement "transmit based on threshold" with
> our existing mechanism of "transmit as soon as we have the whole
> packet", with the additional wrinkle that we don't transmit if
> the packet size is below the specified threshold, and implement
> "transmit by specific request" properly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  hw/net/stellaris_enet.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 47787fd..db6e43e 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -109,6 +109,15 @@ static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
>      return s->tx_fifo_len >= framelen;
>  }
>  
> +/* Return true if the TX FIFO threshold is enabled and the FIFO
> + * has filled enough to reach it.
> + */
> +static inline bool stellaris_tx_thr_reached(stellaris_enet_state *s)
> +{
> +    return (s->thr < 0x3f &&
> +            (s->tx_fifo_len >= 4 * (s->thr * 8 + 1)));
> +}
> +
>  /* Send the packet currently in the TX FIFO */
>  static void stellaris_enet_send(stellaris_enet_state *s)
>  {
> @@ -309,7 +318,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>              s->tx_fifo[s->tx_fifo_len++] = value >> 24;
>          }
>  
> -        if (stellaris_txpacket_complete(s)) {
> +        if (stellaris_tx_thr_reached(s) && stellaris_txpacket_complete(s)) {
>              stellaris_enet_send(s);
>          }
>          break;
> @@ -338,9 +347,13 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>      case 0x2c: /* MTXD */
>          s->mtxd = value & 0xff;
>          break;
> +    case 0x38: /* TR */
> +        if (value & 1) {
> +            stellaris_enet_send(s);
> +        }
> +        break;
>      case 0x30: /* MRXD */
>      case 0x34: /* NP */
> -    case 0x38: /* TR */
>          /* Ignored.  */
>      case 0x3c: /* Undocuented: Timestamp? */
>          /* Ignored.  */
> -- 
> 1.9.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2 5/7] hw/net/stellaris_enet: Fix debug format strings

[Peter Maydell]

Fix various debug format strings which were incorrect for the
data type, so that building with debug enabled is possible.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/net/stellaris_enet.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index db6e43e..e818b9d 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -161,7 +161,7 @@ static ssize_t stellaris_enet_receive(NetClientState *nc, const uint8_t *buf, si
         return -1;
     }
 
-    DPRINTF("Received packet len=%d\n", size);
+    DPRINTF("Received packet len=%zu\n", size);
     n = s->next_packet + s->np;
     if (n >= 31)
         n -= 31;
@@ -276,7 +276,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
     switch (offset) {
     case 0x00: /* IACK */
         s->ris &= ~value;
-        DPRINTF("IRQ ack %02x/%02x\n", value, s->ris);
+        DPRINTF("IRQ ack %02" PRIx64 "/%02x\n", value, s->ris);
         stellaris_enet_update(s);
         /* Clearing TXER also resets the TX fifo.  */
         if (value & SE_INT_TXER) {
@@ -284,7 +284,7 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         }
         break;
     case 0x04: /* IM */
-        DPRINTF("IRQ mask %02x/%02x\n", value, s->ris);
+        DPRINTF("IRQ mask %02" PRIx64 "/%02x\n", value, s->ris);
         s->im = value;
         stellaris_enet_update(s);
         break;
-- 
1.9.0

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

[Peter Maydell]

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);
 
     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;
 }
-- 
1.9.0

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

[Dr. David Alan Gilbert]

* 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

[Qemu-devel] [PATCH v2 7/7] hw/net/stellaris_enet: Convert to vmstate

[Peter Maydell]

Convert this device to use vmstate for its save/load, including
providing a post_load function that sanitizes inbound data to
avoid possible buffer overflows if it is malicious.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/stellaris_enet.c | 147 ++++++++++++++++++++++++++----------------------
 1 file changed, 79 insertions(+), 68 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index af1c3ef..b8cbf82 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -47,6 +47,11 @@ do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__);} while (0)
     OBJECT_CHECK(stellaris_enet_state, (obj), TYPE_STELLARIS_ENET)
 
 typedef struct {
+    uint8_t data[2048];
+    int32_t len;
+} StellarisEnetRxFrame;
+
+typedef struct {
     SysBusDevice parent_obj;
 
     uint32_t ris;
@@ -59,22 +64,88 @@ typedef struct {
     uint32_t mtxd;
     uint32_t mrxd;
     uint32_t np;
-    int tx_fifo_len;
+    int32_t tx_fifo_len;
     uint8_t tx_fifo[2048];
     /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
        We implement a full 31 packet fifo.  */
-    struct {
-        uint8_t data[2048];
-        int len;
-    } rx[31];
-    int rx_fifo_offset;
-    int next_packet;
+    StellarisEnetRxFrame rx[31];
+    int32_t rx_fifo_offset;
+    int32_t next_packet;
     NICState *nic;
     NICConf conf;
     qemu_irq irq;
     MemoryRegion mmio;
 } stellaris_enet_state;
 
+static const VMStateDescription vmstate_rx_frame = {
+    .name = "stellaris_enet/rx_frame",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(data, StellarisEnetRxFrame, 2048),
+        VMSTATE_INT32(len, StellarisEnetRxFrame),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static int stellaris_enet_post_load(void *opaque, int version_id)
+{
+    stellaris_enet_state *s = opaque;
+    int i;
+
+    /* Sanitize inbound state. Note that next_packet is an index but
+     * np is a size; hence their valid upper bounds differ.
+     */
+    if (s->next_packet < 0 || s->next_packet >= ARRAY_SIZE(s->rx)) {
+        return -1;
+    }
+
+    if (s->np > ARRAY_SIZE(s->rx)) {
+        return -1;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(s->rx); i++) {
+        if (s->rx[i].len < 0 || s->rx[i].len > ARRAY_SIZE(s->rx[i].data)) {
+            return -1;
+        }
+    }
+
+    if (s->rx_fifo_offset < 0 ||
+        s->rx_fifo_offset + 4 > ARRAY_SIZE(s->rx[0].data)) {
+        return -1;
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_stellaris_enet = {
+    .name = "stellaris_enet",
+    .version_id = 4,
+    .minimum_version_id = 4,
+    .minimum_version_id_old = 4,
+    .post_load = stellaris_enet_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(ris, stellaris_enet_state),
+        VMSTATE_UINT32(im, stellaris_enet_state),
+        VMSTATE_UINT32(rctl, stellaris_enet_state),
+        VMSTATE_UINT32(tctl, stellaris_enet_state),
+        VMSTATE_UINT32(thr, stellaris_enet_state),
+        VMSTATE_UINT32(mctl, stellaris_enet_state),
+        VMSTATE_UINT32(mdv, stellaris_enet_state),
+        VMSTATE_UINT32(mtxd, stellaris_enet_state),
+        VMSTATE_UINT32(mrxd, stellaris_enet_state),
+        VMSTATE_UINT32(np, stellaris_enet_state),
+        VMSTATE_INT32(tx_fifo_len, stellaris_enet_state),
+        VMSTATE_UINT8_ARRAY(tx_fifo, stellaris_enet_state, 2048),
+        VMSTATE_STRUCT_ARRAY(rx, stellaris_enet_state, 31, 1,
+                             vmstate_rx_frame, StellarisEnetRxFrame),
+        VMSTATE_INT32(next_packet, stellaris_enet_state),
+        VMSTATE_INT32(rx_fifo_offset, stellaris_enet_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void stellaris_enet_update(stellaris_enet_state *s)
 {
     qemu_set_irq(s->irq, (s->ris & s->im) != 0);
@@ -379,63 +450,6 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
     s->tx_fifo_len = 0;
 }
 
-static void stellaris_enet_save(QEMUFile *f, void *opaque)
-{
-    stellaris_enet_state *s = (stellaris_enet_state *)opaque;
-    int i;
-
-    qemu_put_be32(f, s->ris);
-    qemu_put_be32(f, s->im);
-    qemu_put_be32(f, s->rctl);
-    qemu_put_be32(f, s->tctl);
-    qemu_put_be32(f, s->thr);
-    qemu_put_be32(f, s->mctl);
-    qemu_put_be32(f, s->mdv);
-    qemu_put_be32(f, s->mtxd);
-    qemu_put_be32(f, s->mrxd);
-    qemu_put_be32(f, s->np);
-    qemu_put_be32(f, s->tx_fifo_len);
-    qemu_put_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
-    for (i = 0; i < 31; i++) {
-        qemu_put_be32(f, s->rx[i].len);
-        qemu_put_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
-
-    }
-    qemu_put_be32(f, s->next_packet);
-    qemu_put_be32(f, s->rx_fifo_offset);
-}
-
-static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
-{
-    stellaris_enet_state *s = (stellaris_enet_state *)opaque;
-    int i;
-
-    if (version_id != 1)
-        return -EINVAL;
-
-    s->ris = qemu_get_be32(f);
-    s->im = qemu_get_be32(f);
-    s->rctl = qemu_get_be32(f);
-    s->tctl = qemu_get_be32(f);
-    s->thr = qemu_get_be32(f);
-    s->mctl = qemu_get_be32(f);
-    s->mdv = qemu_get_be32(f);
-    s->mtxd = qemu_get_be32(f);
-    s->mrxd = qemu_get_be32(f);
-    s->np = qemu_get_be32(f);
-    s->tx_fifo_len = qemu_get_be32(f);
-    qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
-    for (i = 0; i < 31; i++) {
-        s->rx[i].len = qemu_get_be32(f);
-        qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
-
-    }
-    s->next_packet = qemu_get_be32(f);
-    s->rx_fifo_offset = qemu_get_be32(f);
-
-    return 0;
-}
-
 static void stellaris_enet_cleanup(NetClientState *nc)
 {
     stellaris_enet_state *s = qemu_get_nic_opaque(nc);
@@ -467,8 +481,6 @@ 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, 3,
-                    stellaris_enet_save, stellaris_enet_load, s);
     return 0;
 }
 
@@ -476,8 +488,6 @@ static void stellaris_enet_unrealize(DeviceState *dev, Error **errp)
 {
     stellaris_enet_state *s = STELLARIS_ENET(dev);
 
-    unregister_savevm(DEVICE(s), "stellaris_enet", s);
-
     memory_region_destroy(&s->mmio);
 }
 
@@ -494,6 +504,7 @@ static void stellaris_enet_class_init(ObjectClass *klass, void *data)
     k->init = stellaris_enet_init;
     dc->unrealize = stellaris_enet_unrealize;
     dc->props = stellaris_enet_properties;
+    dc->vmsd = &vmstate_stellaris_enet;
 }
 
 static const TypeInfo stellaris_enet_info = {
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 7/7] hw/net/stellaris_enet: Convert to vmstate

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Convert this device to use vmstate for its save/load, including
> providing a post_load function that sanitizes inbound data to
> avoid possible buffer overflows if it is malicious.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/net/stellaris_enet.c | 147 ++++++++++++++++++++++++++----------------------
>  1 file changed, 79 insertions(+), 68 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index af1c3ef..b8cbf82 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -47,6 +47,11 @@ do { fprintf(stderr, "stellaris_enet: error: " fmt , ## __VA_ARGS__);} while (0)
>      OBJECT_CHECK(stellaris_enet_state, (obj), TYPE_STELLARIS_ENET)
>  
>  typedef struct {
> +    uint8_t data[2048];
> +    int32_t len;
> +} StellarisEnetRxFrame;
> +
> +typedef struct {
>      SysBusDevice parent_obj;
>  
>      uint32_t ris;
> @@ -59,22 +64,88 @@ typedef struct {
>      uint32_t mtxd;
>      uint32_t mrxd;
>      uint32_t np;
> -    int tx_fifo_len;
> +    int32_t tx_fifo_len;
>      uint8_t tx_fifo[2048];
>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
>         We implement a full 31 packet fifo.  */
> -    struct {
> -        uint8_t data[2048];
> -        int len;
> -    } rx[31];
> -    int rx_fifo_offset;
> -    int next_packet;
> +    StellarisEnetRxFrame rx[31];
> +    int32_t rx_fifo_offset;
> +    int32_t next_packet;

I think if I were changing these I'd make them uint's for safety,
IMHO it's better than having to put explicit < 0 checks in the load code.
(assuming there is nowhere that they're designed to go -ve)

>      NICState *nic;
>      NICConf conf;
>      qemu_irq irq;
>      MemoryRegion mmio;
>  } stellaris_enet_state;
>  
> +static const VMStateDescription vmstate_rx_frame = {
> +    .name = "stellaris_enet/rx_frame",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(data, StellarisEnetRxFrame, 2048),
> +        VMSTATE_INT32(len, StellarisEnetRxFrame),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static int stellaris_enet_post_load(void *opaque, int version_id)
> +{
> +    stellaris_enet_state *s = opaque;
> +    int i;
> +
> +    /* Sanitize inbound state. Note that next_packet is an index but
> +     * np is a size; hence their valid upper bounds differ.
> +     */
> +    if (s->next_packet < 0 || s->next_packet >= ARRAY_SIZE(s->rx)) {
> +        return -1;
> +    }
> +
> +    if (s->np > ARRAY_SIZE(s->rx)) {
> +        return -1;
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(s->rx); i++) {
> +        if (s->rx[i].len < 0 || s->rx[i].len > ARRAY_SIZE(s->rx[i].data)) {
> +            return -1;
> +        }
> +    }
> +
> +    if (s->rx_fifo_offset < 0 ||
> +        s->rx_fifo_offset + 4 > ARRAY_SIZE(s->rx[0].data)) {
> +        return -1;
> +    }

You're not checking the tx loaded values, I think most of the code
is safe, but, in your patch 3 you have:

> +        if (s->tx_fifo_len + 4 <= ARRAY_SIZE(s->tx_fifo)) {

which I think could be made to overflow; so should probably
either check tx_fifo_len here, or fix that, or both.


> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_stellaris_enet = {
> +    .name = "stellaris_enet",
> +    .version_id = 4,
> +    .minimum_version_id = 4,
> +    .minimum_version_id_old = 4,
> +    .post_load = stellaris_enet_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(ris, stellaris_enet_state),
> +        VMSTATE_UINT32(im, stellaris_enet_state),
> +        VMSTATE_UINT32(rctl, stellaris_enet_state),
> +        VMSTATE_UINT32(tctl, stellaris_enet_state),
> +        VMSTATE_UINT32(thr, stellaris_enet_state),
> +        VMSTATE_UINT32(mctl, stellaris_enet_state),
> +        VMSTATE_UINT32(mdv, stellaris_enet_state),
> +        VMSTATE_UINT32(mtxd, stellaris_enet_state),
> +        VMSTATE_UINT32(mrxd, stellaris_enet_state),
> +        VMSTATE_UINT32(np, stellaris_enet_state),
> +        VMSTATE_INT32(tx_fifo_len, stellaris_enet_state),
> +        VMSTATE_UINT8_ARRAY(tx_fifo, stellaris_enet_state, 2048),
> +        VMSTATE_STRUCT_ARRAY(rx, stellaris_enet_state, 31, 1,
> +                             vmstate_rx_frame, StellarisEnetRxFrame),
> +        VMSTATE_INT32(next_packet, stellaris_enet_state),
> +        VMSTATE_INT32(rx_fifo_offset, stellaris_enet_state),

The next_packet/rx_fifo_offset are in the opposite order from your structure
after the previous patches.

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void stellaris_enet_update(stellaris_enet_state *s)
>  {
>      qemu_set_irq(s->irq, (s->ris & s->im) != 0);
> @@ -379,63 +450,6 @@ static void stellaris_enet_reset(stellaris_enet_state *s)
>      s->tx_fifo_len = 0;
>  }
>  
> -static void stellaris_enet_save(QEMUFile *f, void *opaque)
> -{

Good :-)


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