Re: [PATCH RFC 2/3] hw/i2c: add mctp core

[Jeremy Kerr]

Hi Klaus,

> Add an abstract MCTP over I2C endpoint model. This implements MCTP
> control message handling as well as handling the actual I2C transport
> (packetization).
> 
> Devices are intended to derive from this and implement the class
> methods.

Looks good, nice to see how it's used by the nmi device later too.

A couple of issues with the state machine though, comments inline, and
a bit of a patch below.

> +static void i2c_mctp_tx(void *opaque)
> +{
> +    DeviceState *dev = DEVICE(opaque);
> +    I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(dev));
> +    I2CSlave *slave = I2C_SLAVE(dev);
> +    MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(dev);
> +    MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> +    MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> +    uint8_t flags = 0;
> +
> +    switch (mctp->tx.state) {
> +    case I2C_MCTP_STATE_TX_SEND_BYTE:
> +        if (mctp->pos < mctp->len) {
> +            uint8_t byte = mctp->buffer[mctp->pos];
> +
> +            trace_i2c_mctp_tx_send_byte(mctp->pos, byte);
> +
> +            /* send next byte */
> +            i2c_send_async(i2c, byte);
> +
> +            mctp->pos++;
> +
> +            break;
> +        }
> +
> +        /* packet sent */
> +        i2c_end_transfer(i2c);

If we're sending a control message, mctp->len will be set to the control
msg len here, then:

> +
> +        /* fall through */
> +
> +    case I2C_MCTP_STATE_TX_START_SEND:
> +        if (mctp->tx.is_control) {
> +            /* packet payload is already in buffer */
> +            flags |= MCTP_H_FLAGS_SOM | MCTP_H_FLAGS_EOM;
> +        } else {
> +            /* get message bytes from derived device */
> +            mctp->len = mc->get_message_bytes(mctp, pkt->mctp.payload,
> +                                              I2C_MCTP_MAXMTU, &flags);
> +        }

... it doesn't get cleared above, so:

> +
> +        if (!mctp->len) {

... we don't hit this conditional, and hence keep sending unlimited
bytes. This presents as continuous interrupts to the aspeed i2c driver
when replying to any control message.

I think we need a mctp->len = 0 with the i2c_end_transfer(). With that,
I can get control protocol communication working with mctpd.

> +static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
> +{
> +    MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
> +    MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> +    MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> +    size_t payload_len;
> +    uint8_t pec;
> +
> +    switch (event) {
> +    case I2C_START_SEND:
> +        if (mctp->state != I2C_MCTP_STATE_IDLE) {
> +            return -1;

mctp->state may (validly) be I2C_MCTP_STATE_RX here, if we're receiving
the start event for the second packet of a multi-packet message.

> +        }
> +
> +        /* the i2c core eats the slave address, so put it back in */
> +        pkt->i2c.dest = i2c->address << 1;
> +        mctp->len = 1;
> +
> +        mctp->state = I2C_MCTP_STATE_RX_STARTED;
> +
> +        return 0;
> +
> +    case I2C_FINISH:
> +        payload_len = mctp->len - (1 + offsetof(MCTPI2CPacket, 
> mctp.payload));
> +
> +        if (pkt->i2c.byte_count + 3 != mctp->len - 1) {
> +            trace_i2c_mctp_drop_invalid_length(pkt->i2c.byte_count +
> 3,
> +                                               mctp->len - 1);
> +            goto drop;
> +        }
> +
> +        pec = i2c_smbus_pec(0, mctp->buffer, mctp->len - 1);
> +        if (mctp->buffer[mctp->len - 1] != pec) {
> +            trace_i2c_mctp_drop_invalid_pec(mctp->buffer[mctp->len - 1], 
> pec);
> +            goto drop;
> +        }
> +
> +        if (pkt->mctp.hdr.eid.dest != mctp->my_eid) {
> +            trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest,
> +                                            mctp->my_eid);
> +            goto drop;
> +        }
> +
> +        if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) {
> +            mctp->tx.is_control = false;
> +
> +            if (mctp->state == I2C_MCTP_STATE_RX) {
> +                mc->reset_message(mctp);
> +            }
> +
> +            mctp->state = I2C_MCTP_STATE_RX;
> +
> +            mctp->tx.addr = pkt->i2c.source;
> +            mctp->tx.eid = pkt->mctp.hdr.eid.source;
> +            mctp->tx.flags = pkt->mctp.hdr.flags & 0x7;
> +            mctp->tx.pktseq = (pkt->mctp.hdr.flags >> 4) & 0x3;
> +
> +            if ((pkt->mctp.payload[0] & 0x7f) == MCTP_MESSAGE_TYPE_CONTROL) {
> +                mctp->tx.is_control = true;
> +
> +                i2c_mctp_handle_control(mctp);
> +
> +                return 0;
> +            }
> +        } else if (mctp->state == I2C_MCTP_STATE_RX_STARTED) {
> +            trace_i2c_mctp_drop("expected SOM");
> +            goto drop;
> +        } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (mctp->tx.pktseq++ 
> & 0x3)) {

The pktseq is the sequence number of the last packet seen, so you want a
pre-increment there: ++mctp->tx.pktseq & 0x3

        } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (++mctp->tx.pktseq & 
0x3)) {

With those changes, I can get control protocol going, and multi-packet
messages work. There's a couple of failures from unsupported commands,
but otherwise looks good:

  # mctp addr add 8 dev mctpi2c6
  # mctp link set mctpi2c6 up
  # mctp link set mctpi2c6 mtu 254
  # systemctl restart mctpd
  # busctl --no-pager call xyz.openbmc_project.MCTP \
    /xyz/openbmc_project/mctp au.com.CodeConstruct.MCTP \
    SetupEndpoint say mctpi2c6 1 0x1d
  yisb 9 1 "/xyz/openbmc_project/mctp/1/9" true
  # mctp route del 9 via mctpi2c6
  # mctp route add 9 via mctpi2c6 mtu 68
  # mi-mctp 1 9 info
  nmi message type 0x2 not handled
  Identify Controller failed, no quirks applied
  NVMe MI subsys info:
   num ports: 1
   major ver: 1
   minor ver: 1
  NVMe MI port info:
    port 0
      type SMBus[2]
      MCTP MTU: 64
      MEB size: 0
      SMBus address: 0x00
      VPD access freq: 0x00
      MCTP address: 0x1d
      MCTP access freq: 0x01
      NVMe basic management: disabled
  nmi command 0x1 not handled
  mi-mctp: can't perform Health Status Poll operation: Success
  # mi-mctp 1 9 get-config 0
  nmi message type 0x2 not handled
  Identify Controller failed, no quirks applied
  SMBus access frequency (port 0): 100k [0x1]
  MCTP MTU (port 0): 64

I've included a patch below, with some fixes for the above, in case that
helps.

Cheers,


Jeremy


diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c
index 46376de95a..1775deb46f 100644
--- a/hw/i2c/mctp.c
+++ b/hw/i2c/mctp.c
@@ -78,6 +78,9 @@ static void i2c_mctp_tx(void *opaque)
         /* packet sent */
         i2c_end_transfer(i2c);
 
+        /* end of any control data */
+        mctp->len = 0;
+
         /* fall through */
 
     case I2C_MCTP_STATE_TX_START_SEND:
@@ -228,7 +231,9 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event 
event)
 
     switch (event) {
     case I2C_START_SEND:
-        if (mctp->state != I2C_MCTP_STATE_IDLE) {
+        if (mctp->state == I2C_MCTP_STATE_IDLE) {
+            mctp->state = I2C_MCTP_STATE_RX_STARTED;
+        } else if (mctp->state != I2C_MCTP_STATE_RX) {
             return -1;
         }
 
@@ -236,8 +241,6 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event 
event)
         pkt->i2c.dest = i2c->address << 1;
         mctp->len = 1;
 
-        mctp->state = I2C_MCTP_STATE_RX_STARTED;
-
         return 0;
 
     case I2C_FINISH:
@@ -285,7 +288,7 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event 
event)
         } else if (mctp->state == I2C_MCTP_STATE_RX_STARTED) {
             trace_i2c_mctp_drop("expected SOM");
             goto drop;
-        } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (mctp->tx.pktseq++ & 
0x3)) {
+        } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (++mctp->tx.pktseq & 
0x3)) {
             trace_i2c_mctp_drop_invalid_pktseq((pkt->mctp.hdr.flags >> 4) & 
0x3,
                                                mctp->tx.pktseq & 0x3);
             goto drop;