On 2020-11-04 07:17, Kunkun Jiang wrote:
Hi Will and Robin,
Sorry for the late reply.
On 2020/11/3 18:21, Robin Murphy wrote:
On 2020-11-03 09:11, Will Deacon wrote:
On Tue, Nov 03, 2020 at 11:00:27AM +0800, Kunkun Jiang wrote:
Recently, I have read and learned the code related to io-pgtable-arm.c.
There
are two question on arm_lpae_install_table.
1、the first
static arm_lpae_iopte arm_lpae_install_table(arm_lpae_iopte *table,
arm_lpae_iopte *ptep,
arm_lpae_iopte curr,
struct io_pgtable_cfg
*cfg)
{
arm_lpae_iopte old, new;
new = __pa(table) | ARM_LPAE_PTE_TYPE_TABLE;
if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS)
new |= ARM_LPAE_PTE_NSTABLE;
/*
* Ensure the table itself is visible before its PTE can be.
* Whilst we could get away with cmpxchg64_release below,
this
* doesn't have any ordering semantics when !CONFIG_SMP.
*/
dma_wmb();
old = cmpxchg64_relaxed(ptep, curr, new);
if (cfg->coherent_walk || (old & ARM_LPAE_PTE_SW_SYNC))
return old;
/* Even if it's not ours, there's no point waiting; just
kick it
*/
__arm_lpae_sync_pte(ptep, cfg);
if (old == curr)
WRITE_ONCE(*ptep, new | ARM_LPAE_PTE_SW_SYNC);
return old;
}
If another thread changes the ptep between cmpxchg64_relaxed and
WRITE_ONCE(*ptep, new | ARM_LPAE_PTE_SW_SYNC), the operation
WRITE_ONCE will overwrite the change.
Can you please provide an example of a code path where this happens? The
idea is that CPUs can race on the cmpxchg(), but there will only be one
winner.
The only way a table entry can suddenly disappear is in a race that
involves mapping or unmapping a whole block/table-sized region, while
simultaneously mapping a page *within* that region. Yes, if someone
uses the API in a nonsensical and completely invalid way that cannot
have a predictable outcome, they get an unpredictable outcome. Whoop
de do...
Yes, as Robin mentioned, there will be an unpredictable outcome in extreme
cases. Now, we have two APIs mapping and unmapping to alter a block/page
descriptor. I worry about that it may be increasingly difficult to add
API in
the future.
I still don't understand your concern. If two threads try to
simultaneously create a block mapping and a page mapping *for the same
virtual address*, the result is that one of those mappings will end up
in place, depending on who managed to write to the PTE last. This code
is self-consistent - it doesn't crash, nor end up with any mapping that
*wasn't* requested - it just doesn't go far out of its way to
accommodate obviously invalid usage. By comparison, say instead those
two threads simultaneously call kfree() on the same pointer; the outcome
of that is considerably worse.
What kind of additional new API are you imagining where such
deliberately racy behaviour would be anything other than broken nonsense?
2、the second
for (i = 0; i < tablesz / sizeof(pte); i++, blk_paddr += split_sz) {
/* Unmap! */
if (i == unmap_idx)
continue;
__arm_lpae_init_pte(data, blk_paddr, pte, lvl,
&tablep[i]);
}
pte = arm_lpae_install_table(tablep, ptep, blk_pte, cfg);
When altering a translation table descriptor include split a block into
constituent granules, the Armv8-A and SMMUv3 architectures require
a break-before-make procedure. But in the function
arm_lpae_split_blk_unmap,
it changes a block descriptor to an equivalent span of page
translations
directly. Is it appropriate to do so?
Break-before-make doesn't really work for the SMMU because faults are
generally fatal.
Are you seeing problems in practice with this code?
TBH I do still wonder if we shouldn't just get rid of split_blk_unmap
and all its complexity. Other drivers treat an unmap of a page from a
block entry as simply unmapping the whole block, and that's the
behaviour VFIO seems to expect. My only worry is that it's been around
long enough that there might be some horrible out-of-tree code that
*is* relying on it, despite the fact that it's impossible to implement
in a way that's definitely 100% safe :/
Robin.
.
I have not encountered a problem. I mean SMMU has supported BBML feature
in SMMUv3.2. Maybe we can use this feature to update translation table
without
break-before-make.
We could. But then what do we do for all the existing hardware without
BBML, or on x86 or other drivers which don't split blocks anyway?
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
