thr3ads.net - Linux Virtualization - [PATCH 1/3] virtio: Basic implementation of virtio pstore driver [Nov 2016]

If this information is useful, please help other people find it:
Share via:

Michael S. Tsirkin

2016-Nov-15 05:06 UTC

[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 at redhat.com>
> > > Cc: Radim Kr?m?? <rkrcmar at redhat.com>
> > > Cc: "Michael S. Tsirkin" <mst at redhat.com>
> > > Cc: Anthony Liguori <aliguori at amazon.com>
> > > Cc: Anton Vorontsov <anton at enomsg.org>
> > > Cc: Colin Cross <ccross at android.com>
> > > Cc: Kees Cook <keescook at chromium.org>
> > > Cc: Tony Luck <tony.luck at intel.com>
> > > Cc: Steven Rostedt <rostedt at goodmis.org>
> > > Cc: Ingo Molnar <mingo at kernel.org>
> > > Cc: Minchan Kim <minchan at kernel.org>
> > > Cc: kvm at vger.kernel.org
> > > Cc: qemu-devel at nongnu.org
> > > Cc: virtualization at lists.linux-foundation.org
> > > Signed-off-by: Namhyung Kim <namhyung at 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

Namhyung Kim

2016-Nov-15 05:50 UTC

head link

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

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

Paolo Bonzini

2016-Nov-15 09:57 UTC

head link

[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 at redhat.com>
>>>> Cc: Radim Kr?m?? <rkrcmar at redhat.com>
>>>> Cc: "Michael S. Tsirkin" <mst at redhat.com>
>>>> Cc: Anthony Liguori <aliguori at amazon.com>
>>>> Cc: Anton Vorontsov <anton at enomsg.org>
>>>> Cc: Colin Cross <ccross at android.com>
>>>> Cc: Kees Cook <keescook at chromium.org>
>>>> Cc: Tony Luck <tony.luck at intel.com>
>>>> Cc: Steven Rostedt <rostedt at goodmis.org>
>>>> Cc: Ingo Molnar <mingo at kernel.org>
>>>> Cc: Minchan Kim <minchan at kernel.org>
>>>> Cc: kvm at vger.kernel.org
>>>> Cc: qemu-devel at nongnu.org
>>>> Cc: virtualization at lists.linux-foundation.org
>>>> Signed-off-by: Namhyung Kim <namhyung at 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 at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

Michael S. Tsirkin

2016-Nov-15 14:35 UTC

head link

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

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

Namhyung Kim

2016-Nov-15 14:36 UTC

head link

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

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

Apparently Analagous Threads

Search for more reasonably related threads

Linux Virtualization - Nov 2016 - [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

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

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

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

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

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

Apparently Analagous Threads