On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim <namhy...@kernel.org> wrote: > The virtio pstore driver provides interface to the pstore subsystem so > that the guest kernel's log/dump message can be saved on the host > machine. Users can access the log file directly on the host, or on the > guest at the next boot using pstore filesystem. It currently deals with > kernel log (printk) buffer only, but we can extend it to have other > information (like ftrace dump) later. > > It supports legacy PCI device using single order-2 page buffer. As all > operation of pstore is synchronous, it would be fine IMHO. However I > don't know how to make write operation synchronous since it's called > with a spinlock held (from any context including NMI). > > Cc: Paolo Bonzini <pbonz...@redhat.com> > Cc: Radim Krčmář <rkrc...@redhat.com> > Cc: "Michael S. Tsirkin" <m...@redhat.com> > Cc: Anthony Liguori <aligu...@amazon.com> > Cc: Anton Vorontsov <an...@enomsg.org> > Cc: Colin Cross <ccr...@android.com> > Cc: Kees Cook <keesc...@chromium.org> > Cc: Tony Luck <tony.l...@intel.com> > Cc: Steven Rostedt <rost...@goodmis.org> > Cc: Ingo Molnar <mi...@kernel.org> > Cc: Minchan Kim <minc...@kernel.org> > Cc: k...@vger.kernel.org > Cc: qemu-de...@nongnu.org > Cc: virtualizat...@lists.linux-foundation.org > Signed-off-by: Namhyung Kim <namhy...@kernel.org>
This looks great to me! I'd love to use this in qemu. (Right now I go through hoops to use the ramoops backend for testing.) Reviewed-by: Kees Cook <keesc...@chromium.org> Notes below... > --- > drivers/virtio/Kconfig | 10 ++ > drivers/virtio/Makefile | 1 + > drivers/virtio/virtio_pstore.c | 317 > +++++++++++++++++++++++++++++++++++++ > include/uapi/linux/Kbuild | 1 + > include/uapi/linux/virtio_ids.h | 1 + > include/uapi/linux/virtio_pstore.h | 53 +++++++ > 6 files changed, 383 insertions(+) > create mode 100644 drivers/virtio/virtio_pstore.c > create mode 100644 include/uapi/linux/virtio_pstore.h > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig > index 77590320d44c..8f0e6c796c12 100644 > --- a/drivers/virtio/Kconfig > +++ b/drivers/virtio/Kconfig > @@ -58,6 +58,16 @@ config VIRTIO_INPUT > > If unsure, say M. > > +config VIRTIO_PSTORE > + tristate "Virtio pstore driver" > + depends on VIRTIO > + depends on PSTORE > + ---help--- > + This driver supports virtio pstore devices to save/restore > + panic and oops messages on the host. > + > + If unsure, say M. > + > config VIRTIO_MMIO > tristate "Platform bus driver for memory mapped virtio devices" > depends on HAS_IOMEM && HAS_DMA > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile > index 41e30e3dc842..bee68cb26d48 100644 > --- a/drivers/virtio/Makefile > +++ b/drivers/virtio/Makefile > @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o > virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o > obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o > obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o > +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o > diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c > new file mode 100644 > index 000000000000..6fe62c0f1508 > --- /dev/null > +++ b/drivers/virtio/virtio_pstore.c > @@ -0,0 +1,317 @@ > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/pstore.h> > +#include <linux/virtio.h> > +#include <linux/virtio_config.h> > +#include <uapi/linux/virtio_ids.h> > +#include <uapi/linux/virtio_pstore.h> > + > +#define VIRT_PSTORE_ORDER 2 > +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER) > + > +struct virtio_pstore { > + struct virtio_device *vdev; > + struct virtqueue *vq; > + struct pstore_info pstore; > + struct virtio_pstore_hdr hdr; > + size_t buflen; > + u64 id; > + > + /* Waiting for host to ack */ > + wait_queue_head_t acked; > +}; > + > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id > type) > +{ > + u16 ret; > + > + switch (type) { > + case PSTORE_TYPE_DMESG: > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG); > + break; > + default: > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); > + break; > + } I would love to see this support PSTORE_TYPE_CONSOLE too. It should be relatively easy to add: I think it'd just be another virtio command? > + > + return ret; > +} > + > +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 > type) > +{ > + enum pstore_type_id ret; > + > + switch (virtio16_to_cpu(vps->vdev, type)) { > + case VIRTIO_PSTORE_TYPE_DMESG: > + ret = PSTORE_TYPE_DMESG; > + break; > + default: > + ret = PSTORE_TYPE_UNKNOWN; > + break; > + } > + > + return ret; > +} > + > +static void virtpstore_ack(struct virtqueue *vq) > +{ > + struct virtio_pstore *vps = vq->vdev->priv; > + > + wake_up(&vps->acked); > +} > + > +static int virt_pstore_open(struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_hdr *hdr = &vps->hdr; > + struct scatterlist sg[1]; > + unsigned int len; > + > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN); > + > + sg_init_one(sg, hdr, sizeof(*hdr)); > + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len)); > + return 0; > +} > + > +static int virt_pstore_close(struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_hdr *hdr = &vps->hdr; > + struct scatterlist sg[1]; > + unsigned int len; > + > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_CLOSE); > + > + sg_init_one(sg, hdr, sizeof(*hdr)); > + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len)); > + return 0; > +} > + > +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type, > + int *count, struct timespec *time, > + char **buf, bool *compressed, > + struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_hdr *hdr = &vps->hdr; > + struct scatterlist sgi[1], sgo[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + unsigned int flags; > + void *bf; > + > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_READ); > + > + sg_init_one(sgo, hdr, sizeof(*hdr)); > + sg_init_one(sgi, psi->buf, psi->bufsize); > + virtqueue_add_sgs(vps->vq, sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len)); > + if (len == 0) > + return 0; > + > + bf = kmalloc(len, GFP_KERNEL); > + if (bf == NULL) > + return -ENOMEM; > + > + *id = virtio64_to_cpu(vps->vdev, hdr->id); > + *type = from_virtio_type(vps, hdr->type); > + > + flags = virtio32_to_cpu(vps->vdev, hdr->flags); > + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED; > + *count = 1; > + > + time->tv_sec = virtio64_to_cpu(vps->vdev, hdr->time_sec); > + time->tv_nsec = virtio32_to_cpu(vps->vdev, hdr->time_nsec); > + > + memcpy(bf, psi->buf, len); > + *buf = bf; > + > + return len; > +} > + > +static int notrace virt_pstore_write(enum pstore_type_id type, > + enum kmsg_dump_reason reason, > + u64 *id, unsigned int part, int count, > + bool compressed, size_t size, > + struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_hdr *hdr = &vps->hdr; > + struct scatterlist sg[2]; > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0; > + > + *id = vps->id++; > + > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE); > + hdr->id = cpu_to_virtio64(vps->vdev, *id); > + hdr->flags = cpu_to_virtio32(vps->vdev, flags); > + hdr->type = to_virtio_type(vps, type); > + > + sg_init_table(sg, 2); > + sg_set_buf(&sg[0], hdr, sizeof(*hdr)); > + sg_set_buf(&sg[1], psi->buf, size); > + virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC); > + virtqueue_kick(vps->vq); > + > + /* TODO: make it synchronous */ > + return 0; The down side to this being asynchronous is the lack of error reporting. Perhaps this could check hdr->type before queuing and error for any VIRTIO_PSTORE_TYPE_UNKNOWN message instead of trying to send it? > +} > + > +static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count, > + struct timespec time, struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_hdr *hdr = &vps->hdr; > + struct scatterlist sg[1]; > + unsigned int len; > + > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE); > + hdr->id = cpu_to_virtio64(vps->vdev, id); > + hdr->type = to_virtio_type(vps, type); > + > + sg_init_one(sg, hdr, sizeof(*hdr)); > + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len)); > + return 0; > +} > + > +static int virt_pstore_init(struct virtio_pstore *vps) > +{ > + struct pstore_info *psinfo = &vps->pstore; > + int err; > + > + vps->id = 0; > + vps->buflen = 0; > + psinfo->bufsize = VIRT_PSTORE_BUFSIZE; > + psinfo->buf = (void *)__get_free_pages(GFP_KERNEL, VIRT_PSTORE_ORDER); > + if (!psinfo->buf) { > + pr_err("cannot allocate pstore buffer\n"); > + return -ENOMEM; > + } > + > + psinfo->owner = THIS_MODULE; > + psinfo->name = "virtio"; > + psinfo->open = virt_pstore_open; > + psinfo->close = virt_pstore_close; > + psinfo->read = virt_pstore_read; > + psinfo->erase = virt_pstore_erase; > + psinfo->write = virt_pstore_write; > + psinfo->flags = PSTORE_FLAGS_FRAGILE; For console support, this flag would need to be dropped -- though I suspect you know that already.:) > + psinfo->data = vps; > + spin_lock_init(&psinfo->buf_lock); > + > + err = pstore_register(psinfo); > + if (err) > + kfree(psinfo->buf); > + > + return err; > +} > + > +static int virt_pstore_exit(struct virtio_pstore *vps) > +{ > + struct pstore_info *psinfo = &vps->pstore; > + > + pstore_unregister(psinfo); > + > + free_pages((unsigned long)psinfo->buf, VIRT_PSTORE_ORDER); > + psinfo->bufsize = 0; > + > + return 0; > +} > + > +static int virtpstore_probe(struct virtio_device *vdev) > +{ > + struct virtio_pstore *vps; > + int err; > + > + if (!vdev->config->get) { > + dev_err(&vdev->dev, "%s failure: config access disabled\n", > + __func__); > + return -EINVAL; > + } > + > + vdev->priv = vps = kmalloc(sizeof(*vps), GFP_KERNEL); > + if (!vps) { > + err = -ENOMEM; > + goto out; > + } > + > + vps->vdev = vdev; > + > + vps->vq = virtio_find_single_vq(vdev, virtpstore_ack, "pstore"); > + if (IS_ERR(vps->vq)) { > + err = PTR_ERR(vps->vq); > + goto out_free; > + } > + > + err = virt_pstore_init(vps); > + if (err) > + goto out_del_vq; > + > + init_waitqueue_head(&vps->acked); > + > + virtio_device_ready(vdev); > + dev_info(&vdev->dev, "virtio pstore driver init: ok\n"); > + > + return 0; > + > +out_del_vq: > + vdev->config->del_vqs(vdev); > +out_free: > + kfree(vps); > +out: > + dev_err(&vdev->dev, "virtio pstore driver init: failed with %d\n", > err); > + return err; > +} > + > +static void virtpstore_remove(struct virtio_device *vdev) > +{ > + struct virtio_pstore *vps = vdev->priv; > + > + virt_pstore_exit(vps); > + > + /* Now we reset the device so we can clean up the queues. */ > + vdev->config->reset(vdev); > + > + vdev->config->del_vqs(vdev); > + > + kfree(vps); > +} > + > +static unsigned int features[] = { > +}; > + > +static struct virtio_device_id id_table[] = { > + { VIRTIO_ID_PSTORE, VIRTIO_DEV_ANY_ID }, > + { 0 }, > +}; > + > +static struct virtio_driver virtio_pstore_driver = { > + .driver.name = KBUILD_MODNAME, > + .driver.owner = THIS_MODULE, > + .feature_table = features, > + .feature_table_size = ARRAY_SIZE(features), > + .id_table = id_table, > + .probe = virtpstore_probe, > + .remove = virtpstore_remove, > +}; > + > +module_virtio_driver(virtio_pstore_driver); > +MODULE_DEVICE_TABLE(virtio, id_table); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Namhyung Kim <namhy...@kernel.org>"); > +MODULE_DESCRIPTION("Virtio pstore driver"); > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild > index 8bdae34d1f9a..57b0d08db322 100644 > --- a/include/uapi/linux/Kbuild > +++ b/include/uapi/linux/Kbuild > @@ -448,6 +448,7 @@ header-y += virtio_ids.h > header-y += virtio_input.h > header-y += virtio_net.h > header-y += virtio_pci.h > +header-y += virtio_pstore.h > header-y += virtio_ring.h > header-y += virtio_rng.h > header-y += virtio_scsi.h > diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h > index 77925f587b15..cba63225d85a 100644 > --- a/include/uapi/linux/virtio_ids.h > +++ b/include/uapi/linux/virtio_ids.h > @@ -41,5 +41,6 @@ > #define VIRTIO_ID_CAIF 12 /* Virtio caif */ > #define VIRTIO_ID_GPU 16 /* virtio GPU */ > #define VIRTIO_ID_INPUT 18 /* virtio input */ > +#define VIRTIO_ID_PSTORE 19 /* virtio pstore */ > > #endif /* _LINUX_VIRTIO_IDS_H */ > diff --git a/include/uapi/linux/virtio_pstore.h > b/include/uapi/linux/virtio_pstore.h > new file mode 100644 > index 000000000000..0aa1575ee35f > --- /dev/null > +++ b/include/uapi/linux/virtio_pstore.h > @@ -0,0 +1,53 @@ > +#ifndef _LINUX_VIRTIO_PSTORE_H > +#define _LINUX_VIRTIO_PSTORE_H > +/* This header is BSD licensed so anyone can use the definitions to implement > + * compatible drivers/servers. > + * > + * 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. > + * 3. Neither the name of IBM nor the names of its contributors > + * may be used to endorse or promote products derived from this software > + * without specific prior written permission. > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS 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 IBM 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 <linux/types.h> > +#include <linux/virtio_types.h> > + > +#define VIRTIO_PSTORE_CMD_NULL 0 > +#define VIRTIO_PSTORE_CMD_OPEN 1 > +#define VIRTIO_PSTORE_CMD_READ 2 > +#define VIRTIO_PSTORE_CMD_WRITE 3 > +#define VIRTIO_PSTORE_CMD_ERASE 4 > +#define VIRTIO_PSTORE_CMD_CLOSE 5 > + > +#define VIRTIO_PSTORE_TYPE_UNKNOWN 0 > +#define VIRTIO_PSTORE_TYPE_DMESG 1 > + > +#define VIRTIO_PSTORE_FL_COMPRESSED 1 > + > +struct virtio_pstore_hdr { > + __virtio64 id; > + __virtio32 flags; > + __virtio16 cmd; > + __virtio16 type; > + __virtio64 time_sec; > + __virtio32 time_nsec; > + __virtio32 unused; > +}; > + > +#endif /* _LINUX_VIRTIO_PSTORE_H */ > -- > 2.8.0 > Awesome! Can't wait to use it. :) -Kees -- Kees Cook Chrome OS & Brillo Security