From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966573AbcKOJ5i (ORCPT ); Tue, 15 Nov 2016 04:57:38 -0500 Received: from mail-wm0-f67.google.com ([74.125.82.67]:35411 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966501AbcKOJ5c (ORCPT ); Tue, 15 Nov 2016 04:57:32 -0500 Subject: Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver To: "Michael S. Tsirkin" , Namhyung Kim 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> <20161115065658-mutt-send-email-mst@kernel.org> Cc: virtio-dev@lists.oasis-open.org, Tony Luck , Kees Cook , kvm@vger.kernel.org, =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Anton Vorontsov , LKML , Steven Rostedt , qemu-devel@nongnu.org, Minchan Kim , Anthony Liguori , Colin Cross , virtualization@lists.linux-foundation.org, Ingo Molnar From: Paolo Bonzini Message-ID: Date: Tue, 15 Nov 2016 10:57:29 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161115065658-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> 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 > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization >