Message-ID: <f514ac65-3105-a12c-9e10-2d3b1e8a0516@redhat.com>
Date:   Tue, 15 Nov 2016 10:57:29 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>
Cc:     virtio-dev@...ts.oasis-open.org, Tony Luck <tony.luck@...el.com>,
        Kees Cook <keescook@...omium.org>, kvm@...r.kernel.org,
        Radim Krčmář <rkrcmar@...hat.com>,
        Anton Vorontsov <anton@...msg.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>, qemu-devel@...gnu.org,
        Minchan Kim <minchan@...nel.org>,
        Anthony Liguori <aliguori@...zon.com>,
        Colin Cross <ccross@...roid.com>,
        virtualization@...ts.linux-foundation.org,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver



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@...hat.com>
>>>> Cc: Radim Krčmář <rkrcmar@...hat.com>
>>>> Cc: "Michael S. Tsirkin" <mst@...hat.com>
>>>> Cc: Anthony Liguori <aliguori@...zon.com>
>>>> Cc: Anton Vorontsov <anton@...msg.org>
>>>> Cc: Colin Cross <ccross@...roid.com>
>>>> Cc: Kees Cook <keescook@...omium.org>
>>>> Cc: Tony Luck <tony.luck@...el.com>
>>>> Cc: Steven Rostedt <rostedt@...dmis.org>
>>>> Cc: Ingo Molnar <mingo@...nel.org>
>>>> Cc: Minchan Kim <minchan@...nel.org>
>>>> Cc: kvm@...r.kernel.org
>>>> Cc: qemu-devel@...gnu.org
>>>> Cc: virtualization@...ts.linux-foundation.org
>>>> Signed-off-by: Namhyung Kim <namhyung@...nel.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@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

Powered by blists - more mailing lists