* Re: [PATCH v3 2/3] virtio-balloon: improve update_balloon_size_func
From: Michael S. Tsirkin @ 2019-01-15 1:00 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, kvm, cohuck, linux-kernel, virtualization, pasic,
pbonzini, dgilbert
In-Reply-To: <1546844466-38079-3-git-send-email-wei.w.wang@intel.com>
On Mon, Jan 07, 2019 at 03:01:05PM +0800, Wei Wang wrote:
> There is no need to update the balloon actual register when there is no
> ballooning request. This patch avoids update_balloon_size when diff is 0.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
It sounds reasonable but it does have a side effect of
not writing into actual anymore.
I am not sure actual never can get out of sync as a result.
So better deferred, pls send this in the next merge window.
> ---
> drivers/virtio/virtio_balloon.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index fb12fe2..e33dc8e 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -457,9 +457,12 @@ static void update_balloon_size_func(struct work_struct *work)
> update_balloon_size_work);
> diff = towards_target(vb);
>
> + if (!diff)
> + return;
> +
> if (diff > 0)
> diff -= fill_balloon(vb, diff);
> - else if (diff < 0)
> + else
> diff += leak_balloon(vb, -diff);
> update_balloon_size(vb);
>
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH v3 1/3] virtio-balloon: tweak config_changed implementation
From: Michael S. Tsirkin @ 2019-01-15 0:53 UTC (permalink / raw)
To: Christian Borntraeger
Cc: virtio-dev, kvm, cohuck, linux-kernel, virtualization, pasic,
pbonzini, dgilbert
In-Reply-To: <4ea022d4-6b89-b481-5747-871e154dd21a@de.ibm.com>
On Mon, Jan 07, 2019 at 02:49:03PM +0100, Christian Borntraeger wrote:
>
>
> On 07.01.2019 08:01, Wei Wang wrote:
> > virtio-ccw has deadlock issues with reading the config space inside the
> > interrupt context, so we tweak the virtballoon_changed implementation
> > by moving the config read operations into the related workqueue contexts.
> > The config_read_bitmap is used as a flag to the workqueue callbacks
> > about the related config fields that need to be read.
> >
> > The cmd_id_received is also renamed to cmd_id_received_cache, and
> > the value should be obtained via virtio_balloon_cmd_id_received.
> >
> > Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>
> Together with
> virtio_pci: use queue idx instead of array idx to set up the vq
> virtio: don't allocate vqs when names[i] = NULL
>
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>
> as already said, would be good to add cc stable (and a Fixes: tag)
I don't think you did that.
Did it manually as the pull needed some manual handling
but I won't next time and you will have to send it to
Greg yourself.
>
> > ---
> > drivers/virtio/virtio_balloon.c | 98 +++++++++++++++++++++++++++--------------
> > 1 file changed, 65 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 728ecd1..fb12fe2 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
> > VIRTIO_BALLOON_VQ_MAX
> > };
> >
> > +enum virtio_balloon_config_read {
> > + VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> > +};
> > +
> > struct virtio_balloon {
> > struct virtio_device *vdev;
> > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > @@ -77,14 +81,20 @@ struct virtio_balloon {
> > /* Prevent updating balloon when it is being canceled. */
> > spinlock_t stop_update_lock;
> > bool stop_update;
> > + /* Bitmap to indicate if reading the related config fields are needed */
> > + unsigned long config_read_bitmap;
> >
> > /* The list of allocated free pages, waiting to be given back to mm */
> > struct list_head free_page_list;
> > spinlock_t free_page_list_lock;
> > /* The number of free page blocks on the above list */
> > unsigned long num_free_page_blocks;
> > - /* The cmd id received from host */
> > - u32 cmd_id_received;
> > + /*
> > + * The cmd id received from host.
> > + * Read it via virtio_balloon_cmd_id_received to get the latest value
> > + * sent from host.
> > + */
> > + u32 cmd_id_received_cache;
> > /* The cmd id that is actively in use */
> > __virtio32 cmd_id_active;
> > /* Buffer to store the stop sign */
> > @@ -390,37 +400,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> > return num_returned;
> > }
> >
> > +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> > +{
> > + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> > + return;
> > +
> > + /* No need to queue the work if the bit was already set. */
> > + if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> > + &vb->config_read_bitmap))
> > + return;
> > +
> > + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> > +}
> > +
> > static void virtballoon_changed(struct virtio_device *vdev)
> > {
> > struct virtio_balloon *vb = vdev->priv;
> > unsigned long flags;
> > - s64 diff = towards_target(vb);
> > -
> > - if (diff) {
> > - spin_lock_irqsave(&vb->stop_update_lock, flags);
> > - if (!vb->stop_update)
> > - queue_work(system_freezable_wq,
> > - &vb->update_balloon_size_work);
> > - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> > - }
> >
> > - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> > - virtio_cread(vdev, struct virtio_balloon_config,
> > - free_page_report_cmd_id, &vb->cmd_id_received);
> > - if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> > - /* Pass ULONG_MAX to give back all the free pages */
> > - return_free_pages_to_mm(vb, ULONG_MAX);
> > - } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> > - vb->cmd_id_received !=
> > - virtio32_to_cpu(vdev, vb->cmd_id_active)) {
> > - spin_lock_irqsave(&vb->stop_update_lock, flags);
> > - if (!vb->stop_update) {
> > - queue_work(vb->balloon_wq,
> > - &vb->report_free_page_work);
> > - }
> > - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> > - }
> > + spin_lock_irqsave(&vb->stop_update_lock, flags);
> > + if (!vb->stop_update) {
> > + queue_work(system_freezable_wq,
> > + &vb->update_balloon_size_work);
> > + virtio_balloon_queue_free_page_work(vb);
> > }
> > + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> > }
> >
> > static void update_balloon_size(struct virtio_balloon *vb)
> > @@ -527,6 +531,17 @@ static int init_vqs(struct virtio_balloon *vb)
> > return 0;
> > }
> >
> > +static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
> > +{
> > + if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> > + &vb->config_read_bitmap))
> > + virtio_cread(vb->vdev, struct virtio_balloon_config,
> > + free_page_report_cmd_id,
> > + &vb->cmd_id_received_cache);
> > +
> > + return vb->cmd_id_received_cache;
> > +}
> > +
> > static int send_cmd_id_start(struct virtio_balloon *vb)
> > {
> > struct scatterlist sg;
> > @@ -537,7 +552,8 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
> > while (virtqueue_get_buf(vq, &unused))
> > ;
> >
> > - vb->cmd_id_active = cpu_to_virtio32(vb->vdev, vb->cmd_id_received);
> > + vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
> > + virtio_balloon_cmd_id_received(vb));
> > sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
> > err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL);
> > if (!err)
> > @@ -620,7 +636,8 @@ static int send_free_pages(struct virtio_balloon *vb)
> > * stop the reporting.
> > */
> > cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> > - if (cmd_id_active != vb->cmd_id_received)
> > + if (unlikely(cmd_id_active !=
> > + virtio_balloon_cmd_id_received(vb)))
> > break;
> >
> > /*
> > @@ -637,11 +654,9 @@ static int send_free_pages(struct virtio_balloon *vb)
> > return 0;
> > }
> >
> > -static void report_free_page_func(struct work_struct *work)
> > +static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
> > {
> > int err;
> > - struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> > - report_free_page_work);
> > struct device *dev = &vb->vdev->dev;
> >
> > /* Start by sending the received cmd id to host with an outbuf. */
> > @@ -659,6 +674,23 @@ static void report_free_page_func(struct work_struct *work)
> > dev_err(dev, "Failed to send a stop id, err = %d\n", err);
> > }
> >
> > +static void report_free_page_func(struct work_struct *work)
> > +{
> > + struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> > + report_free_page_work);
> > + u32 cmd_id_received;
> > +
> > + cmd_id_received = virtio_balloon_cmd_id_received(vb);
> > + if (cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> > + /* Pass ULONG_MAX to give back all the free pages */
> > + return_free_pages_to_mm(vb, ULONG_MAX);
> > + } else if (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> > + cmd_id_received !=
> > + virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
> > + virtio_balloon_report_free_page(vb);
> > + }
> > +}
> > +
> > #ifdef CONFIG_BALLOON_COMPACTION
> > /*
> > * virtballoon_migratepage - perform the balloon page migration on behalf of
> > @@ -885,7 +917,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> > goto out_del_vqs;
> > }
> > INIT_WORK(&vb->report_free_page_work, report_free_page_func);
> > - vb->cmd_id_received = VIRTIO_BALLOON_CMD_ID_STOP;
> > + vb->cmd_id_received_cache = VIRTIO_BALLOON_CMD_ID_STOP;
> > vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
> > VIRTIO_BALLOON_CMD_ID_STOP);
> > vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
> >
^ permalink raw reply
* Re: [PATCH v3 0/5] kvm "virtio pmem" device
From: Dave Chinner @ 2019-01-14 22:21 UTC (permalink / raw)
To: Dan Williams
Cc: Pankaj Gupta, Jan Kara, KVM list, linux-nvdimm, Qemu Developers,
virtualization, adilger kernel, Ross Zwisler, Eric Blake,
dave jiang, darrick wong, vishal l verma, Michael S. Tsirkin,
Matthew Wilcox, Christoph Hellwig, Linux ACPI, jmoyer, linux-ext4,
Rik van Riel, Stefan Hajnoczi, Igor Mammedov, lcapitulino,
Nitesh Narayan Lal, Theodore Ts'o, xiaogu
In-Reply-To: <CAPcyv4jtPcLV-s0sKNHwwk0ug7GLBV6699dpm1h3r2xSo879dg@mail.gmail.com>
On Mon, Jan 14, 2019 at 01:35:57PM -0800, Dan Williams wrote:
> On Mon, Jan 14, 2019 at 1:25 PM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote:
> > >
> > > > > Until you have images (and hence host page cache) shared between
> > > > > multiple guests. People will want to do this, because it means they
> > > > > only need a single set of pages in host memory for executable
> > > > > binaries rather than a set of pages per guest. Then you have
> > > > > multiple guests being able to detect residency of the same set of
> > > > > pages. If the guests can then, in any way, control eviction of the
> > > > > pages from the host cache, then we have a guest-to-guest information
> > > > > leak channel.
> > > >
> > > > I don't think we should ever be considering something that would allow a
> > > > guest to evict page's from the host's pagecache [1]. The guest should
> > > > be able to kick its own references to the host's pagecache out of its
> > > > own pagecache, but not be able to influence whether the host or another
> > > > guest has a read-only mapping cached.
> > > >
> > > > [1] Unless the guest is allowed to modify the host's file; obviously
> > > > truncation, holepunching, etc are going to evict pages from the host's
> > > > page cache.
> > >
> > > This is so correct. Guest does not not evict host page cache pages directly.
> >
> > They don't right now.
> >
> > But someone is going to end up asking for discard to work so that
> > the guest can free unused space in the underlying spares image (i.e.
> > make use of fstrim or mount -o discard) because they have workloads
> > that have bursts of space usage and they need to trim the image
> > files afterwards to keep their overall space usage under control.
> >
> > And then....
>
> ...we reject / push back on that patch citing the above concern.
So at what point do we draw the line?
We're allowing writable DAX mappings, but as I've pointed out that
means we are going to be allowing a potential information leak via
files with shared extents to be directly mapped and written to.
But we won't allow useful admin operations that allow better
management of host side storage space similar to how normal image
files are used by guests because it's an information leak vector?
That's splitting some really fine hairs there...
> > > In case of virtio-pmem & DAX, guest clears guest page cache exceptional entries.
> > > Its solely decision of host to take action on the host page cache pages.
> > >
> > > In case of virtio-pmem, guest does not modify host file directly i.e don't
> > > perform hole punch & truncation operation directly on host file.
> >
> > ... this will no longer be true, and the nuclear landmine in this
> > driver interface will have been armed....
>
> I agree with the need to be careful when / if explicit cache control
> is added, but that's not the case today.
"if"?
I expect it to be "when", not if. Expect the worst, plan for it now.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH 1/3] swiotlb: Export maximum allocation size
From: Michael S. Tsirkin @ 2019-01-14 21:59 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Jens Axboe, Joerg Roedel, brijesh.singh, Joerg Roedel, jon.grimm,
jfehlig, linux-kernel, virtualization, linux-block, iommu, hch
In-Reply-To: <20190114204907.GH32038@char.us.oracle.com>
On Mon, Jan 14, 2019 at 03:49:07PM -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 11, 2019 at 10:12:31AM +0100, Joerg Roedel wrote:
> > On Thu, Jan 10, 2019 at 12:02:05PM -0500, Konrad Rzeszutek Wilk wrote:
> > > Why not use swiotlb_nr_tbl ? That is how drivers/gpu/drm use to figure if they
> > > need to limit the size of pages.
> >
> > That function just exports the overall size of the swiotlb aperture, no?
> > What I need here is the maximum size for a single mapping.
>
> Yes. The other drivers just assumed that if there is SWIOTLB they would use
> the smaller size by default (as in they knew the limitation).
>
> But I agree it would be better to have something smarter - and also convert the
> DRM drivers to piggy back on this.
>
> Or alternatively we could make SWIOTLB handle bigger sizes..
Just a thought: is it a good idea to teach blk_queue_max_segment_size
to get the dma size? This will help us find other devices
possibly missing this check.
> >
> > Regards,
> >
> > Joerg
^ permalink raw reply
* Re: [PATCH v3 0/5] kvm "virtio pmem" device
From: Dan Williams @ 2019-01-14 21:35 UTC (permalink / raw)
To: Dave Chinner
Cc: Pankaj Gupta, Jan Kara, KVM list, linux-nvdimm, Qemu Developers,
virtualization, adilger kernel, Ross Zwisler, Eric Blake,
dave jiang, darrick wong, vishal l verma, Michael S. Tsirkin,
Matthew Wilcox, Christoph Hellwig, Linux ACPI, jmoyer, linux-ext4,
Rik van Riel, Stefan Hajnoczi, Igor Mammedov, lcapitulino,
Nitesh Narayan Lal, Theodore Ts'o, xiaogu
In-Reply-To: <20190114212501.GG4205@dastard>
On Mon, Jan 14, 2019 at 1:25 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote:
> >
> > > > Until you have images (and hence host page cache) shared between
> > > > multiple guests. People will want to do this, because it means they
> > > > only need a single set of pages in host memory for executable
> > > > binaries rather than a set of pages per guest. Then you have
> > > > multiple guests being able to detect residency of the same set of
> > > > pages. If the guests can then, in any way, control eviction of the
> > > > pages from the host cache, then we have a guest-to-guest information
> > > > leak channel.
> > >
> > > I don't think we should ever be considering something that would allow a
> > > guest to evict page's from the host's pagecache [1]. The guest should
> > > be able to kick its own references to the host's pagecache out of its
> > > own pagecache, but not be able to influence whether the host or another
> > > guest has a read-only mapping cached.
> > >
> > > [1] Unless the guest is allowed to modify the host's file; obviously
> > > truncation, holepunching, etc are going to evict pages from the host's
> > > page cache.
> >
> > This is so correct. Guest does not not evict host page cache pages directly.
>
> They don't right now.
>
> But someone is going to end up asking for discard to work so that
> the guest can free unused space in the underlying spares image (i.e.
> make use of fstrim or mount -o discard) because they have workloads
> that have bursts of space usage and they need to trim the image
> files afterwards to keep their overall space usage under control.
>
> And then....
...we reject / push back on that patch citing the above concern.
> > In case of virtio-pmem & DAX, guest clears guest page cache exceptional entries.
> > Its solely decision of host to take action on the host page cache pages.
> >
> > In case of virtio-pmem, guest does not modify host file directly i.e don't
> > perform hole punch & truncation operation directly on host file.
>
> ... this will no longer be true, and the nuclear landmine in this
> driver interface will have been armed....
I agree with the need to be careful when / if explicit cache control
is added, but that's not the case today.
^ permalink raw reply
* Re: [PATCH v3 0/5] kvm "virtio pmem" device
From: Dave Chinner @ 2019-01-14 21:25 UTC (permalink / raw)
To: Pankaj Gupta
Cc: jack, kvm, linux-nvdimm, qemu-devel, virtualization,
adilger kernel, zwisler, eblake, dave jiang, darrick wong,
vishal l verma, mst, Matthew Wilcox, hch, linux-acpi, jmoyer,
linux-ext4, riel, stefanha, imammedo, dan j williams, lcapitulino,
nilal, tytso, xiaoguangrong eric, rjw, linux-kernel, linux-xfs,
linux-fsdevel, pbonzini
In-Reply-To: <942065073.64011540.1547450140670.JavaMail.zimbra@redhat.com>
On Mon, Jan 14, 2019 at 02:15:40AM -0500, Pankaj Gupta wrote:
>
> > > Until you have images (and hence host page cache) shared between
> > > multiple guests. People will want to do this, because it means they
> > > only need a single set of pages in host memory for executable
> > > binaries rather than a set of pages per guest. Then you have
> > > multiple guests being able to detect residency of the same set of
> > > pages. If the guests can then, in any way, control eviction of the
> > > pages from the host cache, then we have a guest-to-guest information
> > > leak channel.
> >
> > I don't think we should ever be considering something that would allow a
> > guest to evict page's from the host's pagecache [1]. The guest should
> > be able to kick its own references to the host's pagecache out of its
> > own pagecache, but not be able to influence whether the host or another
> > guest has a read-only mapping cached.
> >
> > [1] Unless the guest is allowed to modify the host's file; obviously
> > truncation, holepunching, etc are going to evict pages from the host's
> > page cache.
>
> This is so correct. Guest does not not evict host page cache pages directly.
They don't right now.
But someone is going to end up asking for discard to work so that
the guest can free unused space in the underlying spares image (i.e.
make use of fstrim or mount -o discard) because they have workloads
that have bursts of space usage and they need to trim the image
files afterwards to keep their overall space usage under control.
And then....
> In case of virtio-pmem & DAX, guest clears guest page cache exceptional entries.
> Its solely decision of host to take action on the host page cache pages.
>
> In case of virtio-pmem, guest does not modify host file directly i.e don't
> perform hole punch & truncation operation directly on host file.
... this will no longer be true, and the nuclear landmine in this
driver interface will have been armed....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH 1/3] swiotlb: Export maximum allocation size
From: Konrad Rzeszutek Wilk @ 2019-01-14 20:49 UTC (permalink / raw)
To: Joerg Roedel
Cc: Jens Axboe, Joerg Roedel, brijesh.singh, Michael S . Tsirkin,
jon.grimm, jfehlig, linux-kernel, virtualization, linux-block,
iommu, hch
In-Reply-To: <20190111091231.GB5825@8bytes.org>
On Fri, Jan 11, 2019 at 10:12:31AM +0100, Joerg Roedel wrote:
> On Thu, Jan 10, 2019 at 12:02:05PM -0500, Konrad Rzeszutek Wilk wrote:
> > Why not use swiotlb_nr_tbl ? That is how drivers/gpu/drm use to figure if they
> > need to limit the size of pages.
>
> That function just exports the overall size of the swiotlb aperture, no?
> What I need here is the maximum size for a single mapping.
Yes. The other drivers just assumed that if there is SWIOTLB they would use
the smaller size by default (as in they knew the limitation).
But I agree it would be better to have something smarter - and also convert the
DRM drivers to piggy back on this.
Or alternatively we could make SWIOTLB handle bigger sizes..
>
> Regards,
>
> Joerg
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Michael S. Tsirkin @ 2019-01-14 20:48 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, brijesh.singh, Konrad Rzeszutek Wilk, jon.grimm,
jfehlig, linux-kernel, virtualization, linux-block, iommu,
Joerg Roedel
In-Reply-To: <20190114201935.GA10781@lst.de>
On Mon, Jan 14, 2019 at 09:19:35PM +0100, Christoph Hellwig wrote:
> > Christoph is saying that !IOMMU_PLATFORM devices should hide the
> > compatibility code in a special per-device DMA API implementation.
> > Which would be fine especially if we can manage not to introduce a bunch
> > of indirect calls all over the place and hurt performance. It's just
> > that the benefit is unlikely to be big (e.g. we can't also get rid of
> > the virtio specific memory barriers) so no one was motivated enough to
> > work on it.
>
> No.
Oh ok then.
> The problem is that we still haven't fully specified what
> IOMMU_PLATFORM and !IOMMU_PLATFORM actually mean. Your
> "ACCESS_PLATFORM/ORDER_PLATFORM" commit in the virtio-spec repo
> improves it a little bit, but it is still far from enough.
>
> As a start VIRTIO_F_ORDER_PLATFORM and VIRTIO_F_ACCESS_PLATFORM
> absolutely MUST be set for hardware implementations. Otherwise said
> hardware has no chance of working on anything but the most x86-like
> systems.
I think you are exaggerating a little here. Limited access with e.g.
need to flush caches is common on lower end but less common on higher
end systems used for virtualization. As you point out that changes with
e.g. SEV but then SEV is a pretty niche thing so far.
So I would make that a SHOULD probably.
> Second software implementations SHOULD set VIRTIO_F_ACCESS_PLATFORM,
> because otherwise we can't add proper handling for things like SEV or
> the IBM "secure hypervisor" thing.
Yes. If host does not have access to all of memory it SHOULD set
VIRTIO_F_ACCESS_PLATFORM.
It seems rather straight-forward to add conformance clauses
for this, thanks for reminding me.
> Last but not least a lot of wording outside the area describing these
> flags really needs some major updates in terms of DMA access.
Thanks for the reminder. Yes generally spec tends to still say e.g.
"physical address" where it really means a "DMA address" in Linux terms.
Whether changing that will make things much clearer I'm not sure. E.g.
hardware designers don't much care I guess.
If you want to list the most problematic cases, e.g. in a github issue,
we might get to clarifying them sooner.
Thanks!
--
MST
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Michael S. Tsirkin @ 2019-01-14 20:29 UTC (permalink / raw)
To: Robin Murphy
Cc: Jens Axboe, brijesh.singh, Konrad Rzeszutek Wilk, jon.grimm,
linux-kernel, iommu, virtualization, linux-block, jfehlig,
Christoph Hellwig
In-Reply-To: <cb290df3-4e17-a403-6d69-cf5a8a41c987@arm.com>
On Mon, Jan 14, 2019 at 07:12:08PM +0000, Robin Murphy wrote:
> On 14/01/2019 18:20, Michael S. Tsirkin wrote:
> > On Mon, Jan 14, 2019 at 08:41:37PM +0800, Jason Wang wrote:
> > >
> > > On 2019/1/14 下午5:50, Christoph Hellwig wrote:
> > > > On Mon, Jan 14, 2019 at 05:41:56PM +0800, Jason Wang wrote:
> > > > > On 2019/1/11 下午5:15, Joerg Roedel wrote:
> > > > > > On Fri, Jan 11, 2019 at 11:29:31AM +0800, Jason Wang wrote:
> > > > > > > Just wonder if my understanding is correct IOMMU_PLATFORM must be set for
> > > > > > > all virtio devices under AMD-SEV guests?
> > > > > > Yes, that is correct. Emulated DMA can only happen on the SWIOTLB
> > > > > > aperture, because that memory is not encrypted. The guest bounces the
> > > > > > data then to its encrypted memory.
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Joerg
> > > > >
> > > > > Thanks, have you tested vhost-net in this case. I suspect it may not work
> > > > Which brings me back to my pet pevee that we need to take actions
> > > > that virtio uses the proper dma mapping API by default with quirks
> > > > for legacy cases. The magic bypass it uses is just causing problems
> > > > over problems.
> > >
> > >
> > > Yes, I fully agree with you. This is probably an exact example of such
> > > problem.
> > >
> > > Thanks
> >
> > I don't think so - the issue is really that DMA API does not yet handle
> > the SEV case 100% correctly. I suspect passthrough devices would have
> > the same issue.
>
> Huh? Regardless of which virtio devices use it or not, the DMA API is
> handling the SEV case as correctly as it possibly can, by forcing everything
> through the unencrypted bounce buffer. If the segments being mapped are too
> big for that bounce buffer in the first place, there's nothing it can
> possibly do except fail, gracefully or otherwise.
It seems reasonable to be able to ask it what it's capabilities are
though.
> Now, in theory, yes, the real issue at hand is not unique to virtio-blk nor
> SEV - any driver whose device has a sufficiently large DMA segment size and
> who manages to get sufficient physically-contiguous memory could technically
> generate a scatterlist segment longer than SWIOTLB can handle. However, in
> practice that basically never happens, not least because very few drivers
> ever override the default 64K DMA segment limit. AFAICS nothing in
> drivers/virtio is calling dma_set_max_seg_size() or otherwise assigning any
> dma_parms to replace the defaults either, so the really interesting question
> here is how are these apparently-out-of-spec 256K segments getting generated
> at all?
>
> Robin.
I guess this is what you are looking for:
/* Host can optionally specify maximum segment size and number of
* segments. */
err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SIZE_MAX,
struct virtio_blk_config, size_max, &v);
if (!err)
blk_queue_max_segment_size(q, v);
else
blk_queue_max_segment_size(q, -1U);
virtio isn't the only caller with a value >64K:
$ git grep -A1 blk_queue_max_segment_size
Documentation/block/biodoc.txt: blk_queue_max_segment_size(q, max_seg_size)
Documentation/block/biodoc.txt- Maximum size of a clustered segment, 64kB default.
--
block/blk-settings.c: * blk_queue_max_segment_size - set max segment size for blk_rq_map_sg
block/blk-settings.c- * @q: the request queue for the device
--
block/blk-settings.c:void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
block/blk-settings.c-{
--
block/blk-settings.c:EXPORT_SYMBOL(blk_queue_max_segment_size);
block/blk-settings.c-
--
drivers/block/mtip32xx/mtip32xx.c: blk_queue_max_segment_size(dd->queue, 0x400000);
drivers/block/mtip32xx/mtip32xx.c- blk_queue_io_min(dd->queue, 4096);
--
drivers/block/nbd.c: blk_queue_max_segment_size(disk->queue, UINT_MAX);
drivers/block/nbd.c- blk_queue_max_segments(disk->queue, USHRT_MAX);
--
drivers/block/ps3disk.c: blk_queue_max_segment_size(queue, dev->bounce_size);
drivers/block/ps3disk.c-
--
drivers/block/ps3vram.c: blk_queue_max_segment_size(queue, BLK_MAX_SEGMENT_SIZE);
drivers/block/ps3vram.c- blk_queue_max_hw_sectors(queue, BLK_SAFE_MAX_SECTORS);
--
drivers/block/rbd.c: blk_queue_max_segment_size(q, UINT_MAX);
drivers/block/rbd.c- blk_queue_io_min(q, objset_bytes);
--
drivers/block/sunvdc.c: blk_queue_max_segment_size(q, PAGE_SIZE);
drivers/block/sunvdc.c-
--
drivers/block/virtio_blk.c: blk_queue_max_segment_size(q, v);
drivers/block/virtio_blk.c- else
drivers/block/virtio_blk.c: blk_queue_max_segment_size(q, -1U);
drivers/block/virtio_blk.c-
--
drivers/block/xen-blkfront.c: blk_queue_max_segment_size(rq, PAGE_SIZE);
drivers/block/xen-blkfront.c-
--
drivers/cdrom/gdrom.c: blk_queue_max_segment_size(gd.gdrom_rq, 0x40000);
drivers/cdrom/gdrom.c- gd.disk->queue = gd.gdrom_rq;
--
drivers/memstick/core/ms_block.c: blk_queue_max_segment_size(msb->queue,
drivers/memstick/core/ms_block.c- MS_BLOCK_MAX_PAGES * msb->page_size);
--
drivers/memstick/core/mspro_block.c: blk_queue_max_segment_size(msb->queue,
drivers/memstick/core/mspro_block.c- MSPRO_BLOCK_MAX_PAGES * msb->page_size);
--
drivers/mmc/core/queue.c: blk_queue_max_segment_size(mq->queue, host->max_seg_size);
drivers/mmc/core/queue.c-
--
drivers/s390/block/dasd.c: blk_queue_max_segment_size(q, PAGE_SIZE);
drivers/s390/block/dasd.c- blk_queue_segment_boundary(q, PAGE_SIZE - 1);
--
drivers/scsi/be2iscsi/be_main.c: blk_queue_max_segment_size(sdev->request_queue, 65536);
drivers/scsi/be2iscsi/be_main.c- return 0;
--
drivers/scsi/scsi_debug.c: blk_queue_max_segment_size(sdp->request_queue, -1U);
drivers/scsi/scsi_debug.c- if (sdebug_no_uld)
--
drivers/scsi/scsi_lib.c: blk_queue_max_segment_size(q, dma_get_max_seg_size(dev));
drivers/scsi/scsi_lib.c-
--
drivers/scsi/ufs/ufshcd.c: blk_queue_max_segment_size(q, PRDT_DATA_BYTE_COUNT_MAX);
drivers/scsi/ufs/ufshcd.c-
--
include/linux/blkdev.h:extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
include/linux/blkdev.h-extern void blk_queue_max_discard_sectors(struct request_queue *q,
--
include/linux/mmc/host.h: unsigned int max_seg_size; /* see blk_queue_max_segment_size */
include/linux/mmc/host.h- unsigned short max_segs; /* see blk_queue_max_segments */
Some of these devices are probably not going to work well if
passed through to a SEV guest.
Going back to virtio, at some level virtio is like a stacking device so
it does not necessarily need a limit.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Christoph Hellwig @ 2019-01-14 20:22 UTC (permalink / raw)
To: Robin Murphy
Cc: Jens Axboe, brijesh.singh, Konrad Rzeszutek Wilk, jon.grimm,
linux-kernel, iommu, virtualization, linux-block, jfehlig,
Michael S. Tsirkin, Christoph Hellwig
In-Reply-To: <cb290df3-4e17-a403-6d69-cf5a8a41c987@arm.com>
On Mon, Jan 14, 2019 at 07:12:08PM +0000, Robin Murphy wrote:
> Now, in theory, yes, the real issue at hand is not unique to virtio-blk nor
> SEV - any driver whose device has a sufficiently large DMA segment size and
> who manages to get sufficient physically-contiguous memory could
> technically generate a scatterlist segment longer than SWIOTLB can handle.
> However, in practice that basically never happens, not least because very
> few drivers ever override the default 64K DMA segment limit. AFAICS nothing
> in drivers/virtio is calling dma_set_max_seg_size() or otherwise assigning
> any dma_parms to replace the defaults either, so the really interesting
> question here is how are these apparently-out-of-spec 256K segments getting
> generated at all?
drivers/block/virtio_blk.c:virtblk_probe():
/* Host can optionally specify maximum segment size and number of
* segments. */
err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SIZE_MAX,
struct virtio_blk_config, size_max, &v);
if (!err)
blk_queue_max_segment_size(q, v);
else
blk_queue_max_segment_size(q, -1U);
So it really is virtio_blk that is special here. The host could
set VIRTIO_BLK_F_SIZE_MAX to paper over the problem, but I really
think we need a dma_max_segment_size API that is wired up through
the dma_map_ops to sort this out for real.
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Christoph Hellwig @ 2019-01-14 20:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, brijesh.singh, Konrad Rzeszutek Wilk, jon.grimm,
jfehlig, linux-kernel, virtualization, linux-block, iommu,
Christoph Hellwig, Joerg Roedel
In-Reply-To: <20190114131114-mutt-send-email-mst@kernel.org>
On Mon, Jan 14, 2019 at 01:20:45PM -0500, Michael S. Tsirkin wrote:
> I don't think so - the issue is really that DMA API does not yet handle
> the SEV case 100% correctly. I suspect passthrough devices would have
> the same issue.
The DMA API handles the SEV case perfectly. Its just that virtio_blk
supports huge segments that virtio does not generally support, but that
is not related to SEV in any way.
> In fact whoever sets IOMMU_PLATFORM is completely unaffected by
> Christoph's pet peeve.
No, the above happens only when we set IOMMU_PLATFORM.
> Christoph is saying that !IOMMU_PLATFORM devices should hide the
> compatibility code in a special per-device DMA API implementation.
> Which would be fine especially if we can manage not to introduce a bunch
> of indirect calls all over the place and hurt performance. It's just
> that the benefit is unlikely to be big (e.g. we can't also get rid of
> the virtio specific memory barriers) so no one was motivated enough to
> work on it.
No. The problem is that we still haven't fully specified what
IOMMU_PLATFORM and !IOMMU_PLATFORM actually mean. Your
"ACCESS_PLATFORM/ORDER_PLATFORM" commit in the virtio-spec repo
improves it a little bit, but it is still far from enough.
As a start VIRTIO_F_ORDER_PLATFORM and VIRTIO_F_ACCESS_PLATFORM
absolutely MUST be set for hardware implementations. Otherwise said
hardware has no chance of working on anything but the most x86-like
systems.
Second software implementations SHOULD set VIRTIO_F_ACCESS_PLATFORM,
because otherwise we can't add proper handling for things like SEV or
the IBM "secure hypervisor" thing.
Last but not least a lot of wording outside the area describing these
flags really needs some major updates in terms of DMA access.
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Robin Murphy @ 2019-01-14 19:12 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang
Cc: Jens Axboe, brijesh.singh, Konrad Rzeszutek Wilk, jon.grimm,
linux-kernel, iommu, virtualization, linux-block, jfehlig,
Christoph Hellwig
In-Reply-To: <20190114131114-mutt-send-email-mst@kernel.org>
On 14/01/2019 18:20, Michael S. Tsirkin wrote:
> On Mon, Jan 14, 2019 at 08:41:37PM +0800, Jason Wang wrote:
>>
>> On 2019/1/14 下午5:50, Christoph Hellwig wrote:
>>> On Mon, Jan 14, 2019 at 05:41:56PM +0800, Jason Wang wrote:
>>>> On 2019/1/11 下午5:15, Joerg Roedel wrote:
>>>>> On Fri, Jan 11, 2019 at 11:29:31AM +0800, Jason Wang wrote:
>>>>>> Just wonder if my understanding is correct IOMMU_PLATFORM must be set for
>>>>>> all virtio devices under AMD-SEV guests?
>>>>> Yes, that is correct. Emulated DMA can only happen on the SWIOTLB
>>>>> aperture, because that memory is not encrypted. The guest bounces the
>>>>> data then to its encrypted memory.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Joerg
>>>>
>>>> Thanks, have you tested vhost-net in this case. I suspect it may not work
>>> Which brings me back to my pet pevee that we need to take actions
>>> that virtio uses the proper dma mapping API by default with quirks
>>> for legacy cases. The magic bypass it uses is just causing problems
>>> over problems.
>>
>>
>> Yes, I fully agree with you. This is probably an exact example of such
>> problem.
>>
>> Thanks
>
> I don't think so - the issue is really that DMA API does not yet handle
> the SEV case 100% correctly. I suspect passthrough devices would have
> the same issue.
Huh? Regardless of which virtio devices use it or not, the DMA API is
handling the SEV case as correctly as it possibly can, by forcing
everything through the unencrypted bounce buffer. If the segments being
mapped are too big for that bounce buffer in the first place, there's
nothing it can possibly do except fail, gracefully or otherwise.
Now, in theory, yes, the real issue at hand is not unique to virtio-blk
nor SEV - any driver whose device has a sufficiently large DMA segment
size and who manages to get sufficient physically-contiguous memory
could technically generate a scatterlist segment longer than SWIOTLB can
handle. However, in practice that basically never happens, not least
because very few drivers ever override the default 64K DMA segment
limit. AFAICS nothing in drivers/virtio is calling
dma_set_max_seg_size() or otherwise assigning any dma_parms to replace
the defaults either, so the really interesting question here is how are
these apparently-out-of-spec 256K segments getting generated at all?
Robin.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Michael S. Tsirkin @ 2019-01-14 18:20 UTC (permalink / raw)
To: Jason Wang
Cc: Jens Axboe, brijesh.singh, Konrad Rzeszutek Wilk, Joerg Roedel,
jon.grimm, jfehlig, linux-kernel, virtualization, linux-block,
iommu, Christoph Hellwig
In-Reply-To: <ac6d83d8-02de-dbdf-e309-3a1115642d50@redhat.com>
On Mon, Jan 14, 2019 at 08:41:37PM +0800, Jason Wang wrote:
>
> On 2019/1/14 下午5:50, Christoph Hellwig wrote:
> > On Mon, Jan 14, 2019 at 05:41:56PM +0800, Jason Wang wrote:
> > > On 2019/1/11 下午5:15, Joerg Roedel wrote:
> > > > On Fri, Jan 11, 2019 at 11:29:31AM +0800, Jason Wang wrote:
> > > > > Just wonder if my understanding is correct IOMMU_PLATFORM must be set for
> > > > > all virtio devices under AMD-SEV guests?
> > > > Yes, that is correct. Emulated DMA can only happen on the SWIOTLB
> > > > aperture, because that memory is not encrypted. The guest bounces the
> > > > data then to its encrypted memory.
> > > >
> > > > Regards,
> > > >
> > > > Joerg
> > >
> > > Thanks, have you tested vhost-net in this case. I suspect it may not work
> > Which brings me back to my pet pevee that we need to take actions
> > that virtio uses the proper dma mapping API by default with quirks
> > for legacy cases. The magic bypass it uses is just causing problems
> > over problems.
>
>
> Yes, I fully agree with you. This is probably an exact example of such
> problem.
>
> Thanks
I don't think so - the issue is really that DMA API does not yet handle
the SEV case 100% correctly. I suspect passthrough devices would have
the same issue.
In fact whoever sets IOMMU_PLATFORM is completely unaffected by
Christoph's pet peeve.
Christoph is saying that !IOMMU_PLATFORM devices should hide the
compatibility code in a special per-device DMA API implementation.
Which would be fine especially if we can manage not to introduce a bunch
of indirect calls all over the place and hurt performance. It's just
that the benefit is unlikely to be big (e.g. we can't also get rid of
the virtio specific memory barriers) so no one was motivated enough to
work on it.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net V3] vhost: log dirty page correctly
From: Michael S. Tsirkin @ 2019-01-14 18:04 UTC (permalink / raw)
To: Jason Wang; +Cc: Jintack Lim, netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20190111040036.12413-1-jasowang@redhat.com>
On Fri, Jan 11, 2019 at 12:00:36PM +0800, Jason Wang wrote:
> Vhost dirty page logging API is designed to sync through GPA. But we
> try to log GIOVA when device IOTLB is enabled. This is wrong and may
> lead to missing data after migration.
>
> To solve this issue, when logging with device IOTLB enabled, we will:
>
> 1) reuse the device IOTLB translation result of GIOVA->HVA mapping to
> get HVA, for writable descriptor, get HVA through iovec. For used
> ring update, translate its GIOVA to HVA
> 2) traverse the GPA->HVA mapping to get the possible GPA and log
> through GPA. Pay attention this reverse mapping is not guaranteed
> to be unique, so we should log each possible GPA in this case.
>
> This fix the failure of scp to guest during migration. In -next, we
> will probably support passing GIOVA->GPA instead of GIOVA->HVA.
>
> Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
> Reported-by: Jintack Lim <jintack@cs.columbia.edu>
> Cc: Jintack Lim <jintack@cs.columbia.edu>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V2:
> - check and log the case of range overlap
> - remove unnecessary u64 cast
> - use smp_wmb() for the case of device IOTLB as well
> Changes from V1:
> - return error instead of warn
> ---
> drivers/vhost/net.c | 3 +-
> drivers/vhost/vhost.c | 88 ++++++++++++++++++++++++++++++++++++-------
> drivers/vhost/vhost.h | 3 +-
> 3 files changed, 78 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 36f3d0f49e60..bca86bf7189f 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1236,7 +1236,8 @@ static void handle_rx(struct vhost_net *net)
> if (nvq->done_idx > VHOST_NET_BATCH)
> vhost_net_signal_used(nvq);
> if (unlikely(vq_log))
> - vhost_log_write(vq, vq_log, log, vhost_len);
> + vhost_log_write(vq, vq_log, log, vhost_len,
> + vq->iov, in);
> total_len += vhost_len;
> if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
> vhost_poll_queue(&vq->poll);
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 9f7942cbcbb2..55a2e8f9f8ca 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1733,13 +1733,78 @@ static int log_write(void __user *log_base,
> return r;
> }
>
> +static int log_write_hva(struct vhost_virtqueue *vq, u64 hva, u64 len)
> +{
> + struct vhost_umem *umem = vq->umem;
> + struct vhost_umem_node *u;
> + u64 start, end;
> + int r;
> + bool hit = false;
> +
> + /* More than one GPAs can be mapped into a single HVA. So
> + * iterate all possible umems here to be safe.
> + */
> + list_for_each_entry(u, &umem->umem_list, link) {
> + if (u->userspace_addr > hva - 1 + len ||
> + u->userspace_addr - 1 + u->size < hva)
> + continue;
> + start = max(u->userspace_addr, hva);
> + end = min(u->userspace_addr - 1 + u->size, hva - 1 + len);
> + r = log_write(vq->log_base,
> + u->start + start - u->userspace_addr,
> + end - start + 1);
> + if (r < 0)
> + return r;
> + hit = true;
> + }
> +
> + if (!hit)
> + return -EFAULT;
> +
> + return 0;
> +}
> +
I definitely like the simplicity.
But there's one point left here: if len crosses a region boundary,
but doesn't all fall within a region, we don't consistently report -EFAULT.
So I suspect we need to start by finding a region that contains hva.
If there are many of these - move right to the end of the
leftmost one and then repeat until we run out of len
or fail to find a region and exit with -EFAULT.
> +static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
> +{
> + struct iovec iov[64];
> + int i, ret;
> +
> + if (!vq->iotlb)
> + return log_write(vq->log_base, vq->log_addr + used_offset, len);
> +
> + ret = translate_desc(vq, (uintptr_t)vq->used + used_offset,
> + len, iov, 64, VHOST_ACCESS_WO);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < ret; i++) {
> + ret = log_write_hva(vq, (uintptr_t)iov[i].iov_base,
> + iov[i].iov_len);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> - unsigned int log_num, u64 len)
> + unsigned int log_num, u64 len, struct iovec *iov, int count)
> {
> int i, r;
>
> /* Make sure data written is seen before log. */
> smp_wmb();
> +
> + if (vq->iotlb) {
> + for (i = 0; i < count; i++) {
> + r = log_write_hva(vq, (uintptr_t)iov[i].iov_base,
> + iov[i].iov_len);
> + if (r < 0)
> + return r;
> + }
> + return 0;
> + }
> +
> for (i = 0; i < log_num; ++i) {
> u64 l = min(log[i].len, len);
> r = log_write(vq->log_base, log[i].addr, l);
> @@ -1769,9 +1834,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
> smp_wmb();
> /* Log used flag write. */
> used = &vq->used->flags;
> - log_write(vq->log_base, vq->log_addr +
> - (used - (void __user *)vq->used),
> - sizeof vq->used->flags);
> + log_used(vq, (used - (void __user *)vq->used),
> + sizeof vq->used->flags);
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> @@ -1789,9 +1853,8 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
> smp_wmb();
> /* Log avail event write */
> used = vhost_avail_event(vq);
> - log_write(vq->log_base, vq->log_addr +
> - (used - (void __user *)vq->used),
> - sizeof *vhost_avail_event(vq));
> + log_used(vq, (used - (void __user *)vq->used),
> + sizeof *vhost_avail_event(vq));
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> @@ -2191,10 +2254,8 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
> /* Make sure data is seen before log. */
> smp_wmb();
> /* Log used ring entry write. */
> - log_write(vq->log_base,
> - vq->log_addr +
> - ((void __user *)used - (void __user *)vq->used),
> - count * sizeof *used);
> + log_used(vq, ((void __user *)used - (void __user *)vq->used),
> + count * sizeof *used);
> }
> old = vq->last_used_idx;
> new = (vq->last_used_idx += count);
> @@ -2236,9 +2297,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
> /* Make sure used idx is seen before log. */
> smp_wmb();
> /* Log used index update. */
> - log_write(vq->log_base,
> - vq->log_addr + offsetof(struct vring_used, idx),
> - sizeof vq->used->idx);
> + log_used(vq, offsetof(struct vring_used, idx),
> + sizeof vq->used->idx);
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 466ef7542291..1b675dad5e05 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
> bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
>
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> - unsigned int log_num, u64 len);
> + unsigned int log_num, u64 len,
> + struct iovec *iov, int count);
> int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
>
> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
> --
> 2.17.1
^ permalink raw reply
* Re: [PATCH v3 2/5] virtio-pmem: Add virtio pmem driver
From: Michael S. Tsirkin @ 2019-01-14 15:54 UTC (permalink / raw)
To: Pankaj Gupta
Cc: jack, kvm, linux-nvdimm, qemu-devel, virtualization,
adilger.kernel, zwisler, eblake, dave.jiang, darrick.wong,
vishal.l.verma, willy, hch, linux-acpi, jmoyer, nilal, riel,
stefanha, imammedo, dan.j.williams, lcapitulino, linux-ext4,
tytso, xiaoguangrong.eric, rjw, linux-kernel, linux-xfs,
linux-fsdevel, pbonzini
In-Reply-To: <20190109144736.17452-3-pagupta@redhat.com>
On Wed, Jan 09, 2019 at 08:17:33PM +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 | 84 ++++++++++++++++++++++++++
> 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 ++++
As with any uapi change, you need to CC the virtio dev
mailing list (subscribers only, sorry about that).
> 7 files changed, 290 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 0000000..2a1b1ba
> --- /dev/null
> +++ b/drivers/nvdimm/virtio_pmem.c
> @@ -0,0 +1,84 @@
> +// 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);
> + }
> + virtqueue_kick(vpmem->req_vq);
> + spin_unlock_irqrestore(&vpmem->pmem_lock, flags);
> +
> + /* When host has read buffer, this completes via host_ack */
> + wait_event(req->host_acked, req->done);
> + err = req->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 3589764..9f634a2 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 3a2b5c5..143ce91 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 0000000..51f5349
> --- /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);
> + memset(&ndr_desc, 0, sizeof(ndr_desc));
> +
> + ndr_desc.res = &res;
> + ndr_desc.numa_node = nid;
> + ndr_desc.flush = virtio_pmem_flush;
> + set_bit(ND_REGION_PAGEMAP, &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;
> +
> + //virtio_device_ready(vdev);
> + 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);
> + 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 0000000..224f9d9
> --- /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 6d5c3b2..3463895 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 */
> diff --git a/include/uapi/linux/virtio_pmem.h b/include/uapi/linux/virtio_pmem.h
> new file mode 100644
> index 0000000..fa3f7d5
> --- /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.9.3
^ permalink raw reply
* RE: [PATCH] VMCI: Verify PPNs before sending to device
From: Jorgen S. Hansen via Virtualization @ 2019-01-14 14:47 UTC (permalink / raw)
To: Jorgen S. Hansen, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
Cc: Pv-drivers, gregkh@linuxfoundation.org
In-Reply-To: <1547225865-53408-1-git-send-email-jhansen@vmware.com>
> -----Original Message-----
> From: Jorgen Hansen [mailto:jhansen@vmware.com]
> Sent: Friday, January 11, 2019 8:58 AM
> To: linux-kernel@vger.kernel.org; virtualization@lists.linux-foundation.org
> Cc: gregkh@linuxfoundation.org; Pv-drivers <Pv-drivers@vmware.com>;
> Jorgen S. Hansen <jhansen@vmware.com>
> Subject: [PATCH] VMCI: Verify PPNs before sending to device
>
> The current version of the VMCI device only supports 32 bit PPNs, so check
> whether we are truncating PPNs, and fail the operation if we do. One such
> check did exist, but was wrong. Another check was missing.
>
> Testing through code modification: constructed PPN not representable by
> 32-bit and observed that failure was reported.
>
> Fixes: 1f166439917b ("VMCI: guest side driver implementation.")
> Fixes: 06164d2b72aa ("VMCI: queue pairs implementation.")
>
> Signed-off-by: Jorgen Hansen <jhansen@vmware.com>
> Reviewed-by: Adit Ranadive <aditr@vmware.com>
> Reviewed-by: Vishnu Dasa <vdasa@vmware.com>
> ---
> drivers/misc/vmw_vmci/vmci_guest.c | 10 +++++++---
> drivers/misc/vmw_vmci/vmci_queue_pair.c | 10 ++++------
> 2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c
> b/drivers/misc/vmw_vmci/vmci_guest.c
> index dad5abee656e..02bb3866cf9e 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -532,10 +532,14 @@ static int vmci_guest_probe_device(struct pci_dev
> *pdev,
> if (capabilities & VMCI_CAPS_NOTIFICATIONS) {
> unsigned long bitmap_ppn =
> vmci_dev->notification_base >> PAGE_SHIFT;
> - if (!vmci_dbell_register_notification_bitmap(bitmap_ppn)) {
> + u32 bitmap_ppn32 = bitmap_ppn;
> +
> + if ((sizeof(bitmap_ppn) > sizeof(bitmap_ppn32)
> + && bitmap_ppn != bitmap_ppn32) ||
> + !vmci_dbell_register_notification_bitmap(bitmap_ppn)) {
> dev_warn(&pdev->dev,
> - "VMCI device unable to register notification
> bitmap with PPN 0x%x\n",
> - (u32) bitmap_ppn);
> + "VMCI device unable to register notification
> bitmap with PPN 0x%lx\n",
> + bitmap_ppn);
> error = -ENXIO;
> goto err_remove_vmci_dev_g;
> }
> diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c
> b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> index 264f4ed8eef2..1da4f6cb01b2 100644
> --- a/drivers/misc/vmw_vmci/vmci_queue_pair.c
> +++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> @@ -465,9 +465,8 @@ static int qp_alloc_ppn_set(void *prod_q,
> for (i = 0; i < num_produce_pages; i++) {
> unsigned long pfn;
>
> - produce_ppns[i] =
> - produce_q->kernel_if->u.g.pas[i] >> PAGE_SHIFT;
> - pfn = produce_ppns[i];
> + pfn = produce_q->kernel_if->u.g.pas[i] >> PAGE_SHIFT;
> + produce_ppns[i] = pfn;
>
> /* Fail allocation if PFN isn't supported by hypervisor. */
> if (sizeof(pfn) > sizeof(*produce_ppns) @@ -478,9 +477,8
> @@ static int qp_alloc_ppn_set(void *prod_q,
> for (i = 0; i < num_consume_pages; i++) {
> unsigned long pfn;
>
> - consume_ppns[i] =
> - consume_q->kernel_if->u.g.pas[i] >> PAGE_SHIFT;
> - pfn = consume_ppns[i];
> + pfn = consume_q->kernel_if->u.g.pas[i] >> PAGE_SHIFT;
> + consume_ppns[i] = pfn;
>
> /* Fail allocation if PFN isn't supported by hypervisor. */
> if (sizeof(pfn) > sizeof(*consume_ppns)
> --
> 2.17.1
Please ignore this patch.
Thomas Hellstrom reminded me that the dma mask for the device should
be able to take care of this and since the VMCI device uses the default
DMA mask, the device should only get 32-bit addresses. So we should
Just remove the (wrong) overflow checks in vmci_queue_pair.c
Another patch supporting larger addresses will be sent out later.
Thanks,
Jorgen
^ permalink raw reply
* Re: [PATCH 1/2] virtio: fix virtio_config_ops description
From: Cornelia Huck @ 2019-01-14 14:09 UTC (permalink / raw)
To: Michael S . Tsirkin, Jason Wang
Cc: Halil Pasic, virtio-dev, linux-kernel, virtualization
In-Reply-To: <20190103160804.21438-2-cohuck@redhat.com>
On Thu, 3 Jan 2019 17:08:03 +0100
Cornelia Huck <cohuck@redhat.com> wrote:
> - get_features has returned 64 bits since commit d025477368792
> ("virtio: add support for 64 bit features.")
> - properly mark all optional callbacks
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> include/linux/virtio_config.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 32baf8e26735..7087ef946ba7 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -22,7 +22,7 @@ struct irq_affinity;
> * offset: the offset of the configuration field
> * buf: the buffer to read the field value from.
> * len: the length of the buffer
> - * @generation: config generation counter
> + * @generation: config generation counter (optional)
> * vdev: the virtio_device
> * Returns the config generation counter
> * @get_status: read the status byte
> @@ -48,17 +48,17 @@ struct irq_affinity;
> * @del_vqs: free virtqueues found by find_vqs().
> * @get_features: get the array of feature bits for this device.
> * vdev: the virtio_device
> - * Returns the first 32 feature bits (all we currently need).
> + * Returns the first 64 feature bits (all we currently need).
> * @finalize_features: confirm what device features we'll be using.
> * vdev: the virtio_device
> * This gives the final feature bits for the device: it can change
> * the dev->feature bits if it wants.
> * Returns 0 on success or error status
> - * @bus_name: return the bus name associated with the device
> + * @bus_name: return the bus name associated with the device (optional)
> * vdev: the virtio_device
> * This returns a pointer to the bus name a la pci_name from which
> * the caller can then copy.
> - * @set_vq_affinity: set the affinity for a virtqueue.
> + * @set_vq_affinity: set the affinity for a virtqueue (optional).
> * @get_vq_affinity: get the affinity for a virtqueue (optional).
> */
> typedef void vq_callback_t(struct virtqueue *);
Ping. Any feedback on that patch?
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Jason Wang @ 2019-01-14 12:41 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, jon.grimm, brijesh.singh, Konrad Rzeszutek Wilk,
Joerg Roedel, Michael S . Tsirkin, jfehlig, linux-kernel,
virtualization, linux-block, iommu
In-Reply-To: <20190114095002.GA29874@lst.de>
On 2019/1/14 下午5:50, Christoph Hellwig wrote:
> On Mon, Jan 14, 2019 at 05:41:56PM +0800, Jason Wang wrote:
>> On 2019/1/11 下午5:15, Joerg Roedel wrote:
>>> On Fri, Jan 11, 2019 at 11:29:31AM +0800, Jason Wang wrote:
>>>> Just wonder if my understanding is correct IOMMU_PLATFORM must be set for
>>>> all virtio devices under AMD-SEV guests?
>>> Yes, that is correct. Emulated DMA can only happen on the SWIOTLB
>>> aperture, because that memory is not encrypted. The guest bounces the
>>> data then to its encrypted memory.
>>>
>>> Regards,
>>>
>>> Joerg
>>
>> Thanks, have you tested vhost-net in this case. I suspect it may not work
> Which brings me back to my pet pevee that we need to take actions
> that virtio uses the proper dma mapping API by default with quirks
> for legacy cases. The magic bypass it uses is just causing problems
> over problems.
Yes, I fully agree with you. This is probably an exact example of such
problem.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v3 0/5] kvm "virtio pmem" device
From: Pankaj Gupta @ 2019-01-14 10:16 UTC (permalink / raw)
To: Jan Kara
Cc: KVM list, Michael S. Tsirkin, linux-nvdimm, Dave Chinner,
Qemu Developers, virtualization, adilger kernel, Ross Zwisler,
dave jiang, darrick wong, vishal l verma, Matthew Wilcox,
Christoph Hellwig, Linux ACPI, jmoyer, linux-ext4, Rik van Riel,
Stefan Hajnoczi, Paolo Bonzini, Dan Williams, lcapitulino,
Nitesh Narayan Lal, Theodore Ts'o
In-Reply-To: <20190114095520.GC13316@quack2.suse.cz>
> > > > > Right. Thinking about this I would be more concerned about the fact
> > > > > that
> > > > > guest can effectively pin amount of host's page cache upto size of
> > > > > the
> > > > > device/file passed to guest as PMEM, can't it Pankaj? Or is there
> > > > > some
> > > > > QEMU
> > > > > magic that avoids this?
> > > >
> > > > Yes, guest will pin these host page cache pages using 'get_user_pages'
> > > > by
> > > > elevating the page reference count. But these pages can be reclaimed by
> > > > host
> > > > at any time when there is memory pressure.
> > >
> > > Wait, how can the guest pin the host pages? I would expect this to
> > > happen only when using vfio and device assignment. Otherwise, no the
> > > host can't reclaim a pinned page, that's the whole point of a pin to
> > > prevent the mm from reclaiming ownership.
> >
> > yes. You are right I just used the pin word but it does not actually pin
> > pages
> > permanently. I had gone through the discussion on existing problems with
> > get_user_pages and DMA e.g [1] to understand Jan's POV. It does mention GUP
> > pin pages so I also used the word 'pin'. But guest does not permanently pin
> > these pages and these pages can be reclaimed by host.
>
> OK, then I was just confused how virtio-pmem is going to work. Thanks for
> explanation! So can I imagine this as guest mmaping the host file and
> providing the mapped range as "NVDIMM pages" to the kernel inside the
> guest? Or is it more complex?
yes, that's correct. Host's Qemu process virtual address range is used as guest physical
address and a direct mapping(EPT/NPT) is established. At guest side, this physical memory
range is plugged into guest system memory map and DAX mapping is setup using nvdimm calls.
Thanks,
Pankaj
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v3 0/5] kvm "virtio pmem" device
From: Jan Kara @ 2019-01-14 9:55 UTC (permalink / raw)
To: Pankaj Gupta
Cc: Jan Kara, KVM list, Michael S. Tsirkin, linux-nvdimm,
Dave Chinner, Qemu Developers, virtualization, adilger kernel,
Ross Zwisler, dave jiang, darrick wong, vishal l verma,
Matthew Wilcox, Christoph Hellwig, Linux ACPI, jmoyer, linux-ext4,
Rik van Riel, Stefan Hajnoczi, Paolo Bonzini, Dan Williams,
lcapitulino, Nitesh Narayan Lal, Theodore Ts'o
In-Reply-To: <540171952.63371441.1547345866585.JavaMail.zimbra@redhat.com>
On Sat 12-01-19 21:17:46, Pankaj Gupta wrote:
> > > > Right. Thinking about this I would be more concerned about the fact that
> > > > guest can effectively pin amount of host's page cache upto size of the
> > > > device/file passed to guest as PMEM, can't it Pankaj? Or is there some
> > > > QEMU
> > > > magic that avoids this?
> > >
> > > Yes, guest will pin these host page cache pages using 'get_user_pages' by
> > > elevating the page reference count. But these pages can be reclaimed by
> > > host
> > > at any time when there is memory pressure.
> >
> > Wait, how can the guest pin the host pages? I would expect this to
> > happen only when using vfio and device assignment. Otherwise, no the
> > host can't reclaim a pinned page, that's the whole point of a pin to
> > prevent the mm from reclaiming ownership.
>
> yes. You are right I just used the pin word but it does not actually pin pages
> permanently. I had gone through the discussion on existing problems with
> get_user_pages and DMA e.g [1] to understand Jan's POV. It does mention GUP
> pin pages so I also used the word 'pin'. But guest does not permanently pin
> these pages and these pages can be reclaimed by host.
OK, then I was just confused how virtio-pmem is going to work. Thanks for
explanation! So can I imagine this as guest mmaping the host file and
providing the mapped range as "NVDIMM pages" to the kernel inside the
guest? Or is it more complex?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Christoph Hellwig @ 2019-01-14 9:50 UTC (permalink / raw)
To: Jason Wang
Cc: Jens Axboe, jon.grimm, brijesh.singh, Konrad Rzeszutek Wilk,
Joerg Roedel, Michael S . Tsirkin, jfehlig, linux-kernel,
virtualization, linux-block, iommu, hch
In-Reply-To: <38bcbd46-674c-348a-cbd6-66bd431e986a@redhat.com>
On Mon, Jan 14, 2019 at 05:41:56PM +0800, Jason Wang wrote:
>
> On 2019/1/11 下午5:15, Joerg Roedel wrote:
>> On Fri, Jan 11, 2019 at 11:29:31AM +0800, Jason Wang wrote:
>>> Just wonder if my understanding is correct IOMMU_PLATFORM must be set for
>>> all virtio devices under AMD-SEV guests?
>> Yes, that is correct. Emulated DMA can only happen on the SWIOTLB
>> aperture, because that memory is not encrypted. The guest bounces the
>> data then to its encrypted memory.
>>
>> Regards,
>>
>> Joerg
>
>
> Thanks, have you tested vhost-net in this case. I suspect it may not work
Which brings me back to my pet pevee that we need to take actions
that virtio uses the proper dma mapping API by default with quirks
for legacy cases. The magic bypass it uses is just causing problems
over problems.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 0/3] Fix virtio-blk issue with SWIOTLB
From: Jason Wang @ 2019-01-14 9:41 UTC (permalink / raw)
To: Joerg Roedel
Cc: Jens Axboe, jon.grimm, brijesh.singh, Konrad Rzeszutek Wilk,
jfehlig, Michael S . Tsirkin, linux-kernel, virtualization,
linux-block, iommu, hch
In-Reply-To: <20190111091502.GC5825@8bytes.org>
On 2019/1/11 下午5:15, Joerg Roedel wrote:
> On Fri, Jan 11, 2019 at 11:29:31AM +0800, Jason Wang wrote:
>> Just wonder if my understanding is correct IOMMU_PLATFORM must be set for
>> all virtio devices under AMD-SEV guests?
> Yes, that is correct. Emulated DMA can only happen on the SWIOTLB
> aperture, because that memory is not encrypted. The guest bounces the
> data then to its encrypted memory.
>
> Regards,
>
> Joerg
Thanks, have you tested vhost-net in this case. I suspect it may not work
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] drm/cirrus: fix connector leak at unload
From: Gerd Hoffmann @ 2019-01-14 7:38 UTC (permalink / raw)
To: Rob Clark, dri-devel, Rob Clark, Daniel Stone, David Airlie,
YueHaibing, linux-kernel, virtualization, Thomas Zimmermann,
Dave Airlie, Thierry Reding
In-Reply-To: <20190111221638.GG21184@phenom.ffwll.local>
On Fri, Jan 11, 2019 at 11:16:38PM +0100, Daniel Vetter wrote:
> On Fri, Jan 11, 2019 at 11:06:20PM +0100, Daniel Vetter wrote:
> > On Fri, Jan 11, 2019 at 09:02:34AM -0500, Rob Clark wrote:
> > > This fixes an '*ERROR* connector VGA-2 leaked!' splat at driver unload.
> > >
> > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > ---
> > > Similar case to the issue that was fixed recently in drm/ast
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Actually I just pushed a patch to drm-misc-next to rename this function to
> drm_helper_force_disable_all(), so you need to respin ... My r-b still
> holds.
Fixed and queued up.
thanks,
Gerd
^ permalink raw reply
* Re: [PATCH v3 0/5] kvm "virtio pmem" device
From: Pankaj Gupta @ 2019-01-14 7:15 UTC (permalink / raw)
To: Matthew Wilcox
Cc: jack, kvm, linux-nvdimm, Dave Chinner, qemu-devel, virtualization,
adilger kernel, zwisler, eblake, dave jiang, darrick wong,
vishal l verma, mst, hch, linux-acpi, jmoyer, linux-ext4, riel,
stefanha, imammedo, dan j williams, lcapitulino, nilal, tytso,
xiaoguangrong eric, rjw, linux-kernel, linux-xfs, linux-fsdevel,
pbonzini
In-Reply-To: <20190113233820.GX6310@bombadil.infradead.org>
> > Until you have images (and hence host page cache) shared between
> > multiple guests. People will want to do this, because it means they
> > only need a single set of pages in host memory for executable
> > binaries rather than a set of pages per guest. Then you have
> > multiple guests being able to detect residency of the same set of
> > pages. If the guests can then, in any way, control eviction of the
> > pages from the host cache, then we have a guest-to-guest information
> > leak channel.
>
> I don't think we should ever be considering something that would allow a
> guest to evict page's from the host's pagecache [1]. The guest should
> be able to kick its own references to the host's pagecache out of its
> own pagecache, but not be able to influence whether the host or another
> guest has a read-only mapping cached.
>
> [1] Unless the guest is allowed to modify the host's file; obviously
> truncation, holepunching, etc are going to evict pages from the host's
> page cache.
This is so correct. Guest does not not evict host page cache pages directly.
In case of virtio-pmem & DAX, guest clears guest page cache exceptional entries.
Its solely decision of host to take action on the host page cache pages.
In case of virtio-pmem, guest does not modify host file directly i.e don't
perform hole punch & truncation operation directly on host file.
Thanks,
Pankaj
^ permalink raw reply
* Re: [PATCH v3 0/5] kvm "virtio pmem" device
From: Dave Chinner @ 2019-01-14 2:50 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Pankaj Gupta, jack, kvm, linux-nvdimm, qemu-devel, virtualization,
adilger kernel, zwisler, eblake, dave jiang, darrick wong,
vishal l verma, mst, hch, linux-acpi, jmoyer, linux-ext4, riel,
stefanha, imammedo, dan j williams, lcapitulino, nilal, tytso,
xiaoguangrong eric, rjw, linux-kernel, linux-xfs, linux-fsdevel,
pbonzini
In-Reply-To: <20190113233820.GX6310@bombadil.infradead.org>
On Sun, Jan 13, 2019 at 03:38:21PM -0800, Matthew Wilcox wrote:
> On Mon, Jan 14, 2019 at 10:29:02AM +1100, Dave Chinner wrote:
> > Until you have images (and hence host page cache) shared between
> > multiple guests. People will want to do this, because it means they
> > only need a single set of pages in host memory for executable
> > binaries rather than a set of pages per guest. Then you have
> > multiple guests being able to detect residency of the same set of
> > pages. If the guests can then, in any way, control eviction of the
> > pages from the host cache, then we have a guest-to-guest information
> > leak channel.
>
> I don't think we should ever be considering something that would allow a
> guest to evict page's from the host's pagecache [1]. The guest should
> be able to kick its own references to the host's pagecache out of its
> own pagecache, but not be able to influence whether the host or another
> guest has a read-only mapping cached.
>
> [1] Unless the guest is allowed to modify the host's file; obviously
> truncation, holepunching, etc are going to evict pages from the host's
> page cache.
Right, and that's exactly what I mean by "we need to be real careful
with functionality like this".
To be honest, I really don't think I've even touched the surface
here.
e.g. Filesystems and storage can share logical and physical extents.
Which means that image files that share storage (e.g. because they
are all cloned from the same master image and/or there's in-line
deduplication running on the storage) and can be directly accessed
by guests may very well be susceptible to detection of host side
deduplication and subsequent copy-on-write operations.
This really doesn't seem much different to me from the guest being
able to infer host side KSM page deduplication and COW operation in
the guest side page cache. The only difference is that DAX is being
used to probe the host side page cache and storage rather than the
guest side.
IOWs, I suspect there's a world of pain waiting for us if we punch
huge holes through the virtual machine abstractions like this.
Improving performance is a laudible goal, but at what price?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ 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