| Message ID | 20160820080744.10344-2-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. It uses > two virtqueues - one for (sync) read and another for (async) write. > Since it cannot wait for write finished, it supports up to 128 > concurrent IO. The buffer size is configurable now. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Anthony Liguori <aliguori@amazon.com> > Cc: Anton Vorontsov <anton@enomsg.org> > Cc: Colin Cross <ccross@android.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Minchan Kim <minchan@kernel.org> > Cc: kvm@vger.kernel.org > Cc: qemu-devel@nongnu.org > Cc: virtualization@lists.linux-foundation.org > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > drivers/virtio/Kconfig | 10 + > drivers/virtio/Makefile | 1 + > drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++ > include/uapi/linux/Kbuild | 1 + > include/uapi/linux/virtio_ids.h | 1 + > include/uapi/linux/virtio_pstore.h | 74 +++++++ > 6 files changed, 504 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..0a63c7db4278 > --- /dev/null > +++ b/drivers/virtio/virtio_pstore.c > @@ -0,0 +1,417 @@ > +#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) > +#define VIRT_PSTORE_NR_REQ 128 where are these numbers from? > + > +struct virtio_pstore { > + struct virtio_device *vdev; > + struct virtqueue *vq[2]; > + struct pstore_info pstore; > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ]; > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ]; > + unsigned int req_id; > + > + /* Waiting for host to ack */ > + wait_queue_head_t acked; > + int failed; > +}; > + > +#define TYPE_TABLE_ENTRY(_entry) \ > + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry } > + > +struct type_table { > + int pstore; > + u16 virtio; > +} type_table[] = { > + TYPE_TABLE_ENTRY(DMESG), > +}; > + > +#undef TYPE_TABLE_ENTRY Let's not play preprocessor games until this becomes a big issue. Simple { PSTORE_TYPE_DMESG, VIRTIO_PSTORE_TYPE_DMESG} does the trick just as well for now. Also see below. > + > + single empty line pls. > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > + if (type == type_table[i].pstore) > + return cpu_to_virtio16(vps->vdev, type_table[i].virtio); > + } Rather complex for something that always returns a single value. why do we need a table at all? How about a switch statement? static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) { switch (type) { case PSTORE_TYPE_DMESG: return VIRTIO_PSTORE_TYPE_DMESG; default: return VIRTIO_PSTORE_TYPE_UNKNOWN; } } > + > + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); > +} This returns an incorrect type. > + > +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio) > + return type_table[i].pstore; > + } > + > + return PSTORE_TYPE_UNKNOWN; > +} > + > +static void virtpstore_ack(struct virtqueue *vq) > +{ > + struct virtio_pstore *vps = vq->vdev->priv; > + > + wake_up(&vps->acked); > +} > + > +static void virtpstore_check(struct virtqueue *vq) > +{ > + struct virtio_pstore *vps = vq->vdev->priv; > + struct virtio_pstore_res *res; > + unsigned int len; > + > + res = virtqueue_get_buf(vq, &len); > + if (res == NULL) > + return; > + > + if (virtio32_to_cpu(vq->vdev, res->ret) < 0) > + vps->failed = 1; > +} > + > +static void virt_pstore_get_reqs(struct virtio_pstore *vps, > + struct virtio_pstore_req **preq, > + struct virtio_pstore_res **pres) > +{ > + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ; > + > + *preq = &vps->req[idx]; > + *pres = &vps->res[idx]; > + > + memset(*preq, 0, sizeof(**preq)); > + memset(*pres, 0, sizeof(**pres)); > +} > + > +static int virt_pstore_open(struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_req *req; > + struct virtio_pstore_res *res; > + struct scatterlist sgo[1], sgi[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN); > + > + sg_init_one(sgo, req, sizeof(*req)); > + sg_init_one(sgi, res, sizeof(*res)); > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq[0]); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > + return virtio32_to_cpu(vps->vdev, res->ret); > +} > + > +static int virt_pstore_close(struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_req *req = &vps->req[vps->req_id]; > + struct virtio_pstore_res *res = &vps->res[vps->req_id]; > + struct scatterlist sgo[1], sgi[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_CLOSE); > + > + sg_init_one(sgo, req, sizeof(*req)); > + sg_init_one(sgi, res, sizeof(*res)); > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq[0]); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > + return virtio32_to_cpu(vps->vdev, res->ret); > +} > + > +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type, > + int *count, struct timespec *time, > + char **buf, bool *compressed, > + ssize_t *ecc_notice_size, > + struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_req *req; > + struct virtio_pstore_res *res; > + struct virtio_pstore_fileinfo info; > + struct scatterlist sgo[1], sgi[3]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + unsigned int flags; > + int ret; > + void *bf; > + > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_READ); > + > + sg_init_one(sgo, req, sizeof(*req)); > + sg_init_table(sgi, 3); > + sg_set_buf(&sgi[0], res, sizeof(*res)); > + sg_set_buf(&sgi[1], &info, sizeof(info)); > + sg_set_buf(&sgi[2], psi->buf, psi->bufsize); > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq[0]); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > + if (len < sizeof(*res) + sizeof(info)) > + return -1; > + > + ret = virtio32_to_cpu(vps->vdev, res->ret); > + if (ret < 0) > + return ret; > + > + len = virtio32_to_cpu(vps->vdev, info.len); > + > + bf = kmalloc(len, GFP_KERNEL); > + if (bf == NULL) > + return -ENOMEM; > + > + *id = virtio64_to_cpu(vps->vdev, info.id); > + *type = from_virtio_type(vps, info.type); > + *count = virtio32_to_cpu(vps->vdev, info.count); > + > + flags = virtio32_to_cpu(vps->vdev, info.flags); > + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED; > + > + time->tv_sec = virtio64_to_cpu(vps->vdev, info.time_sec); > + time->tv_nsec = virtio32_to_cpu(vps->vdev, info.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_req *req; > + struct virtio_pstore_res *res; > + struct scatterlist sgo[2], sgi[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0; > + > + if (vps->failed) > + return -1; > + > + *id = vps->req_id; > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE); > + req->type = to_virtio_type(vps, type); > + req->flags = cpu_to_virtio32(vps->vdev, flags); > + > + sg_init_table(sgo, 2); > + sg_set_buf(&sgo[0], req, sizeof(*req)); > + sg_set_buf(&sgo[1], pstore_get_buf(psi), size); > + sg_init_one(sgi, res, sizeof(*res)); > + virtqueue_add_sgs(vps->vq[1], sgs, 1, 1, vps, GFP_ATOMIC); > + virtqueue_kick(vps->vq[1]); > + > + 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_req *req; > + struct virtio_pstore_res *res; > + struct scatterlist sgo[1], sgi[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE); > + req->type = to_virtio_type(vps, type); > + req->id = cpu_to_virtio64(vps->vdev, id); > + req->count = cpu_to_virtio32(vps->vdev, count); > + > + sg_init_one(sgo, req, sizeof(*req)); > + sg_init_one(sgi, res, sizeof(*res)); > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq[0]); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > + return virtio32_to_cpu(vps->vdev, res->ret); > +} > + > +static int virt_pstore_init(struct virtio_pstore *vps) > +{ > + struct pstore_info *psinfo = &vps->pstore; > + int err; > + > + if (!psinfo->bufsize) > + psinfo->bufsize = VIRT_PSTORE_BUFSIZE; > + > + psinfo->buf = alloc_pages_exact(psinfo->bufsize, GFP_KERNEL); > + 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_DMESG; > + > + 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); I don't know enough about pstore - does this actually ensure that 1. all existing users close the device 2. no new users can open it somehow? > + > + free_pages_exact(psinfo->buf, psinfo->bufsize); > + psinfo->buf = NULL; > + psinfo->bufsize = 0; > + > + return 0; > +} > + > +static int virtpstore_init_vqs(struct virtio_pstore *vps) > +{ > + vq_callback_t *callbacks[] = { virtpstore_ack, virtpstore_check }; > + const char *names[] = { "pstore_read", "pstore_write" }; > + > + return vps->vdev->config->find_vqs(vps->vdev, 2, vps->vq, > + callbacks, names); > +} > + > +static void virtpstore_init_config(struct virtio_pstore *vps) > +{ > + u32 bufsize; > + > + virtio_cread(vps->vdev, struct virtio_pstore_config, bufsize, &bufsize); > + > + vps->pstore.bufsize = PAGE_ALIGN(bufsize); > +} > + > +static void virtpstore_confirm_config(struct virtio_pstore *vps) > +{ > + u32 bufsize = vps->pstore.bufsize; > + > + virtio_cwrite(vps->vdev, struct virtio_pstore_config, bufsize, > + &bufsize); > +} > + > +static int virtpstore_probe(struct virtio_device *vdev) > +{ > + struct virtio_pstore *vps; > + int err; > + > + if (!vdev->config->get) { > + dev_err(&vdev->dev, "driver init: config access disabled\n"); > + return -EINVAL; > + } > + > + vdev->priv = vps = kzalloc(sizeof(*vps), GFP_KERNEL); > + if (!vps) { > + err = -ENOMEM; > + goto out; > + } > + vps->vdev = vdev; > + > + err = virtpstore_init_vqs(vps); > + if (err < 0) > + goto out_free; > + > + virtpstore_init_config(vps); > + > + err = virt_pstore_init(vps); > + if (err) > + goto out_del_vq; > + > + virtpstore_confirm_config(vps); > + > + init_waitqueue_head(&vps->acked); > + > + virtio_device_ready(vdev); > + > + dev_info(&vdev->dev, "driver init: ok (bufsize = %luK, flags = %x)\n", > + vps->pstore.bufsize >> 10, vps->pstore.flags); > + > + return 0; > + > +out_del_vq: > + vdev->config->del_vqs(vdev); > +out_free: > + kfree(vps); > +out: > + dev_err(&vdev->dev, "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 = { We need some way to avoid trying to load this as a legacy device. There isn't a way to do it yet so I won't block your patch on this but pls try to come up with something, and I'll do, too. > + .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, Won't this need freeze/restore callbacks? > +}; > + > +module_virtio_driver(virtio_pstore_driver); > +MODULE_DEVICE_TABLE(virtio, id_table); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Namhyung Kim <namhyung@kernel.org>"); > +MODULE_DESCRIPTION("Virtio pstore driver"); > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild > index 6d4e92ccdc91..9bbb1554d8b2 100644 > --- a/include/uapi/linux/Kbuild > +++ b/include/uapi/linux/Kbuild > @@ -449,6 +449,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..c72a9ab588c0 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 22 /* 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..f4b0d204d8ae > --- /dev/null > +++ b/include/uapi/linux/virtio_pstore.h > @@ -0,0 +1,74 @@ > +#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 Most other headers use _F_ and not _FL_ Also, we specify bit number and not the bitmask. So: #define VIRTIO_PSTORE_F_COMPRESSED 0 and (0x1 << VIRTIO_PSTORE_F_COMPRESSED) > + > +struct virtio_pstore_req { > + __virtio16 cmd; > + __virtio16 type; > + __virtio32 flags; > + __virtio64 id; > + __virtio32 count; > + __virtio32 reserved; > +}; > + > +struct virtio_pstore_res { > + __virtio16 cmd; > + __virtio16 type; > + __virtio32 ret; > +}; > + > +struct virtio_pstore_fileinfo { > + __virtio64 id; > + __virtio32 count; > + __virtio16 type; > + __virtio16 unused; > + __virtio32 flags; > + __virtio32 len; > + __virtio64 time_sec; > + __virtio32 time_nsec; > + __virtio32 reserved; Any reason one is reserved the other is unused? If not just calls them pad1, pad2? > +}; > + > +struct virtio_pstore_config { > + __virtio32 bufsize; > +}; > + __virtio things are for compatibility things. New devices should just use __le everywhere. Let me post a patch that adds config space accessors so you can do this. > +#endif /* _LINUX_VIRTIO_PSTORE_H */ > -- > 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Michael, Thanks for your detailed review. Btw are you ok with the overall direction of the patch? On Tue, Sep 13, 2016 at 06:19:41PM +0300, Michael S. Tsirkin wrote: > On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. It uses > > two virtqueues - one for (sync) read and another for (async) write. > > Since it cannot wait for write finished, it supports up to 128 > > concurrent IO. The buffer size is configurable now. > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Radim Krčmář <rkrcmar@redhat.com> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Anthony Liguori <aliguori@amazon.com> > > Cc: Anton Vorontsov <anton@enomsg.org> > > Cc: Colin Cross <ccross@android.com> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Tony Luck <tony.luck@intel.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Ingo Molnar <mingo@kernel.org> > > Cc: Minchan Kim <minchan@kernel.org> > > Cc: kvm@vger.kernel.org > > Cc: qemu-devel@nongnu.org > > Cc: virtualization@lists.linux-foundation.org > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > drivers/virtio/Kconfig | 10 + > > drivers/virtio/Makefile | 1 + > > drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++ > > include/uapi/linux/Kbuild | 1 + > > include/uapi/linux/virtio_ids.h | 1 + > > include/uapi/linux/virtio_pstore.h | 74 +++++++ > > 6 files changed, 504 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..0a63c7db4278 > > --- /dev/null > > +++ b/drivers/virtio/virtio_pstore.c > > @@ -0,0 +1,417 @@ > > +#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) > > +#define VIRT_PSTORE_NR_REQ 128 > > where are these numbers from? The buffer size was chosen to be larger than default kmsg_bytes (10240) in the pstore platform code and the request count is a arbitrary value. > > > > + > > +struct virtio_pstore { > > + struct virtio_device *vdev; > > + struct virtqueue *vq[2]; > > + struct pstore_info pstore; > > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ]; > > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ]; > > + unsigned int req_id; > > + > > + /* Waiting for host to ack */ > > + wait_queue_head_t acked; > > + int failed; > > +}; > > + > > +#define TYPE_TABLE_ENTRY(_entry) \ > > + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry } > > + > > +struct type_table { > > + int pstore; > > + u16 virtio; > > +} type_table[] = { > > + TYPE_TABLE_ENTRY(DMESG), > > +}; > > + > > +#undef TYPE_TABLE_ENTRY > > Let's not play preprocessor games until this becomes > a big issue. Simple > { PSTORE_TYPE_DMESG, VIRTIO_PSTORE_TYPE_DMESG} > does the trick just as well for now. > Also see below. > > > > > + > > + > > single empty line pls. Ok. > > > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > > + if (type == type_table[i].pstore) > > + return cpu_to_virtio16(vps->vdev, type_table[i].virtio); > > + } > > Rather complex for something that always returns a single value. > why do we need a table at all? > How about a switch statement? The pstore has 4 message types and I'd like to add a few more. This patch only implements the most popular dmesg type and others will be added later. But I'm ok with the switch too. > > static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) > { > switch (type) { > case PSTORE_TYPE_DMESG: > return VIRTIO_PSTORE_TYPE_DMESG; > default: > return VIRTIO_PSTORE_TYPE_UNKNOWN; > } > } > > > > + > > + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); > > +} > > This returns an incorrect type. Right. But it was fixed in v5 and you seem to review an earlier version unfortunately. > > > > + > > +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > > + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio) > > + return type_table[i].pstore; > > + } > > + > > + return PSTORE_TYPE_UNKNOWN; > > +} > > + > > +static void virtpstore_ack(struct virtqueue *vq) > > +{ > > + struct virtio_pstore *vps = vq->vdev->priv; > > + > > + wake_up(&vps->acked); > > +} > > + > > +static void virtpstore_check(struct virtqueue *vq) > > +{ > > + struct virtio_pstore *vps = vq->vdev->priv; > > + struct virtio_pstore_res *res; > > + unsigned int len; > > + > > + res = virtqueue_get_buf(vq, &len); > > + if (res == NULL) > > + return; > > + > > + if (virtio32_to_cpu(vq->vdev, res->ret) < 0) > > + vps->failed = 1; > > +} > > + > > +static void virt_pstore_get_reqs(struct virtio_pstore *vps, > > + struct virtio_pstore_req **preq, > > + struct virtio_pstore_res **pres) > > +{ > > + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ; > > + > > + *preq = &vps->req[idx]; > > + *pres = &vps->res[idx]; > > + > > + memset(*preq, 0, sizeof(**preq)); > > + memset(*pres, 0, sizeof(**pres)); > > +} > > + > > +static int virt_pstore_open(struct pstore_info *psi) > > +{ > > + struct virtio_pstore *vps = psi->data; > > + struct virtio_pstore_req *req; > > + struct virtio_pstore_res *res; > > + struct scatterlist sgo[1], sgi[1]; > > + struct scatterlist *sgs[2] = { sgo, sgi }; > > + unsigned int len; > > + > > + virt_pstore_get_reqs(vps, &req, &res); > > + > > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN); > > + > > + sg_init_one(sgo, req, sizeof(*req)); > > + sg_init_one(sgi, res, sizeof(*res)); > > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > > + virtqueue_kick(vps->vq[0]); > > + > > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > > + return virtio32_to_cpu(vps->vdev, res->ret); > > +} > > + > > +static int virt_pstore_close(struct pstore_info *psi) > > +{ > > + struct virtio_pstore *vps = psi->data; > > + struct virtio_pstore_req *req = &vps->req[vps->req_id]; > > + struct virtio_pstore_res *res = &vps->res[vps->req_id]; > > + struct scatterlist sgo[1], sgi[1]; > > + struct scatterlist *sgs[2] = { sgo, sgi }; > > + unsigned int len; > > + > > + virt_pstore_get_reqs(vps, &req, &res); > > + > > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_CLOSE); > > + > > + sg_init_one(sgo, req, sizeof(*req)); > > + sg_init_one(sgi, res, sizeof(*res)); > > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > > + virtqueue_kick(vps->vq[0]); > > + > > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > > + return virtio32_to_cpu(vps->vdev, res->ret); > > +} > > + > > +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type, > > + int *count, struct timespec *time, > > + char **buf, bool *compressed, > > + ssize_t *ecc_notice_size, > > + struct pstore_info *psi) > > +{ > > + struct virtio_pstore *vps = psi->data; > > + struct virtio_pstore_req *req; > > + struct virtio_pstore_res *res; > > + struct virtio_pstore_fileinfo info; > > + struct scatterlist sgo[1], sgi[3]; > > + struct scatterlist *sgs[2] = { sgo, sgi }; > > + unsigned int len; > > + unsigned int flags; > > + int ret; > > + void *bf; > > + > > + virt_pstore_get_reqs(vps, &req, &res); > > + > > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_READ); > > + > > + sg_init_one(sgo, req, sizeof(*req)); > > + sg_init_table(sgi, 3); > > + sg_set_buf(&sgi[0], res, sizeof(*res)); > > + sg_set_buf(&sgi[1], &info, sizeof(info)); > > + sg_set_buf(&sgi[2], psi->buf, psi->bufsize); > > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > > + virtqueue_kick(vps->vq[0]); > > + > > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > > + if (len < sizeof(*res) + sizeof(info)) > > + return -1; > > + > > + ret = virtio32_to_cpu(vps->vdev, res->ret); > > + if (ret < 0) > > + return ret; > > + > > + len = virtio32_to_cpu(vps->vdev, info.len); > > + > > + bf = kmalloc(len, GFP_KERNEL); > > + if (bf == NULL) > > + return -ENOMEM; > > + > > + *id = virtio64_to_cpu(vps->vdev, info.id); > > + *type = from_virtio_type(vps, info.type); > > + *count = virtio32_to_cpu(vps->vdev, info.count); > > + > > + flags = virtio32_to_cpu(vps->vdev, info.flags); > > + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED; > > + > > + time->tv_sec = virtio64_to_cpu(vps->vdev, info.time_sec); > > + time->tv_nsec = virtio32_to_cpu(vps->vdev, info.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_req *req; > > + struct virtio_pstore_res *res; > > + struct scatterlist sgo[2], sgi[1]; > > + struct scatterlist *sgs[2] = { sgo, sgi }; > > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0; > > + > > + if (vps->failed) > > + return -1; > > + > > + *id = vps->req_id; > > + virt_pstore_get_reqs(vps, &req, &res); > > + > > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE); > > + req->type = to_virtio_type(vps, type); > > + req->flags = cpu_to_virtio32(vps->vdev, flags); > > + > > + sg_init_table(sgo, 2); > > + sg_set_buf(&sgo[0], req, sizeof(*req)); > > + sg_set_buf(&sgo[1], pstore_get_buf(psi), size); > > + sg_init_one(sgi, res, sizeof(*res)); > > + virtqueue_add_sgs(vps->vq[1], sgs, 1, 1, vps, GFP_ATOMIC); > > + virtqueue_kick(vps->vq[1]); > > + > > + 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_req *req; > > + struct virtio_pstore_res *res; > > + struct scatterlist sgo[1], sgi[1]; > > + struct scatterlist *sgs[2] = { sgo, sgi }; > > + unsigned int len; > > + > > + virt_pstore_get_reqs(vps, &req, &res); > > + > > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE); > > + req->type = to_virtio_type(vps, type); > > + req->id = cpu_to_virtio64(vps->vdev, id); > > + req->count = cpu_to_virtio32(vps->vdev, count); > > + > > + sg_init_one(sgo, req, sizeof(*req)); > > + sg_init_one(sgi, res, sizeof(*res)); > > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > > + virtqueue_kick(vps->vq[0]); > > + > > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > > + return virtio32_to_cpu(vps->vdev, res->ret); > > +} > > + > > +static int virt_pstore_init(struct virtio_pstore *vps) > > +{ > > + struct pstore_info *psinfo = &vps->pstore; > > + int err; > > + > > + if (!psinfo->bufsize) > > + psinfo->bufsize = VIRT_PSTORE_BUFSIZE; > > + > > + psinfo->buf = alloc_pages_exact(psinfo->bufsize, GFP_KERNEL); > > + 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_DMESG; > > + > > + 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); > > I don't know enough about pstore - does this > actually ensure that > 1. all existing users close the device > 2. no new users can open it > somehow? The pstore driver doesn't create a device node (except the pmsg device which this patch doesn't deal with) so it doesn't need to worry about the user AFAIK. It just calls the pstore callbacks (if any) on some system events (e.g. kmsg dump, console write and so on). In fact, pstore is a pseudo file system, works as read-only mode. > > > + > > + free_pages_exact(psinfo->buf, psinfo->bufsize); > > + psinfo->buf = NULL; > > + psinfo->bufsize = 0; > > + > > + return 0; > > +} > > + > > +static int virtpstore_init_vqs(struct virtio_pstore *vps) > > +{ > > + vq_callback_t *callbacks[] = { virtpstore_ack, virtpstore_check }; > > + const char *names[] = { "pstore_read", "pstore_write" }; > > + > > + return vps->vdev->config->find_vqs(vps->vdev, 2, vps->vq, > > + callbacks, names); > > +} > > + > > +static void virtpstore_init_config(struct virtio_pstore *vps) > > +{ > > + u32 bufsize; > > + > > + virtio_cread(vps->vdev, struct virtio_pstore_config, bufsize, &bufsize); > > + > > + vps->pstore.bufsize = PAGE_ALIGN(bufsize); > > +} > > + > > +static void virtpstore_confirm_config(struct virtio_pstore *vps) > > +{ > > + u32 bufsize = vps->pstore.bufsize; > > + > > + virtio_cwrite(vps->vdev, struct virtio_pstore_config, bufsize, > > + &bufsize); > > +} > > + > > +static int virtpstore_probe(struct virtio_device *vdev) > > +{ > > + struct virtio_pstore *vps; > > + int err; > > + > > + if (!vdev->config->get) { > > + dev_err(&vdev->dev, "driver init: config access disabled\n"); > > + return -EINVAL; > > + } > > + > > + vdev->priv = vps = kzalloc(sizeof(*vps), GFP_KERNEL); > > + if (!vps) { > > + err = -ENOMEM; > > + goto out; > > + } > > + vps->vdev = vdev; > > + > > + err = virtpstore_init_vqs(vps); > > + if (err < 0) > > + goto out_free; > > + > > + virtpstore_init_config(vps); > > + > > + err = virt_pstore_init(vps); > > + if (err) > > + goto out_del_vq; > > + > > + virtpstore_confirm_config(vps); > > + > > + init_waitqueue_head(&vps->acked); > > + > > + virtio_device_ready(vdev); > > + > > + dev_info(&vdev->dev, "driver init: ok (bufsize = %luK, flags = %x)\n", > > + vps->pstore.bufsize >> 10, vps->pstore.flags); > > + > > + return 0; > > + > > +out_del_vq: > > + vdev->config->del_vqs(vdev); > > +out_free: > > + kfree(vps); > > +out: > > + dev_err(&vdev->dev, "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 = { > > We need some way to avoid trying to load this > as a legacy device. There isn't a way to do it yet > so I won't block your patch on this but pls try to > come up with something, and I'll do, too. I have no idea what I can do. Also it seems the kvmtools supports only legacy devices (please correct me I was wrong). > > > > + .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, > > Won't this need freeze/restore callbacks? Probably.. :) Will add. > > > +}; > > + > > +module_virtio_driver(virtio_pstore_driver); > > +MODULE_DEVICE_TABLE(virtio, id_table); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Namhyung Kim <namhyung@kernel.org>"); > > +MODULE_DESCRIPTION("Virtio pstore driver"); > > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild > > index 6d4e92ccdc91..9bbb1554d8b2 100644 > > --- a/include/uapi/linux/Kbuild > > +++ b/include/uapi/linux/Kbuild > > @@ -449,6 +449,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..c72a9ab588c0 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 22 /* 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..f4b0d204d8ae > > --- /dev/null > > +++ b/include/uapi/linux/virtio_pstore.h > > @@ -0,0 +1,74 @@ > > +#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 > > Most other headers use _F_ and not _FL_ > Also, we specify bit number and not the > bitmask. So: > > #define VIRTIO_PSTORE_F_COMPRESSED 0 > > and > > (0x1 << VIRTIO_PSTORE_F_COMPRESSED) Ok. > > > > + > > +struct virtio_pstore_req { > > + __virtio16 cmd; > > + __virtio16 type; > > + __virtio32 flags; > > + __virtio64 id; > > + __virtio32 count; > > + __virtio32 reserved; > > +}; > > + > > +struct virtio_pstore_res { > > + __virtio16 cmd; > > + __virtio16 type; > > + __virtio32 ret; > > +}; > > + > > +struct virtio_pstore_fileinfo { > > + __virtio64 id; > > + __virtio32 count; > > + __virtio16 type; > > + __virtio16 unused; > > + __virtio32 flags; > > + __virtio32 len; > > + __virtio64 time_sec; > > + __virtio32 time_nsec; > > + __virtio32 reserved; > > Any reason one is reserved the other is unused? > If not just calls them pad1, pad2? No specific reason, will change. > > > +}; > > + > > +struct virtio_pstore_config { > > + __virtio32 bufsize; > > +}; > > + > > __virtio things are for compatibility things. > New devices should just use __le everywhere. Right. Again, already fixed in v5. :) > > Let me post a patch that adds config space accessors > so you can do this. Please CC me on the patch. Thanks, Namhyung > > > > +#endif /* _LINUX_VIRTIO_PSTORE_H */ > > -- > > 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. Do you mean a legacy virtio device? I don't see why you would want to support pre-1.0 mode. If you drop that, you can drop all cpu_to_virtio things and just use __le accessors. > It uses > two virtqueues - one for (sync) read and another for (async) write. > Since it cannot wait for write finished, it supports up to 128 > concurrent IO. The buffer size is configurable now. > > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Anthony Liguori <aliguori@amazon.com> > Cc: Anton Vorontsov <anton@enomsg.org> > Cc: Colin Cross <ccross@android.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Tony Luck <tony.luck@intel.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Minchan Kim <minchan@kernel.org> > Cc: kvm@vger.kernel.org > Cc: qemu-devel@nongnu.org > Cc: virtualization@lists.linux-foundation.org > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > drivers/virtio/Kconfig | 10 + > drivers/virtio/Makefile | 1 + > drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++ > include/uapi/linux/Kbuild | 1 + > include/uapi/linux/virtio_ids.h | 1 + > include/uapi/linux/virtio_pstore.h | 74 +++++++ > 6 files changed, 504 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..0a63c7db4278 > --- /dev/null > +++ b/drivers/virtio/virtio_pstore.c > @@ -0,0 +1,417 @@ > +#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) > +#define VIRT_PSTORE_NR_REQ 128 > + > +struct virtio_pstore { > + struct virtio_device *vdev; > + struct virtqueue *vq[2]; I'd add named fields instead of an array here, vq[0] vq[1] all over the place is hard to read. > + struct pstore_info pstore; > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ]; > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ]; > + unsigned int req_id; > + > + /* Waiting for host to ack */ > + wait_queue_head_t acked; > + int failed; > +}; > + > +#define TYPE_TABLE_ENTRY(_entry) \ > + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry } > + > +struct type_table { > + int pstore; > + u16 virtio; > +} type_table[] = { > + TYPE_TABLE_ENTRY(DMESG), > +}; > + > +#undef TYPE_TABLE_ENTRY let's avoid macros for now pls. In fact, I would just open-code this in to_virtio_type below. We can always change our minds later if lots of types are added. > + > + single emoty line pls > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > + if (type == type_table[i].pstore) > + return cpu_to_virtio16(vps->vdev, type_table[i].virtio); > + } > + > + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); This assigns u16 to __virtio type, sparse will warn if you enable endian-ness checks. Pls fix that and generally, please make sure this is clean from sparse warnings. > +} > + > +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio) > + return type_table[i].pstore; > + } > + > + return PSTORE_TYPE_UNKNOWN; > +} > + > +static void virtpstore_ack(struct virtqueue *vq) > +{ > + struct virtio_pstore *vps = vq->vdev->priv; > + > + wake_up(&vps->acked); > +} > + > +static void virtpstore_check(struct virtqueue *vq) > +{ > + struct virtio_pstore *vps = vq->vdev->priv; > + struct virtio_pstore_res *res; > + unsigned int len; > + > + res = virtqueue_get_buf(vq, &len); > + if (res == NULL) > + return; > + > + if (virtio32_to_cpu(vq->vdev, res->ret) < 0) > + vps->failed = 1; > +} > + > +static void virt_pstore_get_reqs(struct virtio_pstore *vps, > + struct virtio_pstore_req **preq, > + struct virtio_pstore_res **pres) > +{ > + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ; > + > + *preq = &vps->req[idx]; > + *pres = &vps->res[idx]; > + > + memset(*preq, 0, sizeof(**preq)); > + memset(*pres, 0, sizeof(**pres)); > +} > + > +static int virt_pstore_open(struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_req *req; > + struct virtio_pstore_res *res; > + struct scatterlist sgo[1], sgi[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN); > + > + sg_init_one(sgo, req, sizeof(*req)); > + sg_init_one(sgi, res, sizeof(*res)); > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq[0]); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); Does this block userspace in an uninterruptible wait if hardware is slow? That's not nice. > + return virtio32_to_cpu(vps->vdev, res->ret); > +} > + > +static int virt_pstore_close(struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_req *req = &vps->req[vps->req_id]; > + struct virtio_pstore_res *res = &vps->res[vps->req_id]; > + struct scatterlist sgo[1], sgi[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_CLOSE); > + > + sg_init_one(sgo, req, sizeof(*req)); > + sg_init_one(sgi, res, sizeof(*res)); > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq[0]); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > + return virtio32_to_cpu(vps->vdev, res->ret); > +} > + > +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type, > + int *count, struct timespec *time, > + char **buf, bool *compressed, > + ssize_t *ecc_notice_size, > + struct pstore_info *psi) > +{ > + struct virtio_pstore *vps = psi->data; > + struct virtio_pstore_req *req; > + struct virtio_pstore_res *res; > + struct virtio_pstore_fileinfo info; > + struct scatterlist sgo[1], sgi[3]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + unsigned int flags; > + int ret; > + void *bf; > + > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_READ); > + > + sg_init_one(sgo, req, sizeof(*req)); > + sg_init_table(sgi, 3); > + sg_set_buf(&sgi[0], res, sizeof(*res)); > + sg_set_buf(&sgi[1], &info, sizeof(info)); > + sg_set_buf(&sgi[2], psi->buf, psi->bufsize); > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq[0]); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > + if (len < sizeof(*res) + sizeof(info)) > + return -1; > + > + ret = virtio32_to_cpu(vps->vdev, res->ret); > + if (ret < 0) > + return ret; > + > + len = virtio32_to_cpu(vps->vdev, info.len); > + > + bf = kmalloc(len, GFP_KERNEL); > + if (bf == NULL) > + return -ENOMEM; > + > + *id = virtio64_to_cpu(vps->vdev, info.id); > + *type = from_virtio_type(vps, info.type); > + *count = virtio32_to_cpu(vps->vdev, info.count); > + > + flags = virtio32_to_cpu(vps->vdev, info.flags); > + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED; > + > + time->tv_sec = virtio64_to_cpu(vps->vdev, info.time_sec); > + time->tv_nsec = virtio32_to_cpu(vps->vdev, info.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_req *req; > + struct virtio_pstore_res *res; > + struct scatterlist sgo[2], sgi[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0; > + > + if (vps->failed) > + return -1; > + > + *id = vps->req_id; > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE); > + req->type = to_virtio_type(vps, type); > + req->flags = cpu_to_virtio32(vps->vdev, flags); > + > + sg_init_table(sgo, 2); > + sg_set_buf(&sgo[0], req, sizeof(*req)); > + sg_set_buf(&sgo[1], pstore_get_buf(psi), size); > + sg_init_one(sgi, res, sizeof(*res)); > + virtqueue_add_sgs(vps->vq[1], sgs, 1, 1, vps, GFP_ATOMIC); > + virtqueue_kick(vps->vq[1]); > + > + 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_req *req; > + struct virtio_pstore_res *res; > + struct scatterlist sgo[1], sgi[1]; > + struct scatterlist *sgs[2] = { sgo, sgi }; > + unsigned int len; > + > + virt_pstore_get_reqs(vps, &req, &res); > + > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE); > + req->type = to_virtio_type(vps, type); > + req->id = cpu_to_virtio64(vps->vdev, id); > + req->count = cpu_to_virtio32(vps->vdev, count); > + > + sg_init_one(sgo, req, sizeof(*req)); > + sg_init_one(sgi, res, sizeof(*res)); > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > + virtqueue_kick(vps->vq[0]); > + > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > + return virtio32_to_cpu(vps->vdev, res->ret); > +} > + > +static int virt_pstore_init(struct virtio_pstore *vps) > +{ > + struct pstore_info *psinfo = &vps->pstore; > + int err; > + > + if (!psinfo->bufsize) > + psinfo->bufsize = VIRT_PSTORE_BUFSIZE; > + > + psinfo->buf = alloc_pages_exact(psinfo->bufsize, GFP_KERNEL); > + 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_DMESG; > + > + 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_exact(psinfo->buf, psinfo->bufsize); > + psinfo->buf = NULL; > + psinfo->bufsize = 0; > + > + return 0; > +} > + > +static int virtpstore_init_vqs(struct virtio_pstore *vps) > +{ > + vq_callback_t *callbacks[] = { virtpstore_ack, virtpstore_check }; > + const char *names[] = { "pstore_read", "pstore_write" }; > + > + return vps->vdev->config->find_vqs(vps->vdev, 2, vps->vq, > + callbacks, names); > +} > + > +static void virtpstore_init_config(struct virtio_pstore *vps) > +{ > + u32 bufsize; > + > + virtio_cread(vps->vdev, struct virtio_pstore_config, bufsize, &bufsize); > + > + vps->pstore.bufsize = PAGE_ALIGN(bufsize); > +} > + > +static void virtpstore_confirm_config(struct virtio_pstore *vps) > +{ > + u32 bufsize = vps->pstore.bufsize; > + > + virtio_cwrite(vps->vdev, struct virtio_pstore_config, bufsize, > + &bufsize); > +} > + > +static int virtpstore_probe(struct virtio_device *vdev) > +{ > + struct virtio_pstore *vps; > + int err; > + > + if (!vdev->config->get) { > + dev_err(&vdev->dev, "driver init: config access disabled\n"); > + return -EINVAL; > + } > + > + vdev->priv = vps = kzalloc(sizeof(*vps), GFP_KERNEL); > + if (!vps) { > + err = -ENOMEM; > + goto out; > + } > + vps->vdev = vdev; > + > + err = virtpstore_init_vqs(vps); > + if (err < 0) > + goto out_free; > + > + virtpstore_init_config(vps); > + > + err = virt_pstore_init(vps); > + if (err) > + goto out_del_vq; > + > + virtpstore_confirm_config(vps); > + > + init_waitqueue_head(&vps->acked); > + > + virtio_device_ready(vdev); > + > + dev_info(&vdev->dev, "driver init: ok (bufsize = %luK, flags = %x)\n", > + vps->pstore.bufsize >> 10, vps->pstore.flags); > + > + return 0; > + > +out_del_vq: > + vdev->config->del_vqs(vdev); > +out_free: > + kfree(vps); > +out: > + dev_err(&vdev->dev, "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@kernel.org>"); > +MODULE_DESCRIPTION("Virtio pstore driver"); > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild > index 6d4e92ccdc91..9bbb1554d8b2 100644 > --- a/include/uapi/linux/Kbuild > +++ b/include/uapi/linux/Kbuild > @@ -449,6 +449,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..c72a9ab588c0 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 22 /* 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..f4b0d204d8ae > --- /dev/null > +++ b/include/uapi/linux/virtio_pstore.h > @@ -0,0 +1,74 @@ > +#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_req { > + __virtio16 cmd; > + __virtio16 type; > + __virtio32 flags; > + __virtio64 id; > + __virtio32 count; > + __virtio32 reserved; > +}; > + > +struct virtio_pstore_res { > + __virtio16 cmd; > + __virtio16 type; > + __virtio32 ret; > +}; > + > +struct virtio_pstore_fileinfo { > + __virtio64 id; > + __virtio32 count; > + __virtio16 type; > + __virtio16 unused; > + __virtio32 flags; > + __virtio32 len; > + __virtio64 time_sec; > + __virtio32 time_nsec; > + __virtio32 reserved; > +}; > + > +struct virtio_pstore_config { > + __virtio32 bufsize; > +}; > + What exactly does each field mean? I'm especially interested in time fields - maintaining a consistent time between host and guest is not a simple problem. > +#endif /* _LINUX_VIRTIO_PSTORE_H */ > -- > 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Michael, On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote: > On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. > > Do you mean a legacy virtio device? I don't see why > you would want to support pre-1.0 mode. > If you drop that, you can drop all cpu_to_virtio things > and just use __le accessors. I was thinking about the kvmtools which lacks 1.0 support AFAIK. But I think it'd be better to always use __le type anyway. Will change. > > > It uses > > two virtqueues - one for (sync) read and another for (async) write. > > Since it cannot wait for write finished, it supports up to 128 > > concurrent IO. The buffer size is configurable now. > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Radim Krčmář <rkrcmar@redhat.com> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Anthony Liguori <aliguori@amazon.com> > > Cc: Anton Vorontsov <anton@enomsg.org> > > Cc: Colin Cross <ccross@android.com> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Tony Luck <tony.luck@intel.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Ingo Molnar <mingo@kernel.org> > > Cc: Minchan Kim <minchan@kernel.org> > > Cc: kvm@vger.kernel.org > > Cc: qemu-devel@nongnu.org > > Cc: virtualization@lists.linux-foundation.org > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > drivers/virtio/Kconfig | 10 + > > drivers/virtio/Makefile | 1 + > > drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++ > > include/uapi/linux/Kbuild | 1 + > > include/uapi/linux/virtio_ids.h | 1 + > > include/uapi/linux/virtio_pstore.h | 74 +++++++ > > 6 files changed, 504 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..0a63c7db4278 > > --- /dev/null > > +++ b/drivers/virtio/virtio_pstore.c > > @@ -0,0 +1,417 @@ > > +#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) > > +#define VIRT_PSTORE_NR_REQ 128 > > + > > +struct virtio_pstore { > > + struct virtio_device *vdev; > > + struct virtqueue *vq[2]; > > I'd add named fields instead of an array here, vq[0] > vq[1] all over the place is hard to read. Will change. > > > + struct pstore_info pstore; > > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ]; > > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ]; > > + unsigned int req_id; > > + > > + /* Waiting for host to ack */ > > + wait_queue_head_t acked; > > + int failed; > > +}; > > + > > +#define TYPE_TABLE_ENTRY(_entry) \ > > + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry } > > + > > +struct type_table { > > + int pstore; > > + u16 virtio; > > +} type_table[] = { > > + TYPE_TABLE_ENTRY(DMESG), > > +}; > > + > > +#undef TYPE_TABLE_ENTRY > > let's avoid macros for now pls. In fact, I would just open-code this > in to_virtio_type below. We can always change our minds later if > lots of types are added. Yep. > > > + > > + > > single emoty line pls Ok. > > > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > > + if (type == type_table[i].pstore) > > + return cpu_to_virtio16(vps->vdev, type_table[i].virtio); > > + } > > + > > + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); > > This assigns u16 to __virtio type, sparse will warn > if you enable endian-ness checks. > Pls fix that and generally, please make sure this is > clean from sparse warnings. I'll run sparse before sending patch next time. > > > +} > > + > > +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type) > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > > + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio) > > + return type_table[i].pstore; > > + } > > + > > + return PSTORE_TYPE_UNKNOWN; > > +} > > + > > +static void virtpstore_ack(struct virtqueue *vq) > > +{ > > + struct virtio_pstore *vps = vq->vdev->priv; > > + > > + wake_up(&vps->acked); > > +} > > + > > +static void virtpstore_check(struct virtqueue *vq) > > +{ > > + struct virtio_pstore *vps = vq->vdev->priv; > > + struct virtio_pstore_res *res; > > + unsigned int len; > > + > > + res = virtqueue_get_buf(vq, &len); > > + if (res == NULL) > > + return; > > + > > + if (virtio32_to_cpu(vq->vdev, res->ret) < 0) > > + vps->failed = 1; > > +} > > + > > +static void virt_pstore_get_reqs(struct virtio_pstore *vps, > > + struct virtio_pstore_req **preq, > > + struct virtio_pstore_res **pres) > > +{ > > + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ; > > + > > + *preq = &vps->req[idx]; > > + *pres = &vps->res[idx]; > > + > > + memset(*preq, 0, sizeof(**preq)); > > + memset(*pres, 0, sizeof(**pres)); > > +} > > + > > +static int virt_pstore_open(struct pstore_info *psi) > > +{ > > + struct virtio_pstore *vps = psi->data; > > + struct virtio_pstore_req *req; > > + struct virtio_pstore_res *res; > > + struct scatterlist sgo[1], sgi[1]; > > + struct scatterlist *sgs[2] = { sgo, sgi }; > > + unsigned int len; > > + > > + virt_pstore_get_reqs(vps, &req, &res); > > + > > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN); > > + > > + sg_init_one(sgo, req, sizeof(*req)); > > + sg_init_one(sgi, res, sizeof(*res)); > > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > > + virtqueue_kick(vps->vq[0]); > > + > > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > > Does this block userspace in an uninterruptible wait if > hardware is slow? That's not nice. Yes, but it's not a common operation and I just wanted to make it simple. > > > + return virtio32_to_cpu(vps->vdev, res->ret); > > +} > > + [SNIP] > > +struct virtio_pstore_fileinfo { > > + __virtio64 id; > > + __virtio32 count; > > + __virtio16 type; > > + __virtio16 unused; > > + __virtio32 flags; > > + __virtio32 len; > > + __virtio64 time_sec; > > + __virtio32 time_nsec; > > + __virtio32 reserved; > > +}; > > + > > +struct virtio_pstore_config { > > + __virtio32 bufsize; > > +}; > > + > > What exactly does each field mean? I'm especially > interested in time fields - maintaining a consistent > time between host and guest is not a simple problem. These are required by pstore and will be used to create corresponding files in the pstore filesystem. The time fields are for mtime and ctime and, I think, it's just a hint for user and doesn't require strict consistency. Thanks for your review! Namhyung > > > +#endif /* _LINUX_VIRTIO_PSTORE_H */ > > -- > > 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote: > Hi Michael, > > On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote: > > On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. > > > > Do you mean a legacy virtio device? I don't see why > > you would want to support pre-1.0 mode. > > If you drop that, you can drop all cpu_to_virtio things > > and just use __le accessors. > > I was thinking about the kvmtools which lacks 1.0 support AFAIK. Unless kvmtools wants to be left behind it has to go 1.0. > But > I think it'd be better to always use __le type anyway. Will change. > > > > > > > It uses > > > two virtqueues - one for (sync) read and another for (async) write. > > > Since it cannot wait for write finished, it supports up to 128 > > > concurrent IO. The buffer size is configurable now. > > > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Cc: Radim Krčmář <rkrcmar@redhat.com> > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > > Cc: Anthony Liguori <aliguori@amazon.com> > > > Cc: Anton Vorontsov <anton@enomsg.org> > > > Cc: Colin Cross <ccross@android.com> > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: Tony Luck <tony.luck@intel.com> > > > Cc: Steven Rostedt <rostedt@goodmis.org> > > > Cc: Ingo Molnar <mingo@kernel.org> > > > Cc: Minchan Kim <minchan@kernel.org> > > > Cc: kvm@vger.kernel.org > > > Cc: qemu-devel@nongnu.org > > > Cc: virtualization@lists.linux-foundation.org > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > > --- > > > drivers/virtio/Kconfig | 10 + > > > drivers/virtio/Makefile | 1 + > > > drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++ > > > include/uapi/linux/Kbuild | 1 + > > > include/uapi/linux/virtio_ids.h | 1 + > > > include/uapi/linux/virtio_pstore.h | 74 +++++++ > > > 6 files changed, 504 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..0a63c7db4278 > > > --- /dev/null > > > +++ b/drivers/virtio/virtio_pstore.c > > > @@ -0,0 +1,417 @@ > > > +#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) > > > +#define VIRT_PSTORE_NR_REQ 128 > > > + > > > +struct virtio_pstore { > > > + struct virtio_device *vdev; > > > + struct virtqueue *vq[2]; > > > > I'd add named fields instead of an array here, vq[0] > > vq[1] all over the place is hard to read. > > Will change. > > > > > > + struct pstore_info pstore; > > > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ]; > > > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ]; > > > + unsigned int req_id; > > > + > > > + /* Waiting for host to ack */ > > > + wait_queue_head_t acked; > > > + int failed; > > > +}; > > > + > > > +#define TYPE_TABLE_ENTRY(_entry) \ > > > + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry } > > > + > > > +struct type_table { > > > + int pstore; > > > + u16 virtio; > > > +} type_table[] = { > > > + TYPE_TABLE_ENTRY(DMESG), > > > +}; > > > + > > > +#undef TYPE_TABLE_ENTRY > > > > let's avoid macros for now pls. In fact, I would just open-code this > > in to_virtio_type below. We can always change our minds later if > > lots of types are added. > > Yep. > > > > > > + > > > + > > > > single emoty line pls > > Ok. > > > > > > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) > > > +{ > > > + unsigned int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > > > + if (type == type_table[i].pstore) > > > + return cpu_to_virtio16(vps->vdev, type_table[i].virtio); > > > + } > > > + > > > + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); > > > > This assigns u16 to __virtio type, sparse will warn > > if you enable endian-ness checks. > > Pls fix that and generally, please make sure this is > > clean from sparse warnings. > > I'll run sparse before sending patch next time. > > > > > > +} > > > + > > > +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type) > > > +{ > > > + unsigned int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(type_table); i++) { > > > + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio) > > > + return type_table[i].pstore; > > > + } > > > + > > > + return PSTORE_TYPE_UNKNOWN; > > > +} > > > + > > > +static void virtpstore_ack(struct virtqueue *vq) > > > +{ > > > + struct virtio_pstore *vps = vq->vdev->priv; > > > + > > > + wake_up(&vps->acked); > > > +} > > > + > > > +static void virtpstore_check(struct virtqueue *vq) > > > +{ > > > + struct virtio_pstore *vps = vq->vdev->priv; > > > + struct virtio_pstore_res *res; > > > + unsigned int len; > > > + > > > + res = virtqueue_get_buf(vq, &len); > > > + if (res == NULL) > > > + return; > > > + > > > + if (virtio32_to_cpu(vq->vdev, res->ret) < 0) > > > + vps->failed = 1; > > > +} > > > + > > > +static void virt_pstore_get_reqs(struct virtio_pstore *vps, > > > + struct virtio_pstore_req **preq, > > > + struct virtio_pstore_res **pres) > > > +{ > > > + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ; > > > + > > > + *preq = &vps->req[idx]; > > > + *pres = &vps->res[idx]; > > > + > > > + memset(*preq, 0, sizeof(**preq)); > > > + memset(*pres, 0, sizeof(**pres)); > > > +} > > > + > > > +static int virt_pstore_open(struct pstore_info *psi) > > > +{ > > > + struct virtio_pstore *vps = psi->data; > > > + struct virtio_pstore_req *req; > > > + struct virtio_pstore_res *res; > > > + struct scatterlist sgo[1], sgi[1]; > > > + struct scatterlist *sgs[2] = { sgo, sgi }; > > > + unsigned int len; > > > + > > > + virt_pstore_get_reqs(vps, &req, &res); > > > + > > > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN); > > > + > > > + sg_init_one(sgo, req, sizeof(*req)); > > > + sg_init_one(sgi, res, sizeof(*res)); > > > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); > > > + virtqueue_kick(vps->vq[0]); > > > + > > > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); > > > > Does this block userspace in an uninterruptible wait if > > hardware is slow? That's not nice. > > Yes, but it's not a common operation and I just wanted to make it > simple. > > > > > > > + return virtio32_to_cpu(vps->vdev, res->ret); > > > +} > > > + > > [SNIP] > > > +struct virtio_pstore_fileinfo { > > > + __virtio64 id; > > > + __virtio32 count; > > > + __virtio16 type; > > > + __virtio16 unused; > > > + __virtio32 flags; > > > + __virtio32 len; > > > + __virtio64 time_sec; > > > + __virtio32 time_nsec; > > > + __virtio32 reserved; > > > +}; > > > + > > > +struct virtio_pstore_config { > > > + __virtio32 bufsize; > > > +}; > > > + > > > > What exactly does each field mean? I'm especially > > interested in time fields - maintaining a consistent > > time between host and guest is not a simple problem. > > These are required by pstore and will be used to create corresponding > files in the pstore filesystem. The time fields are for mtime and > ctime and, I think, it's just a hint for user and doesn't require > strict consistency. Pls add documentation. I would just drop hints for now. > > Thanks for your review! > Namhyung > > > > > > +#endif /* _LINUX_VIRTIO_PSTORE_H */ > > > -- > > > 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 15, 2016 at 07:06:28AM +0200, Michael S. Tsirkin wrote: > On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote: > > On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote: > > [SNIP] > > > > +struct virtio_pstore_fileinfo { > > > > + __virtio64 id; > > > > + __virtio32 count; > > > > + __virtio16 type; > > > > + __virtio16 unused; > > > > + __virtio32 flags; > > > > + __virtio32 len; > > > > + __virtio64 time_sec; > > > > + __virtio32 time_nsec; > > > > + __virtio32 reserved; > > > > +}; > > > > + > > > > +struct virtio_pstore_config { > > > > + __virtio32 bufsize; > > > > +}; > > > > + > > > > > > What exactly does each field mean? I'm especially > > > interested in time fields - maintaining a consistent > > > time between host and guest is not a simple problem. > > > > These are required by pstore and will be used to create corresponding > > files in the pstore filesystem. The time fields are for mtime and > > ctime and, I think, it's just a hint for user and doesn't require > > strict consistency. > > Pls add documentation. I would just drop hints for now. Well, I'll add docmentation. But I think just dropping might not good since they all have host time and it's helpful to know their relative difference in guest. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/11/2016 06:06, Michael S. Tsirkin wrote: > On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote: >> Hi Michael, >> >> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote: >>> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. >>> >>> Do you mean a legacy virtio device? I don't see why >>> you would want to support pre-1.0 mode. >>> If you drop that, you can drop all cpu_to_virtio things >>> and just use __le accessors. >> >> I was thinking about the kvmtools which lacks 1.0 support AFAIK. > > Unless kvmtools wants to be left behind it has to go 1.0. And it also has to go ACPI. Is there any reason, apart from kvmtool, to make a completely new virtio device, with no support in existing guests, rather than implement ACPI ERST? Paolo >> But >> I think it'd be better to always use __le type anyway. Will change. >> >> >>> >>>> It uses >>>> two virtqueues - one for (sync) read and another for (async) write. >>>> Since it cannot wait for write finished, it supports up to 128 >>>> concurrent IO. The buffer size is configurable now. >>>> >>>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>>> Cc: Radim Krčmář <rkrcmar@redhat.com> >>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>>> Cc: Anthony Liguori <aliguori@amazon.com> >>>> Cc: Anton Vorontsov <anton@enomsg.org> >>>> Cc: Colin Cross <ccross@android.com> >>>> Cc: Kees Cook <keescook@chromium.org> >>>> Cc: Tony Luck <tony.luck@intel.com> >>>> Cc: Steven Rostedt <rostedt@goodmis.org> >>>> Cc: Ingo Molnar <mingo@kernel.org> >>>> Cc: Minchan Kim <minchan@kernel.org> >>>> Cc: kvm@vger.kernel.org >>>> Cc: qemu-devel@nongnu.org >>>> Cc: virtualization@lists.linux-foundation.org >>>> Signed-off-by: Namhyung Kim <namhyung@kernel.org> >>>> --- >>>> drivers/virtio/Kconfig | 10 + >>>> drivers/virtio/Makefile | 1 + >>>> drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++ >>>> include/uapi/linux/Kbuild | 1 + >>>> include/uapi/linux/virtio_ids.h | 1 + >>>> include/uapi/linux/virtio_pstore.h | 74 +++++++ >>>> 6 files changed, 504 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..0a63c7db4278 >>>> --- /dev/null >>>> +++ b/drivers/virtio/virtio_pstore.c >>>> @@ -0,0 +1,417 @@ >>>> +#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) >>>> +#define VIRT_PSTORE_NR_REQ 128 >>>> + >>>> +struct virtio_pstore { >>>> + struct virtio_device *vdev; >>>> + struct virtqueue *vq[2]; >>> >>> I'd add named fields instead of an array here, vq[0] >>> vq[1] all over the place is hard to read. >> >> Will change. >> >>> >>>> + struct pstore_info pstore; >>>> + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ]; >>>> + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ]; >>>> + unsigned int req_id; >>>> + >>>> + /* Waiting for host to ack */ >>>> + wait_queue_head_t acked; >>>> + int failed; >>>> +}; >>>> + >>>> +#define TYPE_TABLE_ENTRY(_entry) \ >>>> + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry } >>>> + >>>> +struct type_table { >>>> + int pstore; >>>> + u16 virtio; >>>> +} type_table[] = { >>>> + TYPE_TABLE_ENTRY(DMESG), >>>> +}; >>>> + >>>> +#undef TYPE_TABLE_ENTRY >>> >>> let's avoid macros for now pls. In fact, I would just open-code this >>> in to_virtio_type below. We can always change our minds later if >>> lots of types are added. >> >> Yep. >> >>> >>>> + >>>> + >>> >>> single emoty line pls >> >> Ok. >> >>> >>>> +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) >>>> +{ >>>> + unsigned int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(type_table); i++) { >>>> + if (type == type_table[i].pstore) >>>> + return cpu_to_virtio16(vps->vdev, type_table[i].virtio); >>>> + } >>>> + >>>> + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); >>> >>> This assigns u16 to __virtio type, sparse will warn >>> if you enable endian-ness checks. >>> Pls fix that and generally, please make sure this is >>> clean from sparse warnings. >> >> I'll run sparse before sending patch next time. >> >>> >>>> +} >>>> + >>>> +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type) >>>> +{ >>>> + unsigned int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(type_table); i++) { >>>> + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio) >>>> + return type_table[i].pstore; >>>> + } >>>> + >>>> + return PSTORE_TYPE_UNKNOWN; >>>> +} >>>> + >>>> +static void virtpstore_ack(struct virtqueue *vq) >>>> +{ >>>> + struct virtio_pstore *vps = vq->vdev->priv; >>>> + >>>> + wake_up(&vps->acked); >>>> +} >>>> + >>>> +static void virtpstore_check(struct virtqueue *vq) >>>> +{ >>>> + struct virtio_pstore *vps = vq->vdev->priv; >>>> + struct virtio_pstore_res *res; >>>> + unsigned int len; >>>> + >>>> + res = virtqueue_get_buf(vq, &len); >>>> + if (res == NULL) >>>> + return; >>>> + >>>> + if (virtio32_to_cpu(vq->vdev, res->ret) < 0) >>>> + vps->failed = 1; >>>> +} >>>> + >>>> +static void virt_pstore_get_reqs(struct virtio_pstore *vps, >>>> + struct virtio_pstore_req **preq, >>>> + struct virtio_pstore_res **pres) >>>> +{ >>>> + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ; >>>> + >>>> + *preq = &vps->req[idx]; >>>> + *pres = &vps->res[idx]; >>>> + >>>> + memset(*preq, 0, sizeof(**preq)); >>>> + memset(*pres, 0, sizeof(**pres)); >>>> +} >>>> + >>>> +static int virt_pstore_open(struct pstore_info *psi) >>>> +{ >>>> + struct virtio_pstore *vps = psi->data; >>>> + struct virtio_pstore_req *req; >>>> + struct virtio_pstore_res *res; >>>> + struct scatterlist sgo[1], sgi[1]; >>>> + struct scatterlist *sgs[2] = { sgo, sgi }; >>>> + unsigned int len; >>>> + >>>> + virt_pstore_get_reqs(vps, &req, &res); >>>> + >>>> + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN); >>>> + >>>> + sg_init_one(sgo, req, sizeof(*req)); >>>> + sg_init_one(sgi, res, sizeof(*res)); >>>> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); >>>> + virtqueue_kick(vps->vq[0]); >>>> + >>>> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); >>> >>> Does this block userspace in an uninterruptible wait if >>> hardware is slow? That's not nice. >> >> Yes, but it's not a common operation and I just wanted to make it >> simple. >> >> >>> >>>> + return virtio32_to_cpu(vps->vdev, res->ret); >>>> +} >>>> + >> >> [SNIP] >>>> +struct virtio_pstore_fileinfo { >>>> + __virtio64 id; >>>> + __virtio32 count; >>>> + __virtio16 type; >>>> + __virtio16 unused; >>>> + __virtio32 flags; >>>> + __virtio32 len; >>>> + __virtio64 time_sec; >>>> + __virtio32 time_nsec; >>>> + __virtio32 reserved; >>>> +}; >>>> + >>>> +struct virtio_pstore_config { >>>> + __virtio32 bufsize; >>>> +}; >>>> + >>> >>> What exactly does each field mean? I'm especially >>> interested in time fields - maintaining a consistent >>> time between host and guest is not a simple problem. >> >> These are required by pstore and will be used to create corresponding >> files in the pstore filesystem. The time fields are for mtime and >> ctime and, I think, it's just a hint for user and doesn't require >> strict consistency. > > Pls add documentation. I would just drop hints for now. > >> >> Thanks for your review! >> Namhyung >> >>> >>>> +#endif /* _LINUX_VIRTIO_PSTORE_H */ >>>> -- >>>> 2.9.3 > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 15, 2016 at 02:50:11PM +0900, Namhyung Kim wrote: > On Tue, Nov 15, 2016 at 07:06:28AM +0200, Michael S. Tsirkin wrote: > > On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote: > > > On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote: > > > [SNIP] > > > > > +struct virtio_pstore_fileinfo { > > > > > + __virtio64 id; > > > > > + __virtio32 count; > > > > > + __virtio16 type; > > > > > + __virtio16 unused; > > > > > + __virtio32 flags; > > > > > + __virtio32 len; > > > > > + __virtio64 time_sec; > > > > > + __virtio32 time_nsec; > > > > > + __virtio32 reserved; > > > > > +}; > > > > > + > > > > > +struct virtio_pstore_config { > > > > > + __virtio32 bufsize; > > > > > +}; > > > > > + > > > > > > > > What exactly does each field mean? I'm especially > > > > interested in time fields - maintaining a consistent > > > > time between host and guest is not a simple problem. > > > > > > These are required by pstore and will be used to create corresponding > > > files in the pstore filesystem. The time fields are for mtime and > > > ctime and, I think, it's just a hint for user and doesn't require > > > strict consistency. > > > > Pls add documentation. I would just drop hints for now. > > Well, I'll add docmentation. But I think just dropping might not good > since they all have host time and it's helpful to know their relative > difference in guest. > > Thanks, > Namhyung If it's part of host/guest ABI it needs to be better defined. "It's just a hint does not need to be exact" is too vague, we need to specify what kind of change will or will not break guests.
Hi, On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote: > > > On 15/11/2016 06:06, Michael S. Tsirkin wrote: > > On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote: > >> Hi Michael, > >> > >> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote: > >>> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. > >>> > >>> Do you mean a legacy virtio device? I don't see why > >>> you would want to support pre-1.0 mode. > >>> If you drop that, you can drop all cpu_to_virtio things > >>> and just use __le accessors. > >> > >> I was thinking about the kvmtools which lacks 1.0 support AFAIK. > > > > Unless kvmtools wants to be left behind it has to go 1.0. > > And it also has to go ACPI. Is there any reason, apart from kvmtool, to > make a completely new virtio device, with no support in existing guests, > rather than implement ACPI ERST? Well, I know nothing about ACPI. It looks like a huge spec and I don't want to dig into it just for this. What I want is to speed up dumping guest kernel message (especially for ftrace dump). Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/11/2016 15:36, Namhyung Kim wrote: > Hi, > > On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote: >> >> >> On 15/11/2016 06:06, Michael S. Tsirkin wrote: >>> On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote: >>>> Hi Michael, >>>> >>>> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote: >>>>> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. >>>>> >>>>> Do you mean a legacy virtio device? I don't see why >>>>> you would want to support pre-1.0 mode. >>>>> If you drop that, you can drop all cpu_to_virtio things >>>>> and just use __le accessors. >>>> >>>> I was thinking about the kvmtools which lacks 1.0 support AFAIK. >>> >>> Unless kvmtools wants to be left behind it has to go 1.0. >> >> And it also has to go ACPI. Is there any reason, apart from kvmtool, to >> make a completely new virtio device, with no support in existing guests, >> rather than implement ACPI ERST? > > Well, I know nothing about ACPI. It looks like a huge spec and I > don't want to dig into it just for this. ERST (error record serialization table) is a small subset of the ACPI spec. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Nov 15, 2016 at 11:38 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 15/11/2016 15:36, Namhyung Kim wrote: >> Hi, >> >> On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote: >>> >>> >>> On 15/11/2016 06:06, Michael S. Tsirkin wrote: >>>> On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote: >>>>> Hi Michael, >>>>> >>>>> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote: >>>>>> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim 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. >>>>>> >>>>>> Do you mean a legacy virtio device? I don't see why >>>>>> you would want to support pre-1.0 mode. >>>>>> If you drop that, you can drop all cpu_to_virtio things >>>>>> and just use __le accessors. >>>>> >>>>> I was thinking about the kvmtools which lacks 1.0 support AFAIK. >>>> >>>> Unless kvmtools wants to be left behind it has to go 1.0. >>> >>> And it also has to go ACPI. Is there any reason, apart from kvmtool, to >>> make a completely new virtio device, with no support in existing guests, >>> rather than implement ACPI ERST? >> >> Well, I know nothing about ACPI. It looks like a huge spec and I >> don't want to dig into it just for this. > > ERST (error record serialization table) is a small subset of the ACPI spec. Not sure how independent ERST is from ACPI and other specs. It looks like referencing UEFI spec at least. Btw, is the ERST used for pstore only (in Linux)? Also I need to control pstore driver like using bigger buffer, enabling specific message types and so on if ERST supports. Is it possible for ERST to provide such information? Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Not sure how independent ERST is from ACPI and other specs. It looks > like referencing UEFI spec at least. It is just the format of error records that comes from the UEFI spec (include/linux/cper.h) but you can ignore it, I think. It should be handled by tools on the host side. For you, the error log address range contains a CPER header followed by a binary blob. In practice, you only need the record length field (bytes 20-23 of the header), though it may be a good idea to validate the signature at the beginning of the header. > Btw, is the ERST used for pstore only (in Linux)? Yes. It can store various records, including dmesg and MCE. There are other examples in QEMU of interfaces with ACPI. They all use the DSDT, but the logic is similar. For example, docs/specs/acpi_mem_hotplug.txt documents the memory hotplug interface. In all cases, ACPI tables contain small programs that talk to specialized hardware registers, typically allocated to hard-coded I/O ports. In your case, the registers could occupy 16 consecutive I/O ports, like the following: 0x00 read/write operation type (0=write,1=read,2=clear,3=dummy write) 0x01 read-only bit 7: if set, operation in progress bit 0-6: operation status, see "Command Status Definition" in the ACPI spec 0x02 read-only when read: - read a 64-bit record id from the store to memory, from the address that was last written to 0x08. - if the id is valid and is not the last id in the store, write the next 64-bit record id to the same address - otherwise, write the first record id to the same address, or 0xffffffffffffffff if the store is empty 0x03 unused, read as zero 0x04-0x07 read/write offset of the error record into the error log address range 0x08-0x0b read/write when read, return number of stored records when written, the written value is a 32-bit memory address, which points to a 64-bit location used to communicate record ids. 0x0c-0x0f read/write when read, always return -1 (together with the "mask" field and READ_REGISTER, this lets ERST instructions return any value!) when written, trigger the pstore operation: - if the current operation is a dummy write, do nothing - if the current operation is a write, write a new record, using the written value as the base of the error log address range. The length must be parsed from the CPER header. - if the current operation is a clear, read the record id from the memory location that was last written to 0x08 and do the operation. the value written is ignored. - if the current operation is a read, read the record id from the memory location that was last written to 0x08, using the written value as the base of the error log address range. In addition, the firmware will need to reserve a few KB of RAM for the error log address range (I checked a real system and it reserves 8KB). The first eight bytes are needed for the record identifier interface, because there's no such thing as 64-bit I/O ports, and the rest can be used for the actual buffer. QEMU already has an interface to allocate RAM and patch the address into an ACPI table (bios_linker_loader_alloc). Because this interface is actually meant to load data from QEMU into the firmware (using the "fw_cfg" interface), you would have to add a dummy 8KB file to fw_cfg using fw_cfg_add_file (for example "etc/erst-memory"), it can be just full of zeros. QEMU supports two chipsets, PIIX and ICH9, and the free I/O port ranges are different. You could use 0xa20 for ICH9 and 0xae20 for PIIX. All in all, the contents of the ERST table would not be very different from a non-virtual system, except that on real hardware the firmware would use SMIs as the trap mechanism. You almost have a one-to-one mapping between ERST actions and registers accesses: BEGIN_WRITE_OPERATION write value 0 to register at 0x00 BEGIN_READ_OPERATION write value 1 to register at 0x00 BEGIN_CLEAR_OPERATION write value 2 to register at 0x00 BEGIN_DUMMY_WRITE_OPERATION write value 3 to register at 0x00 END_OPERATION no-op CHECK_BUSY_STATUS read register at 0x01 with mask 0x80 GET_COMMAND_STATUS read register at 0x01 with mask 0x7f SET_RECORD_OFFSET write register at 0x04 GET_RECORD_COUNT read register at 0x08 EXECUTE_OPERATION write ERST memory base + 8 to 0x0c GET_ERROR_LOG_ADDRESS_RANGE read register at 0x0c (with mask = ERST memory base + 8) GET_ERROR_LOG_ADDRESS_RANGE_LENGTH read register at 0x0c (with mask = 8192 - 8 = 8184) GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES read register at 0x0c (with mask = 0) Only the get/set record identifier instructions are a little harder: GET_RECORD_IDENTIFIER write ERST memory base to register at 0x08 read register at 0x02 read eight bytes at ERST memory base SET_RECORD_IDENTIFIER write ERST memory base to register at 0x08 write eight bytes at ERST memory base On top of this, you need to add the APEI UUID (see apei_osc_setup in Linux) to build_q35_osc_method, and use "-M q35" when you start QEMU. If you need more help just ask. I or others can help you with the ACPI glue, then you can write the file backend yourself, based on your existing virtio-pstore code. > Also I need to control pstore driver like using bigger buffer, > enabling specific message types and so on if ERST supports. Is it > possible for ERST to provide such information? It's the normal pstore driver, same as on a real server. What exactly do you need? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Thanks for your detailed information, On Wed, Nov 16, 2016 at 07:10:36AM -0500, Paolo Bonzini wrote: > > Not sure how independent ERST is from ACPI and other specs. It looks > > like referencing UEFI spec at least. > > It is just the format of error records that comes from the UEFI spec > (include/linux/cper.h) but you can ignore it, I think. It should be > handled by tools on the host side. For you, the error log address > range contains a CPER header followed by a binary blob. In practice, > you only need the record length field (bytes 20-23 of the header), > though it may be a good idea to validate the signature at the beginning > of the header. > > > Btw, is the ERST used for pstore only (in Linux)? > > Yes. It can store various records, including dmesg and MCE. > > There are other examples in QEMU of interfaces with ACPI. They all use the > DSDT, but the logic is similar. For example, docs/specs/acpi_mem_hotplug.txt > documents the memory hotplug interface. In all cases, ACPI tables contain small > programs that talk to specialized hardware registers, typically allocated to > hard-coded I/O ports. > > In your case, the registers could occupy 16 consecutive I/O ports, like the > following: > > 0x00 read/write operation type (0=write,1=read,2=clear,3=dummy write) > > 0x01 read-only bit 7: if set, operation in progress > > bit 0-6: operation status, see "Command Status Definition" in > the ACPI spec > > 0x02 read-only when read: > > - read a 64-bit record id from the store to memory, > from the address that was last written to 0x08. > > - if the id is valid and is not the last id in the store, > write the next 64-bit record id to the same address > > - otherwise, write the first record id to the same address, > or 0xffffffffffffffff if the store is empty > > 0x03 unused, read as zero > > 0x04-0x07 read/write offset of the error record into the error log address range > > 0x08-0x0b read/write when read, return number of stored records > > when written, the written value is a 32-bit memory address, > which points to a 64-bit location used to communicate record ids. > > 0x0c-0x0f read/write when read, always return -1 (together with the "mask" field > and READ_REGISTER, this lets ERST instructions return any value!) > > when written, trigger the pstore operation: > > - if the current operation is a dummy write, do nothing > > - if the current operation is a write, write a new record, using > the written value as the base of the error log address range. The > length must be parsed from the CPER header. > > - if the current operation is a clear, read the record id > from the memory location that was last written to 0x08 and do the > operation. the value written is ignored. > > - if the current operation is a read, read the record id from the > memory location that was last written to 0x08, using the written > value as the base of the error log address range. > > In addition, the firmware will need to reserve a few KB of RAM for the error log > address range (I checked a real system and it reserves 8KB). The first eight > bytes are needed for the record identifier interface, because there's no such > thing as 64-bit I/O ports, and the rest can be used for the actual buffer. Is there a limit on the size? It'd be great if it can use a few MB.. > > QEMU already has an interface to allocate RAM and patch the address into an > ACPI table (bios_linker_loader_alloc). Because this interface is actually meant > to load data from QEMU into the firmware (using the "fw_cfg" interface), you > would have to add a dummy 8KB file to fw_cfg using fw_cfg_add_file (for > example "etc/erst-memory"), it can be just full of zeros. > > QEMU supports two chipsets, PIIX and ICH9, and the free I/O port ranges are > different. You could use 0xa20 for ICH9 and 0xae20 for PIIX. > > All in all, the contents of the ERST table would not be very different from a > non-virtual system, except that on real hardware the firmware would use SMIs > as the trap mechanism. You almost have a one-to-one mapping between ERST > actions and registers accesses: > > BEGIN_WRITE_OPERATION write value 0 to register at 0x00 > BEGIN_READ_OPERATION write value 1 to register at 0x00 > BEGIN_CLEAR_OPERATION write value 2 to register at 0x00 > BEGIN_DUMMY_WRITE_OPERATION write value 3 to register at 0x00 > END_OPERATION no-op > CHECK_BUSY_STATUS read register at 0x01 with mask 0x80 > GET_COMMAND_STATUS read register at 0x01 with mask 0x7f > SET_RECORD_OFFSET write register at 0x04 > GET_RECORD_COUNT read register at 0x08 > EXECUTE_OPERATION write ERST memory base + 8 to 0x0c > GET_ERROR_LOG_ADDRESS_RANGE read register at 0x0c (with mask = ERST memory base + 8) > GET_ERROR_LOG_ADDRESS_RANGE_LENGTH read register at 0x0c (with mask = 8192 - 8 = 8184) > GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES read register at 0x0c (with mask = 0) > > Only the get/set record identifier instructions are a little harder: > > GET_RECORD_IDENTIFIER write ERST memory base to register at 0x08 > read register at 0x02 > read eight bytes at ERST memory base > > SET_RECORD_IDENTIFIER write ERST memory base to register at 0x08 > write eight bytes at ERST memory base > > On top of this, you need to add the APEI UUID (see apei_osc_setup in Linux) > to build_q35_osc_method, and use "-M q35" when you start QEMU. If you need > more help just ask. I or others can help you with the ACPI glue, then you > can write the file backend yourself, based on your existing virtio-pstore code. > > > Also I need to control pstore driver like using bigger buffer, > > enabling specific message types and so on if ERST supports. Is it > > possible for ERST to provide such information? > > It's the normal pstore driver, same as on a real server. What exactly do you > need? Well, I don't want to send additional pstore messages to the device if it cannot handle them properly - for example, ftrace message should not overwrite kmsg dump. It'd be great if device somehow could expose acceptable message types to the driver IMHO. Btw I prefer using the kvmtool for my kernel work since it's much more simpler.. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 18, 2016 at 12:32:06PM +0900, Namhyung Kim wrote: > Btw I prefer using the kvmtool for my kernel work since it's much more > simpler.. > > Thanks, > Namhyung Up to you but then you should extend that to support 1.0 spec. I strongly object to adding to the list of legacy interfaces we need to maintain.
On 18/11/2016 04:32, Namhyung Kim wrote: >> In addition, the firmware will need to reserve a few KB of RAM for the error log >> address range (I checked a real system and it reserves 8KB). The first eight >> bytes are needed for the record identifier interface, because there's no such >> thing as 64-bit I/O ports, and the rest can be used for the actual buffer. > > Is there a limit on the size? It'd be great if it can use a few MB.. Yes, you can make it customizable. >>> Also I need to control pstore driver like using bigger buffer, >>> enabling specific message types and so on if ERST supports. Is it >>> possible for ERST to provide such information? >> >> It's the normal pstore driver, same as on a real server. What exactly do you >> need? > > Well, I don't want to send additional pstore messages to the device if > it cannot handle them properly - for example, ftrace message should not > overwrite kmsg dump. It'd be great if device somehow could expose > acceptable message types to the driver IMHO. This is something that you have to do in the usual kernel pstore infrastructure. It should not be specific to virtualization. Paolo > Btw I prefer using the kvmtool for my kernel work since it's much more > simpler.. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/11/2016 05:07, Michael S. Tsirkin wrote: > On Fri, Nov 18, 2016 at 12:32:06PM +0900, Namhyung Kim wrote: >> Btw I prefer using the kvmtool for my kernel work since it's much more >> simpler.. > > Up to you but then you should extend that to support 1.0 spec. > I strongly object to adding to the list of legacy interfaces > we need to maintain. I object to adding paravirtualization unless there is a good reason why the usual mechanisms for physical machines cannot be used. The cost of maintaining a spec, two device implementations (kvmtool+qemu) and a driver is not small, plus it will not work on older kernels. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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..0a63c7db4278 --- /dev/null +++ b/drivers/virtio/virtio_pstore.c @@ -0,0 +1,417 @@ +#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) +#define VIRT_PSTORE_NR_REQ 128 + +struct virtio_pstore { + struct virtio_device *vdev; + struct virtqueue *vq[2]; + struct pstore_info pstore; + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ]; + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ]; + unsigned int req_id; + + /* Waiting for host to ack */ + wait_queue_head_t acked; + int failed; +}; + +#define TYPE_TABLE_ENTRY(_entry) \ + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry } + +struct type_table { + int pstore; + u16 virtio; +} type_table[] = { + TYPE_TABLE_ENTRY(DMESG), +}; + +#undef TYPE_TABLE_ENTRY + + +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(type_table); i++) { + if (type == type_table[i].pstore) + return cpu_to_virtio16(vps->vdev, type_table[i].virtio); + } + + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); +} + +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(type_table); i++) { + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio) + return type_table[i].pstore; + } + + return PSTORE_TYPE_UNKNOWN; +} + +static void virtpstore_ack(struct virtqueue *vq) +{ + struct virtio_pstore *vps = vq->vdev->priv; + + wake_up(&vps->acked); +} + +static void virtpstore_check(struct virtqueue *vq) +{ + struct virtio_pstore *vps = vq->vdev->priv; + struct virtio_pstore_res *res; + unsigned int len; + + res = virtqueue_get_buf(vq, &len); + if (res == NULL) + return; + + if (virtio32_to_cpu(vq->vdev, res->ret) < 0) + vps->failed = 1; +} + +static void virt_pstore_get_reqs(struct virtio_pstore *vps, + struct virtio_pstore_req **preq, + struct virtio_pstore_res **pres) +{ + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ; + + *preq = &vps->req[idx]; + *pres = &vps->res[idx]; + + memset(*preq, 0, sizeof(**preq)); + memset(*pres, 0, sizeof(**pres)); +} + +static int virt_pstore_open(struct pstore_info *psi) +{ + struct virtio_pstore *vps = psi->data; + struct virtio_pstore_req *req; + struct virtio_pstore_res *res; + struct scatterlist sgo[1], sgi[1]; + struct scatterlist *sgs[2] = { sgo, sgi }; + unsigned int len; + + virt_pstore_get_reqs(vps, &req, &res); + + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN); + + sg_init_one(sgo, req, sizeof(*req)); + sg_init_one(sgi, res, sizeof(*res)); + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); + virtqueue_kick(vps->vq[0]); + + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); + return virtio32_to_cpu(vps->vdev, res->ret); +} + +static int virt_pstore_close(struct pstore_info *psi) +{ + struct virtio_pstore *vps = psi->data; + struct virtio_pstore_req *req = &vps->req[vps->req_id]; + struct virtio_pstore_res *res = &vps->res[vps->req_id]; + struct scatterlist sgo[1], sgi[1]; + struct scatterlist *sgs[2] = { sgo, sgi }; + unsigned int len; + + virt_pstore_get_reqs(vps, &req, &res); + + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_CLOSE); + + sg_init_one(sgo, req, sizeof(*req)); + sg_init_one(sgi, res, sizeof(*res)); + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); + virtqueue_kick(vps->vq[0]); + + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); + return virtio32_to_cpu(vps->vdev, res->ret); +} + +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type, + int *count, struct timespec *time, + char **buf, bool *compressed, + ssize_t *ecc_notice_size, + struct pstore_info *psi) +{ + struct virtio_pstore *vps = psi->data; + struct virtio_pstore_req *req; + struct virtio_pstore_res *res; + struct virtio_pstore_fileinfo info; + struct scatterlist sgo[1], sgi[3]; + struct scatterlist *sgs[2] = { sgo, sgi }; + unsigned int len; + unsigned int flags; + int ret; + void *bf; + + virt_pstore_get_reqs(vps, &req, &res); + + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_READ); + + sg_init_one(sgo, req, sizeof(*req)); + sg_init_table(sgi, 3); + sg_set_buf(&sgi[0], res, sizeof(*res)); + sg_set_buf(&sgi[1], &info, sizeof(info)); + sg_set_buf(&sgi[2], psi->buf, psi->bufsize); + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); + virtqueue_kick(vps->vq[0]); + + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); + if (len < sizeof(*res) + sizeof(info)) + return -1; + + ret = virtio32_to_cpu(vps->vdev, res->ret); + if (ret < 0) + return ret; + + len = virtio32_to_cpu(vps->vdev, info.len); + + bf = kmalloc(len, GFP_KERNEL); + if (bf == NULL) + return -ENOMEM; + + *id = virtio64_to_cpu(vps->vdev, info.id); + *type = from_virtio_type(vps, info.type); + *count = virtio32_to_cpu(vps->vdev, info.count); + + flags = virtio32_to_cpu(vps->vdev, info.flags); + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED; + + time->tv_sec = virtio64_to_cpu(vps->vdev, info.time_sec); + time->tv_nsec = virtio32_to_cpu(vps->vdev, info.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_req *req; + struct virtio_pstore_res *res; + struct scatterlist sgo[2], sgi[1]; + struct scatterlist *sgs[2] = { sgo, sgi }; + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0; + + if (vps->failed) + return -1; + + *id = vps->req_id; + virt_pstore_get_reqs(vps, &req, &res); + + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE); + req->type = to_virtio_type(vps, type); + req->flags = cpu_to_virtio32(vps->vdev, flags); + + sg_init_table(sgo, 2); + sg_set_buf(&sgo[0], req, sizeof(*req)); + sg_set_buf(&sgo[1], pstore_get_buf(psi), size); + sg_init_one(sgi, res, sizeof(*res)); + virtqueue_add_sgs(vps->vq[1], sgs, 1, 1, vps, GFP_ATOMIC); + virtqueue_kick(vps->vq[1]); + + 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_req *req; + struct virtio_pstore_res *res; + struct scatterlist sgo[1], sgi[1]; + struct scatterlist *sgs[2] = { sgo, sgi }; + unsigned int len; + + virt_pstore_get_reqs(vps, &req, &res); + + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE); + req->type = to_virtio_type(vps, type); + req->id = cpu_to_virtio64(vps->vdev, id); + req->count = cpu_to_virtio32(vps->vdev, count); + + sg_init_one(sgo, req, sizeof(*req)); + sg_init_one(sgi, res, sizeof(*res)); + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL); + virtqueue_kick(vps->vq[0]); + + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len)); + return virtio32_to_cpu(vps->vdev, res->ret); +} + +static int virt_pstore_init(struct virtio_pstore *vps) +{ + struct pstore_info *psinfo = &vps->pstore; + int err; + + if (!psinfo->bufsize) + psinfo->bufsize = VIRT_PSTORE_BUFSIZE; + + psinfo->buf = alloc_pages_exact(psinfo->bufsize, GFP_KERNEL); + 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_DMESG; + + 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_exact(psinfo->buf, psinfo->bufsize); + psinfo->buf = NULL; + psinfo->bufsize = 0; + + return 0; +} + +static int virtpstore_init_vqs(struct virtio_pstore *vps) +{ + vq_callback_t *callbacks[] = { virtpstore_ack, virtpstore_check }; + const char *names[] = { "pstore_read", "pstore_write" }; + + return vps->vdev->config->find_vqs(vps->vdev, 2, vps->vq, + callbacks, names); +} + +static void virtpstore_init_config(struct virtio_pstore *vps) +{ + u32 bufsize; + + virtio_cread(vps->vdev, struct virtio_pstore_config, bufsize, &bufsize); + + vps->pstore.bufsize = PAGE_ALIGN(bufsize); +} + +static void virtpstore_confirm_config(struct virtio_pstore *vps) +{ + u32 bufsize = vps->pstore.bufsize; + + virtio_cwrite(vps->vdev, struct virtio_pstore_config, bufsize, + &bufsize); +} + +static int virtpstore_probe(struct virtio_device *vdev) +{ + struct virtio_pstore *vps; + int err; + + if (!vdev->config->get) { + dev_err(&vdev->dev, "driver init: config access disabled\n"); + return -EINVAL; + } + + vdev->priv = vps = kzalloc(sizeof(*vps), GFP_KERNEL); + if (!vps) { + err = -ENOMEM; + goto out; + } + vps->vdev = vdev; + + err = virtpstore_init_vqs(vps); + if (err < 0) + goto out_free; + + virtpstore_init_config(vps); + + err = virt_pstore_init(vps); + if (err) + goto out_del_vq; + + virtpstore_confirm_config(vps); + + init_waitqueue_head(&vps->acked); + + virtio_device_ready(vdev); + + dev_info(&vdev->dev, "driver init: ok (bufsize = %luK, flags = %x)\n", + vps->pstore.bufsize >> 10, vps->pstore.flags); + + return 0; + +out_del_vq: + vdev->config->del_vqs(vdev); +out_free: + kfree(vps); +out: + dev_err(&vdev->dev, "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@kernel.org>"); +MODULE_DESCRIPTION("Virtio pstore driver"); diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild index 6d4e92ccdc91..9bbb1554d8b2 100644 --- a/include/uapi/linux/Kbuild +++ b/include/uapi/linux/Kbuild @@ -449,6 +449,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..c72a9ab588c0 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 22 /* 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..f4b0d204d8ae --- /dev/null +++ b/include/uapi/linux/virtio_pstore.h @@ -0,0 +1,74 @@ +#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_req { + __virtio16 cmd; + __virtio16 type; + __virtio32 flags; + __virtio64 id; + __virtio32 count; + __virtio32 reserved; +}; + +struct virtio_pstore_res { + __virtio16 cmd; + __virtio16 type; + __virtio32 ret; +}; + +struct virtio_pstore_fileinfo { + __virtio64 id; + __virtio32 count; + __virtio16 type; + __virtio16 unused; + __virtio32 flags; + __virtio32 len; + __virtio64 time_sec; + __virtio32 time_nsec; + __virtio32 reserved; +}; + +struct virtio_pstore_config { + __virtio32 bufsize; +}; + +#endif /* _LINUX_VIRTIO_PSTORE_H */
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. It uses two virtqueues - one for (sync) read and another for (async) write. Since it cannot wait for write finished, it supports up to 128 concurrent IO. The buffer size is configurable now. Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Radim Krčmář <rkrcmar@redhat.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Anthony Liguori <aliguori@amazon.com> Cc: Anton Vorontsov <anton@enomsg.org> Cc: Colin Cross <ccross@android.com> Cc: Kees Cook <keescook@chromium.org> Cc: Tony Luck <tony.luck@intel.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Minchan Kim <minchan@kernel.org> Cc: kvm@vger.kernel.org Cc: qemu-devel@nongnu.org Cc: virtualization@lists.linux-foundation.org Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- drivers/virtio/Kconfig | 10 + drivers/virtio/Makefile | 1 + drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++ include/uapi/linux/Kbuild | 1 + include/uapi/linux/virtio_ids.h | 1 + include/uapi/linux/virtio_pstore.h | 74 +++++++ 6 files changed, 504 insertions(+) create mode 100644 drivers/virtio/virtio_pstore.c create mode 100644 include/uapi/linux/virtio_pstore.h