* Re: [PATCH 2/3] qemu: Implement virtio-pstore device
From: Namhyung Kim @ 2016-07-18 14:21 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Tony Luck, Radim Kr??m????, Kees Cook, kvm, Michael S. Tsirkin,
Anton Vorontsov, LKML, Steven Rostedt, qemu-devel, Minchan Kim,
Anthony Liguori, Colin Cross, Paolo Bonzini, virtualization,
Ingo Molnar
In-Reply-To: <20160718100353.GA15163@stefanha-x1.localdomain>
Hello,
On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote:
> On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote:
> > From: Namhyung Kim <namhyung@gmail.com>
> >
> > Add virtio pstore device to allow kernel log files saved on the host.
> > It will save the log files on the directory given by pstore device
> > option.
> >
> > $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
> >
> > (guest) # echo c > /proc/sysrq-trigger
> >
> > $ ls dir-xx
> > dmesg-0.enc.z dmesg-1.enc.z
> >
> > The log files are usually compressed using zlib. Users can see the log
> > messages directly on the host or on the guest (using pstore filesystem).
>
> The implementation is synchronous (i.e. can pause guest code execution),
> does not handle write errors, and does not limit the amount of data the
> guest can write. This is sufficient for ad-hoc debugging and usage with
> trusted guests.
>
> If you want this to be available in environments where the guest isn't
> trusted then there must be a limit on how much the guest can write or
> some kind of log rotation.
Right. The synchronous IO is required by the pstore subsystem
implementation AFAIK (it uses a single psinfo->buf in the loop). And
I agree that it should have a way to handle write errors and to limit
amount of data.
>
> >
> > Cc: Paolo Bonzini <pbonzini@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@gmail.com>
> > ---
[SNIP]
> > +
> > +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz,
> > + struct virtio_pstore_hdr *hdr)
> > +{
> > + const char *basename;
> > +
> > + switch (hdr->type) {
>
> Missing le16_to_cpu()?
>
> > + case VIRTIO_PSTORE_TYPE_DMESG:
> > + basename = "dmesg";
> > + break;
> > + default:
> > + basename = "unknown";
> > + break;
> > + }
> > +
> > + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename,
> > + (unsigned long long) hdr->id,
>
> Missing le64_to_cpu()?
>
> > + hdr->flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");
>
> Missing le32_to_cpu()?
Oops, will fix.
>
> > +}
> > +
> > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> > + char *buf, size_t sz,
> > + struct virtio_pstore_hdr *hdr)
> > +{
> > + size_t len = strlen(name);
> > +
> > + hdr->flags = 0;
> > + if (!strncmp(name + len - 6, ".enc.z", 6)) {
>
> Please use g_str_has_suffix(name, ".enc.z") to avoid accessing before
> the beginning of the string if the filename is shorter than 6
> characters.
Ah, ok.
>
> > + hdr->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> > + }
> > +
> > + snprintf(buf, sz, "%s/%s", s->directory, name);
> > +
> > + if (!strncmp(name, "dmesg-", 6)) {
>
> g_str_has_prefix(name, "dmesg-")
>
> > + hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_DMESG);
> > + name += 6;
> > + } else if (!strncmp(name, "unknown-", 8)) {
>
> g_str_has_prefix(name, "unknown-")
Will change.
>
> > + hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN);
> > + name += 8;
> > + }
> > +
> > + qemu_strtoull(name, NULL, 0, &hdr->id);
> > +}
> > +
> > +static ssize_t virtio_pstore_do_open(VirtIOPstore *s)
> > +{
> > + s->dir = opendir(s->directory);
> > + if (s->dir == NULL) {
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, void *buf, size_t sz,
> > + struct virtio_pstore_hdr *hdr)
> > +{
> > + char path[PATH_MAX];
> > + FILE *fp;
> > + ssize_t len;
> > + struct stat stbuf;
> > + struct dirent *dent;
> > +
> > + if (s->dir == NULL) {
> > + return -1;
> > + }
> > +
> > + dent = readdir(s->dir);
> > + while (dent) {
> > + if (dent->d_name[0] != '.') {
> > + break;
> > + }
> > + dent = readdir(s->dir);
> > + }
> > +
> > + if (dent == NULL) {
> > + return 0;
> > + }
> > +
> > + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), hdr);
> > + if (stat(path, &stbuf) < 0) {
> > + return -1;
> > + }
>
> Please use fstat(fileno(fp), &stbuf) after opening the file instead.
> The race condition doesn't matter in this case but the race-free code is
> just as simple so it's one less thing someone reading the code has to
> worry about.
Fair enough.
>
> > +
> > + fp = fopen(path, "r");
> > + if (fp == NULL) {
> > + error_report("cannot open %s (%p %p)", path, s, s->directory);
> > + return -1;
> > + }
> > +
> > + len = fread(buf, 1, sz, fp);
> > + if (len < 0 && errno == EAGAIN) {
> > + len = 0;
> > + }
> > +
> > + hdr->id = cpu_to_le64(hdr->id);
> > + hdr->flags = cpu_to_le32(hdr->flags);
> > + hdr->time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec);
> > + hdr->time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
> > +
> > + fclose(fp);
> > + return len;
> > +}
> > +
[SNIP]
> > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > + VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> > + VirtQueueElement *elem;
> > + struct virtio_pstore_hdr *hdr;
> > + ssize_t len;
> > +
> > + for (;;) {
> > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > + if (!elem) {
> > + return;
> > + }
> > +
> > + hdr = elem->out_sg[0].iov_base;
> > + if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
> > + error_report("invalid header size: %u",
> > + (unsigned)elem->out_sg[0].iov_len);
> > + exit(1);
> > + }
>
> Please use iov_to_buf() instead of directly accessing out_sg[]. Virtio
> devices are not supposed to assume a particular iovec layout. In other
> words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs.
I got it.
>
> You must also copy in data (similar to Linux syscall implementations) to
> prevent the guest from modifying data while the command is processed.
> Such race conditions could lead to security bugs.
Ok, but this assumes the operation is synchronous. I agree on your
opinion if I could make it async.
>
> > +
> > + switch (hdr->cmd) {
> > + case VIRTIO_PSTORE_CMD_OPEN:
> > + len = virtio_pstore_do_open(s);
> > + break;
> > + case VIRTIO_PSTORE_CMD_READ:
> > + len = virtio_pstore_do_read(s, elem->in_sg[0].iov_base,
> > + elem->in_sg[0].iov_len, hdr);
>
> Same issue with iovec layout for in_sg[] here. The guest driver must be
> able to submit any in_sg[] iovec array and the device cannot assume
> in_sg[0] is the only iovec to fill.
Ok.
>
> > + break;
> > + case VIRTIO_PSTORE_CMD_WRITE:
> > + len = virtio_pstore_do_write(s, elem->out_sg[1].iov_base,
> > + elem->out_sg[1].iov_len, hdr);
> > + break;
> > + case VIRTIO_PSTORE_CMD_CLOSE:
> > + len = virtio_pstore_do_close(s);
> > + break;
> > + case VIRTIO_PSTORE_CMD_ERASE:
> > + len = virtio_pstore_do_erase(s, hdr);
> > + break;
> > + default:
> > + len = -1;
> > + break;
> > + }
> > +
> > + if (len < 0) {
> > + return;
> > + }
> > +
> > + virtqueue_push(vq, elem, len);
> > +
> > + virtio_notify(vdev, vq);
> > + g_free(elem);
> > + }
> > +}
> > +
> > +static void virtio_pstore_device_realize(DeviceState *dev, Error **errp)
> > +{
> > + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > + VirtIOPstore *s = VIRTIO_PSTORE(dev);
> > +
> > + virtio_init(vdev, "virtio-pstore", VIRTIO_ID_PSTORE, 0);
> > +
> > + s->vq = virtio_add_queue(vdev, 128, virtio_pstore_handle_io);
> > +}
> > +
> > +static void virtio_pstore_device_unrealize(DeviceState *dev, Error **errp)
> > +{
> > + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > +
> > + virtio_cleanup(vdev);
> > +}
> > +
> > +static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
> > +{
> > + return f;
> > +}
> > +
> > +static void pstore_get_directory(Object *obj, Visitor *v,
> > + const char *name, void *opaque,
> > + Error **errp)
> > +{
> > + VirtIOPstore *s = opaque;
> > +
> > + visit_type_str(v, name, &s->directory, errp);
> > +}
> > +
> > +static void pstore_set_directory(Object *obj, Visitor *v,
> > + const char *name, void *opaque,
> > + Error **errp)
> > +{
> > + VirtIOPstore *s = opaque;
> > + Error *local_err = NULL;
> > + char *value;
> > +
> > + visit_type_str(v, name, &value, &local_err);
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + return;
> > + }
> > +
> > + g_free(s->directory);
> > + s->directory = strdup(value);
>
> Please use g_strdup() since this is paired with g_free().
>
> Or even simpler would be s->directory = value and do not g_free(value)
> below.
Ok, I was not sure whether I could use it without alloc/free pair.
Will do it simpler way then. :)
>
> > +
> > + g_free(value);
> > +}
> > +
> > +static void pstore_release_directory(Object *obj, const char *name,
> > + void *opaque)
> > +{
> > + VirtIOPstore *s = opaque;
> > +
> > + g_free(s->directory);
> > + s->directory = NULL;
> > +}
> > +
> > +static Property virtio_pstore_properties[] = {
> > + DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void virtio_pstore_instance_init(Object *obj)
> > +{
> > + VirtIOPstore *s = VIRTIO_PSTORE(obj);
> > +
> > + object_property_add(obj, "directory", "str",
> > + pstore_get_directory, pstore_set_directory,
> > + pstore_release_directory, s, NULL);
> > +}
> > +
> > +static void virtio_pstore_class_init(ObjectClass *klass, void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> > +
> > + dc->props = virtio_pstore_properties;
> > + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > + vdc->realize = virtio_pstore_device_realize;
> > + vdc->unrealize = virtio_pstore_device_unrealize;
> > + vdc->get_features = get_features;
> > +}
> > +
> > +static const TypeInfo virtio_pstore_info = {
> > + .name = TYPE_VIRTIO_PSTORE,
> > + .parent = TYPE_VIRTIO_DEVICE,
> > + .instance_size = sizeof(VirtIOPstore),
> > + .instance_init = virtio_pstore_instance_init,
> > + .class_init = virtio_pstore_class_init,
> > +};
> > +
> > +static void virtio_register_types(void)
> > +{
> > + type_register_static(&virtio_pstore_info);
> > +}
> > +
> > +type_init(virtio_register_types)
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 9ed1624..5689c6f 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -79,6 +79,7 @@
> > #define PCI_DEVICE_ID_VIRTIO_SCSI 0x1004
> > #define PCI_DEVICE_ID_VIRTIO_RNG 0x1005
> > #define PCI_DEVICE_ID_VIRTIO_9P 0x1009
> > +#define PCI_DEVICE_ID_VIRTIO_PSTORE 0x100a
> >
> > #define PCI_VENDOR_ID_REDHAT 0x1b36
> > #define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001
> > diff --git a/include/hw/virtio/virtio-pstore.h b/include/hw/virtio/virtio-pstore.h
> > new file mode 100644
> > index 0000000..74cd1f6
> > --- /dev/null
> > +++ b/include/hw/virtio/virtio-pstore.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Virtio Pstore Support
> > + *
> > + * Authors:
> > + * Namhyung Kim <namhyung@gmail.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2. See
> > + * the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef _QEMU_VIRTIO_PSTORE_H
> > +#define _QEMU_VIRTIO_PSTORE_H
> > +
> > +#include "standard-headers/linux/virtio_pstore.h"
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/pci/pci.h"
> > +
> > +#define TYPE_VIRTIO_PSTORE "virtio-pstore-device"
> > +#define VIRTIO_PSTORE(obj) \
> > + OBJECT_CHECK(VirtIOPstore, (obj), TYPE_VIRTIO_PSTORE)
> > +
> > +typedef struct VirtIOPstore {
> > + VirtIODevice parent_obj;
> > + VirtQueue *vq;
> > + char *directory;
> > + DIR *dir;
> > +} VirtIOPstore;
> > +
> > +#endif
> > diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
> > index 77925f5..cba6322 100644
> > --- a/include/standard-headers/linux/virtio_ids.h
> > +++ b/include/standard-headers/linux/virtio_ids.h
> > @@ -41,5 +41,6 @@
> > #define VIRTIO_ID_CAIF 12 /* Virtio caif */
> > #define VIRTIO_ID_GPU 16 /* virtio GPU */
> > #define VIRTIO_ID_INPUT 18 /* virtio input */
> > +#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
>
> 19 has already been reserved. 22 is the next free ID (vsock, crypto,
> and sdm are currently under review and already use 19, 20, and 21).
I wasn't aware of the ongoing works but Cornelia already told me about
it. Will update.
>
> Please send a VIRTIO draft specification to
> virtio-dev@lists.oasis-open.org. You can find information on the VIRTIO
> standards process here:
> https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio
Thank you very much for this information and your detailed review!
I'll take a look at the virtio standards process too.
Thanks,
Namhyung
^ permalink raw reply
* [PATCH v2] virtio_blk: Fix a slient kernel panic
From: Minfei Huang @ 2016-07-18 14:01 UTC (permalink / raw)
To: mst, cornelia.huck
Cc: Minfei Huang, fanc.fnst, linux-kernel, Minfei Huang,
virtualization
We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.
To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.
Tested-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
Signed-off-by: Minfei Huang <minfei.hmf@alibaba-inc.com>
Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
v1:
- Refactor the patch to make code more readable
---
drivers/block/virtio_blk.c | 32 +++++++++++---------------------
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..d920512 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -381,9 +381,9 @@ static int init_vq(struct virtio_blk *vblk)
{
int err = 0;
int i;
- vq_callback_t **callbacks;
- const char **names;
- struct virtqueue **vqs;
+ vq_callback_t **callbacks = NULL;
+ const char **names = NULL;
+ struct virtqueue **vqs = NULL;
unsigned short num_vqs;
struct virtio_device *vdev = vblk->vdev;
@@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
- if (!vblk->vqs) {
- err = -ENOMEM;
- goto out;
- }
+ if (!vblk->vqs)
+ return -ENOMEM;
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
- if (!names)
- goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
- if (!callbacks)
- goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
- if (!vqs)
- goto err_vqs;
+ if (!names || !callbacks || !vqs) {
+ err = -ENOMEM;
+ goto out;
+ }
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration. */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
- goto err_find_vqs;
+ goto out;
for (i = 0; i < num_vqs; i++) {
spin_lock_init(&vblk->vqs[i].lock);
@@ -428,16 +422,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
}
--
2.7.4 (Apple Git-66)
^ permalink raw reply related
* Re: [PATCH 2/3] qemu: Implement virtio-pstore device
From: Stefan Hajnoczi @ 2016-07-18 10:03 UTC (permalink / raw)
To: Namhyung Kim
Cc: Anton Vorontsov, Kees Cook, kvm, Michael S. Tsirkin, Namhyung Kim,
Radim Krčmář, LKML, Steven Rostedt, qemu-devel,
Minchan Kim, Tony Luck, Anthony Liguori, Colin Cross,
Paolo Bonzini, virtualization, Ingo Molnar
In-Reply-To: <1468816661-6345-3-git-send-email-namhyung@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 19889 bytes --]
On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung@gmail.com>
>
> Add virtio pstore device to allow kernel log files saved on the host.
> It will save the log files on the directory given by pstore device
> option.
>
> $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
>
> (guest) # echo c > /proc/sysrq-trigger
>
> $ ls dir-xx
> dmesg-0.enc.z dmesg-1.enc.z
>
> The log files are usually compressed using zlib. Users can see the log
> messages directly on the host or on the guest (using pstore filesystem).
The implementation is synchronous (i.e. can pause guest code execution),
does not handle write errors, and does not limit the amount of data the
guest can write. This is sufficient for ad-hoc debugging and usage with
trusted guests.
If you want this to be available in environments where the guest isn't
trusted then there must be a limit on how much the guest can write or
some kind of log rotation.
>
> 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@gmail.com>
> ---
> hw/virtio/Makefile.objs | 2 +-
> hw/virtio/virtio-pci.c | 50 ++++
> hw/virtio/virtio-pci.h | 14 +
> hw/virtio/virtio-pstore.c | 328 +++++++++++++++++++++
> include/hw/pci/pci.h | 1 +
> include/hw/virtio/virtio-pstore.h | 30 ++
> include/standard-headers/linux/virtio_ids.h | 1 +
> .../linux/{virtio_ids.h => virtio_pstore.h} | 48 +--
> qdev-monitor.c | 1 +
> 9 files changed, 455 insertions(+), 20 deletions(-)
> create mode 100644 hw/virtio/virtio-pstore.c
> create mode 100644 include/hw/virtio/virtio-pstore.h
> copy include/standard-headers/linux/{virtio_ids.h => virtio_pstore.h} (63%)
>
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index 3e2b175..aae7082 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o
> common-obj-y += virtio-mmio.o
>
> obj-y += virtio.o virtio-balloon.o
> -obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
> +obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 2b34b43..8281b80 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2416,6 +2416,55 @@ static const TypeInfo virtio_host_pci_info = {
> };
> #endif
>
> +/* virtio-pstore-pci */
> +
> +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> + VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
> + DeviceState *vdev = DEVICE(&vps->vdev);
> + Error *err = NULL;
> +
> + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
> + object_property_set_bool(OBJECT(vdev), true, "realized", &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> +}
> +
> +static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
> + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
> +
> + k->realize = virtio_pstore_pci_realize;
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +
> + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE;
> + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
> + pcidev_k->class_id = PCI_CLASS_OTHERS;
> +}
> +
> +static void virtio_pstore_pci_instance_init(Object *obj)
> +{
> + VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj);
> +
> + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> + TYPE_VIRTIO_PSTORE);
> + object_property_add_alias(obj, "directory", OBJECT(&dev->vdev),
> + "directory", &error_abort);
> +}
> +
> +static const TypeInfo virtio_pstore_pci_info = {
> + .name = TYPE_VIRTIO_PSTORE_PCI,
> + .parent = TYPE_VIRTIO_PCI,
> + .instance_size = sizeof(VirtIOPstorePCI),
> + .instance_init = virtio_pstore_pci_instance_init,
> + .class_init = virtio_pstore_pci_class_init,
> +};
> +
> /* virtio-pci-bus */
>
> static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> @@ -2485,6 +2534,7 @@ static void virtio_pci_register_types(void)
> #ifdef CONFIG_VHOST_SCSI
> type_register_static(&vhost_scsi_pci_info);
> #endif
> + type_register_static(&virtio_pstore_pci_info);
> }
>
> type_init(virtio_pci_register_types)
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index e4548c2..b4c039f 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -31,6 +31,7 @@
> #ifdef CONFIG_VHOST_SCSI
> #include "hw/virtio/vhost-scsi.h"
> #endif
> +#include "hw/virtio/virtio-pstore.h"
>
> typedef struct VirtIOPCIProxy VirtIOPCIProxy;
> typedef struct VirtIOBlkPCI VirtIOBlkPCI;
> @@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI;
> typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
> typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
> typedef struct VirtIOGPUPCI VirtIOGPUPCI;
> +typedef struct VirtIOPstorePCI VirtIOPstorePCI;
>
> /* virtio-pci-bus */
>
> @@ -311,6 +313,18 @@ struct VirtIOGPUPCI {
> VirtIOGPU vdev;
> };
>
> +/*
> + * virtio-pstore-pci: This extends VirtioPCIProxy.
> + */
> +#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci"
> +#define VIRTIO_PSTORE_PCI(obj) \
> + OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI)
> +
> +struct VirtIOPstorePCI {
> + VirtIOPCIProxy parent_obj;
> + VirtIOPstore vdev;
> +};
> +
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> #define VIRTIO_PCI_ABI_VERSION 0
>
> diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
> new file mode 100644
> index 0000000..98cee7f
> --- /dev/null
> +++ b/hw/virtio/virtio-pstore.c
> @@ -0,0 +1,328 @@
> +/*
> + * Virtio Pstore Device
> + *
> + * Copyright (C) 2016 LG Electronics
> + *
> + * Authors:
> + * Namhyung Kim <namhyung@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <stdio.h>
> +
> +#include "qemu/osdep.h"
> +#include "qemu/iov.h"
> +#include "qemu-common.h"
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "qapi/visitor.h"
> +#include "qapi-event.h"
> +#include "trace.h"
> +
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
> +#include "hw/virtio/virtio-pstore.h"
> +
> +
> +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz,
> + struct virtio_pstore_hdr *hdr)
> +{
> + const char *basename;
> +
> + switch (hdr->type) {
Missing le16_to_cpu()?
> + case VIRTIO_PSTORE_TYPE_DMESG:
> + basename = "dmesg";
> + break;
> + default:
> + basename = "unknown";
> + break;
> + }
> +
> + snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename,
> + (unsigned long long) hdr->id,
Missing le64_to_cpu()?
> + hdr->flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");
Missing le32_to_cpu()?
> +}
> +
> +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> + char *buf, size_t sz,
> + struct virtio_pstore_hdr *hdr)
> +{
> + size_t len = strlen(name);
> +
> + hdr->flags = 0;
> + if (!strncmp(name + len - 6, ".enc.z", 6)) {
Please use g_str_has_suffix(name, ".enc.z") to avoid accessing before
the beginning of the string if the filename is shorter than 6
characters.
> + hdr->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> + }
> +
> + snprintf(buf, sz, "%s/%s", s->directory, name);
> +
> + if (!strncmp(name, "dmesg-", 6)) {
g_str_has_prefix(name, "dmesg-")
> + hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_DMESG);
> + name += 6;
> + } else if (!strncmp(name, "unknown-", 8)) {
g_str_has_prefix(name, "unknown-")
> + hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN);
> + name += 8;
> + }
> +
> + qemu_strtoull(name, NULL, 0, &hdr->id);
> +}
> +
> +static ssize_t virtio_pstore_do_open(VirtIOPstore *s)
> +{
> + s->dir = opendir(s->directory);
> + if (s->dir == NULL) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, void *buf, size_t sz,
> + struct virtio_pstore_hdr *hdr)
> +{
> + char path[PATH_MAX];
> + FILE *fp;
> + ssize_t len;
> + struct stat stbuf;
> + struct dirent *dent;
> +
> + if (s->dir == NULL) {
> + return -1;
> + }
> +
> + dent = readdir(s->dir);
> + while (dent) {
> + if (dent->d_name[0] != '.') {
> + break;
> + }
> + dent = readdir(s->dir);
> + }
> +
> + if (dent == NULL) {
> + return 0;
> + }
> +
> + virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), hdr);
> + if (stat(path, &stbuf) < 0) {
> + return -1;
> + }
Please use fstat(fileno(fp), &stbuf) after opening the file instead.
The race condition doesn't matter in this case but the race-free code is
just as simple so it's one less thing someone reading the code has to
worry about.
> +
> + fp = fopen(path, "r");
> + if (fp == NULL) {
> + error_report("cannot open %s (%p %p)", path, s, s->directory);
> + return -1;
> + }
> +
> + len = fread(buf, 1, sz, fp);
> + if (len < 0 && errno == EAGAIN) {
> + len = 0;
> + }
> +
> + hdr->id = cpu_to_le64(hdr->id);
> + hdr->flags = cpu_to_le32(hdr->flags);
> + hdr->time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec);
> + hdr->time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
> +
> + fclose(fp);
> + return len;
> +}
> +
> +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, void *buf, size_t sz,
> + struct virtio_pstore_hdr *hdr)
> +{
> + char path[PATH_MAX];
> + FILE *fp;
> +
> + virtio_pstore_to_filename(s, path, sizeof(path), hdr);
> +
> + fp = fopen(path, "w");
> + if (fp == NULL) {
> + error_report("cannot open %s (%p %p)", path, s, s->directory);
> + return -1;
> + }
> + fwrite(buf, 1, sz, fp);
> + fclose(fp);
> +
> + return sz;
> +}
> +
> +static ssize_t virtio_pstore_do_close(VirtIOPstore *s)
> +{
> + if (s->dir == NULL) {
> + return 0;
> + }
> +
> + closedir(s->dir);
> + s->dir = NULL;
> +
> + return 0;
> +}
> +
> +static ssize_t virtio_pstore_do_erase(VirtIOPstore *s,
> + struct virtio_pstore_hdr *hdr)
> +{
> + char path[PATH_MAX];
> +
> + virtio_pstore_to_filename(s, path, sizeof(path), hdr);
> +
> + return unlink(path);
> +}
> +
> +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> +{
> + VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> + VirtQueueElement *elem;
> + struct virtio_pstore_hdr *hdr;
> + ssize_t len;
> +
> + for (;;) {
> + elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> + if (!elem) {
> + return;
> + }
> +
> + hdr = elem->out_sg[0].iov_base;
> + if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
> + error_report("invalid header size: %u",
> + (unsigned)elem->out_sg[0].iov_len);
> + exit(1);
> + }
Please use iov_to_buf() instead of directly accessing out_sg[]. Virtio
devices are not supposed to assume a particular iovec layout. In other
words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs.
You must also copy in data (similar to Linux syscall implementations) to
prevent the guest from modifying data while the command is processed.
Such race conditions could lead to security bugs.
> +
> + switch (hdr->cmd) {
> + case VIRTIO_PSTORE_CMD_OPEN:
> + len = virtio_pstore_do_open(s);
> + break;
> + case VIRTIO_PSTORE_CMD_READ:
> + len = virtio_pstore_do_read(s, elem->in_sg[0].iov_base,
> + elem->in_sg[0].iov_len, hdr);
Same issue with iovec layout for in_sg[] here. The guest driver must be
able to submit any in_sg[] iovec array and the device cannot assume
in_sg[0] is the only iovec to fill.
> + break;
> + case VIRTIO_PSTORE_CMD_WRITE:
> + len = virtio_pstore_do_write(s, elem->out_sg[1].iov_base,
> + elem->out_sg[1].iov_len, hdr);
> + break;
> + case VIRTIO_PSTORE_CMD_CLOSE:
> + len = virtio_pstore_do_close(s);
> + break;
> + case VIRTIO_PSTORE_CMD_ERASE:
> + len = virtio_pstore_do_erase(s, hdr);
> + break;
> + default:
> + len = -1;
> + break;
> + }
> +
> + if (len < 0) {
> + return;
> + }
> +
> + virtqueue_push(vq, elem, len);
> +
> + virtio_notify(vdev, vq);
> + g_free(elem);
> + }
> +}
> +
> +static void virtio_pstore_device_realize(DeviceState *dev, Error **errp)
> +{
> + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> + VirtIOPstore *s = VIRTIO_PSTORE(dev);
> +
> + virtio_init(vdev, "virtio-pstore", VIRTIO_ID_PSTORE, 0);
> +
> + s->vq = virtio_add_queue(vdev, 128, virtio_pstore_handle_io);
> +}
> +
> +static void virtio_pstore_device_unrealize(DeviceState *dev, Error **errp)
> +{
> + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +
> + virtio_cleanup(vdev);
> +}
> +
> +static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
> +{
> + return f;
> +}
> +
> +static void pstore_get_directory(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> +
> + visit_type_str(v, name, &s->directory, errp);
> +}
> +
> +static void pstore_set_directory(Object *obj, Visitor *v,
> + const char *name, void *opaque,
> + Error **errp)
> +{
> + VirtIOPstore *s = opaque;
> + Error *local_err = NULL;
> + char *value;
> +
> + visit_type_str(v, name, &value, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + g_free(s->directory);
> + s->directory = strdup(value);
Please use g_strdup() since this is paired with g_free().
Or even simpler would be s->directory = value and do not g_free(value)
below.
> +
> + g_free(value);
> +}
> +
> +static void pstore_release_directory(Object *obj, const char *name,
> + void *opaque)
> +{
> + VirtIOPstore *s = opaque;
> +
> + g_free(s->directory);
> + s->directory = NULL;
> +}
> +
> +static Property virtio_pstore_properties[] = {
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void virtio_pstore_instance_init(Object *obj)
> +{
> + VirtIOPstore *s = VIRTIO_PSTORE(obj);
> +
> + object_property_add(obj, "directory", "str",
> + pstore_get_directory, pstore_set_directory,
> + pstore_release_directory, s, NULL);
> +}
> +
> +static void virtio_pstore_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +
> + dc->props = virtio_pstore_properties;
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> + vdc->realize = virtio_pstore_device_realize;
> + vdc->unrealize = virtio_pstore_device_unrealize;
> + vdc->get_features = get_features;
> +}
> +
> +static const TypeInfo virtio_pstore_info = {
> + .name = TYPE_VIRTIO_PSTORE,
> + .parent = TYPE_VIRTIO_DEVICE,
> + .instance_size = sizeof(VirtIOPstore),
> + .instance_init = virtio_pstore_instance_init,
> + .class_init = virtio_pstore_class_init,
> +};
> +
> +static void virtio_register_types(void)
> +{
> + type_register_static(&virtio_pstore_info);
> +}
> +
> +type_init(virtio_register_types)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 9ed1624..5689c6f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -79,6 +79,7 @@
> #define PCI_DEVICE_ID_VIRTIO_SCSI 0x1004
> #define PCI_DEVICE_ID_VIRTIO_RNG 0x1005
> #define PCI_DEVICE_ID_VIRTIO_9P 0x1009
> +#define PCI_DEVICE_ID_VIRTIO_PSTORE 0x100a
>
> #define PCI_VENDOR_ID_REDHAT 0x1b36
> #define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001
> diff --git a/include/hw/virtio/virtio-pstore.h b/include/hw/virtio/virtio-pstore.h
> new file mode 100644
> index 0000000..74cd1f6
> --- /dev/null
> +++ b/include/hw/virtio/virtio-pstore.h
> @@ -0,0 +1,30 @@
> +/*
> + * Virtio Pstore Support
> + *
> + * Authors:
> + * Namhyung Kim <namhyung@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef _QEMU_VIRTIO_PSTORE_H
> +#define _QEMU_VIRTIO_PSTORE_H
> +
> +#include "standard-headers/linux/virtio_pstore.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/pci/pci.h"
> +
> +#define TYPE_VIRTIO_PSTORE "virtio-pstore-device"
> +#define VIRTIO_PSTORE(obj) \
> + OBJECT_CHECK(VirtIOPstore, (obj), TYPE_VIRTIO_PSTORE)
> +
> +typedef struct VirtIOPstore {
> + VirtIODevice parent_obj;
> + VirtQueue *vq;
> + char *directory;
> + DIR *dir;
> +} VirtIOPstore;
> +
> +#endif
> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
> index 77925f5..cba6322 100644
> --- a/include/standard-headers/linux/virtio_ids.h
> +++ b/include/standard-headers/linux/virtio_ids.h
> @@ -41,5 +41,6 @@
> #define VIRTIO_ID_CAIF 12 /* Virtio caif */
> #define VIRTIO_ID_GPU 16 /* virtio GPU */
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> +#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
19 has already been reserved. 22 is the next free ID (vsock, crypto,
and sdm are currently under review and already use 19, 20, and 21).
Please send a VIRTIO draft specification to
virtio-dev@lists.oasis-open.org. You can find information on the VIRTIO
standards process here:
https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Cornelia Huck @ 2016-07-18 9:02 UTC (permalink / raw)
To: Namhyung Kim
Cc: Tony Luck, Radim Kr??m????, Kees Cook, kvm, Michael S. Tsirkin,
Anton Vorontsov, LKML, Steven Rostedt, qemu-devel, Minchan Kim,
Anthony Liguori, Colin Cross, Paolo Bonzini, virtualization,
Ingo Molnar
In-Reply-To: <20160718082955.GA12086@danjae.aot.lge.com>
On Mon, 18 Jul 2016 17:29:55 +0900
Namhyung Kim <namhyung@kernel.org> wrote:
> On Mon, Jul 18, 2016 at 09:54:39AM +0200, Cornelia Huck wrote:
> > On Mon, 18 Jul 2016 13:37:39 +0900
> > Namhyung Kim <namhyung@kernel.org> wrote:
> > > +#define VIRT_PSTORE_ORDER 2
> > > +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
> >
> > It may make sense to make the size of the buffer configurable through
> > the config space.
>
> Right. I'm considering it too, but it needs a buffer larger than
> kmsg_bytes (= 10K) to work properly in the current implementation. As
> this version is just to verify the idea is sane and useful, I used a
> fixed size buffer. Will change in the next version.
Sure, that makes sense for a prototype. We can guard any config space
entry with a feature bit, but this one makes sense to add from the
beginning.
> > Speaking of the standard: I think it makes sense to at least reserve a
> > device id for pstore, as the idea is sound. Maybe prepare a patch to
> > the standard as well if you have time?
>
> I'd love to. As I mentioned earlier, I don't have enough knowledge in
> this area. Could you please provide some links about how can I do that?
See the virtio page at OASIS
(https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio)
for a link to our subversion (yes...) repository. Just do two patches:
one to reserve a device id, and one that specifies how device and
driver work. (For examples, look at the proposed device types that have
been posted to the virtualization lists, e.g. virtio-crypto or
virtio-sdm). You just need to be patient, we're currently a bit
stalled...
^ permalink raw reply
* Re: [PATCH 2/3] qemu: Implement virtio-pstore device
From: Namhyung Kim @ 2016-07-18 8:33 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Tony Luck, Radim Kr??m????, Kees Cook, kvm, Michael S. Tsirkin,
Anton Vorontsov, LKML, Steven Rostedt, qemu-devel, Minchan Kim,
Anthony Liguori, Colin Cross, Paolo Bonzini, virtualization,
Ingo Molnar
In-Reply-To: <578C852A.1030502@de.ibm.com>
Hello,
On Mon, Jul 18, 2016 at 09:28:42AM +0200, Christian Borntraeger wrote:
> On 07/18/2016 06:37 AM, Namhyung Kim wrote:
>
> Can you do the virtio-mmio and virtio-ccw plumbing as well, or
> do you need help with that?
Any help would be greatly appreciated!
Thanks,
Namhyung
>
> [...]
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 2b34b43..8281b80 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -2416,6 +2416,55 @@ static const TypeInfo virtio_host_pci_info = {
> > };
> > #endif
> >
> > +/* virtio-pstore-pci */
> > +
> > +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> > +{
> > + VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
> > + DeviceState *vdev = DEVICE(&vps->vdev);
> > + Error *err = NULL;
> > +
> > + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
> > + object_property_set_bool(OBJECT(vdev), true, "realized", &err);
> > + if (err) {
> > + error_propagate(errp, err);
> > + return;
> > + }
> > +}
> > +
> > +static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
> > + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
> > +
> > + k->realize = virtio_pstore_pci_realize;
> > + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > +
> > + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> > + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE;
> > + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
> > + pcidev_k->class_id = PCI_CLASS_OTHERS;
> > +}
> > +
> > +static void virtio_pstore_pci_instance_init(Object *obj)
> > +{
> > + VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj);
> > +
> > + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> > + TYPE_VIRTIO_PSTORE);
> > + object_property_add_alias(obj, "directory", OBJECT(&dev->vdev),
> > + "directory", &error_abort);
> > +}
> > +
> > +static const TypeInfo virtio_pstore_pci_info = {
> > + .name = TYPE_VIRTIO_PSTORE_PCI,
> > + .parent = TYPE_VIRTIO_PCI,
> > + .instance_size = sizeof(VirtIOPstorePCI),
> > + .instance_init = virtio_pstore_pci_instance_init,
> > + .class_init = virtio_pstore_pci_class_init,
> > +};
> > +
> > /* virtio-pci-bus */
> >
> > static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> > @@ -2485,6 +2534,7 @@ static void virtio_pci_register_types(void)
> > #ifdef CONFIG_VHOST_SCSI
> > type_register_static(&vhost_scsi_pci_info);
> > #endif
> > + type_register_static(&virtio_pstore_pci_info);
> > }
> >
> > type_init(virtio_pci_register_types)
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index e4548c2..b4c039f 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
> > @@ -31,6 +31,7 @@
> > #ifdef CONFIG_VHOST_SCSI
> > #include "hw/virtio/vhost-scsi.h"
> > #endif
> > +#include "hw/virtio/virtio-pstore.h"
> >
> > typedef struct VirtIOPCIProxy VirtIOPCIProxy;
> > typedef struct VirtIOBlkPCI VirtIOBlkPCI;
> > @@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI;
> > typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
> > typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
> > typedef struct VirtIOGPUPCI VirtIOGPUPCI;
> > +typedef struct VirtIOPstorePCI VirtIOPstorePCI;
> >
> > /* virtio-pci-bus */
> >
> > @@ -311,6 +313,18 @@ struct VirtIOGPUPCI {
> > VirtIOGPU vdev;
> > };
> >
> > +/*
> > + * virtio-pstore-pci: This extends VirtioPCIProxy.
> > + */
> > +#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci"
> > +#define VIRTIO_PSTORE_PCI(obj) \
> > + OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI)
> > +
> > +struct VirtIOPstorePCI {
> > + VirtIOPCIProxy parent_obj;
> > + VirtIOPstore vdev;
> > +};
> > +
> > /* Virtio ABI version, if we increment this, we break the guest driver. */
> > #define VIRTIO_PCI_ABI_VERSION 0
> >
>
> [...]
>
^ permalink raw reply
* Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Namhyung Kim @ 2016-07-18 8:29 UTC (permalink / raw)
To: Cornelia Huck
Cc: Tony Luck, Radim Kr??m????, Kees Cook, kvm, Michael S. Tsirkin,
Anton Vorontsov, LKML, Steven Rostedt, qemu-devel, Minchan Kim,
Anthony Liguori, Colin Cross, Paolo Bonzini, virtualization,
Ingo Molnar
In-Reply-To: <20160718095439.1eabb340.cornelia.huck@de.ibm.com>
Hello,
On Mon, Jul 18, 2016 at 09:54:39AM +0200, Cornelia Huck wrote:
> On Mon, 18 Jul 2016 13:37:39 +0900
> Namhyung Kim <namhyung@kernel.org> wrote:
>
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine. Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem. It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
>
> Like the idea.
Thanks!
>
> >
> > It supports legacy PCI device using single order-2 page buffer. As all
>
> There should not be anything in there that limits this to pci, no?
Yep, there's no restriction AFAIK. I just choose it to implement the poc
code quickly.
>
> > operation of pstore is synchronous, it would be fine IMHO. However I
> > don't know how to make write operation synchronous since it's called
> > with a spinlock held (from any context including NMI).
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Kr??m???? <rkrcmar@redhat.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Anthony Liguori <aliguori@amazon.com>
> > Cc: Anton Vorontsov <anton@enomsg.org>
> > Cc: Colin Cross <ccross@android.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Cc: kvm@vger.kernel.org
> > Cc: qemu-devel@nongnu.org
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > drivers/virtio/Kconfig | 10 ++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/virtio_pstore.c | 317 +++++++++++++++++++++++++++++++++++++
> > include/uapi/linux/Kbuild | 1 +
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pstore.h | 53 +++++++
> > 6 files changed, 383 insertions(+)
> > create mode 100644 drivers/virtio/virtio_pstore.c
> > create mode 100644 include/uapi/linux/virtio_pstore.h
> >
>
> (...)
>
> > diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> > new file mode 100644
> > index 000000000000..6fe62c0f1508
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_pstore.c
> > @@ -0,0 +1,317 @@
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pstore.h>
> > +#include <linux/virtio.h>
> > +#include <linux/virtio_config.h>
> > +#include <uapi/linux/virtio_ids.h>
> > +#include <uapi/linux/virtio_pstore.h>
> > +
> > +#define VIRT_PSTORE_ORDER 2
> > +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
>
> It may make sense to make the size of the buffer configurable through
> the config space.
Right. I'm considering it too, but it needs a buffer larger than
kmsg_bytes (= 10K) to work properly in the current implementation. As
this version is just to verify the idea is sane and useful, I used a
fixed size buffer. Will change in the next version.
>
> (...)
>
> > diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> > index 77925f587b15..cba63225d85a 100644
> > --- a/include/uapi/linux/virtio_ids.h
> > +++ b/include/uapi/linux/virtio_ids.h
> > @@ -41,5 +41,6 @@
> > #define VIRTIO_ID_CAIF 12 /* Virtio caif */
> > #define VIRTIO_ID_GPU 16 /* virtio GPU */
> > #define VIRTIO_ID_INPUT 18 /* virtio input */
> > +#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
>
> This id is already used by one of the new device types queued but not
> yet in the standard. IIRC, 22 is the next free one.
Ok, will update.
>
> Speaking of the standard: I think it makes sense to at least reserve a
> device id for pstore, as the idea is sound. Maybe prepare a patch to
> the standard as well if you have time?
I'd love to. As I mentioned earlier, I don't have enough knowledge in
this area. Could you please provide some links about how can I do that?
Thanks,
Namhyung
^ permalink raw reply
* Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Cornelia Huck @ 2016-07-18 7:54 UTC (permalink / raw)
To: Namhyung Kim
Cc: Tony Luck, Radim Krčmář, Kees Cook, kvm,
Michael S. Tsirkin, Anton Vorontsov, LKML, Steven Rostedt,
qemu-devel, Minchan Kim, Anthony Liguori, Colin Cross,
Paolo Bonzini, virtualization, Ingo Molnar
In-Reply-To: <1468816661-6345-2-git-send-email-namhyung@kernel.org>
On Mon, 18 Jul 2016 13:37:39 +0900
Namhyung Kim <namhyung@kernel.org> wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine. Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem. It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
Like the idea.
>
> It supports legacy PCI device using single order-2 page buffer. As all
There should not be anything in there that limits this to pci, no?
> operation of pstore is synchronous, it would be fine IMHO. However I
> don't know how to make write operation synchronous since it's called
> with a spinlock held (from any context including NMI).
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Anthony Liguori <aliguori@amazon.com>
> Cc: Anton Vorontsov <anton@enomsg.org>
> Cc: Colin Cross <ccross@android.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: kvm@vger.kernel.org
> Cc: qemu-devel@nongnu.org
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> drivers/virtio/Kconfig | 10 ++
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio_pstore.c | 317 +++++++++++++++++++++++++++++++++++++
> include/uapi/linux/Kbuild | 1 +
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_pstore.h | 53 +++++++
> 6 files changed, 383 insertions(+)
> create mode 100644 drivers/virtio/virtio_pstore.c
> create mode 100644 include/uapi/linux/virtio_pstore.h
>
(...)
> diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> new file mode 100644
> index 000000000000..6fe62c0f1508
> --- /dev/null
> +++ b/drivers/virtio/virtio_pstore.c
> @@ -0,0 +1,317 @@
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pstore.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> +#include <uapi/linux/virtio_ids.h>
> +#include <uapi/linux/virtio_pstore.h>
> +
> +#define VIRT_PSTORE_ORDER 2
> +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
It may make sense to make the size of the buffer configurable through
the config space.
(...)
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 77925f587b15..cba63225d85a 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -41,5 +41,6 @@
> #define VIRTIO_ID_CAIF 12 /* Virtio caif */
> #define VIRTIO_ID_GPU 16 /* virtio GPU */
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> +#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
This id is already used by one of the new device types queued but not
yet in the standard. IIRC, 22 is the next free one.
Speaking of the standard: I think it makes sense to at least reserve a
device id for pstore, as the idea is sound. Maybe prepare a patch to
the standard as well if you have time?
>
> #endif /* _LINUX_VIRTIO_IDS_H */
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/3] qemu: Implement virtio-pstore device
From: Christian Borntraeger @ 2016-07-18 7:28 UTC (permalink / raw)
To: Namhyung Kim, LKML
Cc: Tony Luck, Radim Krčmář, Kees Cook, kvm,
Michael S. Tsirkin, Namhyung Kim, Anton Vorontsov, qemu-devel,
Steven Rostedt, virtualization, Minchan Kim, Anthony Liguori,
Colin Cross, Paolo Bonzini, Ingo Molnar
In-Reply-To: <1468816661-6345-3-git-send-email-namhyung@kernel.org>
On 07/18/2016 06:37 AM, Namhyung Kim wrote:
Can you do the virtio-mmio and virtio-ccw plumbing as well, or
do you need help with that?
[...]
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 2b34b43..8281b80 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2416,6 +2416,55 @@ static const TypeInfo virtio_host_pci_info = {
> };
> #endif
>
> +/* virtio-pstore-pci */
> +
> +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> + VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
> + DeviceState *vdev = DEVICE(&vps->vdev);
> + Error *err = NULL;
> +
> + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
> + object_property_set_bool(OBJECT(vdev), true, "realized", &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> +}
> +
> +static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
> + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
> +
> + k->realize = virtio_pstore_pci_realize;
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +
> + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE;
> + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
> + pcidev_k->class_id = PCI_CLASS_OTHERS;
> +}
> +
> +static void virtio_pstore_pci_instance_init(Object *obj)
> +{
> + VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj);
> +
> + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> + TYPE_VIRTIO_PSTORE);
> + object_property_add_alias(obj, "directory", OBJECT(&dev->vdev),
> + "directory", &error_abort);
> +}
> +
> +static const TypeInfo virtio_pstore_pci_info = {
> + .name = TYPE_VIRTIO_PSTORE_PCI,
> + .parent = TYPE_VIRTIO_PCI,
> + .instance_size = sizeof(VirtIOPstorePCI),
> + .instance_init = virtio_pstore_pci_instance_init,
> + .class_init = virtio_pstore_pci_class_init,
> +};
> +
> /* virtio-pci-bus */
>
> static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> @@ -2485,6 +2534,7 @@ static void virtio_pci_register_types(void)
> #ifdef CONFIG_VHOST_SCSI
> type_register_static(&vhost_scsi_pci_info);
> #endif
> + type_register_static(&virtio_pstore_pci_info);
> }
>
> type_init(virtio_pci_register_types)
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index e4548c2..b4c039f 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -31,6 +31,7 @@
> #ifdef CONFIG_VHOST_SCSI
> #include "hw/virtio/vhost-scsi.h"
> #endif
> +#include "hw/virtio/virtio-pstore.h"
>
> typedef struct VirtIOPCIProxy VirtIOPCIProxy;
> typedef struct VirtIOBlkPCI VirtIOBlkPCI;
> @@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI;
> typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
> typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
> typedef struct VirtIOGPUPCI VirtIOGPUPCI;
> +typedef struct VirtIOPstorePCI VirtIOPstorePCI;
>
> /* virtio-pci-bus */
>
> @@ -311,6 +313,18 @@ struct VirtIOGPUPCI {
> VirtIOGPU vdev;
> };
>
> +/*
> + * virtio-pstore-pci: This extends VirtioPCIProxy.
> + */
> +#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci"
> +#define VIRTIO_PSTORE_PCI(obj) \
> + OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI)
> +
> +struct VirtIOPstorePCI {
> + VirtIOPCIProxy parent_obj;
> + VirtIOPstore vdev;
> +};
> +
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> #define VIRTIO_PCI_ABI_VERSION 0
>
[...]
^ permalink raw reply
* Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Namhyung Kim @ 2016-07-18 5:50 UTC (permalink / raw)
To: Kees Cook
Cc: Tony Luck, Radim Kr??m????, KVM, Michael S. Tsirkin,
Anton Vorontsov, LKML, Steven Rostedt, qemu-devel, Minchan Kim,
Anthony Liguori, Colin Cross, Paolo Bonzini,
virtualization@lists.linux-foundation.org, Ingo Molnar
In-Reply-To: <CAGXu5jL8frrV99QXrw4touwnXAX0pesxKJnq4Bd_2QZDip7K6Q@mail.gmail.com>
Hello,
On Sun, Jul 17, 2016 at 10:12:26PM -0700, Kees Cook wrote:
> On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine. Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem. It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> >
> > It supports legacy PCI device using single order-2 page buffer. As all
> > operation of pstore is synchronous, it would be fine IMHO. However I
> > don't know how to make write operation synchronous since it's called
> > with a spinlock held (from any context including NMI).
> >
> > 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>
>
> This looks great to me! I'd love to use this in qemu. (Right now I go
> through hoops to use the ramoops backend for testing.)
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
Thank you!
>
> Notes below...
>
[SNIP]
> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type)
> > +{
> > + u16 ret;
> > +
> > + switch (type) {
> > + case PSTORE_TYPE_DMESG:
> > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG);
> > + break;
> > + default:
> > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
> > + break;
> > + }
>
> I would love to see this support PSTORE_TYPE_CONSOLE too. It should be
> relatively easy to add: I think it'd just be another virtio command?
Do you want to append the data to the host file as guest does
printk()? I think it needs some kind of buffer management, but it's
not hard to add IMHO.
>
> > +
> > + return ret;
> > +}
> > +
[SNIP]
> > +static int notrace virt_pstore_write(enum pstore_type_id type,
> > + enum kmsg_dump_reason reason,
> > + u64 *id, unsigned int part, int count,
> > + bool compressed, size_t size,
> > + struct pstore_info *psi)
> > +{
> > + struct virtio_pstore *vps = psi->data;
> > + struct virtio_pstore_hdr *hdr = &vps->hdr;
> > + struct scatterlist sg[2];
> > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0;
> > +
> > + *id = vps->id++;
> > +
> > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE);
> > + hdr->id = cpu_to_virtio64(vps->vdev, *id);
> > + hdr->flags = cpu_to_virtio32(vps->vdev, flags);
> > + hdr->type = to_virtio_type(vps, type);
> > +
> > + sg_init_table(sg, 2);
> > + sg_set_buf(&sg[0], hdr, sizeof(*hdr));
> > + sg_set_buf(&sg[1], psi->buf, size);
> > + virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC);
> > + virtqueue_kick(vps->vq);
> > +
> > + /* TODO: make it synchronous */
> > + return 0;
>
> The down side to this being asynchronous is the lack of error
> reporting. Perhaps this could check hdr->type before queuing and error
> for any VIRTIO_PSTORE_TYPE_UNKNOWN message instead of trying to send
> it?
I cannot follow, sorry. Could you please elaborate it more?
>
> > +}
> > +
> > +static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count,
> > + struct timespec time, struct pstore_info *psi)
> > +{
> > + struct virtio_pstore *vps = psi->data;
> > + struct virtio_pstore_hdr *hdr = &vps->hdr;
> > + struct scatterlist sg[1];
> > + unsigned int len;
> > +
> > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE);
> > + hdr->id = cpu_to_virtio64(vps->vdev, id);
> > + hdr->type = to_virtio_type(vps, type);
> > +
> > + sg_init_one(sg, hdr, sizeof(*hdr));
> > + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL);
> > + virtqueue_kick(vps->vq);
> > +
> > + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
> > + return 0;
> > +}
> > +
> > +static int virt_pstore_init(struct virtio_pstore *vps)
> > +{
> > + struct pstore_info *psinfo = &vps->pstore;
> > + int err;
> > +
> > + vps->id = 0;
> > + vps->buflen = 0;
> > + psinfo->bufsize = VIRT_PSTORE_BUFSIZE;
> > + psinfo->buf = (void *)__get_free_pages(GFP_KERNEL, VIRT_PSTORE_ORDER);
> > + if (!psinfo->buf) {
> > + pr_err("cannot allocate pstore buffer\n");
> > + return -ENOMEM;
> > + }
> > +
> > + psinfo->owner = THIS_MODULE;
> > + psinfo->name = "virtio";
> > + psinfo->open = virt_pstore_open;
> > + psinfo->close = virt_pstore_close;
> > + psinfo->read = virt_pstore_read;
> > + psinfo->erase = virt_pstore_erase;
> > + psinfo->write = virt_pstore_write;
> > + psinfo->flags = PSTORE_FLAGS_FRAGILE;
>
> For console support, this flag would need to be dropped -- though I
> suspect you know that already.:)
Yep, I intentionally support DMESG type only in this patchset for
simplicity. Others could be added later. :)
>
> > + psinfo->data = vps;
> > + spin_lock_init(&psinfo->buf_lock);
> > +
> > + err = pstore_register(psinfo);
> > + if (err)
> > + kfree(psinfo->buf);
> > +
> > + return err;
> > +}
[SNIP]
>
> Awesome! Can't wait to use it. :)
Thanks for your review! :)
Thanks,
Namhyung
>
> -Kees
>
> --
> Kees Cook
> Chrome OS & Brillo Security
^ permalink raw reply
* [PATCH v2] virtio_blk: Fix a slient kernel panic
From: Minfei Huang @ 2016-07-18 5:20 UTC (permalink / raw)
To: mst, Cornelia Huck; +Cc: Minfei Huang, lkml, virtualization
[-- Attachment #1.1: Type: text/plain, Size: 1 bytes --]
[-- Attachment #1.2: Type: text/html, Size: 26 bytes --]
[-- Attachment #2: 0001-virtio_blk-Fix-a-slient-kernel-panic.patch --]
[-- Type: application/octet-stream, Size: 2720 bytes --]
From 77bff82238a956ff8ee5f19468961b6863bdfe88 Mon Sep 17 00:00:00 2001
From: Minfei Huang <mnghuan@gmail.com>
Date: Fri, 15 Jul 2016 09:07:47 +0800
Subject: [PATCH v2] virtio_blk: Fix a slient kernel panic
We do a lot of memory allocation in function init_vq, and don't handle
the allocation failure properly. Then this function will return 0,
although initialization fails due to lacking memory. At that moment,
kernel will panic in guest machine, if virtio is used to drive disk.
To fix this bug, we should take care of allocation failure, and return
correct value to let caller know what happen.
Tested-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
Signed-off-by: Minfei Huang <minfei.hmf@alibaba-inc.com>
Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
v1:
- Refactor the patch to make code more readable
---
drivers/block/virtio_blk.c | 32 +++++++++++---------------------
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 42758b5..d920512 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -381,9 +381,9 @@ static int init_vq(struct virtio_blk *vblk)
{
int err = 0;
int i;
- vq_callback_t **callbacks;
- const char **names;
- struct virtqueue **vqs;
+ vq_callback_t **callbacks = NULL;
+ const char **names = NULL;
+ struct virtqueue **vqs = NULL;
unsigned short num_vqs;
struct virtio_device *vdev = vblk->vdev;
@@ -394,22 +394,16 @@ static int init_vq(struct virtio_blk *vblk)
num_vqs = 1;
vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
- if (!vblk->vqs) {
- err = -ENOMEM;
- goto out;
- }
+ if (!vblk->vqs)
+ return -ENOMEM;
names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
- if (!names)
- goto err_names;
-
callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
- if (!callbacks)
- goto err_callbacks;
-
vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
- if (!vqs)
- goto err_vqs;
+ if (!names || !callbacks || !vqs) {
+ err = -ENOMEM;
+ goto out;
+ }
for (i = 0; i < num_vqs; i++) {
callbacks[i] = virtblk_done;
@@ -420,7 +414,7 @@ static int init_vq(struct virtio_blk *vblk)
/* Discover virtqueues and write information to configuration. */
err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
- goto err_find_vqs;
+ goto out;
for (i = 0; i < num_vqs; i++) {
spin_lock_init(&vblk->vqs[i].lock);
@@ -428,16 +422,12 @@ static int init_vq(struct virtio_blk *vblk)
}
vblk->num_vqs = num_vqs;
- err_find_vqs:
+out:
kfree(vqs);
- err_vqs:
kfree(callbacks);
- err_callbacks:
kfree(names);
- err_names:
if (err)
kfree(vblk->vqs);
- out:
return err;
}
--
2.7.4 (Apple Git-66)
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Kees Cook @ 2016-07-18 5:12 UTC (permalink / raw)
To: Namhyung Kim
Cc: Tony Luck, Radim Krčmář, KVM, Michael S. Tsirkin,
Anton Vorontsov, LKML, Steven Rostedt, qemu-devel, Minchan Kim,
Anthony Liguori, Colin Cross, Paolo Bonzini,
virtualization@lists.linux-foundation.org, Ingo Molnar
In-Reply-To: <1468816661-6345-2-git-send-email-namhyung@kernel.org>
On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> The virtio pstore driver provides interface to the pstore subsystem so
> that the guest kernel's log/dump message can be saved on the host
> machine. Users can access the log file directly on the host, or on the
> guest at the next boot using pstore filesystem. It currently deals with
> kernel log (printk) buffer only, but we can extend it to have other
> information (like ftrace dump) later.
>
> It supports legacy PCI device using single order-2 page buffer. As all
> operation of pstore is synchronous, it would be fine IMHO. However I
> don't know how to make write operation synchronous since it's called
> with a spinlock held (from any context including NMI).
>
> 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>
This looks great to me! I'd love to use this in qemu. (Right now I go
through hoops to use the ramoops backend for testing.)
Reviewed-by: Kees Cook <keescook@chromium.org>
Notes below...
> ---
> drivers/virtio/Kconfig | 10 ++
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio_pstore.c | 317 +++++++++++++++++++++++++++++++++++++
> include/uapi/linux/Kbuild | 1 +
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_pstore.h | 53 +++++++
> 6 files changed, 383 insertions(+)
> create mode 100644 drivers/virtio/virtio_pstore.c
> create mode 100644 include/uapi/linux/virtio_pstore.h
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 77590320d44c..8f0e6c796c12 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -58,6 +58,16 @@ config VIRTIO_INPUT
>
> If unsure, say M.
>
> +config VIRTIO_PSTORE
> + tristate "Virtio pstore driver"
> + depends on VIRTIO
> + depends on PSTORE
> + ---help---
> + This driver supports virtio pstore devices to save/restore
> + panic and oops messages on the host.
> +
> + If unsure, say M.
> +
> config VIRTIO_MMIO
> tristate "Platform bus driver for memory mapped virtio devices"
> depends on HAS_IOMEM && HAS_DMA
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 41e30e3dc842..bee68cb26d48 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> new file mode 100644
> index 000000000000..6fe62c0f1508
> --- /dev/null
> +++ b/drivers/virtio/virtio_pstore.c
> @@ -0,0 +1,317 @@
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pstore.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> +#include <uapi/linux/virtio_ids.h>
> +#include <uapi/linux/virtio_pstore.h>
> +
> +#define VIRT_PSTORE_ORDER 2
> +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
> +
> +struct virtio_pstore {
> + struct virtio_device *vdev;
> + struct virtqueue *vq;
> + struct pstore_info pstore;
> + struct virtio_pstore_hdr hdr;
> + size_t buflen;
> + u64 id;
> +
> + /* Waiting for host to ack */
> + wait_queue_head_t acked;
> +};
> +
> +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type)
> +{
> + u16 ret;
> +
> + switch (type) {
> + case PSTORE_TYPE_DMESG:
> + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG);
> + break;
> + default:
> + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
> + break;
> + }
I would love to see this support PSTORE_TYPE_CONSOLE too. It should be
relatively easy to add: I think it'd just be another virtio command?
> +
> + return ret;
> +}
> +
> +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type)
> +{
> + enum pstore_type_id ret;
> +
> + switch (virtio16_to_cpu(vps->vdev, type)) {
> + case VIRTIO_PSTORE_TYPE_DMESG:
> + ret = PSTORE_TYPE_DMESG;
> + break;
> + default:
> + ret = PSTORE_TYPE_UNKNOWN;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static void virtpstore_ack(struct virtqueue *vq)
> +{
> + struct virtio_pstore *vps = vq->vdev->priv;
> +
> + wake_up(&vps->acked);
> +}
> +
> +static int virt_pstore_open(struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_hdr *hdr = &vps->hdr;
> + struct scatterlist sg[1];
> + unsigned int len;
> +
> + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN);
> +
> + sg_init_one(sg, hdr, sizeof(*hdr));
> + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
> + return 0;
> +}
> +
> +static int virt_pstore_close(struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_hdr *hdr = &vps->hdr;
> + struct scatterlist sg[1];
> + unsigned int len;
> +
> + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_CLOSE);
> +
> + sg_init_one(sg, hdr, sizeof(*hdr));
> + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
> + return 0;
> +}
> +
> +static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type,
> + int *count, struct timespec *time,
> + char **buf, bool *compressed,
> + struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_hdr *hdr = &vps->hdr;
> + struct scatterlist sgi[1], sgo[1];
> + struct scatterlist *sgs[2] = { sgo, sgi };
> + unsigned int len;
> + unsigned int flags;
> + void *bf;
> +
> + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_READ);
> +
> + sg_init_one(sgo, hdr, sizeof(*hdr));
> + sg_init_one(sgi, psi->buf, psi->bufsize);
> + virtqueue_add_sgs(vps->vq, sgs, 1, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
> + if (len == 0)
> + return 0;
> +
> + bf = kmalloc(len, GFP_KERNEL);
> + if (bf == NULL)
> + return -ENOMEM;
> +
> + *id = virtio64_to_cpu(vps->vdev, hdr->id);
> + *type = from_virtio_type(vps, hdr->type);
> +
> + flags = virtio32_to_cpu(vps->vdev, hdr->flags);
> + *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED;
> + *count = 1;
> +
> + time->tv_sec = virtio64_to_cpu(vps->vdev, hdr->time_sec);
> + time->tv_nsec = virtio32_to_cpu(vps->vdev, hdr->time_nsec);
> +
> + memcpy(bf, psi->buf, len);
> + *buf = bf;
> +
> + return len;
> +}
> +
> +static int notrace virt_pstore_write(enum pstore_type_id type,
> + enum kmsg_dump_reason reason,
> + u64 *id, unsigned int part, int count,
> + bool compressed, size_t size,
> + struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_hdr *hdr = &vps->hdr;
> + struct scatterlist sg[2];
> + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0;
> +
> + *id = vps->id++;
> +
> + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE);
> + hdr->id = cpu_to_virtio64(vps->vdev, *id);
> + hdr->flags = cpu_to_virtio32(vps->vdev, flags);
> + hdr->type = to_virtio_type(vps, type);
> +
> + sg_init_table(sg, 2);
> + sg_set_buf(&sg[0], hdr, sizeof(*hdr));
> + sg_set_buf(&sg[1], psi->buf, size);
> + virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC);
> + virtqueue_kick(vps->vq);
> +
> + /* TODO: make it synchronous */
> + return 0;
The down side to this being asynchronous is the lack of error
reporting. Perhaps this could check hdr->type before queuing and error
for any VIRTIO_PSTORE_TYPE_UNKNOWN message instead of trying to send
it?
> +}
> +
> +static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count,
> + struct timespec time, struct pstore_info *psi)
> +{
> + struct virtio_pstore *vps = psi->data;
> + struct virtio_pstore_hdr *hdr = &vps->hdr;
> + struct scatterlist sg[1];
> + unsigned int len;
> +
> + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE);
> + hdr->id = cpu_to_virtio64(vps->vdev, id);
> + hdr->type = to_virtio_type(vps, type);
> +
> + sg_init_one(sg, hdr, sizeof(*hdr));
> + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL);
> + virtqueue_kick(vps->vq);
> +
> + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
> + return 0;
> +}
> +
> +static int virt_pstore_init(struct virtio_pstore *vps)
> +{
> + struct pstore_info *psinfo = &vps->pstore;
> + int err;
> +
> + vps->id = 0;
> + vps->buflen = 0;
> + psinfo->bufsize = VIRT_PSTORE_BUFSIZE;
> + psinfo->buf = (void *)__get_free_pages(GFP_KERNEL, VIRT_PSTORE_ORDER);
> + if (!psinfo->buf) {
> + pr_err("cannot allocate pstore buffer\n");
> + return -ENOMEM;
> + }
> +
> + psinfo->owner = THIS_MODULE;
> + psinfo->name = "virtio";
> + psinfo->open = virt_pstore_open;
> + psinfo->close = virt_pstore_close;
> + psinfo->read = virt_pstore_read;
> + psinfo->erase = virt_pstore_erase;
> + psinfo->write = virt_pstore_write;
> + psinfo->flags = PSTORE_FLAGS_FRAGILE;
For console support, this flag would need to be dropped -- though I
suspect you know that already.:)
> + psinfo->data = vps;
> + spin_lock_init(&psinfo->buf_lock);
> +
> + err = pstore_register(psinfo);
> + if (err)
> + kfree(psinfo->buf);
> +
> + return err;
> +}
> +
> +static int virt_pstore_exit(struct virtio_pstore *vps)
> +{
> + struct pstore_info *psinfo = &vps->pstore;
> +
> + pstore_unregister(psinfo);
> +
> + free_pages((unsigned long)psinfo->buf, VIRT_PSTORE_ORDER);
> + psinfo->bufsize = 0;
> +
> + return 0;
> +}
> +
> +static int virtpstore_probe(struct virtio_device *vdev)
> +{
> + struct virtio_pstore *vps;
> + int err;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config access disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vps = kmalloc(sizeof(*vps), GFP_KERNEL);
> + if (!vps) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + vps->vdev = vdev;
> +
> + vps->vq = virtio_find_single_vq(vdev, virtpstore_ack, "pstore");
> + if (IS_ERR(vps->vq)) {
> + err = PTR_ERR(vps->vq);
> + goto out_free;
> + }
> +
> + err = virt_pstore_init(vps);
> + if (err)
> + goto out_del_vq;
> +
> + init_waitqueue_head(&vps->acked);
> +
> + virtio_device_ready(vdev);
> + dev_info(&vdev->dev, "virtio pstore driver init: ok\n");
> +
> + return 0;
> +
> +out_del_vq:
> + vdev->config->del_vqs(vdev);
> +out_free:
> + kfree(vps);
> +out:
> + dev_err(&vdev->dev, "virtio pstore driver init: failed with %d\n", err);
> + return err;
> +}
> +
> +static void virtpstore_remove(struct virtio_device *vdev)
> +{
> + struct virtio_pstore *vps = vdev->priv;
> +
> + virt_pstore_exit(vps);
> +
> + /* Now we reset the device so we can clean up the queues. */
> + vdev->config->reset(vdev);
> +
> + vdev->config->del_vqs(vdev);
> +
> + kfree(vps);
> +}
> +
> +static unsigned int features[] = {
> +};
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_PSTORE, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> +static struct virtio_driver virtio_pstore_driver = {
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .feature_table = features,
> + .feature_table_size = ARRAY_SIZE(features),
> + .id_table = id_table,
> + .probe = virtpstore_probe,
> + .remove = virtpstore_remove,
> +};
> +
> +module_virtio_driver(virtio_pstore_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Namhyung Kim <namhyung@kernel.org>");
> +MODULE_DESCRIPTION("Virtio pstore driver");
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 8bdae34d1f9a..57b0d08db322 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -448,6 +448,7 @@ header-y += virtio_ids.h
> header-y += virtio_input.h
> header-y += virtio_net.h
> header-y += virtio_pci.h
> +header-y += virtio_pstore.h
> header-y += virtio_ring.h
> header-y += virtio_rng.h
> header-y += virtio_scsi.h
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 77925f587b15..cba63225d85a 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -41,5 +41,6 @@
> #define VIRTIO_ID_CAIF 12 /* Virtio caif */
> #define VIRTIO_ID_GPU 16 /* virtio GPU */
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> +#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/include/uapi/linux/virtio_pstore.h b/include/uapi/linux/virtio_pstore.h
> new file mode 100644
> index 000000000000..0aa1575ee35f
> --- /dev/null
> +++ b/include/uapi/linux/virtio_pstore.h
> @@ -0,0 +1,53 @@
> +#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 <linux/types.h>
> +#include <linux/virtio_types.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_FL_COMPRESSED 1
> +
> +struct virtio_pstore_hdr {
> + __virtio64 id;
> + __virtio32 flags;
> + __virtio16 cmd;
> + __virtio16 type;
> + __virtio64 time_sec;
> + __virtio32 time_nsec;
> + __virtio32 unused;
> +};
> +
> +#endif /* _LINUX_VIRTIO_PSTORE_H */
> --
> 2.8.0
>
Awesome! Can't wait to use it. :)
-Kees
--
Kees Cook
Chrome OS & Brillo Security
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 3/3] kvmtool: Implement virtio-pstore device
From: Namhyung Kim @ 2016-07-18 4:37 UTC (permalink / raw)
To: LKML
Cc: Tony Luck, Radim Krčmář, Kees Cook, kvm,
Michael S. Tsirkin, Anton Vorontsov, Steven Rostedt,
virtualization, Minchan Kim, Anthony Liguori, Colin Cross,
Paolo Bonzini, Ingo Molnar
In-Reply-To: <1468816661-6345-1-git-send-email-namhyung@kernel.org>
Add virtio pstore device to allow kernel log messages saved on the
host. With this patch, it will save the log files under directory given
by --pstore option.
$ lkvm run --pstore=dir-xx
(guest) # echo c > /proc/sysrq-trigger
$ ls dir-xx
dmesg-0.enc.z dmesg-1.enc.z
The log files are usually compressed using zlib. User can easily see
the messages on the host or on the guest (using pstore filesystem).
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: virtualization@lists.linux-foundation.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
Makefile | 1 +
builtin-run.c | 2 +
include/kvm/kvm-config.h | 1 +
include/kvm/virtio-pci-dev.h | 2 +
include/kvm/virtio-pstore.h | 31 ++++
include/linux/virtio_ids.h | 1 +
virtio/pstore.c | 359 +++++++++++++++++++++++++++++++++++++++++++
7 files changed, 397 insertions(+)
create mode 100644 include/kvm/virtio-pstore.h
create mode 100644 virtio/pstore.c
diff --git a/Makefile b/Makefile
index 1f0196f..d7462b9 100644
--- a/Makefile
+++ b/Makefile
@@ -67,6 +67,7 @@ OBJS += virtio/net.o
OBJS += virtio/rng.o
OBJS += virtio/balloon.o
OBJS += virtio/pci.o
+OBJS += virtio/pstore.o
OBJS += disk/blk.o
OBJS += disk/qcow.o
OBJS += disk/raw.o
diff --git a/builtin-run.c b/builtin-run.c
index 72b878d..08c12dd 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -128,6 +128,8 @@ void kvm_run_set_wrapper_sandbox(void)
" rootfs"), \
OPT_STRING('\0', "hugetlbfs", &(cfg)->hugetlbfs_path, "path", \
"Hugetlbfs path"), \
+ OPT_STRING('\0', "pstore", &(cfg)->pstore_path, "path", \
+ "pstore data path"), \
\
OPT_GROUP("Kernel options:"), \
OPT_STRING('k', "kernel", &(cfg)->kernel_filename, "kernel", \
diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
index 386fa8c..42b7651 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -45,6 +45,7 @@ struct kvm_config {
const char *hugetlbfs_path;
const char *custom_rootfs_name;
const char *real_cmdline;
+ const char *pstore_path;
struct virtio_net_params *net_params;
bool single_step;
bool vnc;
diff --git a/include/kvm/virtio-pci-dev.h b/include/kvm/virtio-pci-dev.h
index 48ae018..4339d94 100644
--- a/include/kvm/virtio-pci-dev.h
+++ b/include/kvm/virtio-pci-dev.h
@@ -15,6 +15,7 @@
#define PCI_DEVICE_ID_VIRTIO_BLN 0x1005
#define PCI_DEVICE_ID_VIRTIO_SCSI 0x1008
#define PCI_DEVICE_ID_VIRTIO_9P 0x1009
+#define PCI_DEVICE_ID_VIRTIO_PSTORE 0x100a
#define PCI_DEVICE_ID_VESA 0x2000
#define PCI_DEVICE_ID_PCI_SHMEM 0x0001
@@ -34,5 +35,6 @@
#define PCI_CLASS_RNG 0xff0000
#define PCI_CLASS_BLN 0xff0000
#define PCI_CLASS_9P 0xff0000
+#define PCI_CLASS_PSTORE 0xff0000
#endif /* VIRTIO_PCI_DEV_H_ */
diff --git a/include/kvm/virtio-pstore.h b/include/kvm/virtio-pstore.h
new file mode 100644
index 0000000..293ab57
--- /dev/null
+++ b/include/kvm/virtio-pstore.h
@@ -0,0 +1,31 @@
+#ifndef KVM__PSTORE_VIRTIO_H
+#define KVM__PSTORE_VIRTIO_H
+
+struct kvm;
+
+#define VIRTIO_PSTORE_TYPE_UNKNOWN 0
+#define VIRTIO_PSTORE_TYPE_DMESG 1
+
+#define VIRTIO_PSTORE_CMD_NULL 0
+#define VIRTIO_PSTORE_CMD_OPEN 1
+#define VIRTIO_PSTORE_CMD_READ 2
+#define VIRTIO_PSTORE_CMD_WRITE 3
+#define VIRTIO_PSTORE_CMD_ERASE 4
+#define VIRTIO_PSTORE_CMD_CLOSE 5
+
+#define VIRTIO_PSTORE_FL_COMPRESSED 1
+
+struct pstore_hdr {
+ u64 id;
+ u32 flags;
+ u16 cmd;
+ u16 type;
+ u64 time_sec;
+ u32 time_nsec;
+ u32 unused;
+};
+
+int virtio_pstore__init(struct kvm *kvm);
+int virtio_pstore__exit(struct kvm *kvm);
+
+#endif /* KVM__PSTORE_VIRTIO_H */
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 5f60aa4..f34cabc 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -40,5 +40,6 @@
#define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
#define VIRTIO_ID_CAIF 12 /* Virtio caif */
#define VIRTIO_ID_INPUT 18 /* virtio input */
+#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
#endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/virtio/pstore.c b/virtio/pstore.c
new file mode 100644
index 0000000..094e54b
--- /dev/null
+++ b/virtio/pstore.c
@@ -0,0 +1,359 @@
+#include "kvm/virtio-pstore.h"
+
+#include "kvm/virtio-pci-dev.h"
+
+#include "kvm/virtio.h"
+#include "kvm/util.h"
+#include "kvm/kvm.h"
+#include "kvm/threadpool.h"
+#include "kvm/guest_compat.h"
+
+#include <linux/virtio_ring.h>
+
+#include <linux/list.h>
+#include <fcntl.h>
+#include <dirent.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <pthread.h>
+#include <linux/kernel.h>
+
+#define NUM_VIRT_QUEUES 1
+#define VIRTIO_PSTORE_QUEUE_SIZE 128
+
+struct pstore_dev_job {
+ struct virt_queue *vq;
+ struct pstore_dev *pdev;
+ struct thread_pool__job job_id;
+};
+
+struct pstore_dev {
+ struct list_head list;
+ struct virtio_device vdev;
+
+ int fd;
+ DIR *dir;
+
+ /* virtio queue */
+ struct virt_queue vqs[NUM_VIRT_QUEUES];
+ struct pstore_dev_job jobs[NUM_VIRT_QUEUES];
+};
+
+static LIST_HEAD(pdevs);
+static int compat_id = -1;
+
+static u8 *get_config(struct kvm *kvm, void *dev)
+{
+ /* Unused */
+ return 0;
+}
+
+static u32 get_host_features(struct kvm *kvm, void *dev)
+{
+ /* Unused */
+ return 0;
+}
+
+static void set_guest_features(struct kvm *kvm, void *dev, u32 features)
+{
+ /* Unused */
+}
+
+static void virtio_pstore_hdr_to_filename(struct kvm *kvm, struct pstore_hdr *hdr,
+ char *buf, size_t sz)
+{
+ const char *basename;
+
+ switch (hdr->type) {
+ case VIRTIO_PSTORE_TYPE_DMESG:
+ basename = "dmesg";
+ break;
+ default:
+ basename = "unknown";
+ break;
+ }
+
+ snprintf(buf, sz, "%s/%s-%llu%s", kvm->cfg.pstore_path, basename,
+ hdr->id, hdr->flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");
+}
+
+static void virtio_pstore_filename_to_hdr(struct kvm *kvm, struct pstore_hdr *hdr,
+ char *name, char *buf, size_t sz)
+{
+ size_t len = strlen(name);
+
+ hdr->flags = 0;
+ if (!strncmp(name + len - 6, ".enc.z", 6))
+ hdr->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
+
+ snprintf(buf, sz, "%s/%s", kvm->cfg.pstore_path, name);
+
+ if (!strncmp(name, "dmesg", 5)) {
+ hdr->type = VIRTIO_PSTORE_TYPE_DMESG;
+ name += 5;
+ } else if (!strncmp(name, "unknown", 7)) {
+ hdr->type = VIRTIO_PSTORE_TYPE_UNKNOWN;
+ name += 7;
+ }
+
+ hdr->id = strtoul(name + 1, NULL, 0);
+}
+
+static int virtio_pstore_do_open(struct kvm *kvm, struct pstore_dev *pdev,
+ struct pstore_hdr *hdr, struct iovec *iov)
+{
+ pdev->dir = opendir(kvm->cfg.pstore_path);
+ if (pdev->dir == NULL)
+ return -errno;
+
+ return 0;
+}
+
+static int virtio_pstore_do_close(struct kvm *kvm, struct pstore_dev *pdev,
+ struct pstore_hdr *hdr, struct iovec *iov)
+{
+ if (pdev->dir == NULL)
+ return -1;
+
+ closedir(pdev->dir);
+ pdev->dir = NULL;
+
+ return 0;
+}
+
+static ssize_t virtio_pstore_do_write(struct kvm *kvm, struct pstore_dev *pdev,
+ struct pstore_hdr *hdr, struct iovec *iov)
+{
+ char path[PATH_MAX];
+ FILE *fp;
+ ssize_t len = 0;
+
+ virtio_pstore_hdr_to_filename(kvm, hdr, path, sizeof(path));
+
+ fp = fopen(path, "a");
+ if (fp == NULL)
+ return -1;
+
+ len = fwrite(iov[1].iov_base, iov[1].iov_len, 1, fp);
+ if (len < 0 && errno == EAGAIN)
+ len = 0;
+
+ fclose(fp);
+ return len;
+}
+
+static ssize_t virtio_pstore_do_read(struct kvm *kvm, struct pstore_dev *pdev,
+ struct pstore_hdr *hdr, struct iovec *iov)
+{
+ char path[PATH_MAX];
+ FILE *fp;
+ ssize_t len = 0;
+ struct stat stbuf;
+ struct dirent *dent;
+
+ if (pdev->dir == NULL)
+ return 0;
+
+ dent = readdir(pdev->dir);
+ while (dent) {
+ if (dent->d_name[0] != '.')
+ break;
+ dent = readdir(pdev->dir);
+ }
+
+ if (dent == NULL)
+ return 0;
+
+ virtio_pstore_filename_to_hdr(kvm, hdr, dent->d_name, path, sizeof(path));
+ if (stat(path, &stbuf) < 0)
+ return -1;
+
+ fp = fopen(path, "r");
+ if (fp == NULL)
+ return -1;
+
+ len = fread(iov[1].iov_base, 1, iov[1].iov_len, fp);
+ if (len < 0 && errno == EAGAIN)
+ len = 0;
+
+ hdr->id = virtio_host_to_guest_u64(pdev->vqs, hdr->id);
+ hdr->flags = virtio_host_to_guest_u32(pdev->vqs, hdr->flags);
+
+ hdr->time_sec = virtio_host_to_guest_u64(pdev->vqs, stbuf.st_ctim.tv_sec);
+ hdr->time_nsec = virtio_host_to_guest_u32(pdev->vqs, stbuf.st_ctim.tv_nsec);
+
+ fclose(fp);
+ return len;
+}
+
+static ssize_t virtio_pstore_do_erase(struct kvm *kvm, struct pstore_dev *pdev,
+ struct pstore_hdr *hdr, struct iovec *iov)
+{
+ char path[PATH_MAX];
+
+ virtio_pstore_hdr_to_filename(kvm, hdr, path, sizeof(path));
+
+ return unlink(path);
+}
+
+static bool virtio_pstore_do_io_request(struct kvm *kvm, struct pstore_dev *pdev,
+ struct virt_queue *vq)
+{
+ struct iovec iov[VIRTIO_PSTORE_QUEUE_SIZE];
+ struct pstore_hdr *hdr;
+ ssize_t len = 0;
+ u16 out, in, head;
+
+ head = virt_queue__get_iov(vq, iov, &out, &in, kvm);
+
+ hdr = iov[0].iov_base;
+
+ switch (virtio_guest_to_host_u16(vq, hdr->cmd)) {
+ case VIRTIO_PSTORE_CMD_OPEN:
+ len = virtio_pstore_do_open(kvm, pdev, hdr, iov);
+ break;
+ case VIRTIO_PSTORE_CMD_READ:
+ len = virtio_pstore_do_read(kvm, pdev, hdr, iov);
+ break;
+ case VIRTIO_PSTORE_CMD_WRITE:
+ len = virtio_pstore_do_write(kvm, pdev, hdr, iov);
+ break;
+ case VIRTIO_PSTORE_CMD_CLOSE:
+ virtio_pstore_do_close(kvm, pdev, hdr, iov);
+ break;
+ case VIRTIO_PSTORE_CMD_ERASE:
+ len = virtio_pstore_do_erase(kvm, pdev, hdr, iov);
+ break;
+ default:
+ return false;
+ }
+
+ if (len < 0)
+ return false;
+
+ virt_queue__set_used_elem(vq, head, len);
+
+ return true;
+}
+
+static void virtio_pstore_do_io(struct kvm *kvm, void *param)
+{
+ struct pstore_dev_job *job = param;
+ struct virt_queue *vq = job->vq;
+ struct pstore_dev *pdev = job->pdev;
+
+ while (virt_queue__available(vq))
+ virtio_pstore_do_io_request(kvm, pdev, vq);
+
+ pdev->vdev.ops->signal_vq(kvm, &pdev->vdev, vq - pdev->vqs);
+}
+
+static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
+ u32 pfn)
+{
+ struct pstore_dev *pdev = dev;
+ struct virt_queue *queue;
+ struct pstore_dev_job *job;
+ void *p;
+
+ compat__remove_message(compat_id);
+
+ queue = &pdev->vqs[vq];
+ queue->pfn = pfn;
+ p = virtio_get_vq(kvm, queue->pfn, page_size);
+
+ job = &pdev->jobs[vq];
+
+ vring_init(&queue->vring, VIRTIO_PSTORE_QUEUE_SIZE, p, align);
+
+ *job = (struct pstore_dev_job) {
+ .vq = queue,
+ .pdev = pdev,
+ };
+
+ thread_pool__init_job(&job->job_id, kvm, virtio_pstore_do_io, job);
+
+ return 0;
+}
+
+static int notify_vq(struct kvm *kvm, void *dev, u32 vq)
+{
+ struct pstore_dev *pdev = dev;
+
+ thread_pool__do_job(&pdev->jobs[vq].job_id);
+
+ return 0;
+}
+
+static int get_pfn_vq(struct kvm *kvm, void *dev, u32 vq)
+{
+ struct pstore_dev *pdev = dev;
+
+ return pdev->vqs[vq].pfn;
+}
+
+static int get_size_vq(struct kvm *kvm, void *dev, u32 vq)
+{
+ return VIRTIO_PSTORE_QUEUE_SIZE;
+}
+
+static int set_size_vq(struct kvm *kvm, void *dev, u32 vq, int size)
+{
+ /* FIXME: dynamic */
+ return size;
+}
+
+static struct virtio_ops pstore_dev_virtio_ops = {
+ .get_config = get_config,
+ .get_host_features = get_host_features,
+ .set_guest_features = set_guest_features,
+ .init_vq = init_vq,
+ .notify_vq = notify_vq,
+ .get_pfn_vq = get_pfn_vq,
+ .get_size_vq = get_size_vq,
+ .set_size_vq = set_size_vq,
+};
+
+int virtio_pstore__init(struct kvm *kvm)
+{
+ struct pstore_dev *pdev;
+ int r;
+
+ if (!kvm->cfg.pstore_path)
+ return 0;
+
+ pdev = malloc(sizeof(*pdev));
+ if (pdev == NULL)
+ return -ENOMEM;
+
+ r = virtio_init(kvm, pdev, &pdev->vdev, &pstore_dev_virtio_ops,
+ VIRTIO_DEFAULT_TRANS(kvm), PCI_DEVICE_ID_VIRTIO_PSTORE,
+ VIRTIO_ID_PSTORE, PCI_CLASS_PSTORE);
+ if (r < 0)
+ goto cleanup;
+
+ list_add_tail(&pdev->list, &pdevs);
+
+ if (compat_id == -1)
+ compat_id = virtio_compat_add_message("virtio-pstore", "CONFIG_VIRTIO_PSTORE");
+ return 0;
+cleanup:
+ free(pdev);
+
+ return r;
+}
+virtio_dev_init(virtio_pstore__init);
+
+int virtio_pstore__exit(struct kvm *kvm)
+{
+ struct pstore_dev *pdev, *tmp;
+
+ list_for_each_entry_safe(pdev, tmp, &pdevs, list) {
+ list_del(&pdev->list);
+ pdev->vdev.ops->exit(kvm, &pdev->vdev);
+ free(pdev);
+ }
+
+ return 0;
+}
+virtio_dev_exit(virtio_pstore__exit);
--
2.8.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCH 2/3] qemu: Implement virtio-pstore device
From: Namhyung Kim @ 2016-07-18 4:37 UTC (permalink / raw)
To: LKML
Cc: Tony Luck, Radim Krčmář, Kees Cook, kvm,
Michael S. Tsirkin, Namhyung Kim, Anton Vorontsov, qemu-devel,
Steven Rostedt, virtualization, Minchan Kim, Anthony Liguori,
Colin Cross, Paolo Bonzini, Ingo Molnar
In-Reply-To: <1468816661-6345-1-git-send-email-namhyung@kernel.org>
From: Namhyung Kim <namhyung@gmail.com>
Add virtio pstore device to allow kernel log files saved on the host.
It will save the log files on the directory given by pstore device
option.
$ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
(guest) # echo c > /proc/sysrq-trigger
$ ls dir-xx
dmesg-0.enc.z dmesg-1.enc.z
The log files are usually compressed using zlib. Users can see the log
messages directly on the host or on the guest (using pstore filesystem).
Cc: Paolo Bonzini <pbonzini@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@gmail.com>
---
hw/virtio/Makefile.objs | 2 +-
hw/virtio/virtio-pci.c | 50 ++++
hw/virtio/virtio-pci.h | 14 +
hw/virtio/virtio-pstore.c | 328 +++++++++++++++++++++
include/hw/pci/pci.h | 1 +
include/hw/virtio/virtio-pstore.h | 30 ++
include/standard-headers/linux/virtio_ids.h | 1 +
.../linux/{virtio_ids.h => virtio_pstore.h} | 48 +--
qdev-monitor.c | 1 +
9 files changed, 455 insertions(+), 20 deletions(-)
create mode 100644 hw/virtio/virtio-pstore.c
create mode 100644 include/hw/virtio/virtio-pstore.h
copy include/standard-headers/linux/{virtio_ids.h => virtio_pstore.h} (63%)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 3e2b175..aae7082 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
obj-y += virtio.o virtio-balloon.o
-obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
+obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 2b34b43..8281b80 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2416,6 +2416,55 @@ static const TypeInfo virtio_host_pci_info = {
};
#endif
+/* virtio-pstore-pci */
+
+static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+ VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
+ DeviceState *vdev = DEVICE(&vps->vdev);
+ Error *err = NULL;
+
+ qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
+ object_property_set_bool(OBJECT(vdev), true, "realized", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+}
+
+static void virtio_pstore_pci_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+ PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+
+ k->realize = virtio_pstore_pci_realize;
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+
+ pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+ pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PSTORE;
+ pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
+ pcidev_k->class_id = PCI_CLASS_OTHERS;
+}
+
+static void virtio_pstore_pci_instance_init(Object *obj)
+{
+ VirtIOPstorePCI *dev = VIRTIO_PSTORE_PCI(obj);
+
+ virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+ TYPE_VIRTIO_PSTORE);
+ object_property_add_alias(obj, "directory", OBJECT(&dev->vdev),
+ "directory", &error_abort);
+}
+
+static const TypeInfo virtio_pstore_pci_info = {
+ .name = TYPE_VIRTIO_PSTORE_PCI,
+ .parent = TYPE_VIRTIO_PCI,
+ .instance_size = sizeof(VirtIOPstorePCI),
+ .instance_init = virtio_pstore_pci_instance_init,
+ .class_init = virtio_pstore_pci_class_init,
+};
+
/* virtio-pci-bus */
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
@@ -2485,6 +2534,7 @@ static void virtio_pci_register_types(void)
#ifdef CONFIG_VHOST_SCSI
type_register_static(&vhost_scsi_pci_info);
#endif
+ type_register_static(&virtio_pstore_pci_info);
}
type_init(virtio_pci_register_types)
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index e4548c2..b4c039f 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -31,6 +31,7 @@
#ifdef CONFIG_VHOST_SCSI
#include "hw/virtio/vhost-scsi.h"
#endif
+#include "hw/virtio/virtio-pstore.h"
typedef struct VirtIOPCIProxy VirtIOPCIProxy;
typedef struct VirtIOBlkPCI VirtIOBlkPCI;
@@ -44,6 +45,7 @@ typedef struct VirtIOInputPCI VirtIOInputPCI;
typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
typedef struct VirtIOGPUPCI VirtIOGPUPCI;
+typedef struct VirtIOPstorePCI VirtIOPstorePCI;
/* virtio-pci-bus */
@@ -311,6 +313,18 @@ struct VirtIOGPUPCI {
VirtIOGPU vdev;
};
+/*
+ * virtio-pstore-pci: This extends VirtioPCIProxy.
+ */
+#define TYPE_VIRTIO_PSTORE_PCI "virtio-pstore-pci"
+#define VIRTIO_PSTORE_PCI(obj) \
+ OBJECT_CHECK(VirtIOPstorePCI, (obj), TYPE_VIRTIO_PSTORE_PCI)
+
+struct VirtIOPstorePCI {
+ VirtIOPCIProxy parent_obj;
+ VirtIOPstore vdev;
+};
+
/* Virtio ABI version, if we increment this, we break the guest driver. */
#define VIRTIO_PCI_ABI_VERSION 0
diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
new file mode 100644
index 0000000..98cee7f
--- /dev/null
+++ b/hw/virtio/virtio-pstore.c
@@ -0,0 +1,328 @@
+/*
+ * Virtio Pstore Device
+ *
+ * Copyright (C) 2016 LG Electronics
+ *
+ * Authors:
+ * Namhyung Kim <namhyung@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include <stdio.h>
+
+#include "qemu/osdep.h"
+#include "qemu/iov.h"
+#include "qemu-common.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "sysemu/kvm.h"
+#include "qapi/visitor.h"
+#include "qapi-event.h"
+#include "trace.h"
+
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
+#include "hw/virtio/virtio-pstore.h"
+
+
+static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t sz,
+ struct virtio_pstore_hdr *hdr)
+{
+ const char *basename;
+
+ switch (hdr->type) {
+ case VIRTIO_PSTORE_TYPE_DMESG:
+ basename = "dmesg";
+ break;
+ default:
+ basename = "unknown";
+ break;
+ }
+
+ snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename,
+ (unsigned long long) hdr->id,
+ hdr->flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");
+}
+
+static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
+ char *buf, size_t sz,
+ struct virtio_pstore_hdr *hdr)
+{
+ size_t len = strlen(name);
+
+ hdr->flags = 0;
+ if (!strncmp(name + len - 6, ".enc.z", 6)) {
+ hdr->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
+ }
+
+ snprintf(buf, sz, "%s/%s", s->directory, name);
+
+ if (!strncmp(name, "dmesg-", 6)) {
+ hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_DMESG);
+ name += 6;
+ } else if (!strncmp(name, "unknown-", 8)) {
+ hdr->type = cpu_to_le16(VIRTIO_PSTORE_TYPE_UNKNOWN);
+ name += 8;
+ }
+
+ qemu_strtoull(name, NULL, 0, &hdr->id);
+}
+
+static ssize_t virtio_pstore_do_open(VirtIOPstore *s)
+{
+ s->dir = opendir(s->directory);
+ if (s->dir == NULL) {
+ return -1;
+ }
+
+ return 0;
+}
+
+static ssize_t virtio_pstore_do_read(VirtIOPstore *s, void *buf, size_t sz,
+ struct virtio_pstore_hdr *hdr)
+{
+ char path[PATH_MAX];
+ FILE *fp;
+ ssize_t len;
+ struct stat stbuf;
+ struct dirent *dent;
+
+ if (s->dir == NULL) {
+ return -1;
+ }
+
+ dent = readdir(s->dir);
+ while (dent) {
+ if (dent->d_name[0] != '.') {
+ break;
+ }
+ dent = readdir(s->dir);
+ }
+
+ if (dent == NULL) {
+ return 0;
+ }
+
+ virtio_pstore_from_filename(s, dent->d_name, path, sizeof(path), hdr);
+ if (stat(path, &stbuf) < 0) {
+ return -1;
+ }
+
+ fp = fopen(path, "r");
+ if (fp == NULL) {
+ error_report("cannot open %s (%p %p)", path, s, s->directory);
+ return -1;
+ }
+
+ len = fread(buf, 1, sz, fp);
+ if (len < 0 && errno == EAGAIN) {
+ len = 0;
+ }
+
+ hdr->id = cpu_to_le64(hdr->id);
+ hdr->flags = cpu_to_le32(hdr->flags);
+ hdr->time_sec = cpu_to_le64(stbuf.st_ctim.tv_sec);
+ hdr->time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
+
+ fclose(fp);
+ return len;
+}
+
+static ssize_t virtio_pstore_do_write(VirtIOPstore *s, void *buf, size_t sz,
+ struct virtio_pstore_hdr *hdr)
+{
+ char path[PATH_MAX];
+ FILE *fp;
+
+ virtio_pstore_to_filename(s, path, sizeof(path), hdr);
+
+ fp = fopen(path, "w");
+ if (fp == NULL) {
+ error_report("cannot open %s (%p %p)", path, s, s->directory);
+ return -1;
+ }
+ fwrite(buf, 1, sz, fp);
+ fclose(fp);
+
+ return sz;
+}
+
+static ssize_t virtio_pstore_do_close(VirtIOPstore *s)
+{
+ if (s->dir == NULL) {
+ return 0;
+ }
+
+ closedir(s->dir);
+ s->dir = NULL;
+
+ return 0;
+}
+
+static ssize_t virtio_pstore_do_erase(VirtIOPstore *s,
+ struct virtio_pstore_hdr *hdr)
+{
+ char path[PATH_MAX];
+
+ virtio_pstore_to_filename(s, path, sizeof(path), hdr);
+
+ return unlink(path);
+}
+
+static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
+{
+ VirtIOPstore *s = VIRTIO_PSTORE(vdev);
+ VirtQueueElement *elem;
+ struct virtio_pstore_hdr *hdr;
+ ssize_t len;
+
+ for (;;) {
+ elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+ if (!elem) {
+ return;
+ }
+
+ hdr = elem->out_sg[0].iov_base;
+ if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
+ error_report("invalid header size: %u",
+ (unsigned)elem->out_sg[0].iov_len);
+ exit(1);
+ }
+
+ switch (hdr->cmd) {
+ case VIRTIO_PSTORE_CMD_OPEN:
+ len = virtio_pstore_do_open(s);
+ break;
+ case VIRTIO_PSTORE_CMD_READ:
+ len = virtio_pstore_do_read(s, elem->in_sg[0].iov_base,
+ elem->in_sg[0].iov_len, hdr);
+ break;
+ case VIRTIO_PSTORE_CMD_WRITE:
+ len = virtio_pstore_do_write(s, elem->out_sg[1].iov_base,
+ elem->out_sg[1].iov_len, hdr);
+ break;
+ case VIRTIO_PSTORE_CMD_CLOSE:
+ len = virtio_pstore_do_close(s);
+ break;
+ case VIRTIO_PSTORE_CMD_ERASE:
+ len = virtio_pstore_do_erase(s, hdr);
+ break;
+ default:
+ len = -1;
+ break;
+ }
+
+ if (len < 0) {
+ return;
+ }
+
+ virtqueue_push(vq, elem, len);
+
+ virtio_notify(vdev, vq);
+ g_free(elem);
+ }
+}
+
+static void virtio_pstore_device_realize(DeviceState *dev, Error **errp)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+ VirtIOPstore *s = VIRTIO_PSTORE(dev);
+
+ virtio_init(vdev, "virtio-pstore", VIRTIO_ID_PSTORE, 0);
+
+ s->vq = virtio_add_queue(vdev, 128, virtio_pstore_handle_io);
+}
+
+static void virtio_pstore_device_unrealize(DeviceState *dev, Error **errp)
+{
+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+ virtio_cleanup(vdev);
+}
+
+static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
+{
+ return f;
+}
+
+static void pstore_get_directory(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ VirtIOPstore *s = opaque;
+
+ visit_type_str(v, name, &s->directory, errp);
+}
+
+static void pstore_set_directory(Object *obj, Visitor *v,
+ const char *name, void *opaque,
+ Error **errp)
+{
+ VirtIOPstore *s = opaque;
+ Error *local_err = NULL;
+ char *value;
+
+ visit_type_str(v, name, &value, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ g_free(s->directory);
+ s->directory = strdup(value);
+
+ g_free(value);
+}
+
+static void pstore_release_directory(Object *obj, const char *name,
+ void *opaque)
+{
+ VirtIOPstore *s = opaque;
+
+ g_free(s->directory);
+ s->directory = NULL;
+}
+
+static Property virtio_pstore_properties[] = {
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_pstore_instance_init(Object *obj)
+{
+ VirtIOPstore *s = VIRTIO_PSTORE(obj);
+
+ object_property_add(obj, "directory", "str",
+ pstore_get_directory, pstore_set_directory,
+ pstore_release_directory, s, NULL);
+}
+
+static void virtio_pstore_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+ dc->props = virtio_pstore_properties;
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+ vdc->realize = virtio_pstore_device_realize;
+ vdc->unrealize = virtio_pstore_device_unrealize;
+ vdc->get_features = get_features;
+}
+
+static const TypeInfo virtio_pstore_info = {
+ .name = TYPE_VIRTIO_PSTORE,
+ .parent = TYPE_VIRTIO_DEVICE,
+ .instance_size = sizeof(VirtIOPstore),
+ .instance_init = virtio_pstore_instance_init,
+ .class_init = virtio_pstore_class_init,
+};
+
+static void virtio_register_types(void)
+{
+ type_register_static(&virtio_pstore_info);
+}
+
+type_init(virtio_register_types)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 9ed1624..5689c6f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -79,6 +79,7 @@
#define PCI_DEVICE_ID_VIRTIO_SCSI 0x1004
#define PCI_DEVICE_ID_VIRTIO_RNG 0x1005
#define PCI_DEVICE_ID_VIRTIO_9P 0x1009
+#define PCI_DEVICE_ID_VIRTIO_PSTORE 0x100a
#define PCI_VENDOR_ID_REDHAT 0x1b36
#define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001
diff --git a/include/hw/virtio/virtio-pstore.h b/include/hw/virtio/virtio-pstore.h
new file mode 100644
index 0000000..74cd1f6
--- /dev/null
+++ b/include/hw/virtio/virtio-pstore.h
@@ -0,0 +1,30 @@
+/*
+ * Virtio Pstore Support
+ *
+ * Authors:
+ * Namhyung Kim <namhyung@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef _QEMU_VIRTIO_PSTORE_H
+#define _QEMU_VIRTIO_PSTORE_H
+
+#include "standard-headers/linux/virtio_pstore.h"
+#include "hw/virtio/virtio.h"
+#include "hw/pci/pci.h"
+
+#define TYPE_VIRTIO_PSTORE "virtio-pstore-device"
+#define VIRTIO_PSTORE(obj) \
+ OBJECT_CHECK(VirtIOPstore, (obj), TYPE_VIRTIO_PSTORE)
+
+typedef struct VirtIOPstore {
+ VirtIODevice parent_obj;
+ VirtQueue *vq;
+ char *directory;
+ DIR *dir;
+} VirtIOPstore;
+
+#endif
diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
index 77925f5..cba6322 100644
--- a/include/standard-headers/linux/virtio_ids.h
+++ b/include/standard-headers/linux/virtio_ids.h
@@ -41,5 +41,6 @@
#define VIRTIO_ID_CAIF 12 /* Virtio caif */
#define VIRTIO_ID_GPU 16 /* virtio GPU */
#define VIRTIO_ID_INPUT 18 /* virtio input */
+#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
#endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_pstore.h
similarity index 63%
copy from include/standard-headers/linux/virtio_ids.h
copy to include/standard-headers/linux/virtio_pstore.h
index 77925f5..1b89cad 100644
--- a/include/standard-headers/linux/virtio_ids.h
+++ b/include/standard-headers/linux/virtio_pstore.h
@@ -1,9 +1,6 @@
-#ifndef _LINUX_VIRTIO_IDS_H
-#define _LINUX_VIRTIO_IDS_H
-/*
- * Virtio IDs
- *
- * This header is BSD licensed so anyone can use the definitions to implement
+#ifndef _LINUX_VIRTIO_PSTORE_H
+#define _LINUX_VIRTIO_PSTORE_H
+/* This header is BSD licensed so anyone can use the definitions to implement
* compatible drivers/servers.
*
* Redistribution and use in source and binary forms, with or without
@@ -28,18 +25,31 @@
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE. */
+#include "standard-headers/linux/types.h"
+#include "standard-headers/linux/virtio_types.h"
+#include "standard-headers/linux/virtio_ids.h"
+#include "standard-headers/linux/virtio_config.h"
+
+#define VIRTIO_PSTORE_TYPE_UNKNOWN 0
+#define VIRTIO_PSTORE_TYPE_DMESG 1
+
+#define VIRTIO_PSTORE_CMD_NULL 0
+#define VIRTIO_PSTORE_CMD_OPEN 1
+#define VIRTIO_PSTORE_CMD_READ 2
+#define VIRTIO_PSTORE_CMD_WRITE 3
+#define VIRTIO_PSTORE_CMD_ERASE 4
+#define VIRTIO_PSTORE_CMD_CLOSE 5
+
+#define VIRTIO_PSTORE_FL_COMPRESSED 1
-#define VIRTIO_ID_NET 1 /* virtio net */
-#define VIRTIO_ID_BLOCK 2 /* virtio block */
-#define VIRTIO_ID_CONSOLE 3 /* virtio console */
-#define VIRTIO_ID_RNG 4 /* virtio rng */
-#define VIRTIO_ID_BALLOON 5 /* virtio balloon */
-#define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
-#define VIRTIO_ID_SCSI 8 /* virtio scsi */
-#define VIRTIO_ID_9P 9 /* 9p virtio console */
-#define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
-#define VIRTIO_ID_CAIF 12 /* Virtio caif */
-#define VIRTIO_ID_GPU 16 /* virtio GPU */
-#define VIRTIO_ID_INPUT 18 /* virtio input */
+struct virtio_pstore_hdr {
+ __virtio64 id;
+ __virtio32 flags;
+ __virtio16 cmd;
+ __virtio16 type;
+ __virtio64 time_sec;
+ __virtio32 time_nsec;
+ __virtio32 unused;
+};
-#endif /* _LINUX_VIRTIO_IDS_H */
+#endif /* _LINUX_VIRTIO_PSTORE_H */
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e19617f..e1df5a9 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -73,6 +73,7 @@ static const QDevAlias qdev_alias_table[] = {
{ "virtio-serial-pci", "virtio-serial", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
{ "virtio-tablet-ccw", "virtio-tablet", QEMU_ARCH_S390X },
{ "virtio-tablet-pci", "virtio-tablet", QEMU_ARCH_ALL & ~QEMU_ARCH_S390X },
+ { "virtio-pstore-pci", "virtio-pstore" },
{ }
};
--
2.8.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
From: Namhyung Kim @ 2016-07-18 4:37 UTC (permalink / raw)
To: LKML
Cc: Tony Luck, Radim Krčmář, Kees Cook, kvm,
Michael S. Tsirkin, Anton Vorontsov, qemu-devel, Steven Rostedt,
virtualization, Minchan Kim, Anthony Liguori, Colin Cross,
Paolo Bonzini, Ingo Molnar
In-Reply-To: <1468816661-6345-1-git-send-email-namhyung@kernel.org>
The virtio pstore driver provides interface to the pstore subsystem so
that the guest kernel's log/dump message can be saved on the host
machine. Users can access the log file directly on the host, or on the
guest at the next boot using pstore filesystem. It currently deals with
kernel log (printk) buffer only, but we can extend it to have other
information (like ftrace dump) later.
It supports legacy PCI device using single order-2 page buffer. As all
operation of pstore is synchronous, it would be fine IMHO. However I
don't know how to make write operation synchronous since it's called
with a spinlock held (from any context including NMI).
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Anthony Liguori <aliguori@amazon.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Minchan Kim <minchan@kernel.org>
Cc: kvm@vger.kernel.org
Cc: qemu-devel@nongnu.org
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
drivers/virtio/Kconfig | 10 ++
drivers/virtio/Makefile | 1 +
drivers/virtio/virtio_pstore.c | 317 +++++++++++++++++++++++++++++++++++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/virtio_ids.h | 1 +
include/uapi/linux/virtio_pstore.h | 53 +++++++
6 files changed, 383 insertions(+)
create mode 100644 drivers/virtio/virtio_pstore.c
create mode 100644 include/uapi/linux/virtio_pstore.h
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 77590320d44c..8f0e6c796c12 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -58,6 +58,16 @@ config VIRTIO_INPUT
If unsure, say M.
+config VIRTIO_PSTORE
+ tristate "Virtio pstore driver"
+ depends on VIRTIO
+ depends on PSTORE
+ ---help---
+ This driver supports virtio pstore devices to save/restore
+ panic and oops messages on the host.
+
+ If unsure, say M.
+
config VIRTIO_MMIO
tristate "Platform bus driver for memory mapped virtio devices"
depends on HAS_IOMEM && HAS_DMA
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 41e30e3dc842..bee68cb26d48 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
+obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
new file mode 100644
index 000000000000..6fe62c0f1508
--- /dev/null
+++ b/drivers/virtio/virtio_pstore.c
@@ -0,0 +1,317 @@
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pstore.h>
+#include <linux/virtio.h>
+#include <linux/virtio_config.h>
+#include <uapi/linux/virtio_ids.h>
+#include <uapi/linux/virtio_pstore.h>
+
+#define VIRT_PSTORE_ORDER 2
+#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
+
+struct virtio_pstore {
+ struct virtio_device *vdev;
+ struct virtqueue *vq;
+ struct pstore_info pstore;
+ struct virtio_pstore_hdr hdr;
+ size_t buflen;
+ u64 id;
+
+ /* Waiting for host to ack */
+ wait_queue_head_t acked;
+};
+
+static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type)
+{
+ u16 ret;
+
+ switch (type) {
+ case PSTORE_TYPE_DMESG:
+ ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG);
+ break;
+ default:
+ ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
+ break;
+ }
+
+ return ret;
+}
+
+static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type)
+{
+ enum pstore_type_id ret;
+
+ switch (virtio16_to_cpu(vps->vdev, type)) {
+ case VIRTIO_PSTORE_TYPE_DMESG:
+ ret = PSTORE_TYPE_DMESG;
+ break;
+ default:
+ ret = PSTORE_TYPE_UNKNOWN;
+ break;
+ }
+
+ return ret;
+}
+
+static void virtpstore_ack(struct virtqueue *vq)
+{
+ struct virtio_pstore *vps = vq->vdev->priv;
+
+ wake_up(&vps->acked);
+}
+
+static int virt_pstore_open(struct pstore_info *psi)
+{
+ struct virtio_pstore *vps = psi->data;
+ struct virtio_pstore_hdr *hdr = &vps->hdr;
+ struct scatterlist sg[1];
+ unsigned int len;
+
+ hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN);
+
+ sg_init_one(sg, hdr, sizeof(*hdr));
+ virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL);
+ virtqueue_kick(vps->vq);
+
+ wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
+ return 0;
+}
+
+static int virt_pstore_close(struct pstore_info *psi)
+{
+ struct virtio_pstore *vps = psi->data;
+ struct virtio_pstore_hdr *hdr = &vps->hdr;
+ struct scatterlist sg[1];
+ unsigned int len;
+
+ hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_CLOSE);
+
+ sg_init_one(sg, hdr, sizeof(*hdr));
+ virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL);
+ virtqueue_kick(vps->vq);
+
+ wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
+ return 0;
+}
+
+static ssize_t virt_pstore_read(u64 *id, enum pstore_type_id *type,
+ int *count, struct timespec *time,
+ char **buf, bool *compressed,
+ struct pstore_info *psi)
+{
+ struct virtio_pstore *vps = psi->data;
+ struct virtio_pstore_hdr *hdr = &vps->hdr;
+ struct scatterlist sgi[1], sgo[1];
+ struct scatterlist *sgs[2] = { sgo, sgi };
+ unsigned int len;
+ unsigned int flags;
+ void *bf;
+
+ hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_READ);
+
+ sg_init_one(sgo, hdr, sizeof(*hdr));
+ sg_init_one(sgi, psi->buf, psi->bufsize);
+ virtqueue_add_sgs(vps->vq, sgs, 1, 1, vps, GFP_KERNEL);
+ virtqueue_kick(vps->vq);
+
+ wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
+ if (len == 0)
+ return 0;
+
+ bf = kmalloc(len, GFP_KERNEL);
+ if (bf == NULL)
+ return -ENOMEM;
+
+ *id = virtio64_to_cpu(vps->vdev, hdr->id);
+ *type = from_virtio_type(vps, hdr->type);
+
+ flags = virtio32_to_cpu(vps->vdev, hdr->flags);
+ *compressed = flags & VIRTIO_PSTORE_FL_COMPRESSED;
+ *count = 1;
+
+ time->tv_sec = virtio64_to_cpu(vps->vdev, hdr->time_sec);
+ time->tv_nsec = virtio32_to_cpu(vps->vdev, hdr->time_nsec);
+
+ memcpy(bf, psi->buf, len);
+ *buf = bf;
+
+ return len;
+}
+
+static int notrace virt_pstore_write(enum pstore_type_id type,
+ enum kmsg_dump_reason reason,
+ u64 *id, unsigned int part, int count,
+ bool compressed, size_t size,
+ struct pstore_info *psi)
+{
+ struct virtio_pstore *vps = psi->data;
+ struct virtio_pstore_hdr *hdr = &vps->hdr;
+ struct scatterlist sg[2];
+ unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0;
+
+ *id = vps->id++;
+
+ hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE);
+ hdr->id = cpu_to_virtio64(vps->vdev, *id);
+ hdr->flags = cpu_to_virtio32(vps->vdev, flags);
+ hdr->type = to_virtio_type(vps, type);
+
+ sg_init_table(sg, 2);
+ sg_set_buf(&sg[0], hdr, sizeof(*hdr));
+ sg_set_buf(&sg[1], psi->buf, size);
+ virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC);
+ virtqueue_kick(vps->vq);
+
+ /* TODO: make it synchronous */
+ return 0;
+}
+
+static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count,
+ struct timespec time, struct pstore_info *psi)
+{
+ struct virtio_pstore *vps = psi->data;
+ struct virtio_pstore_hdr *hdr = &vps->hdr;
+ struct scatterlist sg[1];
+ unsigned int len;
+
+ hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE);
+ hdr->id = cpu_to_virtio64(vps->vdev, id);
+ hdr->type = to_virtio_type(vps, type);
+
+ sg_init_one(sg, hdr, sizeof(*hdr));
+ virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL);
+ virtqueue_kick(vps->vq);
+
+ wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len));
+ return 0;
+}
+
+static int virt_pstore_init(struct virtio_pstore *vps)
+{
+ struct pstore_info *psinfo = &vps->pstore;
+ int err;
+
+ vps->id = 0;
+ vps->buflen = 0;
+ psinfo->bufsize = VIRT_PSTORE_BUFSIZE;
+ psinfo->buf = (void *)__get_free_pages(GFP_KERNEL, VIRT_PSTORE_ORDER);
+ if (!psinfo->buf) {
+ pr_err("cannot allocate pstore buffer\n");
+ return -ENOMEM;
+ }
+
+ psinfo->owner = THIS_MODULE;
+ psinfo->name = "virtio";
+ psinfo->open = virt_pstore_open;
+ psinfo->close = virt_pstore_close;
+ psinfo->read = virt_pstore_read;
+ psinfo->erase = virt_pstore_erase;
+ psinfo->write = virt_pstore_write;
+ psinfo->flags = PSTORE_FLAGS_FRAGILE;
+ psinfo->data = vps;
+ spin_lock_init(&psinfo->buf_lock);
+
+ err = pstore_register(psinfo);
+ if (err)
+ kfree(psinfo->buf);
+
+ return err;
+}
+
+static int virt_pstore_exit(struct virtio_pstore *vps)
+{
+ struct pstore_info *psinfo = &vps->pstore;
+
+ pstore_unregister(psinfo);
+
+ free_pages((unsigned long)psinfo->buf, VIRT_PSTORE_ORDER);
+ psinfo->bufsize = 0;
+
+ return 0;
+}
+
+static int virtpstore_probe(struct virtio_device *vdev)
+{
+ struct virtio_pstore *vps;
+ int err;
+
+ if (!vdev->config->get) {
+ dev_err(&vdev->dev, "%s failure: config access disabled\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ vdev->priv = vps = kmalloc(sizeof(*vps), GFP_KERNEL);
+ if (!vps) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ vps->vdev = vdev;
+
+ vps->vq = virtio_find_single_vq(vdev, virtpstore_ack, "pstore");
+ if (IS_ERR(vps->vq)) {
+ err = PTR_ERR(vps->vq);
+ goto out_free;
+ }
+
+ err = virt_pstore_init(vps);
+ if (err)
+ goto out_del_vq;
+
+ init_waitqueue_head(&vps->acked);
+
+ virtio_device_ready(vdev);
+ dev_info(&vdev->dev, "virtio pstore driver init: ok\n");
+
+ return 0;
+
+out_del_vq:
+ vdev->config->del_vqs(vdev);
+out_free:
+ kfree(vps);
+out:
+ dev_err(&vdev->dev, "virtio pstore driver init: failed with %d\n", err);
+ return err;
+}
+
+static void virtpstore_remove(struct virtio_device *vdev)
+{
+ struct virtio_pstore *vps = vdev->priv;
+
+ virt_pstore_exit(vps);
+
+ /* Now we reset the device so we can clean up the queues. */
+ vdev->config->reset(vdev);
+
+ vdev->config->del_vqs(vdev);
+
+ kfree(vps);
+}
+
+static unsigned int features[] = {
+};
+
+static struct virtio_device_id id_table[] = {
+ { VIRTIO_ID_PSTORE, VIRTIO_DEV_ANY_ID },
+ { 0 },
+};
+
+static struct virtio_driver virtio_pstore_driver = {
+ .driver.name = KBUILD_MODNAME,
+ .driver.owner = THIS_MODULE,
+ .feature_table = features,
+ .feature_table_size = ARRAY_SIZE(features),
+ .id_table = id_table,
+ .probe = virtpstore_probe,
+ .remove = virtpstore_remove,
+};
+
+module_virtio_driver(virtio_pstore_driver);
+MODULE_DEVICE_TABLE(virtio, id_table);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Namhyung Kim <namhyung@kernel.org>");
+MODULE_DESCRIPTION("Virtio pstore driver");
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 8bdae34d1f9a..57b0d08db322 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -448,6 +448,7 @@ header-y += virtio_ids.h
header-y += virtio_input.h
header-y += virtio_net.h
header-y += virtio_pci.h
+header-y += virtio_pstore.h
header-y += virtio_ring.h
header-y += virtio_rng.h
header-y += virtio_scsi.h
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index 77925f587b15..cba63225d85a 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -41,5 +41,6 @@
#define VIRTIO_ID_CAIF 12 /* Virtio caif */
#define VIRTIO_ID_GPU 16 /* virtio GPU */
#define VIRTIO_ID_INPUT 18 /* virtio input */
+#define VIRTIO_ID_PSTORE 19 /* virtio pstore */
#endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/include/uapi/linux/virtio_pstore.h b/include/uapi/linux/virtio_pstore.h
new file mode 100644
index 000000000000..0aa1575ee35f
--- /dev/null
+++ b/include/uapi/linux/virtio_pstore.h
@@ -0,0 +1,53 @@
+#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 <linux/types.h>
+#include <linux/virtio_types.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_FL_COMPRESSED 1
+
+struct virtio_pstore_hdr {
+ __virtio64 id;
+ __virtio32 flags;
+ __virtio16 cmd;
+ __virtio16 type;
+ __virtio64 time_sec;
+ __virtio32 time_nsec;
+ __virtio32 unused;
+};
+
+#endif /* _LINUX_VIRTIO_PSTORE_H */
--
2.8.0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [RFC/PATCHSET 0/3] virtio-pstore: Implement virtio pstore device
From: Namhyung Kim @ 2016-07-18 4:37 UTC (permalink / raw)
To: LKML
Cc: Tony Luck, Radim Krčmář, Kees Cook, kvm,
Michael S. Tsirkin, Anton Vorontsov, qemu-devel, Steven Rostedt,
virtualization, Minchan Kim, Anthony Liguori, Colin Cross,
Paolo Bonzini, Ingo Molnar
Hello,
This patchset is a proof of concept of virtio-pstore idea [1]. It has
some rough edges and I'm not familiar with this area, so please give
me feedbacks and advices if I'm going to a wrong direction.
It started from the fact that dumping ftrace buffer at kernel
oops/panic takes too much time. Although there's a way to reduce the
size of the original data, sometimes I want to have the information as
many as possible. Maybe kexec/kdump can solve this problem but it
consumes some portion of guest memory so I'd like to avoid it. And I
know the qemu + crashtool can dump and analyze the whole guest memory
including the ftrace buffer without wasting guest memory, but it adds
one more layer and has some limitation as an out-of-tree tool like not
being in sync with the kernel changes.
So I think it'd be great using the pstore interface to dump guest
kernel data on the host. One can read the data on the host directly
or on the guest (at the next boot) using pstore filesystem as usual.
While this patchset only implements dumping kernel log buffer, it can
be extended to have ftrace buffer and probably some more..
The patch 0001 implements virtio pstore driver. It has a single virt
queue, pstore buffer and header structure. The virtio_pstore_hdr
struct is to give information about the current pstore operation.
The patch 0002 and 0003 implement virtio-pstore legacy PCI device on
qemu-kvm and kvmtool respectively. I referenced virtio-baloon and
virtio-rng implementations and I don't know whether kvmtool supports
modern virtio 1.0+ spec.
For example, using virtio-pstore on qemu looks like below:
$ qemu-system-x86_64 -enable-kvm -device virtio-pstore,directory=xxx
When guest kernel gets panic the log messages will be saved under the
xxx directory.
$ ls xxx
dmesg-0.enc.z dmesg-1.enc.z
As you can see the pstore subsystem compresses the log data using
zlib. The data can be extracted with the following command:
$ cat xxx/dmesg-0.enc.z | \
> python -c 'import sys, zlib; print(zlib.decompress(sys.stdin.read()))'
Oops#1 Part1
<5>[ 0.000000] Linux version 4.6.0kvm+ (namhyung@danjae) (gcc version 5.3.0 (GCC) ) #145 SMP Mon Jul 18 10:22:45 KST 2016
<6>[ 0.000000] Command line: root=/dev/vda console=ttyS0
<6>[ 0.000000] x86/fpu: Legacy x87 FPU detected.
<6>[ 0.000000] x86/fpu: Using 'eager' FPU context switches.
<6>[ 0.000000] e820: BIOS-provided physical RAM map:
<6>[ 0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009fbff] usable
<6>[ 0.000000] BIOS-e820: [mem 0x000000000009fc00-0x000000000009ffff] reserved
<6>[ 0.000000] BIOS-e820: [mem 0x00000000000f0000-0x00000000000fffff] reserved
<6>[ 0.000000] BIOS-e820: [mem 0x0000000000100000-0x0000000007fddfff] usable
<6>[ 0.000000] BIOS-e820: [mem 0x0000000007fde000-0x0000000007ffffff] reserved
<6>[ 0.000000] BIOS-e820: [mem 0x00000000feffc000-0x00000000feffffff] reserved
<6>[ 0.000000] BIOS-e820: [mem 0x00000000fffc0000-0x00000000ffffffff] reserved
<6>[ 0.000000] NX (Execute Disable) protection: active
<6>[ 0.000000] SMBIOS 2.8 present.
<7>[ 0.000000] DMI: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
...
Maybe we can add a config option to control the compression later.
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
[1] https://lkml.org/lkml/2016/7/1/6
Thanks,
Namhyung
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 2/4] powerpc/spinlock: support vcpu preempted check
From: Pan Xinhui @ 2016-07-15 15:35 UTC (permalink / raw)
To: Balbir Singh, Pan Xinhui, linux-kernel, linuxppc-dev,
virtualization, linux-s390
Cc: dave, peterz, mpe, boqun.feng, will.deacon, waiman.long, mingo,
paulus, benh, schwidefsky, paulmck
In-Reply-To: <1467802454.9143.1.camel@gmail.com>
Hi, Baibir
sorry for late responce, I missed reading your mail.
在 16/7/6 18:54, Balbir Singh 写道:
> On Tue, 2016-06-28 at 10:43 -0400, Pan Xinhui wrote:
>> This is to fix some lock holder preemption issues. Some other locks
>> implementation do a spin loop before acquiring the lock itself. Currently
>> kernel has an interface of bool vcpu_is_preempted(int cpu). It take the cpu
> ^^ takes
>> as parameter and return true if the cpu is preempted. Then kernel can break
>> the spin loops upon on the retval of vcpu_is_preempted.
>>
>> As kernel has used this interface, So lets support it.
>>
>> Only pSeries need supoort it. And the fact is powerNV are built into same
> ^^ support
>> kernel image with pSeries. So we need return false if we are runnig as
>> powerNV. The another fact is that lppaca->yiled_count keeps zero on
> ^^ yield
>> powerNV. So we can just skip the machine type.
>>
Blame on me, I indeed need avoid such typo..
thanks for pointing it out.
>> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>> Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/spinlock.h | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
>> index 523673d..3ac9fcb 100644
>> --- a/arch/powerpc/include/asm/spinlock.h
>> +++ b/arch/powerpc/include/asm/spinlock.h
>> @@ -52,6 +52,24 @@
>> #define SYNC_IO
>> #endif
>>
>> +/*
>> + * This support kernel to check if one cpu is preempted or not.
>> + * Then we can fix some lock holder preemption issue.
>> + */
>> +#ifdef CONFIG_PPC_PSERIES
>> +#define vcpu_is_preempted vcpu_is_preempted
>> +static inline bool vcpu_is_preempted(int cpu)
>> +{
>> + /*
>> + * pSeries and powerNV can be built into same kernel image. In
>> + * principle we need return false directly if we are running as
>> + * powerNV. However the yield_count is always zero on powerNV, So
>> + * skip such machine type check
>
> Or you could use the ppc_md interface callbacks if required, but your
> solution works as well
>
thanks, So I can keep my code as is.
thanks
xinhui
>> + */
>> + return !!(be32_to_cpu(lppaca_of(cpu).yield_count) & 1);
>> +}
>> +#endif
>> +
>> static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
>> {
>> return lock.slock == 0;
>
>
> Balbir Singh.
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio: Return correct errno for function init_vq's failure
From: Cornelia Huck @ 2016-07-13 12:05 UTC (permalink / raw)
To: Minfei Huang; +Cc: Minfei Huang, virtualization, linux-kernel, mst
In-Reply-To: <20160713115400.GA16403@MinfeideMacBook-Pro.local>
On Wed, 13 Jul 2016 19:54:00 +0800
Minfei Huang <mnghuan@gmail.com> wrote:
> On 07/06/16 at 11:18P, Cornelia Huck wrote:
> > On Mon, 27 Jun 2016 10:09:18 +0800
> > Minfei Huang <mnghuan@gmail.com> wrote:
> >
> > > The error number -ENOENT or 0 will be returned, if we can not allocate
> > > more memory in function init_vq. If host can support multiple virtual
> > > queues, and we fails to allocate necessary memory structures for vq,
> > > kernel may crash due to incorrect returning.
> > >
> > > To fix it, kernel will return correct value in init_vq.
> > The error handling in this function looks horrible.
> >
> > When mq was introduced, init_vq started mixing up several things:
> > - The mq feature is not available - which is not an error, and
> > therefore should not have any influence on the return code.
> > - One of the several memory allocations failed - only ->vqs gets
> > special treatment, however.
> > - The ->find_vqs callback failed.
>
> Yep. And without this patch, it is silent for boot failure. I think it
> makes sense to let user notify about this failure.
Agreed.
>
> >
> > Your patch fixes the code, but it is still very convoluted due to the
> > temporary arrays.
> >
> > May it be worthwile to introduce a helper for setting up the virtqueues
> > where all virtqueues are essentially the same and just get a
> > consecutive number? Michael?
> >
>
> Hmm, How about refactor this function to make it more readable, since we
> do a lot of work in it.
>
> I will post an update to refactor this function.
Anything to make this more readable probably helps :)
^ permalink raw reply
* Re: [PATCH] virtio: Return correct errno for function init_vq's failure
From: Minfei Huang @ 2016-07-13 11:54 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Minfei Huang, virtualization, linux-kernel, mst
In-Reply-To: <20160706111832.5d4d6f43.cornelia.huck@de.ibm.com>
On 07/06/16 at 11:18P, Cornelia Huck wrote:
> On Mon, 27 Jun 2016 10:09:18 +0800
> Minfei Huang <mnghuan@gmail.com> wrote:
>
> > The error number -ENOENT or 0 will be returned, if we can not allocate
> > more memory in function init_vq. If host can support multiple virtual
> > queues, and we fails to allocate necessary memory structures for vq,
> > kernel may crash due to incorrect returning.
> >
> > To fix it, kernel will return correct value in init_vq.
> The error handling in this function looks horrible.
>
> When mq was introduced, init_vq started mixing up several things:
> - The mq feature is not available - which is not an error, and
> therefore should not have any influence on the return code.
> - One of the several memory allocations failed - only ->vqs gets
> special treatment, however.
> - The ->find_vqs callback failed.
Yep. And without this patch, it is silent for boot failure. I think it
makes sense to let user notify about this failure.
>
> Your patch fixes the code, but it is still very convoluted due to the
> temporary arrays.
>
> May it be worthwile to introduce a helper for setting up the virtqueues
> where all virtqueues are essentially the same and just get a
> consecutive number? Michael?
>
Hmm, How about refactor this function to make it more readable, since we
do a lot of work in it.
I will post an update to refactor this function.
Thanks
Minfei
^ permalink raw reply
* WorldCIST'17 - Call for Workshops Proposals
From: ML @ 2016-07-13 10:28 UTC (permalink / raw)
To: virtualization
[-- Attachment #1: Type: text/plain, Size: 3538 bytes --]
---------
WorldCIST'17 - 5th World Conference on Information Systems and Technologies
Porto Santo Island, Madeira, Portugal
11th-13th of April 2017
http://www.worldcist.org/
-------------------------------------------
WORKSHOP FORMAT
The Information Systems and Technologies research and industrial community is invited to submit proposals of Workshops for WorldCist'17 – 5th World Conference on Information Systems and Technologies to be held at Porto Santo Island, Madeira, Portugal, 11th - 13th of April 2017: http://www.worldcist.org/
Workshops should focus on a specific scientific subject on the scope of WorldCist'17 but not directly included on the main conference areas. Each workshop will be coordinated by an Organizing Committee composed of, at least, two researchers in the field, preferably from different institutions and different countries. The organizers should create an international Program Committee for the Workshop, with recognized researchers within the specific Workshop scientific area. Each workshop should have at least 10 submissions and 5 accepted papers in order to be conducted at WorldCist'17.
The selection of Workshops will be performed by WorldCist'17 Conference/Workshop Chairs. Workshops full and short papers will be published in the conference main proceedings in specific Workshop chapters published by Springer in a book of the AISC series. Proceedings will be submitted for indexation by ISI Thomson, SCOPUS, DBLP, EI-Compendex among several other scientific databases. Extended versions of best selected papers will be published in journals indexed by ISI/SCI, SCOPUS and DBLP. Detailed and up-to-date information may be found at WorldCist'17 website: http://www.worldcist.org/
WORKSHOP ORGANIZATION
The Organizing Committee of each Workshop will be responsible for:
- Producing and distributing the Workshop Call for Papers (CFP);
- Coordinating the review and selection process for the papers submitted to the Workshop, as Workshop chairs (on the paper submission system to be installed);
- Delivering the final versions of the papers accepted for the Workshop in accordance with the guidelines and deadlines defined by WorldCist'17 organizers;
- Coordinating and chairing the Workshop sessions at the conference.
WorldCist'17 organizers reserve the right to cancel any Workshop if deadlines are missed or if the number of registered attendees is too low to support the costs associated with the Workshop.
PROPOSAL CONTENT
Workshop proposals should contain the following information:
- Workshop title;
- Brief description of the specific scientific scope of the Workshop;
- List of topics of interest (max 15 topics);
- Reasons the Workshop should be held within WorldCist’17;
- Name, postal address, phone and email of all the members of the Workshop Organizing Committee;
- Proposal for the Workshop Program Committee (Names and affiliations).
Proposals should be submitted electronically by email to worldcist at gmail.com (cc: lpreis at dsi.uminho.pt), in PDF, (in English), by July 31, 2016.
IMPORTANT DATES
- Deadline for Workshop proposals: July 31, 2016
- Notification of Workshop acceptance: September 11, 2016
- Deadline for paper submission: November 27, 2016
- Notification of paper acceptance: December 25, 2016
- Deadline for final versions and conference registration: January 8, 2017
- Conference dates: April 11-13, 2017
WorldCIST'17
http://www.worldcist.org/
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [WorldCIST'17]: Call for Workshops Proposals
From: ML @ 2016-07-13 9:34 UTC (permalink / raw)
To: virtualization
[-- Attachment #1: Type: text/plain, Size: 3538 bytes --]
---------
WorldCIST'17 - 5th World Conference on Information Systems and Technologies
Porto Santo Island, Madeira, Portugal
11th-13th of April 2017
http://www.worldcist.org/
-------------------------------------------
WORKSHOP FORMAT
The Information Systems and Technologies research and industrial community is invited to submit proposals of Workshops for WorldCist'17 – 5th World Conference on Information Systems and Technologies to be held at Porto Santo Island, Madeira, Portugal, 11th - 13th of April 2017: http://www.worldcist.org/
Workshops should focus on a specific scientific subject on the scope of WorldCist'17 but not directly included on the main conference areas. Each workshop will be coordinated by an Organizing Committee composed of, at least, two researchers in the field, preferably from different institutions and different countries. The organizers should create an international Program Committee for the Workshop, with recognized researchers within the specific Workshop scientific area. Each workshop should have at least 10 submissions and 5 accepted papers in order to be conducted at WorldCist'17.
The selection of Workshops will be performed by WorldCist'17 Conference/Workshop Chairs. Workshops full and short papers will be published in the conference main proceedings in specific Workshop chapters published by Springer in a book of the AISC series. Proceedings will be submitted for indexation by ISI Thomson, SCOPUS, DBLP, EI-Compendex among several other scientific databases. Extended versions of best selected papers will be published in journals indexed by ISI/SCI, SCOPUS and DBLP. Detailed and up-to-date information may be found at WorldCist'17 website: http://www.worldcist.org/
WORKSHOP ORGANIZATION
The Organizing Committee of each Workshop will be responsible for:
- Producing and distributing the Workshop Call for Papers (CFP);
- Coordinating the review and selection process for the papers submitted to the Workshop, as Workshop chairs (on the paper submission system to be installed);
- Delivering the final versions of the papers accepted for the Workshop in accordance with the guidelines and deadlines defined by WorldCist'17 organizers;
- Coordinating and chairing the Workshop sessions at the conference.
WorldCist'17 organizers reserve the right to cancel any Workshop if deadlines are missed or if the number of registered attendees is too low to support the costs associated with the Workshop.
PROPOSAL CONTENT
Workshop proposals should contain the following information:
- Workshop title;
- Brief description of the specific scientific scope of the Workshop;
- List of topics of interest (max 15 topics);
- Reasons the Workshop should be held within WorldCist’17;
- Name, postal address, phone and email of all the members of the Workshop Organizing Committee;
- Proposal for the Workshop Program Committee (Names and affiliations).
Proposals should be submitted electronically by email to worldcist at gmail.com (cc: lpreis at dsi.uminho.pt), in PDF, (in English), by July 31, 2016.
IMPORTANT DATES
- Deadline for Workshop proposals: July 31, 2016
- Notification of Workshop acceptance: September 11, 2016
- Deadline for paper submission: November 27, 2016
- Notification of paper acceptance: December 25, 2016
- Deadline for final versions and conference registration: January 8, 2017
- Conference dates: April 11-13, 2017
WorldCIST'17
http://www.worldcist.org/
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 0/4] implement vcpu preempted check
From: Waiman Long @ 2016-07-12 18:16 UTC (permalink / raw)
To: Juergen Gross
Cc: linux-s390, dave, Peter Zijlstra, Pan Xinhui, boqun.feng,
will.deacon, linux-kernel, virtualization, mingo, paulus, mpe,
benh, schwidefsky, pbonzini, paulmck, linuxppc-dev
In-Reply-To: <57846F02.7000306@suse.com>
On 07/12/2016 12:16 AM, Juergen Gross wrote:
> On 11/07/16 17:10, Waiman Long wrote:
>> On 07/06/2016 02:52 AM, Peter Zijlstra wrote:
>>> On Tue, Jun 28, 2016 at 10:43:07AM -0400, Pan Xinhui wrote:
>>>> change fomr v1:
>>>> a simplier definition of default vcpu_is_preempted
>>>> skip mahcine type check on ppc, and add config. remove dedicated
>>>> macro.
>>>> add one patch to drop overload of rwsem_spin_on_owner and
>>>> mutex_spin_on_owner.
>>>> add more comments
>>>> thanks boqun and Peter's suggestion.
>>>>
>>>> This patch set aims to fix lock holder preemption issues.
>>>>
>>>> test-case:
>>>> perf record -a perf bench sched messaging -g 400 -p&& perf report
>>>>
>>>> 18.09% sched-messaging [kernel.vmlinux] [k] osq_lock
>>>> 12.28% sched-messaging [kernel.vmlinux] [k] rwsem_spin_on_owner
>>>> 5.27% sched-messaging [kernel.vmlinux] [k] mutex_unlock
>>>> 3.89% sched-messaging [kernel.vmlinux] [k] wait_consider_task
>>>> 3.64% sched-messaging [kernel.vmlinux] [k] _raw_write_lock_irq
>>>> 3.41% sched-messaging [kernel.vmlinux] [k] mutex_spin_on_owner.is
>>>> 2.49% sched-messaging [kernel.vmlinux] [k] system_call
>>>>
>>>> We introduce interface bool vcpu_is_preempted(int cpu) and use it in
>>>> some spin
>>>> loops of osq_lock, rwsem_spin_on_owner and mutex_spin_on_owner.
>>>> These spin_on_onwer variant also cause rcu stall before we apply this
>>>> patch set
>>>>
>>> Paolo, could you help out with an (x86) KVM interface for this?
>>>
>>> Waiman, could you see if you can utilize this to get rid of the
>>> SPIN_THRESHOLD in qspinlock_paravirt?
>> That API is certainly useful to make the paravirt spinlock perform
>> better. However, I am not sure if we can completely get rid of the
>> SPIN_THRESHOLD at this point. It is not just the kvm, the xen code need
>> to be modified as well.
> This should be rather easy. The relevant information is included in the
> runstate data mapped into kernel memory. I can provide a patch for Xen
> if needed.
>
>
> Juergen
Thanks for the offering. We will wait until Xinhui's patch comes through
before working on the next step.
As for the elimination of SPIN_THRESHOLD, the queue head may not always
have the right CPU number of the lock holder. So I don't think we can
eliminate that for the queue head spinning. I think we can eliminates
the SPIN_THRESHOLD spinning for the other queue node vCPUs.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH] virtio: don't warn if we can't allocate indirect sglist
From: Christian Borntraeger @ 2016-07-12 7:30 UTC (permalink / raw)
To: Mikulas Patocka, Michael S. Tsirkin; +Cc: virtualization
In-Reply-To: <alpine.LRH.2.02.1607111320530.25989@file01.intranet.prod.int.rdu2.redhat.com>
On 07/11/2016 07:22 PM, Mikulas Patocka wrote:
> Don't print warning if memory allocation fails. The virtio driver can
> handle allocation failure (it falls back to direct sglist), so there is
> no functionality degradation.
>
> This warning was observed when swapping to virtio scsi device because
> there may be not enough free memory during swapping.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> ---
> drivers/virtio/virtio_ring.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> Index: linux-4.7-rc7/drivers/virtio/virtio_ring.c
> ===================================================================
> --- linux-4.7-rc7.orig/drivers/virtio/virtio_ring.c 2016-05-09 17:45:04.000000000 +0200
> +++ linux-4.7-rc7/drivers/virtio/virtio_ring.c 2016-07-11 17:30:20.000000000 +0200
> @@ -237,6 +237,12 @@ static struct vring_desc *alloc_indirect
> */
> gfp &= ~__GFP_HIGHMEM;
>
> + /*
> + * Don't warn if the allocation fails, because the driver
> + * handles allocation failures gracefully.
> + */
> + gfp |= __GFP_NOWARN;
> +
> desc = kmalloc(total_sg * sizeof(struct vring_desc), gfp);
> if (!desc)
> return NULL;
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
^ permalink raw reply
* Re: [PATCH v2 0/4] implement vcpu preempted check
From: Juergen Gross @ 2016-07-12 4:16 UTC (permalink / raw)
To: Waiman Long, Peter Zijlstra
Cc: linux-s390, dave, Pan Xinhui, boqun.feng, will.deacon,
linux-kernel, virtualization, mingo, paulus, mpe, benh,
schwidefsky, pbonzini, paulmck, linuxppc-dev
In-Reply-To: <5783B6D7.7020903@hpe.com>
On 11/07/16 17:10, Waiman Long wrote:
> On 07/06/2016 02:52 AM, Peter Zijlstra wrote:
>> On Tue, Jun 28, 2016 at 10:43:07AM -0400, Pan Xinhui wrote:
>>> change fomr v1:
>>> a simplier definition of default vcpu_is_preempted
>>> skip mahcine type check on ppc, and add config. remove dedicated
>>> macro.
>>> add one patch to drop overload of rwsem_spin_on_owner and
>>> mutex_spin_on_owner.
>>> add more comments
>>> thanks boqun and Peter's suggestion.
>>>
>>> This patch set aims to fix lock holder preemption issues.
>>>
>>> test-case:
>>> perf record -a perf bench sched messaging -g 400 -p&& perf report
>>>
>>> 18.09% sched-messaging [kernel.vmlinux] [k] osq_lock
>>> 12.28% sched-messaging [kernel.vmlinux] [k] rwsem_spin_on_owner
>>> 5.27% sched-messaging [kernel.vmlinux] [k] mutex_unlock
>>> 3.89% sched-messaging [kernel.vmlinux] [k] wait_consider_task
>>> 3.64% sched-messaging [kernel.vmlinux] [k] _raw_write_lock_irq
>>> 3.41% sched-messaging [kernel.vmlinux] [k] mutex_spin_on_owner.is
>>> 2.49% sched-messaging [kernel.vmlinux] [k] system_call
>>>
>>> We introduce interface bool vcpu_is_preempted(int cpu) and use it in
>>> some spin
>>> loops of osq_lock, rwsem_spin_on_owner and mutex_spin_on_owner.
>>> These spin_on_onwer variant also cause rcu stall before we apply this
>>> patch set
>>>
>> Paolo, could you help out with an (x86) KVM interface for this?
>>
>> Waiman, could you see if you can utilize this to get rid of the
>> SPIN_THRESHOLD in qspinlock_paravirt?
>
> That API is certainly useful to make the paravirt spinlock perform
> better. However, I am not sure if we can completely get rid of the
> SPIN_THRESHOLD at this point. It is not just the kvm, the xen code need
> to be modified as well.
This should be rather easy. The relevant information is included in the
runstate data mapped into kernel memory. I can provide a patch for Xen
if needed.
Juergen
^ permalink raw reply
* [PATCH] virtio: don't warn if we can't allocate indirect sglist
From: Mikulas Patocka @ 2016-07-11 17:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
Don't print warning if memory allocation fails. The virtio driver can
handle allocation failure (it falls back to direct sglist), so there is
no functionality degradation.
This warning was observed when swapping to virtio scsi device because
there may be not enough free memory during swapping.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
---
drivers/virtio/virtio_ring.c | 6 ++++++
1 file changed, 6 insertions(+)
Index: linux-4.7-rc7/drivers/virtio/virtio_ring.c
===================================================================
--- linux-4.7-rc7.orig/drivers/virtio/virtio_ring.c 2016-05-09 17:45:04.000000000 +0200
+++ linux-4.7-rc7/drivers/virtio/virtio_ring.c 2016-07-11 17:30:20.000000000 +0200
@@ -237,6 +237,12 @@ static struct vring_desc *alloc_indirect
*/
gfp &= ~__GFP_HIGHMEM;
+ /*
+ * Don't warn if the allocation fails, because the driver
+ * handles allocation failures gracefully.
+ */
+ gfp |= __GFP_NOWARN;
+
desc = kmalloc(total_sg * sizeof(struct vring_desc), gfp);
if (!desc)
return NULL;
^ permalink raw reply
* Re: [PATCH v2 0/4] implement vcpu preempted check
From: Waiman Long @ 2016-07-11 15:10 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-s390, dave, benh, Pan Xinhui, boqun.feng, will.deacon,
linux-kernel, virtualization, mingo, paulus, mpe, schwidefsky,
pbonzini, paulmck, linuxppc-dev
In-Reply-To: <20160706065255.GH30909@twins.programming.kicks-ass.net>
On 07/06/2016 02:52 AM, Peter Zijlstra wrote:
> On Tue, Jun 28, 2016 at 10:43:07AM -0400, Pan Xinhui wrote:
>> change fomr v1:
>> a simplier definition of default vcpu_is_preempted
>> skip mahcine type check on ppc, and add config. remove dedicated macro.
>> add one patch to drop overload of rwsem_spin_on_owner and mutex_spin_on_owner.
>> add more comments
>> thanks boqun and Peter's suggestion.
>>
>> This patch set aims to fix lock holder preemption issues.
>>
>> test-case:
>> perf record -a perf bench sched messaging -g 400 -p&& perf report
>>
>> 18.09% sched-messaging [kernel.vmlinux] [k] osq_lock
>> 12.28% sched-messaging [kernel.vmlinux] [k] rwsem_spin_on_owner
>> 5.27% sched-messaging [kernel.vmlinux] [k] mutex_unlock
>> 3.89% sched-messaging [kernel.vmlinux] [k] wait_consider_task
>> 3.64% sched-messaging [kernel.vmlinux] [k] _raw_write_lock_irq
>> 3.41% sched-messaging [kernel.vmlinux] [k] mutex_spin_on_owner.is
>> 2.49% sched-messaging [kernel.vmlinux] [k] system_call
>>
>> We introduce interface bool vcpu_is_preempted(int cpu) and use it in some spin
>> loops of osq_lock, rwsem_spin_on_owner and mutex_spin_on_owner.
>> These spin_on_onwer variant also cause rcu stall before we apply this patch set
>>
> Paolo, could you help out with an (x86) KVM interface for this?
>
> Waiman, could you see if you can utilize this to get rid of the
> SPIN_THRESHOLD in qspinlock_paravirt?
That API is certainly useful to make the paravirt spinlock perform
better. However, I am not sure if we can completely get rid of the
SPIN_THRESHOLD at this point. It is not just the kvm, the xen code need
to be modified as well.
Cheers,
Longman
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox