On Aug 30 15:31, Jonathan Cameron wrote: > On Wed, 23 Aug 2023 11:21:59 +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> > > I made the minor changes to the CXL FM-API PoC to use this and all works as > expected so > > Tested-by: Jonathan Cameron <jonathan.came...@huawei.com> > > > Some minor things inline. With those tidied up or ignored with clear > reasoning. > > Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com> > > > > diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c > > new file mode 100644 > > index 000000000000..217073d62435 > > --- /dev/null > > +++ b/hw/i2c/mctp.c > > > ... > > > > +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, pktseq, msgtype; > > + > > + switch (event) { > > + case I2C_START_SEND: > > + if (mctp->state == I2C_MCTP_STATE_IDLE) { > > + mctp->state = I2C_MCTP_STATE_RX_STARTED; > > + } else if (mctp->state != I2C_MCTP_STATE_RX) { > > + return -1; > > + } > > + > > + /* the i2c core eats the slave address, so put it back in */ > > + pkt->i2c.dest = i2c->address << 1; > > + mctp->len = 1; > > + > > + return 0; > > + > > + case I2C_FINISH: > > + if (mctp->len < sizeof(MCTPI2CPacket) + 1) { > > + trace_i2c_mctp_drop_short_packet(mctp->len); > > + goto drop; > > + } > > + > > + 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 || > > + pkt->mctp.hdr.eid.dest == 0)) { > > + trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest, > > + mctp->my_eid); > > + goto drop; > > + } > > + > > + pktseq = FIELD_EX8(pkt->mctp.hdr.flags, MCTP_H_FLAGS, PKTSEQ); > > + > > + if (FIELD_EX8(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 >> 1; > > + mctp->tx.eid = pkt->mctp.hdr.eid.source; > > + mctp->tx.tag = FIELD_EX8(pkt->mctp.hdr.flags, MCTP_H_FLAGS, > > TAG); > > + mctp->tx.pktseq = pktseq; > > + > > + msgtype = FIELD_EX8(pkt->mctp.payload[0], MCTP_MESSAGE_H, > > TYPE); > > + > > + if (msgtype == 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 (pktseq != (++mctp->tx.pktseq & 0x3)) { > > + trace_i2c_mctp_drop_invalid_pktseq(pktseq, mctp->tx.pktseq & > > 0x3); > > + goto drop; > > + } > > + > > + mc->put_buf(mctp, i2c_mctp_payload(mctp->buffer), payload_len); > > Given this returns -1 on error should probably handle errors. >
Yes. The event callback should only potentially return -1 in the case of I2C_START_SEND, so I just do a `goto drop` here. > > > + > > + if (FIELD_EX8(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..fccbf249cdbe > > --- /dev/null > > +++ b/include/hw/i2c/mctp.h > > @@ -0,0 +1,127 @@ > > +#ifndef QEMU_I2C_MCTP_H > > +#define QEMU_I2C_MCTP_H > > + > > +#include "qom/object.h" > > +#include "hw/qdev-core.h" > > + > > +#define TYPE_MCTP_I2C_ENDPOINT "mctp-i2c-endpoint" > > +OBJECT_DECLARE_TYPE(MCTPI2CEndpoint, MCTPI2CEndpointClass, > > MCTP_I2C_ENDPOINT) > > + > > +struct MCTPI2CEndpointClass { > > + I2CSlaveClass parent_class; > > + > > + /** > > + * > > Drop the blank line for consistency with the other comments. > > > + * put_buf() - receive incoming message fragment > > + * > > + * Must returns 0 for succes or -1 for error. > > I would expect any negative to count as error rather than just -1. > Also, simple imperative should be clear enough > * Return 0 for success or negative 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. > Subsequent call with return next fragment. > > Up to the implementation to decide how it does this. > > > + * > > + * Must return the number of bytes available. > Return number of bytes in message fragment. > > Available might mean in all fragments. > Thanks for these suggestions, I worked that in.
signature.asc
Description: PGP signature