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

[Cornelia Huck]

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.

> We only have pfn check for all helpers now.

So we'll assert() after that change?

> 
> > 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".