[Qemu-devel] [PATCH 0/6] ftgmac100 and NC-SI enhancements for the Aspeed SoC

[Qemu-devel] [PATCH 0/6] ftgmac100 and NC-SI enhancements for the Aspeed SoC

[Cédric Le Goater]

Hello,

Here is a couple of enhancements and fixes for the ftgmac100 NIC used
on the Aspeed SoC. It includes VLAN and multicast support.

Following is an assorted set of changes for the NC-SI backend also
used on the Aspeed SoC when soldered on OpenPOWER boards. These fix a
few problems found when running OpenBMC on aspeed QEMU machines.

Thanks,

Cédric.

Cédric Le Goater (6):
  ftgmac100: compute maximum frame size depending on the protocol
  ftgmac100: add IEEE 802.1Q VLAN support
  net/ftgmac100: fix multicast hash routine
  slirp/ncsi: fix "Get Version ID" payload length
  slirp/ncsi: add a "Get Parameter" response
  slirp/ncsi: add checksum support

 include/hw/net/ftgmac100.h |  7 +++++-
 hw/net/ftgmac100.c         | 59 +++++++++++++++++++++++++++++++++++-----------
 slirp/ncsi.c               | 54 +++++++++++++++++++++++++++++++++++++-----
 3 files changed, 99 insertions(+), 21 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/6] ftgmac100: compute maximum frame size depending on the protocol

[Cédric Le Goater]

The maximum frame size includes the CRC and depends if a VLAN tag is
inserted or not. Adjust the frame size limit in the transmit handler
using on the FTGMAC100State buffer size and in the receive handler use
the packet protocol.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/net/ftgmac100.h |  7 ++++++-
 hw/net/ftgmac100.c         | 23 ++++++++++++-----------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
index d9bc589fbf72..94cfe0533297 100644
--- a/include/hw/net/ftgmac100.h
+++ b/include/hw/net/ftgmac100.h
@@ -16,6 +16,11 @@
 #include "hw/sysbus.h"
 #include "net/net.h"
 
+/*
+ * Max frame size for the receiving buffer
+ */
+#define FTGMAC100_MAX_FRAME_SIZE    9220
+
 typedef struct FTGMAC100State {
     /*< private >*/
     SysBusDevice parent_obj;
@@ -26,7 +31,7 @@ typedef struct FTGMAC100State {
     qemu_irq irq;
     MemoryRegion iomem;
 
-    uint8_t *frame;
+    uint8_t frame[FTGMAC100_MAX_FRAME_SIZE];
 
     uint32_t irq_state;
     uint32_t isr;
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index a6d27a7b01df..7598d08c9cb9 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -207,16 +207,18 @@ typedef struct {
 /*
  * Max frame size for the receiving buffer
  */
-#define FTGMAC100_MAX_FRAME_SIZE    10240
+#define FTGMAC100_MAX_FRAME_SIZE    9220
 
 /* Limits depending on the type of the frame
  *
  *   9216 for Jumbo frames (+ 4 for VLAN)
  *   1518 for other frames (+ 4 for VLAN)
  */
-static int ftgmac100_max_frame_size(FTGMAC100State *s)
+static int ftgmac100_max_frame_size(FTGMAC100State *s, uint16_t proto)
 {
-    return (s->maccr & FTGMAC100_MACCR_JUMBO_LF ? 9216 : 1518) + 4;
+    int max = (s->maccr & FTGMAC100_MACCR_JUMBO_LF ? 9216 : 1518);
+
+    return max + (proto == ETH_P_VLAN ? 4 : 0);
 }
 
 static void ftgmac100_update_irq(FTGMAC100State *s)
@@ -408,7 +410,6 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
     uint8_t *ptr = s->frame;
     uint32_t addr = tx_descriptor;
     uint32_t flags = 0;
-    int max_frame_size = ftgmac100_max_frame_size(s);
 
     while (1) {
         FTGMAC100Desc bd;
@@ -427,11 +428,12 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
             flags = bd.des1;
         }
 
-        len = bd.des0 & 0x3FFF;
-        if (frame_size + len > max_frame_size) {
+        len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
+        if (frame_size + len > sizeof(s->frame)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
                           __func__, len);
-            len = max_frame_size - frame_size;
+            s->isr |= FTGMAC100_INT_XPKT_LOST;
+            len =  sizeof(s->frame) - frame_size;
         }
 
         if (dma_memory_read(&address_space_memory, bd.des3, ptr, len)) {
@@ -788,7 +790,8 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
     uint32_t buf_len;
     size_t size = len;
     uint32_t first = FTGMAC100_RXDES0_FRS;
-    int max_frame_size = ftgmac100_max_frame_size(s);
+    uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
+    int max_frame_size = ftgmac100_max_frame_size(s, proto);
 
     if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN))
          != (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN)) {
@@ -814,9 +817,9 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
 
     /* Huge frames are truncated.  */
     if (size > max_frame_size) {
-        size = max_frame_size;
         qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %zd bytes\n",
                       __func__, size);
+        size = max_frame_size;
         flags |= FTGMAC100_RXDES0_FTL;
     }
 
@@ -934,8 +937,6 @@ static void ftgmac100_realize(DeviceState *dev, Error **errp)
                           object_get_typename(OBJECT(dev)), DEVICE(dev)->id,
                           s);
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
-
-    s->frame = g_malloc(FTGMAC100_MAX_FRAME_SIZE);
 }
 
 static const VMStateDescription vmstate_ftgmac100 = {
-- 
2.13.6

[Qemu-devel] [PATCH 2/6] ftgmac100: add IEEE 802.1Q VLAN support

[Cédric Le Goater]

The ftgmac100 NIC supports VLAN tag insertion and the MAC engine also
has a control to remove VLAN tags from received packets.

The VLAN control bits and VLAN tag information are contained in the
second word of the transmit and receive descriptors. The Insert VLAN
bit and the VLAN Tag available bit are only valid in the first segment
of the packet.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 7598d08c9cb9..50af1222464a 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -443,6 +443,24 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
             break;
         }
 
+        /* Check for VLAN */
+        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
+            bd.des1 & FTGMAC100_TXDES1_INS_VLANTAG &&
+            be16_to_cpu(PKT_GET_ETH_HDR(ptr)->h_proto) != ETH_P_VLAN) {
+            if (frame_size + len + 4 > sizeof(s->frame)) {
+                qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
+                              __func__, len);
+                s->isr |= FTGMAC100_INT_XPKT_LOST;
+                len =  sizeof(s->frame) - frame_size - 4;
+            }
+            memmove(ptr + 16, ptr + 12, len - 12);
+            ptr[12] = 0x81;
+            ptr[13] = 0x00;
+            ptr[14] = (uint8_t) bd.des1 >> 8;
+            ptr[15] = (uint8_t) bd.des1;
+            len += 4;
+        }
+
         ptr += len;
         frame_size += len;
         if (bd.des0 & FTGMAC100_TXDES0_LTS) {
@@ -858,7 +876,19 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
             buf_len += size - 4;
         }
         buf_addr = bd.des3;
-        dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
+        if (first && proto == ETH_P_VLAN && buf_len >= 18) {
+            bd.des1 = (buf[14] << 8) | buf[15] | FTGMAC100_RXDES1_VLANTAG_AVAIL;
+            if (s->maccr & FTGMAC100_MACCR_RM_VLAN) {
+                dma_memory_write(&address_space_memory, buf_addr, buf, 12);
+                dma_memory_write(&address_space_memory, buf_addr + 12, buf + 16,
+                                 buf_len - 16);
+            } else {
+                dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
+            }
+        } else {
+            bd.des1 = 0;
+            dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
+        }
         buf += buf_len;
         if (size < 4) {
             dma_memory_write(&address_space_memory, buf_addr + buf_len,
-- 
2.13.6

[Qemu-devel] [PATCH 3/6] net/ftgmac100: fix multicast hash routine

[Cédric Le Goater]

Based on the multicast hash calculation of the FTGMAC100 Linux driver.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 50af1222464a..fd7699b1c05e 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -778,8 +778,8 @@ static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
                 return 0;
             }
 
-            /* TODO: this does not seem to work for ftgmac100 */
-            mcast_idx = net_crc32(buf, ETH_ALEN) >> 26;
+            mcast_idx = net_crc32_le(buf, ETH_ALEN);
+            mcast_idx = (~(mcast_idx >> 2)) & 0x3f;
             if (!(s->math[mcast_idx / 32] & (1 << (mcast_idx % 32)))) {
                 return 0;
             }
-- 
2.13.6

[Qemu-devel] [PATCH 4/6] slirp/ncsi: fix "Get Version ID" payload length

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 slirp/ncsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp/ncsi.c b/slirp/ncsi.c
index d12ba3e494b0..02d0e9def3e8 100644
--- a/slirp/ncsi.c
+++ b/slirp/ncsi.c
@@ -60,7 +60,7 @@ static const struct ncsi_rsp_handler {
         { NCSI_PKT_RSP_EGMF,    4, NULL },
         { NCSI_PKT_RSP_DGMF,    4, NULL },
         { NCSI_PKT_RSP_SNFC,    4, NULL },
-        { NCSI_PKT_RSP_GVI,    36, NULL },
+        { NCSI_PKT_RSP_GVI,    40, NULL },
         { NCSI_PKT_RSP_GC,     32, ncsi_rsp_handler_gc },
         { NCSI_PKT_RSP_GP,     -1, NULL },
         { NCSI_PKT_RSP_GCPS,  172, NULL },
-- 
2.13.6

[Qemu-devel] [PATCH 5/6] slirp/ncsi: add a "Get Parameter" response

[Cédric Le Goater]

This is a minimum response to exercise the kernel.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 slirp/ncsi.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/slirp/ncsi.c b/slirp/ncsi.c
index 02d0e9def3e8..f00253641ea4 100644
--- a/slirp/ncsi.c
+++ b/slirp/ncsi.c
@@ -35,6 +35,27 @@ static int ncsi_rsp_handler_gls(struct ncsi_rsp_pkt_hdr *rnh)
     return 0;
 }
 
+/* Get Parameter */
+static int ncsi_rsp_handler_gp(struct ncsi_rsp_pkt_hdr *rnh)
+{
+    struct ncsi_rsp_gp_pkt *rsp = (struct ncsi_rsp_gp_pkt *) rnh;
+
+    rsp->mac_cnt = 0x0;
+    rsp->mac_enable = 0x0;
+
+    /* TODO: provide the guest configured MAC */
+    rsp->mac[0] = 0xaa;
+    rsp->mac[1] = 0xbb;
+    rsp->mac[2] = 0xcc;
+    rsp->mac[3] = 0xdd;
+    rsp->mac[4] = 0xee;
+    rsp->mac[5] = 0xff;
+
+    /* more data follows */
+
+    return 0;
+}
+
 static const struct ncsi_rsp_handler {
         unsigned char   type;
         int             payload;
@@ -62,7 +83,7 @@ static const struct ncsi_rsp_handler {
         { NCSI_PKT_RSP_SNFC,    4, NULL },
         { NCSI_PKT_RSP_GVI,    40, NULL },
         { NCSI_PKT_RSP_GC,     32, ncsi_rsp_handler_gc },
-        { NCSI_PKT_RSP_GP,     -1, NULL },
+        { NCSI_PKT_RSP_GP,     40, ncsi_rsp_handler_gp },
         { NCSI_PKT_RSP_GCPS,  172, NULL },
         { NCSI_PKT_RSP_GNS,   172, NULL },
         { NCSI_PKT_RSP_GNPTS, 172, NULL },
-- 
2.13.6

[Qemu-devel] [PATCH 6/6] slirp/ncsi: add checksum support

[Cédric Le Goater]

The checksum field of a NC-SI packet contains a value that may be
included in each command and response. The verification is optional
but the Linux driver does so when a non-zero value is provided. Let's
extend the model to compute the checksum value and exercise a little
more the Linux driver.

See section "8.2.2.3 - 2's Complement Checksum Compensation" in the
Network Controller Sideband Interface (NC-SI) Specification for more
details.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 slirp/ncsi.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/slirp/ncsi.c b/slirp/ncsi.c
index f00253641ea4..fbfb79713f23 100644
--- a/slirp/ncsi.c
+++ b/slirp/ncsi.c
@@ -8,9 +8,23 @@
  */
 #include "qemu/osdep.h"
 #include "slirp.h"
+#include "net/checksum.h"
 
 #include "ncsi-pkt.h"
 
+static uint32_t ncsi_calculate_checksum(unsigned char *data, int len)
+{
+    uint32_t checksum = 0;
+    int i;
+
+    for (i = 0; i < len; i += 2) {
+        checksum += (((uint32_t)data[i] << 8) | data[i + 1]);
+    }
+
+    checksum = (~checksum + 1);
+    return checksum;
+}
+
 /* Get Capabilities */
 static int ncsi_rsp_handler_gc(struct ncsi_rsp_pkt_hdr *rnh)
 {
@@ -108,6 +122,9 @@ void ncsi_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
         (ncsi_reply + ETH_HLEN);
     const struct ncsi_rsp_handler *handler = NULL;
     int i;
+    int ncsi_rsp_len = sizeof(*nh);
+    uint32_t checksum;
+    uint32_t *pchecksum;
 
     memset(ncsi_reply, 0, sizeof(ncsi_reply));
 
@@ -137,15 +154,19 @@ void ncsi_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
             /* TODO: handle errors */
             handler->handler(rnh);
         }
+        ncsi_rsp_len += handler->payload;
     } else {
         rnh->common.length = 0;
         rnh->code          = htons(NCSI_PKT_RSP_C_UNAVAILABLE);
         rnh->reason        = htons(NCSI_PKT_RSP_R_UNKNOWN);
     }
 
-    /* TODO: add a checksum at the end of the frame but the specs
-     * allows it to be zero */
+    /* add a checksum at the end of the frame (the specs allows it to
+     * be zero */
+    checksum = ncsi_calculate_checksum((unsigned char *) rnh, ncsi_rsp_len);
+    pchecksum = (uint32_t *)((void *) rnh + ncsi_rsp_len);
+    *pchecksum = htonl(checksum);
+    ncsi_rsp_len += 4;
 
-    slirp_output(slirp->opaque, ncsi_reply, ETH_HLEN + sizeof(*nh) +
-                 (handler ? handler->payload : 0) + 4);
+    slirp_output(slirp->opaque, ncsi_reply, ETH_HLEN + ncsi_rsp_len);
 }
-- 
2.13.6

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] ftgmac100 and NC-SI enhancements for the Aspeed SoC

[Philippe Mathieu-Daudé]

Hi Cédric,

On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
> Hello,
> 
> Here is a couple of enhancements and fixes for the ftgmac100 NIC used
> on the Aspeed SoC. It includes VLAN and multicast support.
> 
> Following is an assorted set of changes for the NC-SI backend also
> used on the Aspeed SoC when soldered on OpenPOWER boards. These fix a
> few problems found when running OpenBMC on aspeed QEMU machines.
> 
> Thanks,
> 
> Cédric.
> 
> Cédric Le Goater (6):
>   ftgmac100: compute maximum frame size depending on the protocol
>   ftgmac100: add IEEE 802.1Q VLAN support
>   net/ftgmac100: fix multicast hash routine
>   slirp/ncsi: fix "Get Version ID" payload length
>   slirp/ncsi: add a "Get Parameter" response
>   slirp/ncsi: add checksum support

Doesn't it make more sens to have slirp patches applied previous to the
ftgmac100 model?

> 
>  include/hw/net/ftgmac100.h |  7 +++++-
>  hw/net/ftgmac100.c         | 59 +++++++++++++++++++++++++++++++++++-----------
>  slirp/ncsi.c               | 54 +++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 99 insertions(+), 21 deletions(-)
> 

Re: [Qemu-devel] [PATCH 3/6] net/ftgmac100: fix multicast hash routine

[Philippe Mathieu-Daudé]

On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
> Based on the multicast hash calculation of the FTGMAC100 Linux driver.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/net/ftgmac100.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 50af1222464a..fd7699b1c05e 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -778,8 +778,8 @@ static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
>                  return 0;
>              }
>  
> -            /* TODO: this does not seem to work for ftgmac100 */
> -            mcast_idx = net_crc32(buf, ETH_ALEN) >> 26;
> +            mcast_idx = net_crc32_le(buf, ETH_ALEN);
> +            mcast_idx = (~(mcast_idx >> 2)) & 0x3f;
>              if (!(s->math[mcast_idx / 32] & (1 << (mcast_idx % 32)))) {
>                  return 0;
>              }
> 

Re: [Qemu-devel] [PATCH 4/6] slirp/ncsi: fix "Get Version ID" payload length

[Philippe Mathieu-Daudé]

On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  slirp/ncsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/slirp/ncsi.c b/slirp/ncsi.c
> index d12ba3e494b0..02d0e9def3e8 100644
> --- a/slirp/ncsi.c
> +++ b/slirp/ncsi.c
> @@ -60,7 +60,7 @@ static const struct ncsi_rsp_handler {
>          { NCSI_PKT_RSP_EGMF,    4, NULL },
>          { NCSI_PKT_RSP_DGMF,    4, NULL },
>          { NCSI_PKT_RSP_SNFC,    4, NULL },
> -        { NCSI_PKT_RSP_GVI,    36, NULL },
> +        { NCSI_PKT_RSP_GVI,    40, NULL },

sizeof(ncsi_rsp_gvi_pkt) - sizeof(ncsi_rsp_pkt_hdr) = 40, ok.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>          { NCSI_PKT_RSP_GC,     32, ncsi_rsp_handler_gc },
>          { NCSI_PKT_RSP_GP,     -1, NULL },
>          { NCSI_PKT_RSP_GCPS,  172, NULL },
> 

Re: [Qemu-devel] [PATCH 5/6] slirp/ncsi: add a "Get Parameter" response

[Philippe Mathieu-Daudé]

Hi Cédric,

In title: "Get Parameters" (plural)

On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
> This is a minimum response to exercise the kernel.

This type is mandatory in DMTF Standard v1.0.1 indeed (DSP0222).

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  slirp/ncsi.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/slirp/ncsi.c b/slirp/ncsi.c
> index 02d0e9def3e8..f00253641ea4 100644
> --- a/slirp/ncsi.c
> +++ b/slirp/ncsi.c
> @@ -35,6 +35,27 @@ static int ncsi_rsp_handler_gls(struct ncsi_rsp_pkt_hdr *rnh)
>      return 0;
>  }
>  
> +/* Get Parameter */

"Parameters"

> +static int ncsi_rsp_handler_gp(struct ncsi_rsp_pkt_hdr *rnh)
> +{
> +    struct ncsi_rsp_gp_pkt *rsp = (struct ncsi_rsp_gp_pkt *) rnh;
> +
> +    rsp->mac_cnt = 0x0;

"MAC Address Count: The number of MAC addresses supported by the channel"

0 means "no [MAC Address] filter [in use]"

> +    rsp->mac_enable = 0x0;

"MAC Address Flags: all [MAC Address filters] disabled" ...

> +
> +    /* TODO: provide the guest configured MAC */
> +    rsp->mac[0] = 0xaa;
> +    rsp->mac[1] = 0xbb;
> +    rsp->mac[2] = 0xcc;
> +    rsp->mac[3] = 0xdd;
> +    rsp->mac[4] = 0xee;
> +    rsp->mac[5] = 0xff;

So there is no need to set this filter,
or you have to set:

    rsp->mac_cnt = 1;
    rsp->mac_enable = 1 << 0;

Also, you forgot to set:

    rsp->vlan_cnt = 0;
    rsp->vlan_enable = 0;

> +
> +    /* more data follows */
> +
> +    return 0;
> +}
> +
>  static const struct ncsi_rsp_handler {
>          unsigned char   type;
>          int             payload;
> @@ -62,7 +83,7 @@ static const struct ncsi_rsp_handler {
>          { NCSI_PKT_RSP_SNFC,    4, NULL },
>          { NCSI_PKT_RSP_GVI,    40, NULL },
>          { NCSI_PKT_RSP_GC,     32, ncsi_rsp_handler_gc },
> -        { NCSI_PKT_RSP_GP,     -1, NULL },
> +        { NCSI_PKT_RSP_GP,     40, ncsi_rsp_handler_gp },
>          { NCSI_PKT_RSP_GCPS,  172, NULL },
>          { NCSI_PKT_RSP_GNS,   172, NULL },
>          { NCSI_PKT_RSP_GNPTS, 172, NULL },
> 

Re: [Qemu-devel] [PATCH 6/6] slirp/ncsi: add checksum support

[Philippe Mathieu-Daudé]

On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
> The checksum field of a NC-SI packet contains a value that may be
> included in each command and response. The verification is optional
> but the Linux driver does so when a non-zero value is provided. Let's
> extend the model to compute the checksum value and exercise a little
> more the Linux driver.

Due to
- "If the originator of an NC-SI Control packet is not generating a
checksum, the originator shall use a value of all zeros for the header
checksum field."
- "All receivers of NC-SI Control packets shall accept packets with all
zeros as the checksum value (provided that other fields and the CRC are
correct)."
- "The receiver of an NC-SI Control packet may reject (silently discard)
a packet that has an incorrect non-zero checksum."

I was tempted to suggest you to check the input checksum if any, but
"A controller that generates checksums is not required to verify
checksums that it receives." so we don't care.

> 
> See section "8.2.2.3 - 2's Complement Checksum Compensation" in the
> Network Controller Sideband Interface (NC-SI) Specification for more
> details.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  slirp/ncsi.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/slirp/ncsi.c b/slirp/ncsi.c
> index f00253641ea4..fbfb79713f23 100644
> --- a/slirp/ncsi.c
> +++ b/slirp/ncsi.c
> @@ -8,9 +8,23 @@
>   */
>  #include "qemu/osdep.h"
>  #include "slirp.h"
> +#include "net/checksum.h"
>  
>  #include "ncsi-pkt.h"
>  
> +static uint32_t ncsi_calculate_checksum(unsigned char *data, int len)

"NC-SI packet payload interpreted as a series of 16-bit unsigned integer
values."

Can you use the "uint32_t ncsi_calculate_checksum(uint16_t *data, size_t
length)" prototype instead if you respin?

Regardless:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +{
> +    uint32_t checksum = 0;
> +    int i;
> +
> +    for (i = 0; i < len; i += 2) {
> +        checksum += (((uint32_t)data[i] << 8) | data[i + 1]);
> +    }
> +
> +    checksum = (~checksum + 1);
> +    return checksum;
> +}
> +
>  /* Get Capabilities */
>  static int ncsi_rsp_handler_gc(struct ncsi_rsp_pkt_hdr *rnh)
>  {
> @@ -108,6 +122,9 @@ void ncsi_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>          (ncsi_reply + ETH_HLEN);
>      const struct ncsi_rsp_handler *handler = NULL;
>      int i;
> +    int ncsi_rsp_len = sizeof(*nh);
> +    uint32_t checksum;
> +    uint32_t *pchecksum;
>  
>      memset(ncsi_reply, 0, sizeof(ncsi_reply));
>  
> @@ -137,15 +154,19 @@ void ncsi_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>              /* TODO: handle errors */
>              handler->handler(rnh);
>          }
> +        ncsi_rsp_len += handler->payload;
>      } else {
>          rnh->common.length = 0;
>          rnh->code          = htons(NCSI_PKT_RSP_C_UNAVAILABLE);
>          rnh->reason        = htons(NCSI_PKT_RSP_R_UNKNOWN);
>      }
>  
> -    /* TODO: add a checksum at the end of the frame but the specs
> -     * allows it to be zero */
> +    /* add a checksum at the end of the frame (the specs allows it to
> +     * be zero */
> +    checksum = ncsi_calculate_checksum((unsigned char *) rnh, ncsi_rsp_len);
> +    pchecksum = (uint32_t *)((void *) rnh + ncsi_rsp_len);
> +    *pchecksum = htonl(checksum);
> +    ncsi_rsp_len += 4;
>  
> -    slirp_output(slirp->opaque, ncsi_reply, ETH_HLEN + sizeof(*nh) +
> -                 (handler ? handler->payload : 0) + 4);
> +    slirp_output(slirp->opaque, ncsi_reply, ETH_HLEN + ncsi_rsp_len);
>  }
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] ftgmac100: compute maximum frame size depending on the protocol

[Philippe Mathieu-Daudé]

On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
> The maximum frame size includes the CRC and depends if a VLAN tag is
> inserted or not. Adjust the frame size limit in the transmit handler
> using on the FTGMAC100State buffer size and in the receive handler use
> the packet protocol.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/net/ftgmac100.h |  7 ++++++-
>  hw/net/ftgmac100.c         | 23 ++++++++++++-----------
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
> index d9bc589fbf72..94cfe0533297 100644
> --- a/include/hw/net/ftgmac100.h
> +++ b/include/hw/net/ftgmac100.h
> @@ -16,6 +16,11 @@
>  #include "hw/sysbus.h"
>  #include "net/net.h"
>  
> +/*
> + * Max frame size for the receiving buffer
> + */
> +#define FTGMAC100_MAX_FRAME_SIZE    9220
> +
>  typedef struct FTGMAC100State {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -26,7 +31,7 @@ typedef struct FTGMAC100State {
>      qemu_irq irq;
>      MemoryRegion iomem;
>  
> -    uint8_t *frame;
> +    uint8_t frame[FTGMAC100_MAX_FRAME_SIZE];
>  
>      uint32_t irq_state;
>      uint32_t isr;
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index a6d27a7b01df..7598d08c9cb9 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -207,16 +207,18 @@ typedef struct {
>  /*
>   * Max frame size for the receiving buffer
>   */
> -#define FTGMAC100_MAX_FRAME_SIZE    10240
> +#define FTGMAC100_MAX_FRAME_SIZE    9220

Sounds correct, (MAX(1518, 9216) + 4 /* vlan */ ).

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>  
>  /* Limits depending on the type of the frame
>   *
>   *   9216 for Jumbo frames (+ 4 for VLAN)
>   *   1518 for other frames (+ 4 for VLAN)
>   */
> -static int ftgmac100_max_frame_size(FTGMAC100State *s)
> +static int ftgmac100_max_frame_size(FTGMAC100State *s, uint16_t proto)
>  {
> -    return (s->maccr & FTGMAC100_MACCR_JUMBO_LF ? 9216 : 1518) + 4;
> +    int max = (s->maccr & FTGMAC100_MACCR_JUMBO_LF ? 9216 : 1518);
> +
> +    return max + (proto == ETH_P_VLAN ? 4 : 0);
>  }
>  
>  static void ftgmac100_update_irq(FTGMAC100State *s)
> @@ -408,7 +410,6 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>      uint8_t *ptr = s->frame;
>      uint32_t addr = tx_descriptor;
>      uint32_t flags = 0;
> -    int max_frame_size = ftgmac100_max_frame_size(s);
>  
>      while (1) {
>          FTGMAC100Desc bd;
> @@ -427,11 +428,12 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>              flags = bd.des1;
>          }
>  
> -        len = bd.des0 & 0x3FFF;
> -        if (frame_size + len > max_frame_size) {
> +        len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
> +        if (frame_size + len > sizeof(s->frame)) {
>              qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
>                            __func__, len);
> -            len = max_frame_size - frame_size;
> +            s->isr |= FTGMAC100_INT_XPKT_LOST;
> +            len =  sizeof(s->frame) - frame_size;
>          }
>  
>          if (dma_memory_read(&address_space_memory, bd.des3, ptr, len)) {
> @@ -788,7 +790,8 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>      uint32_t buf_len;
>      size_t size = len;
>      uint32_t first = FTGMAC100_RXDES0_FRS;
> -    int max_frame_size = ftgmac100_max_frame_size(s);
> +    uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
> +    int max_frame_size = ftgmac100_max_frame_size(s, proto);
>  
>      if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN))
>           != (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN)) {
> @@ -814,9 +817,9 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>  
>      /* Huge frames are truncated.  */
>      if (size > max_frame_size) {
> -        size = max_frame_size;
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %zd bytes\n",
>                        __func__, size);
> +        size = max_frame_size;
>          flags |= FTGMAC100_RXDES0_FTL;
>      }
>  
> @@ -934,8 +937,6 @@ static void ftgmac100_realize(DeviceState *dev, Error **errp)
>                            object_get_typename(OBJECT(dev)), DEVICE(dev)->id,
>                            s);
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> -
> -    s->frame = g_malloc(FTGMAC100_MAX_FRAME_SIZE);
>  }
>  
>  static const VMStateDescription vmstate_ftgmac100 = {
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/6] ftgmac100: add IEEE 802.1Q VLAN support

[Philippe Mathieu-Daudé]

Hi Cédric,

On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
> The ftgmac100 NIC supports VLAN tag insertion and the MAC engine also
> has a control to remove VLAN tags from received packets.
> 
> The VLAN control bits and VLAN tag information are contained in the
> second word of the transmit and receive descriptors. The Insert VLAN
> bit and the VLAN Tag available bit are only valid in the first segment
> of the packet.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/net/ftgmac100.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 7598d08c9cb9..50af1222464a 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -443,6 +443,24 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>              break;
>          }
>  
> +        /* Check for VLAN */
> +        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
> +            bd.des1 & FTGMAC100_TXDES1_INS_VLANTAG &&
> +            be16_to_cpu(PKT_GET_ETH_HDR(ptr)->h_proto) != ETH_P_VLAN) {
> +            if (frame_size + len + 4 > sizeof(s->frame)) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
> +                              __func__, len);
> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
> +                len =  sizeof(s->frame) - frame_size - 4;
> +            }
> +            memmove(ptr + 16, ptr + 12, len - 12);
> +            ptr[12] = 0x81;
> +            ptr[13] = 0x00;

               stw_be_p(ptr + 12, ETH_P_VLAN);

> +            ptr[14] = (uint8_t) bd.des1 >> 8;
> +            ptr[15] = (uint8_t) bd.des1;

               stw_be_p(ptr + 12, bd.des1);

> +            len += 4;
> +        }
> +
>          ptr += len;
>          frame_size += len;
>          if (bd.des0 & FTGMAC100_TXDES0_LTS) {
> @@ -858,7 +876,19 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>              buf_len += size - 4;
>          }
>          buf_addr = bd.des3;
> -        dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
> +        if (first && proto == ETH_P_VLAN && buf_len >= 18) {
> +            bd.des1 = (buf[14] << 8) | buf[15] | FTGMAC100_RXDES1_VLANTAG_AVAIL;

               bd.des1 = lduw_be_p(buf + 14) |
FTGMAC100_RXDES1_VLANTAG_AVAIL;

> +            if (s->maccr & FTGMAC100_MACCR_RM_VLAN) {
> +                dma_memory_write(&address_space_memory, buf_addr, buf, 12);
> +                dma_memory_write(&address_space_memory, buf_addr + 12, buf + 16,
> +                                 buf_len - 16);
> +            } else {
> +                dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
> +            }
> +        } else {
> +            bd.des1 = 0;
> +            dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
> +        }
>          buf += buf_len;
>          if (size < 4) {
>              dma_memory_write(&address_space_memory, buf_addr + buf_len,
> 

With ldst API uses:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/6] ftgmac100: add IEEE 802.1Q VLAN support

[Cédric Le Goater]

On 05/29/2018 03:34 PM, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
>> The ftgmac100 NIC supports VLAN tag insertion and the MAC engine also
>> has a control to remove VLAN tags from received packets.
>>
>> The VLAN control bits and VLAN tag information are contained in the
>> second word of the transmit and receive descriptors. The Insert VLAN
>> bit and the VLAN Tag available bit are only valid in the first segment
>> of the packet.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/net/ftgmac100.c | 32 +++++++++++++++++++++++++++++++-
>>  1 file changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 7598d08c9cb9..50af1222464a 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -443,6 +443,24 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>              break;
>>          }
>>  
>> +        /* Check for VLAN */
>> +        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
>> +            bd.des1 & FTGMAC100_TXDES1_INS_VLANTAG &&
>> +            be16_to_cpu(PKT_GET_ETH_HDR(ptr)->h_proto) != ETH_P_VLAN) {
>> +            if (frame_size + len + 4 > sizeof(s->frame)) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
>> +                              __func__, len);
>> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
>> +                len =  sizeof(s->frame) - frame_size - 4;
>> +            }
>> +            memmove(ptr + 16, ptr + 12, len - 12);
>> +            ptr[12] = 0x81;
>> +            ptr[13] = 0x00;
> 
>                stw_be_p(ptr + 12, ETH_P_VLAN);
> 
>> +            ptr[14] = (uint8_t) bd.des1 >> 8;
>> +            ptr[15] = (uint8_t) bd.des1;
> 
>                stw_be_p(ptr + 12, bd.des1);

