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
[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
[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
[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, > NamhyungIf 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
[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
Maybe Matching Threads
- [PATCH 6/7] qemu: Implement virtio-pstore device
- [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