On 4/20/21 3:32 PM, Keqian Zhu wrote:
Hi Baolu,
Cheers for the your quick reply.
On 2021/4/20 10:09, Lu Baolu wrote:
Hi Keqian,
On 4/20/21 9:25 AM, Keqian Zhu wrote:
Hi Baolu,
On 2021/4/19 21:33, Lu Baolu wrote:
Hi Keqian,
On 2021/4/19 17:32, Keqian Zhu wrote:
+EXPORT_SYMBOL_GPL(iommu_split_block);
Do you really have any consumers of this interface other than the dirty
bit tracking? If not, I don't suggest to make this as a generic IOMMU
interface.
There is an implicit requirement for such interfaces. The
iommu_map/unmap(iova, size) shouldn't be called at the same time.
Currently there's no such sanity check in the iommu core. A poorly
written driver could mess up the kernel by misusing this interface.
Yes, I don't think up a scenario except dirty tracking.
Indeed, we'd better not make them as a generic interface.
Do you have any suggestion that underlying iommu drivers can share these code
but
not make it as a generic iommu interface?
I have a not so good idea. Make the "split" interfaces as a static function, and
transfer the function pointer to start_dirty_log. But it looks weird and
inflexible.
I understand splitting/merging super pages is an optimization, but not a
functional requirement. So is it possible to let the vendor iommu driver
decide whether splitting super pages when starting dirty bit tracking
and the opposite operation during when stopping it? The requirement for
Right. If I understand you correct, actually that is what this series does.
I mean to say no generic APIs, jut do it by the iommu subsystem itself.
It's totally transparent to the upper level, just like what map() does.
The upper layer doesn't care about either super page or small page is
in use when do a mapping, right?
If you want to consolidate some code, how about putting them in
start/stop_tracking()?
Yep, this reminds me. What we want to reuse is the logic of "chunk by chunk" in
split().
We can implement switch_dirty_log to be "chunk by chunk" too (just the same as
sync/clear),
then the vendor iommu driver can invoke it's own private implementation of
split().
So we can completely remove split() in the IOMMU core layer.
example code logic
iommu.c:
switch_dirty_log(big range) {
for_each_iommu_page(big range) {
ops->switch_dirty_log(iommu_pgsize)
}
}
vendor iommu driver:
switch_dirty_log(iommu_pgsize) {
if (enable) {
ops->split_block(iommu_pgsize)
/* And other actions, such as enable hardware capability */
} else {
for_each_continuous_physical_address(iommu_pgsize)
ops->merge_page()
}
}
Besides, vendor iommu driver can invoke split() in clear_dirty_log instead of
in switch_dirty_log.
The benefit is that we usually clear dirty log gradually during dirty tracking,
then we can split
large page mapping gradually, which speedup start_dirty_log and make less side
effect on DMA performance.
Does it looks good for you?
Yes. It's clearer now.
Thanks,
Keqian
Best regards,
baolu