ptr + 14

>> +            len += 4;
>> +        }
>> +
>>          ptr += len;
>>          frame_size += len;
>>          if (bd.des0 & FTGMAC100_TXDES0_LTS) {
>> @@ -858,7 +876,19 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>>              buf_len += size - 4;
>>          }
>>          buf_addr = bd.des3;
>> -        dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>> +        if (first && proto == ETH_P_VLAN && buf_len >= 18) {
>> +            bd.des1 = (buf[14] << 8) | buf[15] | FTGMAC100_RXDES1_VLANTAG_AVAIL;
> 
>                bd.des1 = lduw_be_p(buf + 14) |
> FTGMAC100_RXDES1_VLANTAG_AVAIL;
> 
>> +            if (s->maccr & FTGMAC100_MACCR_RM_VLAN) {
>> +                dma_memory_write(&address_space_memory, buf_addr, buf, 12);
>> +                dma_memory_write(&address_space_memory, buf_addr + 12, buf + 16,
>> +                                 buf_len - 16);
>> +            } else {
>> +                dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>> +            }
>> +        } else {
>> +            bd.des1 = 0;
>> +            dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>> +        }
>>          buf += buf_len;
>>          if (size < 4) {
>>              dma_memory_write(&address_space_memory, buf_addr + buf_len,
>>
> 
> With ldst API uses:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 

Sure, I will fix these.

Thanks,

C. 

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] ftgmac100 and NC-SI enhancements for the Aspeed SoC

[Cédric Le Goater]

On 05/29/2018 02:34 PM, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
>> Hello,
>>
>> Here is a couple of enhancements and fixes for the ftgmac100 NIC used
>> on the Aspeed SoC. It includes VLAN and multicast support.
>>
>> Following is an assorted set of changes for the NC-SI backend also
>> used on the Aspeed SoC when soldered on OpenPOWER boards. These fix a
>> few problems found when running OpenBMC on aspeed QEMU machines.
>>
>> Thanks,
>>
>> Cédric.
>>
>> Cédric Le Goater (6):
>>   ftgmac100: compute maximum frame size depending on the protocol
>>   ftgmac100: add IEEE 802.1Q VLAN support
>>   net/ftgmac100: fix multicast hash routine
>>   slirp/ncsi: fix "Get Version ID" payload length
>>   slirp/ncsi: add a "Get Parameter" response
>>   slirp/ncsi: add checksum support
> 
> Doesn't it make more sens to have slirp patches applied previous to the
> ftgmac100 model?

why ? They are not necessarily dependent. 

C.

> 
>>
>>  include/hw/net/ftgmac100.h |  7 +++++-
>>  hw/net/ftgmac100.c         | 59 +++++++++++++++++++++++++++++++++++-----------
>>  slirp/ncsi.c               | 54 +++++++++++++++++++++++++++++++++++++-----
>>  3 files changed, 99 insertions(+), 21 deletions(-)
>>

Re: [Qemu-devel] [PATCH 5/6] slirp/ncsi: add a "Get Parameter" response

[Cédric Le Goater]

On 05/29/2018 03:11 PM, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> In title: "Get Parameters" (plural)
> 
> On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
>> This is a minimum response to exercise the kernel.
> 
> This type is mandatory in DMTF Standard v1.0.1 indeed (DSP0222).
> 
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  slirp/ncsi.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/slirp/ncsi.c b/slirp/ncsi.c
>> index 02d0e9def3e8..f00253641ea4 100644
>> --- a/slirp/ncsi.c
>> +++ b/slirp/ncsi.c
>> @@ -35,6 +35,27 @@ static int ncsi_rsp_handler_gls(struct ncsi_rsp_pkt_hdr *rnh)
>>      return 0;
>>  }
>>  
>> +/* Get Parameter */
> 
> "Parameters"
> 
>> +static int ncsi_rsp_handler_gp(struct ncsi_rsp_pkt_hdr *rnh)
>> +{
>> +    struct ncsi_rsp_gp_pkt *rsp = (struct ncsi_rsp_gp_pkt *) rnh;
>> +
>> +    rsp->mac_cnt = 0x0;
> 
> "MAC Address Count: The number of MAC addresses supported by the channel"
> 
> 0 means "no [MAC Address] filter [in use]"
> 
>> +    rsp->mac_enable = 0x0;
> 
> "MAC Address Flags: all [MAC Address filters] disabled" ...
> 
>> +
>> +    /* TODO: provide the guest configured MAC */
>> +    rsp->mac[0] = 0xaa;
>> +    rsp->mac[1] = 0xbb;
>> +    rsp->mac[2] = 0xcc;
>> +    rsp->mac[3] = 0xdd;
>> +    rsp->mac[4] = 0xee;
>> +    rsp->mac[5] = 0xff;
> 
> So there is no need to set this filter,
> or you have to set:
> 
>     rsp->mac_cnt = 1;
>     rsp->mac_enable = 1 << 0;
> 
> Also, you forgot to set:
> 
>     rsp->vlan_cnt = 0;
>     rsp->vlan_enable = 0;


yes. I will clean up the handler.

Thanks,

C.

>> +
>> +    /* more data follows */
>> +
>> +    return 0;
>> +}
>> +
>>  static const struct ncsi_rsp_handler {
>>          unsigned char   type;
>>          int             payload;
>> @@ -62,7 +83,7 @@ static const struct ncsi_rsp_handler {
>>          { NCSI_PKT_RSP_SNFC,    4, NULL },
>>          { NCSI_PKT_RSP_GVI,    40, NULL },
>>          { NCSI_PKT_RSP_GC,     32, ncsi_rsp_handler_gc },
>> -        { NCSI_PKT_RSP_GP,     -1, NULL },
>> +        { NCSI_PKT_RSP_GP,     40, ncsi_rsp_handler_gp },
>>          { NCSI_PKT_RSP_GCPS,  172, NULL },
>>          { NCSI_PKT_RSP_GNS,   172, NULL },
>>          { NCSI_PKT_RSP_GNPTS, 172, NULL },
>>

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/6] ftgmac100: add IEEE 802.1Q VLAN support

[Philippe Mathieu-Daudé]

On 05/29/2018 01:56 PM, Cédric Le Goater wrote:
> On 05/29/2018 03:34 PM, Philippe Mathieu-Daudé wrote:
>> Hi Cédric,
>>
>> On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
>>> The ftgmac100 NIC supports VLAN tag insertion and the MAC engine also
>>> has a control to remove VLAN tags from received packets.
>>>
>>> The VLAN control bits and VLAN tag information are contained in the
>>> second word of the transmit and receive descriptors. The Insert VLAN
>>> bit and the VLAN Tag available bit are only valid in the first segment
>>> of the packet.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  hw/net/ftgmac100.c | 32 +++++++++++++++++++++++++++++++-
>>>  1 file changed, 31 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>>> index 7598d08c9cb9..50af1222464a 100644
>>> --- a/hw/net/ftgmac100.c
>>> +++ b/hw/net/ftgmac100.c
>>> @@ -443,6 +443,24 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>>              break;
>>>          }
>>>  
>>> +        /* Check for VLAN */
>>> +        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
>>> +            bd.des1 & FTGMAC100_TXDES1_INS_VLANTAG &&
>>> +            be16_to_cpu(PKT_GET_ETH_HDR(ptr)->h_proto) != ETH_P_VLAN) {
>>> +            if (frame_size + len + 4 > sizeof(s->frame)) {
>>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too big : %d bytes\n",
>>> +                              __func__, len);
>>> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
>>> +                len =  sizeof(s->frame) - frame_size - 4;
>>> +            }
>>> +            memmove(ptr + 16, ptr + 12, len - 12);
>>> +            ptr[12] = 0x81;
>>> +            ptr[13] = 0x00;
>>
>>                stw_be_p(ptr + 12, ETH_P_VLAN);
>>
>>> +            ptr[14] = (uint8_t) bd.des1 >> 8;
>>> +            ptr[15] = (uint8_t) bd.des1;
>>
>>                stw_be_p(ptr + 12, bd.des1);
> 
> ptr + 14

