Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

[Namhyung Kim]

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 <pbonzini@xxxxxxxxxx>
> > Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> > Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> > Cc: Anthony Liguori <aliguori@xxxxxxxxxx>
> > Cc: Anton Vorontsov <anton@xxxxxxxxxx>
> > Cc: Colin Cross <ccross@xxxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Tony Luck <tony.luck@xxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Minchan Kim <minchan@xxxxxxxxxx>
> > Cc: kvm@xxxxxxxxxxxxxxx
> > Cc: qemu-devel@xxxxxxxxxx
> > Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >  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.

Thanks for your review!
Namhyung

> 
> > +#endif /* _LINUX_VIRTIO_PSTORE_H */
> > -- 
> > 2.9.3