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

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

[Jason Wang]

Hi:

I meet an issue when doing reboot for a guest with two virtio-net-pci 
cards when iommu is enabled. What happens is:


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?

Thanks

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

[Paolo Bonzini]

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);
+    }
 }

 /* virt queue functions */

Paolo

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

[Jason Wang]

On 2017年03月29日 16:09, Paolo Bonzini 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);
> +    }
>   }
>
>   /* virt queue functions */
>
> Paolo

This looks a good fix but may not solve this issue completely. Depends 
on the iova that guest uses, address_space_cache_init() may succeed even 
in this case since vtd does a passthrough translation in this case.

Thanks

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

[Paolo Bonzini]

On 29/03/2017 10:37, Jason Wang wrote:
>>
>> 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);
>> +    }
>>   }
>>
>>   /* virt queue functions */
>>
>> Paolo
> 
> This looks a good fix but may not solve this issue completely. Depends
> on the iova that guest uses, address_space_cache_init() may succeed even
> in this case since vtd does a passthrough translation in this case.

Can you explain this more?

Paolo

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

[Jason Wang]

On 2017年03月29日 16:41, Paolo Bonzini wrote:
>
> On 29/03/2017 10:37, Jason Wang wrote:
>>> 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);
>>> +    }
>>>    }
>>>
>>>    /* virt queue functions */
>>>
>>> Paolo
>> This looks a good fix but may not solve this issue completely. Depends
>> on the iova that guest uses, address_space_cache_init() may succeed even
>> in this case since vtd does a passthrough translation in this case.
> Can you explain this more?
>
> Paolo

Yes.  In this case, virtio-net-pci is not reset, but vtd has been reset. 
So virtio-net-pci will still try to use the iova of ring to setup the 
cache but now vtd treat iova as gpa since dmar has been disabled during 
reset. (E.g In my test with Linux driver, desc map succeed but used fail.)

Thanks

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

[Paolo Bonzini]

On 29/03/2017 11:09, Jason Wang wrote:
>>>>
>>> This looks a good fix but may not solve this issue completely. Depends
>>> on the iova that guest uses, address_space_cache_init() may succeed even
>>> in this case since vtd does a passthrough translation in this case.
>> Can you explain this more?
> 
> Yes.  In this case, virtio-net-pci is not reset, but vtd has been reset.
> So virtio-net-pci will still try to use the iova of ring to setup the
> cache but now vtd treat iova as gpa since dmar has been disabled during
> reset. (E.g In my test with Linux driver, desc map succeed but used fail.)

That would be a Linux bug, the devices below the IOMMU have to be reset
first.

Paolo

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

[Jason Wang]

On 2017年03月29日 17:11, Paolo Bonzini wrote:
>
> On 29/03/2017 11:09, Jason Wang wrote:
>>>> This looks a good fix but may not solve this issue completely. Depends
>>>> on the iova that guest uses, address_space_cache_init() may succeed even
>>>> in this case since vtd does a passthrough translation in this case.
>>> Can you explain this more?
>> Yes.  In this case, virtio-net-pci is not reset, but vtd has been reset.
>> So virtio-net-pci will still try to use the iova of ring to setup the
>> cache but now vtd treat iova as gpa since dmar has been disabled during
>> reset. (E.g In my test with Linux driver, desc map succeed but used fail.)
> That would be a Linux bug, the devices below the IOMMU have to be reset
> first.
>
> Paolo

Maybe, but I meet the same issue if I do a "system_reset". I think we 
should guarantee IOMMU was reset after any pci devices in this case too?

Thanks

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

[Michael S. Tsirkin]

On Wed, Mar 29, 2017 at 05:24:18PM +0800, Jason Wang wrote:
> 
> 
> On 2017年03月29日 17:11, Paolo Bonzini wrote:
> > 
> > On 29/03/2017 11:09, Jason Wang wrote:
> > > > > This looks a good fix but may not solve this issue completely. Depends
> > > > > on the iova that guest uses, address_space_cache_init() may succeed even
> > > > > in this case since vtd does a passthrough translation in this case.
> > > > Can you explain this more?
> > > Yes.  In this case, virtio-net-pci is not reset, but vtd has been reset.
> > > So virtio-net-pci will still try to use the iova of ring to setup the
> > > cache but now vtd treat iova as gpa since dmar has been disabled during
> > > reset. (E.g In my test with Linux driver, desc map succeed but used fail.)
> > That would be a Linux bug, the devices below the IOMMU have to be reset
> > first.
> > 
> > Paolo
> 
> Maybe, but I meet the same issue if I do a "system_reset". I think we should
> guarantee IOMMU was reset after any pci devices in this case too?
> 
> Thanks
> 

system reset would indeed appear to be a big issue.

-- 
MST

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

[Cornelia Huck]

On Wed, 29 Mar 2017 10:09:10 +0200
Paolo Bonzini <pbonzini@redhat.com> 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? And a reset will unset both ->broken and the caches
anyway? What am I missing?

>  }
> 
>  /* virt queue functions */
> 
> Paolo
> 

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