copy/paste mistake ;)

>>> +            len += 4;
>>> +        }
>>> +
>>>          ptr += len;
>>>          frame_size += len;
>>>          if (bd.des0 & FTGMAC100_TXDES0_LTS) {
>>> @@ -858,7 +876,19 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
>>>              buf_len += size - 4;
>>>          }
>>>          buf_addr = bd.des3;
>>> -        dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>>> +        if (first && proto == ETH_P_VLAN && buf_len >= 18) {
>>> +            bd.des1 = (buf[14] << 8) | buf[15] | FTGMAC100_RXDES1_VLANTAG_AVAIL;
>>
>>                bd.des1 = lduw_be_p(buf + 14) |
>> FTGMAC100_RXDES1_VLANTAG_AVAIL;
>>
>>> +            if (s->maccr & FTGMAC100_MACCR_RM_VLAN) {
>>> +                dma_memory_write(&address_space_memory, buf_addr, buf, 12);
>>> +                dma_memory_write(&address_space_memory, buf_addr + 12, buf + 16,
>>> +                                 buf_len - 16);
>>> +            } else {
>>> +                dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>>> +            }
>>> +        } else {
>>> +            bd.des1 = 0;
>>> +            dma_memory_write(&address_space_memory, buf_addr, buf, buf_len);
>>> +        }
>>>          buf += buf_len;
>>>          if (size < 4) {
>>>              dma_memory_write(&address_space_memory, buf_addr + buf_len,
>>>
>>
>> With ldst API uses:
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
> 
> Sure, I will fix these.

Thanks!

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] ftgmac100 and NC-SI enhancements for the Aspeed SoC

[Philippe Mathieu-Daudé]

On 05/29/2018 01:57 PM, Cédric Le Goater wrote:
> On 05/29/2018 02:34 PM, Philippe Mathieu-Daudé wrote:
>> Hi Cédric,
>>
>> On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
>>> Hello,
>>>
>>> Here is a couple of enhancements and fixes for the ftgmac100 NIC used
>>> on the Aspeed SoC. It includes VLAN and multicast support.
>>>
>>> Following is an assorted set of changes for the NC-SI backend also
>>> used on the Aspeed SoC when soldered on OpenPOWER boards. These fix a
>>> few problems found when running OpenBMC on aspeed QEMU machines.
>>>
>>> Thanks,
>>>
>>> Cédric.
>>>
>>> Cédric Le Goater (6):
>>>   ftgmac100: compute maximum frame size depending on the protocol
>>>   ftgmac100: add IEEE 802.1Q VLAN support
>>>   net/ftgmac100: fix multicast hash routine
>>>   slirp/ncsi: fix "Get Version ID" payload length
>>>   slirp/ncsi: add a "Get Parameter" response
>>>   slirp/ncsi: add checksum support
>>
>> Doesn't it make more sens to have slirp patches applied previous to the
>> ftgmac100 model?
> 
> why ? They are not necessarily dependent. 

OK, no worries.

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] ftgmac100 and NC-SI enhancements for the Aspeed SoC

[Cédric Le Goater]

On 05/30/2018 05:04 AM, Philippe Mathieu-Daudé wrote:
> On 05/29/2018 01:57 PM, Cédric Le Goater wrote:
>> On 05/29/2018 02:34 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Cédric,
>>>
>>> On 05/29/2018 03:28 AM, Cédric Le Goater wrote:
>>>> Hello,
>>>>
>>>> Here is a couple of enhancements and fixes for the ftgmac100 NIC used
>>>> on the Aspeed SoC. It includes VLAN and multicast support.
>>>>
>>>> Following is an assorted set of changes for the NC-SI backend also
>>>> used on the Aspeed SoC when soldered on OpenPOWER boards. These fix a
>>>> few problems found when running OpenBMC on aspeed QEMU machines.
>>>>
>>>> Thanks,
>>>>
>>>> Cédric.
>>>>
>>>> Cédric Le Goater (6):
>>>>   ftgmac100: compute maximum frame size depending on the protocol
>>>>   ftgmac100: add IEEE 802.1Q VLAN support
>>>>   net/ftgmac100: fix multicast hash routine
>>>>   slirp/ncsi: fix "Get Version ID" payload length
>>>>   slirp/ncsi: add a "Get Parameter" response
>>>>   slirp/ncsi: add checksum support
>>>
>>> Doesn't it make more sens to have slirp patches applied previous to the
>>> ftgmac100 model?
>>
>> why ? They are not necessarily dependent. 
> 
> OK, no worries.
> 

