Re: [PATCH 2/3] qemu: Implement virtio-pstore device
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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
On Tue, Nov 15, 2016 at 07:06:28AM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> > On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> > [SNIP]
> > > > +struct virtio_pstore_fileinfo {
> > > > + __virtio64 id;
> > > > + __virtio32 count;
> > > > + __virtio16 type;
> > > > + __virtio16 unused;
> > > > + __virtio32 flags;
> > > > + __virtio32 len;
> > > > + __virtio64 time_sec;
> > > > + __virtio32 time_nsec;
> > > > + __virtio32 reserved;
> > > > +};
> > > > +
> > > > +struct virtio_pstore_config {
> > > > + __virtio32 bufsize;
> > > > +};
> > > > +
> > >
> > > What exactly does each field mean? I'm especially
> > > interested in time fields - maintaining a consistent
> > > time between host and guest is not a simple problem.
> >
> > These are required by pstore and will be used to create corresponding
> > files in the pstore filesystem. The time fields are for mtime and
> > ctime and, I think, it's just a hint for user and doesn't require
> > strict consistency.
>
> Pls add documentation. I would just drop hints for now.
Well, I'll add docmentation. But I think just dropping might not good
since they all have host time and it's helpful to know their relative
difference in guest.
Thanks,
Namhyung
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
On Tue, Nov 15, 2016 at 01:50:21PM +0900, Namhyung Kim wrote:
> Hi Michael,
>
> On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> > On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> > > The virtio pstore driver provides interface to the pstore subsystem so
> > > that the guest kernel's log/dump message can be saved on the host
> > > machine. Users can access the log file directly on the host, or on the
> > > guest at the next boot using pstore filesystem. It currently deals with
> > > kernel log (printk) buffer only, but we can extend it to have other
> > > information (like ftrace dump) later.
> > >
> > > It supports legacy PCI device using single order-2 page buffer.
> >
> > Do you mean a legacy virtio device? I don't see why
> > you would want to support pre-1.0 mode.
> > If you drop that, you can drop all cpu_to_virtio things
> > and just use __le accessors.
>
> I was thinking about the kvmtools which lacks 1.0 support AFAIK.
Unless kvmtools wants to be left behind it has to go 1.0.
> But
> I think it'd be better to always use __le type anyway. Will change.
>
>
> >
> > > It uses
> > > two virtqueues - one for (sync) read and another for (async) write.
> > > Since it cannot wait for write finished, it supports up to 128
> > > concurrent IO. The buffer size is configurable now.
> > >
> > > 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-de...@nongnu.org
> > > Cc: virtualization@lists.linux-foundation.org
> > > Signed-off-by: Namhyung Kim
> > > ---
> > > drivers/virtio/Kconfig | 10 +
> > > drivers/virtio/Makefile| 1 +
> > > drivers/virtio/virtio_pstore.c | 417
> > > +
> > > include/uapi/linux/Kbuild | 1 +
> > > include/uapi/linux/virtio_ids.h| 1 +
> > > include/uapi/linux/virtio_pstore.h | 74 +++
> > > 6 files changed, 504 insertions(+)
> > > create mode 100644 drivers/virtio/virtio_pstore.c
> > > create mode 100644 include/uapi/linux/virtio_pstore.h
> > >
> > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > index 77590320d44c..8f0e6c796c12 100644
> > > --- a/drivers/virtio/Kconfig
> > > +++ b/drivers/virtio/Kconfig
> > > @@ -58,6 +58,16 @@ config VIRTIO_INPUT
> > >
> > >If unsure, say M.
> > >
> > > +config VIRTIO_PSTORE
> > > + tristate "Virtio pstore driver"
> > > + depends on VIRTIO
> > > + depends on PSTORE
> > > + ---help---
> > > + This driver supports virtio pstore devices to save/restore
> > > + panic and oops messages on the host.
> > > +
> > > + If unsure, say M.
> > > +
> > > config VIRTIO_MMIO
> > > tristate "Platform bus driver for memory mapped virtio devices"
> > > depends on HAS_IOMEM && HAS_DMA
> > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > index 41e30e3dc842..bee68cb26d48 100644
> > > --- a/drivers/virtio/Makefile
> > > +++ b/drivers/virtio/Makefile
> > > @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > > virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > > obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > > obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > > +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> > > diff --git a/drivers/virtio/virtio_pstore.c
> > > b/drivers/virtio/virtio_pstore.c
> > > new file mode 100644
> > > index ..0a63c7db4278
> > > --- /dev/null
> > > +++ b/drivers/virtio/virtio_pstore.c
> > > @@ -0,0 +1,417 @@
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#define VIRT_PSTORE_ORDER2
> > > +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
> > > +#define VIRT_PSTORE_NR_REQ 128
> > > +
> > > +struct virtio_pstore {
> > > + struct virtio_device*vdev;
> > > + struct virtqueue*vq[2];
> >
> > I'd add named fields instead of an array here, vq[0]
> > vq[1] all over the place is hard to read.
>
> Will change.
>
> >
> > > + struct pstore_info pstore;
> > > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> > > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> > > + unsigned int req_id;
> > > +
> > > + /* Waiting for host to ack */
> > > + wait_queue_head_t acked;
> > > + int failed;
> > > +};
> > > +
> > > +#define TYPE_TABLE_ENTRY(_entry) \
> > > + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> > > +
> > > +struct type_table {
> > > + int pstore;
> > > + u16 virtio;
> > > +} type_table[] = {
> > > + TYPE_TABLE_ENTRY(DMESG),
> > > +};
> > > +
> > > +#undef TYPE_TABLE_ENTRY
> >
> > let's avoi
Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Hi Michael,
On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine. Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem. It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> >
> > It supports legacy PCI device using single order-2 page buffer.
>
> Do you mean a legacy virtio device? I don't see why
> you would want to support pre-1.0 mode.
> If you drop that, you can drop all cpu_to_virtio things
> and just use __le accessors.
I was thinking about the kvmtools which lacks 1.0 support AFAIK. But
I think it'd be better to always use __le type anyway. Will change.
>
> > It uses
> > two virtqueues - one for (sync) read and another for (async) write.
> > Since it cannot wait for write finished, it supports up to 128
> > concurrent IO. The buffer size is configurable now.
> >
> > 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-de...@nongnu.org
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Namhyung Kim
> > ---
> > drivers/virtio/Kconfig | 10 +
> > drivers/virtio/Makefile| 1 +
> > drivers/virtio/virtio_pstore.c | 417
> > +
> > include/uapi/linux/Kbuild | 1 +
> > include/uapi/linux/virtio_ids.h| 1 +
> > include/uapi/linux/virtio_pstore.h | 74 +++
> > 6 files changed, 504 insertions(+)
> > create mode 100644 drivers/virtio/virtio_pstore.c
> > create mode 100644 include/uapi/linux/virtio_pstore.h
> >
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 77590320d44c..8f0e6c796c12 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -58,6 +58,16 @@ config VIRTIO_INPUT
> >
> > If unsure, say M.
> >
> > +config VIRTIO_PSTORE
> > + tristate "Virtio pstore driver"
> > + depends on VIRTIO
> > + depends on PSTORE
> > + ---help---
> > +This driver supports virtio pstore devices to save/restore
> > +panic and oops messages on the host.
> > +
> > +If unsure, say M.
> > +
> > config VIRTIO_MMIO
> > tristate "Platform bus driver for memory mapped virtio devices"
> > depends on HAS_IOMEM && HAS_DMA
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 41e30e3dc842..bee68cb26d48 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> > diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> > new file mode 100644
> > index ..0a63c7db4278
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_pstore.c
> > @@ -0,0 +1,417 @@
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define VIRT_PSTORE_ORDER2
> > +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
> > +#define VIRT_PSTORE_NR_REQ 128
> > +
> > +struct virtio_pstore {
> > + struct virtio_device*vdev;
> > + struct virtqueue*vq[2];
>
> I'd add named fields instead of an array here, vq[0]
> vq[1] all over the place is hard to read.
Will change.
>
> > + struct pstore_info pstore;
> > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> > + unsigned int req_id;
> > +
> > + /* Waiting for host to ack */
> > + wait_queue_head_t acked;
> > + int failed;
> > +};
> > +
> > +#define TYPE_TABLE_ENTRY(_entry) \
> > + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> > +
> > +struct type_table {
> > + int pstore;
> > + u16 virtio;
> > +} type_table[] = {
> > + TYPE_TABLE_ENTRY(DMESG),
> > +};
> > +
> > +#undef TYPE_TABLE_ENTRY
>
> let's avoid macros for now pls. In fact, I would just open-code this
> in to_virtio_type below. We can always change our minds later if
> lots of types are added.
Yep.
>
> > +
> > +
>
> single emoty line pls
Ok.
>
> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id
> > type)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i
Re: [PATCH] virtio_ring: fix description of virtqueue_get_buf
On 2016年11月14日 22:16, Felipe Franciosi wrote: The device (not the driver) populates the used ring and includes the len of how much data was written. Signed-off-by: Felipe Franciosi --- drivers/virtio/virtio_ring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 489bfc6..8a0d6a9 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -649,7 +649,7 @@ static inline bool more_used(const struct vring_virtqueue *vq) * @vq: the struct virtqueue we're talking about. * @len: the length written into the buffer * - * If the driver wrote data into the buffer, @len will be set to the + * If the device wrote data into the buffer, @len will be set to the * amount written. This means you don't need to clear the buffer * beforehand to ensure there's no data leakage in the case of short * writes. Reviewed-by: Jason Wang ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio_ring: fix description of virtqueue_get_buf
The device (not the driver) populates the used ring and includes the len of how much data was written. Signed-off-by: Felipe Franciosi --- drivers/virtio/virtio_ring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 489bfc6..8a0d6a9 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -649,7 +649,7 @@ static inline bool more_used(const struct vring_virtqueue *vq) * @vq: the struct virtqueue we're talking about. * @len: the length written into the buffer * - * If the driver wrote data into the buffer, @len will be set to the + * If the device wrote data into the buffer, @len will be set to the * amount written. This means you don't need to clear the buffer * beforehand to ensure there's no data leakage in the case of short * writes. -- 1.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
