| Message ID | 1484276800-26814-13-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2017年01月13日 11:06, Peter Xu wrote: > Before this one we only invalidate context cache when we receive context > entry invalidations. However it's possible that the invalidation also > contains a domain switch (only if cache-mode is enabled for vIOMMU). So let's check for CM before replaying? > In > that case we need to notify all the registered components about the new > mapping. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/i386/intel_iommu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 59bf683..fd75112 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1162,6 +1162,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s, > trace_vtd_inv_desc_cc_device(bus_n, (devfn_it >> 3) & 0x1f, > devfn_it & 3); > vtd_as->context_cache_entry.context_cache_gen = 0; > + memory_region_iommu_replay_all(&vtd_as->iommu); > } > } > }
On Mon, Jan 16, 2017 at 01:53:54PM +0800, Jason Wang wrote: > > > On 2017年01月13日 11:06, Peter Xu wrote: > >Before this one we only invalidate context cache when we receive context > >entry invalidations. However it's possible that the invalidation also > >contains a domain switch (only if cache-mode is enabled for vIOMMU). > > So let's check for CM before replaying? When CM is not set, there should have no device needs IOMMU_NOTIFIER_MAP notifies. So IMHO it won't hurt if we replay here (so the notifier_list will only contain UNMAP notifiers at most, and sending UNMAP to those devices should not affect them at all). If we check CM before replay, it'll be faster when guest change iommu domain for a specific device. But after all this kind of operation is extremely rare, while if we check CM bit, we have a "assumption" in the code that MAP is depending on CM. In that case, to make the codes cleaner, I'd slightly prefer not check it here. How do you think? Thanks, -- peterx
On 2017年01月16日 15:43, Peter Xu wrote: > On Mon, Jan 16, 2017 at 01:53:54PM +0800, Jason Wang wrote: >> >> On 2017年01月13日 11:06, Peter Xu wrote: >>> Before this one we only invalidate context cache when we receive context >>> entry invalidations. However it's possible that the invalidation also >>> contains a domain switch (only if cache-mode is enabled for vIOMMU). >> So let's check for CM before replaying? > When CM is not set, there should have no device needs > IOMMU_NOTIFIER_MAP notifies. So IMHO it won't hurt if we replay here > (so the notifier_list will only contain UNMAP notifiers at most, and > sending UNMAP to those devices should not affect them at all). > > If we check CM before replay, it'll be faster when guest change iommu > domain for a specific device. But after all this kind of operation is > extremely rare, while if we check CM bit, we have a "assumption" in > the code that MAP is depending on CM. In that case, to make the codes > cleaner, I'd slightly prefer not check it here. How do you think? Ok, I think maybe it's better to add a comment here. Thanks > > Thanks, > > -- peterx >
On Mon, Jan 16, 2017 at 03:52:10PM +0800, Jason Wang wrote: > > > On 2017年01月16日 15:43, Peter Xu wrote: > >On Mon, Jan 16, 2017 at 01:53:54PM +0800, Jason Wang wrote: > >> > >>On 2017年01月13日 11:06, Peter Xu wrote: > >>>Before this one we only invalidate context cache when we receive context > >>>entry invalidations. However it's possible that the invalidation also > >>>contains a domain switch (only if cache-mode is enabled for vIOMMU). > >>So let's check for CM before replaying? > >When CM is not set, there should have no device needs > >IOMMU_NOTIFIER_MAP notifies. So IMHO it won't hurt if we replay here > >(so the notifier_list will only contain UNMAP notifiers at most, and > >sending UNMAP to those devices should not affect them at all). > > > >If we check CM before replay, it'll be faster when guest change iommu > >domain for a specific device. But after all this kind of operation is > >extremely rare, while if we check CM bit, we have a "assumption" in > >the code that MAP is depending on CM. In that case, to make the codes > >cleaner, I'd slightly prefer not check it here. How do you think? > > Ok, I think maybe it's better to add a comment here. Will do it. Thanks! -- peterx
On Mon, Jan 16, 2017 at 03:52:10PM +0800, Jason Wang wrote: > > > On 2017年01月16日 15:43, Peter Xu wrote: > >On Mon, Jan 16, 2017 at 01:53:54PM +0800, Jason Wang wrote: > >> > >>On 2017年01月13日 11:06, Peter Xu wrote: > >>>Before this one we only invalidate context cache when we receive context > >>>entry invalidations. However it's possible that the invalidation also > >>>contains a domain switch (only if cache-mode is enabled for vIOMMU). > >>So let's check for CM before replaying? > >When CM is not set, there should have no device needs > >IOMMU_NOTIFIER_MAP notifies. So IMHO it won't hurt if we replay here > >(so the notifier_list will only contain UNMAP notifiers at most, and > >sending UNMAP to those devices should not affect them at all). > > > >If we check CM before replay, it'll be faster when guest change iommu > >domain for a specific device. But after all this kind of operation is > >extremely rare, while if we check CM bit, we have a "assumption" in > >the code that MAP is depending on CM. In that case, to make the codes > >cleaner, I'd slightly prefer not check it here. How do you think? > > Ok, I think maybe it's better to add a comment here. How about this? + /* + * So a device is moving out of (or moving into) a + * domain, a replay() suites here to notify all the + * IOMMU_NOTIFIER_MAP registers about this change. + * This won't bring bad even if we have no such + * notifier registered - the IOMMU notification + * framework will skip MAP notifications if that + * happened. + */ memory_region_iommu_replay_all(&vtd_as->iommu); Thanks, -- peterx
On 2017年01月16日 16:18, Peter Xu wrote: > On Mon, Jan 16, 2017 at 03:52:10PM +0800, Jason Wang wrote: >> >> On 2017年01月16日 15:43, Peter Xu wrote: >>> On Mon, Jan 16, 2017 at 01:53:54PM +0800, Jason Wang wrote: >>>> On 2017年01月13日 11:06, Peter Xu wrote: >>>>> Before this one we only invalidate context cache when we receive context >>>>> entry invalidations. However it's possible that the invalidation also >>>>> contains a domain switch (only if cache-mode is enabled for vIOMMU). >>>> So let's check for CM before replaying? >>> When CM is not set, there should have no device needs >>> IOMMU_NOTIFIER_MAP notifies. So IMHO it won't hurt if we replay here >>> (so the notifier_list will only contain UNMAP notifiers at most, and >>> sending UNMAP to those devices should not affect them at all). >>> >>> If we check CM before replay, it'll be faster when guest change iommu >>> domain for a specific device. But after all this kind of operation is >>> extremely rare, while if we check CM bit, we have a "assumption" in >>> the code that MAP is depending on CM. In that case, to make the codes >>> cleaner, I'd slightly prefer not check it here. How do you think? >> Ok, I think maybe it's better to add a comment here. > How about this? > > + /* > + * So a device is moving out of (or moving into) a > + * domain, a replay() suites here to notify all the > + * IOMMU_NOTIFIER_MAP registers about this change. > + * This won't bring bad even if we have no such > + * notifier registered - the IOMMU notification > + * framework will skip MAP notifications if that > + * happened. > + */ > memory_region_iommu_replay_all(&vtd_as->iommu); > > Thanks, > > -- peterx I'm fine with this. Thanks
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 59bf683..fd75112 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1162,6 +1162,7 @@ static void vtd_context_device_invalidate(IntelIOMMUState *s, trace_vtd_inv_desc_cc_device(bus_n, (devfn_it >> 3) & 0x1f, devfn_it & 3); vtd_as->context_cache_entry.context_cache_gen = 0; + memory_region_iommu_replay_all(&vtd_as->iommu); } } }
Before this one we only invalidate context cache when we receive context entry invalidations. However it's possible that the invalidation also contains a domain switch (only if cache-mode is enabled for vIOMMU). In that case we need to notify all the registered components about the new mapping. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/intel_iommu.c | 1 + 1 file changed, 1 insertion(+)