Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

From: Kees Cook
Date: Mon Jul 18 2016 - 01:12:36 EST

Next message: Kees Cook: "Re: linux-next: manual merge of the kspp tree with the arm64 tree"
Previous message: Juergen Gross: "Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection"
In reply to: Namhyung Kim: "[PATCH 1/3] virtio: Basic implementation of virtio pstore driver"
Next in thread: Namhyung Kim: "Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim <namhyung@xxxxxxxxxx> 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 <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> Cc: Anthony Liguori <aliguori@xxxxxxxxxx>
> Cc: Anton Vorontsov <anton@xxxxxxxxxx>
> Cc: Colin Cross <ccross@xxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Tony Luck <tony.luck@xxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Minchan Kim <minchan@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: qemu-devel@xxxxxxxxxx
> Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>

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 <keescook@xxxxxxxxxxxx>

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 <namhyung@xxxxxxxxxx>");
> +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

Next message: Kees Cook: "Re: linux-next: manual merge of the kspp tree with the arm64 tree"
Previous message: Juergen Gross: "Re: [PATCH 2/3] xen-scsiback: One function call less in scsiback_device_action() after error detection"
In reply to: Namhyung Kim: "[PATCH 1/3] virtio: Basic implementation of virtio pstore driver"
Next in thread: Namhyung Kim: "Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]