astlpc: Consolidate direct vs indirect LPC access
Use helpers to wrap up LPC accessors so we're not littering the code
with conditional checks for direct or indirect access. As a result, drop
the priv_hdr pointer from struct mctp_binding_astlpc as we're utilising
the buffers provided by the caller, which in turn removes a heap
allocation.
Change-Id: Iab9ca4295879a3f3a0209fb23163c0383476c223
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
diff --git a/astlpc.c b/astlpc.c
index aec1b39..4092ed9 100644
--- a/astlpc.c
+++ b/astlpc.c
@@ -51,18 +51,14 @@
struct mctp_binding_astlpc {
struct mctp_binding binding;
- union {
- void *lpc_map;
- struct mctp_lpcmap_hdr *lpc_hdr;
- };
+ void *lpc_map;
struct mctp_astlpc_layout layout;
uint8_t mode;
/* direct ops data */
- struct mctp_binding_astlpc_ops ops;
- void *ops_data;
- struct mctp_lpcmap_hdr *priv_hdr;
+ struct mctp_binding_astlpc_ops ops;
+ void *ops_data;
/* fileio ops data */
void *lpc_map_base;
@@ -133,14 +129,53 @@
#define KCS_STATUS_IBF 0x02
#define KCS_STATUS_OBF 0x01
-static bool lpc_direct(struct mctp_binding_astlpc *astlpc)
+static inline int mctp_astlpc_lpc_write(struct mctp_binding_astlpc *astlpc,
+ const void *buf, long offset,
+ size_t len)
{
- return astlpc->lpc_map != NULL;
+ astlpc_prdebug(astlpc, "%s: %zu bytes to 0x%lx", __func__, len, offset);
+
+ assert(offset >= 0);
+
+ /* Indirect access */
+ if (astlpc->ops.lpc_write) {
+ void *data = astlpc->ops_data;
+
+ return astlpc->ops.lpc_write(data, buf, offset, len);
+ }
+
+ /* Direct mapping */
+ assert(astlpc->lpc_map);
+ memcpy(&((char *)astlpc->lpc_map)[offset], buf, len);
+
+ return 0;
+}
+
+static inline int mctp_astlpc_lpc_read(struct mctp_binding_astlpc *astlpc,
+ void *buf, long offset, size_t len)
+{
+ astlpc_prdebug(astlpc, "%s: %zu bytes from 0x%lx", __func__, len,
+ offset);
+
+ assert(offset >= 0);
+
+ /* Indirect access */
+ if (astlpc->ops.lpc_read) {
+ void *data = astlpc->ops_data;
+
+ return astlpc->ops.lpc_read(data, buf, offset, len);
+ }
+
+ /* Direct mapping */
+ assert(astlpc->lpc_map);
+ memcpy(buf, &((char *)astlpc->lpc_map)[offset], len);
+
+ return 0;
}
static int mctp_astlpc_init_bmc(struct mctp_binding_astlpc *astlpc)
{
- struct mctp_lpcmap_hdr *hdr;
+ struct mctp_lpcmap_hdr hdr = { 0 };
uint8_t status;
int rc;
@@ -150,24 +185,20 @@
astlpc->layout.tx.offset = rx_offset;
astlpc->layout.tx.size = rx_size;
- if (lpc_direct(astlpc))
- hdr = astlpc->lpc_hdr;
- else
- hdr = astlpc->priv_hdr;
+ hdr = (struct mctp_lpcmap_hdr){
+ .magic = htobe32(ASTLPC_MCTP_MAGIC),
+ .bmc_ver_min = htobe16(ASTLPC_VER_MIN),
+ .bmc_ver_cur = htobe16(ASTLPC_VER_CUR),
- hdr->magic = htobe32(ASTLPC_MCTP_MAGIC);
- hdr->bmc_ver_min = htobe16(ASTLPC_VER_MIN);
- hdr->bmc_ver_cur = htobe16(ASTLPC_VER_CUR);
+ /* Flip the buffers back as we're now describing the host's
+ * configuration to the host */
+ .rx_offset = htobe32(astlpc->layout.tx.offset),
+ .rx_size = htobe32(astlpc->layout.tx.size),
+ .tx_offset = htobe32(astlpc->layout.rx.offset),
+ .tx_size = htobe32(astlpc->layout.rx.size),
+ };
- /* Flip the buffers back as we're now describing the host's
- * configuration to the host */
- hdr->rx_offset = htobe32(astlpc->layout.tx.offset);
- hdr->rx_size = htobe32(astlpc->layout.tx.size);
- hdr->tx_offset = htobe32(astlpc->layout.rx.offset);
- hdr->tx_size = htobe32(astlpc->layout.rx.size);
-
- if (!lpc_direct(astlpc))
- astlpc->ops.lpc_write(astlpc->ops_data, hdr, 0, sizeof(*hdr));
+ mctp_astlpc_lpc_write(astlpc, &hdr, 0, sizeof(hdr));
/* set status indicating that the BMC is now active */
status = KCS_STATUS_BMC_READY | KCS_STATUS_OBF;
@@ -191,7 +222,9 @@
static int mctp_astlpc_init_host(struct mctp_binding_astlpc *astlpc)
{
- struct mctp_lpcmap_hdr *hdr;
+ const uint16_t ver_min_be = htobe16(ASTLPC_VER_MIN);
+ const uint16_t ver_cur_be = htobe16(ASTLPC_VER_CUR);
+ struct mctp_lpcmap_hdr hdr;
uint8_t status;
int rc;
@@ -207,31 +240,20 @@
if (!(status & KCS_STATUS_BMC_READY))
return -EHOSTDOWN;
- if (lpc_direct(astlpc)) {
- hdr = astlpc->lpc_hdr;
- } else {
- hdr = astlpc->priv_hdr;
- astlpc->ops.lpc_read(astlpc->ops_data, hdr, 0, sizeof(*hdr));
- }
+ mctp_astlpc_lpc_read(astlpc, &hdr, 0, sizeof(hdr));
- astlpc->layout.rx.offset = be32toh(hdr->rx_offset);
- astlpc->layout.rx.size = be32toh(hdr->rx_size);
- astlpc->layout.tx.offset = be32toh(hdr->tx_offset);
- astlpc->layout.tx.size = be32toh(hdr->tx_size);
+ astlpc->layout.rx.offset = be32toh(hdr.rx_offset);
+ astlpc->layout.rx.size = be32toh(hdr.rx_size);
+ astlpc->layout.tx.offset = be32toh(hdr.tx_offset);
+ astlpc->layout.tx.size = be32toh(hdr.tx_size);
- hdr->host_ver_min = htobe16(ASTLPC_VER_MIN);
- if (!lpc_direct(astlpc))
- astlpc->ops.lpc_write(astlpc->ops_data, &hdr->host_ver_min,
- offsetof(struct mctp_lpcmap_hdr,
- host_ver_min),
- sizeof(hdr->host_ver_min));
+ mctp_astlpc_lpc_write(astlpc, &ver_min_be,
+ offsetof(struct mctp_lpcmap_hdr, host_ver_min),
+ sizeof(ver_min_be));
- hdr->host_ver_cur = htobe16(ASTLPC_VER_CUR);
- if (!lpc_direct(astlpc))
- astlpc->ops.lpc_write(astlpc->ops_data, &hdr->host_ver_cur,
- offsetof(struct mctp_lpcmap_hdr,
- host_ver_cur),
- sizeof(hdr->host_ver_cur));
+ mctp_astlpc_lpc_write(astlpc, &ver_cur_be,
+ offsetof(struct mctp_lpcmap_hdr, host_ver_cur),
+ sizeof(ver_cur_be));
/* Send channel init command */
rc = astlpc->ops.kcs_write(astlpc->ops_data, MCTP_ASTLPC_KCS_REG_DATA,
@@ -341,7 +363,7 @@
struct mctp_pktbuf *pkt)
{
struct mctp_binding_astlpc *astlpc = binding_to_astlpc(b);
- uint32_t len;
+ uint32_t len, len_be;
struct mctp_hdr *hdr;
hdr = mctp_pktbuf_hdr(pkt);
@@ -357,18 +379,10 @@
return -1;
}
- if (lpc_direct(astlpc)) {
- *(uint32_t *)(astlpc->lpc_map + astlpc->layout.tx.offset) =
- htobe32(len);
- memcpy(astlpc->lpc_map + astlpc->layout.tx.offset + 4,
- mctp_pktbuf_hdr(pkt), len);
- } else {
- uint32_t tmp = htobe32(len);
- astlpc->ops.lpc_write(astlpc->ops_data, &tmp,
- astlpc->layout.tx.offset, sizeof(tmp));
- astlpc->ops.lpc_write(astlpc->ops_data, mctp_pktbuf_hdr(pkt),
- astlpc->layout.tx.offset + 4, len);
- }
+ len_be = htobe32(len);
+ mctp_astlpc_lpc_write(astlpc, &len_be, astlpc->layout.tx.offset,
+ sizeof(len_be));
+ mctp_astlpc_lpc_write(astlpc, hdr, astlpc->layout.tx.offset + 4, len);
mctp_binding_set_tx_enabled(b, false);
@@ -379,18 +393,14 @@
static void mctp_astlpc_init_channel(struct mctp_binding_astlpc *astlpc)
{
/* todo: actual version negotiation */
- if (lpc_direct(astlpc)) {
- astlpc->lpc_hdr->negotiated_ver = htobe16(1);
- } else {
- uint16_t ver = htobe16(1);
- astlpc->ops.lpc_write(astlpc->ops_data, &ver,
- offsetof(struct mctp_lpcmap_hdr,
- negotiated_ver),
- sizeof(ver));
- }
- mctp_astlpc_kcs_set_status(astlpc,
- KCS_STATUS_BMC_READY | KCS_STATUS_CHANNEL_ACTIVE |
- KCS_STATUS_OBF);
+ uint16_t negotiated = htobe16(1);
+ mctp_astlpc_lpc_write(astlpc, &negotiated,
+ offsetof(struct mctp_lpcmap_hdr, negotiated_ver),
+ sizeof(negotiated));
+
+ mctp_astlpc_kcs_set_status(astlpc, KCS_STATUS_BMC_READY |
+ KCS_STATUS_CHANNEL_ACTIVE |
+ KCS_STATUS_OBF);
mctp_binding_set_tx_enabled(&astlpc->binding, true);
}
@@ -400,12 +410,8 @@
struct mctp_pktbuf *pkt;
uint32_t len;
- if (lpc_direct(astlpc)) {
- len = *(uint32_t *)(astlpc->lpc_map + astlpc->layout.rx.offset);
- } else {
- astlpc->ops.lpc_read(astlpc->ops_data, &len,
- astlpc->layout.rx.offset, sizeof(len));
- }
+ mctp_astlpc_lpc_read(astlpc, &len, astlpc->layout.rx.offset,
+ sizeof(len));
len = be32toh(len);
if (len > ASTLPC_BODY_SIZE(astlpc->layout.rx.size)) {
@@ -424,13 +430,8 @@
if (!pkt)
goto out_complete;
- if (lpc_direct(astlpc)) {
- memcpy(mctp_pktbuf_hdr(pkt),
- astlpc->lpc_map + astlpc->layout.rx.offset + 4, len);
- } else {
- astlpc->ops.lpc_read(astlpc->ops_data, mctp_pktbuf_hdr(pkt),
- astlpc->layout.rx.offset + 4, len);
- }
+ mctp_astlpc_lpc_read(astlpc, mctp_pktbuf_hdr(pkt),
+ astlpc->layout.rx.offset + 4, len);
mctp_bus_rx(&astlpc->binding, pkt);
@@ -581,12 +582,6 @@
astlpc->lpc_map = lpc_map;
astlpc->mode = mode;
- /* In indirect mode, we keep a separate buffer of header data.
- * We need to sync this through the lpc_read/lpc_write ops.
- */
- if (!astlpc->lpc_map)
- astlpc->priv_hdr = __mctp_alloc(sizeof(*astlpc->priv_hdr));
-
return astlpc;
}
@@ -600,12 +595,11 @@
void mctp_astlpc_destroy(struct mctp_binding_astlpc *astlpc)
{
- if (astlpc->priv_hdr)
- __mctp_free(astlpc->priv_hdr);
__mctp_free(astlpc);
}
#ifdef MCTP_HAVE_FILEIO
+
static int mctp_astlpc_init_fileio_lpc(struct mctp_binding_astlpc *astlpc)
{
struct aspeed_lpc_ctrl_mapping map = {
diff --git a/libmctp-astlpc.h b/libmctp-astlpc.h
index a3e7ffb..7bfa623 100644
--- a/libmctp-astlpc.h
+++ b/libmctp-astlpc.h
@@ -22,10 +22,10 @@
struct mctp_binding_astlpc_ops {
int (*kcs_read)(void *data, enum mctp_binding_astlpc_kcs_reg reg,
uint8_t *val);
- int (*kcs_write)(void *data, enum mctp_binding_astlpc_kcs_reg reg,
- uint8_t val);
- int (*lpc_read)(void *data, void *buf, long offset, size_t len);
- int (*lpc_write)(void *data, void *buf, long offset, size_t len);
+ int (*kcs_write)(void *data, enum mctp_binding_astlpc_kcs_reg reg,
+ uint8_t val);
+ int (*lpc_read)(void *data, void *buf, long offset, size_t len);
+ int (*lpc_write)(void *data, const void *buf, long offset, size_t len);
};
#define MCTP_BINDING_ASTLPC_MODE_BMC 0
diff --git a/tests/test_astlpc.c b/tests/test_astlpc.c
index 772ba26..7a2b9d9 100644
--- a/tests/test_astlpc.c
+++ b/tests/test_astlpc.c
@@ -118,7 +118,8 @@
return 0;
}
-int mctp_astlpc_mmio_lpc_write(void *data, void *buf, long offset, size_t len)
+int mctp_astlpc_mmio_lpc_write(void *data, const void *buf, long offset,
+ size_t len)
{
struct mctp_binding_astlpc_mmio *mmio = binding_to_mmio(data);
@@ -150,7 +151,12 @@
test->count++;
}
-static const struct mctp_binding_astlpc_ops mctp_binding_astlpc_mmio_ops = {
+static const struct mctp_binding_astlpc_ops astlpc_direct_mmio_ops = {
+ .kcs_read = mctp_astlpc_mmio_kcs_read,
+ .kcs_write = mctp_astlpc_mmio_kcs_write,
+};
+
+static const struct mctp_binding_astlpc_ops astlpc_indirect_mmio_ops = {
.kcs_read = mctp_astlpc_mmio_kcs_read,
.kcs_write = mctp_astlpc_mmio_kcs_write,
.lpc_read = mctp_astlpc_mmio_lpc_read,
@@ -158,8 +164,7 @@
};
static void endpoint_init(struct astlpc_endpoint *ep, mctp_eid_t eid,
- uint8_t mode, uint8_t (*kcs)[2], void *lpc_mem,
- size_t lpc_size)
+ uint8_t mode, uint8_t (*kcs)[2], void *lpc_mem)
{
/*
* Configure the direction of the KCS interface so we know whether to
@@ -173,13 +178,9 @@
/* Inject KCS registers */
ep->mmio.kcs = kcs;
- /* Inject the heap allocation as the LPC mapping */
- ep->mmio.lpc_size = lpc_size;
- ep->mmio.lpc = lpc_mem;
-
/* Initialise the binding */
ep->astlpc = mctp_astlpc_init(mode, MCTP_BTU, lpc_mem,
- &mctp_binding_astlpc_mmio_ops, &ep->mmio);
+ &astlpc_direct_mmio_ops, &ep->mmio);
mctp_register_bus(ep->mctp, &ep->astlpc->binding, eid);
}
@@ -192,18 +193,16 @@
static void network_init(struct astlpc_test *ctx)
{
- const size_t lpc_size = 1 * 1024 * 1024;
-
- ctx->lpc_mem = calloc(1, lpc_size);
+ ctx->lpc_mem = calloc(1, 1 * 1024 * 1024);
assert(ctx->lpc_mem);
/* BMC initialisation */
endpoint_init(&ctx->bmc, 8, MCTP_BINDING_ASTLPC_MODE_BMC, &ctx->kcs,
- ctx->lpc_mem, lpc_size);
+ ctx->lpc_mem);
/* Host initialisation */
endpoint_init(&ctx->host, 9, MCTP_BINDING_ASTLPC_MODE_HOST, &ctx->kcs,
- ctx->lpc_mem, lpc_size);
+ ctx->lpc_mem);
/* BMC processes host channel init request, alerts host */
mctp_astlpc_poll(ctx->bmc.astlpc);
@@ -366,7 +365,7 @@
/* Initialise the binding */
astlpc = mctp_astlpc_init(MCTP_BINDING_ASTLPC_MODE_HOST, MCTP_BTU, NULL,
- &mctp_binding_astlpc_mmio_ops, &mmio);
+ &astlpc_direct_mmio_ops, &mmio);
/* Register the binding to trigger the start-up sequence */
rc = mctp_register_bus(mctp, &astlpc->binding, 8);
@@ -382,24 +381,20 @@
{
struct astlpc_endpoint bmc, host;
uint8_t kcs[2] = { 0 };
- size_t lpc_size;
void *lpc_mem;
/* Test harness initialisation */
- lpc_size = 1 * 1024 * 1024;
- lpc_mem = calloc(1, lpc_size);
+ lpc_mem = calloc(1, 1 * 1024 * 1024);
assert(lpc_mem);
/* BMC initialisation */
- endpoint_init(&bmc, 8, MCTP_BINDING_ASTLPC_MODE_BMC, &kcs, lpc_mem,
- lpc_size);
+ endpoint_init(&bmc, 8, MCTP_BINDING_ASTLPC_MODE_BMC, &kcs, lpc_mem);
/* Verify the BMC binding was initialised */
assert(kcs[MCTP_ASTLPC_KCS_REG_STATUS] & KCS_STATUS_BMC_READY);
/* Host initialisation */
- endpoint_init(&host, 9, MCTP_BINDING_ASTLPC_MODE_HOST, &kcs, lpc_mem,
- lpc_size);
+ endpoint_init(&host, 9, MCTP_BINDING_ASTLPC_MODE_HOST, &kcs, lpc_mem);
/* Host sends channel init command */
assert(kcs[MCTP_ASTLPC_KCS_REG_STATUS] & KCS_STATUS_IBF);
@@ -421,6 +416,72 @@
free(lpc_mem);
}
+static void astlpc_test_simple_indirect_message_bmc_to_host(void)
+{
+ struct astlpc_test ctx = { 0 };
+ uint8_t kcs[2] = { 0 };
+ uint8_t msg[MCTP_BTU];
+ int rc;
+
+ ctx.lpc_mem = calloc(1, LPC_WIN_SIZE);
+ assert(ctx.lpc_mem);
+
+ /* Test message data */
+ memset(&msg[0], 0x5a, MCTP_BTU);
+
+ /* Manually set up the network so we can inject the indirect ops */
+
+ /* BMC initialisation */
+ ctx.bmc.mmio.bmc = true;
+ ctx.bmc.mctp = mctp_init();
+ assert(ctx.bmc.mctp);
+ ctx.bmc.mmio.kcs = &kcs;
+ ctx.bmc.mmio.lpc = ctx.lpc_mem;
+ ctx.bmc.mmio.lpc_size = LPC_WIN_SIZE;
+ ctx.bmc.astlpc =
+ mctp_astlpc_init(MCTP_BINDING_ASTLPC_MODE_BMC, MCTP_BTU, NULL,
+ &astlpc_indirect_mmio_ops, &ctx.bmc.mmio);
+ mctp_register_bus(ctx.bmc.mctp, &ctx.bmc.astlpc->binding, 8);
+
+ /* Host initialisation */
+ ctx.host.mmio.bmc = false;
+ ctx.host.mctp = mctp_init();
+ assert(ctx.host.mctp);
+ ctx.host.mmio.kcs = &kcs;
+ ctx.host.mmio.lpc = ctx.lpc_mem;
+ ctx.host.mmio.lpc_size = LPC_WIN_SIZE;
+ ctx.host.astlpc =
+ mctp_astlpc_init(MCTP_BINDING_ASTLPC_MODE_HOST, MCTP_BTU, NULL,
+ &astlpc_indirect_mmio_ops, &ctx.host.mmio);
+ mctp_register_bus(ctx.host.mctp, &ctx.host.astlpc->binding, 9);
+
+ /* BMC processes host channel init request, alerts host */
+ mctp_astlpc_poll(ctx.bmc.astlpc);
+
+ /* Host dequeues channel init result */
+ mctp_astlpc_poll(ctx.host.astlpc);
+
+ ctx.msg = &msg[0];
+ ctx.count = 0;
+ mctp_set_rx_all(ctx.host.mctp, rx_message, &ctx);
+
+ /* BMC sends the single-packet message */
+ rc = mctp_message_tx(ctx.bmc.mctp, 9, msg, sizeof(msg));
+ assert(rc == 0);
+
+ /* Host receives the single-packet message */
+ rc = mctp_astlpc_poll(ctx.host.astlpc);
+ assert(rc == 0);
+ assert(ctx.count == 1);
+
+ /* BMC dequeues ownership hand-over and sends the queued packet */
+ rc = mctp_astlpc_poll(ctx.bmc.astlpc);
+ assert(rc == 0);
+
+ /* Can still tear-down the network in the normal fashion */
+ network_destroy(&ctx);
+}
+
/* clang-format off */
#define TEST_CASE(test) { #test, test }
static const struct {
@@ -432,6 +493,7 @@
TEST_CASE(astlpc_test_simple_message_bmc_to_host),
TEST_CASE(astlpc_test_simple_message_host_to_bmc),
TEST_CASE(astlpc_test_packetised_message_bmc_to_host),
+ TEST_CASE(astlpc_test_simple_indirect_message_bmc_to_host)
};
/* clang-format on */