[2/2] hw/arm/smmuv3: Implement dummy replay
diff mbox series

Message ID 20190611142821.3874-3-eric.auger@redhat.com
State New
Headers show
Series
  • ARM SMMUv3: Fix spurious notification errors and stall with vfio-pci
Related show

Commit Message

Auger Eric June 11, 2019, 2:28 p.m. UTC
On ARM we currently do not support VFIO-PCI devices protected
by the IOMMU. Any attempt to run such use case results in this
kind of warning:

"-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
warning: SMMUv3 does not support notification on MAP: device vfio-pci
will not function properly".

However this is just a warning and this should not prevent the
guest from booting in a reasonable amount of time. This does not
happen currently.

This is due to the fact the VFIO vfio_listener_region_add() calls
memory_region_iommu_replay(). As the SMMUv3 IOMMUMemoryRegionClass
currently does not implement the replay() callback, the default
memory_region_iommu_replay() implementation is used. This latter
loops on the whole notifier's range (48b address space), translates
each page and call the notifier on the resulting entry. This totally
freezes the guest.

The Intel IOMMU implements the replay() function which only
notifies valid page table entries.

In the looming SMMUv3 nested stage VFIO integration, there will be
no need to replay() anything as there will not be any shadow page
tables: the stage 1 page tables are owned by the guest.

So let's implement a void replay() which satisfies both cases.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/smmuv3.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Peter Maydell June 14, 2019, 1:26 p.m. UTC | #1
On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> wrote:
>
> On ARM we currently do not support VFIO-PCI devices protected
> by the IOMMU. Any attempt to run such use case results in this
> kind of warning:
>
> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
> warning: SMMUv3 does not support notification on MAP: device vfio-pci
> will not function properly".
>
> However this is just a warning and this should not prevent the
> guest from booting in a reasonable amount of time. This does not
> happen currently.
>
> This is due to the fact the VFIO vfio_listener_region_add() calls
> memory_region_iommu_replay(). As the SMMUv3 IOMMUMemoryRegionClass
> currently does not implement the replay() callback, the default
> memory_region_iommu_replay() implementation is used. This latter
> loops on the whole notifier's range (48b address space), translates
> each page and call the notifier on the resulting entry. This totally
> freezes the guest.
>
> The Intel IOMMU implements the replay() function which only
> notifies valid page table entries.
>
> In the looming SMMUv3 nested stage VFIO integration, there will be
> no need to replay() anything as there will not be any shadow page
> tables: the stage 1 page tables are owned by the guest.
>
> So let's implement a void replay() which satisfies both cases.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/smmuv3.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index e2f07d2864..1f578365ef 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1489,6 +1489,11 @@ static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>      }
>  }
>
> +static inline void
> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> +{
> +}

This doesn't seem like a valid implementation of the replay
method to me. The API doc comment says
     * The default implementation of memory_region_iommu_replay() is to
     * call the IOMMU translate method for every page in the address space
     * with flag == IOMMU_NONE and then call the notifier if translate
     * returns a valid mapping. If this method is implemented then it
     * overrides the default behaviour, and must provide the full semantics
     * of memory_region_iommu_replay(), by calling @notifier for every
     * translation present in the IOMMU.

This empty function is definitely not going to call the notifier
for every IOMMU translation...

thanks
-- PMM
Auger Eric June 14, 2019, 1:40 p.m. UTC | #2
Hi Peter,

On 6/14/19 3:26 PM, Peter Maydell wrote:
> On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> wrote:
>>
>> On ARM we currently do not support VFIO-PCI devices protected
>> by the IOMMU. Any attempt to run such use case results in this
>> kind of warning:
>>
>> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
>> warning: SMMUv3 does not support notification on MAP: device vfio-pci
>> will not function properly".
>>
>> However this is just a warning and this should not prevent the
>> guest from booting in a reasonable amount of time. This does not
>> happen currently.
>>
>> This is due to the fact the VFIO vfio_listener_region_add() calls
>> memory_region_iommu_replay(). As the SMMUv3 IOMMUMemoryRegionClass
>> currently does not implement the replay() callback, the default
>> memory_region_iommu_replay() implementation is used. This latter
>> loops on the whole notifier's range (48b address space), translates
>> each page and call the notifier on the resulting entry. This totally
>> freezes the guest.
>>
>> The Intel IOMMU implements the replay() function which only
>> notifies valid page table entries.
>>
>> In the looming SMMUv3 nested stage VFIO integration, there will be
>> no need to replay() anything as there will not be any shadow page
>> tables: the stage 1 page tables are owned by the guest.
>>
>> So let's implement a void replay() which satisfies both cases.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/smmuv3.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index e2f07d2864..1f578365ef 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -1489,6 +1489,11 @@ static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
>>      }
>>  }
>>
>> +static inline void
>> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>> +{
>> +}
> 
> This doesn't seem like a valid implementation of the replay
> method to me. The API doc comment says
>      * The default implementation of memory_region_iommu_replay() is to
>      * call the IOMMU translate method for every page in the address space
>      * with flag == IOMMU_NONE and then call the notifier if translate
>      * returns a valid mapping. If this method is implemented then it
>      * overrides the default behaviour, and must provide the full semantics
>      * of memory_region_iommu_replay(), by calling @notifier for every
>      * translation present in the IOMMU.
> 
> This empty function is definitely not going to call the notifier
> for every IOMMU translation...
The situation is a bit odd. SMMUv3 is not integrated with VFIO so VFIO
devices will not work anyway (we are not able to notify on MAP). There
is a warning already reporting the issue. However the default
implementation of memory_region_iommu_replay() prevents the guest from
booting. So what would you advise?

Thanks

Eric
> 
> thanks
> -- PMM
>
Peter Maydell June 14, 2019, 1:45 p.m. UTC | #3
On Fri, 14 Jun 2019 at 14:40, Auger Eric <eric.auger@redhat.com> wrote:
>
> Hi Peter,
>
> On 6/14/19 3:26 PM, Peter Maydell wrote:
> > On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> wrote:
> >>
> >> On ARM we currently do not support VFIO-PCI devices protected
> >> by the IOMMU. Any attempt to run such use case results in this
> >> kind of warning:
> >>
> >> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
> >> warning: SMMUv3 does not support notification on MAP: device vfio-pci
> >> will not function properly".

> >> +static inline void
> >> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
> >> +{
> >> +}
> >
> > This doesn't seem like a valid implementation of the replay
> > method to me. The API doc comment says
> >      * The default implementation of memory_region_iommu_replay() is to
> >      * call the IOMMU translate method for every page in the address space
> >      * with flag == IOMMU_NONE and then call the notifier if translate
> >      * returns a valid mapping. If this method is implemented then it
> >      * overrides the default behaviour, and must provide the full semantics
> >      * of memory_region_iommu_replay(), by calling @notifier for every
> >      * translation present in the IOMMU.
> >
> > This empty function is definitely not going to call the notifier
> > for every IOMMU translation...
> The situation is a bit odd. SMMUv3 is not integrated with VFIO so VFIO
> devices will not work anyway (we are not able to notify on MAP). There
> is a warning already reporting the issue. However the default
> implementation of memory_region_iommu_replay() prevents the guest from
> booting. So what would you advise?

I dunno, but if the API isn't supposed to behave the way we've
documented it to, we should fix the documentation... Since
the only user of memory_region_iommu_replay() is the vfio code
I guess we can define it however is most convenient for vfio,
but we should document what the method has to do to make things
work.

PS: we have a memory_region_iommu_replay_all() which currently
appears to be not used by anybody, could we get rid of it?

thanks
-- PMM
Auger Eric June 14, 2019, 2:09 p.m. UTC | #4
Hi Peter,

On 6/14/19 3:45 PM, Peter Maydell wrote:
> On Fri, 14 Jun 2019 at 14:40, Auger Eric <eric.auger@redhat.com> wrote:
>>
>> Hi Peter,
>>
>> On 6/14/19 3:26 PM, Peter Maydell wrote:
>>> On Tue, 11 Jun 2019 at 15:29, Eric Auger <eric.auger@redhat.com> wrote:
>>>>
>>>> On ARM we currently do not support VFIO-PCI devices protected
>>>> by the IOMMU. Any attempt to run such use case results in this
>>>> kind of warning:
>>>>
>>>> "-device vfio-pci,host=0004:01:00.0,id=hostdev0,bus=pci.1,addr=0x0:
>>>> warning: SMMUv3 does not support notification on MAP: device vfio-pci
>>>> will not function properly".
> 
>>>> +static inline void
>>>> +smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
>>>> +{
>>>> +}
>>>
>>> This doesn't seem like a valid implementation of the replay
>>> method to me. The API doc comment says
>>>      * The default implementation of memory_region_iommu_replay() is to
>>>      * call the IOMMU translate method for every page in the address space
>>>      * with flag == IOMMU_NONE and then call the notifier if translate
>>>      * returns a valid mapping. If this method is implemented then it
>>>      * overrides the default behaviour, and must provide the full semantics
>>>      * of memory_region_iommu_replay(), by calling @notifier for every
>>>      * translation present in the IOMMU.
>>>
>>> This empty function is definitely not going to call the notifier
>>> for every IOMMU translation...
>> The situation is a bit odd. SMMUv3 is not integrated with VFIO so VFIO
>> devices will not work anyway (we are not able to notify on MAP). There
>> is a warning already reporting the issue. However the default
>> implementation of memory_region_iommu_replay() prevents the guest from
>> booting. So what would you advise?
> 
> I dunno, but if the API isn't supposed to behave the way we've
> documented it to, we should fix the documentation...

fair enough

 Since
> the only user of memory_region_iommu_replay() is the vfio code
> I guess we can define it however is most convenient for vfio,
> but we should document what the method has to do to make things
> work.

OK I need to think about it. Maybe an alternative is to call
memory_region_iommu_replay() only when it makes sense.
> 
> PS: we have a memory_region_iommu_replay_all() which currently
> appears to be not used by anybody, could we get rid of it?

I will do that

Thanks

Eric
> 
> thanks
> -- PMM
>

Patch
diff mbox series

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index e2f07d2864..1f578365ef 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1489,6 +1489,11 @@  static void smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
     }
 }
 
+static inline void
+smmuv3_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
+{
+}
+
 static void smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
                                                   void *data)
 {
@@ -1496,6 +1501,7 @@  static void smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
 
     imrc->translate = smmuv3_translate;
     imrc->notify_flag_changed = smmuv3_notify_flag_changed;
+    imrc->replay = smmuv3_replay;
 }
 
 static const TypeInfo smmuv3_type_info = {