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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux