On Aug 30 15:31, Jonathan Cameron wrote: > On Wed, 23 Aug 2023 11:21:59 +0200 > Klaus Jensen <[email protected]> wrote: > > > From: Klaus Jensen <[email protected]> > > > > 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/[email protected]/ > > [2]: > > https://lore.kernel.org/qemu-devel/[email protected]/ > > > > Signed-off-by: Klaus Jensen <[email protected]> > > I made the minor changes to the CXL FM-API PoC to use this and all works as > expected so > > Tested-by: Jonathan Cameron <[email protected]> > > > Some minor things inline. With those tidied up or ignored with clear > reasoning. > > Reviewed-by: Jonathan Cameron <[email protected]> > > > > 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
