Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1522339 > unrolled thread

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

Started byNamhyung Kim <namhyung@kernel.org>
First post2016-11-15 06:00 +0100
Last post2016-11-18 10:50 +0100
Articles 13 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim <namhyung@kernel.org> - 2016-11-15 06:00 +0100
    Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver "Michael S. Tsirkin" <mst@redhat.com> - 2016-11-15 06:10 +0100
      Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim <namhyung@kernel.org> - 2016-11-15 07:00 +0100
        Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver "Michael S. Tsirkin" <mst@redhat.com> - 2016-11-15 15:40 +0100
      Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Paolo Bonzini <pbonzini@redhat.com> - 2016-11-15 11:00 +0100
        Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim <namhyung@kernel.org> - 2016-11-15 15:40 +0100
          Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Paolo Bonzini <pbonzini@redhat.com> - 2016-11-15 15:40 +0100
            Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim <namhyung@kernel.org> - 2016-11-16 08:10 +0100
              Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore  driver Paolo Bonzini <pbonzini@redhat.com> - 2016-11-16 13:20 +0100
                Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim <namhyung@kernel.org> - 2016-11-18 04:40 +0100
                  Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver "Michael S. Tsirkin" <mst@redhat.com> - 2016-11-18 05:10 +0100
                    Re: [virtio-dev] Re: [PATCH 1/3] virtio: Basic implementation of  virtio pstore driver Paolo Bonzini <pbonzini@redhat.com> - 2016-11-18 10:50 +0100
                  Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Paolo Bonzini <pbonzini@redhat.com> - 2016-11-18 10:50 +0100

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

FromNamhyung Kim <namhyung@kernel.org>
Date2016-11-15 06:00 +0100
SubjectRe: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Message-ID<sDE0G-cO-21@gated-at.bofh.it>
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@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.

Thanks for your review!
Namhyung

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

[toc] | [next] | [standalone]

#1522342

From"Michael S. Tsirkin" <mst@redhat.com>
Date2016-11-15 06:10 +0100
Message-ID<sDEal-vS-3@gated-at.bofh.it>
In reply to#1522339
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

[toc] | [prev] | [next] | [standalone]

#1522360

FromNamhyung Kim <namhyung@kernel.org>
Date2016-11-15 07:00 +0100
Message-ID<sDEWJ-Oi-1@gated-at.bofh.it>
In reply to#1522342
On Tue, Nov 15, 2016 at 07:06:28AM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> > On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> > [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.

Well, I'll add docmentation.  But I think just dropping might not good
since they all have host time and it's helpful to know their relative
difference in guest.

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1522774

From"Michael S. Tsirkin" <mst@redhat.com>
Date2016-11-15 15:40 +0100
Message-ID<sDN3X-69H-1@gated-at.bofh.it>
In reply to#1522360
On Tue, Nov 15, 2016 at 02:50:11PM +0900, Namhyung Kim wrote:
> On Tue, Nov 15, 2016 at 07:06:28AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> > > On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> > > [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.
> 
> Well, I'll add docmentation.  But I think just dropping might not good
> since they all have host time and it's helpful to know their relative
> difference in guest.
> 
> Thanks,
> Namhyung

If it's part of host/guest ABI it needs to be better defined.
"It's just a hint does not need to be exact" is too vague,
we need to specify what kind of change will or will not
break guests.

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1522504

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-11-15 11:00 +0100
Message-ID<sDIH0-3dJ-13@gated-at.bofh.it>
In reply to#1522342

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@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
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

[toc] | [prev] | [next] | [standalone]

#1522784

FromNamhyung Kim <namhyung@kernel.org>
Date2016-11-15 15:40 +0100
Message-ID<sDN3Y-69H-39@gated-at.bofh.it>
In reply to#1522504
Hi,

On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote:
> 
> 
> 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?

Well, I know nothing about ACPI.  It looks like a huge spec and I
don't want to dig into it just for this.

What I want is to speed up dumping guest kernel message (especially
for ftrace dump).

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1522785

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-11-15 15:40 +0100
Message-ID<sDN3Y-69H-43@gated-at.bofh.it>
In reply to#1522784

On 15/11/2016 15:36, Namhyung Kim wrote:
> Hi,
> 
> On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote:
>>
>>
>> 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?
> 
> Well, I know nothing about ACPI.  It looks like a huge spec and I
> don't want to dig into it just for this.

ERST (error record serialization table) is a small subset of the ACPI spec.

Paolo

[toc] | [prev] | [next] | [standalone]

#1523258

FromNamhyung Kim <namhyung@kernel.org>
Date2016-11-16 08:10 +0100
Message-ID<sE2w1-89M-3@gated-at.bofh.it>
In reply to#1522785
Hi,

On Tue, Nov 15, 2016 at 11:38 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 15/11/2016 15:36, Namhyung Kim wrote:
>> Hi,
>>
>> On Tue, Nov 15, 2016 at 10:57:29AM +0100, Paolo Bonzini wrote:
>>>
>>>
>>> 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?
>>
>> Well, I know nothing about ACPI.  It looks like a huge spec and I
>> don't want to dig into it just for this.
>
> ERST (error record serialization table) is a small subset of the ACPI spec.

Not sure how independent ERST is from ACPI and other specs.  It looks
like referencing UEFI spec at least.  Btw, is the ERST used for pstore
only (in Linux)?

Also I need to control pstore driver like using bigger buffer,
enabling specific message types and so on if ERST supports.  Is it
possible for ERST to provide such information?

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

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

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-11-16 13:20 +0100
SubjectRe: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Message-ID<sE7m1-2EG-33@gated-at.bofh.it>
In reply to#1523258
> Not sure how independent ERST is from ACPI and other specs.  It looks
> like referencing UEFI spec at least.

It is just the format of error records that comes from the UEFI spec
(include/linux/cper.h) but you can ignore it, I think.  It should be
handled by tools on the host side.  For you, the error log address
range contains a CPER header followed by a binary blob.  In practice,
you only need the record length field (bytes 20-23 of the header),
though it may be a good idea to validate the signature at the beginning
of the header.

> Btw, is the ERST used for pstore only (in Linux)?

Yes.  It can store various records, including dmesg and MCE.

There are other examples in QEMU of interfaces with ACPI.  They all use the
DSDT, but the logic is similar.  For example, docs/specs/acpi_mem_hotplug.txt
documents the memory hotplug interface. In all cases, ACPI tables contain small
programs that talk to specialized hardware registers, typically allocated to
hard-coded I/O ports.

In your case, the registers could occupy 16 consecutive I/O ports, like the
following:

     0x00       read/write   operation type (0=write,1=read,2=clear,3=dummy write)

     0x01       read-only    bit 7: if set, operation in progress

                             bit 0-6: operation status, see "Command Status Definition" in
                             the ACPI spec

     0x02       read-only    when read:

                             - read a 64-bit record id from the store to memory,
                               from the address that was last written to 0x08.

                             - if the id is valid and is not the last id in the store,
                               write the next 64-bit record id to the same address

                             - otherwise, write the first record id to the same address,
                               or 0xffffffffffffffff if the store is empty

     0x03                    unused, read as zero

     0x04-0x07  read/write   offset of the error record into the error log address range

     0x08-0x0b  read/write   when read, return number of stored records

                             when written, the written value is a 32-bit memory address,
                             which points to a 64-bit location used to communicate record ids.

     0x0c-0x0f  read/write   when read, always return -1 (together with the "mask" field
                             and READ_REGISTER, this lets ERST instructions return any value!)

                             when written, trigger the pstore operation:

                             - if the current operation is a dummy write, do nothing

                             - if the current operation is a write, write a new record, using
                             the written value as the base of the error log address range.  The
                             length must be parsed from the CPER header.

                             - if the current operation is a clear, read the record id
                             from the memory location that was last written to 0x08 and do the
                             operation.  the value written is ignored.

                             - if the current operation is a read, read the record id from the
                             memory location that was last written to 0x08, using the written
                             value as the base of the error log address range.

In addition, the firmware will need to reserve a few KB of RAM for the error log
address range (I checked a real system and it reserves 8KB).  The first eight
bytes are needed for the record identifier interface, because there's no such
thing as 64-bit I/O ports, and the rest can be used for the actual buffer.

QEMU already has an interface to allocate RAM and patch the address into an
ACPI table (bios_linker_loader_alloc).  Because this interface is actually meant
to load data from QEMU into the firmware (using the "fw_cfg" interface), you
would have to add a dummy 8KB file to fw_cfg using fw_cfg_add_file (for
example "etc/erst-memory"), it can be just full of zeros.

QEMU supports two chipsets, PIIX and ICH9, and the free I/O port ranges are
different.  You could use 0xa20 for ICH9 and 0xae20 for PIIX.

All in all, the contents of the ERST table would not be very different from a
non-virtual system, except that on real hardware the firmware would use SMIs
as the trap mechanism.  You almost have a one-to-one mapping between ERST
actions and registers accesses:

   BEGIN_WRITE_OPERATION                  write value 0 to register at 0x00
   BEGIN_READ_OPERATION                   write value 1 to register at 0x00
   BEGIN_CLEAR_OPERATION                  write value 2 to register at 0x00
   BEGIN_DUMMY_WRITE_OPERATION            write value 3 to register at 0x00
   END_OPERATION                          no-op
   CHECK_BUSY_STATUS                      read register at 0x01 with mask 0x80
   GET_COMMAND_STATUS                     read register at 0x01 with mask 0x7f
   SET_RECORD_OFFSET                      write register at 0x04
   GET_RECORD_COUNT                       read register at 0x08
   EXECUTE_OPERATION                      write ERST memory base + 8 to 0x0c
   GET_ERROR_LOG_ADDRESS_RANGE            read register at 0x0c (with mask = ERST memory base + 8)
   GET_ERROR_LOG_ADDRESS_RANGE_LENGTH     read register at 0x0c (with mask = 8192 - 8 = 8184)
   GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES read register at 0x0c (with mask = 0)

Only the get/set record identifier instructions are a little harder:

   GET_RECORD_IDENTIFIER                  write ERST memory base to register at 0x08
                                          read register at 0x02
                                          read eight bytes at ERST memory base

   SET_RECORD_IDENTIFIER                  write ERST memory base to register at 0x08
                                          write eight bytes at ERST memory base

On top of this, you need to add the APEI UUID (see apei_osc_setup in Linux)
to build_q35_osc_method, and use "-M q35" when you start QEMU.  If you need
more help just ask.  I or others can help you with the ACPI glue, then you
can write the file backend yourself, based on your existing virtio-pstore code.

> Also I need to control pstore driver like using bigger buffer,
> enabling specific message types and so on if ERST supports.  Is it
> possible for ERST to provide such information?

It's the normal pstore driver, same as on a real server.  What exactly do you
need?

Paolo

[toc] | [prev] | [next] | [standalone]

#1524996

FromNamhyung Kim <namhyung@kernel.org>
Date2016-11-18 04:40 +0100
Message-ID<sEIbT-1yo-11@gated-at.bofh.it>
In reply to#1523426
Hi,

Thanks for your detailed information,

On Wed, Nov 16, 2016 at 07:10:36AM -0500, Paolo Bonzini wrote:
> > Not sure how independent ERST is from ACPI and other specs.  It looks
> > like referencing UEFI spec at least.
> 
> It is just the format of error records that comes from the UEFI spec
> (include/linux/cper.h) but you can ignore it, I think.  It should be
> handled by tools on the host side.  For you, the error log address
> range contains a CPER header followed by a binary blob.  In practice,
> you only need the record length field (bytes 20-23 of the header),
> though it may be a good idea to validate the signature at the beginning
> of the header.
> 
> > Btw, is the ERST used for pstore only (in Linux)?
> 
> Yes.  It can store various records, including dmesg and MCE.
> 
> There are other examples in QEMU of interfaces with ACPI.  They all use the
> DSDT, but the logic is similar.  For example, docs/specs/acpi_mem_hotplug.txt
> documents the memory hotplug interface. In all cases, ACPI tables contain small
> programs that talk to specialized hardware registers, typically allocated to
> hard-coded I/O ports.
> 
> In your case, the registers could occupy 16 consecutive I/O ports, like the
> following:
> 
>      0x00       read/write   operation type (0=write,1=read,2=clear,3=dummy write)
> 
>      0x01       read-only    bit 7: if set, operation in progress
> 
>                              bit 0-6: operation status, see "Command Status Definition" in
>                              the ACPI spec
> 
>      0x02       read-only    when read:
> 
>                              - read a 64-bit record id from the store to memory,
>                                from the address that was last written to 0x08.
> 
>                              - if the id is valid and is not the last id in the store,
>                                write the next 64-bit record id to the same address
> 
>                              - otherwise, write the first record id to the same address,
>                                or 0xffffffffffffffff if the store is empty
> 
>      0x03                    unused, read as zero
> 
>      0x04-0x07  read/write   offset of the error record into the error log address range
> 
>      0x08-0x0b  read/write   when read, return number of stored records
> 
>                              when written, the written value is a 32-bit memory address,
>                              which points to a 64-bit location used to communicate record ids.
> 
>      0x0c-0x0f  read/write   when read, always return -1 (together with the "mask" field
>                              and READ_REGISTER, this lets ERST instructions return any value!)
> 
>                              when written, trigger the pstore operation:
> 
>                              - if the current operation is a dummy write, do nothing
> 
>                              - if the current operation is a write, write a new record, using
>                              the written value as the base of the error log address range.  The
>                              length must be parsed from the CPER header.
> 
>                              - if the current operation is a clear, read the record id
>                              from the memory location that was last written to 0x08 and do the
>                              operation.  the value written is ignored.
> 
>                              - if the current operation is a read, read the record id from the
>                              memory location that was last written to 0x08, using the written
>                              value as the base of the error log address range.
> 
> In addition, the firmware will need to reserve a few KB of RAM for the error log
> address range (I checked a real system and it reserves 8KB).  The first eight
> bytes are needed for the record identifier interface, because there's no such
> thing as 64-bit I/O ports, and the rest can be used for the actual buffer.

Is there a limit on the size?  It'd be great if it can use a few MB..

> 
> QEMU already has an interface to allocate RAM and patch the address into an
> ACPI table (bios_linker_loader_alloc).  Because this interface is actually meant
> to load data from QEMU into the firmware (using the "fw_cfg" interface), you
> would have to add a dummy 8KB file to fw_cfg using fw_cfg_add_file (for
> example "etc/erst-memory"), it can be just full of zeros.
> 
> QEMU supports two chipsets, PIIX and ICH9, and the free I/O port ranges are
> different.  You could use 0xa20 for ICH9 and 0xae20 for PIIX.
> 
> All in all, the contents of the ERST table would not be very different from a
> non-virtual system, except that on real hardware the firmware would use SMIs
> as the trap mechanism.  You almost have a one-to-one mapping between ERST
> actions and registers accesses:
> 
>    BEGIN_WRITE_OPERATION                  write value 0 to register at 0x00
>    BEGIN_READ_OPERATION                   write value 1 to register at 0x00
>    BEGIN_CLEAR_OPERATION                  write value 2 to register at 0x00
>    BEGIN_DUMMY_WRITE_OPERATION            write value 3 to register at 0x00
>    END_OPERATION                          no-op
>    CHECK_BUSY_STATUS                      read register at 0x01 with mask 0x80
>    GET_COMMAND_STATUS                     read register at 0x01 with mask 0x7f
>    SET_RECORD_OFFSET                      write register at 0x04
>    GET_RECORD_COUNT                       read register at 0x08
>    EXECUTE_OPERATION                      write ERST memory base + 8 to 0x0c
>    GET_ERROR_LOG_ADDRESS_RANGE            read register at 0x0c (with mask = ERST memory base + 8)
>    GET_ERROR_LOG_ADDRESS_RANGE_LENGTH     read register at 0x0c (with mask = 8192 - 8 = 8184)
>    GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES read register at 0x0c (with mask = 0)
> 
> Only the get/set record identifier instructions are a little harder:
> 
>    GET_RECORD_IDENTIFIER                  write ERST memory base to register at 0x08
>                                           read register at 0x02
>                                           read eight bytes at ERST memory base
> 
>    SET_RECORD_IDENTIFIER                  write ERST memory base to register at 0x08
>                                           write eight bytes at ERST memory base
> 
> On top of this, you need to add the APEI UUID (see apei_osc_setup in Linux)
> to build_q35_osc_method, and use "-M q35" when you start QEMU.  If you need
> more help just ask.  I or others can help you with the ACPI glue, then you
> can write the file backend yourself, based on your existing virtio-pstore code.
> 
> > Also I need to control pstore driver like using bigger buffer,
> > enabling specific message types and so on if ERST supports.  Is it
> > possible for ERST to provide such information?
> 
> It's the normal pstore driver, same as on a real server.  What exactly do you
> need?

Well, I don't want to send additional pstore messages to the device if
it cannot handle them properly - for example, ftrace message should not
overwrite kmsg dump.  It'd be great if device somehow could expose
acceptable message types to the driver IMHO.

Btw I prefer using the kvmtool for my kernel work since it's much more
simpler..

Thanks,
Namhyung

[toc] | [prev] | [next] | [standalone]

#1524998

From"Michael S. Tsirkin" <mst@redhat.com>
Date2016-11-18 05:10 +0100
Message-ID<sEIEV-1Yz-9@gated-at.bofh.it>
In reply to#1524996
On Fri, Nov 18, 2016 at 12:32:06PM +0900, Namhyung Kim wrote:
> Btw I prefer using the kvmtool for my kernel work since it's much more
> simpler..
> 
> Thanks,
> Namhyung

Up to you but then you should extend that to support 1.0 spec.
I strongly object to adding to the list of legacy interfaces
we need to maintain.

-- 
MST

[toc] | [prev] | [next] | [standalone]

#1525132 — Re: [virtio-dev] Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-11-18 10:50 +0100
SubjectRe: [virtio-dev] Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Message-ID<sENXY-5iQ-21@gated-at.bofh.it>
In reply to#1524998

On 18/11/2016 05:07, Michael S. Tsirkin wrote:
> On Fri, Nov 18, 2016 at 12:32:06PM +0900, Namhyung Kim wrote:
>> Btw I prefer using the kvmtool for my kernel work since it's much more
>> simpler..
> 
> Up to you but then you should extend that to support 1.0 spec.
> I strongly object to adding to the list of legacy interfaces
> we need to maintain.

I object to adding paravirtualization unless there is a good reason why
the usual mechanisms for physical machines cannot be used.  The cost of
maintaining a spec, two device implementations (kvmtool+qemu) and a
driver is not small, plus it will not work on older kernels.

Paolo

[toc] | [prev] | [next] | [standalone]

#1525131

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-11-18 10:50 +0100
Message-ID<sENXY-5iQ-15@gated-at.bofh.it>
In reply to#1524996

On 18/11/2016 04:32, Namhyung Kim wrote:
>> In addition, the firmware will need to reserve a few KB of RAM for the error log
>> address range (I checked a real system and it reserves 8KB).  The first eight
>> bytes are needed for the record identifier interface, because there's no such
>> thing as 64-bit I/O ports, and the rest can be used for the actual buffer.
> 
> Is there a limit on the size?  It'd be great if it can use a few MB..

Yes, you can make it customizable.

>>> Also I need to control pstore driver like using bigger buffer,
>>> enabling specific message types and so on if ERST supports.  Is it
>>> possible for ERST to provide such information?
>>
>> It's the normal pstore driver, same as on a real server.  What exactly do you
>> need?
> 
> Well, I don't want to send additional pstore messages to the device if
> it cannot handle them properly - for example, ftrace message should not
> overwrite kmsg dump.  It'd be great if device somehow could expose
> acceptable message types to the driver IMHO.

This is something that you have to do in the usual kernel pstore
infrastructure.  It should not be specific to virtualization.

Paolo

> Btw I prefer using the kvmtool for my kernel work since it's much more
> simpler..

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web