On 15/11/2016 06:06, Michael S. Tsirkin wrote:
> 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.
And it also has to go ACPI. Is there any reason, apart from kvmtool, to
make a completely new virtio device, with no support in existing guests,
rather than implement ACPI ERST?
Paolo
>> 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 <pbonz...@redhat.com>
>>>> Cc: Radim Krčmář <rkrc...@redhat.com>
>>>> Cc: "Michael S. Tsirkin" <m...@redhat.com>
>>>> Cc: Anthony Liguori <aligu...@amazon.com>
>>>> Cc: Anton Vorontsov <an...@enomsg.org>
>>>> Cc: Colin Cross <ccr...@android.com>
>>>> Cc: Kees Cook <keesc...@chromium.org>
>>>> Cc: Tony Luck <tony.l...@intel.com>
>>>> Cc: Steven Rostedt <rost...@goodmis.org>
>>>> Cc: Ingo Molnar <mi...@kernel.org>
>>>> Cc: Minchan Kim <minc...@kernel.org>
>>>> Cc: k...@vger.kernel.org
>>>> Cc: qemu-de...@nongnu.org
>>>> Cc: virtualization@lists.linux-foundation.org
>>>> Signed-off-by: Namhyung Kim <namhy...@kernel.org>
>>>> ---
>>>> 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 000000000000..0a63c7db4278
>>>> --- /dev/null
>>>> +++ b/drivers/virtio/virtio_pstore.c
>>>> @@ -0,0 +1,417 @@
>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/pstore.h>
>>>> +#include <linux/virtio.h>
>>>> +#include <linux/virtio_config.h>
>>>> +#include <uapi/linux/virtio_ids.h>
>>>> +#include <uapi/linux/virtio_pstore.h>
>>>> +
>>>> +#define VIRT_PSTORE_ORDER 2
>>>> +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
>>>> +#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 < ARRAY_SIZE(type_table); i++) {
>>>> + if (type == type_table[i].pstore)
>>>> + return cpu_to_virtio16(vps->vdev, type_table[i].virtio);
>>>> + }
>>>> +
>>>> + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
>>>
>>> This assigns u16 to __virtio type, sparse will warn
>>> if you enable endian-ness checks.
>>> Pls fix that and generally, please make sure this is
>>> clean from sparse warnings.
>>
>> I'll run sparse before sending patch next time.
>>
>>>
>>>> +}
>>>> +
>>>> +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps,
>>>> u16 type)
>>>> +{
>>>> + 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;
>>>> +}
>>>> +
>>>> +static void virtpstore_ack(struct virtqueue *vq)
>>>> +{
>>>> + struct virtio_pstore *vps = vq->vdev->priv;
>>>> +
>>>> + wake_up(&vps->acked);
>>>> +}
>>>> +
>>>> +static void virtpstore_check(struct virtqueue *vq)
>>>> +{
>>>> + struct virtio_pstore *vps = vq->vdev->priv;
>>>> + struct virtio_pstore_res *res;
>>>> + unsigned int len;
>>>> +
>>>> + res = virtqueue_get_buf(vq, &len);
>>>> + if (res == NULL)
>>>> + return;
>>>> +
>>>> + if (virtio32_to_cpu(vq->vdev, res->ret) < 0)
>>>> + vps->failed = 1;
>>>> +}
>>>> +
>>>> +static void virt_pstore_get_reqs(struct virtio_pstore *vps,
>>>> + struct virtio_pstore_req **preq,
>>>> + struct virtio_pstore_res **pres)
>>>> +{
>>>> + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ;
>>>> +
>>>> + *preq = &vps->req[idx];
>>>> + *pres = &vps->res[idx];
>>>> +
>>>> + memset(*preq, 0, sizeof(**preq));
>>>> + memset(*pres, 0, sizeof(**pres));
>>>> +}
>>>> +
>>>> +static int virt_pstore_open(struct pstore_info *psi)
>>>> +{
>>>> + struct virtio_pstore *vps = psi->data;
>>>> + struct virtio_pstore_req *req;
>>>> + struct virtio_pstore_res *res;
>>>> + struct scatterlist sgo[1], sgi[1];
>>>> + struct scatterlist *sgs[2] = { sgo, sgi };
>>>> + unsigned int len;
>>>> +
>>>> + virt_pstore_get_reqs(vps, &req, &res);
>>>> +
>>>> + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN);
>>>> +
>>>> + sg_init_one(sgo, req, sizeof(*req));
>>>> + sg_init_one(sgi, res, sizeof(*res));
>>>> + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
>>>> + virtqueue_kick(vps->vq[0]);
>>>> +
>>>> + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
>>>
>>> Does this block userspace in an uninterruptible wait if
>>> hardware is slow? That's not nice.
>>
>> Yes, but it's not a common operation and I just wanted to make it
>> simple.
>>
>>
>>>
>>>> + return virtio32_to_cpu(vps->vdev, res->ret);
>>>> +}
>>>> +
>>
>> [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.
>
>>
>> Thanks for your review!
>> Namhyung
>>
>>>
>>>> +#endif /* _LINUX_VIRTIO_PSTORE_H */
>>>> --
>>>> 2.9.3
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
