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

[Jason Wang]

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