On Wed, 31 May 2023 15:59:44 +0100 Jonathan Cameron <jonathan.came...@huawei.com> wrote:
> On Wed, 31 May 2023 13:47:43 +0200 > Klaus Jensen <i...@irrelevant.dk> wrote: > > > From: Klaus Jensen <k.jen...@samsung.com> > > > > 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. > > > > Parts of this implementation is inspired by code[1] previously posted by > > Jonathan Cameron. > > > > Squashed a fix[2] from Matt Johnston. > > > > [1]: > > https://lore.kernel.org/qemu-devel/20220520170128.4436-1-jonathan.came...@huawei.com/ > > [2]: > > https://lore.kernel.org/qemu-devel/20221121080445.ga29...@codeconstruct.com.au/ > > > > Signed-off-by: Klaus Jensen <k.jen...@samsung.com> > Hi Klaus, > > A few minor comments inline. > > With those tidied up feel free to add > Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com> > > > > > + > > + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) { > > + mctp->tx.is_control = false; > > + > > + if (mctp->state == I2C_MCTP_STATE_RX) { > > + mc->reset(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; > > Maybe worth either defining that mask, or adding a comment that this > is copying the msg tag. Or rename flags... > > > > + 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)) { > > + trace_i2c_mctp_drop_invalid_pktseq((pkt->mctp.hdr.flags >> 4) > > & 0x3, > > + mctp->tx.pktseq & 0x3); > > + goto drop; > > + } > > + > > + mc->put_buf(mctp, i2c_mctp_payload(mctp->buffer), payload_len); > > + > > + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_EOM) { > > + mc->handle(mctp); > > + mctp->state = I2C_MCTP_STATE_WAIT_TX; > > + } > > + > > + return 0; > > + > > + default: > > + return -1; > > + } > > + > > +drop: > > + mc->reset(mctp); > > + > > + mctp->state = I2C_MCTP_STATE_IDLE; > > + > > + return 0; > > +} > > > > diff --git a/include/hw/i2c/mctp.h b/include/hw/i2c/mctp.h > > new file mode 100644 > > index 000000000000..ea97792e8d43 > > --- /dev/null > > +++ b/include/hw/i2c/mctp.h > > ... > > +struct MCTPI2CEndpointClass { > > + I2CSlaveClass parent_class; > > + > > + /** > > + * > > + * put_buf() - receive incoming message fragment > > + * > > + * Must returns 0 for succes or -1 for error. > > + */ > > + int (*put_buf)(MCTPI2CEndpoint *mctp, uint8_t *buf, size_t len); > > + > > + /** > > + * get_buf() - provide pointer to message fragment > > + * > > + * Called by the mctp subsystem to request a pointer to the next > > message > > + * fragment. The implementation must advance its internal position such > > + * that successive calls returns the next fragments. > > + * > > + * Must return the number of bytes available. > > + */ > > + size_t (*get_buf)(MCTPI2CEndpoint *mctp, const uint8_t **buf, > > + size_t maxlen, uint8_t *mctp_flags); > > + > > + /** > > + * handle() - handle an MCTP message > > + * > > + * Called by the mctp subsystem when a full message has been delivered > > and > > + * may be parsed and processed. > > + */ > > + void (*handle)(MCTPI2CEndpoint *mctp); > > + > > + /** > > + * reset() - reset internal state > > + * > > + * Called by the mctp subsystem in the event of some transport error. > > + * Implementation must reset its internal state and drop any fragments > > + * previously receieved. > > + */ > > + void (*reset)(MCTPI2CEndpoint *mctp); > > + > > + /** > > + * get_types() - provide supported mctp message types > > + * > > + * Must provide a buffer with a full MCTP supported message types > > payload > > + * (i.e. `0x0(SUCCESS),0x1(ONE),0x4(NMI)`). > > ONE? Looks to be PLDM > Good to reference DSP0239 Management Component Transport Protocol (MCTP) IDs > and Codes > which has the list. Ah. 0x1 is the length. Call it "type count" instead of ONE and make it obvious. > > > + * > > + * Returns the size of the response. > > + */ > > + size_t (*get_types)(MCTPI2CEndpoint *mctp, const uint8_t **data); > > +}; > > > diff --git a/include/net/mctp.h b/include/net/mctp.h > > new file mode 100644 > > index 000000000000..70b49235ddb2 > > --- /dev/null > > +++ b/include/net/mctp.h > > @@ -0,0 +1,28 @@ > > +#ifndef QEMU_MCTP_H > > +#define QEMU_MCTP_H > > + > > +#define MCTP_BASELINE_MTU 64 > > Ideally add a reference for this as well. > > 8.3.1 Baseline transmission unit in DSP0236 1.3.0 > > + > > +enum { > > + MCTP_H_FLAGS_EOM = 1 << 6, > > + MCTP_H_FLAGS_SOM = 1 << 7, > > Trivial: I'm not really seeing the enum here as useful vs > a pair of defines. > > > +}; > > + > > +#define MCTP_MESSAGE_IC (1 << 7) > > + > >
