Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver

Hi Michael,

On Wed, Aug 31, 2016 at 05:54:04PM +0300, Michael S. Tsirkin wrote:

On Wed, Aug 31, 2016 at 05:08:00PM +0900, Namhyung Kim wrote:

quoted

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.  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 <pbonzini@redhat.com>
Cc: Radim Krčmář <redacted>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Anthony Liguori <redacted>
Cc: Anton Vorontsov <redacted>
Cc: Colin Cross <redacted>
Cc: Kees Cook <redacted>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Will Deacon <redacted>
Cc: kvm@vger.kernel.org
Cc: qemu-devel@nongnu.org
Cc: virtualization@lists.linux-foundation.org
Cc: virtio-dev@lists.oasis-open.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---

[SNIP]

quoted

+#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
+
+
+static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(type_table); i++) {
+		if (type == type_table[i].pstore)
+			return cpu_to_virtio16(vps->vdev, type_table[i].virtio);

Does this pass sparse checks? If yes I'm surprised - this clearly
returns a virtio16 type.

Ah, didn't run sparse.  Will change it to return a __le16 type
(according to your comment below).

quoted

+	}
+
+	return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
+}
+
+static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type)

This one should be '__le16 type' as well.

quoted

+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(type_table); i++) {
+		if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio)
+			return type_table[i].pstore;
+	}
+
+	return PSTORE_TYPE_UNKNOWN;
+}
+

[SNIP]

quoted

+
+struct virtio_pstore_req {
+	__virtio16		cmd;
+	__virtio16		type;
+	__virtio32		flags;
+	__virtio64		id;
+	__virtio32		count;
+	__virtio32		reserved;
+};
+
+struct virtio_pstore_res {
+	__virtio16		cmd;
+	__virtio16		type;
+	__virtio32		ret;
+};

Is there a reason to support legacy endian-ness?
If not, you can just use __le formats.

I just didn't know what's the preferred type.  Will change!

Thanks,
Namhyung

quoted

+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;
+};
+
+#endif /* _LINUX_VIRTIO_PSTORE_H */
-- 
2.9.3

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization