| Message ID | 20221116084312.35808-3-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/{i2c, nvme}: mctp endpoint, nvme management interface model | expand |
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 :-). > > [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. > + > +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. > + 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. > + 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. > + 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? > + > + 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) { > + trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest, > + mctp->my_eid); > + goto drop; > + } > + > + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) { > + mctp->tx.is_control = false; > + > + if (mctp->state == I2C_MCTP_STATE_RX) { > + mc->reset_message(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; > + 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_message_bytes(mctp, i2c_mctp_payload(mctp->buffer), payload_len); > + > + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_EOM) { > + mc->handle_message(mctp); > + mctp->state = I2C_MCTP_STATE_WAIT_TX; > + } > + > + return 0; > + > + default: > + return -1; > + } > + > +drop: > + mc->reset_message(mctp); > + > + mctp->state = I2C_MCTP_STATE_IDLE; > + > + return 0; > +} > + > +static int i2c_mctp_send_cb(I2CSlave *i2c, uint8_t data) > +{ > + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c); > + > + if (mctp->len < I2C_MCTP_MAX_LENGTH) { > + mctp->buffer[mctp->len++] = data; > + return 0; > + } > + > + return -1; > +} > + > +static void i2c_mctp_instance_init(Object *obj) > +{ > + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(obj); > + > + mctp->tx.bh = qemu_bh_new(i2c_mctp_tx, mctp); > +} > + > +static Property mctp_i2c_props[] = { > + DEFINE_PROP_UINT8("eid", MCTPI2CEndpoint, my_eid, 0x9), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void i2c_mctp_class_init(ObjectClass *oc, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(oc); > + I2CSlaveClass *k = I2C_SLAVE_CLASS(oc); > + > + k->event = i2c_mctp_event_cb; > + k->send = i2c_mctp_send_cb; > + > + device_class_set_props(dc, mctp_i2c_props); > +} > + > +static const TypeInfo i2c_mctp_info = { > + .name = TYPE_MCTP_I2C_ENDPOINT, > + .parent = TYPE_I2C_SLAVE, > + .abstract = true, > + .instance_init = i2c_mctp_instance_init, > + .instance_size = sizeof(MCTPI2CEndpoint), > + .class_init = i2c_mctp_class_init, > + .class_size = sizeof(MCTPI2CEndpointClass), > +}; > + > +static void register_types(void) > +{ > + type_register_static(&i2c_mctp_info); > +} > + > +type_init(register_types) > diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build > index d3df273251f7..97d0bfd9ac67 100644 > --- a/hw/i2c/meson.build > +++ b/hw/i2c/meson.build > @@ -1,5 +1,6 @@ > i2c_ss = ss.source_set() > i2c_ss.add(when: 'CONFIG_I2C', if_true: files('core.c')) > +i2c_ss.add(when: 'CONFIG_MCTP_I2C', if_true: files('mctp.c')) > i2c_ss.add(when: 'CONFIG_SMBUS', if_true: files('smbus_slave.c', 'smbus_master.c')) > i2c_ss.add(when: 'CONFIG_ACPI_SMBUS', if_true: files('pm_smbus.c')) > i2c_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('smbus_ich9.c')) > diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events > index af181d43ee64..778139d08166 100644 > --- a/hw/i2c/trace-events > +++ b/hw/i2c/trace-events > @@ -33,3 +33,15 @@ npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s > > pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x" > pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x" > + > +# mctp.c > +i2c_mctp_tx_start_send(size_t len) "len %zu" > +i2c_mctp_tx_send_byte(size_t pos, uint8_t byte) "pos %zu byte 0x%"PRIx8"" > +i2c_mctp_tx_done(void) "packet sent" > +i2c_mctp_handle_control(uint8_t command) "command 0x%"PRIx8"" > +i2c_mctp_unhandled_control(uint8_t command) "command 0x%"PRIx8"" > +i2c_mctp_drop(const char *reason) "%s" > +i2c_mctp_drop_invalid_length(unsigned byte_count, size_t expected) "byte_count %u expected %zu" > +i2c_mctp_drop_invalid_pec(uint8_t pec, uint8_t expected) "pec 0x%"PRIx8" expected 0x%"PRIx8"" > +i2c_mctp_drop_invalid_eid(uint8_t eid, uint8_t expected) "eid 0x%"PRIx8" expected 0x%"PRIx8"" > +i2c_mctp_drop_invalid_pktseq(uint8_t pktseq, uint8_t expected) "pktseq 0x%"PRIx8" expected 0x%"PRIx8"" > diff --git a/include/hw/i2c/mctp.h b/include/hw/i2c/mctp.h > new file mode 100644 > index 000000000000..17dae662138b > --- /dev/null > +++ b/include/hw/i2c/mctp.h > @@ -0,0 +1,83 @@ > +#ifndef QEMU_I2C_MCTP_H > +#define QEMU_I2C_MCTP_H > + > +#include "qom/object.h" > +#include "hw/qdev-core.h" > +#include "hw/misc/mctp.h" > + > +typedef struct MCTPI2CPacketHeader { > + uint8_t dest; > + uint8_t prot; > + uint8_t byte_count; > + uint8_t source; > +} MCTPI2CPacketHeader; > + > +typedef struct MCTPI2CPacket { > + MCTPI2CPacketHeader i2c; > + MCTPPacket mctp; > +} MCTPI2CPacket; > + > +#define i2c_mctp_payload(buf) (buf + offsetof(MCTPI2CPacket, mctp.payload)) > + > +#define TYPE_MCTP_I2C_ENDPOINT "mctp-i2c-endpoint" > +OBJECT_DECLARE_TYPE(MCTPI2CEndpoint, MCTPI2CEndpointClass, MCTP_I2C_ENDPOINT) > + > +struct MCTPI2CEndpointClass { > + I2CSlaveClass parent_class; > + > + int (*put_message_bytes)(MCTPI2CEndpoint *mctp, uint8_t *buf, size_t len); > + size_t (*get_message_bytes)(MCTPI2CEndpoint *mctp, uint8_t *buf, > + size_t maxlen, uint8_t *mctp_flags); > + > + void (*handle_message)(MCTPI2CEndpoint *mctp); > + void (*reset_message)(MCTPI2CEndpoint *mctp); > + > + size_t (*get_message_types)(MCTPI2CEndpoint *mctp, uint8_t *data); > +}; > + > +#define I2C_MCTP_MAXBLOCK 255 > +#define I2C_MCTP_MAXMTU (I2C_MCTP_MAXBLOCK - (sizeof(MCTPPacketHeader) + 1)) > +#define I2C_MCTP_MAX_LENGTH (3 + I2C_MCTP_MAXBLOCK + 1) > + > +typedef enum { > + I2C_MCTP_STATE_IDLE, > + I2C_MCTP_STATE_RX_STARTED, > + I2C_MCTP_STATE_RX, > + I2C_MCTP_STATE_WAIT_TX, > + I2C_MCTP_STATE_TX, > +} MCTPState; > + > +typedef enum { > + I2C_MCTP_STATE_TX_START_SEND, > + I2C_MCTP_STATE_TX_SEND_BYTE, > +} MCTPTxState; > + > +typedef struct MCTPI2CEndpoint { > + I2CSlave parent_obj; > + I2CBus *i2c; > + > + MCTPState state; > + > + /* mctp endpoint identifier */ > + uint8_t my_eid; > + > + uint8_t buffer[I2C_MCTP_MAX_LENGTH]; > + uint64_t pos; > + size_t len; > + > + struct { > + MCTPTxState state; > + bool is_control; > + > + uint8_t eid; > + uint8_t addr; > + uint8_t pktseq; > + uint8_t flags; > + > + QEMUBH *bh; > + } tx; > +} MCTPI2CEndpoint; > + > +void i2c_mctp_schedule_send(MCTPI2CEndpoint *mctp); > + > +#endif /* QEMU_I2C_MCTP_H */ > diff --git a/include/hw/misc/mctp.h b/include/hw/misc/mctp.h > new file mode 100644 > index 000000000000..c936224ecf60 > --- /dev/null > +++ b/include/hw/misc/mctp.h > @@ -0,0 +1,43 @@ > +#ifndef QEMU_MCTP_H > +#define QEMU_MCTP_H > + > +#define MCTP_BASELINE_MTU 64 > + > +enum { > + MCTP_H_FLAGS_EOM = 1 << 6, > + MCTP_H_FLAGS_SOM = 1 << 7, > +}; > + > +enum { > + MCTP_MESSAGE_TYPE_CONTROL = 0x0, > + MCTP_MESSAGE_TYPE_NMI = 0x4, > + > + MCTP_MESSAGE_IC = 1 << 7, > +}; > + > +typedef struct MCTPPacketHeader { > + uint8_t version; > + struct { > + uint8_t dest; > + uint8_t source; > + } eid; > + uint8_t flags; > +} MCTPPacketHeader; > + > +typedef struct MCTPPacket { > + MCTPPacketHeader hdr; > + uint8_t payload[]; > +} MCTPPacket; > + > +typedef struct MCTPControlMessage { > + uint8_t type; > + uint8_t flags; > + uint8_t command; > + uint8_t data[]; > +} MCTPControlMessage; > + > +enum { > + MCTP_CONTROL_ERROR_UNSUPPORTED_CMD = 0x5, > +}; > + > +#endif /* QEMU_MCTP_H */ > -- > 2.38.1 > >
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.
Hi Klaus, > 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. Looks good, nice to see how it's used by the nmi device later too. A couple of issues with the state machine though, comments inline, and a bit of a patch below. > +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); If we're sending a control message, mctp->len will be set to the control msg len here, then: > + > + /* 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); > + } ... it doesn't get cleared above, so: > + > + if (!mctp->len) { ... we don't hit this conditional, and hence keep sending unlimited bytes. This presents as continuous interrupts to the aspeed i2c driver when replying to any control message. I think we need a mctp->len = 0 with the i2c_end_transfer(). With that, I can get control protocol communication working with mctpd. > +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; mctp->state may (validly) be I2C_MCTP_STATE_RX here, if we're receiving the start event for the second packet of a multi-packet message. > + } > + > + /* the i2c core eats the slave address, so put it back in */ > + pkt->i2c.dest = i2c->address << 1; > + mctp->len = 1; > + > + mctp->state = I2C_MCTP_STATE_RX_STARTED; > + > + return 0; > + > + case I2C_FINISH: > + 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) { > + trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest, > + mctp->my_eid); > + goto drop; > + } > + > + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) { > + mctp->tx.is_control = false; > + > + if (mctp->state == I2C_MCTP_STATE_RX) { > + mc->reset_message(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; > + 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)) { The pktseq is the sequence number of the last packet seen, so you want a pre-increment there: ++mctp->tx.pktseq & 0x3 } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (++mctp->tx.pktseq & 0x3)) { With those changes, I can get control protocol going, and multi-packet messages work. There's a couple of failures from unsupported commands, but otherwise looks good: # mctp addr add 8 dev mctpi2c6 # mctp link set mctpi2c6 up # mctp link set mctpi2c6 mtu 254 # systemctl restart mctpd # busctl --no-pager call xyz.openbmc_project.MCTP \ /xyz/openbmc_project/mctp au.com.CodeConstruct.MCTP \ SetupEndpoint say mctpi2c6 1 0x1d yisb 9 1 "/xyz/openbmc_project/mctp/1/9" true # mctp route del 9 via mctpi2c6 # mctp route add 9 via mctpi2c6 mtu 68 # mi-mctp 1 9 info nmi message type 0x2 not handled Identify Controller failed, no quirks applied NVMe MI subsys info: num ports: 1 major ver: 1 minor ver: 1 NVMe MI port info: port 0 type SMBus[2] MCTP MTU: 64 MEB size: 0 SMBus address: 0x00 VPD access freq: 0x00 MCTP address: 0x1d MCTP access freq: 0x01 NVMe basic management: disabled nmi command 0x1 not handled mi-mctp: can't perform Health Status Poll operation: Success # mi-mctp 1 9 get-config 0 nmi message type 0x2 not handled Identify Controller failed, no quirks applied SMBus access frequency (port 0): 100k [0x1] MCTP MTU (port 0): 64 I've included a patch below, with some fixes for the above, in case that helps. Cheers, Jeremy diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c index 46376de95a..1775deb46f 100644 --- a/hw/i2c/mctp.c +++ b/hw/i2c/mctp.c @@ -78,6 +78,9 @@ static void i2c_mctp_tx(void *opaque) /* packet sent */ i2c_end_transfer(i2c); + /* end of any control data */ + mctp->len = 0; + /* fall through */ case I2C_MCTP_STATE_TX_START_SEND: @@ -228,7 +231,9 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event) switch (event) { case I2C_START_SEND: - if (mctp->state != I2C_MCTP_STATE_IDLE) { + if (mctp->state == I2C_MCTP_STATE_IDLE) { + mctp->state = I2C_MCTP_STATE_RX_STARTED; + } else if (mctp->state != I2C_MCTP_STATE_RX) { return -1; } @@ -236,8 +241,6 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event) pkt->i2c.dest = i2c->address << 1; mctp->len = 1; - mctp->state = I2C_MCTP_STATE_RX_STARTED; - return 0; case I2C_FINISH: @@ -285,7 +288,7 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event) } 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)) { + } 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;
Hi Klaus, > With those changes, I can get control protocol going, and multi- > packet messages work. Ah, I also needed a change to the aspeed I2C driver, as I'm seeing the interrupt handler being invoked with both a stop and a start event pending. Patch below; if this seems sensible I will propose upstream. Cheers, Jeremy --- From 6331cf2499c182606979029d2aaed93ee3e544fa Mon Sep 17 00:00:00 2001 From: Jeremy Kerr <jk@codeconstruct.com.au> Date: Fri, 18 Nov 2022 14:04:50 +0800 Subject: [PATCH] i2c: aspeed: Allow combined STOP & START irqs Currently, if we enter our interrupt handler with both a stop and start condition pending, the stop event handler will override the current state, so we'll then lose the start condition. This change handles the stop condition before anything else, which means we can then restart the state machine on any pending start state. Because of this, we no longer need the ASPEED_I2C_SLAVE_STOP state, because we're just reverting directly to INACTIVE. I have only seen this condition on emulation; it may be impossible to hit on real HW. Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> --- drivers/i2c/busses/i2c-aspeed.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index 185dedfebbac..45f2766b2b66 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -135,7 +135,6 @@ enum aspeed_i2c_slave_state { ASPEED_I2C_SLAVE_READ_PROCESSED, ASPEED_I2C_SLAVE_WRITE_REQUESTED, ASPEED_I2C_SLAVE_WRITE_RECEIVED, - ASPEED_I2C_SLAVE_STOP, }; struct aspeed_i2c_bus { @@ -250,6 +249,14 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) command = readl(bus->base + ASPEED_I2C_CMD_REG); + /* handle a stop before starting any new events */ + if (bus->slave_state != ASPEED_I2C_SLAVE_INACTIVE && + irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) { + irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP; + i2c_slave_event(slave, I2C_SLAVE_STOP, &value); + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; + } + /* Slave was requested, restart state machine. */ if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) { irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH; @@ -278,15 +285,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) irq_handled |= ASPEED_I2CD_INTR_RX_DONE; } - /* Slave was asked to stop. */ - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) { - irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP; - bus->slave_state = ASPEED_I2C_SLAVE_STOP; - } if (irq_status & ASPEED_I2CD_INTR_TX_NAK && bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) { irq_handled |= ASPEED_I2CD_INTR_TX_NAK; - bus->slave_state = ASPEED_I2C_SLAVE_STOP; + i2c_slave_event(slave, I2C_SLAVE_STOP, &value); + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; } switch (bus->slave_state) { @@ -316,13 +319,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status) case ASPEED_I2C_SLAVE_WRITE_RECEIVED: i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value); break; - case ASPEED_I2C_SLAVE_STOP: - i2c_slave_event(slave, I2C_SLAVE_STOP, &value); - bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; - break; case ASPEED_I2C_SLAVE_START: /* Slave was just started. Waiting for the next event. */; break; + case ASPEED_I2C_SLAVE_INACTIVE: + break; default: dev_err(bus->dev, "unknown slave_state: %d\n", bus->slave_state);
On Nov 18 13:56, Jeremy Kerr wrote: > Hi Klaus, > > > 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. > > Looks good, nice to see how it's used by the nmi device later too. > > A couple of issues with the state machine though, comments inline, and > a bit of a patch below. > > > +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); > > If we're sending a control message, mctp->len will be set to the control > msg len here, then: > > > + > > + /* 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); > > + } > > ... it doesn't get cleared above, so: > > > + > > + if (!mctp->len) { > > ... we don't hit this conditional, and hence keep sending unlimited > bytes. This presents as continuous interrupts to the aspeed i2c driver > when replying to any control message. > > I think we need a mctp->len = 0 with the i2c_end_transfer(). With that, > I can get control protocol communication working with mctpd. > > > +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; > > mctp->state may (validly) be I2C_MCTP_STATE_RX here, if we're receiving > the start event for the second packet of a multi-packet message. > > > + } > > + > > + /* the i2c core eats the slave address, so put it back in */ > > + pkt->i2c.dest = i2c->address << 1; > > + mctp->len = 1; > > + > > + mctp->state = I2C_MCTP_STATE_RX_STARTED; > > + > > + return 0; > > + > > + case I2C_FINISH: > > + 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) { > > + trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest, > > + mctp->my_eid); > > + goto drop; > > + } > > + > > + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) { > > + mctp->tx.is_control = false; > > + > > + if (mctp->state == I2C_MCTP_STATE_RX) { > > + mc->reset_message(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; > > + 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)) { > > The pktseq is the sequence number of the last packet seen, so you want a > pre-increment there: ++mctp->tx.pktseq & 0x3 > > } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (++mctp->tx.pktseq & 0x3)) { > > With those changes, I can get control protocol going, and multi-packet > messages work. There's a couple of failures from unsupported commands, > but otherwise looks good: > > # mctp addr add 8 dev mctpi2c6 > # mctp link set mctpi2c6 up > # mctp link set mctpi2c6 mtu 254 > # systemctl restart mctpd > # busctl --no-pager call xyz.openbmc_project.MCTP \ > /xyz/openbmc_project/mctp au.com.CodeConstruct.MCTP \ > SetupEndpoint say mctpi2c6 1 0x1d > yisb 9 1 "/xyz/openbmc_project/mctp/1/9" true > # mctp route del 9 via mctpi2c6 > # mctp route add 9 via mctpi2c6 mtu 68 > # mi-mctp 1 9 info > nmi message type 0x2 not handled > Identify Controller failed, no quirks applied > NVMe MI subsys info: > num ports: 1 > major ver: 1 > minor ver: 1 > NVMe MI port info: > port 0 > type SMBus[2] > MCTP MTU: 64 > MEB size: 0 > SMBus address: 0x00 > VPD access freq: 0x00 > MCTP address: 0x1d > MCTP access freq: 0x01 > NVMe basic management: disabled > nmi command 0x1 not handled > mi-mctp: can't perform Health Status Poll operation: Success > # mi-mctp 1 9 get-config 0 > nmi message type 0x2 not handled > Identify Controller failed, no quirks applied > SMBus access frequency (port 0): 100k [0x1] > MCTP MTU (port 0): 64 > > I've included a patch below, with some fixes for the above, in case that > helps. > Thanks for the review and patch, Definitely helps!
On Nov 18 14:15, Jeremy Kerr wrote: > Hi Klaus, > > > With those changes, I can get control protocol going, and multi- > > packet messages work. > > Ah, I also needed a change to the aspeed I2C driver, as I'm seeing > the interrupt handler being invoked with both a stop and a start event > pending. > I had to reverse the target mode functionality in QEMU from the linux driver, so I am really not too sure if having START and STOP set in the interrupt register is allowed behavior or not (or if we are doing something wrong in the target mode implementation). I guess thats a question for Aspeed I2C maintainers (here and on lkml).
Hi Klaus, > I had to reverse the target mode functionality in QEMU from the linux > driver, so I am really not too sure if having START and STOP set in > the interrupt register is allowed behavior or not From my interpretation of things, there's nothing explicitly preventing both a pending start and stop - more that the interrupt is very likely to have been serviced between those two events on the kind of speeds we would see on the i2c bus. I guess we could try (temporarily) masking the irq on real hardware and see what happens? :D Cheers, Jeremy
On Fri, Nov 18, 2022 at 08:01:40AM +0100, Klaus Jensen wrote: > On Nov 18 13:56, Jeremy Kerr wrote: > > Hi Klaus, > > > > > Add an abstract MCTP over I2C endpoint model. This implements MCTP > > > control message handling as well as handling the actual I2C transport > > > (packetization). > > > > > With those changes, I can get control protocol going, and multi-packet > > messages work. There's a couple of failures from unsupported commands, > > but otherwise looks good: > > > > # mctp addr add 8 dev mctpi2c6 > > # mctp link set mctpi2c6 up > > # mctp link set mctpi2c6 mtu 254 > > # systemctl restart mctpd > > # busctl --no-pager call xyz.openbmc_project.MCTP \ > > /xyz/openbmc_project/mctp au.com.CodeConstruct.MCTP \ > > SetupEndpoint say mctpi2c6 1 0x1d Hi Klaus, Thanks for the MCTP model, it's useful here. I needed the following patch to be able to call SetupEndpoint again when a device has already been assigned an EID. That tries a Set Endpoint ID/ Get Endpoint ID, addressed to EID 0. Cheers, Matt --- From cb7ad91474367f8e47bdaf03aba9a822f2648f41 Mon Sep 17 00:00:00 2001 From: Matt Johnston <matt@codeconstruct.com.au> Date: Mon, 21 Nov 2022 15:10:13 +0800 Subject: [PATCH] i2c/mctp: Allow receiving messages to dest eid 0 The Null Destination ID, 0, is used for MCTP control messages when addressing by physical ID. That is used for Get Endpoint ID and Set Endpoint ID when querying/assigning an EID to an endpoint. Signed-off-by: Matt Johnston <matt@codeconstruct.com.au> --- hw/i2c/mctp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/i2c/mctp.c b/hw/i2c/mctp.c index 1775deb46f..9d9e519ba9 100644 --- a/hw/i2c/mctp.c +++ b/hw/i2c/mctp.c @@ -258,7 +258,8 @@ static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event) goto drop; } - if (pkt->mctp.hdr.eid.dest != mctp->my_eid) { + 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;
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; +} + +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); + 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)); + 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; + mctp->len = 1; + + mctp->state = I2C_MCTP_STATE_RX_STARTED; + + return 0; + + case I2C_FINISH: + 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) { + trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest, + mctp->my_eid); + goto drop; + } + + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) { + mctp->tx.is_control = false; + + if (mctp->state == I2C_MCTP_STATE_RX) { + mc->reset_message(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; + 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_message_bytes(mctp, i2c_mctp_payload(mctp->buffer), payload_len); + + if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_EOM) { + mc->handle_message(mctp); + mctp->state = I2C_MCTP_STATE_WAIT_TX; + } + + return 0; + + default: + return -1; + } + +drop: + mc->reset_message(mctp); + + mctp->state = I2C_MCTP_STATE_IDLE; + + return 0; +} + +static int i2c_mctp_send_cb(I2CSlave *i2c, uint8_t data) +{ + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c); + + if (mctp->len < I2C_MCTP_MAX_LENGTH) { + mctp->buffer[mctp->len++] = data; + return 0; + } + + return -1; +} + +static void i2c_mctp_instance_init(Object *obj) +{ + MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(obj); + + mctp->tx.bh = qemu_bh_new(i2c_mctp_tx, mctp); +} + +static Property mctp_i2c_props[] = { + DEFINE_PROP_UINT8("eid", MCTPI2CEndpoint, my_eid, 0x9), + DEFINE_PROP_END_OF_LIST(), +}; + +static void i2c_mctp_class_init(ObjectClass *oc, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(oc); + I2CSlaveClass *k = I2C_SLAVE_CLASS(oc); + + k->event = i2c_mctp_event_cb; + k->send = i2c_mctp_send_cb; + + device_class_set_props(dc, mctp_i2c_props); +} + +static const TypeInfo i2c_mctp_info = { + .name = TYPE_MCTP_I2C_ENDPOINT, + .parent = TYPE_I2C_SLAVE, + .abstract = true, + .instance_init = i2c_mctp_instance_init, + .instance_size = sizeof(MCTPI2CEndpoint), + .class_init = i2c_mctp_class_init, + .class_size = sizeof(MCTPI2CEndpointClass), +}; + +static void register_types(void) +{ + type_register_static(&i2c_mctp_info); +} + +type_init(register_types) diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build index d3df273251f7..97d0bfd9ac67 100644 --- a/hw/i2c/meson.build +++ b/hw/i2c/meson.build @@ -1,5 +1,6 @@ i2c_ss = ss.source_set() i2c_ss.add(when: 'CONFIG_I2C', if_true: files('core.c')) +i2c_ss.add(when: 'CONFIG_MCTP_I2C', if_true: files('mctp.c')) i2c_ss.add(when: 'CONFIG_SMBUS', if_true: files('smbus_slave.c', 'smbus_master.c')) i2c_ss.add(when: 'CONFIG_ACPI_SMBUS', if_true: files('pm_smbus.c')) i2c_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('smbus_ich9.c')) diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events index af181d43ee64..778139d08166 100644 --- a/hw/i2c/trace-events +++ b/hw/i2c/trace-events @@ -33,3 +33,15 @@ npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x" pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x" + +# mctp.c +i2c_mctp_tx_start_send(size_t len) "len %zu" +i2c_mctp_tx_send_byte(size_t pos, uint8_t byte) "pos %zu byte 0x%"PRIx8"" +i2c_mctp_tx_done(void) "packet sent" +i2c_mctp_handle_control(uint8_t command) "command 0x%"PRIx8"" +i2c_mctp_unhandled_control(uint8_t command) "command 0x%"PRIx8"" +i2c_mctp_drop(const char *reason) "%s" +i2c_mctp_drop_invalid_length(unsigned byte_count, size_t expected) "byte_count %u expected %zu" +i2c_mctp_drop_invalid_pec(uint8_t pec, uint8_t expected) "pec 0x%"PRIx8" expected 0x%"PRIx8"" +i2c_mctp_drop_invalid_eid(uint8_t eid, uint8_t expected) "eid 0x%"PRIx8" expected 0x%"PRIx8"" +i2c_mctp_drop_invalid_pktseq(uint8_t pktseq, uint8_t expected) "pktseq 0x%"PRIx8" expected 0x%"PRIx8"" diff --git a/include/hw/i2c/mctp.h b/include/hw/i2c/mctp.h new file mode 100644 index 000000000000..17dae662138b --- /dev/null +++ b/include/hw/i2c/mctp.h @@ -0,0 +1,83 @@ +#ifndef QEMU_I2C_MCTP_H +#define QEMU_I2C_MCTP_H + +#include "qom/object.h" +#include "hw/qdev-core.h" +#include "hw/misc/mctp.h" + +typedef struct MCTPI2CPacketHeader { + uint8_t dest; + uint8_t prot; + uint8_t byte_count; + uint8_t source; +} MCTPI2CPacketHeader; + +typedef struct MCTPI2CPacket { + MCTPI2CPacketHeader i2c; + MCTPPacket mctp; +} MCTPI2CPacket; + +#define i2c_mctp_payload(buf) (buf + offsetof(MCTPI2CPacket, mctp.payload)) + +#define TYPE_MCTP_I2C_ENDPOINT "mctp-i2c-endpoint" +OBJECT_DECLARE_TYPE(MCTPI2CEndpoint, MCTPI2CEndpointClass, MCTP_I2C_ENDPOINT) + +struct MCTPI2CEndpointClass { + I2CSlaveClass parent_class; + + int (*put_message_bytes)(MCTPI2CEndpoint *mctp, uint8_t *buf, size_t len); + size_t (*get_message_bytes)(MCTPI2CEndpoint *mctp, uint8_t *buf, + size_t maxlen, uint8_t *mctp_flags); + + void (*handle_message)(MCTPI2CEndpoint *mctp); + void (*reset_message)(MCTPI2CEndpoint *mctp); + + size_t (*get_message_types)(MCTPI2CEndpoint *mctp, uint8_t *data); +}; + +#define I2C_MCTP_MAXBLOCK 255 +#define I2C_MCTP_MAXMTU (I2C_MCTP_MAXBLOCK - (sizeof(MCTPPacketHeader) + 1)) +#define I2C_MCTP_MAX_LENGTH (3 + I2C_MCTP_MAXBLOCK + 1) + +typedef enum { + I2C_MCTP_STATE_IDLE, + I2C_MCTP_STATE_RX_STARTED, + I2C_MCTP_STATE_RX, + I2C_MCTP_STATE_WAIT_TX, + I2C_MCTP_STATE_TX, +} MCTPState; + +typedef enum { + I2C_MCTP_STATE_TX_START_SEND, + I2C_MCTP_STATE_TX_SEND_BYTE, +} MCTPTxState; + +typedef struct MCTPI2CEndpoint { + I2CSlave parent_obj; + I2CBus *i2c; + + MCTPState state; + + /* mctp endpoint identifier */ + uint8_t my_eid; + + uint8_t buffer[I2C_MCTP_MAX_LENGTH]; + uint64_t pos; + size_t len; + + struct { + MCTPTxState state; + bool is_control; + + uint8_t eid; + uint8_t addr; + uint8_t pktseq; + uint8_t flags; + + QEMUBH *bh; + } tx; +} MCTPI2CEndpoint; + +void i2c_mctp_schedule_send(MCTPI2CEndpoint *mctp); + +#endif /* QEMU_I2C_MCTP_H */ diff --git a/include/hw/misc/mctp.h b/include/hw/misc/mctp.h new file mode 100644 index 000000000000..c936224ecf60 --- /dev/null +++ b/include/hw/misc/mctp.h @@ -0,0 +1,43 @@ +#ifndef QEMU_MCTP_H +#define QEMU_MCTP_H + +#define MCTP_BASELINE_MTU 64 + +enum { + MCTP_H_FLAGS_EOM = 1 << 6, + MCTP_H_FLAGS_SOM = 1 << 7, +}; + +enum { + MCTP_MESSAGE_TYPE_CONTROL = 0x0, + MCTP_MESSAGE_TYPE_NMI = 0x4, + + MCTP_MESSAGE_IC = 1 << 7, +}; + +typedef struct MCTPPacketHeader { + uint8_t version; + struct { + uint8_t dest; + uint8_t source; + } eid; + uint8_t flags; +} MCTPPacketHeader; + +typedef struct MCTPPacket { + MCTPPacketHeader hdr; + uint8_t payload[]; +} MCTPPacket; + +typedef struct MCTPControlMessage { + uint8_t type; + uint8_t flags; + uint8_t command; + uint8_t data[]; +} MCTPControlMessage; + +enum { + MCTP_CONTROL_ERROR_UNSUPPORTED_CMD = 0x5, +}; + +#endif /* QEMU_MCTP_H */