From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965863AbcKOEvY (ORCPT ); Mon, 14 Nov 2016 23:51:24 -0500 Received: from mail-pg0-f67.google.com ([74.125.83.67]:32803 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932906AbcKOEvW (ORCPT ); Mon, 14 Nov 2016 23:51:22 -0500 Date: Tue, 15 Nov 2016 13:50:21 +0900 From: Namhyung Kim To: "Michael S. Tsirkin" Cc: virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org, qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org, LKML , Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= , Anthony Liguori , Anton Vorontsov , Colin Cross , Kees Cook , Tony Luck , Steven Rostedt , Ingo Molnar , Minchan Kim Subject: Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Message-ID: <20161115045021.GA15992@danjae.aot.lge.com> References: <20160820080744.10344-1-namhyung@kernel.org> <20160820080744.10344-2-namhyung@kernel.org> <20161110182611-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20161110182611-mutt-send-email-mst@kernel.org> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > Cc: Radim Krčmář > > Cc: "Michael S. Tsirkin" > > Cc: Anthony Liguori > > Cc: Anton Vorontsov > > Cc: Colin Cross > > Cc: Kees Cook > > Cc: Tony Luck > > Cc: Steven Rostedt > > Cc: Ingo Molnar > > Cc: Minchan Kim > > Cc: kvm@vger.kernel.org > > Cc: qemu-devel@nongnu.org > > Cc: virtualization@lists.linux-foundation.org > > Signed-off-by: Namhyung Kim > > --- > > 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#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