On 23 May 2018 at 10:51, Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Peter Maydell <peter.mayd...@linaro.org> writes:
>
>> Currently we don't support board configurations that put an IOMMU
>> in the path of the CPU's memory transactions, and instead just
>> assert() if the memory region fonud in address_space_translate_for_iotlb()
>> is an IOMMUMemoryRegion.
>>
>> Remove this limitation by having the function handle IOMMUs.
>> This is mostly straightforward, but we must make sure we have
>> a notifier registered for every IOMMU that a transaction has
>> passed through, so that we can flush the TLB appropriately
>> when any of the IOMMUs change their mappings.
>>
>> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
>> ---
>> include/exec/exec-all.h | 3 +-
>> include/qom/cpu.h | 3 +
>> accel/tcg/cputlb.c | 3 +-
>> exec.c | 147 +++++++++++++++++++++++++++++++++++++++-
>> 4 files changed, 152 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index 4d09eaba72..e0ff19b711 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -469,7 +469,8 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong
>> addr);
>>
>> MemoryRegionSection *
>> address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
>> - hwaddr *xlat, hwaddr *plen);
>> + hwaddr *xlat, hwaddr *plen,
>> + MemTxAttrs attrs, int *prot);
>> hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>> MemoryRegionSection *section,
>> target_ulong vaddr,
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 9d3afc6c75..d4a30149dd 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -429,6 +429,9 @@ struct CPUState {
>> uint16_t pending_tlb_flush;
>>
>> int hvf_fd;
>> +
>> + /* track IOMMUs whose translations we've cached in the TCG TLB */
>> + GSList *iommu_notifiers;
>
> So we are only concerned about TCG IOMMU notifiers here, specifically
> TCGIOMMUNotifier structures. Why not just use a GArray and save
> ourselves chasing pointers?
I don't have a strong opinion about which data structure to use;
but GSList has a "find an entry" API and GArray doesn't, so I
picked the one that had the API that meant I didn't need to
hand-code a search loop.
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -650,18 +650,158 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr
>> addr, hwaddr *xlat,
>> return mr;
>> }
>>
>> +typedef struct TCGIOMMUNotifier {
>> + IOMMUNotifier n;
>> + MemoryRegion *mr;
>> + CPUState *cpu;
>
> This seems superfluous if we are storing the list of notifiers in the CPUState
You need it because in the notifier callback all you get is a pointer
to the IOMMUNotifier, and we need to get from there to the CPUState*.
>> + int iommu_idx;
>> + bool active;
>> +} TCGIOMMUNotifier;
thanks
-- PMM
