Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
sev_active() is gone now in linux-next, at least as a global API. And once again this is entirely going in the wrong direction. The only way using the DMA API is going to work at all is if the device is ready for it. So we need a flag on the virtio device, exposed by the hypervisor (or hardware for hw virtio devices) that says: hey, I'm real, don't take a shortcut. And that means on power and s390 qemu will always have to set thos if you want to be ready for the ultravisor and co games. It's not like we haven't been through this a few times before, have we? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
On Sat, Aug 10, 2019 at 02:57:17PM -0400, Michael S. Tsirkin wrote: > On Tue, Jan 29, 2019 at 03:08:12PM -0200, Thiago Jung Bauermann wrote: > > > > Hello, > > > > With Christoph's rework of the DMA API that recently landed, the patch > > below is the only change needed in virtio to make it work in a POWER > > secure guest under the ultravisor. > > > > The other change we need (making sure the device's dma_map_ops is NULL > > so that the dma-direct/swiotlb code is used) can be made in > > powerpc-specific code. > > > > Of course, I also have patches (soon to be posted as RFC) which hook up > > to the powerpc secure guest support code. > > > > What do you think? > > > > >From d0629a36a75c678b4a72b853f8f7f8c17eedd6b3 Mon Sep 17 00:00:00 2001 > > From: Thiago Jung Bauermann > > Date: Thu, 24 Jan 2019 22:08:02 -0200 > > Subject: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted > > > > The host can't access the guest memory when it's encrypted, so using > > regular memory pages for the ring isn't an option. Go through the DMA API. > > > > Signed-off-by: Thiago Jung Bauermann > > --- > > drivers/virtio/virtio_ring.c | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index cd7e755484e3..321a27075380 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -259,8 +259,11 @@ static bool vring_use_dma_api(struct virtio_device > > *vdev) > > * not work without an even larger kludge. Instead, enable > > * the DMA API if we're a Xen guest, which at least allows > > * all of the sensible Xen configurations to work correctly. > > +* > > +* Also, if guest memory is encrypted the host can't access > > +* it directly. In this case, we'll need to use the DMA API. > > */ > > - if (xen_domain()) > > + if (xen_domain() || sev_active()) > > return true; > > > > return false; > > So I gave this lots of thought, and I'm coming round to > basically accepting something very similar to this patch. > > But not exactly like this :). > > Let's see what are the requirements. > > If > > 1. We do not trust the device (so we want to use a bounce buffer with it) > 2. DMA address is also a physical address of a buffer > > then we should use DMA API with virtio. > > > sev_active() above is one way to put (1). I can't say I love it but > it's tolerable. > > > But we also want promise from DMA API about 2. > > > Without promise 2 we simply can't use DMA API with a legacy device. > > > Otherwise, on a SEV system with an IOMMU which isn't 1:1 > and with a virtio device without ACCESS_PLATFORM, we are trying > to pass a virtual address, and devices without ACCESS_PLATFORM > can only access CPU physical addresses. > > So something like: > > dma_addr_is_phys_addr? On our Secure pseries platform, dma address is physical address and this proposal will help us, use DMA API. On our normal pseries platform, dma address is physical address too. But we do not necessarily need to use the DMA API. We can use the DMA API, but our handlers will do the same thing, the generic virtio handlers would do. If there is an opt-out option; even when dma addr is same as physical addr, than there will be less code duplication. Would something like this be better. (dma_addr_is_phys_addr && arch_want_to_use_dma_api()) ? RP > -- > MST -- Ram Pai ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Thu, Aug 08, 2019 at 08:54:54PM +0800, Jason Wang wrote: > I don't have any objection to convert to spinlock() but just want to > know if any case that the above smp_mb() + counter looks good to you? So how about we try this: - revert the original patch for this release - new safe patch with a spinlock for the next release - whatever improvements we can come up with on top Thoughts? Because I think this needs much more scrutiny than we can give an incremental patch. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
On Tue, Jan 29, 2019 at 03:08:12PM -0200, Thiago Jung Bauermann wrote: > > Hello, > > With Christoph's rework of the DMA API that recently landed, the patch > below is the only change needed in virtio to make it work in a POWER > secure guest under the ultravisor. > > The other change we need (making sure the device's dma_map_ops is NULL > so that the dma-direct/swiotlb code is used) can be made in > powerpc-specific code. > > Of course, I also have patches (soon to be posted as RFC) which hook up > to the powerpc secure guest support code. > > What do you think? > > >From d0629a36a75c678b4a72b853f8f7f8c17eedd6b3 Mon Sep 17 00:00:00 2001 > From: Thiago Jung Bauermann > Date: Thu, 24 Jan 2019 22:08:02 -0200 > Subject: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted > > The host can't access the guest memory when it's encrypted, so using > regular memory pages for the ring isn't an option. Go through the DMA API. > > Signed-off-by: Thiago Jung Bauermann > --- > drivers/virtio/virtio_ring.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index cd7e755484e3..321a27075380 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -259,8 +259,11 @@ static bool vring_use_dma_api(struct virtio_device *vdev) >* not work without an even larger kludge. Instead, enable >* the DMA API if we're a Xen guest, which at least allows >* all of the sensible Xen configurations to work correctly. > + * > + * Also, if guest memory is encrypted the host can't access > + * it directly. In this case, we'll need to use the DMA API. >*/ > - if (xen_domain()) > + if (xen_domain() || sev_active()) > return true; > > return false; So I gave this lots of thought, and I'm coming round to basically accepting something very similar to this patch. But not exactly like this :). Let's see what are the requirements. If 1. We do not trust the device (so we want to use a bounce buffer with it) 2. DMA address is also a physical address of a buffer then we should use DMA API with virtio. sev_active() above is one way to put (1). I can't say I love it but it's tolerable. But we also want promise from DMA API about 2. Without promise 2 we simply can't use DMA API with a legacy device. Otherwise, on a SEV system with an IOMMU which isn't 1:1 and with a virtio device without ACCESS_PLATFORM, we are trying to pass a virtual address, and devices without ACCESS_PLATFORM can only access CPU physical addresses. So something like: dma_addr_is_phys_addr? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 1/2] virtio_console: free unused buffers with port delete
On Fri, Aug 09, 2019 at 12:18:46PM +0530, Pankaj Gupta wrote:
> The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> deferred detaching of unused buffer to virtio device unplug time.
> This causes unplug/replug of single port in virtio device with an
> error "Error allocating inbufs\n". As we don't free the unused buffers
> attached with the port. Re-plug the same port tries to allocate new
> buffers in virtqueue and results in this error if queue is full.
So why not reuse the buffers that are already there in this case?
Seems quite possible.
> This patch removes the unused buffers in vq's when we unplug the port.
> This is the best we can do as we cannot call device_reset because virtio
> device is still active.
>
> Reported-by: Xiaohui Li
> Fixes: a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Pankaj Gupta
This is really a revert of a7a69ec0d8e4, just tagged confusingly.
And the original is also supposed to be a bugfix.
So how will the original bug be fixed?
"this is the best we can do" is rarely the case.
I am not necessarily against the revert. But if we go that way then what
we need to do is specify the behaviour in the spec, since strict spec
compliance is exactly what the original patch was addressing.
In particular, we'd document that console has a special property that
when port is detached virtqueue is considered stopped, device must not
use any buffers, and it is legal to take buffers out of the device.
> ---
> drivers/char/virtio_console.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 7270e7b69262..e8be82f1bae9 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1494,15 +1494,25 @@ static void remove_port(struct kref *kref)
> kfree(port);
> }
>
> +static void remove_unused_bufs(struct virtqueue *vq)
> +{
> + struct port_buffer *buf;
> +
> + while ((buf = virtqueue_detach_unused_buf(vq)))
> + free_buf(buf, true);
> +}
> +
> static void remove_port_data(struct port *port)
> {
> spin_lock_irq(>inbuf_lock);
> /* Remove unused data this port might have received. */
> discard_port_data(port);
> + remove_unused_bufs(port->in_vq);
> spin_unlock_irq(>inbuf_lock);
>
> spin_lock_irq(>outvq_lock);
> reclaim_consumed_buffers(port);
> + remove_unused_bufs(port->out_vq);
> spin_unlock_irq(>outvq_lock);
> }
>
> @@ -1938,11 +1948,9 @@ static void remove_vqs(struct ports_device *portdev)
> struct virtqueue *vq;
>
> virtio_device_for_each_vq(portdev->vdev, vq) {
> - struct port_buffer *buf;
>
> flush_bufs(vq, true);
> - while ((buf = virtqueue_detach_unused_buf(vq)))
> - free_buf(buf, true);
> + remove_unused_bufs(vq);
> }
> portdev->vdev->config->del_vqs(portdev->vdev);
> kfree(portdev->in_vqs);
> --
> 2.21.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/2] virtio: decrement avail idx with buffer detach for packed ring
On Fri, Aug 09, 2019 at 12:18:47PM +0530, Pankaj Gupta wrote:
> This patch decrements 'next_avail_idx' count when detaching a buffer
> from vq for packed ring code. Split ring code already does this in
> virtqueue_detach_unused_buf_split function. This updates the
> 'next_avail_idx' to the previous correct index after an unused buffer
> is detatched from the vq.
>
> Signed-off-by: Pankaj Gupta
I would make this patch 1, not patch 2, otherwise
patch 1 corrupts the ring.
> ---
> drivers/virtio/virtio_ring.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index c8be1c4f5b55..7c69181113e2 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1537,6 +1537,12 @@ static void *virtqueue_detach_unused_buf_packed(struct
> virtqueue *_vq)
> /* detach_buf clears data, so grab it now. */
> buf = vq->packed.desc_state[i].data;
> detach_buf_packed(vq, i, NULL);
> + vq->packed.next_avail_idx--;
> + if (vq->packed.next_avail_idx < 0) {
> + vq->packed.next_avail_idx = vq->packed.vring.num - 1;
> + vq->packed.avail_wrap_counter ^= 1;
> + }
> +
> END_USE(vq);
> return buf;
> }
> --
> 2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V5 0/9] Fixes for vhost metadata acceleration
On Fri, Aug 09, 2019 at 01:48:42AM -0400, Jason Wang wrote: > Hi all: > > This series try to fix several issues introduced by meta data > accelreation series. Please review. > > Changes from V4: > - switch to use spinlock synchronize MMU notifier with accessors > > Changes from V3: > - remove the unnecessary patch > > Changes from V2: > - use seqlck helper to synchronize MMU notifier with vhost worker > > Changes from V1: > - try not use RCU to syncrhonize MMU notifier with vhost worker > - set dirty pages after no readers > - return -EAGAIN only when we find the range is overlapped with > metadata > > Jason Wang (9): > vhost: don't set uaddr for invalid address > vhost: validate MMU notifier registration > vhost: fix vhost map leak > vhost: reset invalidate_count in vhost_set_vring_num_addr() > vhost: mark dirty pages during map uninit > vhost: don't do synchronize_rcu() in vhost_uninit_vq_maps() > vhost: do not use RCU to synchronize MMU notifier with worker > vhost: correctly set dirty pages in MMU notifiers callback > vhost: do not return -EAGAIN for non blocking invalidation too early > > drivers/vhost/vhost.c | 202 +- > drivers/vhost/vhost.h | 6 +- > 2 files changed, 122 insertions(+), 86 deletions(-) This generally looks more solid. But this amounts to a significant overhaul of the code. At this point how about we revert 7f466032dc9e5a61217f22ea34b2df932786bbfc for this release, and then re-apply a corrected version for the next one? > -- > 2.18.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drm/virtio: use virtio_max_dma_size
On Thu, Aug 08, 2019 at 05:34:45PM +0200, Gerd Hoffmann wrote: > We must make sure our scatterlist segments are not too big, otherwise > we might see swiotlb failures (happens with sev, also reproducable with > swiotlb=force). Btw, any chance I could also draft you to replace the remaining abuses of swiotlb_max_segment and swiotlb_nr_tbl in drm with proper dma level interface? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost: do not reference a file that does not exist
On Wed, Aug 07, 2019 at 05:52:55PM -0700, egran...@chromium.org wrote: > From: Enrico Granata > > lguest was removed from the mainline kernel in late 2017. > > Signed-off-by: Enrico Granata But this particular file even has an override in the script looking for dead references, which together with the content of the overal contents makes me thing the dangling reference is somewhat intentional. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 0/5] iommu/amd: Convert the AMD iommu driver to the dma-iommu api
On Sun, Jun 23, 2019 at 11:19:45PM -0700, Christoph Hellwig wrote: > Tom, > > next time please cc Jerg as the AMD IOMMU maintainer. > > Joerg, any chance you could review this? Toms patches to convert the > AMD and Intel IOMMU drivers to the dma-iommu code are going to make my > life in DMA land significantly easier, so I have a vested interest > in this series moving forward :) Tom, can you repost the series? Seems like there hasn't been any news for a month. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
