Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Michael S. Tsirkin]

On Tue, Nov 15, 2016 at 03:23:36PM +0900, Namhyung Kim wrote:
> On Fri, Nov 11, 2016 at 12:50:03AM +0200, Michael S. Tsirkin wrote:
> > On Fri, Sep 16, 2016 at 07:05:47PM +0900, Namhyung Kim wrote:
> > > On Tue, Sep 13, 2016 at 06:57:10PM +0300, Michael S. Tsirkin wrote:
> > > > On Sat, Aug 20, 2016 at 05:07:43PM +0900, Namhyung Kim wrote:
> > > > > +
> > > > > +/* the index should match to the type value */
> > > > > +static const char *virtio_pstore_file_prefix[] = {
> > > > > +"unknown-",  /* VIRTIO_PSTORE_TYPE_UNKNOWN */
> > > > 
> > > > Is there value in treating everything unexpected as "unknown"
> > > > and rotating them as if they were logs?
> > > > It might be better to treat everything that's not known
> > > > as guest error.
> > > 
> > > I was thinking about the version mismatch between the kernel and qemu.
> > > I'd like to make the device can deal with a new kernel version which
> > > might implement a new pstore message type.  It will be saved as
> > > unknown but the kernel can read it properly later.
> > 
> > Well it'll have a different prefix. E.g. if kernel has
> > two different types they will end up in the same
> > file, hardly what was wanted.
> 
> Right, I think it needs to add 'type' info to the filename for unknown
> type.
> 
> Thanks,
> Namhyung

And that opens all kind of resource management issues as guest
might be able to open a ton of these unexpected types.
So don't try to predict the future, if you add a new type
you add a feature flag. Ignore or error on things you can
not handle.

-- 
MST

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

On Fri, Nov 11, 2016 at 12:50:03AM +0200, Michael S. Tsirkin wrote:
> On Fri, Sep 16, 2016 at 07:05:47PM +0900, Namhyung Kim wrote:
> > On Tue, Sep 13, 2016 at 06:57:10PM +0300, Michael S. Tsirkin wrote:
> > > On Sat, Aug 20, 2016 at 05:07:43PM +0900, Namhyung Kim wrote:
> > > > +
> > > > +/* the index should match to the type value */
> > > > +static const char *virtio_pstore_file_prefix[] = {
> > > > +"unknown-",/* VIRTIO_PSTORE_TYPE_UNKNOWN */
> > > 
> > > Is there value in treating everything unexpected as "unknown"
> > > and rotating them as if they were logs?
> > > It might be better to treat everything that's not known
> > > as guest error.
> > 
> > I was thinking about the version mismatch between the kernel and qemu.
> > I'd like to make the device can deal with a new kernel version which
> > might implement a new pstore message type.  It will be saved as
> > unknown but the kernel can read it properly later.
> 
> Well it'll have a different prefix. E.g. if kernel has
> two different types they will end up in the same
> file, hardly what was wanted.

Right, I think it needs to add 'type' info to the filename for unknown
type.

Thanks,
Namhyung

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Michael S. Tsirkin]

On Fri, Sep 16, 2016 at 07:05:47PM +0900, Namhyung Kim wrote:
> On Tue, Sep 13, 2016 at 06:57:10PM +0300, Michael S. Tsirkin wrote:
> > On Sat, Aug 20, 2016 at 05:07:43PM +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' property to set size of pstore bufer.
> > > 
> > > 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: Daniel P. Berrange 
> > > 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 |  52 ++
> > >  hw/virtio/virtio-pci.h |  14 +
> > >  hw/virtio/virtio-pstore.c  | 699 
> > > +
> > >  include/hw/pci/pci.h   |   1 +
> > >  include/hw/virtio/virtio-pstore.h  |  36 ++
> > >  include/standard-headers/linux/virtio_ids.h|   1 +
> > >  include/standard-headers/linux/virtio_pstore.h |  76 +++
> > >  qdev-monitor.c |   1 +
> > >  9 files changed, 881 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 755f921..c184823 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -2416,6 +2416,57 @@ 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);
> > > +}
> > > +
> > > +static const TypeInfo virtio_pstore_pci_info = {
> > > +.name  = 

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

On Thu, Sep 22, 2016 at 01:23:16PM +0100, Stefan Hajnoczi wrote:
> On Sun, Sep 04, 2016 at 11:38:59PM +0900, Namhyung Kim wrote:
> > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > +VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> > +VirtQueueElement *elem;
> > +struct virtio_pstore_req req;
> > +struct virtio_pstore_res res;
> > +ssize_t len = 0;
> > +int ret;
> > +
> > +for (;;) {
> > +elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > +if (!elem) {
> > +return;
> > +}
> > +
> > +if (elem->out_num < 1 || elem->in_num < 1) {
> > +error_report("request or response buffer is missing");
> > +exit(1);
> 
> The new virtio_error() function might be available, depending on when
> this patch series is merged.  virtio_error() should be used instead of
> exit(1).  See "[PATCH v5 0/9] virtio: avoid exit() when device enters
> invalid states" on qemu-devel.

Thanks for the info, will take a look.

> 
> > +}
> > +
> > +if (elem->out_num > 2 || elem->in_num > 3) {
> > +error_report("invalid number of input/output buffer");
> > +exit(1);
> > +}
> 
> The VIRTIO specification requires that flexible framing is supported.
> The device cannot make assumptions about the scatter-gather list.  It
> must support any layout (e.g. even multiple 1-byte iovecs making up the
> buffer).

Ok.

> 
> > +
> > +len = iov_to_buf(elem->out_sg, elem->out_num, 0, , 
> > sizeof(req));
> > +if (len != (ssize_t)sizeof(req)) {
> > +error_report("invalid request size: %ld", (long)len);
> > +exit(1);
> > +}
> > +res.cmd  = req.cmd;
> > +res.type = req.type;
> > +
> > +switch (le16_to_cpu(req.cmd)) {
> > +case VIRTIO_PSTORE_CMD_OPEN:
> > +ret = virtio_pstore_do_open(s);
> > +break;
> > +case VIRTIO_PSTORE_CMD_CLOSE:
> > +ret = virtio_pstore_do_close(s);
> > +break;
> > +case VIRTIO_PSTORE_CMD_ERASE:
> > +ret = virtio_pstore_do_erase(s, );
> > +break;
> > +case VIRTIO_PSTORE_CMD_READ:
> > +ret = virtio_pstore_do_read(s, elem);
> > +if (ret == 1) {
> > +/* async channel io */
> > +continue;
> > +}
> > +break;
> > +case VIRTIO_PSTORE_CMD_WRITE:
> > +ret = virtio_pstore_do_write(s, elem, );
> > +if (ret == 1) {
> > +/* async channel io */
> > +continue;
> > +}
> > +break;
> > +default:
> > +ret = -1;
> > +break;
> > +}
> > +
> > +res.ret = ret;
> 
> Missing cpu_to_le()?

Right!

> 
> > +static void pstore_set_bufsize(Object *obj, Visitor *v,
> > +   const char *name, void *opaque,
> > +   Error **errp)
> > +{
> > +VirtIOPstore *s = opaque;
> > +Error *error = NULL;
> > +uint64_t value;
> > +
> > +visit_type_size(v, name, , );
> > +if (error) {
> > +error_propagate(errp, error);
> > +return;
> > +}
> > +
> > +if (value < 4096) {
> > +error_setg(, "Warning: too small buffer size: %"PRIu64, 
> > value);
> 
> This is an error, not a warning.  Please remove "Warning:" so it's clear
> to the user that this message caused QEMU to fail.

Will do.

Thanks,
Namhyung

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Stefan Hajnoczi]

On Sun, Sep 04, 2016 at 11:38:59PM +0900, Namhyung Kim wrote:
> +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> +VirtQueueElement *elem;
> +struct virtio_pstore_req req;
> +struct virtio_pstore_res res;
> +ssize_t len = 0;
> +int ret;
> +
> +for (;;) {
> +elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +if (!elem) {
> +return;
> +}
> +
> +if (elem->out_num < 1 || elem->in_num < 1) {
> +error_report("request or response buffer is missing");
> +exit(1);

The new virtio_error() function might be available, depending on when
this patch series is merged.  virtio_error() should be used instead of
exit(1).  See "[PATCH v5 0/9] virtio: avoid exit() when device enters
invalid states" on qemu-devel.

> +}
> +
> +if (elem->out_num > 2 || elem->in_num > 3) {
> +error_report("invalid number of input/output buffer");
> +exit(1);
> +}

The VIRTIO specification requires that flexible framing is supported.
The device cannot make assumptions about the scatter-gather list.  It
must support any layout (e.g. even multiple 1-byte iovecs making up the
buffer).

> +
> +len = iov_to_buf(elem->out_sg, elem->out_num, 0, , sizeof(req));
> +if (len != (ssize_t)sizeof(req)) {
> +error_report("invalid request size: %ld", (long)len);
> +exit(1);
> +}
> +res.cmd  = req.cmd;
> +res.type = req.type;
> +
> +switch (le16_to_cpu(req.cmd)) {
> +case VIRTIO_PSTORE_CMD_OPEN:
> +ret = virtio_pstore_do_open(s);
> +break;
> +case VIRTIO_PSTORE_CMD_CLOSE:
> +ret = virtio_pstore_do_close(s);
> +break;
> +case VIRTIO_PSTORE_CMD_ERASE:
> +ret = virtio_pstore_do_erase(s, );
> +break;
> +case VIRTIO_PSTORE_CMD_READ:
> +ret = virtio_pstore_do_read(s, elem);
> +if (ret == 1) {
> +/* async channel io */
> +continue;
> +}
> +break;
> +case VIRTIO_PSTORE_CMD_WRITE:
> +ret = virtio_pstore_do_write(s, elem, );
> +if (ret == 1) {
> +/* async channel io */
> +continue;
> +}
> +break;
> +default:
> +ret = -1;
> +break;
> +}
> +
> +res.ret = ret;

Missing cpu_to_le()?

> +static void pstore_set_bufsize(Object *obj, Visitor *v,
> +   const char *name, void *opaque,
> +   Error **errp)
> +{
> +VirtIOPstore *s = opaque;
> +Error *error = NULL;
> +uint64_t value;
> +
> +visit_type_size(v, name, , );
> +if (error) {
> +error_propagate(errp, error);
> +return;
> +}
> +
> +if (value < 4096) {
> +error_setg(, "Warning: too small buffer size: %"PRIu64, value);

This is an error, not a warning.  Please remove "Warning:" so it's clear
to the user that this message caused QEMU to fail.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

On Tue, Sep 13, 2016 at 06:57:10PM +0300, Michael S. Tsirkin wrote:
> On Sat, Aug 20, 2016 at 05:07:43PM +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' property to set size of pstore bufer.
> > 
> > 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: Daniel P. Berrange 
> > 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 |  52 ++
> >  hw/virtio/virtio-pci.h |  14 +
> >  hw/virtio/virtio-pstore.c  | 699 
> > +
> >  include/hw/pci/pci.h   |   1 +
> >  include/hw/virtio/virtio-pstore.h  |  36 ++
> >  include/standard-headers/linux/virtio_ids.h|   1 +
> >  include/standard-headers/linux/virtio_pstore.h |  76 +++
> >  qdev-monitor.c |   1 +
> >  9 files changed, 881 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 755f921..c184823 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -2416,6 +2416,57 @@ 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);
> > +}
> > +
> > +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 

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Michael S. Tsirkin]

On Sat, Aug 20, 2016 at 05:07:43PM +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' property to set size of pstore bufer.
> 
> 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: Daniel P. Berrange 
> 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 |  52 ++
>  hw/virtio/virtio-pci.h |  14 +
>  hw/virtio/virtio-pstore.c  | 699 
> +
>  include/hw/pci/pci.h   |   1 +
>  include/hw/virtio/virtio-pstore.h  |  36 ++
>  include/standard-headers/linux/virtio_ids.h|   1 +
>  include/standard-headers/linux/virtio_pstore.h |  76 +++
>  qdev-monitor.c |   1 +
>  9 files changed, 881 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 755f921..c184823 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2416,6 +2416,57 @@ 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);
> +}
> +
> +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 +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 

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Daniel P. Berrange]

