From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941202AbcKOFGe (ORCPT ); Tue, 15 Nov 2016 00:06:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39518 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751803AbcKOFGd (ORCPT ); Tue, 15 Nov 2016 00:06:33 -0500 Date: Tue, 15 Nov 2016 07:06:28 +0200 From: "Michael S. Tsirkin" To: Namhyung Kim 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: <20161115065658-mutt-send-email-mst@kernel.org> References: <20160820080744.10344-1-namhyung@kernel.org> <20160820080744.10344-2-namhyung@kernel.org> <20161110182611-mutt-send-email-mst@kernel.org> <20161115045021.GA15992@danjae.aot.lge.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20161115045021.GA15992@danjae.aot.lge.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 15 Nov 2016 05:06:32 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > 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. Pls add documentation. I would just drop hints for now. > > Thanks for your review! > Namhyung > > > > > > +#endif /* _LINUX_VIRTIO_PSTORE_H */ > > > -- > > > 2.9.3