Date: Sat, 27 Apr 2024 17:30:25 +0000 From: Colin Percival <cperciva@tarsnap.com> To: Andrew Turner <andrew@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Cc: Isaac Cilia Attard <iciliaat@gmail.com> Subject: Re: git: a87dd74125d2 - main - scmi: Add an SCMI VirtIO transport driver Message-ID: <0100018f209bb336-8c67fe6d-09d3-4ec3-84ec-9f442360c28b-000000@email.amazonses.com> In-Reply-To: <202404110959.43B9xVBJ090851@gitrepo.freebsd.org> References: <202404110959.43B9xVBJ090851@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Andrew, It looks like this breaks arm64 kernel configs without 'device virtio_scmi' since sys/dev/firmware/arm/scmi_virtio.c (which is 'optional fdt scmi') uses functions from sys/dev/virtio/scmi/virtio_scmi.c (which is 'optional virtio_scmi'). In particular, the "ARM" kernel configuration (whcih has std.dev and std.arm but not std.virt) breaks. Reported by Isaac Cilia Attard (CCed). Colin Percival On 4/11/24 02:59, Andrew Turner wrote: > The branch main has been updated by andrew: > > URL: https://cgit.FreeBSD.org/src/commit/?id=a87dd74125d290791d7259ceeab9507bada9987e > > commit a87dd74125d290791d7259ceeab9507bada9987e > Author: Cristian Marussi <cristian.marussi@arm.com> > AuthorDate: 2023-10-23 18:07:06 +0000 > Commit: Andrew Turner <andrew@FreeBSD.org> > CommitDate: 2024-04-11 09:58:57 +0000 > > scmi: Add an SCMI VirtIO transport driver > > Add an SCMI transport driver based on the virtio-scmi backend. > > Reviewed by: andrew, bryanv > Sponsored by: Arm Ltd > Differential Revision: https://reviews.freebsd.org/D43048 > --- > sys/conf/files.arm64 | 1 + > sys/dev/firmware/arm/scmi.c | 15 +- > sys/dev/firmware/arm/scmi.h | 2 + > sys/dev/firmware/arm/scmi_virtio.c | 298 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 313 insertions(+), 3 deletions(-) > > diff --git a/sys/conf/files.arm64 b/sys/conf/files.arm64 > index a22ffa2b7f0c..632fbab5070d 100644 > --- a/sys/conf/files.arm64 > +++ b/sys/conf/files.arm64 > @@ -275,6 +275,7 @@ dev/firmware/arm/scmi_clk.c optional fdt scmi > dev/firmware/arm/scmi_if.m optional fdt scmi > dev/firmware/arm/scmi_mailbox.c optional fdt scmi > dev/firmware/arm/scmi_smc.c optional fdt scmi > +dev/firmware/arm/scmi_virtio.c optional fdt scmi > dev/firmware/arm/scmi_shmem.c optional fdt scmi > > dev/gpio/pl061.c optional pl061 gpio > diff --git a/sys/dev/firmware/arm/scmi.c b/sys/dev/firmware/arm/scmi.c > index 945c2b2e9f6e..ef4bcbf13996 100644 > --- a/sys/dev/firmware/arm/scmi.c > +++ b/sys/dev/firmware/arm/scmi.c > @@ -484,10 +484,19 @@ scmi_process_response(struct scmi_softc *sc, uint32_t hdr) > > mtx_lock_spin(&req->mtx); > req->done = true; > - if (!req->timed_out) > - wakeup(req); > - else > + if (!req->timed_out) { > + /* > + * Consider the case in which a polled message is picked > + * by chance on the IRQ path on another CPU: setting poll_done > + * will terminate the other poll loop. > + */ > + if (!req->msg.polling) > + wakeup(req); > + else > + atomic_store_rel_int(&req->msg.poll_done, 1); > + } else { > timed_out = true; > + } > mtx_unlock_spin(&req->mtx); > > if (timed_out) > diff --git a/sys/dev/firmware/arm/scmi.h b/sys/dev/firmware/arm/scmi.h > index 572422594292..345ae6eeb03a 100644 > --- a/sys/dev/firmware/arm/scmi.h > +++ b/sys/dev/firmware/arm/scmi.h > @@ -62,12 +62,14 @@ struct scmi_softc { > > struct scmi_msg { > bool polling; > + int poll_done; > uint32_t tx_len; > uint32_t rx_len; > #define SCMI_MSG_HDR_SIZE (sizeof(uint32_t)) > uint32_t hdr; > uint8_t payld[]; > }; > +#define hdr_to_msg(h) __containerof((h), struct scmi_msg, hdr) > > void *scmi_buf_get(device_t dev, uint8_t protocol_id, uint8_t message_id, > int tx_payd_sz, int rx_payld_sz); > diff --git a/sys/dev/firmware/arm/scmi_virtio.c b/sys/dev/firmware/arm/scmi_virtio.c > new file mode 100644 > index 000000000000..12cbb9ecefd5 > --- /dev/null > +++ b/sys/dev/firmware/arm/scmi_virtio.c > @@ -0,0 +1,298 @@ > +/*- > + * SPDX-License-Identifier: BSD-2-Clause > + * > + * Copyright (c) 2023 Arm Ltd > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include <sys/cdefs.h> > +#include <sys/param.h> > +#include <sys/systm.h> > +#include <sys/bus.h> > +#include <sys/cpu.h> > +#include <sys/kernel.h> > +#include <sys/module.h> > + > +#include <dev/fdt/simplebus.h> > +#include <dev/fdt/fdt_common.h> > +#include <dev/ofw/ofw_bus_subr.h> > + > +#include <dev/virtio/scmi/virtio_scmi.h> > + > +#include "scmi.h" > +#include "scmi_protocols.h" > + > +#define SCMI_VIRTIO_POLLING_INTERVAL_MS 2 > + > +struct scmi_virtio_softc { > + struct scmi_softc base; > + device_t virtio_dev; > + int cmdq_sz; > + int evtq_sz; > + void *p2a_pool; > +}; > + > +static void scmi_virtio_callback(void *, unsigned int, void *); > +static void *scmi_virtio_p2a_pool_init(device_t, unsigned int); > +static int scmi_virtio_transport_init(device_t); > +static void scmi_virtio_transport_cleanup(device_t); > +static int scmi_virtio_xfer_msg(device_t, struct scmi_msg *); > +static int scmi_virtio_poll_msg(device_t, struct scmi_msg *, unsigned int); > +static void scmi_virtio_clear_channel(device_t, void *); > +static int scmi_virtio_probe(device_t); > +static int scmi_virtio_attach(device_t); > + > +static void > +scmi_virtio_callback(void *msg, unsigned int len, void *priv) > +{ > + struct scmi_virtio_softc *sc; > + uint32_t hdr; > + > + sc = priv; > + > + if (msg == NULL || len < sizeof(hdr)) { > + device_printf(sc->virtio_dev, "Ignoring malformed message.\n"); > + return; > + } > + > + hdr = le32toh(*((uint32_t *)msg)); > + scmi_rx_irq_callback(sc->base.dev, msg, hdr); > +} > + > +static void * > +scmi_virtio_p2a_pool_init(device_t dev, unsigned int max_msg) > +{ > + struct scmi_virtio_softc *sc; > + void *pool; > + uint8_t *buf; > + int i; > + > + sc = device_get_softc(dev); > + > + pool = mallocarray(max_msg, SCMI_MAX_MSG_SIZE, M_DEVBUF, > + M_ZERO | M_WAITOK); > + > + for (i = 0, buf = pool; i < max_msg; i++, buf += SCMI_MAX_MSG_SIZE) { > + /* Feed platform with pre-allocated P2A buffers */ > + virtio_scmi_message_enqueue(sc->virtio_dev, > + VIRTIO_SCMI_CHAN_P2A, buf, 0, SCMI_MAX_MSG_SIZE); > + } > + > + device_printf(dev, > + "Fed %d initial P2A buffers to platform.\n", max_msg); > + > + return (pool); > +} > + > +static void > +scmi_virtio_clear_channel(device_t dev, void *msg) > +{ > + struct scmi_virtio_softc *sc; > + > + sc = device_get_softc(dev); > + virtio_scmi_message_enqueue(sc->virtio_dev, VIRTIO_SCMI_CHAN_P2A, > + msg, 0, SCMI_MAX_MSG_SIZE); > +} > + > +static int > +scmi_virtio_transport_init(device_t dev) > +{ > + struct scmi_virtio_softc *sc; > + int ret; > + > + sc = device_get_softc(dev); > + > + sc->cmdq_sz = virtio_scmi_channel_size_get(sc->virtio_dev, > + VIRTIO_SCMI_CHAN_A2P); > + sc->evtq_sz = virtio_scmi_channel_size_get(sc->virtio_dev, > + VIRTIO_SCMI_CHAN_P2A); > + > + if (!sc->cmdq_sz) { > + device_printf(dev, > + "VirtIO cmdq virtqueue not found. Aborting.\n"); > + return (ENXIO); > + } > + > + /* > + * P2A buffers are owned by the platform initially; allocate a feed an > + * appropriate number of buffers. > + */ > + if (sc->evtq_sz != 0) { > + sc->p2a_pool = scmi_virtio_p2a_pool_init(dev, sc->evtq_sz); > + if (sc->p2a_pool == NULL) > + return (ENOMEM); > + } > + > + /* Note that setting a callback also enables that VQ interrupts */ > + ret = virtio_scmi_channel_callback_set(sc->virtio_dev, > + VIRTIO_SCMI_CHAN_A2P, scmi_virtio_callback, sc); > + if (ret) { > + device_printf(dev, "Failed to set VirtIO cmdq callback.\n"); > + return (ENXIO); > + } > + > + device_printf(dev, > + "VirtIO cmdq virtqueue configured - cmdq_sz:%d\n", sc->cmdq_sz); > + > + /* P2A channel is optional */ > + if (sc->evtq_sz) { > + ret = virtio_scmi_channel_callback_set(sc->virtio_dev, > + VIRTIO_SCMI_CHAN_P2A, scmi_virtio_callback, sc); > + if (ret == 0) { > + device_printf(dev, > + "VirtIO evtq virtqueue configured - evtq_sz:%d\n", > + sc->evtq_sz); > + } else { > + device_printf(dev, > + "Failed to set VirtIO evtq callback.Skip.\n"); > + sc->evtq_sz = 0; > + } > + } > + > + sc->base.trs_desc.reply_timo_ms = 100; > + > + return (0); > +} > + > +static void > +scmi_virtio_transport_cleanup(device_t dev) > +{ > + struct scmi_virtio_softc *sc; > + > + sc = device_get_softc(dev); > + > + if (sc->evtq_sz != 0) { > + virtio_scmi_channel_callback_set(sc->virtio_dev, > + VIRTIO_SCMI_CHAN_P2A, NULL, NULL); > + free(sc->p2a_pool, M_DEVBUF); > + } > + > + virtio_scmi_channel_callback_set(sc->virtio_dev, > + VIRTIO_SCMI_CHAN_A2P, NULL, NULL); > +} > + > +static int > +scmi_virtio_xfer_msg(device_t dev, struct scmi_msg *msg) > +{ > + struct scmi_virtio_softc *sc; > + > + sc = device_get_softc(dev); > + > + return (virtio_scmi_message_enqueue(sc->virtio_dev, > + VIRTIO_SCMI_CHAN_A2P, &msg->hdr, msg->tx_len, msg->rx_len)); > +} > + > +static int > +scmi_virtio_poll_msg(device_t dev, struct scmi_msg *msg, unsigned int tmo_ms) > +{ > + struct scmi_virtio_softc *sc; > + device_t vdev; > + int tmo_loops; > + > + sc = device_get_softc(dev); > + vdev = sc->virtio_dev; > + > + tmo_loops = tmo_ms / SCMI_VIRTIO_POLLING_INTERVAL_MS; > + while (tmo_loops-- && atomic_load_acq_int(&msg->poll_done) == 0) { > + struct scmi_msg *rx_msg; > + void *rx_buf; > + uint32_t rx_len; > + > + rx_buf = virtio_scmi_message_poll(vdev, &rx_len); > + if (rx_buf == NULL) { > + DELAY(SCMI_VIRTIO_POLLING_INTERVAL_MS * 1000); > + continue; > + } > + > + rx_msg = hdr_to_msg(rx_buf); > + rx_msg->rx_len = rx_len; > + /* Complete the polling on any poll path */ > + if (rx_msg->polling) > + atomic_store_rel_int(&rx_msg->poll_done, 1); > + > + if (__predict_true(rx_msg == msg)) > + break; > + > + /* > + * Polling returned an unexpected message: either a message > + * polled by some other thread of execution or a message not > + * supposed to be polled. > + */ > + device_printf(dev, "POLLED OoO HDR:|%08X| - polling:%d\n", > + rx_msg->hdr, rx_msg->polling); > + > + if (!rx_msg->polling) > + scmi_rx_irq_callback(sc->base.dev, rx_msg, rx_msg->hdr); > + } > + > + return (tmo_loops > 0 ? 0 : ETIMEDOUT); > +} > + > +static int > +scmi_virtio_probe(device_t dev) > +{ > + if (!ofw_bus_is_compatible(dev, "arm,scmi-virtio")) > + return (ENXIO); > + > + if (!ofw_bus_status_okay(dev)) > + return (ENXIO); > + > + device_set_desc(dev, "ARM SCMI VirtIO Transport driver"); > + > + return (BUS_PROBE_DEFAULT); > +} > + > +static int > +scmi_virtio_attach(device_t dev) > +{ > + struct scmi_virtio_softc *sc; > + > + sc = device_get_softc(dev); > + sc->virtio_dev = virtio_scmi_transport_get(); > + if (sc->virtio_dev == NULL) > + return (1); > + > + /* When attach fails there is nothing to cleanup*/ > + return (scmi_attach(dev)); > +} > + > +static device_method_t scmi_virtio_methods[] = { > + DEVMETHOD(device_probe, scmi_virtio_probe), > + DEVMETHOD(device_attach, scmi_virtio_attach), > + > + /* SCMI interface */ > + DEVMETHOD(scmi_transport_init, scmi_virtio_transport_init), > + DEVMETHOD(scmi_transport_cleanup, scmi_virtio_transport_cleanup), > + DEVMETHOD(scmi_xfer_msg, scmi_virtio_xfer_msg), > + DEVMETHOD(scmi_poll_msg, scmi_virtio_poll_msg), > + DEVMETHOD(scmi_clear_channel, scmi_virtio_clear_channel), > + > + DEVMETHOD_END > +}; > + > +DEFINE_CLASS_1(scmi_virtio, scmi_virtio_driver, scmi_virtio_methods, > + sizeof(struct scmi_virtio_softc), scmi_driver); > + > +/* Needs to be after the mmio_sram driver */ > +DRIVER_MODULE(scmi_virtio, simplebus, scmi_virtio_driver, 0, 0); > +MODULE_VERSION(scmi_virtio, 1); > -- Colin Percival FreeBSD Release Engineering Lead & EC2 platform maintainer Founder, Tarsnap | www.tarsnap.com | Online backups for the truly paranoid
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0100018f209bb336-8c67fe6d-09d3-4ec3-84ec-9f442360c28b-000000>