On Fri, Aug 26, 2016 at 01:48:40PM +0900, Namhyung Kim wrote:
> Hi Daniel,
> 
> On Wed, Aug 24, 2016 at 06:00:51PM -0400, Daniel P. Berrange wrote:

> > > +fd = open(filename, O_RDONLY);
> > > +if (fd < 0) {
> > > +error_report("cannot open %s", filename);
> > > +goto out;
> > > +}
> > > +
> > > +if (fstat(fd, ) < 0) {
> > > +goto out;
> > > +}
> > > +
> > > +rarg->vps= s;
> > > +rarg->elem   = elem;
> > > +rarg->info.id= cpu_to_le64(rarg->info.id);
> > > +rarg->info.type  = cpu_to_le16(rarg->info.type);
> > > +rarg->info.flags = cpu_to_le32(rarg->info.flags);
> > > +rarg->info.time_sec  = cpu_to_le64(stbuf.st_ctim.tv_sec);
> > > +rarg->info.time_nsec = cpu_to_le32(stbuf.st_ctim.tv_nsec);
> > > +
> > > +rarg->ioc = qio_channel_new_fd(fd, );
> > 
> > You should just use qio_channel_open_path() and avoid the earlier
> > call to open()
> 
> I did it because to call fstat() using the fd and wanted to keep the
> generic ioc pointer.

