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

[Jonathan Cameron]

On Wed, 31 May 2023 15:59:44 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 31 May 2023 13:47:43 +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>  
> Hi Klaus,
> 
> A few minor comments inline.
> 
> With those tidied up feel free to add
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@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)
> > +  
> 
>