Re: [PATCH 02/13] iommu/io-pgtable-arm: Remove redundant call to io_pgtable_tlb_sync()
On 15/08/2019 14:57, Will Deacon wrote:
Hi Robin,
On Thu, Aug 15, 2019 at 01:43:11PM +0100, Robin Murphy wrote:
On 14/08/2019 18:56, Will Deacon wrote:
Commit b6b65ca20bc9 ("iommu/io-pgtable-arm: Add support for non-strict
mode") added an unconditional call to io_pgtable_tlb_sync() immediately
after the case where we replace a block entry with a table entry during
an unmap() call. This is redundant, since the IOMMU API will call
iommu_tlb_sync() on this path and the patch in question mentions this:
| To save having to reason about it too much, make sure the invalidation
| in arm_lpae_split_blk_unmap() just performs its own unconditional sync
| to minimise the window in which we're technically violating the break-
| before-make requirement on a live mapping. This might work out redundant
| with an outer-level sync for strict unmaps, but we'll never be splitting
| blocks on a DMA fastpath anyway.
However, this sync gets in the way of deferred TLB invalidation for leaf
entries and is at best a questionable, unproven hack. Remove it.
Hey, that's my questionable, unproven hack! :P
I thought you'd like to remain anonymous, but I can credit you if you like?
;)
It's not entirely clear to me how this gets in the way though - AFAICS the
intent of tlb_flush_leaf exactly matches the desired operation here, so
couldn't these just wait to be converted in patch #8?
Good point. I think there are two things:
1. Initially, I didn't plan to have tlb_flush_leaf() at all because
I didn't think it would be needed. Then I ran into the v7s CONT
stuff and ended up needing it after all (I think it's the only
user). So that's an oversight.
2. If we do the tlb_flush_leaf() here, then we could potentially
put a hole in the ongoing gather structure, but I suppose we
could do both a tlb_add_page() *and* a tlb_flush_leaf() to get
around that.
So yes, I probably could move this back if the sync is necessary but...
In principle the concern is that if the caller splits a block with
iommu_unmap_fast(), there's no guarantee of seeing an iommu_tlb_sync()
before returning to the caller, and thus there's the potential to run into a
TLB conflict on a subsequent access even if the endpoint was "good" and
didn't make any accesses *during* the unmap call.
... this just feels pretty theoretical to me. The fact of the matter is
that we're unable to do break before make because we can't reliably tolerate
faults. If the hardware actually requires BBM for correctness, then we
should probably explore proper solutions (e.g. quirks, avoiding block
mappings, handling faults) rather than emitting a random sync and hoping
for the best.
Did you add the sync just in case, or was it based on a real crash?
Nope, just a theoretical best-effort thing, which I'm certainly not
going to lose sleep over either way - I just felt compelled to question
the rationale which didn't seem to fit. Realistically, this
partial-unmap case is not well-defined in IOMMU API terms, and other
drivers don't handle it consistently. I think VFIO explicitly rejects
partial unmaps, so if we see them at all it's only likely to be from
GPU/SVA type users who in principle ought to be able to tolerate
transient faults from BBM anyway.
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 02/13] iommu/io-pgtable-arm: Remove redundant call to io_pgtable_tlb_sync()
Hi Robin,
On Thu, Aug 15, 2019 at 01:43:11PM +0100, Robin Murphy wrote:
> On 14/08/2019 18:56, Will Deacon wrote:
> > Commit b6b65ca20bc9 ("iommu/io-pgtable-arm: Add support for non-strict
> > mode") added an unconditional call to io_pgtable_tlb_sync() immediately
> > after the case where we replace a block entry with a table entry during
> > an unmap() call. This is redundant, since the IOMMU API will call
> > iommu_tlb_sync() on this path and the patch in question mentions this:
> >
> > | To save having to reason about it too much, make sure the invalidation
> > | in arm_lpae_split_blk_unmap() just performs its own unconditional sync
> > | to minimise the window in which we're technically violating the break-
> > | before-make requirement on a live mapping. This might work out redundant
> > | with an outer-level sync for strict unmaps, but we'll never be splitting
> > | blocks on a DMA fastpath anyway.
> >
> > However, this sync gets in the way of deferred TLB invalidation for leaf
> > entries and is at best a questionable, unproven hack. Remove it.
>
> Hey, that's my questionable, unproven hack! :P
I thought you'd like to remain anonymous, but I can credit you if you like?
;)
> It's not entirely clear to me how this gets in the way though - AFAICS the
> intent of tlb_flush_leaf exactly matches the desired operation here, so
> couldn't these just wait to be converted in patch #8?
Good point. I think there are two things:
1. Initially, I didn't plan to have tlb_flush_leaf() at all because
I didn't think it would be needed. Then I ran into the v7s CONT
stuff and ended up needing it after all (I think it's the only
user). So that's an oversight.
2. If we do the tlb_flush_leaf() here, then we could potentially
put a hole in the ongoing gather structure, but I suppose we
could do both a tlb_add_page() *and* a tlb_flush_leaf() to get
around that.
So yes, I probably could move this back if the sync is necessary but...
> In principle the concern is that if the caller splits a block with
> iommu_unmap_fast(), there's no guarantee of seeing an iommu_tlb_sync()
> before returning to the caller, and thus there's the potential to run into a
> TLB conflict on a subsequent access even if the endpoint was "good" and
> didn't make any accesses *during* the unmap call.
... this just feels pretty theoretical to me. The fact of the matter is
that we're unable to do break before make because we can't reliably tolerate
faults. If the hardware actually requires BBM for correctness, then we
should probably explore proper solutions (e.g. quirks, avoiding block
mappings, handling faults) rather than emitting a random sync and hoping
for the best.
Did you add the sync just in case, or was it based on a real crash?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 02/13] iommu/io-pgtable-arm: Remove redundant call to io_pgtable_tlb_sync()
On 14/08/2019 18:56, Will Deacon wrote:
Commit b6b65ca20bc9 ("iommu/io-pgtable-arm: Add support for non-strict
mode") added an unconditional call to io_pgtable_tlb_sync() immediately
after the case where we replace a block entry with a table entry during
an unmap() call. This is redundant, since the IOMMU API will call
iommu_tlb_sync() on this path and the patch in question mentions this:
| To save having to reason about it too much, make sure the invalidation
| in arm_lpae_split_blk_unmap() just performs its own unconditional sync
| to minimise the window in which we're technically violating the break-
| before-make requirement on a live mapping. This might work out redundant
| with an outer-level sync for strict unmaps, but we'll never be splitting
| blocks on a DMA fastpath anyway.
However, this sync gets in the way of deferred TLB invalidation for leaf
entries and is at best a questionable, unproven hack. Remove it.
Hey, that's my questionable, unproven hack! :P
It's not entirely clear to me how this gets in the way though - AFAICS
the intent of tlb_flush_leaf exactly matches the desired operation here,
so couldn't these just wait to be converted in patch #8?
In principle the concern is that if the caller splits a block with
iommu_unmap_fast(), there's no guarantee of seeing an iommu_tlb_sync()
before returning to the caller, and thus there's the potential to run
into a TLB conflict on a subsequent access even if the endpoint was
"good" and didn't make any accesses *during* the unmap call.
Robin.
Signed-off-by: Will Deacon
---
drivers/iommu/io-pgtable-arm-v7s.c | 1 -
drivers/iommu/io-pgtable-arm.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/iommu/io-pgtable-arm-v7s.c
b/drivers/iommu/io-pgtable-arm-v7s.c
index 0fc8dfab2abf..a62733c6a632 100644
--- a/drivers/iommu/io-pgtable-arm-v7s.c
+++ b/drivers/iommu/io-pgtable-arm-v7s.c
@@ -587,7 +587,6 @@ static size_t arm_v7s_split_blk_unmap(struct
arm_v7s_io_pgtable *data,
}
io_pgtable_tlb_add_flush(>iop, iova, size, size, true);
- io_pgtable_tlb_sync(>iop);
return size;
}
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 161a7d56264d..0d6633921c1e 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -583,7 +583,6 @@ static size_t arm_lpae_split_blk_unmap(struct
arm_lpae_io_pgtable *data,
tablep = iopte_deref(pte, data);
} else if (unmap_idx >= 0) {
io_pgtable_tlb_add_flush(>iop, iova, size, size, true);
- io_pgtable_tlb_sync(>iop);
return size;
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