I'd suggest just using a cast inline, eg

  fstat(QIO_CHANNEL_FILE(ioc)->fd, )


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 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

Hi Daniel,

On Wed, Aug 24, 2016 at 06:00:51PM -0400, Daniel P. Berrange wrote:
> 
> > diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
> > new file mode 100644
> > index 000..b8fb4be
> > --- /dev/null
> > +++ b/hw/virtio/virtio-pstore.c
> > @@ -0,0 +1,699 @@
> > +/*
> > + * Virtio Pstore Device
> > + *
> > + * Copyright (C) 2016  LG Electronics
> > + *
> > + * Authors:
> > + *  Namhyung Kim  
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * 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 "io/channel-util.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"
> > +
> > +#define PSTORE_DEFAULT_BUFSIZE   (16 * 1024)
> > +#define PSTORE_DEFAULT_FILE_MAX  5
> > +
> > +/* the index should match to the type value */
> > +static const char *virtio_pstore_file_prefix[] = {
> > +"unknown-",/* VIRTIO_PSTORE_TYPE_UNKNOWN */
> > +"dmesg-",  /* VIRTIO_PSTORE_TYPE_DMESG */
> > +};
> > +
> > +static char *virtio_pstore_to_filename(VirtIOPstore *s,
> > +   struct virtio_pstore_req *req)
> > +{
> > +const char *basename;
> > +unsigned long long id;
> > +unsigned int type = le16_to_cpu(req->type);
> > +unsigned int flags = le32_to_cpu(req->flags);
> > +
> > +if (type < ARRAY_SIZE(virtio_pstore_file_prefix)) {
> > +basename = virtio_pstore_file_prefix[type];
> > +} else {
> > +basename = "unknown-";
> > +}
> > +
> > +id = s->id++;
> > +return g_strdup_printf("%s/%s%llu%s", s->directory, basename, id,
> > +flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" 
> > : "");
> > +}
> > +
> > +static char *virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> > + struct virtio_pstore_fileinfo 
> > *info)
> > +{
> > +char *filename;
> > +unsigned int idx;
> > +
> > +filename = g_strdup_printf("%s/%s", s->directory, name);
> > +if (filename == NULL)
> > +return NULL;
> > +
> > +for (idx = 0; idx < ARRAY_SIZE(virtio_pstore_file_prefix); idx++) {
> > +if (g_str_has_prefix(name, virtio_pstore_file_prefix[idx])) {
> > +info->type = idx;
> > +name += strlen(virtio_pstore_file_prefix[idx]);
> > +break;
> > +}
> > +}
> > +
> > +if (idx == ARRAY_SIZE(virtio_pstore_file_prefix)) {
> > +g_free(filename);
> > +return NULL;
> > +}
> > +
> > +qemu_strtoull(name, NULL, 0, >id);
> > +
> > +info->flags = 0;
> > +if (g_str_has_suffix(name, ".enc.z")) {
> > +info->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> > +}
> > +
> > +return filename;
> > +}
> > +
> > +static int prefix_idx;
> > +static int prefix_count;
> > +static int prefix_len;
> > +
> > +static int filter_pstore(const struct dirent *de)
> > +{
> > +int i;
> > +
> > +for (i = 0; i < prefix_count; i++) {
> > +const char *prefix = virtio_pstore_file_prefix[prefix_idx + i];
> > +
> > +if (g_str_has_prefix(de->d_name, prefix)) {
> > +return 1;
> > +}
> > +}
> > +return 0;
> > +}
> > +
> > +static int sort_pstore(const struct dirent **a, const struct dirent **b)
> > +{
> > +uint64_t id_a, id_b;
> > +
> > +qemu_strtoull((*a)->d_name + prefix_len, NULL, 0, _a);
> > +qemu_strtoull((*b)->d_name + prefix_len, NULL, 0, _b);
> > +
> > +return id_a - id_b;
> > +}
> > +
> > +static int rotate_pstore_file(VirtIOPstore *s, unsigned short type)
> 
> AFAIK you're not actually doing file rotation here - that implies a
> fixed base filename, with .0, .1, .2, etc suffixes where we rename
> files each time. It looks like you are assuming separate filenames,
> and are merely deleting the oldest each time.

Ah, right.  It's not rotation and I think it's enough for my purpose.
I need to change the name.

> 
> > +{
> > +int ret = 0;
> > +int i, num;
> > +char *filename;
> > +struct dirent **files;
> > +
> > +if (type >= ARRAY_SIZE(virtio_pstore_file_prefix)) {
> > +type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> > +}
> > +
> > +prefix_idx = type;
> > +prefix_len = strlen(virtio_pstore_file_prefix[type]);
> > +prefix_count = 1;  /* only scan current type */
> > +
> > +/* delete the oldest file in the same type */
> > +num = scandir(s->directory, , filter_pstore, sort_pstore);
> > +if (num < 0)
> > +return num;
> > +if (num < (int)s->file_max)
> > +goto 

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Daniel P. Berrange]

> diff --git a/hw/virtio/virtio-pstore.c b/hw/virtio/virtio-pstore.c
> new file mode 100644
> index 000..b8fb4be
> --- /dev/null
> +++ b/hw/virtio/virtio-pstore.c
> @@ -0,0 +1,699 @@
> +/*
> + * Virtio Pstore Device
> + *
> + * Copyright (C) 2016  LG Electronics
> + *
> + * Authors:
> + *  Namhyung Kim  
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * 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 "io/channel-util.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"
> +
> +#define PSTORE_DEFAULT_BUFSIZE   (16 * 1024)
> +#define PSTORE_DEFAULT_FILE_MAX  5
> +
> +/* the index should match to the type value */
> +static const char *virtio_pstore_file_prefix[] = {
> +"unknown-",  /* VIRTIO_PSTORE_TYPE_UNKNOWN */
> +"dmesg-",/* VIRTIO_PSTORE_TYPE_DMESG */
> +};
> +
> +static char *virtio_pstore_to_filename(VirtIOPstore *s,
> +   struct virtio_pstore_req *req)
> +{
> +const char *basename;
> +unsigned long long id;
> +unsigned int type = le16_to_cpu(req->type);
> +unsigned int flags = le32_to_cpu(req->flags);
> +
> +if (type < ARRAY_SIZE(virtio_pstore_file_prefix)) {
> +basename = virtio_pstore_file_prefix[type];
> +} else {
> +basename = "unknown-";
> +}
> +
> +id = s->id++;
> +return g_strdup_printf("%s/%s%llu%s", s->directory, basename, id,
> +flags & VIRTIO_PSTORE_FL_COMPRESSED ? ".enc.z" : 
> "");
> +}
> +
> +static char *virtio_pstore_from_filename(VirtIOPstore *s, char *name,
> + struct virtio_pstore_fileinfo *info)
> +{
> +char *filename;
> +unsigned int idx;
> +
> +filename = g_strdup_printf("%s/%s", s->directory, name);
> +if (filename == NULL)
> +return NULL;
> +
> +for (idx = 0; idx < ARRAY_SIZE(virtio_pstore_file_prefix); idx++) {
> +if (g_str_has_prefix(name, virtio_pstore_file_prefix[idx])) {
> +info->type = idx;
> +name += strlen(virtio_pstore_file_prefix[idx]);
> +break;
> +}
> +}
> +
> +if (idx == ARRAY_SIZE(virtio_pstore_file_prefix)) {
> +g_free(filename);
> +return NULL;
> +}
> +
> +qemu_strtoull(name, NULL, 0, >id);
> +
> +info->flags = 0;
> +if (g_str_has_suffix(name, ".enc.z")) {
> +info->flags |= VIRTIO_PSTORE_FL_COMPRESSED;
> +}
> +
> +return filename;
> +}
> +
> +static int prefix_idx;
> +static int prefix_count;
> +static int prefix_len;
> +
> +static int filter_pstore(const struct dirent *de)
> +{
> +int i;
> +
> +for (i = 0; i < prefix_count; i++) {
> +const char *prefix = virtio_pstore_file_prefix[prefix_idx + i];
> +
> +if (g_str_has_prefix(de->d_name, prefix)) {
> +return 1;
> +}
> +}
> +return 0;
> +}
> +
> +static int sort_pstore(const struct dirent **a, const struct dirent **b)
> +{
> +uint64_t id_a, id_b;
> +
> +qemu_strtoull((*a)->d_name + prefix_len, NULL, 0, _a);
> +qemu_strtoull((*b)->d_name + prefix_len, NULL, 0, _b);
> +
> +return id_a - id_b;
> +}
> +
> +static int rotate_pstore_file(VirtIOPstore *s, unsigned short type)

