Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2016-11-15 05:06:34
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:quoted
On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:quoted
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.quoted
quoted
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ář <redacted> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Anthony Liguori <redacted> Cc: Anton Vorontsov <redacted> Cc: Colin Cross <redacted> Cc: Kees Cook <redacted> 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.hdiff --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_DMAdiff --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.odiff --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.quoted
quoted
+ 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_ENTRYlet'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.quoted
quoted
+ +single emoty line plsOk.quoted
quoted
+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.quoted
quoted
+} + +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.quoted
quoted
+ return virtio32_to_cpu(vps->vdev, res->ret); +} +[SNIP]quoted
quoted
+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! Namhyungquoted
quoted
+#endif /* _LINUX_VIRTIO_PSTORE_H */ -- 2.9.3
_______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization