Namhyung Kim
2016-Jul-18 05:50 UTC
[PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Hello, On Sun, Jul 17, 2016 at 10:12:26PM -0700, Kees Cook wrote:> On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim <namhyung at kernel.org> 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. As all > > operation of pstore is synchronous, it would be fine IMHO. However I > > don't know how to make write operation synchronous since it's called > > with a spinlock held (from any context including NMI). > > > > Cc: Paolo Bonzini <pbonzini at redhat.com> > > Cc: Radim Kr??m???? <rkrcmar at redhat.com> > > Cc: "Michael S. Tsirkin" <mst at redhat.com> > > Cc: Anthony Liguori <aliguori at amazon.com> > > Cc: Anton Vorontsov <anton at enomsg.org> > > Cc: Colin Cross <ccross at android.com> > > Cc: Kees Cook <keescook at chromium.org> > > Cc: Tony Luck <tony.luck at intel.com> > > Cc: Steven Rostedt <rostedt at goodmis.org> > > Cc: Ingo Molnar <mingo at kernel.org> > > Cc: Minchan Kim <minchan at kernel.org> > > Cc: kvm at vger.kernel.org > > Cc: qemu-devel at nongnu.org > > Cc: virtualization at lists.linux-foundation.org > > Signed-off-by: Namhyung Kim <namhyung at kernel.org> > > This looks great to me! I'd love to use this in qemu. (Right now I go > through hoops to use the ramoops backend for testing.) > > Reviewed-by: Kees Cook <keescook at chromium.org>Thank you!> > Notes below... >[SNIP]> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) > > +{ > > + u16 ret; > > + > > + switch (type) { > > + case PSTORE_TYPE_DMESG: > > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG); > > + break; > > + default: > > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); > > + break; > > + } > > I would love to see this support PSTORE_TYPE_CONSOLE too. It should be > relatively easy to add: I think it'd just be another virtio command?Do you want to append the data to the host file as guest does printk()? I think it needs some kind of buffer management, but it's not hard to add IMHO.> > > + > > + return ret; > > +} > > +[SNIP]> > +static int notrace virt_pstore_write(enum pstore_type_id type, > > + enum kmsg_dump_reason reason, > > + u64 *id, unsigned int part, int count, > > + bool compressed, size_t size, > > + struct pstore_info *psi) > > +{ > > + struct virtio_pstore *vps = psi->data; > > + struct virtio_pstore_hdr *hdr = &vps->hdr; > > + struct scatterlist sg[2]; > > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0; > > + > > + *id = vps->id++; > > + > > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE); > > + hdr->id = cpu_to_virtio64(vps->vdev, *id); > > + hdr->flags = cpu_to_virtio32(vps->vdev, flags); > > + hdr->type = to_virtio_type(vps, type); > > + > > + sg_init_table(sg, 2); > > + sg_set_buf(&sg[0], hdr, sizeof(*hdr)); > > + sg_set_buf(&sg[1], psi->buf, size); > > + virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC); > > + virtqueue_kick(vps->vq); > > + > > + /* TODO: make it synchronous */ > > + return 0; > > The down side to this being asynchronous is the lack of error > reporting. Perhaps this could check hdr->type before queuing and error > for any VIRTIO_PSTORE_TYPE_UNKNOWN message instead of trying to send > it?I cannot follow, sorry. Could you please elaborate it more?> > > +} > > + > > +static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count, > > + struct timespec time, struct pstore_info *psi) > > +{ > > + struct virtio_pstore *vps = psi->data; > > + struct virtio_pstore_hdr *hdr = &vps->hdr; > > + struct scatterlist sg[1]; > > + unsigned int len; > > + > > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE); > > + hdr->id = cpu_to_virtio64(vps->vdev, id); > > + hdr->type = to_virtio_type(vps, type); > > + > > + sg_init_one(sg, hdr, sizeof(*hdr)); > > + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL); > > + virtqueue_kick(vps->vq); > > + > > + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len)); > > + return 0; > > +} > > + > > +static int virt_pstore_init(struct virtio_pstore *vps) > > +{ > > + struct pstore_info *psinfo = &vps->pstore; > > + int err; > > + > > + vps->id = 0; > > + vps->buflen = 0; > > + psinfo->bufsize = VIRT_PSTORE_BUFSIZE; > > + psinfo->buf = (void *)__get_free_pages(GFP_KERNEL, VIRT_PSTORE_ORDER); > > + if (!psinfo->buf) { > > + pr_err("cannot allocate pstore buffer\n"); > > + return -ENOMEM; > > + } > > + > > + psinfo->owner = THIS_MODULE; > > + psinfo->name = "virtio"; > > + psinfo->open = virt_pstore_open; > > + psinfo->close = virt_pstore_close; > > + psinfo->read = virt_pstore_read; > > + psinfo->erase = virt_pstore_erase; > > + psinfo->write = virt_pstore_write; > > + psinfo->flags = PSTORE_FLAGS_FRAGILE; > > For console support, this flag would need to be dropped -- though I > suspect you know that already.:)Yep, I intentionally support DMESG type only in this patchset for simplicity. Others could be added later. :)> > > + psinfo->data = vps; > > + spin_lock_init(&psinfo->buf_lock); > > + > > + err = pstore_register(psinfo); > > + if (err) > > + kfree(psinfo->buf); > > + > > + return err; > > +}[SNIP]> > Awesome! Can't wait to use it. :)Thanks for your review! :) Thanks, Namhyung> > -Kees > > -- > Kees Cook > Chrome OS & Brillo Security
Kees Cook
2016-Jul-18 17:50 UTC
[PATCH 1/3] virtio: Basic implementation of virtio pstore driver
On Sun, Jul 17, 2016 at 10:50 PM, Namhyung Kim <namhyung at kernel.org> wrote:> Hello, > > On Sun, Jul 17, 2016 at 10:12:26PM -0700, Kees Cook wrote: >> On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim <namhyung at kernel.org> 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. As all >> > operation of pstore is synchronous, it would be fine IMHO. However I >> > don't know how to make write operation synchronous since it's called >> > with a spinlock held (from any context including NMI). >> > >> > Cc: Paolo Bonzini <pbonzini at redhat.com> >> > Cc: Radim Kr??m???? <rkrcmar at redhat.com> >> > Cc: "Michael S. Tsirkin" <mst at redhat.com> >> > Cc: Anthony Liguori <aliguori at amazon.com> >> > Cc: Anton Vorontsov <anton at enomsg.org> >> > Cc: Colin Cross <ccross at android.com> >> > Cc: Kees Cook <keescook at chromium.org> >> > Cc: Tony Luck <tony.luck at intel.com> >> > Cc: Steven Rostedt <rostedt at goodmis.org> >> > Cc: Ingo Molnar <mingo at kernel.org> >> > Cc: Minchan Kim <minchan at kernel.org> >> > Cc: kvm at vger.kernel.org >> > Cc: qemu-devel at nongnu.org >> > Cc: virtualization at lists.linux-foundation.org >> > Signed-off-by: Namhyung Kim <namhyung at kernel.org> >> >> This looks great to me! I'd love to use this in qemu. (Right now I go >> through hoops to use the ramoops backend for testing.) >> >> Reviewed-by: Kees Cook <keescook at chromium.org> > > Thank you! > >> >> Notes below... >> > > [SNIP] >> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) >> > +{ >> > + u16 ret; >> > + >> > + switch (type) { >> > + case PSTORE_TYPE_DMESG: >> > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG); >> > + break; >> > + default: >> > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); >> > + break; >> > + } >> >> I would love to see this support PSTORE_TYPE_CONSOLE too. It should be >> relatively easy to add: I think it'd just be another virtio command? > > Do you want to append the data to the host file as guest does > printk()? I think it needs some kind of buffer management, but it's > not hard to add IMHO.Well, with most pstore backends, the buffer size is limited, so it tends to be a circular buffer of some sort. I think whatever you choose to do is fine (I saw the various mentions of resource limits in the qemu part of this thread), as long as the last N bytes of console can be seen on the host side, where N is some portion of the memory set aside for the log. (I don't mind the idea of an unlimited console log either, but I suspect that will not be accepted on the qemu side...)> [SNIP] >> > +static int notrace virt_pstore_write(enum pstore_type_id type, >> > + enum kmsg_dump_reason reason, >> > + u64 *id, unsigned int part, int count, >> > + bool compressed, size_t size, >> > + struct pstore_info *psi) >> > +{ >> > + struct virtio_pstore *vps = psi->data; >> > + struct virtio_pstore_hdr *hdr = &vps->hdr; >> > + struct scatterlist sg[2]; >> > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0; >> > + >> > + *id = vps->id++; >> > + >> > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE); >> > + hdr->id = cpu_to_virtio64(vps->vdev, *id); >> > + hdr->flags = cpu_to_virtio32(vps->vdev, flags); >> > + hdr->type = to_virtio_type(vps, type); >> > + >> > + sg_init_table(sg, 2); >> > + sg_set_buf(&sg[0], hdr, sizeof(*hdr)); >> > + sg_set_buf(&sg[1], psi->buf, size); >> > + virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC); >> > + virtqueue_kick(vps->vq); >> > + >> > + /* TODO: make it synchronous */ >> > + return 0; >> >> The down side to this being asynchronous is the lack of error >> reporting. Perhaps this could check hdr->type before queuing and error >> for any VIRTIO_PSTORE_TYPE_UNKNOWN message instead of trying to send >> it? > > I cannot follow, sorry. Could you please elaborate it more?The mention you have here of "TODO: make it synchronous" made me think about what effects that could have. If a pstore_write() were issued for a type other than DMESG, the above code would send it through virtio anyway. No error reporting is possible unless this is synchronous, but the only error here would simply be "I don't know what anything except DMESG is", so maybe this code could refuse to forward anything with type UNKNOWN in the first place. (Just an idea: I don't think there's anything very wrong here. It just seemed like a potential improvement.)>> > +} >> > + >> > +static int virt_pstore_erase(enum pstore_type_id type, u64 id, int count, >> > + struct timespec time, struct pstore_info *psi) >> > +{ >> > + struct virtio_pstore *vps = psi->data; >> > + struct virtio_pstore_hdr *hdr = &vps->hdr; >> > + struct scatterlist sg[1]; >> > + unsigned int len; >> > + >> > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_ERASE); >> > + hdr->id = cpu_to_virtio64(vps->vdev, id); >> > + hdr->type = to_virtio_type(vps, type); >> > + >> > + sg_init_one(sg, hdr, sizeof(*hdr)); >> > + virtqueue_add_outbuf(vps->vq, sg, 1, vps, GFP_KERNEL); >> > + virtqueue_kick(vps->vq); >> > + >> > + wait_event(vps->acked, virtqueue_get_buf(vps->vq, &len)); >> > + return 0; >> > +} >> > + >> > +static int virt_pstore_init(struct virtio_pstore *vps) >> > +{ >> > + struct pstore_info *psinfo = &vps->pstore; >> > + int err; >> > + >> > + vps->id = 0; >> > + vps->buflen = 0; >> > + psinfo->bufsize = VIRT_PSTORE_BUFSIZE; >> > + psinfo->buf = (void *)__get_free_pages(GFP_KERNEL, VIRT_PSTORE_ORDER); >> > + if (!psinfo->buf) { >> > + pr_err("cannot allocate pstore buffer\n"); >> > + return -ENOMEM; >> > + } >> > + >> > + psinfo->owner = THIS_MODULE; >> > + psinfo->name = "virtio"; >> > + psinfo->open = virt_pstore_open; >> > + psinfo->close = virt_pstore_close; >> > + psinfo->read = virt_pstore_read; >> > + psinfo->erase = virt_pstore_erase; >> > + psinfo->write = virt_pstore_write; >> > + psinfo->flags = PSTORE_FLAGS_FRAGILE; >> >> For console support, this flag would need to be dropped -- though I >> suspect you know that already.:) > > Yep, I intentionally support DMESG type only in this patchset for > simplicity. Others could be added later. :)Cool by me. :)> > >> >> > + psinfo->data = vps; >> > + spin_lock_init(&psinfo->buf_lock); >> > + >> > + err = pstore_register(psinfo); >> > + if (err) >> > + kfree(psinfo->buf); >> > + >> > + return err; >> > +} > > [SNIP] >> >> Awesome! Can't wait to use it. :) > > Thanks for your review! :)You're welcome! :) -Kees -- Kees Cook Chrome OS & Brillo Security
Namhyung Kim
2016-Jul-19 13:43 UTC
[PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Hi Kees, On Mon, Jul 18, 2016 at 10:50:06AM -0700, Kees Cook wrote:> On Sun, Jul 17, 2016 at 10:50 PM, Namhyung Kim <namhyung at kernel.org> wrote: > > Hello, > > > > On Sun, Jul 17, 2016 at 10:12:26PM -0700, Kees Cook wrote: > >> On Sun, Jul 17, 2016 at 9:37 PM, Namhyung Kim <namhyung at kernel.org> wrote: > > [SNIP] > >> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type) > >> > +{ > >> > + u16 ret; > >> > + > >> > + switch (type) { > >> > + case PSTORE_TYPE_DMESG: > >> > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_DMESG); > >> > + break; > >> > + default: > >> > + ret = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN); > >> > + break; > >> > + } > >> > >> I would love to see this support PSTORE_TYPE_CONSOLE too. It should be > >> relatively easy to add: I think it'd just be another virtio command? > > > > Do you want to append the data to the host file as guest does > > printk()? I think it needs some kind of buffer management, but it's > > not hard to add IMHO. > > Well, with most pstore backends, the buffer size is limited, so it > tends to be a circular buffer of some sort. I think whatever you > choose to do is fine (I saw the various mentions of resource limits in > the qemu part of this thread), as long as the last N bytes of console > can be seen on the host side, where N is some portion of the memory > set aside for the log. (I don't mind the idea of an unlimited console > log either, but I suspect that will not be accepted on the qemu > side...)I think it needs two kinds of buffer management. The first one is the psinfo->buf (or something similar). IIUC the PSTORE_TYPE_CONSOLE is different than PSTORE_TYPE_DMESG as it is emitted every time printk() sends messages to console. So I think the it should remain in async mode due to performance reason. To do that, the message should be copied to psinfo->buf and then sent via virtio. Then it needs to keep track of the available buffer position IMHO. The other one is the file management on the host side. I am thinking of a simple way that the log file is splitted when it exceeds the half of the allowed max size. It would be configurable and might allow unlimited logs if user requests it explicitly (if qemu guys say ok).. Maybe we need to use 'part' or 'count' for filenames to identify the splitted files.> > > [SNIP] > >> > +static int notrace virt_pstore_write(enum pstore_type_id type, > >> > + enum kmsg_dump_reason reason, > >> > + u64 *id, unsigned int part, int count, > >> > + bool compressed, size_t size, > >> > + struct pstore_info *psi) > >> > +{ > >> > + struct virtio_pstore *vps = psi->data; > >> > + struct virtio_pstore_hdr *hdr = &vps->hdr; > >> > + struct scatterlist sg[2]; > >> > + unsigned int flags = compressed ? VIRTIO_PSTORE_FL_COMPRESSED : 0; > >> > + > >> > + *id = vps->id++; > >> > + > >> > + hdr->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_WRITE); > >> > + hdr->id = cpu_to_virtio64(vps->vdev, *id); > >> > + hdr->flags = cpu_to_virtio32(vps->vdev, flags); > >> > + hdr->type = to_virtio_type(vps, type); > >> > + > >> > + sg_init_table(sg, 2); > >> > + sg_set_buf(&sg[0], hdr, sizeof(*hdr)); > >> > + sg_set_buf(&sg[1], psi->buf, size); > >> > + virtqueue_add_outbuf(vps->vq, sg, 2, vps, GFP_ATOMIC); > >> > + virtqueue_kick(vps->vq); > >> > + > >> > + /* TODO: make it synchronous */ > >> > + return 0; > >> > >> The down side to this being asynchronous is the lack of error > >> reporting. Perhaps this could check hdr->type before queuing and error > >> for any VIRTIO_PSTORE_TYPE_UNKNOWN message instead of trying to send > >> it? > > > > I cannot follow, sorry. Could you please elaborate it more? > > The mention you have here of "TODO: make it synchronous" made me think > about what effects that could have. If a pstore_write() were issued > for a type other than DMESG, the above code would send it through > virtio anyway. No error reporting is possible unless this is > synchronous, but the only error here would simply be "I don't know > what anything except DMESG is", so maybe this code could refuse to > forward anything with type UNKNOWN in the first place. (Just an idea: > I don't think there's anything very wrong here. It just seemed like a > potential improvement.)Yep, that kind of error handling should be easy. My concern is when write operation is failed on the host side. We need a way to report it back to the guest and might disallow further writes at least for the same type. Thanks, Namhyung
Seemingly Similar Threads
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
- [PATCH 1/3] virtio: Basic implementation of virtio pstore driver