Re: [PATCH 3/4] iommu/amd: Do not sync on page size changes

From: Robin Murphy
Date: Tue Jun 01 2021 - 13:27:38 EST

On 2021-06-01 17:39, Nadav Amit wrote:


On Jun 1, 2021, at 8:59 AM, Robin Murphy <robin.murphy@xxxxxxx> wrote:

On 2021-05-02 07:59, Nadav Amit wrote:
From: Nadav Amit <namit@xxxxxxxxxx>
Some IOMMU architectures perform invalidations regardless of the page
size. In such architectures there is no need to sync when the page size
changes or to regard pgsize when making interim flush in
iommu_iotlb_gather_add_page().
Add a "ignore_gather_pgsize" property for each IOMMU-ops to decide
whether gather's pgsize is tracked and triggers a flush.

I've objected before[1], and I'll readily object again ;)

I still think it's very silly to add a bunch of indirection all over the place to make a helper function not do the main thing it's intended to help with. If you only need trivial address gathering then it's far simpler to just implement trivial address gathering. I suppose if you really want to you could factor out another helper to share the 5 lines of code which ended up in mtk-iommu (see commit f21ae3b10084).

Thanks, Robin.

I read your comments but I cannot fully understand the alternative that you propose, although I do understand your objection to the indirection “ignore_gather_pgsize”. Would it be ok if “ignore_gather_pgsize" was provided as an argument for iommu_iotlb_gather_add_page()?

No, I mean if iommu_iotlb_gather_add_page() doesn't have the behaviour your driver wants, just don't call it. Write or factor out a suitable helper that *does* do what you want and call that, or just implement the logic directly inline. Indirect argument or not, it just doesn't make much sense to have a helper function call which says "do this except don't do most of it".

In general, I can live without this patch. It probably would have negligent impact on performance anyhow.

As I implied, it sounds like your needs are the same as the Mediatek driver had, so you could readily factor out a new page-size-agnostic gather helper from that. I fully support making the functional change to amd-iommu *somehow* - nobody likes unnecessary syncs - just not with this particular implementation :)

Robin.