From: Namhyung Kim <namhyung at gmail.com>
Add virtio pstore device to allow kernel log files saved on the host.
It will save the log files on the directory given by pstore device
option.
$ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
(guest) # echo c > /proc/sysrq-trigger
$ ls dir-xx
dmesg-0.enc.z dmesg-1.enc.z
The log files are usually compressed using zlib. Users can see the log
messages directly on the host or on the guest (using pstore filesystem).
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 gmail.com>
---
hw/virtio/Makefile.objs | 2 +-
hw/virtio/virtio-pci.c | 50 ++++
hw/virtio/virtio-pci.h | 14 +
hw/virtio/virtio-pstore.c | 328 +++++++++++++++++++++
include/hw/pci/pci.h | 1 +
include/hw/virtio/virtio-pstore.h | 30 ++
include/standard-headers/linux/virtio_ids.h | 1 +
.../linux/{virtio_ids.h => virtio_pstore.h} | 48 +--
qdev-monitor.c | 1 +
9 files changed, 455 insertions(+), 20 deletions(-)
create mode 100644 hw/virtio/virtio-pstore.c
create mode 100644 include/hw/virtio/virtio-pstore.h
copy include/standard-headers/linux/{virtio_ids.h => virtio_pstore.h} (63%)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 3e2b175..aae7082 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
obj-y += virtio.o virtio-balloon.o
-obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
+obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 2b34b43..8281b80 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2416,6 +2416,55 @@ static const TypeInfo virtio_host_pci_info = {
};
#endif
+/* virtio-pstore-pci */
+
+static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+ VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
+ DeviceState *vdev = DEVICE(&vps->vdev);
+ Error *err = NULL;
+
+ qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
+ object_property_set_bool(OBJECT(vdev), true, "realized",
&err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+}
+
+static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+ PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+
+ k->realize = virtio_pstore_pci_realize;
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+
+ pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+ pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE;
+ pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
+ pcidev_k->class_id = PCI_CLASS_OTHERS;
+}
+
+static void virtio_pstore_pci_instance_init(Object *obj)
+{
+ VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj);
+
+ virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+ TYPE_VIRTIO_PSTORE);
+ object_property_add_alias(obj, "directory",
OBJECT(&dev->vdev),
+ "directory", &error_abort);
+}
+
+static const TypeInfo virtio_pstore_pci_info = {
+ .name = TYPE_VIRTIO_PSTORE_PCI,
+ .parent = TYPE_VIRTIO_PCI,
+ .instance_size = sizeof(VirtIOPstorePCI),
+ .instance_init = virtio_pstore_pci_instance_init,
+ .class_init = virtio_pstore_pci_class_init,
+};
+
/* virtio-pci-bus */
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
@@ -2485,6 +2534,7 @@ static void virtio_pci_register_types(void)
#ifdef CONFIG_VHOST_SCSI
type_register_static(&vhost_scsi_pci_info);
#endif
+ type_register_static(&virtio_pstore_pci_info);
}
type_init(virtio_pci_register_types)
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index e4548c2..b4c039f 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -31,6 +31,7 @@
#ifdef CONFIG_VHOST_SCSI
#include "hw/virtio/vhost-scsi.h"
#endif
+#include "hw/virtio/virtio-pstore.h"
typedef struct VirtIOPCIProxy VirtIOPCIProxy;
typedef struct VirtIOBlkPCI VirtIOBlkPCI;
@@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI;
typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
typedef struct VirtIOGPUPCI VirtIOGPUPCI;
+typedef struct VirtIOPstorePCI VirtIOPstorePCI;
/* virtio-pci-bus */
@@ -311,6 +313,18 @@ struct VirtIOGPUPCI {
VirtIOGPU vdev;
};
+/*
+ * virtio-pstore-pci: This extends VirtioPCIProxy.
+ */
+#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci"
+#define VIRTIO_PSTORE_PCI(obj) \
+ OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI)
+
+struct VirtIOPstorePCI {
+ VirtIOPCIProxy parent_obj;
+ VirtIOPstore vdev;
+};
+
/* Virtio ABI version, if we increment this, we break the guest driver. */
#define VIRTIO_PCI_ABI_VERSION 0
diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
new file mode 100644
index 0000000..98cee7f
--- /dev/null
+++ b/hw/virtio/virtio-pstore.c
@@ -0,0 +1,328 @@
+/*
+ * Virtio Pstore Device
+ *
+ * Copyright (C) 2016 LG Electronics
+ *
+ * Authors:
+ * Namhyung Kim <namhyung at gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include <stdio.h>
+
+#include "qemu/osdep.h"
+#include "qemu/iov.h"
+#include "qemu-common.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "sysemu/kvm.h"
+#include "qapi/visitor.h"
+#include "qapi-event.h"
+#include "trace.h"
+
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
+#include "hw/virtio/virtio-pstore.h"
+
+
+static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz,
+ struct virtio_pstore_hdr *hdr)
+{
+ const char *basename;
+
+ switch (hdr->type) {
+ case VIRTIO_PSTORE_TYPE_DMESG:
+ basename = "dmesg";
+ break;
+ default:
+ basename = "unknown";
+ break;
+ }
+
+ snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename,
+ (unsigned long long) hdr->id,
+ hdr->flags & VIRTIO_PSTORE_FL_COMPRESSED ?
".enc.z" : "");
+}
+
+static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
+ char *buf, size_t sz,
+ struct virtio_pstore_hdr *hdr)
+{
+ size_t len = strlen(name);
+
+ hdr->flags = 0;
+ if (!strncmp(name + len - 6, ".enc.z", 6)) {
+ hdr->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
+ }
+
+ snprintf(buf, sz, "%s/%s", s->directory, name);
+
+ if (!strncmp(name, "dmesg-", 6)) {
+ hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_DMESG);
+ name += 6;
+ } else if (!strncmp(name, "unknown-", 8)) {
+ hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN);
+ name += 8;
+ }
+
+ qemu_strtoull(name, NULL, 0, &hdr->id);
+}
+
+static ssize_t virtio_pstore_do_open(VirtIOPstore *s)
+{
+ s->dir = opendir(s->directory);
+ if (s->dir == NULL) {
+ return -1;
+ }
+
+ return 0;
+}
+
+static ssize_t virtio_pstore_do_read(VirtIOPstore *s, void *buf, size_t sz,
+ struct virtio_pstore_hdr *hdr)
+{
+ char path[PATH_MAX];
+ FILE *fp;
+ ssize_t len;
+ struct stat stbuf;
+ struct dirent *dent;
+
+ if (s->dir == NULL) {
+ return -1;
+ }
+
+ dent = readdir(s->dir);
+ while (dent) {
+ if (dent->d_name[0] != '.') {
+ break;
+ }
+ dent = readdir(s->dir);
+ }
+
+ if (dent == NULL) {
+ return 0;
+ }
+
+ virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), hdr);
+ if (stat(path, &stbuf) < 0) {
+ return -1;
+ }
+
+ fp = fopen(path, "r");
+ if (fp == NULL) {
+ error_report("cannot open %s (%p %p)", path, s,
s->directory);
+ return -1;
+ }
+
+ len = fread(buf, 1, sz, fp);
+ if (len < 0 && errno == EAGAIN) {
+ len = 0;
+ }
+
+ hdr->id = cpu_to_le64(hdr->id);
+ hdr->flags = cpu_to_le32(hdr->flags);
+ hdr->time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec);
+ hdr->time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
+
+ fclose(fp);
+ return len;
+}
+
+static ssize_t virtio_pstore_do_write(VirtIOPstore *s, void *buf, size_t sz,
+ struct virtio_pstore_hdr *hdr)
+{
+ char path[PATH_MAX];
+ FILE *fp;
+
+ virtio_pstore_to_filename(s, path, sizeof(path), hdr);
+
+ fp = fopen(path, "w");
+ if (fp == NULL) {
+ error_report("cannot open %s (%p %p)", path, s,
s->directory);
+ return -1;
+ }
+ fwrite(buf, 1, sz, fp);
+ fclose(fp);
+
+ return sz;
+}
+
+static ssize_t virtio_pstore_do_close(VirtIOPstore *s)
+{
+ if (s->dir == NULL) {
+ return 0;
+ }
+
+ closedir(s->dir);
+ s->dir = NULL;
+
+ return 0;
+}
+
+static ssize_t virtio_pstore_do_erase(VirtIOPstore *s,
+ struct virtio_pstore_hdr *hdr)
+{
+ char path[PATH_MAX];
+
+ virtio_pstore_to_filename(s, path, sizeof(path), hdr);
+
+ return unlink(path);
+}
+
+static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
+{
+ VirtIOPstore *s = VIRTIO_PSTORE(vdev);
+ VirtQueueElement *elem;
+ struct virtio_pstore_hdr *hdr;
+ ssize_t len;
+
+ for (;;) {
+ elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+ if (!elem) {
+ return;
+ }
+
+ hdr = elem->out_sg[0].iov_base;
+ if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
+ error_report("invalid header size: %u",
+ (unsigned)elem->out_sg[0].iov_len);
+ exit(1);
+ }
+
+ switch (hdr->cmd) {
+ case VIRTIO_PSTORE_CMD_OPEN:
+ len = virtio_pstore_do_open(s);
+ break;
+ case VIRTIO_PSTORE_CMD_READ:
+ len = virtio_pstore_do_read(s, elem->in_sg[0].iov_base,
+ elem->in_sg[0].iov_len, hdr);
+ break;
+ case VIRTIO_PSTORE_CMD_WRITE:
+ len = virtio_pstore_do_write(s, elem->out_sg[1].iov_base,
+ elem->out_sg[1].iov_len, hdr);
+ break;
+ case VIRTIO_PSTORE_CMD_CLOSE:
+ len = virtio_pstore_do_close(s);
+ break;
+ case VIRTIO_PSTORE_CMD_ERASE:
+ len = virtio_pstore_do_erase(s, hdr);
+ break;
+ default:
+ len = -1;
+ break;
+ }
+
+ if (len < 0) {
+ return;
+ }
+
+ virtqueue_push(vq, elem, len);
+
+ virtio_notify(vdev, vq);
+ g_free(elem);
+ }
+}
+
+static void virtio_pstore_device_realize(DeviceState *dev, Error **errp)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+ VirtIOPstore *s = VIRTIO_PSTORE(dev);
+
+ virtio_init(vdev, "virtio-pstore", VIRTIO_ID_PSTORE, 0);
+
+ s->vq = virtio_add_queue(vdev, 128, virtio_pstore_handle_io);
+}
+
+static void virtio_pstore_device_unrealize(DeviceState *dev, Error **errp)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+ virtio_cleanup(vdev);
+}
+
+static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
+{
+ return f;
+}
+
+static void pstore_get_directory(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ VirtIOPstore *s = opaque;
+
+ visit_type_str(v, name, &s->directory, errp);
+}
+
+static void pstore_set_directory(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ VirtIOPstore *s = opaque;
+ Error *local_err = NULL;
+ char *value;
+
+ visit_type_str(v, name, &value, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ g_free(s->directory);
+ s->directory = strdup(value);
+
+ g_free(value);
+}
+
+static void pstore_release_directory(Object *obj, const char *name,
+ void *opaque)
+{
+ VirtIOPstore *s = opaque;
+
+ g_free(s->directory);
+ s->directory = NULL;
+}
+
+static Property virtio_pstore_properties[] = {
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_pstore_instance_init(Object *obj)
+{
+ VirtIOPstore *s = VIRTIO_PSTORE(obj);
+
+ object_property_add(obj, "directory", "str",
+ pstore_get_directory, pstore_set_directory,
+ pstore_release_directory, s, NULL);
+}
+
+static void virtio_pstore_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+ dc->props = virtio_pstore_properties;
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+ vdc->realize = virtio_pstore_device_realize;
+ vdc->unrealize = virtio_pstore_device_unrealize;
+ vdc->get_features = get_features;
+}
+
+static const TypeInfo virtio_pstore_info = {
+ .name = TYPE_VIRTIO_PSTORE,
+ .parent = TYPE_VIRTIO_DEVICE,
+ .instance_size = sizeof(VirtIOPstore),
+ .instance_init = virtio_pstore_instance_init,
+ .class_init = virtio_pstore_class_init,
+};
+
+static void virtio_register_types(void)
+{
+ type_register_static(&virtio_pstore_info);
+}
+
+type_init(virtio_register_types)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 9ed1624..5689c6f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -79,6 +79,7 @@
#define PCI_DEVICE_ID_VIRTIO_SCSI 0x1004
#define PCI_DEVICE_ID_VIRTIO_RNG 0x1005
#define PCI_DEVICE_ID_VIRTIO_9P 0x1009
+#define PCI_DEVICE_ID_VIRTIO_PSTORE 0x100a
#define PCI_VENDOR_ID_REDHAT 0x1b36
#define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001
diff --git a/include/hw/virtio/virtio-pstore.h
b/include/hw/virtio/virtio-pstore.h
new file mode 100644
index 0000000..74cd1f6
--- /dev/null
+++ b/include/hw/virtio/virtio-pstore.h
@@ -0,0 +1,30 @@
+/*
+ * Virtio Pstore Support
+ *
+ * Authors:
+ * Namhyung Kim <namhyung at gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef _QEMU_VIRTIO_PSTORE_H
+#define _QEMU_VIRTIO_PSTORE_H
+
+#include "standard-headers/linux/virtio_pstore.h"
+#include "hw/virtio/virtio.h"
+#include "hw/pci/pci.h"
+
+#define TYPE_VIRTIO_PSTORE "virtio-pstore-device"
+#define VIRTIO_PSTORE(obj) \
+ OBJECT_CHECK(VirtIOPstore, (obj), TYPE_VIRTIO_PSTORE)
+
+typedef struct VirtIOPstore {
+ VirtIODevice parent_obj;
+ VirtQueue *vq;
+ char *directory;
+ DIR *dir;
+} VirtIOPstore;
+
+#endif
diff --git a/include/standard-headers/linux/virtio_ids.h
b/include/standard-headers/linux/virtio_ids.h
index 77925f5..cba6322 100644
--- a/include/standard-headers/linux/virtio_ids.h
+++ b/include/standard-headers/linux/virtio_ids.h
@@ -41,5 +41,6 @@
#define VIRTIO_ID_CAIF 12 /* Virtio caif */
#define VIRTIO_ID_GPU 16 /* virtio GPU */
#define VIRTIO_ID_INPUT 18 /* virtio input */
+#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
#endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/include/standard-headers/linux/virtio_ids.h
b/include/standard-headers/linux/virtio_pstore.h
similarity index 63%
copy from include/standard-headers/linux/virtio_ids.h
copy to include/standard-headers/linux/virtio_pstore.h
index 77925f5..1b89cad 100644
--- a/include/standard-headers/linux/virtio_ids.h
+++ b/include/standard-headers/linux/virtio_pstore.h
@@ -1,9 +1,6 @@
-#ifndef _LINUX_VIRTIO_IDS_H
-#define _LINUX_VIRTIO_IDS_H
-/*
- * Virtio IDs
- *
- * This header is BSD licensed so anyone can use the definitions to implement
+#ifndef _LINUX_VIRTIO_PSTORE_H
+#define _LINUX_VIRTIO_PSTORE_H
+/* This header is BSD licensed so anyone can use the definitions to implement
* compatible drivers/servers.
*
* Redistribution and use in source and binary forms, with or without
@@ -28,18 +25,31 @@
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE. */
+#include "standard-headers/linux/types.h"
+#include "standard-headers/linux/virtio_types.h"
+#include "standard-headers/linux/virtio_ids.h"
+#include "standard-headers/linux/virtio_config.h"
+
+#define VIRTIO_PSTORE_TYPE_UNKNOWN 0
+#define VIRTIO_PSTORE_TYPE_DMESG 1
+
+#define VIRTIO_PSTORE_CMD_NULL 0
+#define VIRTIO_PSTORE_CMD_OPEN 1
+#define VIRTIO_PSTORE_CMD_READ 2
+#define VIRTIO_PSTORE_CMD_WRITE 3
+#define VIRTIO_PSTORE_CMD_ERASE 4
+#define VIRTIO_PSTORE_CMD_CLOSE 5
+
+#define VIRTIO_PSTORE_FL_COMPRESSED 1
-#define VIRTIO_ID_NET 1 /* virtio net */
-#define VIRTIO_ID_BLOCK 2 /* virtio block */
-#define VIRTIO_ID_CONSOLE 3 /* virtio console */
-#define VIRTIO_ID_RNG 4 /* virtio rng */
-#define VIRTIO_ID_BALLOON 5 /* virtio balloon */
-#define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
-#define VIRTIO_ID_SCSI 8 /* virtio scsi */
-#define VIRTIO_ID_9P 9 /* 9p virtio console */
-#define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
-#define VIRTIO_ID_CAIF 12 /* Virtio caif */
-#define VIRTIO_ID_GPU 16 /* virtio GPU */
-#define VIRTIO_ID_INPUT 18 /* virtio input */
+struct virtio_pstore_hdr {
+ __virtio64 id;
+ __virtio32 flags;
+ __virtio16 cmd;
+ __virtio16 type;
+ __virtio64 time_sec;
+ __virtio32 time_nsec;
+ __virtio32 unused;
+};
-#endif /* _LINUX_VIRTIO_IDS_H */
+#endif /* _LINUX_VIRTIO_PSTORE_H */
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e19617f..e1df5a9 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -73,6 +73,7 @@ static const QDevAlias qdev_alias_table[] = {
{ "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL
& ~QEMU_ARCH_S390X },
{ "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X
},
{ "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL
& ~QEMU_ARCH_S390X },
+ { "virtio-pstore-pci", "virtio-pstore" },
{ }
};
--
2.8.0
Hello, On Mon, Jul 18, 2016 at 09:28:42AM +0200, Christian Borntraeger wrote:> On 07/18/2016 06:37 AM, Namhyung Kim wrote: > > Can you do the virtio-mmio and virtio-ccw plumbing as well, or > do you need help with that?Any help would be greatly appreciated! Thanks, Namhyung> > [...] > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 2b34b43..8281b80 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -2416,6 +2416,55 @@ static const TypeInfo virtio_host_pci_info = { > > }; > > #endif > > > > +/* virtio-pstore-pci */ > > + > > +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) > > +{ > > + VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev); > > + DeviceState *vdev = DEVICE(&vps->vdev); > > + Error *err = NULL; > > + > > + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); > > + object_property_set_bool(OBJECT(vdev), true, "realized", &err); > > + if (err) { > > + error_propagate(errp, err); > > + return; > > + } > > +} > > + > > +static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass); > > + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass); > > + > > + k->realize = virtio_pstore_pci_realize; > > + set_bit(DEVICE_CATEGORY_MISC, dc->categories); > > + > > + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET; > > + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE; > > + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION; > > + pcidev_k->class_id = PCI_CLASS_OTHERS; > > +} > > + > > +static void virtio_pstore_pci_instance_init(Object *obj) > > +{ > > + VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj); > > + > > + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev), > > + TYPE_VIRTIO_PSTORE); > > + object_property_add_alias(obj, "directory", OBJECT(&dev->vdev), > > + "directory", &error_abort); > > +} > > + > > +static const TypeInfo virtio_pstore_pci_info = { > > + .name = TYPE_VIRTIO_PSTORE_PCI, > > + .parent = TYPE_VIRTIO_PCI, > > + .instance_size = sizeof(VirtIOPstorePCI), > > + .instance_init = virtio_pstore_pci_instance_init, > > + .class_init = virtio_pstore_pci_class_init, > > +}; > > + > > /* virtio-pci-bus */ > > > > static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size, > > @@ -2485,6 +2534,7 @@ static void virtio_pci_register_types(void) > > #ifdef CONFIG_VHOST_SCSI > > type_register_static(&vhost_scsi_pci_info); > > #endif > > + type_register_static(&virtio_pstore_pci_info); > > } > > > > type_init(virtio_pci_register_types) > > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h > > index e4548c2..b4c039f 100644 > > --- a/hw/virtio/virtio-pci.h > > +++ b/hw/virtio/virtio-pci.h > > @@ -31,6 +31,7 @@ > > #ifdef CONFIG_VHOST_SCSI > > #include "hw/virtio/vhost-scsi.h" > > #endif > > +#include "hw/virtio/virtio-pstore.h" > > > > typedef struct VirtIOPCIProxy VirtIOPCIProxy; > > typedef struct VirtIOBlkPCI VirtIOBlkPCI; > > @@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI; > > typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI; > > typedef struct VirtIOInputHostPCI VirtIOInputHostPCI; > > typedef struct VirtIOGPUPCI VirtIOGPUPCI; > > +typedef struct VirtIOPstorePCI VirtIOPstorePCI; > > > > /* virtio-pci-bus */ > > > > @@ -311,6 +313,18 @@ struct VirtIOGPUPCI { > > VirtIOGPU vdev; > > }; > > > > +/* > > + * virtio-pstore-pci: This extends VirtioPCIProxy. > > + */ > > +#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci" > > +#define VIRTIO_PSTORE_PCI(obj) \ > > + OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI) > > + > > +struct VirtIOPstorePCI { > > + VirtIOPCIProxy parent_obj; > > + VirtIOPstore vdev; > > +}; > > + > > /* Virtio ABI version, if we increment this, we break the guest driver. */ > > #define VIRTIO_PCI_ABI_VERSION 0 > > > > [...] >
Hello, On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote:> On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote: > > From: Namhyung Kim <namhyung at gmail.com> > > > > Add virtio pstore device to allow kernel log files saved on the host. > > It will save the log files on the directory given by pstore device > > option. > > > > $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ... > > > > (guest) # echo c > /proc/sysrq-trigger > > > > $ ls dir-xx > > dmesg-0.enc.z dmesg-1.enc.z > > > > The log files are usually compressed using zlib. Users can see the log > > messages directly on the host or on the guest (using pstore filesystem). > > The implementation is synchronous (i.e. can pause guest code execution), > does not handle write errors, and does not limit the amount of data the > guest can write. This is sufficient for ad-hoc debugging and usage with > trusted guests. > > If you want this to be available in environments where the guest isn't > trusted then there must be a limit on how much the guest can write or > some kind of log rotation.Right. The synchronous IO is required by the pstore subsystem implementation AFAIK (it uses a single psinfo->buf in the loop). And I agree that it should have a way to handle write errors and to limit amount of data.> > > > > 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 gmail.com> > > ---[SNIP]> > + > > +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz, > > + struct virtio_pstore_hdr *hdr) > > +{ > > + const char *basename; > > + > > + switch (hdr->type) { > > Missing le16_to_cpu()? > > > + case VIRTIO_PSTORE_TYPE_DMESG: > > + basename = "dmesg"; > > + break; > > + default: > > + basename = "unknown"; > > + break; > > + } > > + > > + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, > > + (unsigned long long) hdr->id, > > Missing le64_to_cpu()? > > > + hdr->flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : ""); > > Missing le32_to_cpu()?Oops, will fix.> > > +} > > + > > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name, > > + char *buf, size_t sz, > > + struct virtio_pstore_hdr *hdr) > > +{ > > + size_t len = strlen(name); > > + > > + hdr->flags = 0; > > + if (!strncmp(name + len - 6, ".enc.z", 6)) { > > Please use g_str_has_suffix(name, ".enc.z") to avoid accessing before > the beginning of the string if the filename is shorter than 6 > characters.Ah, ok.> > > + hdr->flags |= VIRTIO_PSTORE_FL_COMPRESSED; > > + } > > + > > + snprintf(buf, sz, "%s/%s", s->directory, name); > > + > > + if (!strncmp(name, "dmesg-", 6)) { > > g_str_has_prefix(name, "dmesg-") > > > + hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_DMESG); > > + name += 6; > > + } else if (!strncmp(name, "unknown-", 8)) { > > g_str_has_prefix(name, "unknown-")Will change.> > > + hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN); > > + name += 8; > > + } > > + > > + qemu_strtoull(name, NULL, 0, &hdr->id); > > +} > > + > > +static ssize_t virtio_pstore_do_open(VirtIOPstore *s) > > +{ > > + s->dir = opendir(s->directory); > > + if (s->dir == NULL) { > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, void *buf, size_t sz, > > + struct virtio_pstore_hdr *hdr) > > +{ > > + char path[PATH_MAX]; > > + FILE *fp; > > + ssize_t len; > > + struct stat stbuf; > > + struct dirent *dent; > > + > > + if (s->dir == NULL) { > > + return -1; > > + } > > + > > + dent = readdir(s->dir); > > + while (dent) { > > + if (dent->d_name[0] != '.') { > > + break; > > + } > > + dent = readdir(s->dir); > > + } > > + > > + if (dent == NULL) { > > + return 0; > > + } > > + > > + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), hdr); > > + if (stat(path, &stbuf) < 0) { > > + return -1; > > + } > > Please use fstat(fileno(fp), &stbuf) after opening the file instead. > The race condition doesn't matter in this case but the race-free code is > just as simple so it's one less thing someone reading the code has to > worry about.Fair enough.> > > + > > + fp = fopen(path, "r"); > > + if (fp == NULL) { > > + error_report("cannot open %s (%p %p)", path, s, s->directory); > > + return -1; > > + } > > + > > + len = fread(buf, 1, sz, fp); > > + if (len < 0 && errno == EAGAIN) { > > + len = 0; > > + } > > + > > + hdr->id = cpu_to_le64(hdr->id); > > + hdr->flags = cpu_to_le32(hdr->flags); > > + hdr->time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec); > > + hdr->time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec); > > + > > + fclose(fp); > > + return len; > > +} > > +[SNIP]> > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq) > > +{ > > + VirtIOPstore *s = VIRTIO_PSTORE(vdev); > > + VirtQueueElement *elem; > > + struct virtio_pstore_hdr *hdr; > > + ssize_t len; > > + > > + for (;;) { > > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); > > + if (!elem) { > > + return; > > + } > > + > > + hdr = elem->out_sg[0].iov_base; > > + if (elem->out_sg[0].iov_len != sizeof(*hdr)) { > > + error_report("invalid header size: %u", > > + (unsigned)elem->out_sg[0].iov_len); > > + exit(1); > > + } > > Please use iov_to_buf() instead of directly accessing out_sg[]. Virtio > devices are not supposed to assume a particular iovec layout. In other > words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs.I got it.> > You must also copy in data (similar to Linux syscall implementations) to > prevent the guest from modifying data while the command is processed. > Such race conditions could lead to security bugs.Ok, but this assumes the operation is synchronous. I agree on your opinion if I could make it async.> > > + > > + switch (hdr->cmd) { > > + case VIRTIO_PSTORE_CMD_OPEN: > > + len = virtio_pstore_do_open(s); > > + break; > > + case VIRTIO_PSTORE_CMD_READ: > > + len = virtio_pstore_do_read(s, elem->in_sg[0].iov_base, > > + elem->in_sg[0].iov_len, hdr); > > Same issue with iovec layout for in_sg[] here. The guest driver must be > able to submit any in_sg[] iovec array and the device cannot assume > in_sg[0] is the only iovec to fill.Ok.> > > + break; > > + case VIRTIO_PSTORE_CMD_WRITE: > > + len = virtio_pstore_do_write(s, elem->out_sg[1].iov_base, > > + elem->out_sg[1].iov_len, hdr); > > + break; > > + case VIRTIO_PSTORE_CMD_CLOSE: > > + len = virtio_pstore_do_close(s); > > + break; > > + case VIRTIO_PSTORE_CMD_ERASE: > > + len = virtio_pstore_do_erase(s, hdr); > > + break; > > + default: > > + len = -1; > > + break; > > + } > > + > > + if (len < 0) { > > + return; > > + } > > + > > + virtqueue_push(vq, elem, len); > > + > > + virtio_notify(vdev, vq); > > + g_free(elem); > > + } > > +} > > + > > +static void virtio_pstore_device_realize(DeviceState *dev, Error **errp) > > +{ > > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > + VirtIOPstore *s = VIRTIO_PSTORE(dev); > > + > > + virtio_init(vdev, "virtio-pstore", VIRTIO_ID_PSTORE, 0); > > + > > + s->vq = virtio_add_queue(vdev, 128, virtio_pstore_handle_io); > > +} > > + > > +static void virtio_pstore_device_unrealize(DeviceState *dev, Error **errp) > > +{ > > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > + > > + virtio_cleanup(vdev); > > +} > > + > > +static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp) > > +{ > > + return f; > > +} > > + > > +static void pstore_get_directory(Object *obj, Visitor *v, > > + const char *name, void *opaque, > > + Error **errp) > > +{ > > + VirtIOPstore *s = opaque; > > + > > + visit_type_str(v, name, &s->directory, errp); > > +} > > + > > +static void pstore_set_directory(Object *obj, Visitor *v, > > + const char *name, void *opaque, > > + Error **errp) > > +{ > > + VirtIOPstore *s = opaque; > > + Error *local_err = NULL; > > + char *value; > > + > > + visit_type_str(v, name, &value, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + g_free(s->directory); > > + s->directory = strdup(value); > > Please use g_strdup() since this is paired with g_free(). > > Or even simpler would be s->directory = value and do not g_free(value) > below.Ok, I was not sure whether I could use it without alloc/free pair. Will do it simpler way then. :)> > > + > > + g_free(value); > > +} > > + > > +static void pstore_release_directory(Object *obj, const char *name, > > + void *opaque) > > +{ > > + VirtIOPstore *s = opaque; > > + > > + g_free(s->directory); > > + s->directory = NULL; > > +} > > + > > +static Property virtio_pstore_properties[] = { > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > +static void virtio_pstore_instance_init(Object *obj) > > +{ > > + VirtIOPstore *s = VIRTIO_PSTORE(obj); > > + > > + object_property_add(obj, "directory", "str", > > + pstore_get_directory, pstore_set_directory, > > + pstore_release_directory, s, NULL); > > +} > > + > > +static void virtio_pstore_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass); > > + > > + dc->props = virtio_pstore_properties; > > + set_bit(DEVICE_CATEGORY_MISC, dc->categories); > > + vdc->realize = virtio_pstore_device_realize; > > + vdc->unrealize = virtio_pstore_device_unrealize; > > + vdc->get_features = get_features; > > +} > > + > > +static const TypeInfo virtio_pstore_info = { > > + .name = TYPE_VIRTIO_PSTORE, > > + .parent = TYPE_VIRTIO_DEVICE, > > + .instance_size = sizeof(VirtIOPstore), > > + .instance_init = virtio_pstore_instance_init, > > + .class_init = virtio_pstore_class_init, > > +}; > > + > > +static void virtio_register_types(void) > > +{ > > + type_register_static(&virtio_pstore_info); > > +} > > + > > +type_init(virtio_register_types) > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > > index 9ed1624..5689c6f 100644 > > --- a/include/hw/pci/pci.h > > +++ b/include/hw/pci/pci.h > > @@ -79,6 +79,7 @@ > > #define PCI_DEVICE_ID_VIRTIO_SCSI 0x1004 > > #define PCI_DEVICE_ID_VIRTIO_RNG 0x1005 > > #define PCI_DEVICE_ID_VIRTIO_9P 0x1009 > > +#define PCI_DEVICE_ID_VIRTIO_PSTORE 0x100a > > > > #define PCI_VENDOR_ID_REDHAT 0x1b36 > > #define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001 > > diff --git a/include/hw/virtio/virtio-pstore.h b/include/hw/virtio/virtio-pstore.h > > new file mode 100644 > > index 0000000..74cd1f6 > > --- /dev/null > > +++ b/include/hw/virtio/virtio-pstore.h > > @@ -0,0 +1,30 @@ > > +/* > > + * Virtio Pstore Support > > + * > > + * Authors: > > + * Namhyung Kim <namhyung at gmail.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2. See > > + * the COPYING file in the top-level directory. > > + * > > + */ > > + > > +#ifndef _QEMU_VIRTIO_PSTORE_H > > +#define _QEMU_VIRTIO_PSTORE_H > > + > > +#include "standard-headers/linux/virtio_pstore.h" > > +#include "hw/virtio/virtio.h" > > +#include "hw/pci/pci.h" > > + > > +#define TYPE_VIRTIO_PSTORE "virtio-pstore-device" > > +#define VIRTIO_PSTORE(obj) \ > > + OBJECT_CHECK(VirtIOPstore, (obj), TYPE_VIRTIO_PSTORE) > > + > > +typedef struct VirtIOPstore { > > + VirtIODevice parent_obj; > > + VirtQueue *vq; > > + char *directory; > > + DIR *dir; > > +} VirtIOPstore; > > + > > +#endif > > diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h > > index 77925f5..cba6322 100644 > > --- a/include/standard-headers/linux/virtio_ids.h > > +++ b/include/standard-headers/linux/virtio_ids.h > > @@ -41,5 +41,6 @@ > > #define VIRTIO_ID_CAIF 12 /* Virtio caif */ > > #define VIRTIO_ID_GPU 16 /* virtio GPU */ > > #define VIRTIO_ID_INPUT 18 /* virtio input */ > > +#define VIRTIO_ID_PSTORE 19 /* virtio pstore */ > > 19 has already been reserved. 22 is the next free ID (vsock, crypto, > and sdm are currently under review and already use 19, 20, and 21).I wasn't aware of the ongoing works but Cornelia already told me about it. Will update.> > Please send a VIRTIO draft specification to > virtio-dev at lists.oasis-open.org. You can find information on the VIRTIO > standards process here: > https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtioThank you very much for this information and your detailed review! I'll take a look at the virtio standards process too. Thanks, Namhyung
Hello, On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote:> On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote: > > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq) > > +{ > > + VirtIOPstore *s = VIRTIO_PSTORE(vdev); > > + VirtQueueElement *elem; > > + struct virtio_pstore_hdr *hdr; > > + ssize_t len; > > + > > + for (;;) { > > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); > > + if (!elem) { > > + return; > > + } > > + > > + hdr = elem->out_sg[0].iov_base; > > + if (elem->out_sg[0].iov_len != sizeof(*hdr)) { > > + error_report("invalid header size: %u", > > + (unsigned)elem->out_sg[0].iov_len); > > + exit(1); > > + } > > Please use iov_to_buf() instead of directly accessing out_sg[]. Virtio > devices are not supposed to assume a particular iovec layout. In other > words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs. > > You must also copy in data (similar to Linux syscall implementations) to > prevent the guest from modifying data while the command is processed. > Such race conditions could lead to security bugs.By accessing elem->out_sg[0].iov_base directly, I abused it as an in-and-out buffer. But it seems not allowed by the virtio spec, do I have to use separate buffers for request and response? Thanks, Namhyung