| 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 | expand |
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
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 >
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
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 >
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 = {
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(+)