Namhyung Kim
2016-Jul-18 04:37 UTC
[PATCH 1/3] virtio: Basic implementation of virtio pstore driver
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 at redhat.com>
Cc: Radim Kr?m?? <rkrcmar at redhat.com>
Cc: "Michael S. Tsirkin" <mst at redhat.com>
Cc: Anthony Liguori <aliguori at amazon.com>
Cc: Anton Vorontsov <anton at enomsg.org>
Cc: Colin Cross <ccross at android.com>
Cc: Kees Cook <keescook at chromium.org>
Cc: Tony Luck <tony.luck at intel.com>
Cc: Steven Rostedt <rostedt at goodmis.org>
Cc: Ingo Molnar <mingo at kernel.org>
Cc: Minchan Kim <minchan at kernel.org>
Cc: kvm at vger.kernel.org
Cc: qemu-devel at nongnu.org
Cc: virtualization at lists.linux-foundation.org
Signed-off-by: Namhyung Kim <namhyung at kernel.org>
---
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;
+ }
+
+ 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;
+}
+
+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;
+ 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 at 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
Namhyung Kim
2016-Jul-18 05:50 UTC
[PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Hello, On Sun, Jul 17, 2016 at 10:12:26PM -0700, Kees Cook wrote:> On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim <namhyung at 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 <pbonzini at redhat.com> > > Cc: Radim Kr??m???? <rkrcmar at redhat.com> > > Cc: "Michael S. Tsirkin" <mst at redhat.com> > > Cc: Anthony Liguori <aliguori at amazon.com> > > Cc: Anton Vorontsov <anton at enomsg.org> > > Cc: Colin Cross <ccross at android.com> > > Cc: Kees Cook <keescook at chromium.org> > > Cc: Tony Luck <tony.luck at intel.com> > > Cc: Steven Rostedt <rostedt at goodmis.org> > > Cc: Ingo Molnar <mingo at kernel.org> > > Cc: Minchan Kim <minchan at kernel.org> > > Cc: kvm at vger.kernel.org > > Cc: qemu-devel at nongnu.org > > Cc: virtualization at lists.linux-foundation.org > > Signed-off-by: Namhyung Kim <namhyung at 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 <keescook at chromium.org>Thank you!> > Notes below... >[SNIP]> > +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?Do you want to append the data to the host file as guest does printk()? I think it needs some kind of buffer management, but it's not hard to add IMHO.> > > + > > + return ret; > > +} > > +[SNIP]> > +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?I cannot follow, sorry. Could you please elaborate it more?> > > +} > > + > > +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.:)Yep, I intentionally support DMESG type only in this patchset for simplicity. Others could be added later. :)> > > + psinfo->data = vps; > > + spin_lock_init(&psinfo->buf_lock); > > + > > + err = pstore_register(psinfo); > > + if (err) > > + kfree(psinfo->buf); > > + > > + return err; > > +}[SNIP]> > Awesome! Can't wait to use it. :)Thanks for your review! :) Thanks, Namhyung> > -Kees > > -- > Kees Cook > Chrome OS & Brillo Security
Namhyung Kim
2016-Jul-18 08:29 UTC
[PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Hello, On Mon, Jul 18, 2016 at 09:54:39AM +0200, Cornelia Huck wrote:> On Mon, 18 Jul 2016 13:37:39 +0900 > Namhyung Kim <namhyung at 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. > > Like the idea.Thanks!> > > > > It supports legacy PCI device using single order-2 page buffer. As all > > There should not be anything in there that limits this to pci, no?Yep, there's no restriction AFAIK. I just choose it to implement the poc code quickly.> > > 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 at redhat.com> > > Cc: Radim Kr??m???? <rkrcmar at redhat.com> > > Cc: "Michael S. Tsirkin" <mst at redhat.com> > > Cc: Anthony Liguori <aliguori at amazon.com> > > Cc: Anton Vorontsov <anton at enomsg.org> > > Cc: Colin Cross <ccross at android.com> > > Cc: Kees Cook <keescook at chromium.org> > > Cc: Tony Luck <tony.luck at intel.com> > > Cc: Steven Rostedt <rostedt at goodmis.org> > > Cc: Ingo Molnar <mingo at kernel.org> > > Cc: Minchan Kim <minchan at kernel.org> > > Cc: kvm at vger.kernel.org > > Cc: qemu-devel at nongnu.org > > Cc: virtualization at lists.linux-foundation.org > > Signed-off-by: Namhyung Kim <namhyung at kernel.org> > > --- > > 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/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) > > It may make sense to make the size of the buffer configurable through > the config space.Right. I'm considering it too, but it needs a buffer larger than kmsg_bytes (= 10K) to work properly in the current implementation. As this version is just to verify the idea is sane and useful, I used a fixed size buffer. Will change in the next version.> > (...) > > > 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 */ > > This id is already used by one of the new device types queued but not > yet in the standard. IIRC, 22 is the next free one.Ok, will update.> > Speaking of the standard: I think it makes sense to at least reserve a > device id for pstore, as the idea is sound. Maybe prepare a patch to > the standard as well if you have time?I'd love to. As I mentioned earlier, I don't have enough knowledge in this area. Could you please provide some links about how can I do that? Thanks, Namhyung
Possibly Parallel Threads
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver