Re: [Qemu-arm] [PATCH 17/27] exec.c: Handle IOMMUs in address_space_translate_for_iotlb()

[Peter Maydell]

On 23 May 2018 at 10:51, Alex Bennée <address@hidden> wrote:
>
> Peter Maydell <address@hidden> 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 <address@hidden>
>> ---
>>  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