[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
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 <pbonzini@redhat.com>
> > > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: Anthony Liguori <aliguori@amazon.com>
> > > Cc: Anton Vorontsov <anton@enomsg.org>
> > > Cc: Colin Cross <ccross@android.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Tony Luck <tony.luck@intel.com>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Ingo Molnar <mingo@kernel.org>
> > > Cc: Minchan Kim <minchan@kernel.org>
> > > Cc: kvm@vger.kernel.org
> > > Cc: qemu-devel@nongnu.org
> > > Cc: virtualization@lists.linux-foundation.org
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]