qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] issues of region cache and iommu reset


From: Jason Wang
Subject: Re: [Qemu-devel] issues of region cache and iommu reset
Date: Thu, 30 Mar 2017 10:14:25 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0



On 2017年03月29日 19:39, Cornelia Huck wrote:
On Wed, 29 Mar 2017 17:18:00 +0800
Jason Wang <address@hidden> wrote:

On 2017年03月29日 16:45, Cornelia Huck wrote:
On Wed, 29 Mar 2017 10:09:10 +0200
Paolo Bonzini <address@hidden> wrote:

On 29/03/2017 10:00, Jason Wang wrote:
1) vtd was reset first

2) during the reset of virtio-net-pci #1, deletion of msix subregion
will cause a commit of all memory listeners

3) virito-net-pci #2's region cache will be update, but since vtd has
already been reset, it can't get a valid mappings here

Any ideas on how to fix this? Need region cache be aware of IOMMU/IOTLB
state in this case? Or can we simply reset IOMMU as the last one?
Something like this?

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 03592c5..73e69ac 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -176,6 +176,10 @@ err_used:
       address_space_cache_destroy(&new->desc);
   err_desc:
       g_free(new);
+    atomic_rcu_set(&vq->vring.caches, NULL);
+    if (old) {
+        call_rcu(old, virtio_free_region_cache, rcu);
+    }
Maybe I'm just confused here, but isn't ->broken enough to prevent
further accesses?
Looks not (e.g virtio_queue_update_used_idx() that was called by vhost).
We probably should take care that we don't do things like that for
broken devices. But that ties into the virtio_error() discussion, and
we probably want to revisit that in that context.

Yes, but the issue here is, the device should not be treated as broken since it does nothing wrong.


We only have pfn check for all helpers now.
So we'll assert() after that change?

I think we don't want assert too, since it was a indicator of bug somewhere.


And a reset will unset both ->broken and the caches
anyway? What am I missing?
Yes, but is it good to store invalid or even wrong mappings in the cache?
Not really; but I'd like to see ->broken as a kind of master indicator
"this device does not work, do not trust/use anything until it has been
reset".


Then I guess we probably check broken for all exported helpers like what we did for gfn.

Thanks



reply via email to

[Prev in Thread] Current Thread [Next in Thread]