Re: [Qemu-devel] [RFC v3 2/2] virtio-pmem: Add virtio pmem driver
Hi Stefan,
> > + /* The request submission function */
> > +static int virtio_pmem_flush(struct device *dev)
> > +{
> > + int err;
> > + unsigned long flags;
> > + struct scatterlist *sgs[2], sg, ret;
> > + struct virtio_device *vdev = dev_to_virtio(dev->parent->parent);
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct virtio_pmem_request *req = kmalloc(sizeof(*req), GFP_KERNEL);
> > +
> > + req->done = false;
> > + init_waitqueue_head(>acked);
> > + spin_lock_irqsave(>pmem_lock, flags);
> > +
> > + sg_init_one(, req, sizeof(req));
>
> What are you trying to do here?
>
> sizeof(req) == sizeof(struct virtio_pmem_request *) == sizeof(void *)
>
> Did you mean sizeof(*req)?
yes, I meant: sizeof(struct virtio_pmem_request)
Thanks for catching this.
>
> But why map struct virtio_pmem_request to the device? struct
> virtio_pmem_request is the driver-internal request state and is not part
> of the hardware interface.
o.k. I will separate out request from 'virtio_pmem_request' struct
and use that.
>
> > + sgs[0] =
> > + sg_init_one(, >ret, sizeof(req->ret));
> > + sgs[1] =
> > + err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC);
> > + if (err) {
> > + dev_err(>dev, "failed to send command to virtio pmem
> > device\n");
>
> This can happen if the virtqueue is full. Printing a message and
> failing the flush isn't appropriate. This thread needs to wait until
> virtqueue space becomes available.
o.k. I will implement this.
>
> > + spin_unlock_irqrestore(>pmem_lock, flags);
> > + return -ENOSPC;
>
> req is leaked.
will free req.
>
> > + virtio_device_ready(vdev);
>
> This call isn't needed. Driver use it when they wish to submit buffers
> on virtqueues before ->probe() returns.
o.k. I will remove it.
>
> > diff --git a/include/linux/virtio_pmem.h b/include/linux/virtio_pmem.h
> > new file mode 100644
> > index 000..0f83d9c
> > --- /dev/null
> > +++ b/include/linux/virtio_pmem.h
>
> include/ is for declarations (e.g. kernel APIs) needed by other
> compilation units. The contents of this header are internal to the
> virtio_pmem driver implementation and can therefore be in virtio_pmem.c.
> include/linux/virtio_pmem.h isn't necessary since nothing besides
> virtio_pmem.c will need to include it.
Agree. Will move declarations from virtio_pmem.h to virtio_pmem.c
Thanks,
Pankaj
Re: [Qemu-devel] [RFC v3 2/2] virtio-pmem: Add virtio pmem driver
On Fri, Jul 13, 2018 at 01:22:31PM +0530, Pankaj Gupta wrote:
> + /* The request submission function */
> +static int virtio_pmem_flush(struct device *dev)
> +{
> + int err;
> + unsigned long flags;
> + struct scatterlist *sgs[2], sg, ret;
> + struct virtio_device *vdev = dev_to_virtio(dev->parent->parent);
> + struct virtio_pmem *vpmem = vdev->priv;
> + struct virtio_pmem_request *req = kmalloc(sizeof(*req), GFP_KERNEL);
> +
> + req->done = false;
> + init_waitqueue_head(>acked);
> + spin_lock_irqsave(>pmem_lock, flags);
> +
> + sg_init_one(, req, sizeof(req));
What are you trying to do here?
sizeof(req) == sizeof(struct virtio_pmem_request *) == sizeof(void *)
Did you mean sizeof(*req)?
But why map struct virtio_pmem_request to the device? struct
virtio_pmem_request is the driver-internal request state and is not part
of the hardware interface.
> + sgs[0] =
> + sg_init_one(, >ret, sizeof(req->ret));
> + sgs[1] =
> + err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC);
> + if (err) {
> + dev_err(>dev, "failed to send command to virtio pmem
> device\n");
This can happen if the virtqueue is full. Printing a message and
failing the flush isn't appropriate. This thread needs to wait until
virtqueue space becomes available.
> + spin_unlock_irqrestore(>pmem_lock, flags);
> + return -ENOSPC;
req is leaked.
> + virtio_device_ready(vdev);
This call isn't needed. Driver use it when they wish to submit buffers
on virtqueues before ->probe() returns.
> diff --git a/include/linux/virtio_pmem.h b/include/linux/virtio_pmem.h
> new file mode 100644
> index 000..0f83d9c
> --- /dev/null
> +++ b/include/linux/virtio_pmem.h
include/ is for declarations (e.g. kernel APIs) needed by other
compilation units. The contents of this header are internal to the
virtio_pmem driver implementation and can therefore be in virtio_pmem.c.
include/linux/virtio_pmem.h isn't necessary since nothing besides
virtio_pmem.c will need to include it.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC v3 2/2] virtio-pmem: Add virtio pmem driver
>
> > >
> > > > This patch adds virtio-pmem driver for KVM guest.
> > > >
> > > > Guest reads the persistent memory range information from Qemu over
> > > > VIRTIO and registers it on nvdimm_bus. It also creates a nd_region
> > > > object with the persistent memory range information so that existing
> > > > 'nvdimm/pmem' driver can reserve this into system memory map. This way
> > > > 'virtio-pmem' driver uses existing functionality of pmem driver to
> > > > register persistent memory compatible for DAX capable filesystems.
> > > >
> > > > This also provides function to perform guest flush over VIRTIO from
> > > > 'pmem' driver when userspace performs flush on DAX memory range.
> > > >
> > > > Signed-off-by: Pankaj Gupta
> > > > ---
> > > > drivers/virtio/Kconfig | 9 ++
> > > > drivers/virtio/Makefile | 1 +
> > > > drivers/virtio/virtio_pmem.c | 190
> > > > +++
> > > > include/linux/virtio_pmem.h | 44 +
> > > > include/uapi/linux/virtio_ids.h | 1 +
> > > > include/uapi/linux/virtio_pmem.h | 40 +
> > > > 6 files changed, 285 insertions(+)
> > > > create mode 100644 drivers/virtio/virtio_pmem.c
> > > > create mode 100644 include/linux/virtio_pmem.h
> > > > create mode 100644 include/uapi/linux/virtio_pmem.h
> > > >
> > > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > > index 3589764..a331e23 100644
> > > > --- a/drivers/virtio/Kconfig
> > > > +++ b/drivers/virtio/Kconfig
> > > > @@ -42,6 +42,15 @@ config VIRTIO_PCI_LEGACY
> > > >
> > > >If unsure, say Y.
> > > >
> > > > +config VIRTIO_PMEM
> > > > +tristate "Support for virtio pmem driver"
> > > > +depends on VIRTIO
> > > > +help
> > > > +This driver provides support for virtio based flushing
> > > > interface
> > > > +for persistent memory range.
> > > > +
> > > > +If unsure, say M.
> > > > +
> > > > config VIRTIO_BALLOON
> > > > tristate "Virtio balloon driver"
> > > > depends on VIRTIO
> > > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > > index 3a2b5c5..cbe91c6 100644
> > > > --- a/drivers/virtio/Makefile
> > > > +++ b/drivers/virtio/Makefile
> > > > @@ -6,3 +6,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_PMEM) += virtio_pmem.o
> > > > diff --git a/drivers/virtio/virtio_pmem.c
> > > > b/drivers/virtio/virtio_pmem.c
> > > > new file mode 100644
> > > > index 000..6200b5e
> > > > --- /dev/null
> > > > +++ b/drivers/virtio/virtio_pmem.c
> > > > @@ -0,0 +1,190 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * virtio_pmem.c: Virtio pmem Driver
> > > > + *
> > > > + * Discovers persistent memory range information
> > > > + * from host and provides a virtio based flushing
> > > > + * interface.
> > > > + */
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +
> > > > +static struct virtio_device_id id_table[] = {
> > > > +{ VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > > > +{ 0 },
> > > > +};
> > > > +
> > > > + /* The interrupt handler */
> > > > +static void host_ack(struct virtqueue *vq)
> > > > +{
> > > > +unsigned int len;
> > > > +unsigned long flags;
> > > > +struct virtio_pmem_request *req;
> > > > +struct virtio_pmem *vpmem = vq->vdev->priv;
> > > > +
> > > > +spin_lock_irqsave(>pmem_lock, flags);
> > > > +while ((req = virtqueue_get_buf(vq, )) != NULL) {
> > > > +req->done = true;
> > > > +wake_up(>acked);
> > > > +}
> > > > +spin_unlock_irqrestore(>pmem_lock, flags);
> > >
> > > Honest question: why do you need to disable interrupts here?
> >
> > To avoid interrupt for VQ trying to take same spinlock already taken by
> > process
> > context and resulting in deadlock. Looks like interrupts are already
> > disabled in
> > function call, see [1]. But still to protect with any future work.
> >
> > [1]
> >vp_interrupt
> >vp_vring_interrupt
> >vring_interrupt
>
> I think you're right, and I think I may have caused some confusion. See
> below.
>
> > >
> > > > +}
> > > > + /* Initialize virt queue */
> > > > +static int init_vq(struct virtio_pmem *vpmem)
> > > > +{
> > > > +struct virtqueue *vq;
> > > > +
> > > > +/* single vq */
> > > > +vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> > > > +host_ack, "flush_queue");
> > > > +if (IS_ERR(vq))
> > > > +return PTR_ERR(vq);
> > > > +spin_lock_init(>pmem_lock);
> > > > +
> > > > +return 0;
> > > > +};
> > > > +
> > > > + /* The request submission function */
> > > >
Re: [Qemu-devel] [RFC v3 2/2] virtio-pmem: Add virtio pmem driver
On Mon, 16 Jul 2018 07:46:30 -0400 (EDT)
Pankaj Gupta wrote:
> >
> > > This patch adds virtio-pmem driver for KVM guest.
> > >
> > > Guest reads the persistent memory range information from Qemu over
> > > VIRTIO and registers it on nvdimm_bus. It also creates a nd_region
> > > object with the persistent memory range information so that existing
> > > 'nvdimm/pmem' driver can reserve this into system memory map. This way
> > > 'virtio-pmem' driver uses existing functionality of pmem driver to
> > > register persistent memory compatible for DAX capable filesystems.
> > >
> > > This also provides function to perform guest flush over VIRTIO from
> > > 'pmem' driver when userspace performs flush on DAX memory range.
> > >
> > > Signed-off-by: Pankaj Gupta
> > > ---
> > > drivers/virtio/Kconfig | 9 ++
> > > drivers/virtio/Makefile | 1 +
> > > drivers/virtio/virtio_pmem.c | 190
> > > +++
> > > include/linux/virtio_pmem.h | 44 +
> > > include/uapi/linux/virtio_ids.h | 1 +
> > > include/uapi/linux/virtio_pmem.h | 40 +
> > > 6 files changed, 285 insertions(+)
> > > create mode 100644 drivers/virtio/virtio_pmem.c
> > > create mode 100644 include/linux/virtio_pmem.h
> > > create mode 100644 include/uapi/linux/virtio_pmem.h
> > >
> > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > > index 3589764..a331e23 100644
> > > --- a/drivers/virtio/Kconfig
> > > +++ b/drivers/virtio/Kconfig
> > > @@ -42,6 +42,15 @@ config VIRTIO_PCI_LEGACY
> > >
> > > If unsure, say Y.
> > >
> > > +config VIRTIO_PMEM
> > > + tristate "Support for virtio pmem driver"
> > > + depends on VIRTIO
> > > + help
> > > + This driver provides support for virtio based flushing interface
> > > + for persistent memory range.
> > > +
> > > + If unsure, say M.
> > > +
> > > config VIRTIO_BALLOON
> > > tristate "Virtio balloon driver"
> > > depends on VIRTIO
> > > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > > index 3a2b5c5..cbe91c6 100644
> > > --- a/drivers/virtio/Makefile
> > > +++ b/drivers/virtio/Makefile
> > > @@ -6,3 +6,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_PMEM) += virtio_pmem.o
> > > diff --git a/drivers/virtio/virtio_pmem.c b/drivers/virtio/virtio_pmem.c
> > > new file mode 100644
> > > index 000..6200b5e
> > > --- /dev/null
> > > +++ b/drivers/virtio/virtio_pmem.c
> > > @@ -0,0 +1,190 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * virtio_pmem.c: Virtio pmem Driver
> > > + *
> > > + * Discovers persistent memory range information
> > > + * from host and provides a virtio based flushing
> > > + * interface.
> > > + */
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +static struct virtio_device_id id_table[] = {
> > > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > > + { 0 },
> > > +};
> > > +
> > > + /* The interrupt handler */
> > > +static void host_ack(struct virtqueue *vq)
> > > +{
> > > + unsigned int len;
> > > + unsigned long flags;
> > > + struct virtio_pmem_request *req;
> > > + struct virtio_pmem *vpmem = vq->vdev->priv;
> > > +
> > > + spin_lock_irqsave(>pmem_lock, flags);
> > > + while ((req = virtqueue_get_buf(vq, )) != NULL) {
> > > + req->done = true;
> > > + wake_up(>acked);
> > > + }
> > > + spin_unlock_irqrestore(>pmem_lock, flags);
> >
> > Honest question: why do you need to disable interrupts here?
>
> To avoid interrupt for VQ trying to take same spinlock already taken by
> process
> context and resulting in deadlock. Looks like interrupts are already disabled
> in
> function call, see [1]. But still to protect with any future work.
>
> [1]
>vp_interrupt
>vp_vring_interrupt
>vring_interrupt
I think you're right, and I think I may have caused some confusion. See
below.
> >
> > > +}
> > > + /* Initialize virt queue */
> > > +static int init_vq(struct virtio_pmem *vpmem)
> > > +{
> > > + struct virtqueue *vq;
> > > +
> > > + /* single vq */
> > > + vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> > > + host_ack, "flush_queue");
> > > + if (IS_ERR(vq))
> > > + return PTR_ERR(vq);
> > > + spin_lock_init(>pmem_lock);
> > > +
> > > + return 0;
> > > +};
> > > +
> > > + /* The request submission function */
> > > +static int virtio_pmem_flush(struct device *dev)
> > > +{
> > > + int err;
> > > + unsigned long flags;
> > > + struct scatterlist *sgs[2], sg, ret;
> > > + struct virtio_device *vdev = dev_to_virtio(dev->parent->parent);
> > > + struct virtio_pmem *vpmem = vdev->priv;
> > > + struct virtio_pmem_request *req = kmalloc(sizeof(*req), GFP_KERNEL);
> >
> > Not checking kmalloc() return.
>
> Will add it.
> >
Re: [Qemu-devel] [RFC v3 2/2] virtio-pmem: Add virtio pmem driver
>
> > This patch adds virtio-pmem driver for KVM guest.
> >
> > Guest reads the persistent memory range information from Qemu over
> > VIRTIO and registers it on nvdimm_bus. It also creates a nd_region
> > object with the persistent memory range information so that existing
> > 'nvdimm/pmem' driver can reserve this into system memory map. This way
> > 'virtio-pmem' driver uses existing functionality of pmem driver to
> > register persistent memory compatible for DAX capable filesystems.
> >
> > This also provides function to perform guest flush over VIRTIO from
> > 'pmem' driver when userspace performs flush on DAX memory range.
> >
> > Signed-off-by: Pankaj Gupta
> > ---
> > drivers/virtio/Kconfig | 9 ++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/virtio_pmem.c | 190
> > +++
> > include/linux/virtio_pmem.h | 44 +
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 40 +
> > 6 files changed, 285 insertions(+)
> > create mode 100644 drivers/virtio/virtio_pmem.c
> > create mode 100644 include/linux/virtio_pmem.h
> > create mode 100644 include/uapi/linux/virtio_pmem.h
> >
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 3589764..a331e23 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -42,6 +42,15 @@ config VIRTIO_PCI_LEGACY
> >
> > If unsure, say Y.
> >
> > +config VIRTIO_PMEM
> > + tristate "Support for virtio pmem driver"
> > + depends on VIRTIO
> > + help
> > + This driver provides support for virtio based flushing interface
> > + for persistent memory range.
> > +
> > + If unsure, say M.
> > +
> > config VIRTIO_BALLOON
> > tristate "Virtio balloon driver"
> > depends on VIRTIO
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 3a2b5c5..cbe91c6 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -6,3 +6,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_PMEM) += virtio_pmem.o
> > diff --git a/drivers/virtio/virtio_pmem.c b/drivers/virtio/virtio_pmem.c
> > new file mode 100644
> > index 000..6200b5e
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_pmem.c
> > @@ -0,0 +1,190 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and provides a virtio based flushing
> > + * interface.
> > + */
> > +#include
> > +#include
> > +#include
> > +
> > +static struct virtio_device_id id_table[] = {
> > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > + { 0 },
> > +};
> > +
> > + /* The interrupt handler */
> > +static void host_ack(struct virtqueue *vq)
> > +{
> > + unsigned int len;
> > + unsigned long flags;
> > + struct virtio_pmem_request *req;
> > + struct virtio_pmem *vpmem = vq->vdev->priv;
> > +
> > + spin_lock_irqsave(>pmem_lock, flags);
> > + while ((req = virtqueue_get_buf(vq, )) != NULL) {
> > + req->done = true;
> > + wake_up(>acked);
> > + }
> > + spin_unlock_irqrestore(>pmem_lock, flags);
>
> Honest question: why do you need to disable interrupts here?
To avoid interrupt for VQ trying to take same spinlock already taken by process
context and resulting in deadlock. Looks like interrupts are already disabled
in
function call, see [1]. But still to protect with any future work.
[1]
vp_interrupt
vp_vring_interrupt
vring_interrupt
>
> > +}
> > + /* Initialize virt queue */
> > +static int init_vq(struct virtio_pmem *vpmem)
> > +{
> > + struct virtqueue *vq;
> > +
> > + /* single vq */
> > + vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> > + host_ack, "flush_queue");
> > + if (IS_ERR(vq))
> > + return PTR_ERR(vq);
> > + spin_lock_init(>pmem_lock);
> > +
> > + return 0;
> > +};
> > +
> > + /* The request submission function */
> > +static int virtio_pmem_flush(struct device *dev)
> > +{
> > + int err;
> > + unsigned long flags;
> > + struct scatterlist *sgs[2], sg, ret;
> > + struct virtio_device *vdev = dev_to_virtio(dev->parent->parent);
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct virtio_pmem_request *req = kmalloc(sizeof(*req), GFP_KERNEL);
>
> Not checking kmalloc() return.
Will add it.
>
> > +
> > + req->done = false;
> > + init_waitqueue_head(>acked);
> > + spin_lock_irqsave(>pmem_lock, flags);
>
> Why do you need spin_lock_irqsave()? There are two points consider:
>
> 1. Will virtio_pmem_flush() ever be called with interrupts disabled?
>If yes, then it's broken since you should be using GFP_ATOMIC in the
>kmalloc() call
Re: [Qemu-devel] [RFC v3 2/2] virtio-pmem: Add virtio pmem driver
On Fri, 13 Jul 2018 13:22:31 +0530
Pankaj Gupta wrote:
> This patch adds virtio-pmem driver for KVM guest.
>
> Guest reads the persistent memory range information from Qemu over
> VIRTIO and registers it on nvdimm_bus. It also creates a nd_region
> object with the persistent memory range information so that existing
> 'nvdimm/pmem' driver can reserve this into system memory map. This way
> 'virtio-pmem' driver uses existing functionality of pmem driver to
> register persistent memory compatible for DAX capable filesystems.
>
> This also provides function to perform guest flush over VIRTIO from
> 'pmem' driver when userspace performs flush on DAX memory range.
>
> Signed-off-by: Pankaj Gupta
> ---
> drivers/virtio/Kconfig | 9 ++
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio_pmem.c | 190
> +++
> include/linux/virtio_pmem.h | 44 +
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_pmem.h | 40 +
> 6 files changed, 285 insertions(+)
> create mode 100644 drivers/virtio/virtio_pmem.c
> create mode 100644 include/linux/virtio_pmem.h
> create mode 100644 include/uapi/linux/virtio_pmem.h
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 3589764..a331e23 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -42,6 +42,15 @@ config VIRTIO_PCI_LEGACY
>
> If unsure, say Y.
>
> +config VIRTIO_PMEM
> + tristate "Support for virtio pmem driver"
> + depends on VIRTIO
> + help
> + This driver provides support for virtio based flushing interface
> + for persistent memory range.
> +
> + If unsure, say M.
> +
> config VIRTIO_BALLOON
> tristate "Virtio balloon driver"
> depends on VIRTIO
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 3a2b5c5..cbe91c6 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -6,3 +6,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_PMEM) += virtio_pmem.o
> diff --git a/drivers/virtio/virtio_pmem.c b/drivers/virtio/virtio_pmem.c
> new file mode 100644
> index 000..6200b5e
> --- /dev/null
> +++ b/drivers/virtio/virtio_pmem.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * virtio_pmem.c: Virtio pmem Driver
> + *
> + * Discovers persistent memory range information
> + * from host and provides a virtio based flushing
> + * interface.
> + */
> +#include
> +#include
> +#include
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> + /* The interrupt handler */
> +static void host_ack(struct virtqueue *vq)
> +{
> + unsigned int len;
> + unsigned long flags;
> + struct virtio_pmem_request *req;
> + struct virtio_pmem *vpmem = vq->vdev->priv;
> +
> + spin_lock_irqsave(>pmem_lock, flags);
> + while ((req = virtqueue_get_buf(vq, )) != NULL) {
> + req->done = true;
> + wake_up(>acked);
> + }
> + spin_unlock_irqrestore(>pmem_lock, flags);
Honest question: why do you need to disable interrupts here?
> +}
> + /* Initialize virt queue */
> +static int init_vq(struct virtio_pmem *vpmem)
> +{
> + struct virtqueue *vq;
> +
> + /* single vq */
> + vpmem->req_vq = vq = virtio_find_single_vq(vpmem->vdev,
> + host_ack, "flush_queue");
> + if (IS_ERR(vq))
> + return PTR_ERR(vq);
> + spin_lock_init(>pmem_lock);
> +
> + return 0;
> +};
> +
> + /* The request submission function */
> +static int virtio_pmem_flush(struct device *dev)
> +{
> + int err;
> + unsigned long flags;
> + struct scatterlist *sgs[2], sg, ret;
> + struct virtio_device *vdev = dev_to_virtio(dev->parent->parent);
> + struct virtio_pmem *vpmem = vdev->priv;
> + struct virtio_pmem_request *req = kmalloc(sizeof(*req), GFP_KERNEL);
Not checking kmalloc() return.
> +
> + req->done = false;
> + init_waitqueue_head(>acked);
> + spin_lock_irqsave(>pmem_lock, flags);
Why do you need spin_lock_irqsave()? There are two points consider:
1. Will virtio_pmem_flush() ever be called with interrupts disabled?
If yes, then it's broken since you should be using GFP_ATOMIC in the
kmalloc() call and you can't call wait_event()
2. If virtio_pmem_flush() is never called with interrupts disabled, do
you really need to disable interrupts? If yes, why?
Another point to consider is whether or not virtio_pmem_flush()
can be called from atomic context. nvdimm_flush() itself is called
from a few atomic sites, but I can't tell if virtio_pmem_flush()
will ever be called from those sites. If it can be called atomic
context, then item 1 applies here.