AFAIK you're not actually doing file rotation here - that implies a
fixed base filename, with .0, .1, .2, etc suffixes where we rename
files each time. It looks like you are assuming separate filenames,
and are merely deleting the oldest each time.

> +{
> +int ret = 0;
> +int i, num;
> +char *filename;
> +struct dirent **files;
> +
> +if (type >= ARRAY_SIZE(virtio_pstore_file_prefix)) {
> +type = VIRTIO_PSTORE_TYPE_UNKNOWN;
> +}
> +
> +prefix_idx = type;
> +prefix_len = strlen(virtio_pstore_file_prefix[type]);
> +prefix_count = 1;  /* only scan current type */
> +
> +/* delete the oldest file in the same type */
> +num = scandir(s->directory, , filter_pstore, sort_pstore);
> +if (num < 0)
> +return num;
> +if (num < (int)s->file_max)
> +goto out;
> +
> +filename = g_strdup_printf("%s/%s", s->directory, files[0]->d_name);
> +if (filename == NULL) {
> +ret = -1;
> +goto out;
> +}
> +
> +ret = unlink(filename);





> +static gboolean pstore_async_read_fn(QIOChannel *ioc, GIOCondition condition,
> + gpointer data)
> +{
> +struct pstore_read_arg *rarg = data;
> +struct virtio_pstore_fileinfo *info = >info;
> +VirtIOPstore *vps = rarg->vps;
> +

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

On Wed, Jul 20, 2016 at 09:29:06AM +0100, Stefan Hajnoczi wrote:
> On Mon, Jul 18, 2016 at 11:21:18PM +0900, Namhyung Kim wrote:
> > 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 
> > > > 
> > > > 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).
> 
> The pstore subsystem in Linux may be synchronous but the QEMU device
> emulation does not have to be synchronous.
> 
> Synchronous device emulation means that no other vcpu or QEMU main loop
> processing can occur while device emulation is blocked in a syscall.
> This can make the QEMU monitor unavailable for libvirt and management
> tools.  The guest can experience jitter since vcpus freeze if they
> vmexit while device emulation is blocked (it holds the QEMU global
> mutex and prevents other QEMU threads from making progress).

Thanks for your detailed explanation.  I'll try to change pstore
implementation to deal with async devices.

Thanks,
Namhyung

> 
> You could use include/io.h for asynchronous I/O (qio_channel_add_watch()).
> 
> Stefan

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

On Wed, Jul 20, 2016 at 09:21:08AM +0100, Stefan Hajnoczi wrote:
> On Wed, Jul 20, 2016 at 12:48:39AM +0900, Namhyung Kim wrote:
> > Hello,
> > 
> > On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote:
> > > On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote:
> > > > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> > > > +{
> > > > +VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> > > > +VirtQueueElement *elem;
> > > > +struct virtio_pstore_hdr *hdr;
> > > > +ssize_t len;
> > > > +
> > > > +for (;;) {
> > > > +elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > > > +if (!elem) {
> > > > +return;
> > > > +}
> > > > +
> > > > +hdr = elem->out_sg[0].iov_base;
> > > > +if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
> > > > +error_report("invalid header size: %u",
> > > > + (unsigned)elem->out_sg[0].iov_len);
> > > > +exit(1);
> > > > +}
> > > 
> > > Please use iov_to_buf() instead of directly accessing out_sg[].  Virtio
> > > devices are not supposed to assume a particular iovec layout.  In other
> > > words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs.
> > > 
> > > You must also copy in data (similar to Linux syscall implementations) to
> > > prevent the guest from modifying data while the command is processed.
> > > Such race conditions could lead to security bugs.
> > 
> > By accessing elem->out_sg[0].iov_base directly, I abused it as an
> > in-and-out buffer.  But it seems not allowed by the virtio spec, do I
> > have to use separate buffers for request and response?
> 
> Yes, a virtqueue element has (host read-only) out buffers followed by
> (host write-only) in buffers.

Thanks for clarification.  I'll split them.

Thanks,
Namhyung

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Stefan Hajnoczi]

On Wed, Jul 20, 2016 at 12:48:39AM +0900, Namhyung Kim wrote:
> Hello,
> 
> On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote:
> > On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote:
> > > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> > > +{
> > > +VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> > > +VirtQueueElement *elem;
> > > +struct virtio_pstore_hdr *hdr;
> > > +ssize_t len;
> > > +
> > > +for (;;) {
> > > +elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > > +if (!elem) {
> > > +return;
> > > +}
> > > +
> > > +hdr = elem->out_sg[0].iov_base;
> > > +if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
> > > +error_report("invalid header size: %u",
> > > + (unsigned)elem->out_sg[0].iov_len);
> > > +exit(1);
> > > +}
> > 
> > Please use iov_to_buf() instead of directly accessing out_sg[].  Virtio
> > devices are not supposed to assume a particular iovec layout.  In other
> > words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs.
> > 
> > You must also copy in data (similar to Linux syscall implementations) to
> > prevent the guest from modifying data while the command is processed.
> > Such race conditions could lead to security bugs.
> 
> By accessing elem->out_sg[0].iov_base directly, I abused it as an
> in-and-out buffer.  But it seems not allowed by the virtio spec, do I
> have to use separate buffers for request and response?

Yes, a virtqueue element has (host read-only) out buffers followed by
(host write-only) in buffers.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Stefan Hajnoczi]

On Mon, Jul 18, 2016 at 11:21:18PM +0900, Namhyung Kim wrote:
> 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 
> > > 
> > > 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).

