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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 7050 bytes --]

On Aug 30 15:31, Jonathan Cameron wrote:
> On Wed, 23 Aug 2023 11:21:59 +0200
> Klaus Jensen <its@irrelevant.dk> wrote:
> 
> > From: Klaus Jensen <k.jensen@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.Cameron@huawei.com/
> >   [2]: https://lore.kernel.org/qemu-devel/20221121080445.GA29062@codeconstruct.com.au/
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@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.Cameron@huawei.com>
> 
> 
> Some minor things inline.  With those tidied up or ignored with clear reasoning.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]