Re: Some questions about arm_lpae_install_table

[Robin Murphy <robin.murphy@arm.com>]

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...

>> 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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu