* [PATCH v2 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model
@ 2023-04-25 6:35 Klaus Jensen
2023-04-25 6:35 ` [PATCH v2 1/3] hw/i2c: add mctp core Klaus Jensen
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Klaus Jensen @ 2023-04-25 6:35 UTC (permalink / raw)
To: qemu-devel
Cc: Corey Minyard, Jeremy Kerr, Klaus Jensen, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Jonathan Cameron, Klaus Jensen
From: Klaus Jensen <k.jensen@samsung.com>
This adds a generic MCTP endpoint model that other devices may derive
from. I'm not 100% happy with the design of the class methods, but it's
a start.
Also included is a very basic implementation of an NVMe-MI device,
supporting only a small subset of the required commands. Lior (CC'ed) has some
patches coming up that adds futher support.
Since this all relies on i2c target mode, this can currently only be
used with an SoC that includes the Aspeed I2C controller.
The easiest way to get up and running with this, is to grab my buildroot
overlay[1]. It includes modified a modified dts as well as a couple of
required packages.
QEMU can then be launched along these lines:
qemu-system-arm \
-nographic \
-M ast2600-evb \
-kernel output/images/zImage \
-initrd output/images/rootfs.cpio \
-dtb output/images/aspeed-ast2600-evb-nmi.dtb \
-nic user,hostfwd=tcp::2222-:22 \
-device nmi-i2c,address=0x3a \
-serial mon:stdio
From within the booted system,
mctp addr add 8 dev mctpi2c15
mctp link set mctpi2c15 up
mctp route add 9 via mctpi2c15
mctp neigh add 9 dev mctpi2c15 lladdr 0x3a
mi-mctp 1 9 info
Comments are very welcome!
[1]: https://github.com/birkelund/buildroots/tree/main/mctp-i2c
Changes since v1
~~~~~~~~~~~~~~~~
- Fix SPDX-License tag for hw/nvme/nmi-i2c.c (Philippe)
- Add some asserts to verify buffer indices (by request from Corey).
- Drop short packets that could result in underflow (Corey)
- Move i2c_smbus_pec() to smbus common code (Corey)
- A couple of logic fixes (patch from Jeremy squashed in)
- Added a patch to handle messages with dest eid 0 (Matt)
Maybe squash this as well.
Klaus Jensen (2):
hw/i2c: add mctp core
hw/nvme: add nvme management interface model
Matt Johnston (1):
i2c/mctp: Allow receiving messages to dest eid 0
MAINTAINERS | 7 +
hw/arm/Kconfig | 1 +
hw/i2c/Kconfig | 4 +
hw/i2c/mctp.c | 353 +++++++++++++++++++++++++++++++
hw/i2c/meson.build | 1 +
hw/i2c/smbus_master.c | 28 +++
hw/i2c/trace-events | 12 ++
hw/nvme/meson.build | 1 +
hw/nvme/nmi-i2c.c | 382 ++++++++++++++++++++++++++++++++++
hw/nvme/trace-events | 6 +
include/hw/i2c/mctp.h | 114 ++++++++++
include/hw/i2c/smbus_master.h | 3 +
include/net/mctp.h | 43 ++++
13 files changed, 955 insertions(+)
create mode 100644 hw/i2c/mctp.c
create mode 100644 hw/nvme/nmi-i2c.c
create mode 100644 include/hw/i2c/mctp.h
create mode 100644 include/net/mctp.h
--
2.40.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] hw/i2c: add mctp core
2023-04-25 6:35 [PATCH v2 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model Klaus Jensen
@ 2023-04-25 6:35 ` Klaus Jensen
2023-04-25 15:19 ` Corey Minyard
2023-05-25 11:27 ` Jonathan Cameron via
2023-04-25 6:35 ` [PATCH v2 2/3] i2c/mctp: Allow receiving messages to dest eid 0 Klaus Jensen
2023-04-25 6:35 ` [PATCH v2 3/3] hw/nvme: add nvme management interface model Klaus Jensen
2 siblings, 2 replies; 11+ messages in thread
From: Klaus Jensen @ 2023-04-25 6:35 UTC (permalink / raw)
To: qemu-devel
Cc: Corey Minyard, Jeremy Kerr, Klaus Jensen, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Jonathan Cameron, Klaus Jensen
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.
[1]: https://lore.kernel.org/qemu-devel/20220520170128.4436-1-Jonathan.Cameron@huawei.com/
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
MAINTAINERS | 7 +
hw/arm/Kconfig | 1 +
hw/i2c/Kconfig | 4 +
hw/i2c/mctp.c | 352 ++++++++++++++++++++++++++++++++++
hw/i2c/meson.build | 1 +
hw/i2c/smbus_master.c | 28 +++
hw/i2c/trace-events | 12 ++
include/hw/i2c/mctp.h | 114 +++++++++++
include/hw/i2c/smbus_master.h | 3 +
include/net/mctp.h | 43 +++++
10 files changed, 565 insertions(+)
create mode 100644 hw/i2c/mctp.c
create mode 100644 include/hw/i2c/mctp.h
create mode 100644 include/net/mctp.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 24154f5721c7..054aad1f3e97 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3339,6 +3339,13 @@ F: tests/qtest/adm1272-test.c
F: tests/qtest/max34451-test.c
F: tests/qtest/isl_pmbus_vr-test.c
+MCTP I2C Transport
+M: Klaus Jensen <k.jensen@samsung.com>
+S: Maintained
+F: hw/i2c/mctp.c
+F: include/hw/i2c/mctp.h
+F: include/net/mctp.h
+
Firmware schema specifications
M: Philippe Mathieu-Daudé <philmd@linaro.org>
R: Daniel P. Berrange <berrange@redhat.com>
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index b53bd7f0b2a0..d7ecbc99e5ee 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -457,6 +457,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 14886b35dac2..3415e8421ab1 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -45,3 +45,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..0f4045d0d685
--- /dev/null
+++ b/hw/i2c/mctp.c
@@ -0,0 +1,352 @@
+/*
+ * 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/smbus_master.h"
+#include "hw/i2c/mctp.h"
+
+#include "trace.h"
+
+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);
+
+ /* end of any control data */
+ mctp->len = 0;
+
+ /* 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);
+ assert(mctp->len < I2C_MCTP_MAX_LENGTH);
+
+ 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 = sizeof(MCTPControlMessage);
+
+ 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),
+ MCTP_BASELINE_MTU - mctp->len);
+ break;
+
+ default:
+ trace_i2c_mctp_unhandled_control(msg->command);
+
+ msg->data[0] = MCTP_CONTROL_ERROR_UNSUPPORTED_CMD;
+ mctp->len++;
+
+ break;
+ }
+
+ assert(mctp->len <= MCTP_BASELINE_MTU);
+
+ 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) {
+ 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");
+ 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) {
+ 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 3996564c25c6..fd1f9022fd96 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_ICH9', if_true: files('smbus_ich9.c'))
diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
index 6a53c34e70b7..47f9eb24e033 100644
--- a/hw/i2c/smbus_master.c
+++ b/hw/i2c/smbus_master.c
@@ -15,6 +15,34 @@
#include "hw/i2c/i2c.h"
#include "hw/i2c/smbus_master.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
+}
+
+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;
+}
+
/* Master device commands. */
int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
{
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 8e88aa24c1ac..2e3065a99873 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -45,3 +45,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..c53ee6a3b61b
--- /dev/null
+++ b/include/hw/i2c/mctp.h
@@ -0,0 +1,114 @@
+#ifndef QEMU_I2C_MCTP_H
+#define QEMU_I2C_MCTP_H
+
+#include "qom/object.h"
+#include "hw/qdev-core.h"
+#include "net/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,
+ size_t maxlen);
+};
+
+/*
+ * Maximum value of the SMBus Block Write "Byte Count" field (8 bits).
+ *
+ * This is the count of bytes that follow the Byte Count field and up to, but
+ * not including, the PEC byte.
+ */
+#define I2C_MCTP_MAXBLOCK 255
+
+/*
+ * Maximum Transmission Unit under I2C.
+ *
+ * This is for the MCTP Packet Payload (255, subtracting the 4 byte MCTP Packet
+ * Header or the 1 byte MCTP/I2C piggy-backed source address).
+ */
+#define I2C_MCTP_MAXMTU (I2C_MCTP_MAXBLOCK - (sizeof(MCTPPacketHeader) + 1))
+
+/*
+ * Maximum length of an MCTP/I2C packet.
+ *
+ * This is the sum of the three I2C header bytes (Destination target address,
+ * Command Code and Byte Count), the maximum number of bytes in a message (255)
+ * and the 1 byte Packet Error Code.
+ */
+#define I2C_MCTP_MAX_LENGTH (3 + I2C_MCTP_MAXBLOCK + 1)
+
+/*
+ * Maximum length of an MCTP/I2C Control Message.
+ *
+ * This is the 64 byte MCTP Baseline Maximum Transmission Unit, adding the
+ * combined MCTP/I2C headers and the trailing 1 byte PEC.
+ */
+#define I2C_MCTP_CONTROL_MAX_LENGTH \
+ (sizeof(MCTPI2CPacket) + MCTP_BASELINE_MTU + 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/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
index bb13bc423c22..ea5eff3a2cd0 100644
--- a/include/hw/i2c/smbus_master.h
+++ b/include/hw/i2c/smbus_master.h
@@ -27,6 +27,9 @@
#include "hw/i2c/i2c.h"
+/* SMBus PEC */
+uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len);
+
/* Master device commands. */
int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
int smbus_receive_byte(I2CBus *bus, uint8_t addr);
diff --git a/include/net/mctp.h b/include/net/mctp.h
new file mode 100644
index 000000000000..c936224ecf60
--- /dev/null
+++ b/include/net/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.40.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] i2c/mctp: Allow receiving messages to dest eid 0
2023-04-25 6:35 [PATCH v2 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model Klaus Jensen
2023-04-25 6:35 ` [PATCH v2 1/3] hw/i2c: add mctp core Klaus Jensen
@ 2023-04-25 6:35 ` Klaus Jensen
2023-05-25 11:29 ` Jonathan Cameron via
2023-04-25 6:35 ` [PATCH v2 3/3] hw/nvme: add nvme management interface model Klaus Jensen
2 siblings, 1 reply; 11+ messages in thread
From: Klaus Jensen @ 2023-04-25 6:35 UTC (permalink / raw)
To: qemu-devel
Cc: Corey Minyard, Jeremy Kerr, Klaus Jensen, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Jonathan Cameron, Klaus Jensen
From: Matt Johnston <matt@codeconstruct.com.au>
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 0f4045d0d685..db42dc72264b 100644
--- a/hw/i2c/mctp.c
+++ b/hw/i2c/mctp.c
@@ -242,7 +242,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;
--
2.40.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] hw/nvme: add nvme management interface model
2023-04-25 6:35 [PATCH v2 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model Klaus Jensen
2023-04-25 6:35 ` [PATCH v2 1/3] hw/i2c: add mctp core Klaus Jensen
2023-04-25 6:35 ` [PATCH v2 2/3] i2c/mctp: Allow receiving messages to dest eid 0 Klaus Jensen
@ 2023-04-25 6:35 ` Klaus Jensen
2023-05-25 11:34 ` Jonathan Cameron via
2 siblings, 1 reply; 11+ messages in thread
From: Klaus Jensen @ 2023-04-25 6:35 UTC (permalink / raw)
To: qemu-devel
Cc: Corey Minyard, Jeremy Kerr, Klaus Jensen, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Jonathan Cameron, Klaus Jensen
From: Klaus Jensen <k.jensen@samsung.com>
Add the 'nmi-i2c' device that emulates an NVMe Management Interface
controller.
Initial support is very basic (Read NMI DS, Configuration Get).
This is based on previously posted code by Padmakar Kalghatgi, Arun
Kumar Agasar and Saurav Kumar.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
hw/nvme/meson.build | 1 +
hw/nvme/nmi-i2c.c | 382 +++++++++++++++++++++++++++++++++++++++++++
hw/nvme/trace-events | 6 +
3 files changed, 389 insertions(+)
create mode 100644 hw/nvme/nmi-i2c.c
diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
index 3cf40046eea9..b231e3fa12c2 100644
--- a/hw/nvme/meson.build
+++ b/hw/nvme/meson.build
@@ -1 +1,2 @@
softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c', 'ns.c', 'subsys.c'))
+softmmu_ss.add(when: 'CONFIG_MCTP_I2C', if_true: files('nmi-i2c.c'))
diff --git a/hw/nvme/nmi-i2c.c b/hw/nvme/nmi-i2c.c
new file mode 100644
index 000000000000..81738f185bba
--- /dev/null
+++ b/hw/nvme/nmi-i2c.c
@@ -0,0 +1,382 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * SPDX-FileCopyrightText: Copyright (c) 2022 Samsung Electronics Co., Ltd.
+ *
+ * SPDX-FileContributor: Padmakar Kalghatgi <p.kalghatgi@samsung.com>
+ * SPDX-FileContributor: Arun Kumar Agasar <arun.kka@samsung.com>
+ * SPDX-FileContributor: Saurav Kumar <saurav.29@partner.samsung.com>
+ * SPDX-FileContributor: Klaus Jensen <k.jensen@samsung.com>
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/crc32c.h"
+#include "hw/i2c/i2c.h"
+#include "hw/registerfields.h"
+#include "hw/i2c/mctp.h"
+#include "trace.h"
+
+#define NMI_MAX_MESSAGE_LENGTH 4224
+
+#define TYPE_NMI_I2C_DEVICE "nmi-i2c"
+OBJECT_DECLARE_SIMPLE_TYPE(NMIDevice, NMI_I2C_DEVICE)
+
+typedef struct NMIDevice {
+ MCTPI2CEndpoint mctp;
+
+ uint8_t buffer[NMI_MAX_MESSAGE_LENGTH];
+ uint8_t scratch[NMI_MAX_MESSAGE_LENGTH];
+
+ size_t len;
+ int64_t pos;
+} NMIDevice;
+
+FIELD(NMI_NMP, ROR, 7, 1)
+FIELD(NMI_NMP, NMIMT, 3, 4)
+
+#define NMI_NMP_NMIMT_NMI_CMD 0x1
+#define NMI_NMP_NMIMT_NM_ADMIN 0x2
+
+typedef struct NMIMessage {
+ uint8_t mctpd;
+ uint8_t nmp;
+ uint8_t rsvd2[2];
+ uint8_t payload[]; /* includes the Message Integrity Check */
+} NMIMessage;
+
+typedef struct NMIRequest {
+ uint8_t opc;
+ uint8_t rsvd1[3];
+ uint32_t dw0;
+ uint32_t dw1;
+ uint32_t mic;
+} NMIRequest;
+
+typedef struct NMIResponse {
+ uint8_t status;
+ uint8_t response[3];
+ uint8_t payload[]; /* includes the Message Integrity Check */
+} NMIResponse;
+
+typedef enum NMIReadDSType {
+ NMI_CMD_READ_NMI_DS_SUBSYSTEM = 0x0,
+ NMI_CMD_READ_NMI_DS_PORTS = 0x1,
+ NMI_CMD_READ_NMI_DS_CTRL_LIST = 0x2,
+ NMI_CMD_READ_NMI_DS_CTRL_INFO = 0x3,
+ NMI_CMD_READ_NMI_DS_CMD_SUPPORT = 0x4,
+ NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT = 0x5,
+} NMIReadDSType;
+
+static void nmi_handle_mi_read_nmi_ds(NMIDevice *nmi, NMIRequest *request)
+{
+ I2CSlave *i2c = I2C_SLAVE(nmi);
+
+ uint32_t dw0 = le32_to_cpu(request->dw0);
+ uint8_t dtyp = (dw0 >> 24) & 0xf;
+ uint8_t *buf;
+ size_t len;
+
+ trace_nmi_handle_mi_read_nmi_ds(dtyp);
+
+ static uint8_t nmi_ds_subsystem[36] = {
+ 0x00, /* success */
+ 0x20, /* response data length */
+ 0x00, 0x00, /* reserved */
+ 0x00, /* number of ports */
+ 0x01, /* major version */
+ 0x01, /* minor version */
+ };
+
+ static uint8_t nmi_ds_ports[36] = {
+ 0x00, /* success */
+ 0x20, /* response data length */
+ 0x00, 0x00, /* reserved */
+ 0x02, /* port type (smbus) */
+ 0x00, /* reserved */
+ 0x40, 0x00, /* maximum mctp transission unit size (64 bytes) */
+ 0x00, 0x00, 0x00, 0x00, /* management endpoint buffer size */
+ 0x00, 0x00, /* vpd i2c address/freq */
+ 0x00, 0x01, /* management endpoint i2c address/freq */
+ };
+
+ static uint8_t nmi_ds_error[4] = {
+ 0x04, /* invalid parameter */
+ 0x00, /* first invalid bit position */
+ 0x00, 0x00, /* first invalid byte position */
+ };
+
+ static uint8_t nmi_ds_empty[8] = {
+ 0x00, /* success */
+ 0x02, /* response data length */
+ 0x00, 0x00, /* reserved */
+ 0x00, 0x00, /* number of controllers */
+ 0x00, 0x00, /* padding */
+ };
+
+ switch (dtyp) {
+ case NMI_CMD_READ_NMI_DS_SUBSYSTEM:
+ len = 36;
+ buf = nmi_ds_subsystem;
+
+ break;
+
+ case NMI_CMD_READ_NMI_DS_PORTS:
+ len = 36;
+ buf = nmi_ds_ports;
+
+ /* patch in the i2c address of the endpoint */
+ buf[14] = i2c->address;
+
+ break;
+
+ case NMI_CMD_READ_NMI_DS_CTRL_INFO:
+ len = 4;
+ buf = nmi_ds_error;
+
+ break;
+
+ case NMI_CMD_READ_NMI_DS_CTRL_LIST:
+ case NMI_CMD_READ_NMI_DS_CMD_SUPPORT:
+ case NMI_CMD_READ_NMI_DS_MEB_CMD_SUPPORT:
+ len = 8;
+ buf = nmi_ds_empty;
+
+ break;
+
+ default:
+ len = 4;
+ buf = nmi_ds_error;
+
+ /* patch in the invalid parameter position */
+ buf[2] = 0x03; /* first invalid byte position (dtyp) */
+
+ break;
+ }
+
+ memcpy(nmi->scratch + nmi->pos, buf, len);
+ nmi->pos += len;
+}
+
+enum {
+ NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ = 0x1,
+ NMI_CMD_CONFIGURATION_GET_HEALTH_STATUS_CHANGE = 0x2,
+ NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT = 0x3,
+};
+
+static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest *request)
+{
+ uint32_t dw0 = le32_to_cpu(request->dw0);
+ uint8_t identifier = dw0 & 0xff;
+ uint8_t *buf;
+
+ trace_nmi_handle_mi_config_get(identifier);
+
+ switch (identifier) {
+ case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
+ buf = (uint8_t[]) {
+ 0x0, 0x1, 0x0, 0x0,
+ };
+
+ break;
+
+ case NMI_CMD_CONFIGURATION_GET_HEALTH_STATUS_CHANGE:
+ buf = (uint8_t[]) {
+ 0x0, 0x0, 0x0, 0x0,
+ };
+
+ break;
+
+ case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
+ buf = (uint8_t[]) {
+ 0x0, 0x40, 0x0, 0x0,
+ };
+
+ break;
+ }
+
+ memcpy(nmi->scratch + nmi->pos, buf, 4);
+ nmi->pos += 4;
+}
+
+enum {
+ NMI_CMD_READ_NMI_DS = 0x0,
+ NMI_CMD_CONFIGURATION_GET = 0x4,
+};
+
+static void nmi_set_parameter_error(NMIDevice *nmi, uint8_t bit, uint16_t byte)
+{
+ nmi->scratch[nmi->pos++] = 0x4;
+ nmi->scratch[nmi->pos++] = bit;
+ nmi->scratch[nmi->pos++] = (byte >> 4) & 0xf;
+ nmi->scratch[nmi->pos++] = byte & 0xf;
+}
+
+static void nmi_set_error(NMIDevice *nmi, uint8_t status)
+{
+ uint8_t buf[4] = {};
+
+ buf[0] = status;
+
+ memcpy(nmi->scratch + nmi->pos, buf, 4);
+ nmi->pos += 4;
+}
+
+static void nmi_handle_mi(NMIDevice *nmi, NMIMessage *msg)
+{
+ NMIRequest *request = (NMIRequest *)msg->payload;
+
+ trace_nmi_handle_mi(request->opc);
+
+ switch (request->opc) {
+ case NMI_CMD_READ_NMI_DS:
+ nmi_handle_mi_read_nmi_ds(nmi, request);
+ break;
+
+ case NMI_CMD_CONFIGURATION_GET:
+ nmi_handle_mi_config_get(nmi, request);
+ break;
+
+ default:
+ nmi_set_parameter_error(nmi, 0x0, 0x0);
+ fprintf(stderr, "nmi command 0x%x not handled\n", request->opc);
+
+ break;
+ }
+}
+
+enum {
+ NMI_MESSAGE_TYPE_NMI = 0x1,
+};
+
+static void nmi_handle_message(MCTPI2CEndpoint *mctp)
+{
+ NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
+ NMIMessage *msg = (NMIMessage *)nmi->buffer;
+ uint32_t crc;
+ uint8_t nmimt;
+
+ uint8_t buf[] = {
+ MCTP_MESSAGE_TYPE_NMI | MCTP_MESSAGE_IC,
+ FIELD_DP8(msg->nmp, NMI_NMP, ROR, 1),
+ 0x0, 0x0,
+ };
+
+ memcpy(nmi->scratch, buf, sizeof(buf));
+ nmi->pos = sizeof(buf);
+
+ nmimt = FIELD_EX8(msg->nmp, NMI_NMP, NMIMT);
+
+ trace_nmi_handle_msg(nmimt);
+
+ switch (nmimt) {
+ case NMI_MESSAGE_TYPE_NMI:
+ nmi_handle_mi(nmi, msg);
+ break;
+
+ default:
+ fprintf(stderr, "nmi message type 0x%x not handled\n", nmimt);
+
+ nmi_set_error(nmi, 0x3);
+
+ break;
+ }
+
+ /* add message integrity check */
+ memset(nmi->scratch + nmi->pos, 0x0, sizeof(crc));
+
+ crc = crc32c(0xffffffff, nmi->scratch, nmi->pos);
+ memcpy(nmi->scratch + nmi->pos, &crc, sizeof(crc));
+
+ nmi->len = nmi->pos + sizeof(crc);
+ nmi->pos = 0;
+
+ i2c_mctp_schedule_send(mctp);
+}
+
+static size_t nmi_get_message_bytes(MCTPI2CEndpoint *mctp, uint8_t *buf,
+ size_t maxlen, uint8_t *mctp_flags)
+{
+ NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
+ size_t len;
+
+ len = MIN(maxlen, nmi->len - nmi->pos);
+
+ if (len == 0) {
+ return 0;
+ }
+
+ if (nmi->pos == 0) {
+ *mctp_flags |= MCTP_H_FLAGS_SOM;
+ }
+
+ memcpy(buf, nmi->scratch + nmi->pos, len);
+ nmi->pos += len;
+
+ if (nmi->pos == nmi->len) {
+ *mctp_flags |= MCTP_H_FLAGS_EOM;
+
+ nmi->pos = nmi->len = 0;
+ }
+
+ return len;
+}
+
+static int nmi_put_message_bytes(MCTPI2CEndpoint *mctp, uint8_t *buf,
+ size_t len)
+{
+ NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
+
+ if (nmi->len + len > NMI_MAX_MESSAGE_LENGTH) {
+ return -1;
+ }
+
+ memcpy(nmi->buffer + nmi->len, buf, len);
+ nmi->len += len;
+
+ return 0;
+}
+
+static void nmi_reset_message(MCTPI2CEndpoint *mctp)
+{
+ NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
+ nmi->len = 0;
+}
+
+static size_t nmi_get_message_types(MCTPI2CEndpoint *mctp, uint8_t *data,
+ size_t maxlen)
+{
+ uint8_t buf[] = {
+ 0x0, 0x1, 0x4,
+ };
+
+ memcpy(data, buf, sizeof(buf));
+
+ return sizeof(buf);
+}
+
+static void nvme_mi_class_init(ObjectClass *oc, void *data)
+{
+ MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_CLASS(oc);
+
+ mc->get_message_types = nmi_get_message_types;
+
+ mc->get_message_bytes = nmi_get_message_bytes;
+ mc->put_message_bytes = nmi_put_message_bytes;
+
+ mc->handle_message = nmi_handle_message;
+ mc->reset_message = nmi_reset_message;
+}
+
+static const TypeInfo nvme_mi = {
+ .name = TYPE_NMI_I2C_DEVICE,
+ .parent = TYPE_MCTP_I2C_ENDPOINT,
+ .instance_size = sizeof(NMIDevice),
+ .class_init = nvme_mi_class_init,
+};
+
+static void register_types(void)
+{
+ type_register_static(&nvme_mi);
+}
+
+type_init(register_types);
diff --git a/hw/nvme/trace-events b/hw/nvme/trace-events
index 9afddf3b951c..e71171c539bd 100644
--- a/hw/nvme/trace-events
+++ b/hw/nvme/trace-events
@@ -215,3 +215,9 @@ pci_nvme_ub_db_wr_invalid_sq(uint32_t qid) "submission queue doorbell write for
pci_nvme_ub_db_wr_invalid_sqtail(uint32_t qid, uint16_t new_tail) "submission queue doorbell write value beyond queue size, sqid=%"PRIu32", new_head=%"PRIu16", ignoring"
pci_nvme_ub_unknown_css_value(void) "unknown value in cc.css field"
pci_nvme_ub_too_many_mappings(void) "too many prp/sgl mappings"
+
+# nmi-i2c
+nmi_handle_mi_read_nmi_ds(uint8_t dtyp) "dtyp 0x%"PRIx8""
+nmi_handle_mi_config_get(uint8_t identifier) "identifier 0x%"PRIx8""
+nmi_handle_mi(uint8_t opc) "opc 0x%"PRIx8""
+nmi_handle_msg(uint8_t nmint) "nmint 0x%"PRIx8""
--
2.40.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] hw/i2c: add mctp core
2023-04-25 6:35 ` [PATCH v2 1/3] hw/i2c: add mctp core Klaus Jensen
@ 2023-04-25 15:19 ` Corey Minyard
2023-04-26 7:11 ` Klaus Jensen
2023-05-25 11:27 ` Jonathan Cameron via
1 sibling, 1 reply; 11+ messages in thread
From: Corey Minyard @ 2023-04-25 15:19 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Corey Minyard, Jeremy Kerr, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Jonathan Cameron, Klaus Jensen
On Tue, Apr 25, 2023 at 08:35:38AM +0200, 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.
All in all this looks good. Two comments:
I would like to see the buffer handling consolidated into one function
and the length checked, even for (especially for) the outside users of
this code, like the nvme code. Best to avoid future issues with buffer
overruns. This will require reworking the get_message_types function,
unfortunately.
You have one trace function on a bad receive message check, but lots of
other bad receive message checks with no trace. Just a suggestion, but
it might be nice for tracking down issues to trace all the reasons a
message is dropped.
Thanks,
-corey
>
> [1]: https://lore.kernel.org/qemu-devel/20220520170128.4436-1-Jonathan.Cameron@huawei.com/
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
> MAINTAINERS | 7 +
> hw/arm/Kconfig | 1 +
> hw/i2c/Kconfig | 4 +
> hw/i2c/mctp.c | 352 ++++++++++++++++++++++++++++++++++
> hw/i2c/meson.build | 1 +
> hw/i2c/smbus_master.c | 28 +++
> hw/i2c/trace-events | 12 ++
> include/hw/i2c/mctp.h | 114 +++++++++++
> include/hw/i2c/smbus_master.h | 3 +
> include/net/mctp.h | 43 +++++
> 10 files changed, 565 insertions(+)
> create mode 100644 hw/i2c/mctp.c
> create mode 100644 include/hw/i2c/mctp.h
> create mode 100644 include/net/mctp.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 24154f5721c7..054aad1f3e97 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3339,6 +3339,13 @@ F: tests/qtest/adm1272-test.c
> F: tests/qtest/max34451-test.c
> F: tests/qtest/isl_pmbus_vr-test.c
>
> +MCTP I2C Transport
> +M: Klaus Jensen <k.jensen@samsung.com>
> +S: Maintained
> +F: hw/i2c/mctp.c
> +F: include/hw/i2c/mctp.h
> +F: include/net/mctp.h
> +
> Firmware schema specifications
> M: Philippe Mathieu-Daudé <philmd@linaro.org>
> R: Daniel P. Berrange <berrange@redhat.com>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index b53bd7f0b2a0..d7ecbc99e5ee 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -457,6 +457,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 14886b35dac2..3415e8421ab1 100644
> --- a/hw/i2c/Kconfig
> +++ b/hw/i2c/Kconfig
> @@ -45,3 +45,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..0f4045d0d685
> --- /dev/null
> +++ b/hw/i2c/mctp.c
> @@ -0,0 +1,352 @@
> +/*
> + * 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/smbus_master.h"
> +#include "hw/i2c/mctp.h"
> +
> +#include "trace.h"
> +
> +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);
> +
> + /* end of any control data */
> + mctp->len = 0;
> +
> + /* 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);
> + assert(mctp->len < I2C_MCTP_MAX_LENGTH);
> +
> + 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 = sizeof(MCTPControlMessage);
> +
> + 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),
> + MCTP_BASELINE_MTU - mctp->len);
> + break;
> +
> + default:
> + trace_i2c_mctp_unhandled_control(msg->command);
> +
> + msg->data[0] = MCTP_CONTROL_ERROR_UNSUPPORTED_CMD;
> + mctp->len++;
> +
> + break;
> + }
> +
> + assert(mctp->len <= MCTP_BASELINE_MTU);
> +
> + 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) {
> + 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");
> + 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) {
> + 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 3996564c25c6..fd1f9022fd96 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_ICH9', if_true: files('smbus_ich9.c'))
> diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
> index 6a53c34e70b7..47f9eb24e033 100644
> --- a/hw/i2c/smbus_master.c
> +++ b/hw/i2c/smbus_master.c
> @@ -15,6 +15,34 @@
> #include "hw/i2c/i2c.h"
> #include "hw/i2c/smbus_master.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
> +}
> +
> +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;
> +}
> +
> /* Master device commands. */
> int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
> {
> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> index 8e88aa24c1ac..2e3065a99873 100644
> --- a/hw/i2c/trace-events
> +++ b/hw/i2c/trace-events
> @@ -45,3 +45,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..c53ee6a3b61b
> --- /dev/null
> +++ b/include/hw/i2c/mctp.h
> @@ -0,0 +1,114 @@
> +#ifndef QEMU_I2C_MCTP_H
> +#define QEMU_I2C_MCTP_H
> +
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +#include "net/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,
> + size_t maxlen);
> +};
> +
> +/*
> + * Maximum value of the SMBus Block Write "Byte Count" field (8 bits).
> + *
> + * This is the count of bytes that follow the Byte Count field and up to, but
> + * not including, the PEC byte.
> + */
> +#define I2C_MCTP_MAXBLOCK 255
> +
> +/*
> + * Maximum Transmission Unit under I2C.
> + *
> + * This is for the MCTP Packet Payload (255, subtracting the 4 byte MCTP Packet
> + * Header or the 1 byte MCTP/I2C piggy-backed source address).
> + */
> +#define I2C_MCTP_MAXMTU (I2C_MCTP_MAXBLOCK - (sizeof(MCTPPacketHeader) + 1))
> +
> +/*
> + * Maximum length of an MCTP/I2C packet.
> + *
> + * This is the sum of the three I2C header bytes (Destination target address,
> + * Command Code and Byte Count), the maximum number of bytes in a message (255)
> + * and the 1 byte Packet Error Code.
> + */
> +#define I2C_MCTP_MAX_LENGTH (3 + I2C_MCTP_MAXBLOCK + 1)
> +
> +/*
> + * Maximum length of an MCTP/I2C Control Message.
> + *
> + * This is the 64 byte MCTP Baseline Maximum Transmission Unit, adding the
> + * combined MCTP/I2C headers and the trailing 1 byte PEC.
> + */
> +#define I2C_MCTP_CONTROL_MAX_LENGTH \
> + (sizeof(MCTPI2CPacket) + MCTP_BASELINE_MTU + 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/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
> index bb13bc423c22..ea5eff3a2cd0 100644
> --- a/include/hw/i2c/smbus_master.h
> +++ b/include/hw/i2c/smbus_master.h
> @@ -27,6 +27,9 @@
>
> #include "hw/i2c/i2c.h"
>
> +/* SMBus PEC */
> +uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len);
> +
> /* Master device commands. */
> int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
> int smbus_receive_byte(I2CBus *bus, uint8_t addr);
> diff --git a/include/net/mctp.h b/include/net/mctp.h
> new file mode 100644
> index 000000000000..c936224ecf60
> --- /dev/null
> +++ b/include/net/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.40.0
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] hw/i2c: add mctp core
2023-04-25 15:19 ` Corey Minyard
@ 2023-04-26 7:11 ` Klaus Jensen
2023-04-26 11:52 ` Corey Minyard
0 siblings, 1 reply; 11+ messages in thread
From: Klaus Jensen @ 2023-04-26 7:11 UTC (permalink / raw)
To: Corey Minyard
Cc: qemu-devel, Corey Minyard, Jeremy Kerr, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Jonathan Cameron, Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]
On Apr 25 10:19, Corey Minyard wrote:
> On Tue, Apr 25, 2023 at 08:35:38AM +0200, 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.
>
> All in all this looks good. Two comments:
>
> I would like to see the buffer handling consolidated into one function
> and the length checked, even for (especially for) the outside users of
> this code, like the nvme code. Best to avoid future issues with buffer
> overruns. This will require reworking the get_message_types function,
> unfortunately.
>
Right now the implementations (i.e. hw/nvme/nmi-i2c.c) writes directly
into the mctp core buffer for get_message_bytes(). The contract is that
it must not write more than the `maxlen` parameter. Is that bad? Would
it be better that get_message_bytes() returned a pointer to its own
buffer that hw/mctp can then copy from?
> You have one trace function on a bad receive message check, but lots of
> other bad receive message checks with no trace. Just a suggestion, but
> it might be nice for tracking down issues to trace all the reasons a
> message is dropped.
>
Sounds reasonable! :)
Thanks for the review!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] hw/i2c: add mctp core
2023-04-26 7:11 ` Klaus Jensen
@ 2023-04-26 11:52 ` Corey Minyard
0 siblings, 0 replies; 11+ messages in thread
From: Corey Minyard @ 2023-04-26 11:52 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Corey Minyard, Jeremy Kerr, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Jonathan Cameron, Klaus Jensen
On Wed, Apr 26, 2023 at 09:11:16AM +0200, Klaus Jensen wrote:
> On Apr 25 10:19, Corey Minyard wrote:
> > On Tue, Apr 25, 2023 at 08:35:38AM +0200, 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.
> >
> > All in all this looks good. Two comments:
> >
> > I would like to see the buffer handling consolidated into one function
> > and the length checked, even for (especially for) the outside users of
> > this code, like the nvme code. Best to avoid future issues with buffer
> > overruns. This will require reworking the get_message_types function,
> > unfortunately.
> >
>
> Right now the implementations (i.e. hw/nvme/nmi-i2c.c) writes directly
> into the mctp core buffer for get_message_bytes(). The contract is that
> it must not write more than the `maxlen` parameter. Is that bad? Would
> it be better that get_message_bytes() returned a pointer to its own
> buffer that hw/mctp can then copy from?
qemu has had several instances of unchecked writing into a buffer
eventually getting it into trouble. It might be ok in the beginning,
but as things change and code is added, something might come in that is
not ok.
nmi_get_message_types(), for instance, does not check maxlen.
It may be borderline paranoia, but I've seen too many instances where
paranoia was warranted :).
Plus I think it would make the code a little cleaner and easier to
maintain. If you wanted to change how the buffer worked, trace data put
into the buffer, or something like that, all the code to handle that is
in one place.
>
> > You have one trace function on a bad receive message check, but lots of
> > other bad receive message checks with no trace. Just a suggestion, but
> > it might be nice for tracking down issues to trace all the reasons a
> > message is dropped.
> >
>
> Sounds reasonable! :)
>
> Thanks for the review!
Thank you for the submission :).
-corey
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] hw/i2c: add mctp core
2023-04-25 6:35 ` [PATCH v2 1/3] hw/i2c: add mctp core Klaus Jensen
2023-04-25 15:19 ` Corey Minyard
@ 2023-05-25 11:27 ` Jonathan Cameron via
2023-05-25 11:33 ` Klaus Jensen
1 sibling, 1 reply; 11+ messages in thread
From: Jonathan Cameron via @ 2023-05-25 11:27 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Corey Minyard, Jeremy Kerr, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Klaus Jensen
On Tue, 25 Apr 2023 08:35:38 +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.
>
> [1]: https://lore.kernel.org/qemu-devel/20220520170128.4436-1-Jonathan.Cameron@huawei.com/
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
This is very useful for the CXL MCTP CCI support - got rid of most of the
nasty code when I moved over to using this (will post code shortly)
A few comments inline - but all trivial stuff.
What I can give now though is:
Tested-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..0f4045d0d685
> --- /dev/null
> +++ b/hw/i2c/mctp.c
> @@ -0,0 +1,352 @@
...
> +enum {
> + MCTP_CONTROL_SET_EID = 0x01,
> + MCTP_CONTROL_GET_EID = 0x02,
> + MCTP_CONTROL_GET_VERSION = 0x04,
> + MCTP_CONTROL_GET_MESSAGE_TYPE_SUPPORT = 0x05,
> +};
See below. If the control command codes are down here, why
not also bring the structure they go in out of the header as well.
There should be no reason for an emulated device to be touching
that directly (I think anyway?)
> diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
> index 3996564c25c6..fd1f9022fd96 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_ICH9', if_true: files('smbus_ich9.c'))
> diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
> index 6a53c34e70b7..47f9eb24e033 100644
> --- a/hw/i2c/smbus_master.c
> +++ b/hw/i2c/smbus_master.c
While it's first used in this patch, the smbus pec code stuff
is from the SMBUS spec itself and stands independently of the rest of the
set. As such I'd make this a precursor patch.
> @@ -15,6 +15,34 @@
> #include "hw/i2c/i2c.h"
> #include "hw/i2c/smbus_master.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
> +}
> +
> +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;
> +}
> +
> /* Master device commands. */
> int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
> {
...
> diff --git a/include/hw/i2c/mctp.h b/include/hw/i2c/mctp.h
> new file mode 100644
> index 000000000000..c53ee6a3b61b
> --- /dev/null
> +++ b/include/hw/i2c/mctp.h
> @@ -0,0 +1,114 @@
> +#ifndef QEMU_I2C_MCTP_H
> +#define QEMU_I2C_MCTP_H
> +
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +#include "net/mctp.h"
I'd definitely put some specification references in here to the
MCTP SMBus/I2C Transport Binding Specification
> +
> +typedef struct MCTPI2CPacketHeader {
> + uint8_t dest;
> + uint8_t prot;
This is called Command Code in the spec I'm looking at...
Why I have no idea given it always takes 0x0F
> + 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,
> + size_t maxlen);
> +};
> +
> +/*
> + * Maximum value of the SMBus Block Write "Byte Count" field (8 bits).
> + *
> + * This is the count of bytes that follow the Byte Count field and up to, but
> + * not including, the PEC byte.
> + */
> +#define I2C_MCTP_MAXBLOCK 255
> +
> +/*
> + * Maximum Transmission Unit under I2C.
> + *
> + * This is for the MCTP Packet Payload (255, subtracting the 4 byte MCTP Packet
> + * Header or the 1 byte MCTP/I2C piggy-backed source address).
> + */
> +#define I2C_MCTP_MAXMTU (I2C_MCTP_MAXBLOCK - (sizeof(MCTPPacketHeader) + 1))
> +
> +/*
> + * Maximum length of an MCTP/I2C packet.
> + *
> + * This is the sum of the three I2C header bytes (Destination target address,
> + * Command Code and Byte Count), the maximum number of bytes in a message (255)
> + * and the 1 byte Packet Error Code.
> + */
> +#define I2C_MCTP_MAX_LENGTH (3 + I2C_MCTP_MAXBLOCK + 1)
> +
> +/*
> + * Maximum length of an MCTP/I2C Control Message.
> + *
> + * This is the 64 byte MCTP Baseline Maximum Transmission Unit, adding the
> + * combined MCTP/I2C headers and the trailing 1 byte PEC.
> + */
> +#define I2C_MCTP_CONTROL_MAX_LENGTH \
> + (sizeof(MCTPI2CPacket) + MCTP_BASELINE_MTU + 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;
I'd not bother trying to align these. Just use 1 space
everywhere. It's inconsistent already...
> +
> + 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/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
> index bb13bc423c22..ea5eff3a2cd0 100644
> --- a/include/hw/i2c/smbus_master.h
> +++ b/include/hw/i2c/smbus_master.h
> @@ -27,6 +27,9 @@
>
> #include "hw/i2c/i2c.h"
>
> +/* SMBus PEC */
Trivial, but I'd not bother with the comment given the function
name makes that clear.
> +uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len);
> +
> /* Master device commands. */
> int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
> int smbus_receive_byte(I2CBus *bus, uint8_t addr);
> diff --git a/include/net/mctp.h b/include/net/mctp.h
> new file mode 100644
> index 000000000000..c936224ecf60
> --- /dev/null
> +++ b/include/net/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,
As there is no need for the core mctp support to know about
the other protocols, I'd move them to the relevant device specific
emulation. Otherwise we'll have potential churn and merge conflicts
in here if lots of new ones get added.
> +
> + MCTP_MESSAGE_IC = 1 << 7,
That's nasty - just make it a define rather than pushing it into
the enum.
> +};
> +
> +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;
> +
Maybe worth some spec references in here to help reviewers find the
right tables. This took a bit of finding...
Figure 20 in 1.3.0
> +typedef struct MCTPControlMessage {
> + uint8_t type;
> + uint8_t flags;
Perhaps good to add defines for the various fields hidden in flags.
> + uint8_t command;
Maybe worth an enum for the many command codes.
Ah. There is one down in the implementation .c
Why not push this structure down there as well?
> + uint8_t data[];
> +} MCTPControlMessage;
> +
> +enum {
> + MCTP_CONTROL_ERROR_UNSUPPORTED_CMD = 0x5,
> +};
> +
> +#endif /* QEMU_MCTP_H */
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] i2c/mctp: Allow receiving messages to dest eid 0
2023-04-25 6:35 ` [PATCH v2 2/3] i2c/mctp: Allow receiving messages to dest eid 0 Klaus Jensen
@ 2023-05-25 11:29 ` Jonathan Cameron via
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron via @ 2023-05-25 11:29 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Corey Minyard, Jeremy Kerr, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Klaus Jensen
On Tue, 25 Apr 2023 08:35:39 +0200
Klaus Jensen <its@irrelevant.dk> wrote:
> From: Matt Johnston <matt@codeconstruct.com.au>
>
> 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>
Seems to work though maybe just squash into previous and add
a note to thank Matt for the fix?
Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> 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 0f4045d0d685..db42dc72264b 100644
> --- a/hw/i2c/mctp.c
> +++ b/hw/i2c/mctp.c
> @@ -242,7 +242,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;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] hw/i2c: add mctp core
2023-05-25 11:27 ` Jonathan Cameron via
@ 2023-05-25 11:33 ` Klaus Jensen
0 siblings, 0 replies; 11+ messages in thread
From: Klaus Jensen @ 2023-05-25 11:33 UTC (permalink / raw)
To: Jonathan Cameron
Cc: qemu-devel, Corey Minyard, Jeremy Kerr, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Klaus Jensen
[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]
On May 25 12:27, Jonathan Cameron wrote:
> On Tue, 25 Apr 2023 08:35:38 +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.
> >
> > [1]: https://lore.kernel.org/qemu-devel/20220520170128.4436-1-Jonathan.Cameron@huawei.com/
> >
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>
> This is very useful for the CXL MCTP CCI support - got rid of most of the
> nasty code when I moved over to using this (will post code shortly)
>
> A few comments inline - but all trivial stuff.
>
> What I can give now though is:
> Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
Hi Jonathan,
This came at just the right time! I was finally preparing v3 since Lior
is also requesting that this gets merged sooner rather than later ;)
I'll work in your very resonable comments (thanks!) and ship it.
Thanks again,
Klaus
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] hw/nvme: add nvme management interface model
2023-04-25 6:35 ` [PATCH v2 3/3] hw/nvme: add nvme management interface model Klaus Jensen
@ 2023-05-25 11:34 ` Jonathan Cameron via
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron via @ 2023-05-25 11:34 UTC (permalink / raw)
To: Klaus Jensen
Cc: qemu-devel, Corey Minyard, Jeremy Kerr, qemu-arm,
Peter Delevoryas, Keith Busch, Cédric Le Goater, Jason Wang,
Lior Weintraub, qemu-block, Peter Maydell, Matt Johnston,
Klaus Jensen
On Tue, 25 Apr 2023 08:35:40 +0200
Klaus Jensen <its@irrelevant.dk> wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Add the 'nmi-i2c' device that emulates an NVMe Management Interface
> controller.
>
> Initial support is very basic (Read NMI DS, Configuration Get).
>
> This is based on previously posted code by Padmakar Kalghatgi, Arun
> Kumar Agasar and Saurav Kumar.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
I don't have time to do too much spec diving so this is very superficial...
Mostly commenting because it gave me a build error.
> diff --git a/hw/nvme/nmi-i2c.c b/hw/nvme/nmi-i2c.c
> new file mode 100644
> index 000000000000..81738f185bba
> --- /dev/null
> +++ b/hw/nvme/nmi-i2c.c
> @@ -0,0 +1,382 @@
...
> +static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest *request)
> +{
> + uint32_t dw0 = le32_to_cpu(request->dw0);
> + uint8_t identifier = dw0 & 0xff;
> + uint8_t *buf;
> +
> + trace_nmi_handle_mi_config_get(identifier);
> +
> + switch (identifier) {
> + case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
> + buf = (uint8_t[]) {
> + 0x0, 0x1, 0x0, 0x0,
> + };
> +
> + break;
> +
> + case NMI_CMD_CONFIGURATION_GET_HEALTH_STATUS_CHANGE:
> + buf = (uint8_t[]) {
> + 0x0, 0x0, 0x0, 0x0,
> + };
> +
> + break;
> +
> + case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
> + buf = (uint8_t[]) {
> + 0x0, 0x40, 0x0, 0x0,
> + };
> +
> + break;
No default, which gave me a build error as buf is uninitialized.
> + }
> +
> + memcpy(nmi->scratch + nmi->pos, buf, 4);
> + nmi->pos += 4;
> +}
> +
> +enum {
> + NMI_CMD_READ_NMI_DS = 0x0,
> + NMI_CMD_CONFIGURATION_GET = 0x4,
> +};
> +
> +static size_t nmi_get_message_types(MCTPI2CEndpoint *mctp, uint8_t *data,
> + size_t maxlen)
> +{
> + uint8_t buf[] = {
> + 0x0, 0x1, 0x4,
PLDM? Are you using that so far? Maybe keep it for when you
add PLDM support?
> + };
> +
> + memcpy(data, buf, sizeof(buf));
> +
> + return sizeof(buf);
> +}
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-05-25 11:35 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-25 6:35 [PATCH v2 0/3] hw/{i2c, nvme}: mctp endpoint, nvme management interface model Klaus Jensen
2023-04-25 6:35 ` [PATCH v2 1/3] hw/i2c: add mctp core Klaus Jensen
2023-04-25 15:19 ` Corey Minyard
2023-04-26 7:11 ` Klaus Jensen
2023-04-26 11:52 ` Corey Minyard
2023-05-25 11:27 ` Jonathan Cameron via
2023-05-25 11:33 ` Klaus Jensen
2023-04-25 6:35 ` [PATCH v2 2/3] i2c/mctp: Allow receiving messages to dest eid 0 Klaus Jensen
2023-05-25 11:29 ` Jonathan Cameron via
2023-04-25 6:35 ` [PATCH v2 3/3] hw/nvme: add nvme management interface model Klaus Jensen
2023-05-25 11:34 ` Jonathan Cameron via
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).