bah, I will just resend two distinct series one for 'slirp' 
and one for 'net'.

C.  

Re: [Qemu-devel] [PATCH 0/6] ftgmac100 and NC-SI enhancements for the Aspeed SoC

[Joel Stanley]

On 29 May 2018 at 15:58, Cédric Le Goater <clg@kaod.org> wrote:
> Hello,
>
> Here is a couple of enhancements and fixes for the ftgmac100 NIC used
> on the Aspeed SoC. It includes VLAN and multicast support.
>
> Following is an assorted set of changes for the NC-SI backend also
> used on the Aspeed SoC when soldered on OpenPOWER boards. These fix a
> few problems found when running OpenBMC on aspeed QEMU machines.
>
> Thanks,
>
> Cédric.
>
> Cédric Le Goater (6):
>   ftgmac100: compute maximum frame size depending on the protocol
>   ftgmac100: add IEEE 802.1Q VLAN support
>   net/ftgmac100: fix multicast hash routine
>   slirp/ncsi: fix "Get Version ID" payload length
>   slirp/ncsi: add a "Get Parameter" response
>   slirp/ncsi: add checksum support

I tested these with -M palmetto-bmc and linux-next as the guest, and
networking appeared to work fine. For the series:

Tested-by: Joel Stanley <joel@jms.id.au>

One question I had. I get these messages on boot:

[    1.184472] ftgmac100 1e660000.ethernet: Read MAC address
52:54:00:12:34:56 from chip
[    1.184751] ftgmac100 1e660000.ethernet: Using NCSI interface
[    1.197107] ftgmac100 1e660000.ethernet eth0: irq 19, mapped at b30b8905
[    1.904237] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[    1.905682] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[    1.905943] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[    1.906182] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[    1.906415] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[    1.906643] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[    1.906873] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[    1.907103] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[    2.024094] ftgmac100 1e660000.ethernet eth0: NCSI: configuring channel 0
[    2.025428] ftgmac100 1e660000.ethernet eth0: NCSI: channel 0 config done
[    2.025544] ftgmac100 1e660000.ethernet eth0: NCSI: No more
channels to process
[    2.025608] ftgmac100 1e660000.ethernet eth0: NCSI interface up


Comparing to hardware, I see:

[    4.071159] ftgmac100 1e660000.ethernet: Read MAC address
98:be:94:83:00:ad from chip
[    4.079024] ftgmac100 1e660000.ethernet: Using NCSI interface
[    4.085976] ftgmac100 1e660000.ethernet eth0: irq 19, mapped at a0963000
[   40.670099] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
packet type 0x82 returned -19
[   43.852036] ftgmac100 1e660000.ethernet eth0: NCSI: configuring channel 1
[   43.877060] ftgmac100 1e660000.ethernet eth0: NCSI: channel 1 config done
[   43.877093] ftgmac100 1e660000.ethernet eth0: NCSI: No more
channels to process
[   43.877112] ftgmac100 1e660000.ethernet eth0: NCSI interface up

Is it expected that we get multiple ENODEVs for packet type 0x82 in qemu?

Cheers,

Joel

Re: [Qemu-devel] [PATCH 0/6] ftgmac100 and NC-SI enhancements for the Aspeed SoC

[Cédric Le Goater]

On 05/30/2018 07:35 AM, Joel Stanley wrote:
> On 29 May 2018 at 15:58, Cédric Le Goater <clg@kaod.org> wrote:
>> Hello,
>>
>> Here is a couple of enhancements and fixes for the ftgmac100 NIC used
>> on the Aspeed SoC. It includes VLAN and multicast support.
>>
>> Following is an assorted set of changes for the NC-SI backend also
>> used on the Aspeed SoC when soldered on OpenPOWER boards. These fix a
>> few problems found when running OpenBMC on aspeed QEMU machines.
>>
>> Thanks,
>>
>> Cédric.
>>
>> Cédric Le Goater (6):
>>   ftgmac100: compute maximum frame size depending on the protocol
>>   ftgmac100: add IEEE 802.1Q VLAN support
>>   net/ftgmac100: fix multicast hash routine
>>   slirp/ncsi: fix "Get Version ID" payload length
>>   slirp/ncsi: add a "Get Parameter" response
>>   slirp/ncsi: add checksum support
> 
> I tested these with -M palmetto-bmc and linux-next as the guest, and
> networking appeared to work fine. For the series:
> 
> Tested-by: Joel Stanley <joel@jms.id.au>
> 
> One question I had. I get these messages on boot:
> 
> [    1.184472] ftgmac100 1e660000.ethernet: Read MAC address
> 52:54:00:12:34:56 from chip
> [    1.184751] ftgmac100 1e660000.ethernet: Using NCSI interface
> [    1.197107] ftgmac100 1e660000.ethernet eth0: irq 19, mapped at b30b8905
> [    1.904237] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [    1.905682] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [    1.905943] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [    1.906182] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [    1.906415] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [    1.906643] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [    1.906873] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [    1.907103] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [    2.024094] ftgmac100 1e660000.ethernet eth0: NCSI: configuring channel 0
> [    2.025428] ftgmac100 1e660000.ethernet eth0: NCSI: channel 0 config done
> [    2.025544] ftgmac100 1e660000.ethernet eth0: NCSI: No more
> channels to process
> [    2.025608] ftgmac100 1e660000.ethernet eth0: NCSI interface up
> 
> 
> Comparing to hardware, I see:
> 
> [    4.071159] ftgmac100 1e660000.ethernet: Read MAC address
> 98:be:94:83:00:ad from chip
> [    4.079024] ftgmac100 1e660000.ethernet: Using NCSI interface
> [    4.085976] ftgmac100 1e660000.ethernet eth0: irq 19, mapped at a0963000
> [   40.670099] ftgmac100 1e660000.ethernet eth0: NCSI: Handler for
> packet type 0x82 returned -19
> [   43.852036] ftgmac100 1e660000.ethernet eth0: NCSI: configuring channel 1
> [   43.877060] ftgmac100 1e660000.ethernet eth0: NCSI: channel 1 config done
> [   43.877093] ftgmac100 1e660000.ethernet eth0: NCSI: No more
> channels to process
> [   43.877112] ftgmac100 1e660000.ethernet eth0: NCSI interface up
> 
> Is it expected that we get multiple ENODEVs for packet type 0x82 in qemu?
 
When the NSCI probing is done in Linux, 8 deselect packages requests
are sent and QEMU answers them all. I wonder what the real HW does.


Something to check.

Cheers,

C.