[Jason Wang]

On 2017年03月29日 16:45, Cornelia Huck wrote:
> On Wed, 29 Mar 2017 10:09:10 +0200
> Paolo Bonzini <pbonzini@redhat.com> 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 only have pfn check for all helpers now.

> 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?

Thanks

>
>>   }
>>
>>   /* virt queue functions */
>>
>> Paolo
>>

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

[Cornelia Huck]

On Wed, 29 Mar 2017 17:18:00 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2017年03月29日 16:45, Cornelia Huck wrote:
> > On Wed, 29 Mar 2017 10:09:10 +0200
> > Paolo Bonzini <pbonzini@redhat.com> 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".

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 <jasowang@redhat.com> wrote:
>
>> On 2017年03月29日 16:45, Cornelia Huck wrote:
>>> On Wed, 29 Mar 2017 10:09:10 +0200
>>> Paolo Bonzini <pbonzini@redhat.com> 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

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

[Michael S. Tsirkin]

On Wed, Mar 29, 2017 at 10:09:10AM +0200, Paolo Bonzini 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);
> +    }
>  }
> 
>  /* virt queue functions */

I would be worried about call_rcu here - this means
something can hang on to and use the old cache,
and reset really must act as a sync/flush point.


> Paolo

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

[Paolo Bonzini]

On 29/03/2017 23:28, Michael S. Tsirkin wrote:
>> 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);
>> +    }
>>  }
>>
>>  /* virt queue functions */
> I would be worried about call_rcu here - this means
> something can hang on to and use the old cache,
> and reset really must act as a sync/flush point.

The flush is done later in virtio_reset.  Here it's just reacting
asynchronously to the IOMMU reset.

I'm thinking of adding a global generation count for IOMMU mappings, and
forcing an update when the IOMMU mappings have changed.

Paolo

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

[Peter Xu]

On Wed, Mar 29, 2017 at 04:00:37PM +0800, Jason Wang wrote:
> Hi:
> 
> I meet an issue when doing reboot for a guest with two virtio-net-pci cards
> when iommu is enabled. What happens is:
> 
> 
> 1) vtd was reset first

I'll add an extra question:

Currently with Q35 and VT-d, our system qtree looks like (simplified
version):

bus: system bus
  dev: intel-iommu
  dev: q35-pcihost
    bus: pcie.0
      dev: pci device 1
      dev: pci device 2
      ...

Not sure whether it'll be clearer to switch to:

bus: system bus
  dev: q35-pcihost
    bus: iommu-scope 0
      dev: intel-iommu
        bus: pcie.0
          dev: pci device 1
          dev: pci device 2
          ...

since logically the IOMMU device should be part of q35 pci-host?

Further, if we'll have more intel-iommus in the future in a single
guest, we'll be able to have devices dangle under the specific iommu
that it belongs. That'll be nice imho.

(Btw, I see that we are using qbus-qdev-qbus-... iterations to
 describe the system tree. Why cannot we have qdev under another qdev?
 Sorry if the question is stupid... Any pointers on explanations to
 these qdev logics would be appreciated as well.)

> 
> 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?
> 
> Thanks
> 

-- peterx

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

[Michael S. Tsirkin]

On Wed, Mar 29, 2017 at 04:16:30PM +0800, Peter Xu wrote:
> On Wed, Mar 29, 2017 at 04:00:37PM +0800, Jason Wang wrote:
> > Hi:
> > 
> > I meet an issue when doing reboot for a guest with two virtio-net-pci cards
> > when iommu is enabled. What happens is:
> > 
> > 
> > 1) vtd was reset first
> 
> I'll add an extra question:
> 
> Currently with Q35 and VT-d, our system qtree looks like (simplified
> version):
> 
> bus: system bus
>   dev: intel-iommu
>   dev: q35-pcihost
>     bus: pcie.0
>       dev: pci device 1
>       dev: pci device 2
>       ...
> 
> Not sure whether it'll be clearer to switch to:
> 
> bus: system bus
>   dev: q35-pcihost
>     bus: iommu-scope 0
>       dev: intel-iommu
>         bus: pcie.0
>           dev: pci device 1
>           dev: pci device 2
>           ...
> 
> since logically the IOMMU device should be part of q35 pci-host?
> 
> Further, if we'll have more intel-iommus in the future in a single
> guest, we'll be able to have devices dangle under the specific iommu
> that it belongs. That'll be nice imho.
> 
> (Btw, I see that we are using qbus-qdev-qbus-... iterations to
>  describe the system tree. Why cannot we have qdev under another qdev?
>  Sorry if the question is stupid... Any pointers on explanations to
>  these qdev logics would be appreciated as well.)

Especially when you have many iommus, it's quite common to have iommus
be guest programmable with the bus range that they handle.

They are integrated devices, yes, but they simply aren't buses in the
real world.  I am guessing what happens on bare metal is that host
bridge resets the bus first and then resets itself
including the IOMMU.

> > 
> > 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?
> > 
> > Thanks
> > 
> 
> -- peterx