Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Namhyung Kim]

On Mon, Aug 01, 2016 at 10:24:39AM +0100, Daniel P. Berrange wrote:
> On Sat, Jul 30, 2016 at 05:57:02PM +0900, Namhyung Kim wrote:
> > On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote:
> > > > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> > > > +char *buf, size_t sz,
> > > > +struct virtio_pstore_fileinfo 
> > > > *info)
> > > > +{
> > > > +snprintf(buf, sz, "%s/%s", s->directory, name);
> > > > +
> > > > +if (g_str_has_prefix(name, "dmesg-")) {
> > > > +info->type = VIRTIO_PSTORE_TYPE_DMESG;
> > > > +name += strlen("dmesg-");
> > > > +} else if (g_str_has_prefix(name, "console-")) {
> > > > +info->type = VIRTIO_PSTORE_TYPE_CONSOLE;
> > > > +name += strlen("console-");
> > > > +} else if (g_str_has_prefix(name, "unknown-")) {
> > > > +info->type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> > > > +name += strlen("unknown-");
> > > > +}
> 
> [snip]
> 
> > > > +struct virtio_pstore_fileinfo info;
> > > > +size_t offset = sizeof(*res) + sizeof(info);
> > > > +
> > > > +if (s->dirp == NULL) {
> > > > +return -1;
> > > > +}
> > > > +
> > > > +dent = readdir(s->dirp);
> > > > +while (dent) {
> > > > +if (dent->d_name[0] != '.') {
> > > > +break;
> > > > +}
> > > > +dent = readdir(s->dirp);
> > > > +}
> > > > +
> > > > +if (dent == NULL) {
> > > > +return 0;
> > > > +}
> > > 
> > > So this seems to just be picking the first filename reported by
> > > readdir that isn't starting with '.'. Surely this can't the right
> > > logic when your corresponding do_write method can pick several
> > > different filenames, its potluck which do_read will give back.
> > 
> > Do you mean that it'd be better to check a list of known filenames and
> > fail if not?
> 
> No, I mean that you have several different VIRTIO_PSTORE_TYPE_nnn and
> use a different file for each constant. When reading this directory
> though you're not looking for the file corresponding to any given
> VIRTIO_PSTORE_TYPE_nnn - you're simply reading whichever file is found
> first. So you might have just read a TYPE_CONSOLE or have read a
> TYPE_DMESG - it surely doesn't make sense to randonly read either
> TYPE_CONSOLE or TYPE_DMESG based on whatever order readdir() lists
> the files.

In fact, each VIRTIO_PSTORE_TYPE_xxx can have more than one files and
I don't care about the ordering between them.  They will be fed to the
pstore filesystem on the guest.

But I also think it'd be better to scan files of known types only
rather than read out whatever readdir() gives.

Thanks,
Namhyung

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Namhyung Kim]

Hi Daniel,

On Mon, Aug 01, 2016 at 10:21:30AM +0100, Daniel P. Berrange wrote:
> On Sat, Jul 30, 2016 at 05:38:27PM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > On Thu, Jul 28, 2016 at 02:08:41PM +0100, Daniel P. Berrange wrote:
> > > On Thu, Jul 28, 2016 at 01:56:07PM +0100, Stefan Hajnoczi wrote:
> > > > On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote:
> > > > > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> > > > > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct 
> > > > > > > iovec *out_sg,
> > > > > > > +  unsigned int out_num,
> > > > > > > +  struct virtio_pstore_req 
> > > > > > > *req)
> > > > > > > +{
> > > > > > > +char path[PATH_MAX];
> > > > > > > +int fd;
> > > > > > > +ssize_t len;
> > > > > > > +unsigned short type;
> > > > > > > +int flags = O_WRONLY | O_CREAT;
> > > > > > > +
> > > > > > > +/* we already consume the req */
> > > > > > > +iov_discard_front(_sg, _num, sizeof(*req));
> > > > > > > +
> > > > > > > +virtio_pstore_to_filename(s, path, sizeof(path), req);
> > > > > > > +
> > > > > > > +type = le16_to_cpu(req->type);
> > > > > > > +
> > > > > > > +if (type == VIRTIO_PSTORE_TYPE_DMESG) {
> > > > > > > +flags |= O_TRUNC;
> > > > > > > +} else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) {
> > > > > > > +flags |= O_APPEND;
> > > > > > > +}
> > > > > > > +
> > > > > > > +fd = open(path, flags, 0644);
> > > > > > > +if (fd < 0) {
> > > > > > > +error_report("cannot open %s", path);
> > > > > > > +return -1;
> > > > > > > +}
> > > > > > > +len = writev(fd, out_sg, out_num);
> > > > > > > +close(fd);
> > > > > > > +
> > > > > > > +return len;
> > > > > > 
> > > > > > All this is blocking VM until host io completes.
> > > > > 
> > > > > Hmm.. I don't know about the internals of qemu.  So does it make guest
> > > > > stop?  If so, that's what I want to do for _DMESG. :)  As it's called
> > > > > only on kernel oops I think it's admittable.  But for _CONSOLE, it
> > > > > needs to do asynchronously.  Maybe I can add a thread to do the work.
> > > > 
> > > > Please look at include/io/channel.h.  QEMU is event-driven and tends to
> > > > use asynchronous I/O instead of spawning threads.  The include/io/ APIs
> > > > allow you to do asynchronous I/O in the event loop.
> > > 
> > > That is true, except for I/O to/from plain files - the QIOChannelFile
> > > impl doesn't do anything special (yet) to make that work correctly in
> > > non-blocking mode. Of course that could be fixed...
> > 
> > Yep, I don't know how I can use the QIOChannelFile for async IO.
> > AFAICS it's just a wrapper for normal readv/writev.  Who is
> > responsible to do these IO when I use the IO channel API?  Also does
> > it guarantee that all IOs are processed in order?
> 
> I'd suggest just using QIOChannelFile regardless - we need to fix the
> blocking behaviour already for sake of the qemu chardev code, and your
> code just adds more pressure to fix it. I/O will be done in the order
> in which you make the calls, as with regular POSIX I/O funcs you're
> currently using.

