| Message ID | 1469632111-23260-7-git-send-email-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > 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 So if the point is handling system crashes, I suspect virtio is the wrong protocol to use. ATM it's rather elaborate for performance, it's likely not to work when linux is crashing. I think you want something very very simple that will still work when you crash. Like maybe a serial device? > $ ls dir-xx > dmesg-1.enc.z dmesg-2.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). So this lacks all management tools that a regular storage device has, and these are necessary if people are to use this in production. For example, some kind of provision for limiting the amount of host disk this can consume seems called for. Rate-limiting disk writes on host also seems necessary. Handling host disk errors always propagates error to guest but an option to e.g. stop vm might be useful to avoid corruption. > > The 'directory' property is required for virtio-pstore device to work. > It also adds 'bufsize' and 'console' (boolean) properties. No idea what these do. Seem to be RW by guest but have no effect otherwise. > 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> > --- > hw/virtio/Makefile.objs | 2 +- > hw/virtio/virtio-pci.c | 54 +++ > hw/virtio/virtio-pci.h | 14 + > hw/virtio/virtio-pstore.c | 477 +++++++++++++++++++++++++ > include/hw/pci/pci.h | 1 + > include/hw/virtio/virtio-pstore.h | 34 ++ > include/standard-headers/linux/virtio_ids.h | 1 + > include/standard-headers/linux/virtio_pstore.h | 80 +++++ > qdev-monitor.c | 1 + > 9 files changed, 663 insertions(+), 1 deletion(-) > create mode 100644 hw/virtio/virtio-pstore.c > create mode 100644 include/hw/virtio/virtio-pstore.h > create mode 100644 include/standard-headers/linux/virtio_pstore.h > > 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 f0677b7..d99a405 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2414,6 +2414,59 @@ 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); > + object_property_add_alias(obj, "bufsize", OBJECT(&dev->vdev), > + "bufsize", &error_abort); > + object_property_add_alias(obj, "console", OBJECT(&dev->vdev), > + "console", &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, > @@ -2483,6 +2536,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..2ca7786 > --- /dev/null > +++ b/hw/virtio/virtio-pstore.c > @@ -0,0 +1,477 @@ > +/* > + * Virtio Pstore Device > + * > + * Copyright (C) 2016 LG Electronics > + * > + * Authors: > + * Namhyung Kim <namhyung@gmail.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. We generally ask new code to be v2 or later. > 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_req *req) > +{ > + const char *basename; > + unsigned long long id = 0; > + unsigned int flags = le32_to_cpu(req->flags); > + > + switch (le16_to_cpu(req->type)) { > + case VIRTIO_PSTORE_TYPE_DMESG: > + basename = "dmesg"; > + id = s->id++; > + break; > + case VIRTIO_PSTORE_TYPE_CONSOLE: > + basename = "console"; > + if (s->console_id) { > + id = s->console_id; > + } else { > + id = s->console_id = s->id++; > + } > + break; > + default: > + basename = "unknown"; > + break; > + } > + > + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id, > + 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_fileinfo *info) > +{ > + snprintf(buf, sz, "%s/%s", s->directory, name); if this does not fit, buf will not be 0 terminated and can generally be corrupted. > + > + if (g_str_has_prefix(name, "dmesg-")) { > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > + name += strlen("dmesg-"); > + } else if (g_str_has_prefix(name, "console-")) { > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > + name += strlen("console-"); > + } else if (g_str_has_prefix(name, "unknown-")) { > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > + name += strlen("unknown-"); > + } > + > + qemu_strtoull(name, NULL, 0, &info->id); > + > + info->flags = 0; > + if (g_str_has_suffix(name, ".enc.z")) { > + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED; > + } > +} > + > +static ssize_t virtio_pstore_do_open(VirtIOPstore *s) > +{ > + s->dirp = opendir(s->directory); > + if (s->dirp == NULL) { > + return -1; > + } > + > + return 0; > +} > + > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, struct iovec *in_sg, > + unsigned int in_num, > + struct virtio_pstore_res *res) > +{ > + char path[PATH_MAX]; > + int fd; > + ssize_t len; > + struct stat stbuf; > + struct dirent *dent; > + int sg_num = in_num; > + struct iovec sg[sg_num]; > + struct virtio_pstore_fileinfo info; > + size_t offset = sizeof(*res) + sizeof(info); > + > + if (s->dirp == NULL) { > + return -1; > + } > + > + dent = readdir(s->dirp); > + while (dent) { > + if (dent->d_name[0] != '.') { > + break; > + } > + dent = readdir(s->dirp); > + } > + > + if (dent == NULL) { > + return 0; > + } > + > + /* skip res and fileinfo */ > + sg_num = iov_copy(sg, sg_num, in_sg, in_num, offset, > + iov_size(in_sg, in_num) - offset); > + > + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), &info); > + fd = open(path, O_RDONLY); > + if (fd < 0) { > + error_report("cannot open %s", path); > + return -1; > + } > + > + if (fstat(fd, &stbuf) < 0) { > + len = -1; > + goto out; > + } > + > + len = readv(fd, sg, sg_num); > + if (len < 0) { > + if (errno == EAGAIN) { > + len = 0; > + } > + goto out; > + } > + > + info.id = cpu_to_le64(info.id); > + info.type = cpu_to_le16(info.type); > + info.flags = cpu_to_le32(info.flags); > + info.len = cpu_to_le32(len); > + info.time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec); > + info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec); > + > + iov_from_buf(in_sg, in_num, sizeof(*res), &info, sizeof(info)); > + len += sizeof(info); > + > + out: > + close(fd); > + return len; > +} > + > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > + unsigned int out_num, > + struct virtio_pstore_req *req) > +{ > + char path[PATH_MAX]; > + int fd; > + ssize_t len; > + unsigned short type; > + int flags = O_WRONLY | O_CREAT; > + > + /* we already consume the req */ > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > + > + virtio_pstore_to_filename(s, path, sizeof(path), req); > + > + type = le16_to_cpu(req->type); > + > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > + flags |= O_TRUNC; > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > + flags |= O_APPEND; > + } > + > + fd = open(path, flags, 0644); > + if (fd < 0) { > + error_report("cannot open %s", path); > + return -1; > + } > + len = writev(fd, out_sg, out_num); > + close(fd); > + > + return len; All this is blocking VM until host io completes. > +} > + > +static ssize_t virtio_pstore_do_close(VirtIOPstore *s) > +{ > + if (s->dirp == NULL) { > + return 0; > + } > + > + closedir(s->dirp); > + s->dirp = NULL; > + > + return 0; > +} > + > +static ssize_t virtio_pstore_do_erase(VirtIOPstore *s, > + struct virtio_pstore_req *req) > +{ > + char path[PATH_MAX]; > + > + virtio_pstore_to_filename(s, path, sizeof(path), req); > + > + return unlink(path); > +} > + > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq) > +{ > + VirtIOPstore *s = VIRTIO_PSTORE(vdev); > + VirtQueueElement *elem; > + struct virtio_pstore_req req; > + struct virtio_pstore_res res; > + ssize_t len = 0; > + int ret; > + > + for (;;) { > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); > + if (!elem) { > + return; > + } > + > + if (elem->out_num < 1 || elem->in_num < 1) { > + error_report("request or response buffer is missing"); > + exit(1); > + } > + > + len = iov_to_buf(elem->out_sg, elem->out_num, 0, &req, sizeof(req)); > + if (len != (ssize_t)sizeof(req)) { > + error_report("invalid request size: %ld", (long)len); > + exit(1); > + } > + res.cmd = req.cmd; > + res.type = req.type; > + > + switch (le16_to_cpu(req.cmd)) { > + case VIRTIO_PSTORE_CMD_OPEN: > + ret = virtio_pstore_do_open(s); > + break; > + case VIRTIO_PSTORE_CMD_READ: > + ret = virtio_pstore_do_read(s, elem->in_sg, elem->in_num, &res); > + if (ret > 0) { > + len = ret; > + ret = 0; > + } > + break; > + case VIRTIO_PSTORE_CMD_WRITE: > + ret = virtio_pstore_do_write(s, elem->out_sg, elem->out_num, &req); > + break; > + case VIRTIO_PSTORE_CMD_CLOSE: > + ret = virtio_pstore_do_close(s); > + break; > + case VIRTIO_PSTORE_CMD_ERASE: > + ret = virtio_pstore_do_erase(s, &req); > + break; > + default: > + ret = -1; > + break; > + } > + > + res.ret = ret; > + > + iov_from_buf(elem->in_sg, elem->in_num, 0, &res, sizeof(res)); > + virtqueue_push(vq, elem, sizeof(res) + len); this is wrong - len should be # of bytes written into guest memory. > + > + virtio_notify(vdev, vq); > + g_free(elem); > + > + if (ret < 0) { > + return; > + } > + } > +} > + > +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, > + sizeof(struct virtio_pstore_config)); > + > + s->id = 1; > + s->console_id = 0; > + > + s->vq[0] = virtio_add_queue(vdev, 128, virtio_pstore_handle_io); > + s->vq[1] = 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 void virtio_pstore_get_config(VirtIODevice *vdev, uint8_t *config_data) > +{ > + VirtIOPstore *dev = VIRTIO_PSTORE(vdev); > + struct virtio_pstore_config config; > + > + config.bufsize = cpu_to_le32(dev->bufsize); > + if (dev->console) { > + config.flags |= cpu_to_le32(VIRTIO_PSTORE_CONFIG_FL_CONSOLE); > + } > + > + memcpy(config_data, &config, sizeof(struct virtio_pstore_config)); > +} > + > +static void virtio_pstore_set_config(VirtIODevice *vdev, > + const uint8_t *config_data) > +{ > + VirtIOPstore *dev = VIRTIO_PSTORE(vdev); > + struct virtio_pstore_config config; > + > + memcpy(&config, config_data, sizeof(struct virtio_pstore_config)); > + > + dev->bufsize = le32_to_cpu(config.bufsize); > +} > + > +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 = value; > +} > + > +static void pstore_release_directory(Object *obj, const char *name, > + void *opaque) > +{ > + VirtIOPstore *s = opaque; > + > + g_free(s->directory); > + s->directory = NULL; > +} > + > +static void pstore_get_bufsize(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + VirtIOPstore *s = opaque; > + uint64_t value = s->bufsize; > + > + visit_type_size(v, name, &value, errp); > +} > + > +static void pstore_set_bufsize(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + VirtIOPstore *s = opaque; > + Error *error = NULL; > + uint64_t value; > + > + visit_type_size(v, name, &value, &error); > + if (error) { > + error_propagate(errp, error); > + return; > + } > + > + if (value < 4096) { > + error_report("Warning: too small buffer size: %"PRIu64, value); > + } > + > + s->bufsize = value; > +} > + > +static void pstore_get_console(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + VirtIOPstore *s = opaque; > + bool value = s->console; > + > + visit_type_bool(v, name, &value, errp); > +} > + > +static void pstore_set_console(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + VirtIOPstore *s = opaque; > + Error *error = NULL; > + bool value; > + > + visit_type_bool(v, name, &value, &error); > + if (error) { > + error_propagate(errp, error); > + return; > + } > + > + s->console = value; > +} > + > +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); > + object_property_add(obj, "bufsize", "size", > + pstore_get_bufsize, pstore_set_bufsize, NULL, s, NULL); > + object_property_add(obj, "console", "bool", > + pstore_get_console, pstore_set_console, NULL, 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_config = virtio_pstore_get_config; > + vdc->set_config = virtio_pstore_set_config; > + 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 74d797d..000e1e9 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..d188a48 > --- /dev/null > +++ b/include/hw/virtio/virtio-pstore.h > @@ -0,0 +1,34 @@ > +/* > + * Virtio Pstore Support > + * > + * Authors: > + * Namhyung Kim <namhyung@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[2]; > + char *directory; > + uint64_t id; > + uint64_t console_id; > + DIR *dirp; > + uint64_t bufsize; > + bool console; > +} VirtIOPstore; > + > +#endif > diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h > index 77925f5..c72a9ab 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 22 /* virtio pstore */ > > #endif /* _LINUX_VIRTIO_IDS_H */ > diff --git a/include/standard-headers/linux/virtio_pstore.h b/include/standard-headers/linux/virtio_pstore.h > new file mode 100644 > index 0000000..b893d15 > --- /dev/null > +++ b/include/standard-headers/linux/virtio_pstore.h > @@ -0,0 +1,80 @@ > +#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 > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. Neither the name of IBM nor the names of its contributors > + * may be used to endorse or promote products derived from this software > + * without specific prior written permission. > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE > + * ARE DISCLAIMED. IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT > + * 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_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_TYPE_UNKNOWN 0 > +#define VIRTIO_PSTORE_TYPE_DMESG 1 > +#define VIRTIO_PSTORE_TYPE_CONSOLE 2 > + > +#define VIRTIO_PSTORE_FL_COMPRESSED 1 > + > +#define VIRTIO_PSTORE_CONFIG_FL_CONSOLE (1 << 0) > + > +struct virtio_pstore_req { > + __virtio16 cmd; > + __virtio16 type; > + __virtio32 flags; > + __virtio64 id; > + __virtio32 count; > + __virtio32 reserved; > +}; > + > +struct virtio_pstore_res { > + __virtio16 cmd; > + __virtio16 type; > + __virtio32 ret; > +}; > + > +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; > + __virtio32 flags; > +}; > + > +#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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote: > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > > 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 > > So if the point is handling system crashes, I suspect > virtio is the wrong protocol to use. ATM it's rather > elaborate for performance, it's likely not to work when > linux is crashing. I think you want something very very > simple that will still work when you crash. Like maybe > a serial device? Well, I dont' know. As you know, the kernel oops dump is already sent to serial device but it's rather slow. As I wrote in the cover letter, enabling ftrace_dump_on_oops makes it even worse.. Also pstore saves the (compressed) binary data so I thought it'd be better to have a dedicated IO channel. I know that we can't rely on anything in kernel when the kernel is crashing. In the virtualization environment, I think it'd be great if it can dump the crash data in the host directly so I chose the virtio. I thought the virtio is a relatively thin layer and independent from other kernel features. Even if it doesn't guarantee to work 100%, it's worth trying IMHO. > > > $ ls dir-xx > > dmesg-1.enc.z dmesg-2.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). > > So this lacks all management tools that a regular storage device > has, and these are necessary if people are to use this > in production. > > For example, some kind of provision for limiting the amount of host disk > this can consume seems called for. Rate-limiting disk writes > on host also seems necessary. Handling host disk errors always > propagates error to guest but an option to e.g. stop vm might > be useful to avoid corruption. Yes, it needs that kind of management. I'd like to look at the existing implementation. But basically I thought it as a debugging feature and it won't produce much data for the default setting. Maybe I can limit the file size not to exceed the buffer size and keep up to pre-configured number of files for each type. Now host disk error will be propagated to guest. > > > > > The 'directory' property is required for virtio-pstore device to work. > > It also adds 'bufsize' and 'console' (boolean) properties. > > No idea what these do. Seem to be RW by guest but have no effect > otherwise. The 'directory' is a pathname which it saves the data files. The 'bufsize' sets the size of buffer used by pstore. The 'console' enables saving pstore console type data. > > > > 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> > > --- [SNIP] > > diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c > > new file mode 100644 > > index 0000000..2ca7786 > > --- /dev/null > > +++ b/hw/virtio/virtio-pstore.c > > @@ -0,0 +1,477 @@ > > +/* > > + * Virtio Pstore Device > > + * > > + * Copyright (C) 2016 LG Electronics > > + * > > + * Authors: > > + * Namhyung Kim <namhyung@gmail.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2. > > We generally ask new code to be v2 or later. Ok, will update. > > > 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_req *req) > > +{ > > + const char *basename; > > + unsigned long long id = 0; > > + unsigned int flags = le32_to_cpu(req->flags); > > + > > + switch (le16_to_cpu(req->type)) { > > + case VIRTIO_PSTORE_TYPE_DMESG: > > + basename = "dmesg"; > > + id = s->id++; > > + break; > > + case VIRTIO_PSTORE_TYPE_CONSOLE: > > + basename = "console"; > > + if (s->console_id) { > > + id = s->console_id; > > + } else { > > + id = s->console_id = s->id++; > > + } > > + break; > > + default: > > + basename = "unknown"; > > + break; > > + } > > + > > + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id, > > + 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_fileinfo *info) > > +{ > > + snprintf(buf, sz, "%s/%s", s->directory, name); > > if this does not fit, buf will not be 0 terminated and can > generally be corrupted. Will change it to use malloc instead. > > > > + > > + if (g_str_has_prefix(name, "dmesg-")) { > > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > > + name += strlen("dmesg-"); > > + } else if (g_str_has_prefix(name, "console-")) { > > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > > + name += strlen("console-"); > > + } else if (g_str_has_prefix(name, "unknown-")) { > > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > > + name += strlen("unknown-"); > > + } > > + > > + qemu_strtoull(name, NULL, 0, &info->id); > > + > > + info->flags = 0; > > + if (g_str_has_suffix(name, ".enc.z")) { > > + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED; > > + } > > +} [SNIP] > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > > + unsigned int out_num, > > + struct virtio_pstore_req *req) > > +{ > > + char path[PATH_MAX]; > > + int fd; > > + ssize_t len; > > + unsigned short type; > > + int flags = O_WRONLY | O_CREAT; > > + > > + /* we already consume the req */ > > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > > + > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > + > > + type = le16_to_cpu(req->type); > > + > > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > > + flags |= O_TRUNC; > > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > > + flags |= O_APPEND; > > + } > > + > > + fd = open(path, flags, 0644); > > + if (fd < 0) { > > + error_report("cannot open %s", path); > > + return -1; > > + } > > + len = writev(fd, out_sg, out_num); > > + close(fd); > > + > > + return len; > > All this is blocking VM until host io completes. Hmm.. I don't know about the internals of qemu. So does it make guest stop? If so, that's what I want to do for _DMESG. :) As it's called only on kernel oops I think it's admittable. But for _CONSOLE, it needs to do asynchronously. Maybe I can add a thread to do the work. > > > > +} > > + > > +static ssize_t virtio_pstore_do_close(VirtIOPstore *s) > > +{ > > + if (s->dirp == NULL) { > > + return 0; > > + } > > + > > + closedir(s->dirp); > > + s->dirp = NULL; > > + > > + return 0; > > +} > > + > > +static ssize_t virtio_pstore_do_erase(VirtIOPstore *s, > > + struct virtio_pstore_req *req) > > +{ > > + char path[PATH_MAX]; > > + > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > + > > + return unlink(path); > > +} > > + > > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq) > > +{ > > + VirtIOPstore *s = VIRTIO_PSTORE(vdev); > > + VirtQueueElement *elem; > > + struct virtio_pstore_req req; > > + struct virtio_pstore_res res; > > + ssize_t len = 0; > > + int ret; > > + > > + for (;;) { > > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); > > + if (!elem) { > > + return; > > + } > > + > > + if (elem->out_num < 1 || elem->in_num < 1) { > > + error_report("request or response buffer is missing"); > > + exit(1); > > + } > > + > > + len = iov_to_buf(elem->out_sg, elem->out_num, 0, &req, sizeof(req)); > > + if (len != (ssize_t)sizeof(req)) { > > + error_report("invalid request size: %ld", (long)len); > > + exit(1); > > + } > > + res.cmd = req.cmd; > > + res.type = req.type; > > + > > + switch (le16_to_cpu(req.cmd)) { > > + case VIRTIO_PSTORE_CMD_OPEN: > > + ret = virtio_pstore_do_open(s); > > + break; > > + case VIRTIO_PSTORE_CMD_READ: > > + ret = virtio_pstore_do_read(s, elem->in_sg, elem->in_num, &res); > > + if (ret > 0) { > > + len = ret; > > + ret = 0; > > + } > > + break; > > + case VIRTIO_PSTORE_CMD_WRITE: > > + ret = virtio_pstore_do_write(s, elem->out_sg, elem->out_num, &req); > > + break; > > + case VIRTIO_PSTORE_CMD_CLOSE: > > + ret = virtio_pstore_do_close(s); > > + break; > > + case VIRTIO_PSTORE_CMD_ERASE: > > + ret = virtio_pstore_do_erase(s, &req); > > + break; > > + default: > > + ret = -1; > > + break; > > + } > > + > > + res.ret = ret; > > + > > + iov_from_buf(elem->in_sg, elem->in_num, 0, &res, sizeof(res)); > > + virtqueue_push(vq, elem, sizeof(res) + len); > > this is wrong - len should be # of bytes written into guest memory. ??? All command except READ only write the virtio_pstore_ret so len is 0. For READ, virtio_pstore_do_read() returns the actual bytes it wrote (minus sizeof(res) of course), so I think it's fine, no? Anyway, thanks for your review! Thanks, Namhyung > > > + > > + virtio_notify(vdev, vq); > > + g_free(elem); > > + > > + if (ret < 0) { > > + return; > > + } > > + } > > +} -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote: > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote: > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > > > + unsigned int out_num, > > > + struct virtio_pstore_req *req) > > > +{ > > > + char path[PATH_MAX]; > > > + int fd; > > > + ssize_t len; > > > + unsigned short type; > > > + int flags = O_WRONLY | O_CREAT; > > > + > > > + /* we already consume the req */ > > > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > > > + > > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > > + > > > + type = le16_to_cpu(req->type); > > > + > > > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > > > + flags |= O_TRUNC; > > > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > > > + flags |= O_APPEND; > > > + } > > > + > > > + fd = open(path, flags, 0644); > > > + if (fd < 0) { > > > + error_report("cannot open %s", path); > > > + return -1; > > > + } > > > + len = writev(fd, out_sg, out_num); > > > + close(fd); > > > + > > > + return len; > > > > All this is blocking VM until host io completes. > > Hmm.. I don't know about the internals of qemu. So does it make guest > stop? If so, that's what I want to do for _DMESG. :) As it's called > only on kernel oops I think it's admittable. But for _CONSOLE, it > needs to do asynchronously. Maybe I can add a thread to do the work. Please look at include/io/channel.h. QEMU is event-driven and tends to use asynchronous I/O instead of spawning threads. The include/io/ APIs allow you to do asynchronous I/O in the event loop. Stefan
On Thu, Jul 28, 2016 at 01:56:07PM +0100, Stefan Hajnoczi wrote: > On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote: > > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote: > > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > > > > + unsigned int out_num, > > > > + struct virtio_pstore_req *req) > > > > +{ > > > > + char path[PATH_MAX]; > > > > + int fd; > > > > + ssize_t len; > > > > + unsigned short type; > > > > + int flags = O_WRONLY | O_CREAT; > > > > + > > > > + /* we already consume the req */ > > > > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > > > > + > > > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > > > + > > > > + type = le16_to_cpu(req->type); > > > > + > > > > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > > > > + flags |= O_TRUNC; > > > > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > > > > + flags |= O_APPEND; > > > > + } > > > > + > > > > + fd = open(path, flags, 0644); > > > > + if (fd < 0) { > > > > + error_report("cannot open %s", path); > > > > + return -1; > > > > + } > > > > + len = writev(fd, out_sg, out_num); > > > > + close(fd); > > > > + > > > > + return len; > > > > > > All this is blocking VM until host io completes. > > > > Hmm.. I don't know about the internals of qemu. So does it make guest > > stop? If so, that's what I want to do for _DMESG. :) As it's called > > only on kernel oops I think it's admittable. But for _CONSOLE, it > > needs to do asynchronously. Maybe I can add a thread to do the work. > > Please look at include/io/channel.h. QEMU is event-driven and tends to > use asynchronous I/O instead of spawning threads. The include/io/ APIs > allow you to do asynchronous I/O in the event loop. That is true, except for I/O to/from plain files - the QIOChannelFile impl doesn't do anything special (yet) to make that work correctly in non-blocking mode. Of course that could be fixed... Regards, Daniel
On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > 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-1.enc.z dmesg-2.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 'directory' property is required for virtio-pstore device to work. > It also adds 'bufsize' and 'console' (boolean) properties. > > 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> > --- > hw/virtio/Makefile.objs | 2 +- > hw/virtio/virtio-pci.c | 54 +++ > hw/virtio/virtio-pci.h | 14 + > hw/virtio/virtio-pstore.c | 477 +++++++++++++++++++++++++ > include/hw/pci/pci.h | 1 + > include/hw/virtio/virtio-pstore.h | 34 ++ > include/standard-headers/linux/virtio_ids.h | 1 + > include/standard-headers/linux/virtio_pstore.h | 80 +++++ > qdev-monitor.c | 1 + > 9 files changed, 663 insertions(+), 1 deletion(-) > create mode 100644 hw/virtio/virtio-pstore.c > create mode 100644 include/hw/virtio/virtio-pstore.h > create mode 100644 include/standard-headers/linux/virtio_pstore.h > > 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 f0677b7..d99a405 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -2414,6 +2414,59 @@ 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); > + object_property_add_alias(obj, "bufsize", OBJECT(&dev->vdev), > + "bufsize", &error_abort); > + object_property_add_alias(obj, "console", OBJECT(&dev->vdev), > + "console", &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, > @@ -2483,6 +2536,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..2ca7786 > --- /dev/null > +++ b/hw/virtio/virtio-pstore.c > @@ -0,0 +1,477 @@ > +/* > + * Virtio Pstore Device > + * > + * Copyright (C) 2016 LG Electronics > + * > + * Authors: > + * Namhyung Kim <namhyung@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_req *req) > +{ > + const char *basename; > + unsigned long long id = 0; > + unsigned int flags = le32_to_cpu(req->flags); > + > + switch (le16_to_cpu(req->type)) { > + case VIRTIO_PSTORE_TYPE_DMESG: > + basename = "dmesg"; > + id = s->id++; > + break; > + case VIRTIO_PSTORE_TYPE_CONSOLE: > + basename = "console"; > + if (s->console_id) { > + id = s->console_id; > + } else { > + id = s->console_id = s->id++; > + } > + break; > + default: > + basename = "unknown"; > + break; > + } > + > + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id, > + flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : ""); Please use g_strdup_printf() instead of splattering into a pre-allocated buffer than may or may not be large enough. > +} > + > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name, > + char *buf, size_t sz, > + struct virtio_pstore_fileinfo *info) > +{ > + snprintf(buf, sz, "%s/%s", s->directory, name); > + > + if (g_str_has_prefix(name, "dmesg-")) { > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > + name += strlen("dmesg-"); > + } else if (g_str_has_prefix(name, "console-")) { > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > + name += strlen("console-"); > + } else if (g_str_has_prefix(name, "unknown-")) { > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > + name += strlen("unknown-"); > + } > + > + qemu_strtoull(name, NULL, 0, &info->id); > + > + info->flags = 0; > + if (g_str_has_suffix(name, ".enc.z")) { > + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED; > + } > +} > + > +static ssize_t virtio_pstore_do_open(VirtIOPstore *s) > +{ > + s->dirp = opendir(s->directory); > + if (s->dirp == NULL) { > + return -1; > + } > + > + return 0; > +} > + > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, struct iovec *in_sg, > + unsigned int in_num, > + struct virtio_pstore_res *res) > +{ > + char path[PATH_MAX]; Don't declare PATH_MAX sized variables > + int fd; > + ssize_t len; > + struct stat stbuf; > + struct dirent *dent; > + int sg_num = in_num; > + struct iovec sg[sg_num]; 'sg_num' is initialized from 'in_num' which comes from the guest, and I'm not seeing anything which is bounds-checking the 'in_num' value. So you've possibly got a security flaw here I think, if the guest can cause QEMU to allocate arbitrary stack memory & thus overflow by setting arbitrarily large in_num. > + struct virtio_pstore_fileinfo info; > + size_t offset = sizeof(*res) + sizeof(info); > + > + if (s->dirp == NULL) { > + return -1; > + } > + > + dent = readdir(s->dirp); > + while (dent) { > + if (dent->d_name[0] != '.') { > + break; > + } > + dent = readdir(s->dirp); > + } > + > + if (dent == NULL) { > + return 0; > + } So this seems to just be picking the first filename reported by readdir that isn't starting with '.'. Surely this can't the right logic when your corresponding do_write method can pick several different filenames, its potluck which do_read will give back. > + > + /* skip res and fileinfo */ > + sg_num = iov_copy(sg, sg_num, in_sg, in_num, offset, > + iov_size(in_sg, in_num) - offset); > + > + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), &info); > + fd = open(path, O_RDONLY); > + if (fd < 0) { > + error_report("cannot open %s", path); > + return -1; > + } > + > + if (fstat(fd, &stbuf) < 0) { > + len = -1; > + goto out; > + } > + > + len = readv(fd, sg, sg_num); > + if (len < 0) { > + if (errno == EAGAIN) { > + len = 0; > + } > + goto out; > + } > + > + info.id = cpu_to_le64(info.id); > + info.type = cpu_to_le16(info.type); > + info.flags = cpu_to_le32(info.flags); > + info.len = cpu_to_le32(len); > + info.time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec); > + info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec); > + > + iov_from_buf(in_sg, in_num, sizeof(*res), &info, sizeof(info)); > + len += sizeof(info); > + > + out: > + close(fd); > + return len; > +} > + > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > + unsigned int out_num, > + struct virtio_pstore_req *req) > +{ > + char path[PATH_MAX]; > + int fd; > + ssize_t len; > + unsigned short type; > + int flags = O_WRONLY | O_CREAT; > + > + /* we already consume the req */ > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > + > + virtio_pstore_to_filename(s, path, sizeof(path), req); > + > + type = le16_to_cpu(req->type); > + > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > + flags |= O_TRUNC; > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > + flags |= O_APPEND; Using O_APPEND will cause the file to grow without bound on the host, which is highly undesirable, aka a security flaw. > + } > + > + fd = open(path, flags, 0644); > + if (fd < 0) { > + error_report("cannot open %s", path); > + return -1; > + } > + len = writev(fd, out_sg, out_num); > + close(fd); > + > + return len; > +} Regards, Daniel
On Thu, 28 Jul 2016 14:39:53 +0900 Namhyung Kim <namhyung@kernel.org> wrote: > Well, I dont' know. As you know, the kernel oops dump is already sent > to serial device but it's rather slow. As I wrote in the cover > letter, enabling ftrace_dump_on_oops makes it even worse.. Also > pstore saves the (compressed) binary data so I thought it'd be better > to have a dedicated IO channel. BTW, I agree with this. It is better to have a quick way to grab the ftrace buffers when a crash happens, as serial is excruciatingly slow. Although, currently I still use kexec/kdump, but as Namhyung said, it depends on crash being up to date. I tend to be sending in updates every time I have to use it. -- Steve -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Thu, Jul 28, 2016 at 02:08:41PM +0100, Daniel P. Berrange wrote: > On Thu, Jul 28, 2016 at 01:56:07PM +0100, Stefan Hajnoczi wrote: > > On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote: > > > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote: > > > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > > > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > > > > > + unsigned int out_num, > > > > > + struct virtio_pstore_req *req) > > > > > +{ > > > > > + char path[PATH_MAX]; > > > > > + int fd; > > > > > + ssize_t len; > > > > > + unsigned short type; > > > > > + int flags = O_WRONLY | O_CREAT; > > > > > + > > > > > + /* we already consume the req */ > > > > > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > > > > > + > > > > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > > > > + > > > > > + type = le16_to_cpu(req->type); > > > > > + > > > > > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > > > > > + flags |= O_TRUNC; > > > > > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > > > > > + flags |= O_APPEND; > > > > > + } > > > > > + > > > > > + fd = open(path, flags, 0644); > > > > > + if (fd < 0) { > > > > > + error_report("cannot open %s", path); > > > > > + return -1; > > > > > + } > > > > > + len = writev(fd, out_sg, out_num); > > > > > + close(fd); > > > > > + > > > > > + return len; > > > > > > > > All this is blocking VM until host io completes. > > > > > > Hmm.. I don't know about the internals of qemu. So does it make guest > > > stop? If so, that's what I want to do for _DMESG. :) As it's called > > > only on kernel oops I think it's admittable. But for _CONSOLE, it > > > needs to do asynchronously. Maybe I can add a thread to do the work. > > > > Please look at include/io/channel.h. QEMU is event-driven and tends to > > use asynchronous I/O instead of spawning threads. The include/io/ APIs > > allow you to do asynchronous I/O in the event loop. > > That is true, except for I/O to/from plain files - the QIOChannelFile > impl doesn't do anything special (yet) to make that work correctly in > non-blocking mode. Of course that could be fixed... Yep, I don't know how I can use the QIOChannelFile for async IO. AFAICS it's just a wrapper for normal readv/writev. Who is responsible to do these IO when I use the IO channel API? Also does it guarantee that all IOs are processed in order? Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote: > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > > 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-1.enc.z dmesg-2.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 'directory' property is required for virtio-pstore device to work. > > It also adds 'bufsize' and 'console' (boolean) properties. > > > > 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> > > --- [SNIP] > > +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz, > > + struct virtio_pstore_req *req) > > +{ > > + const char *basename; > > + unsigned long long id = 0; > > + unsigned int flags = le32_to_cpu(req->flags); > > + > > + switch (le16_to_cpu(req->type)) { > > + case VIRTIO_PSTORE_TYPE_DMESG: > > + basename = "dmesg"; > > + id = s->id++; > > + break; > > + case VIRTIO_PSTORE_TYPE_CONSOLE: > > + basename = "console"; > > + if (s->console_id) { > > + id = s->console_id; > > + } else { > > + id = s->console_id = s->id++; > > + } > > + break; > > + default: > > + basename = "unknown"; > > + break; > > + } > > + > > + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id, > > + flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : ""); > > Please use g_strdup_printf() instead of splattering into a pre-allocated > buffer than may or may not be large enough. OK. > > > +} > > + > > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name, > > + char *buf, size_t sz, > > + struct virtio_pstore_fileinfo *info) > > +{ > > + snprintf(buf, sz, "%s/%s", s->directory, name); > > + > > + if (g_str_has_prefix(name, "dmesg-")) { > > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > > + name += strlen("dmesg-"); > > + } else if (g_str_has_prefix(name, "console-")) { > > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > > + name += strlen("console-"); > > + } else if (g_str_has_prefix(name, "unknown-")) { > > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > > + name += strlen("unknown-"); > > + } > > + > > + qemu_strtoull(name, NULL, 0, &info->id); > > + > > + info->flags = 0; > > + if (g_str_has_suffix(name, ".enc.z")) { > > + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED; > > + } > > +} > > + > > +static ssize_t virtio_pstore_do_open(VirtIOPstore *s) > > +{ > > + s->dirp = opendir(s->directory); > > + if (s->dirp == NULL) { > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, struct iovec *in_sg, > > + unsigned int in_num, > > + struct virtio_pstore_res *res) > > +{ > > + char path[PATH_MAX]; > > Don't declare PATH_MAX sized variables Will change to use g_strdup_printf() as you said. > > > + int fd; > > + ssize_t len; > > + struct stat stbuf; > > + struct dirent *dent; > > + int sg_num = in_num; > > + struct iovec sg[sg_num]; > > 'sg_num' is initialized from 'in_num' which comes from the > guest, and I'm not seeing anything which is bounds-checking > the 'in_num' value. So you've possibly got a security flaw here > I think, if the guest can cause QEMU to allocate arbitrary stack > memory & thus overflow by setting arbitrarily large in_num. Will add a bound check then. > > > + struct virtio_pstore_fileinfo info; > > + size_t offset = sizeof(*res) + sizeof(info); > > + > > + if (s->dirp == NULL) { > > + return -1; > > + } > > + > > + dent = readdir(s->dirp); > > + while (dent) { > > + if (dent->d_name[0] != '.') { > > + break; > > + } > > + dent = readdir(s->dirp); > > + } > > + > > + if (dent == NULL) { > > + return 0; > > + } > > So this seems to just be picking the first filename reported by > readdir that isn't starting with '.'. Surely this can't the right > logic when your corresponding do_write method can pick several > different filenames, its potluck which do_read will give back. Do you mean that it'd be better to check a list of known filenames and fail if not? > > > + > > + /* skip res and fileinfo */ > > + sg_num = iov_copy(sg, sg_num, in_sg, in_num, offset, > > + iov_size(in_sg, in_num) - offset); > > + > > + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), &info); > > + fd = open(path, O_RDONLY); > > + if (fd < 0) { > > + error_report("cannot open %s", path); > > + return -1; > > + } > > + > > + if (fstat(fd, &stbuf) < 0) { > > + len = -1; > > + goto out; > > + } > > + > > + len = readv(fd, sg, sg_num); > > + if (len < 0) { > > + if (errno == EAGAIN) { > > + len = 0; > > + } > > + goto out; > > + } > > + > > + info.id = cpu_to_le64(info.id); > > + info.type = cpu_to_le16(info.type); > > + info.flags = cpu_to_le32(info.flags); > > + info.len = cpu_to_le32(len); > > + info.time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec); > > + info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec); > > + > > + iov_from_buf(in_sg, in_num, sizeof(*res), &info, sizeof(info)); > > + len += sizeof(info); > > + > > + out: > > + close(fd); > > + return len; > > +} > > + > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > > + unsigned int out_num, > > + struct virtio_pstore_req *req) > > +{ > > + char path[PATH_MAX]; > > + int fd; > > + ssize_t len; > > + unsigned short type; > > + int flags = O_WRONLY | O_CREAT; > > + > > + /* we already consume the req */ > > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > > + > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > + > > + type = le16_to_cpu(req->type); > > + > > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > > + flags |= O_TRUNC; > > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > > + flags |= O_APPEND; > > Using O_APPEND will cause the file to grow without bound on the > host, which is highly undesirable, aka a security flaw. Right. The plan is to add size checking and to split if it exceeds some limit. And we can keep some number of recent files only. Thanks, Namhyung > > > + } > > + > > + fd = open(path, flags, 0644); > > + if (fd < 0) { > > + error_report("cannot open %s", path); > > + return -1; > > + } > > + len = writev(fd, out_sg, out_num); > > + close(fd); > > + > > + return len; > > +} > > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 30, 2016 at 05:38:27PM +0900, Namhyung Kim wrote: > Hello, > > On Thu, Jul 28, 2016 at 02:08:41PM +0100, Daniel P. Berrange wrote: > > On Thu, Jul 28, 2016 at 01:56:07PM +0100, Stefan Hajnoczi wrote: > > > On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote: > > > > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote: > > > > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > > > > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > > > > > > + unsigned int out_num, > > > > > > + struct virtio_pstore_req *req) > > > > > > +{ > > > > > > + char path[PATH_MAX]; > > > > > > + int fd; > > > > > > + ssize_t len; > > > > > > + unsigned short type; > > > > > > + int flags = O_WRONLY | O_CREAT; > > > > > > + > > > > > > + /* we already consume the req */ > > > > > > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > > > > > > + > > > > > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > > > > > + > > > > > > + type = le16_to_cpu(req->type); > > > > > > + > > > > > > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > > > > > > + flags |= O_TRUNC; > > > > > > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > > > > > > + flags |= O_APPEND; > > > > > > + } > > > > > > + > > > > > > + fd = open(path, flags, 0644); > > > > > > + if (fd < 0) { > > > > > > + error_report("cannot open %s", path); > > > > > > + return -1; > > > > > > + } > > > > > > + len = writev(fd, out_sg, out_num); > > > > > > + close(fd); > > > > > > + > > > > > > + return len; > > > > > > > > > > All this is blocking VM until host io completes. > > > > > > > > Hmm.. I don't know about the internals of qemu. So does it make guest > > > > stop? If so, that's what I want to do for _DMESG. :) As it's called > > > > only on kernel oops I think it's admittable. But for _CONSOLE, it > > > > needs to do asynchronously. Maybe I can add a thread to do the work. > > > > > > Please look at include/io/channel.h. QEMU is event-driven and tends to > > > use asynchronous I/O instead of spawning threads. The include/io/ APIs > > > allow you to do asynchronous I/O in the event loop. > > > > That is true, except for I/O to/from plain files - the QIOChannelFile > > impl doesn't do anything special (yet) to make that work correctly in > > non-blocking mode. Of course that could be fixed... > > Yep, I don't know how I can use the QIOChannelFile for async IO. > AFAICS it's just a wrapper for normal readv/writev. Who is > responsible to do these IO when I use the IO channel API? Also does > it guarantee that all IOs are processed in order? I'd suggest just using QIOChannelFile regardless - we need to fix the blocking behaviour already for sake of the qemu chardev code, and your code just adds more pressure to fix it. I/O will be done in the order in which you make the calls, as with regular POSIX I/O funcs you're currently using. Regards, Daniel
On Sat, Jul 30, 2016 at 05:57:02PM +0900, Namhyung Kim wrote: > On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote: > > > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name, > > > + char *buf, size_t sz, > > > + struct virtio_pstore_fileinfo *info) > > > +{ > > > + snprintf(buf, sz, "%s/%s", s->directory, name); > > > + > > > + if (g_str_has_prefix(name, "dmesg-")) { > > > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > > > + name += strlen("dmesg-"); > > > + } else if (g_str_has_prefix(name, "console-")) { > > > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > > > + name += strlen("console-"); > > > + } else if (g_str_has_prefix(name, "unknown-")) { > > > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > > > + name += strlen("unknown-"); > > > + } [snip] > > > + struct virtio_pstore_fileinfo info; > > > + size_t offset = sizeof(*res) + sizeof(info); > > > + > > > + if (s->dirp == NULL) { > > > + return -1; > > > + } > > > + > > > + dent = readdir(s->dirp); > > > + while (dent) { > > > + if (dent->d_name[0] != '.') { > > > + break; > > > + } > > > + dent = readdir(s->dirp); > > > + } > > > + > > > + if (dent == NULL) { > > > + return 0; > > > + } > > > > So this seems to just be picking the first filename reported by > > readdir that isn't starting with '.'. Surely this can't the right > > logic when your corresponding do_write method can pick several > > different filenames, its potluck which do_read will give back. > > Do you mean that it'd be better to check a list of known filenames and > fail if not? No, I mean that you have several different VIRTIO_PSTORE_TYPE_nnn and use a different file for each constant. When reading this directory though you're not looking for the file corresponding to any given VIRTIO_PSTORE_TYPE_nnn - you're simply reading whichever file is found first. So you might have just read a TYPE_CONSOLE or have read a TYPE_DMESG - it surely doesn't make sense to randonly read either TYPE_CONSOLE or TYPE_DMESG based on whatever order readdir() lists the files. Regards, Daniel
Hi Daniel, On Mon, Aug 01, 2016 at 10:21:30AM +0100, Daniel P. Berrange wrote: > On Sat, Jul 30, 2016 at 05:38:27PM +0900, Namhyung Kim wrote: > > Hello, > > > > On Thu, Jul 28, 2016 at 02:08:41PM +0100, Daniel P. Berrange wrote: > > > On Thu, Jul 28, 2016 at 01:56:07PM +0100, Stefan Hajnoczi wrote: > > > > On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote: > > > > > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote: > > > > > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote: > > > > > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, > > > > > > > + unsigned int out_num, > > > > > > > + struct virtio_pstore_req *req) > > > > > > > +{ > > > > > > > + char path[PATH_MAX]; > > > > > > > + int fd; > > > > > > > + ssize_t len; > > > > > > > + unsigned short type; > > > > > > > + int flags = O_WRONLY | O_CREAT; > > > > > > > + > > > > > > > + /* we already consume the req */ > > > > > > > + iov_discard_front(&out_sg, &out_num, sizeof(*req)); > > > > > > > + > > > > > > > + virtio_pstore_to_filename(s, path, sizeof(path), req); > > > > > > > + > > > > > > > + type = le16_to_cpu(req->type); > > > > > > > + > > > > > > > + if (type == VIRTIO_PSTORE_TYPE_DMESG) { > > > > > > > + flags |= O_TRUNC; > > > > > > > + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { > > > > > > > + flags |= O_APPEND; > > > > > > > + } > > > > > > > + > > > > > > > + fd = open(path, flags, 0644); > > > > > > > + if (fd < 0) { > > > > > > > + error_report("cannot open %s", path); > > > > > > > + return -1; > > > > > > > + } > > > > > > > + len = writev(fd, out_sg, out_num); > > > > > > > + close(fd); > > > > > > > + > > > > > > > + return len; > > > > > > > > > > > > All this is blocking VM until host io completes. > > > > > > > > > > Hmm.. I don't know about the internals of qemu. So does it make guest > > > > > stop? If so, that's what I want to do for _DMESG. :) As it's called > > > > > only on kernel oops I think it's admittable. But for _CONSOLE, it > > > > > needs to do asynchronously. Maybe I can add a thread to do the work. > > > > > > > > Please look at include/io/channel.h. QEMU is event-driven and tends to > > > > use asynchronous I/O instead of spawning threads. The include/io/ APIs > > > > allow you to do asynchronous I/O in the event loop. > > > > > > That is true, except for I/O to/from plain files - the QIOChannelFile > > > impl doesn't do anything special (yet) to make that work correctly in > > > non-blocking mode. Of course that could be fixed... > > > > Yep, I don't know how I can use the QIOChannelFile for async IO. > > AFAICS it's just a wrapper for normal readv/writev. Who is > > responsible to do these IO when I use the IO channel API? Also does > > it guarantee that all IOs are processed in order? > > I'd suggest just using QIOChannelFile regardless - we need to fix the > blocking behaviour already for sake of the qemu chardev code, and your > code just adds more pressure to fix it. I/O will be done in the order > in which you make the calls, as with regular POSIX I/O funcs you're > currently using. Thanks for the info. So can I simply replace regular IO funcs to QIOChannel API like below? ioc = qio_channel_file_new_path(path, flags, 0644, &err); qio_channel_set_blocking(ioc, false, &err); len = qio_channel_writev(ioc, out_sg, out_num, &err); qio_channel_file_close(ioc, &err); return len; Or do I need to call qio_channel_add_watch() or something? Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 01, 2016 at 10:24:39AM +0100, Daniel P. Berrange wrote: > On Sat, Jul 30, 2016 at 05:57:02PM +0900, Namhyung Kim wrote: > > On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote: > > > > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name, > > > > + char *buf, size_t sz, > > > > + struct virtio_pstore_fileinfo *info) > > > > +{ > > > > + snprintf(buf, sz, "%s/%s", s->directory, name); > > > > + > > > > + if (g_str_has_prefix(name, "dmesg-")) { > > > > + info->type = VIRTIO_PSTORE_TYPE_DMESG; > > > > + name += strlen("dmesg-"); > > > > + } else if (g_str_has_prefix(name, "console-")) { > > > > + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; > > > > + name += strlen("console-"); > > > > + } else if (g_str_has_prefix(name, "unknown-")) { > > > > + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; > > > > + name += strlen("unknown-"); > > > > + } > > [snip] > > > > > + struct virtio_pstore_fileinfo info; > > > > + size_t offset = sizeof(*res) + sizeof(info); > > > > + > > > > + if (s->dirp == NULL) { > > > > + return -1; > > > > + } > > > > + > > > > + dent = readdir(s->dirp); > > > > + while (dent) { > > > > + if (dent->d_name[0] != '.') { > > > > + break; > > > > + } > > > > + dent = readdir(s->dirp); > > > > + } > > > > + > > > > + if (dent == NULL) { > > > > + return 0; > > > > + } > > > > > > So this seems to just be picking the first filename reported by > > > readdir that isn't starting with '.'. Surely this can't the right > > > logic when your corresponding do_write method can pick several > > > different filenames, its potluck which do_read will give back. > > > > Do you mean that it'd be better to check a list of known filenames and > > fail if not? > > No, I mean that you have several different VIRTIO_PSTORE_TYPE_nnn and > use a different file for each constant. When reading this directory > though you're not looking for the file corresponding to any given > VIRTIO_PSTORE_TYPE_nnn - you're simply reading whichever file is found > first. So you might have just read a TYPE_CONSOLE or have read a > TYPE_DMESG - it surely doesn't make sense to randonly read either > TYPE_CONSOLE or TYPE_DMESG based on whatever order readdir() lists > the files. In fact, each VIRTIO_PSTORE_TYPE_xxx can have more than one files and I don't care about the ordering between them. They will be fed to the pstore filesystem on the guest. But I also think it'd be better to scan files of known types only rather than read out whatever readdir() gives. Thanks, Namhyung -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 f0677b7..d99a405 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -2414,6 +2414,59 @@ 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); + object_property_add_alias(obj, "bufsize", OBJECT(&dev->vdev), + "bufsize", &error_abort); + object_property_add_alias(obj, "console", OBJECT(&dev->vdev), + "console", &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, @@ -2483,6 +2536,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..2ca7786 --- /dev/null +++ b/hw/virtio/virtio-pstore.c @@ -0,0 +1,477 @@ +/* + * Virtio Pstore Device + * + * Copyright (C) 2016 LG Electronics + * + * Authors: + * Namhyung Kim <namhyung@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_req *req) +{ + const char *basename; + unsigned long long id = 0; + unsigned int flags = le32_to_cpu(req->flags); + + switch (le16_to_cpu(req->type)) { + case VIRTIO_PSTORE_TYPE_DMESG: + basename = "dmesg"; + id = s->id++; + break; + case VIRTIO_PSTORE_TYPE_CONSOLE: + basename = "console"; + if (s->console_id) { + id = s->console_id; + } else { + id = s->console_id = s->id++; + } + break; + default: + basename = "unknown"; + break; + } + + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id, + 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_fileinfo *info) +{ + snprintf(buf, sz, "%s/%s", s->directory, name); + + if (g_str_has_prefix(name, "dmesg-")) { + info->type = VIRTIO_PSTORE_TYPE_DMESG; + name += strlen("dmesg-"); + } else if (g_str_has_prefix(name, "console-")) { + info->type = VIRTIO_PSTORE_TYPE_CONSOLE; + name += strlen("console-"); + } else if (g_str_has_prefix(name, "unknown-")) { + info->type = VIRTIO_PSTORE_TYPE_UNKNOWN; + name += strlen("unknown-"); + } + + qemu_strtoull(name, NULL, 0, &info->id); + + info->flags = 0; + if (g_str_has_suffix(name, ".enc.z")) { + info->flags |= VIRTIO_PSTORE_FL_COMPRESSED; + } +} + +static ssize_t virtio_pstore_do_open(VirtIOPstore *s) +{ + s->dirp = opendir(s->directory); + if (s->dirp == NULL) { + return -1; + } + + return 0; +} + +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, struct iovec *in_sg, + unsigned int in_num, + struct virtio_pstore_res *res) +{ + char path[PATH_MAX]; + int fd; + ssize_t len; + struct stat stbuf; + struct dirent *dent; + int sg_num = in_num; + struct iovec sg[sg_num]; + struct virtio_pstore_fileinfo info; + size_t offset = sizeof(*res) + sizeof(info); + + if (s->dirp == NULL) { + return -1; + } + + dent = readdir(s->dirp); + while (dent) { + if (dent->d_name[0] != '.') { + break; + } + dent = readdir(s->dirp); + } + + if (dent == NULL) { + return 0; + } + + /* skip res and fileinfo */ + sg_num = iov_copy(sg, sg_num, in_sg, in_num, offset, + iov_size(in_sg, in_num) - offset); + + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), &info); + fd = open(path, O_RDONLY); + if (fd < 0) { + error_report("cannot open %s", path); + return -1; + } + + if (fstat(fd, &stbuf) < 0) { + len = -1; + goto out; + } + + len = readv(fd, sg, sg_num); + if (len < 0) { + if (errno == EAGAIN) { + len = 0; + } + goto out; + } + + info.id = cpu_to_le64(info.id); + info.type = cpu_to_le16(info.type); + info.flags = cpu_to_le32(info.flags); + info.len = cpu_to_le32(len); + info.time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec); + info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec); + + iov_from_buf(in_sg, in_num, sizeof(*res), &info, sizeof(info)); + len += sizeof(info); + + out: + close(fd); + return len; +} + +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec *out_sg, + unsigned int out_num, + struct virtio_pstore_req *req) +{ + char path[PATH_MAX]; + int fd; + ssize_t len; + unsigned short type; + int flags = O_WRONLY | O_CREAT; + + /* we already consume the req */ + iov_discard_front(&out_sg, &out_num, sizeof(*req)); + + virtio_pstore_to_filename(s, path, sizeof(path), req); + + type = le16_to_cpu(req->type); + + if (type == VIRTIO_PSTORE_TYPE_DMESG) { + flags |= O_TRUNC; + } else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) { + flags |= O_APPEND; + } + + fd = open(path, flags, 0644); + if (fd < 0) { + error_report("cannot open %s", path); + return -1; + } + len = writev(fd, out_sg, out_num); + close(fd); + + return len; +} + +static ssize_t virtio_pstore_do_close(VirtIOPstore *s) +{ + if (s->dirp == NULL) { + return 0; + } + + closedir(s->dirp); + s->dirp = NULL; + + return 0; +} + +static ssize_t virtio_pstore_do_erase(VirtIOPstore *s, + struct virtio_pstore_req *req) +{ + char path[PATH_MAX]; + + virtio_pstore_to_filename(s, path, sizeof(path), req); + + return unlink(path); +} + +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq) +{ + VirtIOPstore *s = VIRTIO_PSTORE(vdev); + VirtQueueElement *elem; + struct virtio_pstore_req req; + struct virtio_pstore_res res; + ssize_t len = 0; + int ret; + + for (;;) { + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); + if (!elem) { + return; + } + + if (elem->out_num < 1 || elem->in_num < 1) { + error_report("request or response buffer is missing"); + exit(1); + } + + len = iov_to_buf(elem->out_sg, elem->out_num, 0, &req, sizeof(req)); + if (len != (ssize_t)sizeof(req)) { + error_report("invalid request size: %ld", (long)len); + exit(1); + } + res.cmd = req.cmd; + res.type = req.type; + + switch (le16_to_cpu(req.cmd)) { + case VIRTIO_PSTORE_CMD_OPEN: + ret = virtio_pstore_do_open(s); + break; + case VIRTIO_PSTORE_CMD_READ: + ret = virtio_pstore_do_read(s, elem->in_sg, elem->in_num, &res); + if (ret > 0) { + len = ret; + ret = 0; + } + break; + case VIRTIO_PSTORE_CMD_WRITE: + ret = virtio_pstore_do_write(s, elem->out_sg, elem->out_num, &req); + break; + case VIRTIO_PSTORE_CMD_CLOSE: + ret = virtio_pstore_do_close(s); + break; + case VIRTIO_PSTORE_CMD_ERASE: + ret = virtio_pstore_do_erase(s, &req); + break; + default: + ret = -1; + break; + } + + res.ret = ret; + + iov_from_buf(elem->in_sg, elem->in_num, 0, &res, sizeof(res)); + virtqueue_push(vq, elem, sizeof(res) + len); + + virtio_notify(vdev, vq); + g_free(elem); + + if (ret < 0) { + return; + } + } +} + +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, + sizeof(struct virtio_pstore_config)); + + s->id = 1; + s->console_id = 0; + + s->vq[0] = virtio_add_queue(vdev, 128, virtio_pstore_handle_io); + s->vq[1] = 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 void virtio_pstore_get_config(VirtIODevice *vdev, uint8_t *config_data) +{ + VirtIOPstore *dev = VIRTIO_PSTORE(vdev); + struct virtio_pstore_config config; + + config.bufsize = cpu_to_le32(dev->bufsize); + if (dev->console) { + config.flags |= cpu_to_le32(VIRTIO_PSTORE_CONFIG_FL_CONSOLE); + } + + memcpy(config_data, &config, sizeof(struct virtio_pstore_config)); +} + +static void virtio_pstore_set_config(VirtIODevice *vdev, + const uint8_t *config_data) +{ + VirtIOPstore *dev = VIRTIO_PSTORE(vdev); + struct virtio_pstore_config config; + + memcpy(&config, config_data, sizeof(struct virtio_pstore_config)); + + dev->bufsize = le32_to_cpu(config.bufsize); +} + +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 = value; +} + +static void pstore_release_directory(Object *obj, const char *name, + void *opaque) +{ + VirtIOPstore *s = opaque; + + g_free(s->directory); + s->directory = NULL; +} + +static void pstore_get_bufsize(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + VirtIOPstore *s = opaque; + uint64_t value = s->bufsize; + + visit_type_size(v, name, &value, errp); +} + +static void pstore_set_bufsize(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + VirtIOPstore *s = opaque; + Error *error = NULL; + uint64_t value; + + visit_type_size(v, name, &value, &error); + if (error) { + error_propagate(errp, error); + return; + } + + if (value < 4096) { + error_report("Warning: too small buffer size: %"PRIu64, value); + } + + s->bufsize = value; +} + +static void pstore_get_console(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + VirtIOPstore *s = opaque; + bool value = s->console; + + visit_type_bool(v, name, &value, errp); +} + +static void pstore_set_console(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + VirtIOPstore *s = opaque; + Error *error = NULL; + bool value; + + visit_type_bool(v, name, &value, &error); + if (error) { + error_propagate(errp, error); + return; + } + + s->console = value; +} + +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); + object_property_add(obj, "bufsize", "size", + pstore_get_bufsize, pstore_set_bufsize, NULL, s, NULL); + object_property_add(obj, "console", "bool", + pstore_get_console, pstore_set_console, NULL, 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_config = virtio_pstore_get_config; + vdc->set_config = virtio_pstore_set_config; + 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 74d797d..000e1e9 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..d188a48 --- /dev/null +++ b/include/hw/virtio/virtio-pstore.h @@ -0,0 +1,34 @@ +/* + * Virtio Pstore Support + * + * Authors: + * Namhyung Kim <namhyung@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[2]; + char *directory; + uint64_t id; + uint64_t console_id; + DIR *dirp; + uint64_t bufsize; + bool console; +} VirtIOPstore; + +#endif diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h index 77925f5..c72a9ab 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 22 /* virtio pstore */ #endif /* _LINUX_VIRTIO_IDS_H */ diff --git a/include/standard-headers/linux/virtio_pstore.h b/include/standard-headers/linux/virtio_pstore.h new file mode 100644 index 0000000..b893d15 --- /dev/null +++ b/include/standard-headers/linux/virtio_pstore.h @@ -0,0 +1,80 @@ +#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 + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the name of IBM nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL IBM OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * 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_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_TYPE_UNKNOWN 0 +#define VIRTIO_PSTORE_TYPE_DMESG 1 +#define VIRTIO_PSTORE_TYPE_CONSOLE 2 + +#define VIRTIO_PSTORE_FL_COMPRESSED 1 + +#define VIRTIO_PSTORE_CONFIG_FL_CONSOLE (1 << 0) + +struct virtio_pstore_req { + __virtio16 cmd; + __virtio16 type; + __virtio32 flags; + __virtio64 id; + __virtio32 count; + __virtio32 reserved; +}; + +struct virtio_pstore_res { + __virtio16 cmd; + __virtio16 type; + __virtio32 ret; +}; + +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; + __virtio32 flags; +}; + +#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" }, { } };
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-1.enc.z dmesg-2.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 'directory' property is required for virtio-pstore device to work. It also adds 'bufsize' and 'console' (boolean) properties. 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> --- hw/virtio/Makefile.objs | 2 +- hw/virtio/virtio-pci.c | 54 +++ hw/virtio/virtio-pci.h | 14 + hw/virtio/virtio-pstore.c | 477 +++++++++++++++++++++++++ include/hw/pci/pci.h | 1 + include/hw/virtio/virtio-pstore.h | 34 ++ include/standard-headers/linux/virtio_ids.h | 1 + include/standard-headers/linux/virtio_pstore.h | 80 +++++ qdev-monitor.c | 1 + 9 files changed, 663 insertions(+), 1 deletion(-) create mode 100644 hw/virtio/virtio-pstore.c create mode 100644 include/hw/virtio/virtio-pstore.h create mode 100644 include/standard-headers/linux/virtio_pstore.h