| Message ID | 1442495357-26547-6-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 17/09/15 15:09, David Gibson wrote: > When we have guest visible IOMMUs, we allow notifiers to be registered > which will be informed of all changes to IOMMU mappings. This is used by > vfio to keep the host IOMMU mappings in sync with guest IOMMU mappings. > > However, unlike with a memory region listener, an iommu notifier won't be > told about any mappings which already exist in the (guest) IOMMU at the > time it is registered. This can cause problems if hotplugging a VFIO > device onto a guest bus which had existing guest IOMMU mappings, but didn't > previously have an VFIO devices (and hence no host IOMMU mappings). > > This adds a memory_region_register_iommu_notifier_replay() function to > handle this case. As well as registering the new notifier it replays > existing mappings. Because the IOMMU memory region doesn't internally > remember the granularity of the guest IOMMU it has a small hack where the > caller must specify a granularity at which to replay mappings. > > If there are finer mappings in the guest IOMMU these will be reported in > the iotlb structures passed to the notifier which it must handle (probably > causing it to flag an error). This isn't new - the VFIO iommu notifier > must already handle notifications about guest IOMMU mappings too short > for it to represent in the host IOMMU. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > include/exec/memory.h | 16 ++++++++++++++++ > memory.c | 18 ++++++++++++++++++ > 2 files changed, 34 insertions(+) > diff --git a/memory.c b/memory.c > index 0d8b2d9..6b5a2f1 100644 > --- a/memory.c > +++ b/memory.c > @@ -1403,6 +1403,24 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > notifier_list_add(&mr->iommu_notify, n); > } > > +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, > + hwaddr granularity, bool is_write) granularity itself is not an address, but a size, isn't it? So using "hwaddr" sounds wrong here. > +{ > + hwaddr addr; dma_addr_t ? > + IOMMUTLBEntry iotlb; > + > + memory_region_register_iommu_notifier(mr, n); > + > + for (addr = 0; > + int128_lt(int128_make64(addr), mr->size); > + addr += granularity) { > + > + iotlb = mr->iommu_ops->translate(mr, addr, is_write); > + if (iotlb.perm != IOMMU_NONE) > + n->notify(n, &iotlb); Missing curly braces. > + } > +} > + Thomas
On 23/09/2015 12:40, Thomas Huth wrote: > On 17/09/15 15:09, David Gibson wrote: >> When we have guest visible IOMMUs, we allow notifiers to be registered >> which will be informed of all changes to IOMMU mappings. This is used by >> vfio to keep the host IOMMU mappings in sync with guest IOMMU mappings. >> >> However, unlike with a memory region listener, an iommu notifier won't be >> told about any mappings which already exist in the (guest) IOMMU at the >> time it is registered. This can cause problems if hotplugging a VFIO >> device onto a guest bus which had existing guest IOMMU mappings, but didn't >> previously have an VFIO devices (and hence no host IOMMU mappings). >> >> This adds a memory_region_register_iommu_notifier_replay() function to >> handle this case. As well as registering the new notifier it replays >> existing mappings. Because the IOMMU memory region doesn't internally >> remember the granularity of the guest IOMMU it has a small hack where the >> caller must specify a granularity at which to replay mappings. >> >> If there are finer mappings in the guest IOMMU these will be reported in >> the iotlb structures passed to the notifier which it must handle (probably >> causing it to flag an error). This isn't new - the VFIO iommu notifier >> must already handle notifications about guest IOMMU mappings too short >> for it to represent in the host IOMMU. >> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> include/exec/memory.h | 16 ++++++++++++++++ >> memory.c | 18 ++++++++++++++++++ >> 2 files changed, 34 insertions(+) > >> diff --git a/memory.c b/memory.c >> index 0d8b2d9..6b5a2f1 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -1403,6 +1403,24 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) >> notifier_list_add(&mr->iommu_notify, n); >> } >> >> +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, >> + hwaddr granularity, bool is_write) > > granularity itself is not an address, but a size, isn't it? So using > "hwaddr" sounds wrong here. I disagree: in memory.c, all size are defined with hwaddr. >> +{ >> + hwaddr addr; > > dma_addr_t ? > >> + IOMMUTLBEntry iotlb; >> + >> + memory_region_register_iommu_notifier(mr, n); >> + >> + for (addr = 0; >> + int128_lt(int128_make64(addr), mr->size); >> + addr += granularity) { >> + >> + iotlb = mr->iommu_ops->translate(mr, addr, is_write); >> + if (iotlb.perm != IOMMU_NONE) >> + n->notify(n, &iotlb); > > Missing curly braces. > >> + } >> +} >> + > > Thomas >
On 17/09/2015 15:09, David Gibson wrote: > When we have guest visible IOMMUs, we allow notifiers to be registered > which will be informed of all changes to IOMMU mappings. This is used by > vfio to keep the host IOMMU mappings in sync with guest IOMMU mappings. > > However, unlike with a memory region listener, an iommu notifier won't be > told about any mappings which already exist in the (guest) IOMMU at the > time it is registered. This can cause problems if hotplugging a VFIO > device onto a guest bus which had existing guest IOMMU mappings, but didn't > previously have an VFIO devices (and hence no host IOMMU mappings). > > This adds a memory_region_register_iommu_notifier_replay() function to > handle this case. As well as registering the new notifier it replays > existing mappings. Because the IOMMU memory region doesn't internally > remember the granularity of the guest IOMMU it has a small hack where the > caller must specify a granularity at which to replay mappings. > > If there are finer mappings in the guest IOMMU these will be reported in > the iotlb structures passed to the notifier which it must handle (probably > causing it to flag an error). This isn't new - the VFIO iommu notifier > must already handle notifications about guest IOMMU mappings too short > for it to represent in the host IOMMU. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > include/exec/memory.h | 16 ++++++++++++++++ > memory.c | 18 ++++++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 5baaf48..3cf145b 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -583,6 +583,22 @@ void memory_region_notify_iommu(MemoryRegion *mr, > void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > > /** > + * memory_region_register_iommu_notifier_replay: register a notifier > + * for changes to IOMMU translation entries, and replay existing IOMMU > + * translations to the new notifier. > + * > + * @mr: the memory region to observe > + * @n: the notifier to be added; the notifier receives a pointer to an > + * #IOMMUTLBEntry as the opaque value; the pointer ceases to be > + * valid on exit from the notifier. > + * @granularity: Minimum page granularity to replay notifications for > + * @is_write: Whether to treat the replay as a translate "write" > + * through the iommu > + */ > +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, > + hwaddr granularity, bool is_write); > + > +/** > * memory_region_unregister_iommu_notifier: unregister a notifier for > * changes to IOMMU translation entries. > * > diff --git a/memory.c b/memory.c > index 0d8b2d9..6b5a2f1 100644 > --- a/memory.c > +++ b/memory.c > @@ -1403,6 +1403,24 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > notifier_list_add(&mr->iommu_notify, n); > } > > +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, > + hwaddr granularity, bool is_write) > +{ > + hwaddr addr; > + IOMMUTLBEntry iotlb; > + > + memory_region_register_iommu_notifier(mr, n); > + > + for (addr = 0; > + int128_lt(int128_make64(addr), mr->size); "addr < memory_region_size(mr)" should be enough. > + addr += granularity) { > + > + iotlb = mr->iommu_ops->translate(mr, addr, is_write); > + if (iotlb.perm != IOMMU_NONE) > + n->notify(n, &iotlb); > + } > +} > + > void memory_region_unregister_iommu_notifier(Notifier *n) > { > notifier_remove(n); >
On Wed, Sep 23, 2015 at 12:40:18PM +0200, Thomas Huth wrote: > On 17/09/15 15:09, David Gibson wrote: > > When we have guest visible IOMMUs, we allow notifiers to be registered > > which will be informed of all changes to IOMMU mappings. This is used by > > vfio to keep the host IOMMU mappings in sync with guest IOMMU mappings. > > > > However, unlike with a memory region listener, an iommu notifier won't be > > told about any mappings which already exist in the (guest) IOMMU at the > > time it is registered. This can cause problems if hotplugging a VFIO > > device onto a guest bus which had existing guest IOMMU mappings, but didn't > > previously have an VFIO devices (and hence no host IOMMU mappings). > > > > This adds a memory_region_register_iommu_notifier_replay() function to > > handle this case. As well as registering the new notifier it replays > > existing mappings. Because the IOMMU memory region doesn't internally > > remember the granularity of the guest IOMMU it has a small hack where the > > caller must specify a granularity at which to replay mappings. > > > > If there are finer mappings in the guest IOMMU these will be reported in > > the iotlb structures passed to the notifier which it must handle (probably > > causing it to flag an error). This isn't new - the VFIO iommu notifier > > must already handle notifications about guest IOMMU mappings too short > > for it to represent in the host IOMMU. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > include/exec/memory.h | 16 ++++++++++++++++ > > memory.c | 18 ++++++++++++++++++ > > 2 files changed, 34 insertions(+) > > > diff --git a/memory.c b/memory.c > > index 0d8b2d9..6b5a2f1 100644 > > --- a/memory.c > > +++ b/memory.c > > @@ -1403,6 +1403,24 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > > notifier_list_add(&mr->iommu_notify, n); > > } > > > > +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, > > + hwaddr granularity, bool is_write) > > granularity itself is not an address, but a size, isn't it? So using > "hwaddr" sounds wrong here. As Laurent says, hwaddr is used for sizes through memory.c - I think the rationale is that a size is a difference between two hwaddrs, so should have the same type. > > +{ > > + hwaddr addr; > > dma_addr_t ? As mentioned in previus reply, I don't think so on consideration. > > + IOMMUTLBEntry iotlb; > > + > > + memory_region_register_iommu_notifier(mr, n); > > + > > + for (addr = 0; > > + int128_lt(int128_make64(addr), mr->size); > > + addr += granularity) { > > + > > + iotlb = mr->iommu_ops->translate(mr, addr, is_write); > > + if (iotlb.perm != IOMMU_NONE) > > + n->notify(n, &iotlb); > > Missing curly braces. Thanks, fixed. > > > + } > > +} > > + > > Thomas >
On Wed, Sep 23, 2015 at 07:04:55PM +0200, Laurent Vivier wrote: > > > On 17/09/2015 15:09, David Gibson wrote: > > When we have guest visible IOMMUs, we allow notifiers to be registered > > which will be informed of all changes to IOMMU mappings. This is used by > > vfio to keep the host IOMMU mappings in sync with guest IOMMU mappings. > > > > However, unlike with a memory region listener, an iommu notifier won't be > > told about any mappings which already exist in the (guest) IOMMU at the > > time it is registered. This can cause problems if hotplugging a VFIO > > device onto a guest bus which had existing guest IOMMU mappings, but didn't > > previously have an VFIO devices (and hence no host IOMMU mappings). > > > > This adds a memory_region_register_iommu_notifier_replay() function to > > handle this case. As well as registering the new notifier it replays > > existing mappings. Because the IOMMU memory region doesn't internally > > remember the granularity of the guest IOMMU it has a small hack where the > > caller must specify a granularity at which to replay mappings. > > > > If there are finer mappings in the guest IOMMU these will be reported in > > the iotlb structures passed to the notifier which it must handle (probably > > causing it to flag an error). This isn't new - the VFIO iommu notifier > > must already handle notifications about guest IOMMU mappings too short > > for it to represent in the host IOMMU. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > include/exec/memory.h | 16 ++++++++++++++++ > > memory.c | 18 ++++++++++++++++++ > > 2 files changed, 34 insertions(+) > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index 5baaf48..3cf145b 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -583,6 +583,22 @@ void memory_region_notify_iommu(MemoryRegion *mr, > > void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); > > > > /** > > + * memory_region_register_iommu_notifier_replay: register a notifier > > + * for changes to IOMMU translation entries, and replay existing IOMMU > > + * translations to the new notifier. > > + * > > + * @mr: the memory region to observe > > + * @n: the notifier to be added; the notifier receives a pointer to an > > + * #IOMMUTLBEntry as the opaque value; the pointer ceases to be > > + * valid on exit from the notifier. > > + * @granularity: Minimum page granularity to replay notifications for > > + * @is_write: Whether to treat the replay as a translate "write" > > + * through the iommu > > + */ > > +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, > > + hwaddr granularity, bool is_write); > > + > > +/** > > * memory_region_unregister_iommu_notifier: unregister a notifier for > > * changes to IOMMU translation entries. > > * > > diff --git a/memory.c b/memory.c > > index 0d8b2d9..6b5a2f1 100644 > > --- a/memory.c > > +++ b/memory.c > > @@ -1403,6 +1403,24 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) > > notifier_list_add(&mr->iommu_notify, n); > > } > > > > +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, > > + hwaddr granularity, bool is_write) > > +{ > > + hwaddr addr; > > + IOMMUTLBEntry iotlb; > > + > > + memory_region_register_iommu_notifier(mr, n); > > + > > + for (addr = 0; > > + int128_lt(int128_make64(addr), mr->size); > > "addr < memory_region_size(mr)" should be enough. Ah, yes, much neater, thanks. > > + addr += granularity) { > > + > > + iotlb = mr->iommu_ops->translate(mr, addr, is_write); > > + if (iotlb.perm != IOMMU_NONE) > > + n->notify(n, &iotlb); > > + } > > +} > > + > > void memory_region_unregister_iommu_notifier(Notifier *n) > > { > > notifier_remove(n); > > >
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 24/09/2015 01:50, David Gibson wrote: > On Wed, Sep 23, 2015 at 07:04:55PM +0200, Laurent Vivier wrote: >> >> >> On 17/09/2015 15:09, David Gibson wrote: >>> When we have guest visible IOMMUs, we allow notifiers to be >>> registered which will be informed of all changes to IOMMU >>> mappings. This is used by vfio to keep the host IOMMU mappings >>> in sync with guest IOMMU mappings. >>> >>> However, unlike with a memory region listener, an iommu >>> notifier won't be told about any mappings which already exist >>> in the (guest) IOMMU at the time it is registered. This can >>> cause problems if hotplugging a VFIO device onto a guest bus >>> which had existing guest IOMMU mappings, but didn't previously >>> have an VFIO devices (and hence no host IOMMU mappings). >>> >>> This adds a memory_region_register_iommu_notifier_replay() >>> function to handle this case. As well as registering the new >>> notifier it replays existing mappings. Because the IOMMU >>> memory region doesn't internally remember the granularity of >>> the guest IOMMU it has a small hack where the caller must >>> specify a granularity at which to replay mappings. >>> >>> If there are finer mappings in the guest IOMMU these will be >>> reported in the iotlb structures passed to the notifier which >>> it must handle (probably causing it to flag an error). This >>> isn't new - the VFIO iommu notifier must already handle >>> notifications about guest IOMMU mappings too short for it to >>> represent in the host IOMMU. >>> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- >>> include/exec/memory.h | 16 ++++++++++++++++ memory.c >>> | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+) >>> >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index 5baaf48..3cf145b 100644 --- a/include/exec/memory.h +++ >>> b/include/exec/memory.h @@ -583,6 +583,22 @@ void >>> memory_region_notify_iommu(MemoryRegion *mr, void >>> memory_region_register_iommu_notifier(MemoryRegion *mr, >>> Notifier *n); >>> >>> /** + * memory_region_register_iommu_notifier_replay: register >>> a notifier + * for changes to IOMMU translation entries, and >>> replay existing IOMMU + * translations to the new notifier. + >>> * + * @mr: the memory region to observe + * @n: the notifier to >>> be added; the notifier receives a pointer to an + * >>> #IOMMUTLBEntry as the opaque value; the pointer ceases to be + >>> * valid on exit from the notifier. + * @granularity: >>> Minimum page granularity to replay notifications for + * >>> @is_write: Whether to treat the replay as a translate "write" + >>> * through the iommu + */ +void >>> memory_region_register_iommu_notifier_replay(MemoryRegion *mr, >>> Notifier *n, + >>> hwaddr granularity, bool is_write); + +/** * >>> memory_region_unregister_iommu_notifier: unregister a notifier >>> for * changes to IOMMU translation entries. * diff --git >>> a/memory.c b/memory.c index 0d8b2d9..6b5a2f1 100644 --- >>> a/memory.c +++ b/memory.c @@ -1403,6 +1403,24 @@ void >>> memory_region_register_iommu_notifier(MemoryRegion *mr, >>> Notifier *n) notifier_list_add(&mr->iommu_notify, n); } >>> >>> +void memory_region_register_iommu_notifier_replay(MemoryRegion >>> *mr, Notifier *n, + >>> hwaddr granularity, bool is_write) +{ + hwaddr addr; + >>> IOMMUTLBEntry iotlb; + + >>> memory_region_register_iommu_notifier(mr, n); + + for (addr >>> = 0; + int128_lt(int128_make64(addr), mr->size); >> >> "addr < memory_region_size(mr)" should be enough. > > Ah, yes, much neater, thanks. but rethinking about that, you can have an infinite loop (with int128 or with memory_region_size()) if mr->size >= UINT64_MAX: as hwaddr is a 64bit and a multiple of granularity which is a power of two. the last value of addr is UINT64 + 1 - granularity, so the next is (uint64_t)(UINT64 + 1), which is 0, so addr is never >= mr->size. > >>> + addr += granularity) { + + iotlb = >>> mr->iommu_ops->translate(mr, addr, is_write); + if >>> (iotlb.perm != IOMMU_NONE) + n->notify(n, &iotlb); + >>> } +} + void memory_region_unregister_iommu_notifier(Notifier >>> *n) { notifier_remove(n); >>> >> > -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAlYDob0ACgkQNKT2yavzbFOnFACcDk+2PHhX/WfCCkTdXKH4XhWi UYcAoOpe+C+8tzX02VlGTsCAV9ZxiEwQ =Cnfl -----END PGP SIGNATURE-----
diff --git a/include/exec/memory.h b/include/exec/memory.h index 5baaf48..3cf145b 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -583,6 +583,22 @@ void memory_region_notify_iommu(MemoryRegion *mr, void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n); /** + * memory_region_register_iommu_notifier_replay: register a notifier + * for changes to IOMMU translation entries, and replay existing IOMMU + * translations to the new notifier. + * + * @mr: the memory region to observe + * @n: the notifier to be added; the notifier receives a pointer to an + * #IOMMUTLBEntry as the opaque value; the pointer ceases to be + * valid on exit from the notifier. + * @granularity: Minimum page granularity to replay notifications for + * @is_write: Whether to treat the replay as a translate "write" + * through the iommu + */ +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, + hwaddr granularity, bool is_write); + +/** * memory_region_unregister_iommu_notifier: unregister a notifier for * changes to IOMMU translation entries. * diff --git a/memory.c b/memory.c index 0d8b2d9..6b5a2f1 100644 --- a/memory.c +++ b/memory.c @@ -1403,6 +1403,24 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n) notifier_list_add(&mr->iommu_notify, n); } +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, Notifier *n, + hwaddr granularity, bool is_write) +{ + hwaddr addr; + IOMMUTLBEntry iotlb; + + memory_region_register_iommu_notifier(mr, n); + + for (addr = 0; + int128_lt(int128_make64(addr), mr->size); + addr += granularity) { + + iotlb = mr->iommu_ops->translate(mr, addr, is_write); + if (iotlb.perm != IOMMU_NONE) + n->notify(n, &iotlb); + } +} + void memory_region_unregister_iommu_notifier(Notifier *n) { notifier_remove(n);
When we have guest visible IOMMUs, we allow notifiers to be registered which will be informed of all changes to IOMMU mappings. This is used by vfio to keep the host IOMMU mappings in sync with guest IOMMU mappings. However, unlike with a memory region listener, an iommu notifier won't be told about any mappings which already exist in the (guest) IOMMU at the time it is registered. This can cause problems if hotplugging a VFIO device onto a guest bus which had existing guest IOMMU mappings, but didn't previously have an VFIO devices (and hence no host IOMMU mappings). This adds a memory_region_register_iommu_notifier_replay() function to handle this case. As well as registering the new notifier it replays existing mappings. Because the IOMMU memory region doesn't internally remember the granularity of the guest IOMMU it has a small hack where the caller must specify a granularity at which to replay mappings. If there are finer mappings in the guest IOMMU these will be reported in the iotlb structures passed to the notifier which it must handle (probably causing it to flag an error). This isn't new - the VFIO iommu notifier must already handle notifications about guest IOMMU mappings too short for it to represent in the host IOMMU. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- include/exec/memory.h | 16 ++++++++++++++++ memory.c | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+)