Thanks for the info.

So can I simply replace regular IO funcs to QIOChannel API like below?

  ioc = qio_channel_file_new_path(path, flags, 0644, );
  qio_channel_set_blocking(ioc, false, );
  len = qio_channel_writev(ioc, out_sg, out_num, );
  qio_channel_file_close(ioc, );
  return len;

Or do I need to call qio_channel_add_watch() or something?

Thanks,
Namhyung

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Daniel P. Berrange]

On Sat, Jul 30, 2016 at 05:38:27PM +0900, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Jul 28, 2016 at 02:08:41PM +0100, Daniel P. Berrange wrote:
> > On Thu, Jul 28, 2016 at 01:56:07PM +0100, Stefan Hajnoczi wrote:
> > > On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote:
> > > > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> > > > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct 
> > > > > > iovec *out_sg,
> > > > > > +  unsigned int out_num,
> > > > > > +  struct virtio_pstore_req 
> > > > > > *req)
> > > > > > +{
> > > > > > +char path[PATH_MAX];
> > > > > > +int fd;
> > > > > > +ssize_t len;
> > > > > > +unsigned short type;
> > > > > > +int flags = O_WRONLY | O_CREAT;
> > > > > > +
> > > > > > +/* we already consume the req */
> > > > > > +iov_discard_front(_sg, _num, sizeof(*req));
> > > > > > +
> > > > > > +virtio_pstore_to_filename(s, path, sizeof(path), req);
> > > > > > +
> > > > > > +type = le16_to_cpu(req->type);
> > > > > > +
> > > > > > +if (type == VIRTIO_PSTORE_TYPE_DMESG) {
> > > > > > +flags |= O_TRUNC;
> > > > > > +} else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) {
> > > > > > +flags |= O_APPEND;
> > > > > > +}
> > > > > > +
> > > > > > +fd = open(path, flags, 0644);
> > > > > > +if (fd < 0) {
> > > > > > +error_report("cannot open %s", path);
> > > > > > +return -1;
> > > > > > +}
> > > > > > +len = writev(fd, out_sg, out_num);
> > > > > > +close(fd);
> > > > > > +
> > > > > > +return len;
> > > > > 
> > > > > All this is blocking VM until host io completes.
> > > > 
> > > > Hmm.. I don't know about the internals of qemu.  So does it make guest
> > > > stop?  If so, that's what I want to do for _DMESG. :)  As it's called
> > > > only on kernel oops I think it's admittable.  But for _CONSOLE, it
> > > > needs to do asynchronously.  Maybe I can add a thread to do the work.
> > > 
> > > Please look at include/io/channel.h.  QEMU is event-driven and tends to
> > > use asynchronous I/O instead of spawning threads.  The include/io/ APIs
> > > allow you to do asynchronous I/O in the event loop.
> > 
> > That is true, except for I/O to/from plain files - the QIOChannelFile
> > impl doesn't do anything special (yet) to make that work correctly in
> > non-blocking mode. Of course that could be fixed...
> 
> Yep, I don't know how I can use the QIOChannelFile for async IO.
> AFAICS it's just a wrapper for normal readv/writev.  Who is
> responsible to do these IO when I use the IO channel API?  Also does
> it guarantee that all IOs are processed in order?

I'd suggest just using QIOChannelFile regardless - we need to fix the
blocking behaviour already for sake of the qemu chardev code, and your
code just adds more pressure to fix it. I/O will be done in the order
in which you make the calls, as with regular POSIX I/O funcs you're
currently using.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Daniel P. Berrange]

On Sat, Jul 30, 2016 at 05:57:02PM +0900, Namhyung Kim wrote:
> On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote:
> > > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> > > +char *buf, size_t sz,
> > > +struct virtio_pstore_fileinfo 
> > > *info)
> > > +{
> > > +snprintf(buf, sz, "%s/%s", s->directory, name);
> > > +
> > > +if (g_str_has_prefix(name, "dmesg-")) {
> > > +info->type = VIRTIO_PSTORE_TYPE_DMESG;
> > > +name += strlen("dmesg-");
> > > +} else if (g_str_has_prefix(name, "console-")) {
> > > +info->type = VIRTIO_PSTORE_TYPE_CONSOLE;
> > > +name += strlen("console-");
> > > +} else if (g_str_has_prefix(name, "unknown-")) {
> > > +info->type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> > > +name += strlen("unknown-");
> > > +}

[snip]

> > > +struct virtio_pstore_fileinfo info;
> > > +size_t offset = sizeof(*res) + sizeof(info);
> > > +
> > > +if (s->dirp == NULL) {
> > > +return -1;
> > > +}
> > > +
> > > +dent = readdir(s->dirp);
> > > +while (dent) {
> > > +if (dent->d_name[0] != '.') {
> > > +break;
> > > +}
> > > +dent = readdir(s->dirp);
> > > +}
> > > +
> > > +if (dent == NULL) {
> > > +return 0;
> > > +}
> > 
> > So this seems to just be picking the first filename reported by
> > readdir that isn't starting with '.'. Surely this can't the right
> > logic when your corresponding do_write method can pick several
> > different filenames, its potluck which do_read will give back.
> 
> Do you mean that it'd be better to check a list of known filenames and
> fail if not?

No, I mean that you have several different VIRTIO_PSTORE_TYPE_nnn and
use a different file for each constant. When reading this directory
though you're not looking for the file corresponding to any given
VIRTIO_PSTORE_TYPE_nnn - you're simply reading whichever file is found
first. So you might have just read a TYPE_CONSOLE or have read a
TYPE_DMESG - it surely doesn't make sense to randonly read either
TYPE_CONSOLE or TYPE_DMESG based on whatever order readdir() lists
the files.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Namhyung Kim]

On Thu, Jul 28, 2016 at 02:22:39PM +0100, Daniel P. Berrange wrote:
> On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> > Add virtio pstore device to allow kernel log files saved on the host.
> > It will save the log files on the directory given by pstore device
> > option.
> > 
> >   $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
> > 
> >   (guest) # echo c > /proc/sysrq-trigger
> > 
> >   $ ls dir-xx
> >   dmesg-1.enc.z  dmesg-2.enc.z
> > 
> > The log files are usually compressed using zlib.  Users can see the log
> > messages directly on the host or on the guest (using pstore filesystem).
> > 
> > The 'directory' property is required for virtio-pstore device to work.
> > It also adds 'bufsize' and 'console' (boolean) properties.
> > 
> > Cc: Paolo Bonzini 
> > Cc: Radim Kr??m 
> > Cc: "Michael S. Tsirkin" 
> > Cc: Anthony Liguori 
> > Cc: Anton Vorontsov 
> > Cc: Colin Cross 
> > Cc: Kees Cook 
> > Cc: Tony Luck 
> > Cc: Steven Rostedt 
> > Cc: Ingo Molnar 
> > Cc: Minchan Kim 
> > Cc: k...@vger.kernel.org
> > Cc: qemu-devel@nongnu.org
> > Cc: virtualizat...@lists.linux-foundation.org
> > Signed-off-by: Namhyung Kim 
> > ---

[SNIP]
> > +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t 
> > sz,
> > +  struct virtio_pstore_req *req)
> > +{
> > +const char *basename;
> > +unsigned long long id = 0;
> > +unsigned int flags = le32_to_cpu(req->flags);
> > +
> > +switch (le16_to_cpu(req->type)) {
> > +case VIRTIO_PSTORE_TYPE_DMESG:
> > +basename = "dmesg";
> > +id = s->id++;
> > +break;
> > +case VIRTIO_PSTORE_TYPE_CONSOLE:
> > +basename = "console";
> > +if (s->console_id) {
> > +id = s->console_id;
> > +} else {
> > +id = s->console_id = s->id++;
> > +}
> > +break;
> > +default:
> > +basename = "unknown";
> > +break;
> > +}
> > +
> > +snprintf(buf, sz, "%s/%s-%llu%s", s->directory, basename, id,
> > + flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : "");
> 
> Please use g_strdup_printf() instead of splattering into a pre-allocated
> buffer than may or may not be large enough.

OK.

> 
> > +}
> > +
> > +static void virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> > +char *buf, size_t sz,
> > +struct virtio_pstore_fileinfo 
> > *info)
> > +{
> > +snprintf(buf, sz, "%s/%s", s->directory, name);
> > +
> > +if (g_str_has_prefix(name, "dmesg-")) {
> > +info->type = VIRTIO_PSTORE_TYPE_DMESG;
> > +name += strlen("dmesg-");
> > +} else if (g_str_has_prefix(name, "console-")) {
> > +info->type = VIRTIO_PSTORE_TYPE_CONSOLE;
> > +name += strlen("console-");
> > +} else if (g_str_has_prefix(name, "unknown-")) {
> > +info->type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> > +name += strlen("unknown-");
> > +}
> > +
> > +qemu_strtoull(name, NULL, 0, >id);
> > +
> > +info->flags = 0;
> > +if (g_str_has_suffix(name, ".enc.z")) {
> > +info->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> > +}
> > +}
> > +
> > +static ssize_t virtio_pstore_do_open(VirtIOPstore *s)
> > +{
> > +s->dirp = opendir(s->directory);
> > +if (s->dirp == NULL) {
> > +return -1;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +static ssize_t virtio_pstore_do_read(VirtIOPstore *s, struct iovec *in_sg,
> > + unsigned int in_num,
> > + struct virtio_pstore_res *res)
> > +{
> > +char path[PATH_MAX];
> 
> Don't declare PATH_MAX sized variables

Will change to use g_strdup_printf() as you said.

> 
> > +int fd;
> > +ssize_t len;
> > +struct stat stbuf;
> > +struct dirent *dent;
> > +int sg_num = in_num;
> > +struct iovec sg[sg_num];
> 
> 'sg_num' is initialized from 'in_num' which comes from the
> guest, and I'm not seeing anything which is bounds-checking
> the 'in_num' value. So you've possibly got a security flaw here
> I think, if the guest can cause QEMU to allocate arbitrary stack
> memory & thus overflow by setting arbitrarily large in_num.

Will add a bound check then.

> 
> > +struct virtio_pstore_fileinfo info;
> > +size_t offset = sizeof(*res) + sizeof(info);
> > +
> > +if (s->dirp == NULL) {
> > +return -1;
> > +}
> > +
> > +dent = readdir(s->dirp);
> > +while (dent) {
> > +if (dent->d_name[0] != '.') {
> > +break;
> > +}
> > +dent = readdir(s->dirp);
> > +}
> > +
> > +if (dent == NULL) {
> > +return 0;
> > + 

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Namhyung Kim]

Hello,

On Thu, Jul 28, 2016 at 02:08:41PM +0100, Daniel P. Berrange wrote:
> On Thu, Jul 28, 2016 at 01:56:07PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote:
> > > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> > > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec 
> > > > > *out_sg,
> > > > > +  unsigned int out_num,
> > > > > +  struct virtio_pstore_req *req)
> > > > > +{
> > > > > +char path[PATH_MAX];
> > > > > +int fd;
> > > > > +ssize_t len;
> > > > > +unsigned short type;
> > > > > +int flags = O_WRONLY | O_CREAT;
> > > > > +
> > > > > +/* we already consume the req */
> > > > > +iov_discard_front(_sg, _num, sizeof(*req));
> > > > > +
> > > > > +virtio_pstore_to_filename(s, path, sizeof(path), req);
> > > > > +
> > > > > +type = le16_to_cpu(req->type);
> > > > > +
> > > > > +if (type == VIRTIO_PSTORE_TYPE_DMESG) {
> > > > > +flags |= O_TRUNC;
> > > > > +} else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) {
> > > > > +flags |= O_APPEND;
> > > > > +}
> > > > > +
> > > > > +fd = open(path, flags, 0644);
> > > > > +if (fd < 0) {
> > > > > +error_report("cannot open %s", path);
> > > > > +return -1;
> > > > > +}
> > > > > +len = writev(fd, out_sg, out_num);
> > > > > +close(fd);
> > > > > +
> > > > > +return len;
> > > > 
> > > > All this is blocking VM until host io completes.
> > > 
> > > Hmm.. I don't know about the internals of qemu.  So does it make guest
> > > stop?  If so, that's what I want to do for _DMESG. :)  As it's called
> > > only on kernel oops I think it's admittable.  But for _CONSOLE, it
> > > needs to do asynchronously.  Maybe I can add a thread to do the work.
> > 
> > Please look at include/io/channel.h.  QEMU is event-driven and tends to
> > use asynchronous I/O instead of spawning threads.  The include/io/ APIs
> > allow you to do asynchronous I/O in the event loop.
> 
> That is true, except for I/O to/from plain files - the QIOChannelFile
> impl doesn't do anything special (yet) to make that work correctly in
> non-blocking mode. Of course that could be fixed...

Yep, I don't know how I can use the QIOChannelFile for async IO.
AFAICS it's just a wrapper for normal readv/writev.  Who is
responsible to do these IO when I use the IO channel API?  Also does
it guarantee that all IOs are processed in order?

Thanks,
Namhyung

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Steven Rostedt]

On Thu, 28 Jul 2016 14:39:53 +0900
Namhyung Kim  wrote:

> Well, I dont' know.  As you know, the kernel oops dump is already sent
> to serial device but it's rather slow.  As I wrote in the cover
> letter, enabling ftrace_dump_on_oops makes it even worse..  Also
> pstore saves the (compressed) binary data so I thought it'd be better
> to have a dedicated IO channel.

BTW, I agree with this. It is better to have a quick way to grab the
ftrace buffers when a crash happens, as serial is excruciatingly slow.
Although, currently I still use kexec/kdump, but as Namhyung said, it
depends on crash being up to date. I tend to be sending in updates
every time I have to use it.

-- Steve

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Daniel P. Berrange]

On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> Add virtio pstore device to allow kernel log files saved on the host.
> It will save the log files on the directory given by pstore device
> option.
> 
>   $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
> 
>   (guest) # echo c > /proc/sysrq-trigger
> 
>   $ ls dir-xx
>   dmesg-1.enc.z  dmesg-2.enc.z
> 
> The log files are usually compressed using zlib.  Users can see the log
> messages directly on the host or on the guest (using pstore filesystem).
> 
> The 'directory' property is required for virtio-pstore device to work.
> It also adds 'bufsize' and 'console' (boolean) properties.
> 
> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: "Michael S. Tsirkin" 
> Cc: Anthony Liguori 
> Cc: Anton Vorontsov 
> Cc: Colin Cross 
> Cc: Kees Cook 
> Cc: Tony Luck 
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Cc: Minchan Kim 
> Cc: k...@vger.kernel.org
> Cc: qemu-devel@nongnu.org
> Cc: virtualizat...@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim 
> ---
>  hw/virtio/Makefile.objs|   2 +-
>  hw/virtio/virtio-pci.c |  54 +++
>  hw/virtio/virtio-pci.h |  14 +
>  hw/virtio/virtio-pstore.c  | 477 
> +
>  include/hw/pci/pci.h   |   1 +
>  include/hw/virtio/virtio-pstore.h  |  34 ++
>  include/standard-headers/linux/virtio_ids.h|   1 +
>  include/standard-headers/linux/virtio_pstore.h |  80 +
>  qdev-monitor.c |   1 +
>  9 files changed, 663 insertions(+), 1 deletion(-)
>  create mode 100644 hw/virtio/virtio-pstore.c
>  create mode 100644 include/hw/virtio/virtio-pstore.h
>  create mode 100644 include/standard-headers/linux/virtio_pstore.h
> 
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index 3e2b175..aae7082 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o
>  common-obj-y += virtio-mmio.o
>  
>  obj-y += virtio.o virtio-balloon.o 
> -obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
> +obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f0677b7..d99a405 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2414,6 +2414,59 @@ static const TypeInfo virtio_host_pci_info = {
>  };
>  #endif
>  
> +/* virtio-pstore-pci */
> +
> +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> +VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
> +DeviceState *vdev = DEVICE(>vdev);
> +Error *err = NULL;
> +
> +qdev_set_parent_bus(vdev, BUS(_dev->bus));
> +object_property_set_bool(OBJECT(vdev), true, "realized", );
> +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, >vdev, sizeof(dev->vdev),
> +TYPE_VIRTIO_PSTORE);
> +object_property_add_alias(obj, "directory", OBJECT(>vdev),
> +  "directory", _abort);
> +object_property_add_alias(obj, "bufsize", OBJECT(>vdev),
> +  "bufsize", _abort);
> +object_property_add_alias(obj, "console", OBJECT(>vdev),
> +  "console", _abort);
> +}
> +
> +static const TypeInfo virtio_pstore_pci_info = {
> +.name  = TYPE_VIRTIO_PSTORE_PCI,
> +.parent= TYPE_VIRTIO_PCI,
> +.instance_size = sizeof(VirtIOPstorePCI),
> +.instance_init = virtio_pstore_pci_instance_init,
> +.class_init= virtio_pstore_pci_class_init,
> +};
> +
>  /* virtio-pci-bus */
>  
>  static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
> @@ -2483,6 +2536,7 @@ static void virtio_pci_register_types(void)
>  #ifdef CONFIG_VHOST_SCSI
>  type_register_static(_scsi_pci_info);
>  #endif
> +type_register_static(_pstore_pci_info);
>  }

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Daniel P. Berrange]

On Thu, Jul 28, 2016 at 01:56:07PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote:
> > On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> > > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec 
> > > > *out_sg,
> > > > +  unsigned int out_num,
> > > > +  struct virtio_pstore_req *req)
> > > > +{
> > > > +char path[PATH_MAX];
> > > > +int fd;
> > > > +ssize_t len;
> > > > +unsigned short type;
> > > > +int flags = O_WRONLY | O_CREAT;
> > > > +
> > > > +/* we already consume the req */
> > > > +iov_discard_front(_sg, _num, sizeof(*req));
> > > > +
> > > > +virtio_pstore_to_filename(s, path, sizeof(path), req);
> > > > +
> > > > +type = le16_to_cpu(req->type);
> > > > +
> > > > +if (type == VIRTIO_PSTORE_TYPE_DMESG) {
> > > > +flags |= O_TRUNC;
> > > > +} else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) {
> > > > +flags |= O_APPEND;
> > > > +}
> > > > +
> > > > +fd = open(path, flags, 0644);
> > > > +if (fd < 0) {
> > > > +error_report("cannot open %s", path);
> > > > +return -1;
> > > > +}
> > > > +len = writev(fd, out_sg, out_num);
> > > > +close(fd);
> > > > +
> > > > +return len;
> > > 
> > > All this is blocking VM until host io completes.
> > 
> > Hmm.. I don't know about the internals of qemu.  So does it make guest
> > stop?  If so, that's what I want to do for _DMESG. :)  As it's called
> > only on kernel oops I think it's admittable.  But for _CONSOLE, it
> > needs to do asynchronously.  Maybe I can add a thread to do the work.
> 
> Please look at include/io/channel.h.  QEMU is event-driven and tends to
> use asynchronous I/O instead of spawning threads.  The include/io/ APIs
> allow you to do asynchronous I/O in the event loop.

That is true, except for I/O to/from plain files - the QIOChannelFile
impl doesn't do anything special (yet) to make that work correctly in
non-blocking mode. Of course that could be fixed...

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Stefan Hajnoczi]

On Thu, Jul 28, 2016 at 02:39:53PM +0900, Namhyung Kim wrote:
> On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> > > +static ssize_t virtio_pstore_do_write(VirtIOPstore *s, struct iovec 
> > > *out_sg,
> > > +  unsigned int out_num,
> > > +  struct virtio_pstore_req *req)
> > > +{
> > > +char path[PATH_MAX];
> > > +int fd;
> > > +ssize_t len;
> > > +unsigned short type;
> > > +int flags = O_WRONLY | O_CREAT;
> > > +
> > > +/* we already consume the req */
> > > +iov_discard_front(_sg, _num, sizeof(*req));
> > > +
> > > +virtio_pstore_to_filename(s, path, sizeof(path), req);
> > > +
> > > +type = le16_to_cpu(req->type);
> > > +
> > > +if (type == VIRTIO_PSTORE_TYPE_DMESG) {
> > > +flags |= O_TRUNC;
> > > +} else if (type == VIRTIO_PSTORE_TYPE_CONSOLE) {
> > > +flags |= O_APPEND;
> > > +}
> > > +
> > > +fd = open(path, flags, 0644);
> > > +if (fd < 0) {
> > > +error_report("cannot open %s", path);
> > > +return -1;
> > > +}
> > > +len = writev(fd, out_sg, out_num);
> > > +close(fd);
> > > +
> > > +return len;
> > 
> > All this is blocking VM until host io completes.
> 
> Hmm.. I don't know about the internals of qemu.  So does it make guest
> stop?  If so, that's what I want to do for _DMESG. :)  As it's called
> only on kernel oops I think it's admittable.  But for _CONSOLE, it
> needs to do asynchronously.  Maybe I can add a thread to do the work.

Please look at include/io/channel.h.  QEMU is event-driven and tends to
use asynchronous I/O instead of spawning threads.  The include/io/ APIs
allow you to do asynchronous I/O in the event loop.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Namhyung Kim]

On Thu, Jul 28, 2016 at 03:02:54AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> > Add virtio pstore device to allow kernel log files saved on the host.
> > It will save the log files on the directory given by pstore device
> > option.
> > 
> >   $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
> > 
> >   (guest) # echo c > /proc/sysrq-trigger
> 
> So if the point is handling system crashes, I suspect
> virtio is the wrong protocol to use. ATM it's rather
> elaborate for performance, it's likely not to work when
> linux is crashing. I think you want something very very
> simple that will still work when you crash. Like maybe
> a serial device?

Well, I dont' know.  As you know, the kernel oops dump is already sent
to serial device but it's rather slow.  As I wrote in the cover
letter, enabling ftrace_dump_on_oops makes it even worse..  Also
pstore saves the (compressed) binary data so I thought it'd be better
to have a dedicated IO channel.

I know that we can't rely on anything in kernel when the kernel is
crashing.  In the virtualization environment, I think it'd be great if
it can dump the crash data in the host directly so I chose the virtio.
I thought the virtio is a relatively thin layer and independent from
other kernel features.  Even if it doesn't guarantee to work 100%,
it's worth trying IMHO.


> 
> >   $ ls dir-xx
> >   dmesg-1.enc.z  dmesg-2.enc.z
> > 
> > The log files are usually compressed using zlib.  Users can see the log
> > messages directly on the host or on the guest (using pstore filesystem).
> 
> So this lacks all management tools that a regular storage device
> has, and these are necessary if people are to use this
> in production.
> 
> For example, some kind of provision for limiting the amount of host disk
> this can consume seems called for. Rate-limiting disk writes
> on host also seems necessary. Handling host disk errors always
> propagates error to guest but an option to e.g. stop vm might
> be useful to avoid corruption.

Yes, it needs that kind of management.  I'd like to look at the
existing implementation.  But basically I thought it as a debugging
feature and it won't produce much data for the default setting.  Maybe
I can limit the file size not to exceed the buffer size and keep up to
pre-configured number of files for each type.  Now host disk error
will be propagated to guest.


> 
> > 
> > The 'directory' property is required for virtio-pstore device to work.
> > It also adds 'bufsize' and 'console' (boolean) properties.
> 
> No idea what these do. Seem to be RW by guest but have no effect
> otherwise.

The 'directory' is a pathname which it saves the data files.  The
'bufsize' sets the size of buffer used by pstore.  The 'console'
enables saving pstore console type data.


> 
> 
> > Cc: Paolo Bonzini 
> > Cc: Radim Krčmář 
> > Cc: "Michael S. Tsirkin" 
> > Cc: Anthony Liguori 
> > Cc: Anton Vorontsov 
> > Cc: Colin Cross 
> > Cc: Kees Cook 
> > Cc: Tony Luck 
> > Cc: Steven Rostedt 
> > Cc: Ingo Molnar 
> > Cc: Minchan Kim 
> > Cc: k...@vger.kernel.org
> > Cc: qemu-devel@nongnu.org
> > Cc: virtualizat...@lists.linux-foundation.org
> > Signed-off-by: Namhyung Kim 
> > ---

[SNIP]
> > diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
> > new file mode 100644
> > index 000..2ca7786
> > --- /dev/null
> > +++ b/hw/virtio/virtio-pstore.c
> > @@ -0,0 +1,477 @@
> > +/*
> > + * Virtio Pstore Device
> > + *
> > + * Copyright (C) 2016  LG Electronics
> > + *
> > + * Authors:
> > + *  Namhyung Kim  
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.
> 
> We generally ask new code to be v2 or later.

Ok, will update.


> 
> >  See
> > + * the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include 
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/iov.h"
> > +#include "qemu-common.h"
> > +#include "qemu/cutils.h"
> > +#include "qemu/error-report.h"
> > +#include "sysemu/kvm.h"
> > +#include "qapi/visitor.h"
> > +#include "qapi-event.h"
> > +#include "trace.h"
> > +
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/virtio/virtio-bus.h"
> > +#include "hw/virtio/virtio-access.h"
> > +#include "hw/virtio/virtio-pstore.h"
> > +
> > +
> > +static void virtio_pstore_to_filename(VirtIOPstore *s, char *buf, size_t 
> > sz,
> > +  struct virtio_pstore_req *req)
> > +{
> > +const char *basename;
> > +unsigned long long id = 0;
> > +unsigned int flags = le32_to_cpu(req->flags);
> > +
> > +switch (le16_to_cpu(req->type)) {
> > +case VIRTIO_PSTORE_TYPE_DMESG:
> > +basename = "dmesg";
> > +id = s->id++;
> > +

Re: [Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Michael S. Tsirkin]

On Thu, Jul 28, 2016 at 12:08:30AM +0900, Namhyung Kim wrote:
> Add virtio pstore device to allow kernel log files saved on the host.
> It will save the log files on the directory given by pstore device
> option.
> 
>   $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...
> 
>   (guest) # echo c > /proc/sysrq-trigger

So if the point is handling system crashes, I suspect
virtio is the wrong protocol to use. ATM it's rather
elaborate for performance, it's likely not to work when
linux is crashing. I think you want something very very
simple that will still work when you crash. Like maybe
a serial device?

>   $ ls dir-xx
>   dmesg-1.enc.z  dmesg-2.enc.z
> 
> The log files are usually compressed using zlib.  Users can see the log
> messages directly on the host or on the guest (using pstore filesystem).

So this lacks all management tools that a regular storage device
has, and these are necessary if people are to use this
in production.

For example, some kind of provision for limiting the amount of host disk
this can consume seems called for. Rate-limiting disk writes
on host also seems necessary. Handling host disk errors always
propagates error to guest but an option to e.g. stop vm might
be useful to avoid corruption.

> 
> The 'directory' property is required for virtio-pstore device to work.
> It also adds 'bufsize' and 'console' (boolean) properties.

No idea what these do. Seem to be RW by guest but have no effect
otherwise.


> Cc: Paolo Bonzini 
> Cc: Radim Krčmář 
> Cc: "Michael S. Tsirkin" 
> Cc: Anthony Liguori 
> Cc: Anton Vorontsov 
> Cc: Colin Cross 
> Cc: Kees Cook 
> Cc: Tony Luck 
> Cc: Steven Rostedt 
> Cc: Ingo Molnar 
> Cc: Minchan Kim 
> Cc: k...@vger.kernel.org
> Cc: qemu-devel@nongnu.org
> Cc: virtualizat...@lists.linux-foundation.org
> Signed-off-by: Namhyung Kim 
> ---
>  hw/virtio/Makefile.objs|   2 +-
>  hw/virtio/virtio-pci.c |  54 +++
>  hw/virtio/virtio-pci.h |  14 +
>  hw/virtio/virtio-pstore.c  | 477 
> +
>  include/hw/pci/pci.h   |   1 +
>  include/hw/virtio/virtio-pstore.h  |  34 ++
>  include/standard-headers/linux/virtio_ids.h|   1 +
>  include/standard-headers/linux/virtio_pstore.h |  80 +
>  qdev-monitor.c |   1 +
>  9 files changed, 663 insertions(+), 1 deletion(-)
>  create mode 100644 hw/virtio/virtio-pstore.c
>  create mode 100644 include/hw/virtio/virtio-pstore.h
>  create mode 100644 include/standard-headers/linux/virtio_pstore.h
> 
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index 3e2b175..aae7082 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o
>  common-obj-y += virtio-mmio.o
>  
>  obj-y += virtio.o virtio-balloon.o 
> -obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
> +obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f0677b7..d99a405 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2414,6 +2414,59 @@ static const TypeInfo virtio_host_pci_info = {
>  };
>  #endif
>  
> +/* virtio-pstore-pci */
> +
> +static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> +VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
> +DeviceState *vdev = DEVICE(>vdev);
> +Error *err = NULL;
> +
> +qdev_set_parent_bus(vdev, BUS(_dev->bus));
> +object_property_set_bool(OBJECT(vdev), true, "realized", );
> +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, >vdev, sizeof(dev->vdev),
> +TYPE_VIRTIO_PSTORE);
> +object_property_add_alias(obj, "directory", OBJECT(>vdev),
> +  "directory", _abort);
> +object_property_add_alias(obj, "bufsize", OBJECT(>vdev),

[Qemu-devel] [PATCH 6/7] qemu: Implement virtio-pstore device

[Namhyung Kim]

Add virtio pstore device to allow kernel log files saved on the host.
It will save the log files on the directory given by pstore device
option.

  $ qemu-system-x86_64 -device virtio-pstore,directory=dir-xx ...

  (guest) # echo c > /proc/sysrq-trigger

  $ ls dir-xx
  dmesg-1.enc.z  dmesg-2.enc.z

The log files are usually compressed using zlib.  Users can see the log
messages directly on the host or on the guest (using pstore filesystem).

The 'directory' property is required for virtio-pstore device to work.
It also adds 'bufsize' and 'console' (boolean) properties.

Cc: Paolo Bonzini 
Cc: Radim Krčmář 
Cc: "Michael S. Tsirkin" 
Cc: Anthony Liguori 
Cc: Anton Vorontsov 
Cc: Colin Cross 
Cc: Kees Cook 
Cc: Tony Luck 
Cc: Steven Rostedt 
Cc: Ingo Molnar 
Cc: Minchan Kim 
Cc: k...@vger.kernel.org
Cc: qemu-devel@nongnu.org
Cc: virtualizat...@lists.linux-foundation.org
Signed-off-by: Namhyung Kim 
---
 hw/virtio/Makefile.objs|   2 +-
 hw/virtio/virtio-pci.c |  54 +++
 hw/virtio/virtio-pci.h |  14 +
 hw/virtio/virtio-pstore.c  | 477 +
 include/hw/pci/pci.h   |   1 +
 include/hw/virtio/virtio-pstore.h  |  34 ++
 include/standard-headers/linux/virtio_ids.h|   1 +
 include/standard-headers/linux/virtio_pstore.h |  80 +
 qdev-monitor.c |   1 +
 9 files changed, 663 insertions(+), 1 deletion(-)
 create mode 100644 hw/virtio/virtio-pstore.c
 create mode 100644 include/hw/virtio/virtio-pstore.h
 create mode 100644 include/standard-headers/linux/virtio_pstore.h

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 3e2b175..aae7082 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,4 +4,4 @@ common-obj-y += virtio-bus.o
 common-obj-y += virtio-mmio.o
 
 obj-y += virtio.o virtio-balloon.o 
-obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
+obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o virtio-pstore.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f0677b7..d99a405 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -2414,6 +2414,59 @@ static const TypeInfo virtio_host_pci_info = {
 };
 #endif
 
+/* virtio-pstore-pci */
+
+static void virtio_pstore_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VirtIOPstorePCI *vps = VIRTIO_PSTORE_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(>vdev);
+Error *err = NULL;
+
+qdev_set_parent_bus(vdev, BUS(_dev->bus));
+object_property_set_bool(OBJECT(vdev), true, "realized", );
+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, >vdev, sizeof(dev->vdev),
+TYPE_VIRTIO_PSTORE);
+object_property_add_alias(obj, "directory", OBJECT(>vdev),
+  "directory", _abort);
+object_property_add_alias(obj, "bufsize", OBJECT(>vdev),
+  "bufsize", _abort);
+object_property_add_alias(obj, "console", OBJECT(>vdev),
+  "console", _abort);
+}
+
+static const TypeInfo virtio_pstore_pci_info = {
+.name  = TYPE_VIRTIO_PSTORE_PCI,
+.parent= TYPE_VIRTIO_PCI,
+.instance_size = sizeof(VirtIOPstorePCI),
+.instance_init = virtio_pstore_pci_instance_init,
+.class_init= virtio_pstore_pci_class_init,
+};
+
 /* virtio-pci-bus */
 
 static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
@@ -2483,6 +2536,7 @@ static void virtio_pci_register_types(void)
 #ifdef CONFIG_VHOST_SCSI
 type_register_static(_scsi_pci_info);
 #endif
+type_register_static(_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"