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

[Klaus Jensen]

On Nov 16 08:27, Corey Minyard wrote:
> On Wed, Nov 16, 2022 at 09:43:11AM +0100, Klaus Jensen 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.
> 
> I have some comments inline, mostly about buffer handling.  Buffer
> handling is scary to me, so you might see some paranoia here :-).
> 

Totally understood :) Thanks for the review!

> > 
> >   [1]: 
> > https://lore.kernel.org/qemu-devel/20220520170128.4436-1-Jonathan.Cameron@huawei.com/
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/arm/Kconfig         |   1 +
> >  hw/i2c/Kconfig         |   4 +
> >  hw/i2c/mctp.c          | 365 +++++++++++++++++++++++++++++++++++++++++
> >  hw/i2c/meson.build     |   1 +
> >  hw/i2c/trace-events    |  12 ++
> >  include/hw/i2c/mctp.h  |  83 ++++++++++
> >  include/hw/misc/mctp.h |  43 +++++
> >  7 files changed, 509 insertions(+)
> >  create mode 100644 hw/i2c/mctp.c
> >  create mode 100644 include/hw/i2c/mctp.h
> >  create mode 100644 include/hw/misc/mctp.h
> > 
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 17fcde8e1ccc..3233bdc193d7 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -444,6 +444,7 @@ config ASPEED_SOC
> >      select DS1338
> >      select FTGMAC100
> >      select I2C
> > +    select MCTP_I2C
> >      select DPS310
> >      select PCA9552
> >      select SERIAL
> > diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> > index 9bb8870517f8..5dd43d550c32 100644
> > --- a/hw/i2c/Kconfig
> > +++ b/hw/i2c/Kconfig
> > @@ -41,3 +41,7 @@ config PCA954X
> >  config PMBUS
> >      bool
> >      select SMBUS
> > +
> > +config MCTP_I2C
> > +    bool
> > +    select I2C
> > diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c
> > new file mode 100644
> > index 000000000000..46376de95a98
> > --- /dev/null
> > +++ b/hw/i2c/mctp.c
> > @@ -0,0 +1,365 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + * SPDX-FileCopyrightText: Copyright (c) 2022 Samsung Electronics Co., Ltd.
> > + * SPDX-FileContributor: Klaus Jensen <k.jensen@samsung.com>
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/main-loop.h"
> > +
> > +#include "hw/qdev-properties.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "hw/i2c/mctp.h"
> > +
> > +#include "trace.h"
> > +
> > +static uint8_t crc8(uint16_t data)
> > +{
> > +#define POLY (0x1070U << 3)
> > +    int i;
> > +
> > +    for (i = 0; i < 8; i++) {
> > +        if (data & 0x8000) {
> > +            data = data ^ POLY;
> > +        }
> > +
> > +        data = data << 1;
> > +    }
> > +
> > +    return (uint8_t)(data >> 8);
> > +#undef POLY
> > +}
> > +
> > +static uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < len; i++) {
> > +        crc = crc8((crc ^ buf[i]) << 8);
> > +    }
> > +
> > +    return crc;
> > +}
> 
> The PEC calculation probably belongs in it's own smbus.c file, since
> it's generic, so someone looking will find it.
> 

Makes sense. I'll move it.

> > +
> > +void i2c_mctp_schedule_send(MCTPI2CEndpoint *mctp)
> > +{
> > +    I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(DEVICE(mctp)));
> > +
> > +    mctp->tx.state = I2C_MCTP_STATE_TX_START_SEND;
> > +
> > +    i2c_bus_master(i2c, mctp->tx.bh);
> > +}
> > +
> > +static void i2c_mctp_tx(void *opaque)
> > +{
> > +    DeviceState *dev = DEVICE(opaque);
> > +    I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(dev));
> > +    I2CSlave *slave = I2C_SLAVE(dev);
> > +    MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(dev);
> > +    MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > +    MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> > +    uint8_t flags = 0;
> > +
> > +    switch (mctp->tx.state) {
> > +    case I2C_MCTP_STATE_TX_SEND_BYTE:
> > +        if (mctp->pos < mctp->len) {
> > +            uint8_t byte = mctp->buffer[mctp->pos];
> > +
> > +            trace_i2c_mctp_tx_send_byte(mctp->pos, byte);
> > +
> > +            /* send next byte */
> > +            i2c_send_async(i2c, byte);
> > +
> > +            mctp->pos++;
> > +
> > +            break;
> > +        }
> > +
> > +        /* packet sent */
> > +        i2c_end_transfer(i2c);
> > +
> > +        /* fall through */
> > +
> > +    case I2C_MCTP_STATE_TX_START_SEND:
> > +        if (mctp->tx.is_control) {
> > +            /* packet payload is already in buffer */
> > +            flags |= MCTP_H_FLAGS_SOM | MCTP_H_FLAGS_EOM;
> > +        } else {
> > +            /* get message bytes from derived device */
> > +            mctp->len = mc->get_message_bytes(mctp, pkt->mctp.payload,
> > +                                              I2C_MCTP_MAXMTU, &flags);
> > +        }
> > +
> > +        if (!mctp->len) {
> > +            trace_i2c_mctp_tx_done();
> > +
> > +            /* no more packets needed; release the bus */
> > +            i2c_bus_release(i2c);
> > +
> > +            mctp->state = I2C_MCTP_STATE_IDLE;
> > +            mctp->tx.is_control = false;
> > +
> > +            break;
> > +        }
> > +
> > +        mctp->state = I2C_MCTP_STATE_TX;
> > +
> > +        pkt->i2c = (MCTPI2CPacketHeader) {
> > +            .dest = mctp->tx.addr & ~0x1,
> > +            .prot = 0xf,
> > +            .byte_count = 5 + mctp->len,
> > +            .source = slave->address << 1 | 0x1,
> > +        };
> > +
> > +        pkt->mctp.hdr = (MCTPPacketHeader) {
> > +            .version = 0x1,
> > +            .eid.dest = mctp->tx.eid,
> > +            .eid.source = mctp->my_eid,
> > +            .flags = flags | (mctp->tx.pktseq++ & 0x3) << 4 | 
> > mctp->tx.flags,
> > +        };
> > +
> > +        mctp->len += sizeof(MCTPI2CPacket);
> 
> Do you need overflow checking here?  There are lots of increments of
> mctp->len in the code that might or might not need overflow checks.
> It does seem like you have pre-calculated everything so it fits; I worry
> more about later changes that might violate those assumptions.
> You could use something like i2c_mctp_send_cb() to send all data.  Not
> sure, but something to think about.
> 

I agree. It would be better to be a bit defensive here. I'll rework it.

> > +        mctp->buffer[mctp->len] = i2c_smbus_pec(0, mctp->buffer, 
> > mctp->len);
> > +        mctp->len++;
> > +
> > +        trace_i2c_mctp_tx_start_send(mctp->len);
> > +
> > +        i2c_start_send_async(i2c, pkt->i2c.dest >> 1);
> > +
> > +        /* already "sent" the destination slave address */
> > +        mctp->pos = 1;
> > +
> > +        mctp->tx.state = I2C_MCTP_STATE_TX_SEND_BYTE;
> > +
> > +        break;
> > +    }
> > +}
> > +
> > +#define i2c_mctp_control_data(buf) \
> > +    (i2c_mctp_payload(buf) + offsetof(MCTPControlMessage, data))
> > +
> > +static void i2c_mctp_handle_control_set_eid(MCTPI2CEndpoint *mctp, uint8_t 
> > eid)
> > +{
> > +    mctp->my_eid = eid;
> > +
> > +    uint8_t buf[] = {
> > +        0x0, 0x0, eid, 0x0,
> > +    };
> > +
> > +    memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> > +    mctp->len += sizeof(buf);
> > +}
> > +
> > +static void i2c_mctp_handle_control_get_eid(MCTPI2CEndpoint *mctp)
> > +{
> > +    uint8_t buf[] = {
> > +        0x0, mctp->my_eid, 0x0, 0x0,
> > +    };
> > +
> > +    memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> > +    mctp->len += sizeof(buf);
> > +}
> > +
> > +static void i2c_mctp_handle_control_get_version(MCTPI2CEndpoint *mctp)
> > +{
> > +    uint8_t buf[] = {
> > +        0x0, 0x1, 0x0, 0x1, 0x3, 0x1,
> > +    };
> > +
> > +    memcpy(i2c_mctp_control_data(mctp->buffer), buf, sizeof(buf));
> > +    mctp->len += sizeof(buf);
> > +}
> > +
> > +enum {
> > +    MCTP_CONTROL_SET_EID                    = 0x01,
> > +    MCTP_CONTROL_GET_EID                    = 0x02,
> > +    MCTP_CONTROL_GET_VERSION                = 0x04,
> > +    MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT   = 0x05,
> > +};
> > +
> > +static void i2c_mctp_handle_control(MCTPI2CEndpoint *mctp)
> > +{
> > +    MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > +    MCTPControlMessage *msg = (MCTPControlMessage 
> > *)i2c_mctp_payload(mctp->buffer);
> > +
> > +    /* clear Rq/D */
> > +    msg->flags &= 0x1f;
> > +
> > +    mctp->len = offsetof(MCTPControlMessage, data);
> > +
> > +    trace_i2c_mctp_handle_control(msg->command);
> > +
> > +    switch (msg->command) {
> > +    case MCTP_CONTROL_SET_EID:
> > +        i2c_mctp_handle_control_set_eid(mctp, msg->data[1]);
> > +        break;
> > +
> > +    case MCTP_CONTROL_GET_EID:
> > +        i2c_mctp_handle_control_get_eid(mctp);
> > +        break;
> > +
> > +    case MCTP_CONTROL_GET_VERSION:
> > +        i2c_mctp_handle_control_get_version(mctp);
> > +        break;
> > +
> > +    case MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT:
> > +        mctp->len += mc->get_message_types(mctp, 
> > i2c_mctp_control_data(mctp->buffer));
> 
> You don't pass in how much data is available for the subclass to use.
> That's generally good API behavior.
> 

True, I'll fix it up.

> > +        break;
> > +
> > +    default:
> > +        trace_i2c_mctp_unhandled_control(msg->command);
> > +
> > +        msg->data[0] = MCTP_CONTROL_ERROR_UNSUPPORTED_CMD;
> > +        mctp->len++;
> > +
> > +        break;
> > +    }
> > +
> > +    i2c_mctp_schedule_send(mctp);
> > +}
> > +
> > +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;
> > +
> > +    switch (event) {
> > +    case I2C_START_SEND:
> > +        if (mctp->state != I2C_MCTP_STATE_IDLE) {
> > +            return -1;
> > +        }
> > +
> > +        /* the i2c core eats the slave address, so put it back in */
> > +        pkt->i2c.dest = i2c->address << 1;
> 
> This seems like a bit of a hack since pkt->i2c.dest never seems to be
> used.  I guess it's ok, since it's matching what the specifications say,
> but it seems a bit odd since you don't need it.
> 

Yeah it is definitely a hack around the i2c core. I need it to calculate
the PEC, so I have to put it into the buffer at some point. I think the
smbus implementation would suffer from this as well. We could maybe fold
that into the i2c_smbus_pec() call instead. I'll see what I can come up
with.

> > +        mctp->len = 1;
> > +
> > +        mctp->state = I2C_MCTP_STATE_RX_STARTED;
> > +
> > +        return 0;
> > +
> > +    case I2C_FINISH:
> > +        payload_len = mctp->len - (1 + offsetof(MCTPI2CPacket, 
> > mctp.payload));
> 
> Is there a way this can underflow?
> 

Hmm. Potentially. I'll audit it.

signature.asc
Description: PGP signature