The pstore subsystem in Linux may be synchronous but the QEMU device
emulation does not have to be synchronous.

Synchronous device emulation means that no other vcpu or QEMU main loop
processing can occur while device emulation is blocked in a syscall.
This can make the QEMU monitor unavailable for libvirt and management
tools.  The guest can experience jitter since vcpus freeze if they
vmexit while device emulation is blocked (it holds the QEMU global
mutex and prevents other QEMU threads from making progress).

You could use include/io.h for asynchronous I/O (qio_channel_add_watch()).

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

Hello,

On Mon, Jul 18, 2016 at 11:03:53AM +0100, Stefan Hajnoczi wrote:
> On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote:
> > +static void virtio_pstore_handle_io(VirtIODevice *vdev, VirtQueue *vq)
> > +{
> > +VirtIOPstore *s = VIRTIO_PSTORE(vdev);
> > +VirtQueueElement *elem;
> > +struct virtio_pstore_hdr *hdr;
> > +ssize_t len;
> > +
> > +for (;;) {
> > +elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> > +if (!elem) {
> > +return;
> > +}
> > +
> > +hdr = elem->out_sg[0].iov_base;
> > +if (elem->out_sg[0].iov_len != sizeof(*hdr)) {
> > +error_report("invalid header size: %u",
> > + (unsigned)elem->out_sg[0].iov_len);
> > +exit(1);
> > +}
> 
> Please use iov_to_buf() instead of directly accessing out_sg[].  Virtio
> devices are not supposed to assume a particular iovec layout.  In other
> words, virtio_pstore_hdr could be split across multiple out_sg[] iovecs.
> 
> You must also copy in data (similar to Linux syscall implementations) to
> prevent the guest from modifying data while the command is processed.
> Such race conditions could lead to security bugs.

By accessing elem->out_sg[0].iov_base directly, I abused it as an
in-and-out buffer.  But it seems not allowed by the virtio spec, do I
have to use separate buffers for request and response?

Thanks,
Namhyung

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

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 
> > 
> > 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 
> > 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_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, >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, ) < 0) {
> > +return -1;
> > +}
> 
> Please use fstat(fileno(fp), ) 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 

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Stefan Hajnoczi]

On Mon, Jul 18, 2016 at 01:37:40PM +0900, Namhyung Kim wrote:
> From: 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-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 
> 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 |  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(>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);
> +}
> +
> +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 

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Namhyung Kim]

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(>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);
> > +}
> > +
> > +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(_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"
> > 
> >  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
> > 
> 
> [...]
> 

Re: [Qemu-devel] [PATCH 2/3] qemu: Implement virtio-pstore device

[Christian Borntraeger]

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(>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);
> +}
> +
> +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(_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"
> 
>  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
> 

[...]