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.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. > > 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.came...@huawei.com/ > > > > Signed-off-by: Klaus Jensen <k.jen...@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.jen...@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