From: Paolo Bonzini <pbonzini@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>,
Namhyung Kim <namhyung@kernel.org>
Cc: virtio-dev@lists.oasis-open.org,
"Tony Luck" <tony.luck@intel.com>,
"Kees Cook" <keescook@chromium.org>,
kvm@vger.kernel.org, "Radim Krčmář" <rkrcmar@redhat.com>,
"Anton Vorontsov" <anton@enomsg.org>,
LKML <linux-kernel@vger.kernel.org>,
"Steven Rostedt" <rostedt@goodmis.org>,
qemu-devel@nongnu.org, "Minchan Kim" <minchan@kernel.org>,
"Anthony Liguori" <aliguori@amazon.com>,
"Colin Cross" <ccross@android.com>,
virtualization@lists.linux-foundation.org,
"Ingo Molnar" <mingo@kernel.org>
Subject: Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Date: Tue, 15 Nov 2016 10:57:29 +0100 [thread overview]
Message-ID: <f514ac65-3105-a12c-9e10-2d3b1e8a0516@redhat.com> (raw)
In-Reply-To: <20161115065658-mutt-send-email-mst@kernel.org>
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 <pbonzini@redhat.com>
>>>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Anthony Liguori <aliguori@amazon.com>
>>>> Cc: Anton Vorontsov <anton@enomsg.org>
>>>> Cc: Colin Cross <ccross@android.com>
>>>> Cc: Kees Cook <keescook@chromium.org>
>>>> 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: kvm@vger.kernel.org
>>>> Cc: qemu-devel@nongnu.org
>>>> Cc: virtualization@lists.linux-foundation.org
>>>> Signed-off-by: Namhyung Kim <namhyung@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
>
next prev parent reply other threads:[~2016-11-15 9:57 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-20 8:07 [RFC/PATCHSET 0/3] virtio: Implement virtio pstore device (v3) Namhyung Kim
2016-08-20 8:07 ` [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim
2016-09-13 15:19 ` Michael S. Tsirkin
2016-09-16 9:05 ` Namhyung Kim
2016-11-10 16:39 ` Michael S. Tsirkin
2016-11-15 4:50 ` Namhyung Kim
2016-11-15 5:06 ` Michael S. Tsirkin
2016-11-15 5:50 ` Namhyung Kim
2016-11-15 14:35 ` Michael S. Tsirkin
2016-11-15 9:57 ` Paolo Bonzini [this message]
2016-11-15 14:36 ` Namhyung Kim
2016-11-15 14:38 ` Paolo Bonzini
2016-11-16 7:04 ` Namhyung Kim
2016-11-16 12:10 ` Paolo Bonzini
2016-11-18 3:32 ` Namhyung Kim
2016-11-18 4:07 ` Michael S. Tsirkin
2016-11-18 9:46 ` [virtio-dev] " Paolo Bonzini
2016-11-18 9:45 ` Paolo Bonzini
2016-08-20 8:07 ` [PATCH 2/3] qemu: Implement virtio-pstore device Namhyung Kim
2016-08-24 22:00 ` Daniel P. Berrange
2016-08-26 4:48 ` Namhyung Kim
2016-08-26 12:27 ` Daniel P. Berrange
2016-09-13 15:57 ` Michael S. Tsirkin
2016-09-16 10:05 ` Namhyung Kim
2016-11-10 22:50 ` Michael S. Tsirkin
2016-11-15 6:23 ` Namhyung Kim
2016-11-15 14:38 ` Michael S. Tsirkin
2016-08-20 8:07 ` [PATCH 3/3] kvmtool: " Namhyung Kim
2016-08-23 10:25 ` [RFC/PATCHSET 0/3] virtio: Implement virtio pstore device (v3) Joel Fernandes
2016-08-23 15:20 ` Namhyung Kim
2016-08-24 7:10 ` Joel
-- strict thread matches above, loose matches on Subject: below --
2016-09-04 14:38 [RFC/PATCHSET 0/3] virtio: Implement virtio pstore device (v5) Namhyung Kim
2016-09-04 14:38 ` [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim
2016-09-08 20:49 ` Kees Cook
2016-09-22 11:57 ` Stefan Hajnoczi
2016-09-23 5:48 ` Namhyung Kim
2016-08-31 8:07 [RFC/PATCHSET 0/3] virtio: Implement virtio pstore device (v4) Namhyung Kim
2016-08-31 8:08 ` [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim
2016-08-31 14:54 ` Michael S. Tsirkin
2016-09-01 0:03 ` Namhyung Kim
2016-07-18 4:37 [RFC/PATCHSET 0/3] virtio-pstore: Implement virtio pstore device Namhyung Kim
2016-07-18 4:37 ` [PATCH 1/3] virtio: Basic implementation of virtio pstore driver Namhyung Kim
2016-07-18 5:12 ` Kees Cook
2016-07-18 5:50 ` Namhyung Kim
2016-07-18 17:50 ` Kees Cook
2016-07-19 13:43 ` Namhyung Kim
2016-07-19 15:32 ` Namhyung Kim
2016-07-20 12:56 ` Namhyung Kim
2016-07-18 7:54 ` Cornelia Huck
2016-07-18 8:29 ` Namhyung Kim
2016-07-18 9:02 ` Cornelia Huck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f514ac65-3105-a12c-9e10-2d3b1e8a0516@redhat.com \
--to=pbonzini@redhat.com \
--cc=aliguori@amazon.com \
--cc=anton@enomsg.org \
--cc=ccross@android.com \
--cc=keescook@chromium.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=minchan@kernel.org \
--cc=mingo@kernel.org \
--cc=mst@redhat.com \
--cc=namhyung@kernel.org \
--cc=qemu-devel@nongnu.org \
--cc=rkrcmar@redhat.com \
--cc=rostedt@goodmis.org \
--cc=tony.luck@intel.com \
--cc=virtio-dev@lists.oasis-open.org \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).