* Re: [PATCH v5 0/6] virtio pmem driver
From: Arkadiusz Miśkiewicz @ 2019-04-10 8:08 UTC (permalink / raw)
To: Pankaj Gupta, linux-nvdimm, linux-kernel, virtualization, kvm,
linux-fsdevel, linux-acpi, qemu-devel, linux-ext4, linux-xfs
Cc: jack, mst, david, lcapitulino, adilger.kernel, zwisler, aarcange,
dave.jiang, darrick.wong, vishal.l.verma, willy, hch, jmoyer,
nilal, lenb, kilobyte, riel, yuval.shaia, stefanha, pbonzini,
dan.j.williams, tytso, xiaoguangrong.eric, cohuck, rjw, imammedo
In-Reply-To: <20190410040826.24371-1-pagupta@redhat.com>
On 10/04/2019 06:08, Pankaj Gupta wrote:
> This patch series has implementation for "virtio pmem".
> "virtio pmem" is fake persistent memory(nvdimm) in guest
> which allows to bypass the guest page cache. This also
Will kernel pstore be able to use this persistent memory for storing
crash dumps?
--
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v5 4/6] dax: check synchronous mapping is supported
From: Jan Kara @ 2019-04-10 8:25 UTC (permalink / raw)
To: Pankaj Gupta
Cc: cohuck, jack, kvm, mst, david, qemu-devel, virtualization,
adilger.kernel, zwisler, aarcange, dave.jiang, linux-nvdimm,
vishal.l.verma, willy, hch, linux-acpi, jmoyer, linux-ext4, lenb,
kilobyte, riel, yuval.shaia, stefanha, pbonzini, dan.j.williams,
lcapitulino, nilal, tytso, xiaoguangrong.eric, darrick.wong, rjw,
linux-kernel, linux-xfs, linux-fsdevel, imammedo
In-Reply-To: <20190410040826.24371-5-pagupta@redhat.com>
On Wed 10-04-19 09:38:24, Pankaj Gupta wrote:
> This patch introduces 'daxdev_mapping_supported' helper
> which checks if 'MAP_SYNC' is supported with filesystem
> mapping. It also checks if corresponding dax_device is
> synchronous. Virtio pmem device is asynchronous and
> does not not support VM_SYNC.
>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
> include/linux/dax.h | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/include/linux/dax.h b/include/linux/dax.h
> index b896706a5ee9..4a2a60ffec86 100644
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -38,6 +38,24 @@ void kill_dax(struct dax_device *dax_dev);
> void dax_write_cache(struct dax_device *dax_dev, bool wc);
> bool dax_write_cache_enabled(struct dax_device *dax_dev);
> bool dax_synchronous(struct dax_device *dax_dev);
> +
> +/*
> + * Callers check if synchronous mapping is enabled for DAX file
> + * and attached dax device is also synchronous.
> + *
> + * dax_synchronous function verifies if dax device is synchronous.
> + * Currently, only virtio pmem device supports asynchronous device
> + * flush.
> + */
Thanks for the patch! I'd restructure this comment like:
/*
* Check if given mapping is supported by the file / underlying device.
*/
> +static inline bool daxdev_mapping_supported(struct vm_area_struct *vma,
> + struct dax_device *dax_dev)
> +{
/* Everyone supports non-sync mappings */
> + if (!(vma->vm_flags & VM_SYNC))
> + return true;
/* Sync mappings are supported only for files using DAX */
> + if (!IS_DAX(file_inode(vma->vm_file)))
> + return false;
/* Underlying device must support persisting through CPU instructions */
> + return dax_synchronous(dax_dev);
> +}
> #else
> static inline struct dax_device *dax_get_by_host(const char *host)
> {
> @@ -69,6 +87,11 @@ static inline bool dax_synchronous(struct dax_device *dax_dev)
> {
> return true;
> }
> +static inline bool daxdev_mapping_supported(struct vm_area_struct *vma,
> + struct dax_device *dax_dev)
> +{
> + return true;
This looks wrong. Shouldn't it rather be:
return !(vma->flags & VM_SYNC);
?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [RFC PATCH 05/12] s390/cio: add protected virtualization support to cio
From: Cornelia Huck @ 2019-04-10 8:25 UTC (permalink / raw)
To: Halil Pasic
Cc: Vasily Gorbik, linux-s390, Eric Farman, Claudio Imbrenda, kvm,
Sebastian Ott, Farhan Ali, virtualization, Martin Schwidefsky,
Viktor Mihajlovski, Janosch Frank
In-Reply-To: <20190410021044.4da3e847@oc2783563651>
On Wed, 10 Apr 2019 02:10:44 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:
> On Tue, 9 Apr 2019 19:55:48 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
> > On Fri, 5 Apr 2019 01:16:15 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > Thus we need to make sure any memory that is used for communication with
> > > the hypervisor is shared.
> >
> > In this context, does 'hypervisor' always mean 'QEMU/KVM'? If Other
> > Hypervisors implement protected virtualization, we probably need to
> > make sure that all common I/O layer control blocks are in the dma area
> > (including e.g. QDIO), not just what virtio-ccw devices use.
> >
>
> Hypervisor could theoretically be something different than QEMU/KVM. Yet,
> as stated before, this series is about getting virtio-ccw working
> (modulo the TODOs).
Sure, just wanted to point it out. If this is "enable the common I/O
layer, except for QDIO" or so, that would sound fine to me :)
>
> [..]
>
> > >
> >
> > So, this leaves some things I'm not sure about, especially as I do not
> > know the architecture of this new feature.
> >
> > - This applies only to asynchronously handled things, it seems? So
> > things like control blocks modified by stsch/msch/etc does not need
> > special treatment?
>
> I had a feeble attempt at explaining this in the cover letter:
>
> * make sure that virtio-ccw specific stuff uses shared memory when
> talking to the hypervisor (except communication blocks like ORB, these
> are handled by the hypervisor)
>
> Unfortunately the last 'hypervisor' was supposed to be 'ultravisor'.
>
> I.e. the ultravisor will take care of exposing the control blocks
> to the hypervisor (and of changes as well).
Yeah, that "control blocks" or "communication blocks" leaves me a bit
fuzzy :)
So, what is a high-level summary of areas that need the treatment?
What I get from looking at the patches so far, it's:
- stuff that is written by the hypervisor's interrupt injection code:
IRB, indicators, etc.
- buffers that are filled by a channel program: sense, sense id, etc.
- ccws themselves (because of translation?)
^ permalink raw reply
* Re: [PATCH v5 3/6] libnvdimm: add dax_dev sync flag
From: Jan Kara @ 2019-04-10 8:28 UTC (permalink / raw)
To: Pankaj Gupta
Cc: cohuck, jack, kvm, mst, david, qemu-devel, virtualization,
adilger.kernel, zwisler, aarcange, dave.jiang, linux-nvdimm,
vishal.l.verma, willy, hch, linux-acpi, jmoyer, linux-ext4, lenb,
kilobyte, riel, yuval.shaia, stefanha, pbonzini, dan.j.williams,
lcapitulino, nilal, tytso, xiaoguangrong.eric, darrick.wong, rjw,
linux-kernel, linux-xfs, linux-fsdevel, imammedo
In-Reply-To: <20190410040826.24371-4-pagupta@redhat.com>
On Wed 10-04-19 09:38:23, Pankaj Gupta wrote:
> @@ -64,6 +65,10 @@ static inline bool dax_write_cache_enabled(struct dax_device *dax_dev)
> {
> return false;
> }
> +static inline bool dax_synchronous(struct dax_device *dax_dev)
> +{
> + return true;
> +}
Is there a need to define dax_synchronous() for !CONFIG_DAX? Because that
property of dax device is pretty much undefined and I don't see anything
needing to call it for !CONFIG_DAX...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v5 4/6] dax: check synchronous mapping is supported
From: Pankaj Gupta @ 2019-04-10 8:31 UTC (permalink / raw)
To: Jan Kara
Cc: cohuck, kvm, mst, david, qemu-devel, virtualization,
adilger kernel, zwisler, aarcange, dave jiang, linux-nvdimm,
vishal l verma, willy, hch, linux-acpi, jmoyer, linux-ext4, lenb,
kilobyte, riel, yuval shaia, stefanha, pbonzini, dan j williams,
lcapitulino, nilal, tytso, xiaoguangrong eric, darrick wong, rjw,
linux-kernel, linux-xfs, linux-fsdevel
In-Reply-To: <20190410082504.GF1426@quack2.suse.cz>
> > This patch introduces 'daxdev_mapping_supported' helper
> > which checks if 'MAP_SYNC' is supported with filesystem
> > mapping. It also checks if corresponding dax_device is
> > synchronous. Virtio pmem device is asynchronous and
> > does not not support VM_SYNC.
> >
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> > ---
> > include/linux/dax.h | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/include/linux/dax.h b/include/linux/dax.h
> > index b896706a5ee9..4a2a60ffec86 100644
> > --- a/include/linux/dax.h
> > +++ b/include/linux/dax.h
> > @@ -38,6 +38,24 @@ void kill_dax(struct dax_device *dax_dev);
> > void dax_write_cache(struct dax_device *dax_dev, bool wc);
> > bool dax_write_cache_enabled(struct dax_device *dax_dev);
> > bool dax_synchronous(struct dax_device *dax_dev);
> > +
> > +/*
> > + * Callers check if synchronous mapping is enabled for DAX file
> > + * and attached dax device is also synchronous.
> > + *
> > + * dax_synchronous function verifies if dax device is synchronous.
> > + * Currently, only virtio pmem device supports asynchronous device
> > + * flush.
> > + */
>
> Thanks for the patch! I'd restructure this comment like:
>
> /*
> * Check if given mapping is supported by the file / underlying device.
> */
Sure.
> > +static inline bool daxdev_mapping_supported(struct vm_area_struct *vma,
> > + struct dax_device *dax_dev)
> > +{
>
> /* Everyone supports non-sync mappings */
> > + if (!(vma->vm_flags & VM_SYNC))
> > + return true;
> /* Sync mappings are supported only for files using DAX */
> > + if (!IS_DAX(file_inode(vma->vm_file)))
> > + return false;
> /* Underlying device must support persisting through CPU instructions */
> > + return dax_synchronous(dax_dev);
> > +}
> > #else
> > static inline struct dax_device *dax_get_by_host(const char *host)
> > {
> > @@ -69,6 +87,11 @@ static inline bool dax_synchronous(struct dax_device
> > *dax_dev)
> > {
> > return true;
> > }
> > +static inline bool daxdev_mapping_supported(struct vm_area_struct *vma,
> > + struct dax_device *dax_dev)
> > +{
> > + return true;
>
> This looks wrong. Shouldn't it rather be:
>
> return !(vma->flags & VM_SYNC);
>
> ?
Right. I will correct this.
Thanks,
Pankaj
>
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>
^ permalink raw reply
* Re: [PATCH v5 3/6] libnvdimm: add dax_dev sync flag
From: Pankaj Gupta @ 2019-04-10 8:38 UTC (permalink / raw)
To: Jan Kara
Cc: cohuck, kvm, mst, david, qemu-devel, virtualization,
adilger kernel, zwisler, aarcange, dave jiang, linux-nvdimm,
vishal l verma, willy, hch, linux-acpi, jmoyer, linux-ext4, lenb,
kilobyte, riel, yuval shaia, stefanha, pbonzini, dan j williams,
lcapitulino, nilal, tytso, xiaoguangrong eric, darrick wong, rjw,
linux-kernel, linux-xfs, linux-fsdevel
In-Reply-To: <20190410082804.GG1426@quack2.suse.cz>
>
> On Wed 10-04-19 09:38:23, Pankaj Gupta wrote:
> > @@ -64,6 +65,10 @@ static inline bool dax_write_cache_enabled(struct
> > dax_device *dax_dev)
> > {
> > return false;
> > }
> > +static inline bool dax_synchronous(struct dax_device *dax_dev)
> > +{
> > + return true;
> > +}
>
> Is there a need to define dax_synchronous() for !CONFIG_DAX? Because that
> property of dax device is pretty much undefined and I don't see anything
> needing to call it for !CONFIG_DAX...
You are right. Will remove this.
Thanks for the review.
Best regards,
Pankaj
>
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>
^ permalink raw reply
* Re: [RFC PATCH 07/12] virtio/s390: use DMA memory for ccw I/O
From: Cornelia Huck @ 2019-04-10 8:42 UTC (permalink / raw)
To: Halil Pasic
Cc: Vasily Gorbik, linux-s390, Eric Farman, Claudio Imbrenda, kvm,
Sebastian Ott, Farhan Ali, virtualization, Martin Schwidefsky,
Viktor Mihajlovski, Janosch Frank
In-Reply-To: <20190404231622.52531-8-pasic@linux.ibm.com>
On Fri, 5 Apr 2019 01:16:17 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:
> Before virtio-ccw could get away with not using DMA API for the pieces of
> memory it does ccw I/O with. With protected virtualization this has to
> change, since the hypervisor needs to read and sometimes also write these
> pieces of memory.
>
> Let us make sure all ccw I/O is done through shared memory.
>
> Note: The control blocks of I/O instructions do not need to be shared.
> These are marshalled by the ultravisor.
Ok, so direct parameters of I/O instructions are handled by the
ultravisor?
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
> drivers/s390/virtio/virtio_ccw.c | 177 +++++++++++++++++++++++----------------
> 1 file changed, 107 insertions(+), 70 deletions(-)
>
(...)
> @@ -167,6 +170,28 @@ static struct virtio_ccw_device *to_vc_device(struct virtio_device *vdev)
> return container_of(vdev, struct virtio_ccw_device, vdev);
> }
>
> +#define vc_dma_decl_struct(type, field) \
> + dma_addr_t field ## _dma_addr; \
> + struct type *field
> +
> +static inline void *__vc_dma_alloc(struct virtio_device *vdev, size_t size,
> + dma_addr_t *dma_handle)
> +{
> + return dma_alloc_coherent(vdev->dev.parent, size, dma_handle,
> + GFP_DMA | GFP_KERNEL | __GFP_ZERO);
> +}
> +
> +static inline void __vc_dma_free(struct virtio_device *vdev, size_t size,
> + void *cpu_addr, dma_addr_t dma_handle)
> +{
> + dma_free_coherent(vdev->dev.parent, size, cpu_addr, dma_handle);
> +}
> +
> +#define vc_dma_alloc_struct(vdev, ptr) \
> + ({ ptr = __vc_dma_alloc(vdev, (sizeof(*(ptr))), &(ptr ## _dma_addr)); })
> +#define vc_dma_free_struct(vdev, ptr) \
> + __vc_dma_free(vdev, sizeof(*(ptr)), (ptr), (ptr ## _dma_addr))
Not sure I'm a fan of those wrappers... I think they actually hurt
readability of the code.
> +
> static void drop_airq_indicator(struct virtqueue *vq, struct airq_info *info)
> {
> unsigned long i, flags;
> @@ -322,12 +347,12 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> {
> int ret;
> unsigned long *indicatorp = NULL;
> - struct virtio_thinint_area *thinint_area = NULL;
> + vc_dma_decl_struct(virtio_thinint_area, thinint_area) = NULL;
> + dma_addr_t indicatorp_dma_addr;
> struct airq_info *airq_info = vcdev->airq_info;
>
> if (vcdev->is_thinint) {
> - thinint_area = kzalloc(sizeof(*thinint_area),
> - GFP_DMA | GFP_KERNEL);
> + vc_dma_alloc_struct(&vcdev->vdev, thinint_area);
> if (!thinint_area)
> return;
> thinint_area->summary_indicator =
> @@ -338,8 +363,9 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> ccw->cda = (__u32)(unsigned long) thinint_area;
> } else {
> /* payload is the address of the indicators */
> - indicatorp = kmalloc(sizeof(&vcdev->indicators),
> - GFP_DMA | GFP_KERNEL);
> + indicatorp = __vc_dma_alloc(&vcdev->vdev,
> + sizeof(&vcdev->indicators),
> + &indicatorp_dma_addr);
> if (!indicatorp)
> return;
> *indicatorp = 0;
> @@ -359,8 +385,10 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> "Failed to deregister indicators (%d)\n", ret);
> else if (vcdev->is_thinint)
> virtio_ccw_drop_indicators(vcdev);
> - kfree(indicatorp);
> - kfree(thinint_area);
> + if (indicatorp)
> + __vc_dma_free(&vcdev->vdev, sizeof(&vcdev->indicators),
> + indicatorp, indicatorp_dma_addr);
> + vc_dma_free_struct(&vcdev->vdev, thinint_area);
Don't you need to check for !NULL here as well?
> }
>
> static inline long __do_kvm_notify(struct subchannel_id schid,
(...)
> @@ -1280,7 +1318,6 @@ static int virtio_ccw_online(struct ccw_device *cdev)
>
> vcdev->is_thinint = virtio_ccw_use_airq; /* at least try */
>
> - vcdev->vdev.dev.parent = &cdev->dev;
Hm?
(You added a line like that in a previous patch; should it simply have
been a movement instead? Or am I misremembering?)
> vcdev->vdev.dev.release = virtio_ccw_release_dev;
> vcdev->vdev.config = &virtio_ccw_config_ops;
> vcdev->cdev = cdev;
^ permalink raw reply
* Re: [RFC PATCH 10/12] virtio/s390: consolidate DMA allocations
From: Cornelia Huck @ 2019-04-10 8:46 UTC (permalink / raw)
To: Halil Pasic
Cc: Vasily Gorbik, linux-s390, Eric Farman, Claudio Imbrenda, kvm,
Sebastian Ott, Farhan Ali, virtualization, Martin Schwidefsky,
Viktor Mihajlovski, Janosch Frank
In-Reply-To: <20190404231622.52531-11-pasic@linux.ibm.com>
On Fri, 5 Apr 2019 01:16:20 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:
> We can reduce the number of DMA allocations by pulling the bits
> of memory that belongs to virtio_ccw_device and needs to be DMA
> memory into a single area.
That makes a lot of sense (maybe start with introducing the dma area
from the beginning?), but...
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
> drivers/s390/virtio/virtio_ccw.c | 48 ++++++++++++++--------------------------
> 1 file changed, 16 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index aa45a6a027ae..7268149f2ee8 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -49,12 +49,12 @@ struct vq_config_block {
> struct vcdev_dma_area {
> unsigned long indicators;
> unsigned long indicators2;
> + struct vq_config_block config_block;
> + __u8 status; /* TODO check __aligned(8); */
...I think that needs attention.
> };
>
> struct virtio_ccw_device {
> struct virtio_device vdev;
> - __u8 *status;
> - dma_addr_t status_dma_addr;
> __u8 config[VIRTIO_CCW_CONFIG_SIZE];
> struct ccw_device *cdev;
> __u32 curr_io;
^ permalink raw reply
* Re: [RFC PATCH 00/12] s390: virtio: support protected virtualization
From: Cornelia Huck @ 2019-04-10 9:20 UTC (permalink / raw)
To: Halil Pasic
Cc: Vasily Gorbik, linux-s390, Eric Farman, Claudio Imbrenda, kvm,
Sebastian Ott, Farhan Ali, virtualization, Martin Schwidefsky,
Viktor Mihajlovski, Janosch Frank
In-Reply-To: <20190404231622.52531-1-pasic@linux.ibm.com>
On Fri, 5 Apr 2019 01:16:10 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:
> Enhanced virtualization protection technology may require the use of
> bounce buffers for I/O. While support for this was built into the virtio
> core, virtio-ccw wasn't changed accordingly.
>
> Thus what needs to be done to bring virtio-ccw up to speed with respect
> to this is:
> * use some 'new' common virtio stuff
> * make sure that virtio-ccw specific stuff uses shared memory when
> talking to the hypervisor (except communication blocks like ORB, these
> are handled by the hypervisor)
> * make sure the DMA API does what is necessary to talk through shared
> memory if we are a protected virtualization guest.
> * make sure the common IO layer plays along as well (airqs, sense).
It would be good to have a summary somewhere in the code (or
Documentation/) as to what needs the dma treatment and what doesn't,
for later reference. We don't want people to accidentally break things
(especially if they cannot refer to architecture documentation - or
will at least some of that be published?)
>
> The series is structured in incremental fashion: some of the changes are
> overridden by following patches. The main reason why is that this is how I
> developed. But I think it ain't bad for the didactic and we are a bit more
> flexible with regards to throwing out some of the stuff in the end.
FWIW, I think reshuffling the patches in the next iteration would ease
review.
>
> Important notes:
>
> * This is an early (WIP) RFC that does not add any function to the
> kernel at his stage, as the ultravisor interactions are left out.
> The purpose is getting some early feedback ASAP.
I would like some comments from people who have experience with the dma
api.
>
> * In future these patches will depend on some code interacting with the
> ultravisor (WIP by Vasily and Janosch).
>
> * The s390 names are by no means final, and are not properly explained. Should
> not hamper understanding too much. If it does please ask.
>
> * The existing naming in the common infrastructure (kernel internal
> interfaces) is pretty much based on the AMD SEV terminology. Thus the
> names aren't always perfect. There might be merit to changing these
> names to more abstract ones. I did not put much thought into that at
> the current stage.
If we can find some generic names that work well for everyone,
converting seems like a good idea. But following SEV is not that bad,
either (you'll probably find more people who have heard about SEV than
folks familiar with s390 ;)
>
>
> Testing:
>
> Please use iommu_platform=on for any virtio devices you are going
> to test this code with (so virtio actually uses the DMA API).
>
> Looking forward to your review or any other type of input.
I have now read through the whole series and commented in some places.
But I'd really like to see comments from others as well.
^ permalink raw reply
* Re: [PATCH v2 1/3] drm: switch drm_fb_memcpy_dstclip to accept __iomem dst
From: Noralf Trønnes @ 2019-04-10 9:42 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Maxime Ripard,
Dave Airlie, Sean Paul
In-Reply-To: <20190410063815.17062-2-kraxel@redhat.com>
Den 10.04.2019 08.38, skrev Gerd Hoffmann:
> Not all archs have the __io_virt() macro, so cirrus can't simply convert
> pointers that way. The drm format helpers have to use memcpy_toio()
> instead.
>
> This patch makes drm_fb_memcpy_dstclip() accept a __iomem dst pointer
> and use memcpy_toio() instead of memcpy(). With that separating out the
> memcpy loop into the drm_fb_memcpy_lines() helper isn't useful any more,
> so move the code back into the calling functins.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 2/3] drm: switch drm_fb_xrgb8888_to_rgb565_dstclip to accept __iomem dst
From: Noralf Trønnes @ 2019-04-10 9:44 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Maxime Ripard,
Dave Airlie, Sean Paul
In-Reply-To: <20190410063815.17062-3-kraxel@redhat.com>
Den 10.04.2019 08.38, skrev Gerd Hoffmann:
> Not all archs have the __io_virt() macro, so cirrus can't simply convert
> pointers that way. The drm format helpers have to use memcpy_toio()
> instead.
>
> This patch makes drm_fb_xrgb8888_to_rgb565_dstclip() accept a __iomem
> dst pointer and use memcpy_toio() instead of memcpy(). The helper
> function (drm_fb_xrgb8888_to_rgb565_line) has been changed to process
> a single scanline.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> include/drm/drm_format_helper.h | 2 +-
> drivers/gpu/drm/cirrus/cirrus.c | 2 +-
> drivers/gpu/drm/drm_format_helper.c | 113 ++++++++++++++--------------
> 3 files changed, 60 insertions(+), 57 deletions(-)
>
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index bc2e1004e166..d1b8a9ea01b4 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -23,7 +23,7 @@ void drm_fb_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
> void drm_fb_xrgb8888_to_rgb565(void *dst, void *vaddr,
> struct drm_framebuffer *fb,
> struct drm_rect *clip, bool swap);
> -void drm_fb_xrgb8888_to_rgb565_dstclip(void *dst, unsigned int dst_pitch,
> +void drm_fb_xrgb8888_to_rgb565_dstclip(void __iomem *dst, unsigned int dst_pitch,
> void *vaddr, struct drm_framebuffer *fb,
> struct drm_rect *clip, bool swap);
> void drm_fb_xrgb8888_to_rgb888_dstclip(void *dst, unsigned int dst_pitch,
> diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c
> index 0fc3aa31b5a4..ed2f2d8cfb6f 100644
> --- a/drivers/gpu/drm/cirrus/cirrus.c
> +++ b/drivers/gpu/drm/cirrus/cirrus.c
> @@ -311,7 +311,7 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
> vmap, fb, rect);
>
> else if (fb->format->cpp[0] == 4 && cirrus->cpp == 2)
> - drm_fb_xrgb8888_to_rgb565_dstclip(__io_virt(cirrus->vram),
> + drm_fb_xrgb8888_to_rgb565_dstclip(cirrus->vram,
> cirrus->pitch,
> vmap, fb, rect, false);
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index dace05638bc3..c9521af4e90b 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -113,42 +113,22 @@ void drm_fb_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
> }
> EXPORT_SYMBOL(drm_fb_swab16);
>
> -static void drm_fb_xrgb8888_to_rgb565_lines(void *dst, unsigned int dst_pitch,
> - void *src, unsigned int src_pitch,
> - unsigned int src_linelength,
> - unsigned int lines,
> - bool swap)
> +static void drm_fb_xrgb8888_to_rgb565_line(u16 *dbuf, u32 *sbuf,
> + unsigned int pixels,
> + bool swab)
Both here and further down you change the argument name: swap -> swab.
If you want that, you need to fix the function declaration and the docs
as well.
With that sorted out:
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> {
> - unsigned int linepixels = src_linelength / sizeof(u32);
> - unsigned int x, y;
> - u32 *sbuf;
> - u16 *dbuf, val16;
> + unsigned int x;
> + u16 val16;
>
> - /*
> - * The cma memory is write-combined so reads are uncached.
> - * Speed up by fetching one line at a time.
> - */
> - sbuf = kmalloc(src_linelength, GFP_KERNEL);
> - if (!sbuf)
> - return;
> -
> - for (y = 0; y < lines; y++) {
> - memcpy(sbuf, src, src_linelength);
> - dbuf = dst;
> - for (x = 0; x < linepixels; x++) {
> - val16 = ((sbuf[x] & 0x00F80000) >> 8) |
> - ((sbuf[x] & 0x0000FC00) >> 5) |
> - ((sbuf[x] & 0x000000F8) >> 3);
> - if (swap)
> - *dbuf++ = swab16(val16);
> - else
> - *dbuf++ = val16;
> - }
> - src += src_pitch;
> - dst += dst_pitch;
> + for (x = 0; x < pixels; x++) {
> + val16 = ((sbuf[x] & 0x00F80000) >> 8) |
> + ((sbuf[x] & 0x0000FC00) >> 5) |
> + ((sbuf[x] & 0x000000F8) >> 3);
> + if (swab)
> + dbuf[x] = swab16(val16);
> + else
> + dbuf[x] = val16;
> }
> -
> - kfree(sbuf);
> }
>
> /**
> @@ -167,23 +147,37 @@ static void drm_fb_xrgb8888_to_rgb565_lines(void *dst, unsigned int dst_pitch,
> */
> void drm_fb_xrgb8888_to_rgb565(void *dst, void *vaddr,
> struct drm_framebuffer *fb,
> - struct drm_rect *clip, bool swap)
> + struct drm_rect *clip, bool swab)
> {
> - unsigned int src_offset = (clip->y1 * fb->pitches[0])
> - + (clip->x1 * sizeof(u32));
> - size_t src_len = (clip->x2 - clip->x1) * sizeof(u32);
> - size_t dst_len = (clip->x2 - clip->x1) * sizeof(u16);
> + size_t linepixels = clip->x2 - clip->x1;
> + size_t src_len = linepixels * sizeof(u32);
> + size_t dst_len = linepixels * sizeof(u16);
> + unsigned y, lines = clip->y2 - clip->y1;
> + void *sbuf;
>
> - drm_fb_xrgb8888_to_rgb565_lines(dst, dst_len,
> - vaddr + src_offset, fb->pitches[0],
> - src_len, clip->y2 - clip->y1,
> - swap);
> + /*
> + * The cma memory is write-combined so reads are uncached.
> + * Speed up by fetching one line at a time.
> + */
> + sbuf = kmalloc(src_len, GFP_KERNEL);
> + if (!sbuf)
> + return;
> +
> + vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
> + for (y = 0; y < lines; y++) {
> + memcpy(sbuf, vaddr, src_len);
> + drm_fb_xrgb8888_to_rgb565_line(dst, sbuf, linepixels, swab);
> + vaddr += fb->pitches[0];
> + dst += dst_len;
> + }
> +
> + kfree(sbuf);
> }
> EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
>
> /**
> * drm_fb_xrgb8888_to_rgb565_dstclip - Convert XRGB8888 to RGB565 clip buffer
> - * @dst: RGB565 destination buffer
> + * @dst: RGB565 destination buffer (iomem)
> * @dst_pitch: destination buffer pitch
> * @vaddr: XRGB8888 source buffer
> * @fb: DRM framebuffer
> @@ -194,22 +188,31 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
> * support XRGB8888.
> *
> * This function applies clipping on dst, i.e. the destination is a
> - * full framebuffer but only the clip rect content is copied over.
> + * full (iomem) framebuffer but only the clip rect content is copied over.
> */
> -void drm_fb_xrgb8888_to_rgb565_dstclip(void *dst, unsigned int dst_pitch,
> +void drm_fb_xrgb8888_to_rgb565_dstclip(void __iomem *dst, unsigned int dst_pitch,
> void *vaddr, struct drm_framebuffer *fb,
> - struct drm_rect *clip, bool swap)
> + struct drm_rect *clip, bool swab)
> {
> - unsigned int src_offset = (clip->y1 * fb->pitches[0])
> - + (clip->x1 * sizeof(u32));
> - unsigned int dst_offset = (clip->y1 * dst_pitch)
> - + (clip->x1 * sizeof(u16));
> - size_t src_len = (clip->x2 - clip->x1) * sizeof(u32);
> + size_t linepixels = clip->x2 - clip->x1;
> + size_t dst_len = linepixels * sizeof(u16);
> + unsigned y, lines = clip->y2 - clip->y1;
> + void *dbuf;
>
> - drm_fb_xrgb8888_to_rgb565_lines(dst + dst_offset, dst_pitch,
> - vaddr + src_offset, fb->pitches[0],
> - src_len, clip->y2 - clip->y1,
> - swap);
> + dbuf = kmalloc(dst_len, GFP_KERNEL);
> + if (!dbuf)
> + return;
> +
> + vaddr += clip_offset(clip, fb->pitches[0], sizeof(u32));
> + dst += clip_offset(clip, dst_pitch, sizeof(u16));
> + for (y = 0; y < lines; y++) {
> + drm_fb_xrgb8888_to_rgb565_line(dbuf, vaddr, linepixels, swab);
> + memcpy_toio(dst, dbuf, dst_len);
> + vaddr += fb->pitches[0];
> + dst += dst_len;
> + }
> +
> + kfree(dbuf);
> }
> EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565_dstclip);
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 3/3] drm: switch drm_fb_xrgb8888_to_rgb888_dstclip to accept __iomem dst
From: Noralf Trønnes @ 2019-04-10 9:45 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Maxime Ripard,
Dave Airlie, Sean Paul
In-Reply-To: <20190410063815.17062-4-kraxel@redhat.com>
Den 10.04.2019 08.38, skrev Gerd Hoffmann:
> Not all archs have the __io_virt() macro, so cirrus can't simply convert
> pointers that way. The drm format helpers have to use memcpy_toio()
> instead.
>
> This patch makes drm_fb_xrgb8888_to_rgb888_dstclip() accept a __iomem
> dst pointer and use memcpy_toio() instead of memcpy(). The helper
> function (drm_fb_xrgb8888_to_rgb888_line) has been changed to process a
> single scanline.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH 1/3] virtio-gpu api: comment feature flags
From: Gerd Hoffmann @ 2019-04-10 11:42 UTC (permalink / raw)
To: dri-devel, virtio
Cc: Tomeu Vizoso, Michael S. Tsirkin, David Airlie, open list,
Gurchetan Singh, Marc-André Lureau, David Airlie,
open list:VIRTIO CORE, NET AND BLOCK DRIVERS
In-Reply-To: <20190410114227.25846-1-kraxel@redhat.com>
Add comments to the existing feature flags,
documenting which commands belong to them.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/uapi/linux/virtio_gpu.h | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 8e88eba1fa7a..0c85914d9369 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -40,8 +40,16 @@
#include <linux/types.h>
-#define VIRTIO_GPU_F_VIRGL 0
-#define VIRTIO_GPU_F_EDID 1
+/*
+ * VIRTIO_GPU_CMD_CTX_*
+ * VIRTIO_GPU_CMD_*_3D
+ */
+#define VIRTIO_GPU_F_VIRGL 0
+
+/*
+ * VIRTIO_GPU_CMD_GET_EDID
+ */
+#define VIRTIO_GPU_F_EDID 1
enum virtio_gpu_ctrl_type {
VIRTIO_GPU_UNDEFINED = 0,
--
2.18.1
^ permalink raw reply related
* [PATCH 2/3] virtio-gpu api: VIRTIO_GPU_F_MEMORY
From: Gerd Hoffmann @ 2019-04-10 11:42 UTC (permalink / raw)
To: dri-devel, virtio
Cc: Tomeu Vizoso, Michael S. Tsirkin, David Airlie, open list,
Gurchetan Singh, Marc-André Lureau, David Airlie,
open list:VIRTIO CORE, NET AND BLOCK DRIVERS
In-Reply-To: <20190410114227.25846-1-kraxel@redhat.com>
Introduce the concept of memory regions to virtio-gpu. Initially only
memory regions composed of guest pages are supported (pretty much like
current backing storage for resources). I expect support for other
memory types will be added later on.
VIRTIO_GPU_CMD_MEMORY_CREATE:
creates a new memory region.
VIRTIO_GPU_CMD_MEMORY_UNREF:
destroys a memory region.
VIRTIO_GPU_CMD_RESOURCE_ATTACH_MEMORY:
Use memory region as backing storage for the given resource.
The existing VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING command
can be used to detach.
Motivation: This separates storage management from resource management.
It allows memory pooling (vulkan support will most likely need this).
It makes things a bit more flexible in general, for example we can
represent gem objects as memory regions even if we don't know the format
yet (happens on dma-buf import for example).
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/uapi/linux/virtio_gpu.h | 38 +++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 0c85914d9369..732bb16a39f8 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -51,6 +51,13 @@
*/
#define VIRTIO_GPU_F_EDID 1
+/*
+ * VIRTIO_GPU_CMD_MEMORY_CREATE
+ * VIRTIO_GPU_CMD_MEMORY_UNREF
+ * VIRTIO_GPU_CMD_RESOURCE_ATTACH_MEMORY
+ */
+#define VIRTIO_GPU_F_MEMORY 2
+
enum virtio_gpu_ctrl_type {
VIRTIO_GPU_UNDEFINED = 0,
@@ -66,6 +73,9 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_CMD_GET_CAPSET_INFO,
VIRTIO_GPU_CMD_GET_CAPSET,
VIRTIO_GPU_CMD_GET_EDID,
+ VIRTIO_GPU_CMD_MEMORY_CREATE,
+ VIRTIO_GPU_CMD_MEMORY_UNREF,
+ VIRTIO_GPU_CMD_RESOURCE_ATTACH_MEMORY,
/* 3d commands */
VIRTIO_GPU_CMD_CTX_CREATE = 0x0200,
@@ -95,6 +105,7 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID,
VIRTIO_GPU_RESP_ERR_INVALID_CONTEXT_ID,
VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER,
+ VIRTIO_GPU_RESP_ERR_INVALID_MEMORY_ID,
};
#define VIRTIO_GPU_FLAG_FENCE (1 << 0)
@@ -187,6 +198,7 @@ struct virtio_gpu_resource_attach_backing {
struct virtio_gpu_ctrl_hdr hdr;
__le32 resource_id;
__le32 nr_entries;
+ /* struct virtio_gpu_mem_entry entries follow here */
};
/* VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING */
@@ -270,6 +282,32 @@ struct virtio_gpu_cmd_submit {
__le32 padding;
};
+/* VIRTIO_GPU_CMD_MEMORY_CREATE */
+struct virtio_gpu_cmd_memory_create {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le64 size;
+ __le32 memory_id;
+ __le32 nr_entries;
+ __le32 flags;
+ __le32 padding;
+ /* struct virtio_gpu_mem_entry entries follow here */
+};
+
+/* VIRTIO_GPU_CMD_MEMORY_UNREF */
+struct virtio_gpu_cmd_memory_unref {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 memory_id;
+ __le32 padding;
+};
+
+/* VIRTIO_GPU_CMD_RESOURCE_ATTACH_MEMORY */
+struct virtio_gpu_cmd_resource_attach_memory {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 resource_id;
+ __le32 memory_id;
+ __le64 offset[4];
+};
+
#define VIRTIO_GPU_CAPSET_VIRGL 1
#define VIRTIO_GPU_CAPSET_VIRGL2 2
--
2.18.1
^ permalink raw reply related
* [PATCH 3/3] virtio-gpu api: VIRTIO_GPU_F_RESSOURCE_V2
From: Gerd Hoffmann @ 2019-04-10 11:42 UTC (permalink / raw)
To: dri-devel, virtio
Cc: Tomeu Vizoso, Michael S. Tsirkin, David Airlie, open list,
Gurchetan Singh, Marc-André Lureau, David Airlie,
open list:VIRTIO CORE, NET AND BLOCK DRIVERS
In-Reply-To: <20190410114227.25846-1-kraxel@redhat.com>
Add new command VIRTIO_GPU_CMD_RESOURCE_CREATE_V2 to create resources.
It adds (a) support planar resources and (b) returns stride and size of
the resource planes. The later will be needed in case we support
mapping host resources into the guest some day.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/uapi/linux/virtio_gpu.h | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 732bb16a39f8..00010315e500 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -58,6 +58,11 @@
*/
#define VIRTIO_GPU_F_MEMORY 2
+/*
+ * VIRTIO_GPU_CMD_RESOURCE_CREATE_V2 command
+ */
+#define VIRTIO_GPU_F_RESSOURCE_V2 3
+
enum virtio_gpu_ctrl_type {
VIRTIO_GPU_UNDEFINED = 0,
@@ -76,6 +81,7 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_CMD_MEMORY_CREATE,
VIRTIO_GPU_CMD_MEMORY_UNREF,
VIRTIO_GPU_CMD_RESOURCE_ATTACH_MEMORY,
+ VIRTIO_GPU_CMD_RESOURCE_CREATE_V2,
/* 3d commands */
VIRTIO_GPU_CMD_CTX_CREATE = 0x0200,
@@ -97,6 +103,7 @@ enum virtio_gpu_ctrl_type {
VIRTIO_GPU_RESP_OK_CAPSET_INFO,
VIRTIO_GPU_RESP_OK_CAPSET,
VIRTIO_GPU_RESP_OK_EDID,
+ VIRTIO_GPU_RESP_OK_RESOURCE_INFO,
/* error responses */
VIRTIO_GPU_RESP_ERR_UNSPEC = 0x1200,
@@ -308,6 +315,30 @@ struct virtio_gpu_cmd_resource_attach_memory {
__le64 offset[4];
};
+/* VIRTIO_GPU_CMD_RESOURCE_CREATE_V2 */
+struct virtio_gpu_cmd_resource_create_v2 {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 resource_id;
+ __le32 format;
+ __le32 width;
+ __le32 height;
+ /* 3d only */
+ __le32 target;
+ __le32 bind;
+ __le32 depth;
+ __le32 array_size;
+ __le32 last_level;
+ __le32 nr_samples;
+ __le32 flags;
+};
+
+/* VIRTIO_GPU_RESP_OK_RESOURCE_INFO */
+struct virtio_gpu_resp_resource_info {
+ struct virtio_gpu_ctrl_hdr hdr;
+ __le32 stride[4];
+ __le32 size[4];
+};
+
#define VIRTIO_GPU_CAPSET_VIRGL 1
#define VIRTIO_GPU_CAPSET_VIRGL2 2
--
2.18.1
^ permalink raw reply related
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Cornelia Huck @ 2019-04-10 12:24 UTC (permalink / raw)
To: Pankaj Gupta
Cc: jack, kvm, mst, david, qemu-devel, virtualization, adilger.kernel,
zwisler, aarcange, dave.jiang, linux-nvdimm, vishal.l.verma,
willy, hch, linux-acpi, jmoyer, linux-ext4, lenb, kilobyte, riel,
yuval.shaia, stefanha, pbonzini, dan.j.williams, lcapitulino,
nilal, tytso, xiaoguangrong.eric, darrick.wong, rjw, linux-kernel,
linux-xfs, linux-fsdevel, imammedo
In-Reply-To: <20190410040826.24371-3-pagupta@redhat.com>
On Wed, 10 Apr 2019 09:38:22 +0530
Pankaj Gupta <pagupta@redhat.com> 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 <pagupta@redhat.com>
> ---
> drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> drivers/virtio/Kconfig | 10 +++
> drivers/virtio/Makefile | 1 +
> drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> include/linux/virtio_pmem.h | 60 +++++++++++++++
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_pmem.h | 10 +++
> 7 files changed, 294 insertions(+)
> create mode 100644 drivers/nvdimm/virtio_pmem.c
> create mode 100644 drivers/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/pmem.c b/drivers/virtio/pmem.c
> new file mode 100644
> index 000000000000..cc9de9589d56
> --- /dev/null
> +++ b/drivers/virtio/pmem.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * virtio_pmem.c: Virtio pmem Driver
> + *
> + * Discovers persistent memory range information
> + * from host and registers the virtual pmem device
> + * with libnvdimm core.
> + */
> +#include <linux/virtio_pmem.h>
> +#include <../../drivers/nvdimm/nd.h>
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> + /* Initialize virt queue */
> +static int init_vq(struct virtio_pmem *vpmem)
IMHO, you don't gain much by splitting off this function...
> +{
> + 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);
I'm personally not a fan of chained assignments... I think I'd just
drop the 'vq' variable and operate on vpmem->req_vq directly.
> +
> + spin_lock_init(&vpmem->pmem_lock);
> + INIT_LIST_HEAD(&vpmem->req_list);
> +
> + return 0;
> +};
> +
> +static int virtio_pmem_probe(struct virtio_device *vdev)
> +{
> + int err = 0;
> + struct resource res;
> + struct virtio_pmem *vpmem;
> + struct nvdimm_bus *nvdimm_bus;
> + struct nd_region_desc ndr_desc = {};
> + int nid = dev_to_node(&vdev->dev);
> + struct nd_region *nd_region;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config disabled\n",
Maybe s/config disabled/config access disabled/ ? That seems to be the
more common message.
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> + GFP_KERNEL);
Here, the vpmem variable makes sense for convenience, but I'm again not
a fan of the chaining :)
> + if (!vpmem) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> +
> + vpmem->vdev = vdev;
> + err = init_vq(vpmem);
> + if (err)
> + goto out_err;
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, &vpmem->start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, &vpmem->size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> + &vpmem->nd_desc);
And here :)
> + if (!nvdimm_bus)
> + goto out_vq;
> +
> + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> + nd_region->provider_data = dev_to_virtio
> + (nd_region->dev.parent->parent);
Isn't it clear that this parent chain will always end up at &vdev->dev?
Maybe simply set ->provider_data to vdev directly? (Does it need to
grab a reference count of the device, BTW?)
> +
> + if (!nd_region)
> + goto out_nd;
Probably better to do this check before you access nd_region's
members :)
> +
> + return 0;
> +out_nd:
> + err = -ENXIO;
> + nvdimm_bus_unregister(nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> + return err;
> +}
> +
> +static void virtio_pmem_remove(struct virtio_device *vdev)
> +{
> + struct virtio_pmem *vpmem = vdev->priv;
> + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> +
> + nvdimm_bus_unregister(nvdimm_bus);
I haven't followed this around the nvdimm code, but is the nd_region
you created during probe cleaned up automatically, or would you need to
do something here?
> + vdev->config->del_vqs(vdev);
> + vdev->config->reset(vdev);
> + kfree(vpmem);
You allocated vpmem via devm_kzalloc; isn't it freed automatically on
remove?
> +}
> +
> +static struct virtio_driver virtio_pmem_driver = {
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = id_table,
> + .probe = virtio_pmem_probe,
> + .remove = virtio_pmem_remove,
> +};
> +
> +module_virtio_driver(virtio_pmem_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio pmem driver");
> +MODULE_LICENSE("GPL");
Only looked at this from the general virtio driver angle; seems fine
apart from some easy-to-fix issues and some personal style preference
things.
^ permalink raw reply
* Re: [PATCH] drm/bochs: use simple display pipe
From: Noralf Trønnes @ 2019-04-10 12:55 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list,
open list:DRM DRIVER FOR BOCHS VIRTUAL GPU
In-Reply-To: <20190410074828.10296-1-kraxel@redhat.com>
Den 10.04.2019 09.48, skrev Gerd Hoffmann:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
Acked-by: Noralf Trønnes <noralf@tronnes.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/2] virtio-blk: limit number of hw queues by nr_cpu_ids
From: Stefan Hajnoczi @ 2019-04-10 13:12 UTC (permalink / raw)
To: Dongli Zhang
Cc: axboe, martin.petersen, linux-scsi, mst, cohuck, linux-kernel,
virtualization, linux-block, jejb
In-Reply-To: <1553682995-5682-2-git-send-email-dongli.zhang@oracle.com>
[-- Attachment #1.1: Type: text/plain, Size: 926 bytes --]
On Wed, Mar 27, 2019 at 06:36:34PM +0800, Dongli Zhang wrote:
> When tag_set->nr_maps is 1, the block layer limits the number of hw queues
> by nr_cpu_ids. No matter how many hw queues are used by virtio-blk, as it
> has (tag_set->nr_maps == 1), it can use at most nr_cpu_ids hw queues.
>
> In addition, specifically for pci scenario, when the 'num-queues' specified
> by qemu is more than maxcpus, virtio-blk would not be able to allocate more
> than maxcpus vectors in order to have a vector for each queue. As a result,
> it falls back into MSI-X with one vector for config and one shared for
> queues.
>
> Considering above reasons, this patch limits the number of hw queues used
> by virtio-blk by nr_cpu_ids.
>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
> drivers/block/virtio_blk.c | 2 ++
> 1 file changed, 2 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 2/2] scsi: virtio_scsi: limit number of hw queues by nr_cpu_ids
From: Stefan Hajnoczi @ 2019-04-10 13:12 UTC (permalink / raw)
To: Dongli Zhang
Cc: axboe, martin.petersen, linux-scsi, mst, cohuck, linux-kernel,
virtualization, linux-block, jejb
In-Reply-To: <1553682995-5682-3-git-send-email-dongli.zhang@oracle.com>
[-- Attachment #1.1: Type: text/plain, Size: 927 bytes --]
On Wed, Mar 27, 2019 at 06:36:35PM +0800, Dongli Zhang wrote:
> When tag_set->nr_maps is 1, the block layer limits the number of hw queues
> by nr_cpu_ids. No matter how many hw queues are used by virtio-scsi, as it
> has (tag_set->nr_maps == 1), it can use at most nr_cpu_ids hw queues.
>
> In addition, specifically for pci scenario, when the 'num_queues' specified
> by qemu is more than maxcpus, virtio-scsi would not be able to allocate
> more than maxcpus vectors in order to have a vector for each queue. As a
> result, it falls back into MSI-X with one vector for config and one shared
> for queues.
>
> Considering above reasons, this patch limits the number of hw queues used
> by virtio-scsi by nr_cpu_ids.
>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
> drivers/scsi/virtio_scsi.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Michael S. Tsirkin @ 2019-04-10 13:12 UTC (permalink / raw)
To: Pankaj Gupta
Cc: cohuck, jack, kvm, david, qemu-devel, virtualization,
adilger.kernel, zwisler, aarcange, dave.jiang, linux-nvdimm,
vishal.l.verma, willy, hch, linux-acpi, jmoyer, linux-ext4, lenb,
kilobyte, riel, yuval.shaia, stefanha, pbonzini, dan.j.williams,
lcapitulino, nilal, tytso, xiaoguangrong.eric, darrick.wong, rjw,
linux-kernel, linux-xfs, linux-fsdevel, imammedo
In-Reply-To: <20190410040826.24371-3-pagupta@redhat.com>
On Wed, Apr 10, 2019 at 09:38:22AM +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 <pagupta@redhat.com>
> ---
> drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> drivers/virtio/Kconfig | 10 +++
> drivers/virtio/Makefile | 1 +
> drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> include/linux/virtio_pmem.h | 60 +++++++++++++++
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_pmem.h | 10 +++
> 7 files changed, 294 insertions(+)
> create mode 100644 drivers/nvdimm/virtio_pmem.c
> create mode 100644 drivers/virtio/pmem.c
> create mode 100644 include/linux/virtio_pmem.h
> create mode 100644 include/uapi/linux/virtio_pmem.h
>
> diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> new file mode 100644
> index 000000000000..01044cc2f3a3
> --- /dev/null
> +++ b/drivers/nvdimm/virtio_pmem.c
> @@ -0,0 +1,88 @@
> +// 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 <linux/virtio_pmem.h>
> +#include "nd.h"
> +
> + /* The interrupt handler */
> +void host_ack(struct virtqueue *vq)
> +{
> + unsigned int len;
> + unsigned long flags;
> + struct virtio_pmem_request *req, *req_buf;
> + struct virtio_pmem *vpmem = vq->vdev->priv;
> +
> + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> + while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> + req->done = true;
> + wake_up(&req->host_acked);
> +
> + if (!list_empty(&vpmem->req_list)) {
> + req_buf = list_first_entry(&vpmem->req_list,
> + struct virtio_pmem_request, list);
> + list_del(&vpmem->req_list);
> + req_buf->wq_buf_avail = true;
> + wake_up(&req_buf->wq_buf);
> + }
> + }
> + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(host_ack);
> +
> + /* The request submission function */
> +int virtio_pmem_flush(struct nd_region *nd_region)
> +{
> + int err;
> + unsigned long flags;
> + struct scatterlist *sgs[2], sg, ret;
> + struct virtio_device *vdev = nd_region->provider_data;
> + struct virtio_pmem *vpmem = vdev->priv;
> + struct virtio_pmem_request *req;
> +
> + might_sleep();
> + req = kmalloc(sizeof(*req), GFP_KERNEL);
> + if (!req)
> + return -ENOMEM;
> +
> + req->done = req->wq_buf_avail = false;
> + strcpy(req->name, "FLUSH");
> + init_waitqueue_head(&req->host_acked);
> + init_waitqueue_head(&req->wq_buf);
> + sg_init_one(&sg, req->name, strlen(req->name));
> + sgs[0] = &sg;
> + sg_init_one(&ret, &req->ret, sizeof(req->ret));
> + sgs[1] = &ret;
> +
> + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> + err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC);
> + if (err) {
> + dev_err(&vdev->dev, "failed to send command to virtio pmem device\n");
> +
> + list_add_tail(&vpmem->req_list, &req->list);
> + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +
> + /* When host has read buffer, this completes via host_ack */
> + wait_event(req->wq_buf, req->wq_buf_avail);
> + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> + }
> + err = virtqueue_kick(vpmem->req_vq);
> + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +
> + if (!err) {
> + err = -EIO;
> + goto ret;
> + }
> + /* When host has read buffer, this completes via host_ack */
> + wait_event(req->host_acked, req->done);
> + err = req->ret;
> +ret:
> + kfree(req);
> + return err;
> +};
> +EXPORT_SYMBOL_GPL(virtio_pmem_flush);
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 35897649c24f..9f634a2ed638 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -42,6 +42,16 @@ config VIRTIO_PCI_LEGACY
>
> If unsure, say Y.
>
> +config VIRTIO_PMEM
> + tristate "Support for virtio pmem driver"
> + depends on VIRTIO
> + depends on LIBNVDIMM
> + 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 3a2b5c5dcf46..143ce91eabe9 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) += pmem.o ../nvdimm/virtio_pmem.o
> diff --git a/drivers/virtio/pmem.c b/drivers/virtio/pmem.c
> new file mode 100644
> index 000000000000..cc9de9589d56
> --- /dev/null
> +++ b/drivers/virtio/pmem.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * virtio_pmem.c: Virtio pmem Driver
> + *
> + * Discovers persistent memory range information
> + * from host and registers the virtual pmem device
> + * with libnvdimm core.
> + */
> +#include <linux/virtio_pmem.h>
> +#include <../../drivers/nvdimm/nd.h>
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> + /* 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(&vpmem->pmem_lock);
> + INIT_LIST_HEAD(&vpmem->req_list);
> +
> + return 0;
> +};
> +
> +static int virtio_pmem_probe(struct virtio_device *vdev)
> +{
> + int err = 0;
> + struct resource res;
> + struct virtio_pmem *vpmem;
> + struct nvdimm_bus *nvdimm_bus;
> + struct nd_region_desc ndr_desc = {};
> + int nid = dev_to_node(&vdev->dev);
> + struct nd_region *nd_region;
> +
> + if (!vdev->config->get) {
> + dev_err(&vdev->dev, "%s failure: config disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> + GFP_KERNEL);
> + if (!vpmem) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> +
> + vpmem->vdev = vdev;
> + err = init_vq(vpmem);
> + if (err)
> + goto out_err;
> +
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + start, &vpmem->start);
> + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> + size, &vpmem->size);
> +
> + res.start = vpmem->start;
> + res.end = vpmem->start + vpmem->size-1;
> + vpmem->nd_desc.provider_name = "virtio-pmem";
> + vpmem->nd_desc.module = THIS_MODULE;
> +
> + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> + &vpmem->nd_desc);
> + if (!nvdimm_bus)
> + goto out_vq;
> +
> + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> + nd_region->provider_data = dev_to_virtio
> + (nd_region->dev.parent->parent);
> +
> + if (!nd_region)
> + goto out_nd;
> +
> + return 0;
> +out_nd:
> + err = -ENXIO;
> + nvdimm_bus_unregister(nvdimm_bus);
> +out_vq:
> + vdev->config->del_vqs(vdev);
> +out_err:
> + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> + return err;
> +}
> +
> +static void virtio_pmem_remove(struct virtio_device *vdev)
> +{
> + struct virtio_pmem *vpmem = vdev->priv;
> + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> +
> + nvdimm_bus_unregister(nvdimm_bus);
> + vdev->config->del_vqs(vdev);
> + vdev->config->reset(vdev);
> + kfree(vpmem);
> +}
> +
> +static struct virtio_driver virtio_pmem_driver = {
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = id_table,
> + .probe = virtio_pmem_probe,
> + .remove = virtio_pmem_remove,
> +};
> +
> +module_virtio_driver(virtio_pmem_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio pmem driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/virtio_pmem.h b/include/linux/virtio_pmem.h
> new file mode 100644
> index 000000000000..224f9d934be6
> --- /dev/null
> +++ b/include/linux/virtio_pmem.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * virtio_pmem.h: virtio pmem Driver
> + *
> + * Discovers persistent memory range information
> + * from host and provides a virtio based flushing
> + * interface.
> + **/
> +
> +#ifndef _LINUX_VIRTIO_PMEM_H
> +#define _LINUX_VIRTIO_PMEM_H
> +
> +#include <linux/virtio_ids.h>
> +#include <linux/module.h>
> +#include <linux/virtio_config.h>
> +#include <uapi/linux/virtio_pmem.h>
> +#include <linux/libnvdimm.h>
> +#include <linux/spinlock.h>
> +
> +struct virtio_pmem_request {
> + /* Host return status corresponding to flush request */
> + int ret;
> +
> + /* command name*/
> + char name[16];
> +
> + /* Wait queue to process deferred work after ack from host */
> + wait_queue_head_t host_acked;
> + bool done;
> +
> + /* Wait queue to process deferred work after virt queue buffer avail */
> + wait_queue_head_t wq_buf;
> + bool wq_buf_avail;
> + struct list_head list;
> +};
> +
> +struct virtio_pmem {
> + struct virtio_device *vdev;
> +
> + /* Virtio pmem request queue */
> + struct virtqueue *req_vq;
> +
> + /* nvdimm bus registers virtio pmem device */
> + struct nvdimm_bus *nvdimm_bus;
> + struct nvdimm_bus_descriptor nd_desc;
> +
> + /* List to store deferred work if virtqueue is full */
> + struct list_head req_list;
> +
> + /* Synchronize virtqueue data */
> + spinlock_t pmem_lock;
> +
> + /* Memory region information */
> + uint64_t start;
> + uint64_t size;
> +};
> +
> +void host_ack(struct virtqueue *vq);
> +int virtio_pmem_flush(struct nd_region *nd_region);
> +#endif
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 6d5c3b2d4f4d..346389565ac1 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -43,5 +43,6 @@
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> #define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
> #define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
> +#define VIRTIO_ID_PMEM 25 /* virtio pmem */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
Didn't Paolo point out someone is using 25 for audio?
Please, reserve an ID with the Virtio TC before using it.
> diff --git a/include/uapi/linux/virtio_pmem.h b/include/uapi/linux/virtio_pmem.h
> new file mode 100644
> index 000000000000..fa3f7d52717a
> --- /dev/null
> +++ b/include/uapi/linux/virtio_pmem.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_PMEM_H
> +#define _UAPI_LINUX_VIRTIO_PMEM_H
> +
> +struct virtio_pmem_config {
> + __le64 start;
> + __le64 size;
> +};
> +#endif
> --
> 2.20.1
^ permalink raw reply
* [PULL] virtio: fixes
From: Michael S. Tsirkin @ 2019-04-10 13:18 UTC (permalink / raw)
To: Linus Torvalds
Cc: kvm, mst, netdev, cohuck, linux-kernel, virtualization, longpeng2
The following changes since commit 15ade5d2e7775667cf191cf2f94327a4889f8b9d:
Linux 5.1-rc4 (2019-04-07 14:09:59 -1000)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to cf94db21905333e610e479688add629397a4b384:
virtio: Honour 'may_reduce_num' in vring_create_virtqueue (2019-04-08 17:05:52 -0400)
----------------------------------------------------------------
virtio: fixes, reviewers
Several fixes, add more reviewers to the list
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Cornelia Huck (1):
virtio: Honour 'may_reduce_num' in vring_create_virtqueue
Longpeng (1):
virtio_pci: fix a NULL pointer reference in vp_del_vqs
Michael S. Tsirkin (1):
MAiNTAINERS: add Paolo, Stefan for virtio blk/scsi
MAINTAINERS | 15 ++++++++++++++-
drivers/virtio/virtio_pci_common.c | 8 +++++---
drivers/virtio/virtio_ring.c | 2 ++
include/linux/virtio_ring.h | 2 +-
4 files changed, 22 insertions(+), 5 deletions(-)
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Pankaj Gupta @ 2019-04-10 14:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jack, kvm, david, qemu-devel, virtualization, adilger kernel,
zwisler, aarcange, dave jiang, linux-nvdimm, vishal l verma,
willy, hch, linux-acpi, jmoyer, linux-ext4, lenb, kilobyte, riel,
yuval shaia, stefanha, imammedo, dan j williams, lcapitulino,
nilal, tytso, xiaoguangrong eric, cohuck, rjw, linux-kernel,
linux-xfs, linux-fsdevel, pbonzini, darrick wong <darric>
In-Reply-To: <20190410091216-mutt-send-email-mst@kernel.org>
>
> On Wed, Apr 10, 2019 at 09:38:22AM +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 <pagupta@redhat.com>
> > ---
> > drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> > drivers/virtio/Kconfig | 10 +++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> > include/linux/virtio_pmem.h | 60 +++++++++++++++
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 10 +++
> > 7 files changed, 294 insertions(+)
> > create mode 100644 drivers/nvdimm/virtio_pmem.c
> > create mode 100644 drivers/virtio/pmem.c
> > create mode 100644 include/linux/virtio_pmem.h
> > create mode 100644 include/uapi/linux/virtio_pmem.h
> >
> > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c
> > new file mode 100644
> > index 000000000000..01044cc2f3a3
> > --- /dev/null
> > +++ b/drivers/nvdimm/virtio_pmem.c
> > @@ -0,0 +1,88 @@
> > +// 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 <linux/virtio_pmem.h>
> > +#include "nd.h"
> > +
> > + /* The interrupt handler */
> > +void host_ack(struct virtqueue *vq)
> > +{
> > + unsigned int len;
> > + unsigned long flags;
> > + struct virtio_pmem_request *req, *req_buf;
> > + struct virtio_pmem *vpmem = vq->vdev->priv;
> > +
> > + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> > + while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> > + req->done = true;
> > + wake_up(&req->host_acked);
> > +
> > + if (!list_empty(&vpmem->req_list)) {
> > + req_buf = list_first_entry(&vpmem->req_list,
> > + struct virtio_pmem_request, list);
> > + list_del(&vpmem->req_list);
> > + req_buf->wq_buf_avail = true;
> > + wake_up(&req_buf->wq_buf);
> > + }
> > + }
> > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +}
> > +EXPORT_SYMBOL_GPL(host_ack);
> > +
> > + /* The request submission function */
> > +int virtio_pmem_flush(struct nd_region *nd_region)
> > +{
> > + int err;
> > + unsigned long flags;
> > + struct scatterlist *sgs[2], sg, ret;
> > + struct virtio_device *vdev = nd_region->provider_data;
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct virtio_pmem_request *req;
> > +
> > + might_sleep();
> > + req = kmalloc(sizeof(*req), GFP_KERNEL);
> > + if (!req)
> > + return -ENOMEM;
> > +
> > + req->done = req->wq_buf_avail = false;
> > + strcpy(req->name, "FLUSH");
> > + init_waitqueue_head(&req->host_acked);
> > + init_waitqueue_head(&req->wq_buf);
> > + sg_init_one(&sg, req->name, strlen(req->name));
> > + sgs[0] = &sg;
> > + sg_init_one(&ret, &req->ret, sizeof(req->ret));
> > + sgs[1] = &ret;
> > +
> > + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> > + err = virtqueue_add_sgs(vpmem->req_vq, sgs, 1, 1, req, GFP_ATOMIC);
> > + if (err) {
> > + dev_err(&vdev->dev, "failed to send command to virtio pmem device\n");
> > +
> > + list_add_tail(&vpmem->req_list, &req->list);
> > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +
> > + /* When host has read buffer, this completes via host_ack */
> > + wait_event(req->wq_buf, req->wq_buf_avail);
> > + spin_lock_irqsave(&vpmem->pmem_lock, flags);
> > + }
> > + err = virtqueue_kick(vpmem->req_vq);
> > + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> > +
> > + if (!err) {
> > + err = -EIO;
> > + goto ret;
> > + }
> > + /* When host has read buffer, this completes via host_ack */
> > + wait_event(req->host_acked, req->done);
> > + err = req->ret;
> > +ret:
> > + kfree(req);
> > + return err;
> > +};
> > +EXPORT_SYMBOL_GPL(virtio_pmem_flush);
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 35897649c24f..9f634a2ed638 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -42,6 +42,16 @@ config VIRTIO_PCI_LEGACY
> >
> > If unsure, say Y.
> >
> > +config VIRTIO_PMEM
> > + tristate "Support for virtio pmem driver"
> > + depends on VIRTIO
> > + depends on LIBNVDIMM
> > + 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 3a2b5c5dcf46..143ce91eabe9 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) += pmem.o ../nvdimm/virtio_pmem.o
> > diff --git a/drivers/virtio/pmem.c b/drivers/virtio/pmem.c
> > new file mode 100644
> > index 000000000000..cc9de9589d56
> > --- /dev/null
> > +++ b/drivers/virtio/pmem.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and registers the virtual pmem device
> > + * with libnvdimm core.
> > + */
> > +#include <linux/virtio_pmem.h>
> > +#include <../../drivers/nvdimm/nd.h>
> > +
> > +static struct virtio_device_id id_table[] = {
> > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > + { 0 },
> > +};
> > +
> > + /* 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(&vpmem->pmem_lock);
> > + INIT_LIST_HEAD(&vpmem->req_list);
> > +
> > + return 0;
> > +};
> > +
> > +static int virtio_pmem_probe(struct virtio_device *vdev)
> > +{
> > + int err = 0;
> > + struct resource res;
> > + struct virtio_pmem *vpmem;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct nd_region_desc ndr_desc = {};
> > + int nid = dev_to_node(&vdev->dev);
> > + struct nd_region *nd_region;
> > +
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "%s failure: config disabled\n",
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> > + GFP_KERNEL);
> > + if (!vpmem) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + vpmem->vdev = vdev;
> > + err = init_vq(vpmem);
> > + if (err)
> > + goto out_err;
> > +
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + start, &vpmem->start);
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + size, &vpmem->size);
> > +
> > + res.start = vpmem->start;
> > + res.end = vpmem->start + vpmem->size-1;
> > + vpmem->nd_desc.provider_name = "virtio-pmem";
> > + vpmem->nd_desc.module = THIS_MODULE;
> > +
> > + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > + &vpmem->nd_desc);
> > + if (!nvdimm_bus)
> > + goto out_vq;
> > +
> > + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> > +
> > + ndr_desc.res = &res;
> > + ndr_desc.numa_node = nid;
> > + ndr_desc.flush = virtio_pmem_flush;
> > + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> > + nd_region->provider_data = dev_to_virtio
> > + (nd_region->dev.parent->parent);
> > +
> > + if (!nd_region)
> > + goto out_nd;
> > +
> > + return 0;
> > +out_nd:
> > + err = -ENXIO;
> > + nvdimm_bus_unregister(nvdimm_bus);
> > +out_vq:
> > + vdev->config->del_vqs(vdev);
> > +out_err:
> > + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> > + return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > + nvdimm_bus_unregister(nvdimm_bus);
> > + vdev->config->del_vqs(vdev);
> > + vdev->config->reset(vdev);
> > + kfree(vpmem);
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > + .driver.name = KBUILD_MODNAME,
> > + .driver.owner = THIS_MODULE,
> > + .id_table = id_table,
> > + .probe = virtio_pmem_probe,
> > + .remove = virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/virtio_pmem.h b/include/linux/virtio_pmem.h
> > new file mode 100644
> > index 000000000000..224f9d934be6
> > --- /dev/null
> > +++ b/include/linux/virtio_pmem.h
> > @@ -0,0 +1,60 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * virtio_pmem.h: virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and provides a virtio based flushing
> > + * interface.
> > + **/
> > +
> > +#ifndef _LINUX_VIRTIO_PMEM_H
> > +#define _LINUX_VIRTIO_PMEM_H
> > +
> > +#include <linux/virtio_ids.h>
> > +#include <linux/module.h>
> > +#include <linux/virtio_config.h>
> > +#include <uapi/linux/virtio_pmem.h>
> > +#include <linux/libnvdimm.h>
> > +#include <linux/spinlock.h>
> > +
> > +struct virtio_pmem_request {
> > + /* Host return status corresponding to flush request */
> > + int ret;
> > +
> > + /* command name*/
> > + char name[16];
> > +
> > + /* Wait queue to process deferred work after ack from host */
> > + wait_queue_head_t host_acked;
> > + bool done;
> > +
> > + /* Wait queue to process deferred work after virt queue buffer avail */
> > + wait_queue_head_t wq_buf;
> > + bool wq_buf_avail;
> > + struct list_head list;
> > +};
> > +
> > +struct virtio_pmem {
> > + struct virtio_device *vdev;
> > +
> > + /* Virtio pmem request queue */
> > + struct virtqueue *req_vq;
> > +
> > + /* nvdimm bus registers virtio pmem device */
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct nvdimm_bus_descriptor nd_desc;
> > +
> > + /* List to store deferred work if virtqueue is full */
> > + struct list_head req_list;
> > +
> > + /* Synchronize virtqueue data */
> > + spinlock_t pmem_lock;
> > +
> > + /* Memory region information */
> > + uint64_t start;
> > + uint64_t size;
> > +};
> > +
> > +void host_ack(struct virtqueue *vq);
> > +int virtio_pmem_flush(struct nd_region *nd_region);
> > +#endif
> > diff --git a/include/uapi/linux/virtio_ids.h
> > b/include/uapi/linux/virtio_ids.h
> > index 6d5c3b2d4f4d..346389565ac1 100644
> > --- a/include/uapi/linux/virtio_ids.h
> > +++ b/include/uapi/linux/virtio_ids.h
> > @@ -43,5 +43,6 @@
> > #define VIRTIO_ID_INPUT 18 /* virtio input */
> > #define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
> > #define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
> > +#define VIRTIO_ID_PMEM 25 /* virtio pmem */
> >
> > #endif /* _LINUX_VIRTIO_IDS_H */
>
> Didn't Paolo point out someone is using 25 for audio?
Sorry! I did not notice this.
>
> Please, reserve an ID with the Virtio TC before using it.
I thought I requested[1] ID 25.
[1] https://github.com/oasis-tcs/virtio-spec/issues/38
[2] https://lists.oasis-open.org/archives/virtio-dev/201805/threads.html
In that case I will send request for next available ID i.e 26?
Thanks,
Pankaj
>
> > diff --git a/include/uapi/linux/virtio_pmem.h
> > b/include/uapi/linux/virtio_pmem.h
> > new file mode 100644
> > index 000000000000..fa3f7d52717a
> > --- /dev/null
> > +++ b/include/uapi/linux/virtio_pmem.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _UAPI_LINUX_VIRTIO_PMEM_H
> > +#define _UAPI_LINUX_VIRTIO_PMEM_H
> > +
> > +struct virtio_pmem_config {
> > + __le64 start;
> > + __le64 size;
> > +};
> > +#endif
> > --
> > 2.20.1
>
>
^ permalink raw reply
* Re: [PATCH 0/2] Limit number of hw queues by nr_cpu_ids for virtio-blk and virtio-scsi
From: Jens Axboe @ 2019-04-10 14:18 UTC (permalink / raw)
To: Dongli Zhang, linux-scsi, virtualization, linux-block
Cc: martin.petersen, mst, cohuck, linux-kernel, jejb
In-Reply-To: <1553682995-5682-1-git-send-email-dongli.zhang@oracle.com>
On 3/27/19 4:36 AM, Dongli Zhang wrote:
> When tag_set->nr_maps is 1, the block layer limits the number of hw queues
> by nr_cpu_ids. No matter how many hw queues are use by
> virtio-blk/virtio-scsi, as they both have (tag_set->nr_maps == 1), they
> can use at most nr_cpu_ids hw queues.
>
> In addition, specifically for pci scenario, when the 'num-queues' specified
> by qemu is more than maxcpus, virtio-blk/virtio-scsi would not be able to
> allocate more than maxcpus vectors in order to have a vector for each
> queue. As a result, they fall back into MSI-X with one vector for config
> and one shared for queues.
>
> Considering above reasons, this patch set limits the number of hw queues
> used by nr_cpu_ids for both virtio-blk and virtio-scsi.
I picked both up for 5.1.
--
Jens Axboe
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Cornelia Huck @ 2019-04-10 14:31 UTC (permalink / raw)
To: Pankaj Gupta
Cc: jack, kvm, david, qemu-devel, virtualization, adilger kernel,
zwisler, aarcange, dave jiang, linux-nvdimm, vishal l verma,
Michael S. Tsirkin, willy, hch, linux-acpi, jmoyer, linux-ext4,
lenb, kilobyte, riel, yuval shaia, stefanha, imammedo,
dan j williams, lcapitulino, nilal, tytso, xiaoguangrong eric,
darrick wong, rjw, linux-kernel, linux-xfs, linux-fsdevel
In-Reply-To: <127904196.20807948.1554904981681.JavaMail.zimbra@redhat.com>
On Wed, 10 Apr 2019 10:03:01 -0400 (EDT)
Pankaj Gupta <pagupta@redhat.com> wrote:
> >
> > On Wed, Apr 10, 2019 at 09:38:22AM +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 <pagupta@redhat.com>
> > > diff --git a/include/uapi/linux/virtio_ids.h
> > > b/include/uapi/linux/virtio_ids.h
> > > index 6d5c3b2d4f4d..346389565ac1 100644
> > > --- a/include/uapi/linux/virtio_ids.h
> > > +++ b/include/uapi/linux/virtio_ids.h
> > > @@ -43,5 +43,6 @@
> > > #define VIRTIO_ID_INPUT 18 /* virtio input */
> > > #define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
> > > #define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
> > > +#define VIRTIO_ID_PMEM 25 /* virtio pmem */
> > >
> > > #endif /* _LINUX_VIRTIO_IDS_H */
> >
> > Didn't Paolo point out someone is using 25 for audio?
>
>
> Sorry! I did not notice this.
>
> >
> > Please, reserve an ID with the Virtio TC before using it.
>
> I thought I requested[1] ID 25.
>
> [1] https://github.com/oasis-tcs/virtio-spec/issues/38
> [2] https://lists.oasis-open.org/archives/virtio-dev/201805/threads.html
>
> In that case I will send request for next available ID i.e 26?
Have the folks using this for audio sent a reservation request as well?
If not, I'd say the first requester should get the id...
(And yes, we need to be quicker with voting on/applying id
reservations :/)
^ permalink raw reply
* Re: [PATCH v5 2/5] virtio-pmem: Add virtio pmem driver
From: Yuval Shaia @ 2019-04-10 14:38 UTC (permalink / raw)
To: Cornelia Huck
Cc: Pankaj Gupta, jack, kvm, mst, david, qemu-devel, virtualization,
adilger.kernel, zwisler, aarcange, dave.jiang, linux-nvdimm,
vishal.l.verma, willy, hch, linux-acpi, jmoyer, linux-ext4, lenb,
kilobyte, riel, stefanha, pbonzini, dan.j.williams, lcapitulino,
nilal, tytso, xiaoguangrong.eric, darrick.wong, rjw, linux-kernel,
linux-xfs, linux-fsdevel, imammedo
In-Reply-To: <20190410142426.5bf0d9a4.cohuck@redhat.com>
On Wed, Apr 10, 2019 at 02:24:26PM +0200, Cornelia Huck wrote:
> On Wed, 10 Apr 2019 09:38:22 +0530
> Pankaj Gupta <pagupta@redhat.com> 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 <pagupta@redhat.com>
> > ---
> > drivers/nvdimm/virtio_pmem.c | 88 ++++++++++++++++++++++
> > drivers/virtio/Kconfig | 10 +++
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/pmem.c | 124 +++++++++++++++++++++++++++++++
> > include/linux/virtio_pmem.h | 60 +++++++++++++++
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pmem.h | 10 +++
> > 7 files changed, 294 insertions(+)
> > create mode 100644 drivers/nvdimm/virtio_pmem.c
> > create mode 100644 drivers/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/pmem.c b/drivers/virtio/pmem.c
> > new file mode 100644
> > index 000000000000..cc9de9589d56
> > --- /dev/null
> > +++ b/drivers/virtio/pmem.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * virtio_pmem.c: Virtio pmem Driver
> > + *
> > + * Discovers persistent memory range information
> > + * from host and registers the virtual pmem device
> > + * with libnvdimm core.
> > + */
> > +#include <linux/virtio_pmem.h>
> > +#include <../../drivers/nvdimm/nd.h>
> > +
> > +static struct virtio_device_id id_table[] = {
> > + { VIRTIO_ID_PMEM, VIRTIO_DEV_ANY_ID },
> > + { 0 },
> > +};
> > +
> > + /* Initialize virt queue */
> > +static int init_vq(struct virtio_pmem *vpmem)
>
> IMHO, you don't gain much by splitting off this function...
It make sense to have all the vq-init-related stuff in one function, so
here pmem_lock and req_list are used only for the vq.
Saying that - maybe it would be better to have the 3 in one struct.
>
> > +{
> > + 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);
>
> I'm personally not a fan of chained assignments... I think I'd just
> drop the 'vq' variable and operate on vpmem->req_vq directly.
+1
>
> > +
> > + spin_lock_init(&vpmem->pmem_lock);
> > + INIT_LIST_HEAD(&vpmem->req_list);
> > +
> > + return 0;
> > +};
> > +
> > +static int virtio_pmem_probe(struct virtio_device *vdev)
> > +{
> > + int err = 0;
> > + struct resource res;
> > + struct virtio_pmem *vpmem;
> > + struct nvdimm_bus *nvdimm_bus;
> > + struct nd_region_desc ndr_desc = {};
> > + int nid = dev_to_node(&vdev->dev);
> > + struct nd_region *nd_region;
> > +
> > + if (!vdev->config->get) {
> > + dev_err(&vdev->dev, "%s failure: config disabled\n",
>
> Maybe s/config disabled/config access disabled/ ? That seems to be the
> more common message.
>
> > + __func__);
> > + return -EINVAL;
> > + }
> > +
> > + vdev->priv = vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem),
> > + GFP_KERNEL);
>
> Here, the vpmem variable makes sense for convenience, but I'm again not
> a fan of the chaining :)
+1
>
> > + if (!vpmem) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > +
> > + vpmem->vdev = vdev;
> > + err = init_vq(vpmem);
> > + if (err)
> > + goto out_err;
> > +
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + start, &vpmem->start);
> > + virtio_cread(vpmem->vdev, struct virtio_pmem_config,
> > + size, &vpmem->size);
> > +
> > + res.start = vpmem->start;
> > + res.end = vpmem->start + vpmem->size-1;
> > + vpmem->nd_desc.provider_name = "virtio-pmem";
> > + vpmem->nd_desc.module = THIS_MODULE;
> > +
> > + vpmem->nvdimm_bus = nvdimm_bus = nvdimm_bus_register(&vdev->dev,
> > + &vpmem->nd_desc);
>
> And here :)
>
> > + if (!nvdimm_bus)
> > + goto out_vq;
> > +
> > + dev_set_drvdata(&vdev->dev, nvdimm_bus);
> > +
> > + ndr_desc.res = &res;
> > + ndr_desc.numa_node = nid;
> > + ndr_desc.flush = virtio_pmem_flush;
> > + set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> > + set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
> > + nd_region = nvdimm_pmem_region_create(nvdimm_bus, &ndr_desc);
> > + nd_region->provider_data = dev_to_virtio
> > + (nd_region->dev.parent->parent);
>
> Isn't it clear that this parent chain will always end up at &vdev->dev?
> Maybe simply set ->provider_data to vdev directly? (Does it need to
> grab a reference count of the device, BTW?)
>
> > +
> > + if (!nd_region)
> > + goto out_nd;
>
> Probably better to do this check before you access nd_region's
> members :)
>
> > +
> > + return 0;
> > +out_nd:
> > + err = -ENXIO;
> > + nvdimm_bus_unregister(nvdimm_bus);
> > +out_vq:
> > + vdev->config->del_vqs(vdev);
> > +out_err:
> > + dev_err(&vdev->dev, "failed to register virtio pmem memory\n");
> > + return err;
> > +}
> > +
> > +static void virtio_pmem_remove(struct virtio_device *vdev)
> > +{
> > + struct virtio_pmem *vpmem = vdev->priv;
> > + struct nvdimm_bus *nvdimm_bus = dev_get_drvdata(&vdev->dev);
> > +
> > + nvdimm_bus_unregister(nvdimm_bus);
>
> I haven't followed this around the nvdimm code, but is the nd_region
> you created during probe cleaned up automatically, or would you need to
> do something here?
>
> > + vdev->config->del_vqs(vdev);
> > + vdev->config->reset(vdev);
> > + kfree(vpmem);
>
> You allocated vpmem via devm_kzalloc; isn't it freed automatically on
> remove?
>
> > +}
> > +
> > +static struct virtio_driver virtio_pmem_driver = {
> > + .driver.name = KBUILD_MODNAME,
> > + .driver.owner = THIS_MODULE,
> > + .id_table = id_table,
> > + .probe = virtio_pmem_probe,
> > + .remove = virtio_pmem_remove,
> > +};
> > +
> > +module_virtio_driver(virtio_pmem_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> > +MODULE_DESCRIPTION("Virtio pmem driver");
> > +MODULE_LICENSE("GPL");
>
> Only looked at this from the general virtio driver angle; seems fine
> apart from some easy-to-fix issues and some personal style preference
> things.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox