Hi Geert,

On 2021-08-24 14:25, Geert Uytterhoeven wrote:
Hi Robin,

On Wed, Aug 11, 2021 at 2:24 PM Robin Murphy <robin.mur...@arm.com> wrote:
IO_PGTABLE_QUIRK_NON_STRICT was never a very comfortable fit, since it's
not a quirk of the pagetable format itself. Now that we have a more
appropriate way to convey non-strict unmaps, though, this last of the
non-quirk quirks can also go, and with the flush queue code also now
enforcing its own ordering we can have a lovely cleanup all round.

Signed-off-by: Robin Murphy <robin.mur...@arm.com>

Thanks for your patch, which is now commit a8e5f04458c4e496
("iommu/io-pgtable: Remove non-strict quirk") in iommu/next.

--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -700,14 +700,7 @@ static size_t __arm_v7s_unmap(struct arm_v7s_io_pgtable 
*data,
                                                 ARM_V7S_BLOCK_SIZE(lvl + 1));
                                 ptep = iopte_deref(pte[i], lvl, data);
                                 __arm_v7s_free_table(ptep, lvl + 1, data);
-                       } else if (iop->cfg.quirks & 
IO_PGTABLE_QUIRK_NON_STRICT) {
-                               /*
-                                * Order the PTE update against queueing the 
IOVA, to
-                                * guarantee that a flush callback from a 
different CPU
-                                * has observed it before the TLBIALL can be 
issued.
-                                */
-                               smp_wmb();
-                       } else {
+                       } else if (!gather->queued) {

If CONFIG_IOMMU_API=n:

error: ‘struct iommu_iotlb_gather’ has no member named ‘queued’

This can be reproduced using e.g. shmobile_defconfig with
     CONFIG_IOMMU_SUPPORT=y
     CONFIG_IOMMU_IO_PGTABLE_ARMV7S=y


                                 io_pgtable_tlb_add_page(iop, gather, iova, 
blk_size);
                         }
                         iova += blk_size;

--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -638,14 +638,7 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable 
*data,
                                 io_pgtable_tlb_flush_walk(iop, iova + i * 
size, size,
                                                           
ARM_LPAE_GRANULE(data));
                                 __arm_lpae_free_pgtable(data, lvl + 1, 
iopte_deref(pte, data));
-                       } else if (iop->cfg.quirks & 
IO_PGTABLE_QUIRK_NON_STRICT) {
-                               /*
-                                * Order the PTE update against queueing the 
IOVA, to
-                                * guarantee that a flush callback from a 
different CPU
-                                * has observed it before the TLBIALL can be 
issued.
-                                */
-                               smp_wmb();
-                       } else {
+                       } else if (!gather->queued) {

If CONFIG_IOMMU_API=n:

error: ‘struct iommu_iotlb_gather’ has no member named ‘queued’

This can be reproduced using e.g. shmobile_defconfig with
     CONFIG_IOMMU_SUPPORT=y
     CONFIG_IOMMU_IO_PGTABLE_LPAE=y

                                 io_pgtable_tlb_add_page(iop, gather, iova + i 
* size, size);
                         }


Perhaps "select IOMMU_API" should be added (moved from individual
drivers) to both IOMMU_IO_PGTABLE_ARMV7S and IOMMU_IO_PGTABLE_LPAE?
Or iommu_iotlb_gather.queued should not be accessed here, or the
access wrapped into a static inline helper function with a dummy for
the CONFIG_IOMMU_API=n case?

Those (and worse) should be fixed by this patch:

https://lore.kernel.org/linux-iommu/83672ee76f6405c82845a55c148fa836f56fbbc1.1629465282.git.robin.mur...@arm.com/

which apparently hasn't made it to -next yet.

Thanks,
Robin.


Gr{oetje,eeting}s,

                         Geert

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Reply via email to