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

[Namhyung Kim]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Radim Kr??m???? <rkrcmar@xxxxxxxxxx>
> > Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> > Cc: Anthony Liguori <aliguori@xxxxxxxxxx>
> > Cc: Anton Vorontsov <anton@xxxxxxxxxx>
> > Cc: Colin Cross <ccross@xxxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Tony Luck <tony.luck@xxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Minchan Kim <minchan@xxxxxxxxxx>
> > Cc: kvm@xxxxxxxxxxxxxxx
> > Cc: qemu-devel@xxxxxxxxxx
> > Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>

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