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;
