| Message ID | 1493201210-14357-13-git-send-email-yi.l.liu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
On 26/04/2017 12:06, Liu, Yi L wrote: > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > + void *data) > +{ > + IOMMUNotifier *iommu_notifier; > + IOMMUNotifierFlag request_flags; > + > + assert(memory_region_is_iommu(mr)); > + > + /*TODO: support other bind requests with smaller gran, > + * e.g. bind signle pasid entry > + */ > + request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > + > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > + if (iommu_notifier->notifier_flags & request_flags) { > + iommu_notifier->notify(iommu_notifier, data); > + break; > + } > + } Peter, should this reuse ->notify, or should it be different function pointer in IOMMUNotifier? Paolo
On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > + void *data) > > +{ > > + IOMMUNotifier *iommu_notifier; > > + IOMMUNotifierFlag request_flags; > > + > > + assert(memory_region_is_iommu(mr)); > > + > > + /*TODO: support other bind requests with smaller gran, > > + * e.g. bind signle pasid entry > > + */ > > + request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > + > > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > + if (iommu_notifier->notifier_flags & request_flags) { > > + iommu_notifier->notify(iommu_notifier, data); > > + break; > > + } > > + } > > Peter, > > should this reuse ->notify, or should it be different function pointer > in IOMMUNotifier? Hi Paolo, Thx for your review. I think it should be “->notify” here. In this patchset, the new notifier is registered with the existing notifier registration API. So the all the notifiers are in the mr->iommu_notify list. And notifiers are labeled by notify flag, so it is able to differentiate the IOMMUNotifier nodes. When the flag meets, trigger it by “->notify”. The diagram below shows my understanding , wish it helps to make me understood. VFIOContainer | giommu_list(VFIOGuestIOMMU) \ VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 ... | | | mr->iommu_notify: IOMMUNotifier -> IOMMUNotifier -> IOMMUNotifier (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb invalidate) Actually, compared with the MAP/UNMAP notifier, the newly added notifier has no start/end check, and there may be other types of bind notfier flag in future, so I added a separate fire func for SVM bind notifier. Thanks, Yi L > Paolo >
On Thu, Apr 27, 2017 at 10:37:19AM +0800, Liu, Yi L wrote: > On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > > + void *data) > > > +{ > > > + IOMMUNotifier *iommu_notifier; > > > + IOMMUNotifierFlag request_flags; > > > + > > > + assert(memory_region_is_iommu(mr)); > > > + > > > + /*TODO: support other bind requests with smaller gran, > > > + * e.g. bind signle pasid entry > > > + */ > > > + request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > > + > > > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > + if (iommu_notifier->notifier_flags & request_flags) { > > > + iommu_notifier->notify(iommu_notifier, data); > > > + break; > > > + } > > > + } > > > > Peter, > > > > should this reuse ->notify, or should it be different function pointer > > in IOMMUNotifier? > > Hi Paolo, > > Thx for your review. > > I think it should be “->notify” here. In this patchset, the new notifier > is registered with the existing notifier registration API. So the all the > notifiers are in the mr->iommu_notify list. And notifiers are labeled > by notify flag, so it is able to differentiate the IOMMUNotifier nodes. > When the flag meets, trigger it by “->notify”. The diagram below shows > my understanding , wish it helps to make me understood. > > VFIOContainer > | > giommu_list(VFIOGuestIOMMU) > \ > VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 ... > | | | > mr->iommu_notify: IOMMUNotifier -> IOMMUNotifier -> IOMMUNotifier > (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb invalidate) > > > Actually, compared with the MAP/UNMAP notifier, the newly added notifier has > no start/end check, and there may be other types of bind notfier flag in > future, so I added a separate fire func for SVM bind notifier. I agree with Paolo that this interface might not be the suitable place for the SVM notifiers (just like what I worried about in previous discussions). The biggest problem is that, if you see current notifier mechanism, it's per-memory-region. However iiuc your messages should be per-iommu, or say, per translation unit. While, for each iommu, there can be more than one memory regions (ppc can be an example). When there are more than one MRs binded to the same iommu unit, which memory region should you register to? Any one of them, or all? So my conclusion is, it just has nothing to do with memory regions... Instead of a different function pointer in IOMMUNotifer, IMHO we can even move a step further, to isolate IOTLB notifications (targeted at memory regions and with start/end ranges) out of SVM/other notifications, since they are different in general. So we basically need two notification mechanism: - one for memory regions, currently what I can see is IOTLB notifications - one for translation units, currently I see all the rest of notifications needed in virt-svm in this category Maybe some RFC patches would be good to show what I mean... I'll see whether I can prepare some. Thanks,
On Thu, Apr 27, 2017 at 02:14:27PM +0800, Peter Xu wrote: > On Thu, Apr 27, 2017 at 10:37:19AM +0800, Liu, Yi L wrote: > > On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > > > > > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > > > + void *data) > > > > +{ > > > > + IOMMUNotifier *iommu_notifier; > > > > + IOMMUNotifierFlag request_flags; > > > > + > > > > + assert(memory_region_is_iommu(mr)); > > > > + > > > > + /*TODO: support other bind requests with smaller gran, > > > > + * e.g. bind signle pasid entry > > > > + */ > > > > + request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > > > + > > > > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > > + if (iommu_notifier->notifier_flags & request_flags) { > > > > + iommu_notifier->notify(iommu_notifier, data); > > > > + break; > > > > + } > > > > + } > > > > > > Peter, > > > > > > should this reuse ->notify, or should it be different function pointer > > > in IOMMUNotifier? > > > > Hi Paolo, > > > > Thx for your review. > > > > I think it should be “->notify” here. In this patchset, the new notifier > > is registered with the existing notifier registration API. So the all the > > notifiers are in the mr->iommu_notify list. And notifiers are labeled > > by notify flag, so it is able to differentiate the IOMMUNotifier nodes. > > When the flag meets, trigger it by “->notify”. The diagram below shows > > my understanding , wish it helps to make me understood. > > > > VFIOContainer > > | > > giommu_list(VFIOGuestIOMMU) > > \ > > VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 ... > > | | | > > mr->iommu_notify: IOMMUNotifier -> IOMMUNotifier -> IOMMUNotifier > > (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb invalidate) > > > > > > Actually, compared with the MAP/UNMAP notifier, the newly added notifier has > > no start/end check, and there may be other types of bind notfier flag in > > future, so I added a separate fire func for SVM bind notifier. > > I agree with Paolo that this interface might not be the suitable place > for the SVM notifiers (just like what I worried about in previous > discussions). > > The biggest problem is that, if you see current notifier mechanism, > it's per-memory-region. However iiuc your messages should be > per-iommu, or say, per translation unit. While, for each iommu, there > can be more than one memory regions (ppc can be an example). When > there are more than one MRs binded to the same iommu unit, which > memory region should you register to? Any one of them, or all? > > So my conclusion is, it just has nothing to do with memory regions... > > Instead of a different function pointer in IOMMUNotifer, IMHO we can > even move a step further, to isolate IOTLB notifications (targeted at > memory regions and with start/end ranges) out of SVM/other > notifications, since they are different in general. So we basically > need two notification mechanism: > > - one for memory regions, currently what I can see is IOTLB > notifications > > - one for translation units, currently I see all the rest of > notifications needed in virt-svm in this category > > Maybe some RFC patches would be good to show what I mean... I'll see > whether I can prepare some. Here it is (on qemu-devel): [RFC PATCH 0/8] IOMMU: introduce common IOMMUObject Thanks,
On Thu, Apr 27, 2017 at 02:14:27PM +0800, Peter Xu wrote: > On Thu, Apr 27, 2017 at 10:37:19AM +0800, Liu, Yi L wrote: > > On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > > > > > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > > > + void *data) > > > > +{ > > > > + IOMMUNotifier *iommu_notifier; > > > > + IOMMUNotifierFlag request_flags; > > > > + > > > > + assert(memory_region_is_iommu(mr)); > > > > + > > > > + /*TODO: support other bind requests with smaller gran, > > > > + * e.g. bind signle pasid entry > > > > + */ > > > > + request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > > > + > > > > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > > + if (iommu_notifier->notifier_flags & request_flags) { > > > > + iommu_notifier->notify(iommu_notifier, data); > > > > + break; > > > > + } > > > > + } > > > > > > Peter, > > > > > > should this reuse ->notify, or should it be different function pointer > > > in IOMMUNotifier? > > > > Hi Paolo, > > > > Thx for your review. > > > > I think it should be “->notify” here. In this patchset, the new notifier > > is registered with the existing notifier registration API. So the all the > > notifiers are in the mr->iommu_notify list. And notifiers are labeled > > by notify flag, so it is able to differentiate the IOMMUNotifier nodes. > > When the flag meets, trigger it by “->notify”. The diagram below shows > > my understanding , wish it helps to make me understood. > > > > VFIOContainer > > | > > giommu_list(VFIOGuestIOMMU) > > \ > > VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 ... > > | | | > > mr->iommu_notify: IOMMUNotifier -> IOMMUNotifier -> IOMMUNotifier > > (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb invalidate) > > > > > > Actually, compared with the MAP/UNMAP notifier, the newly added notifier has > > no start/end check, and there may be other types of bind notfier flag in > > future, so I added a separate fire func for SVM bind notifier. > > I agree with Paolo that this interface might not be the suitable place > for the SVM notifiers (just like what I worried about in previous > discussions). > > The biggest problem is that, if you see current notifier mechanism, > it's per-memory-region. However iiuc your messages should be > per-iommu, or say, per translation unit. Hi Peter, yes, you're right. the newly added notifier is per-iommu. > While, for each iommu, there > can be more than one memory regions (ppc can be an example). When > there are more than one MRs binded to the same iommu unit, which > memory region should you register to? Any one of them, or all? Honestly, I'm not expert on ppc. According to the current code, I can only find one MR initialized with memory_region_init_iommu() in spapr_tce_table_realize(). So to better get your point, let me check. Do you mean there may be multiple of iommu MRs behind a iommu? I admit it must be considered if there are multiple iommu MRs. I may choose to register for one of them since the notifier is per-iommu as you've pointed. Then vIOMMU emulator need to trigger the notifier with the correct MR. Not sure if ppc vIOMMU is fine with it. > So my conclusion is, it just has nothing to do with memory regions... > > Instead of a different function pointer in IOMMUNotifer, IMHO we can > even move a step further, to isolate IOTLB notifications (targeted at > memory regions and with start/end ranges) out of SVM/other > notifications, since they are different in general. So we basically > need two notification mechanism: > > - one for memory regions, currently what I can see is IOTLB > notifications > > - one for translation units, currently I see all the rest of > notifications needed in virt-svm in this category > > Maybe some RFC patches would be good to show what I mean... I'll see > whether I can prepare some. I agree that it would be helpful to split the two kinds of notifiers. I marked it as a FIXME in patch 0006 of this series. Just saw your RFC patch for common IOMMUObject. Thx for your work, would try to review it. Besides the notifier registration, pls also help to review the SVM virtualization itself. Would be glad to know your comments. Thanks, Yi L > Thanks, > > -- > Peter Xu >
On Thu, Apr 27, 2017 at 06:25:37PM +0800, Liu, Yi L wrote: > On Thu, Apr 27, 2017 at 02:14:27PM +0800, Peter Xu wrote: > > On Thu, Apr 27, 2017 at 10:37:19AM +0800, Liu, Yi L wrote: > > > On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > > > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > > > > + void *data) > > > > > +{ > > > > > + IOMMUNotifier *iommu_notifier; > > > > > + IOMMUNotifierFlag request_flags; > > > > > + > > > > > + assert(memory_region_is_iommu(mr)); > > > > > + > > > > > + /*TODO: support other bind requests with smaller gran, > > > > > + * e.g. bind signle pasid entry > > > > > + */ > > > > > + request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > > > > + > > > > > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > > > + if (iommu_notifier->notifier_flags & request_flags) { > > > > > + iommu_notifier->notify(iommu_notifier, data); > > > > > + break; > > > > > + } > > > > > + } > > > > > > > > Peter, > > > > > > > > should this reuse ->notify, or should it be different function pointer > > > > in IOMMUNotifier? > > > > > > Hi Paolo, > > > > > > Thx for your review. > > > > > > I think it should be “->notify” here. In this patchset, the new notifier > > > is registered with the existing notifier registration API. So the all the > > > notifiers are in the mr->iommu_notify list. And notifiers are labeled > > > by notify flag, so it is able to differentiate the IOMMUNotifier nodes. > > > When the flag meets, trigger it by “->notify”. The diagram below shows > > > my understanding , wish it helps to make me understood. > > > > > > VFIOContainer > > > | > > > giommu_list(VFIOGuestIOMMU) > > > \ > > > VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 ... > > > | | | > > > mr->iommu_notify: IOMMUNotifier -> IOMMUNotifier -> IOMMUNotifier > > > (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb invalidate) > > > > > > > > > Actually, compared with the MAP/UNMAP notifier, the newly added notifier has > > > no start/end check, and there may be other types of bind notfier flag in > > > future, so I added a separate fire func for SVM bind notifier. > > > > I agree with Paolo that this interface might not be the suitable place > > for the SVM notifiers (just like what I worried about in previous > > discussions). > > > > The biggest problem is that, if you see current notifier mechanism, > > it's per-memory-region. However iiuc your messages should be > > per-iommu, or say, per translation unit. > > Hi Peter, > > yes, you're right. the newly added notifier is per-iommu. > > > While, for each iommu, there > > can be more than one memory regions (ppc can be an example). When > > there are more than one MRs binded to the same iommu unit, which > > memory region should you register to? Any one of them, or all? > > Honestly, I'm not expert on ppc. According to the current code, > I can only find one MR initialized with memory_region_init_iommu() > in spapr_tce_table_realize(). So to better get your point, let me > check. Do you mean there may be multiple of iommu MRs behind a iommu? I am not either. :) But yes, that's what I mean. At least that's how I understand it. > > I admit it must be considered if there are multiple iommu MRs. I may > choose to register for one of them since the notifier is per-iommu as > you've pointed. Then vIOMMU emulator need to trigger the notifier with > the correct MR. Not sure if ppc vIOMMU is fine with it. > > > So my conclusion is, it just has nothing to do with memory regions... > > > > Instead of a different function pointer in IOMMUNotifer, IMHO we can > > even move a step further, to isolate IOTLB notifications (targeted at > > memory regions and with start/end ranges) out of SVM/other > > notifications, since they are different in general. So we basically > > need two notification mechanism: > > > > - one for memory regions, currently what I can see is IOTLB > > notifications > > > > - one for translation units, currently I see all the rest of > > notifications needed in virt-svm in this category > > > > Maybe some RFC patches would be good to show what I mean... I'll see > > whether I can prepare some. > > I agree that it would be helpful to split the two kinds of notifiers. I > marked it as a FIXME in patch 0006 of this series. Just saw your RFC patch > for common IOMMUObject. Thx for your work, would try to review it. Thanks, looking forward to your review comments. > > Besides the notifier registration, pls also help to review the SVM > virtualization itself. Would be glad to know your comments. Yes. It's on my list. Thanks,
diff --git a/include/exec/memory.h b/include/exec/memory.h index 49087ef..3b8f487 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -695,6 +695,17 @@ uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr); void memory_region_notify_iommu(MemoryRegion *mr, IOMMUTLBEntry entry); +/* + * memory_region_notify_iommu_svm_bind notify SVM bind + * request from vIOMMU emulator. + * + * @mr: the memory region of IOMMU + * @data: IOMMU SVM data + */ +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, + void *data); + + /** * memory_region_notify_one: notify a change in an IOMMU translation * entry to a single notifier diff --git a/memory.c b/memory.c index 45ef069..ce0b0ff 100644 --- a/memory.c +++ b/memory.c @@ -1729,6 +1729,27 @@ void memory_region_notify_iommu(MemoryRegion *mr, } } +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, + void *data) +{ + IOMMUNotifier *iommu_notifier; + IOMMUNotifierFlag request_flags; + + assert(memory_region_is_iommu(mr)); + + /*TODO: support other bind requests with smaller gran, + * e.g. bind signle pasid entry + */ + request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; + + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { + if (iommu_notifier->notifier_flags & request_flags) { + iommu_notifier->notify(iommu_notifier, data); + break; + } + } +} + void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client) { uint8_t mask = 1 << client;
Add a separate function to fire pasid table bind notifier. In future there may be more pasid bind type with different granularity. e.g. binding pasid entry instead of binding pasid table. It can be supported by adding bind_type, check bind_type in fire func and trigger correct notifier. Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com> --- include/exec/memory.h | 11 +++++++++++ memory.c | 21 +++++++++++++++++++++ 2 files changed, 32 insertions(+)