Hi Peter,
On 05/25/2018 10:52 AM, Peter Maydell wrote:
> On 24 May 2018 at 20:54, Auger Eric <eric.au...@redhat.com> wrote:
>> Hi Peter,
>>
>> On 05/23/2018 11:51 AM, Alex Bennée 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()
>> found
>>>> 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
>> Can you elaborate on what (TCG) TLB we are talking about?
>
> The TCG TLB, as implemented in accel/tcg/cputlb.c. Basically
> the thing that caches the results it gets back from the memory
> system so it can fast path device and memory accesses.
>
>> The concept of IOMMUs downstream to a CPU is not obvious to me. Maybe an
>> example may be documented in the commit message?
>
> The MPC implemented in this patchset is an example.
>
>
>
>>>> +static void tcg_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>>>> +{
>>>> + TCGIOMMUNotifier *notifier = container_of(n, TCGIOMMUNotifier, n);
>>>> +
>>>> + if (!notifier->active) {
>>>> + return;
>>>> + }
>>>> + tlb_flush(notifier->cpu);
>>>> + notifier->active = false;
>>>> + /* We leave the notifier struct on the list to avoid reallocating it
>>>> later.
>>>> + * Generally the number of IOMMUs a CPU deals with will be small.
>>>> + * In any case we can't unregister the iommu notifier from a notify
>>>> + * callback.
>>>> + */
>> I don't get the life cycle of the notifier and why it becomes inactive
>> after the invalidate. Could you detail the specificity of this one?
>
> Once we've flushed the TLB it is empty and will have no cached
> information from the IOMMU. So there's no point in flushing the
> TLB again (which is expensive) until the next time a transaction
> goes through the IOMMU and we're caching something from it.
Ak OK. there is no finer granularity for TLB flush?
>
> So the cycle goes:
> * CPU makes transaction that goes through an IOMMU
> * in tcg_register_iommu_notifier() we register the notifier
> if we haven't already, and make sure it's got active = true
> * in the unmap notify, we flush the whole TLB for the CPU, and
> set active = false
> * repeat...
OK thank you for the explanation
>
>
>>>> +static void tcg_iommu_notifier_destroy(gpointer data)
>>>> +{
>>>> + TCGIOMMUNotifier *notifier = data;
>>>> +
>>>> + if (notifier->active) {
>>>> + memory_region_unregister_iommu_notifier(notifier->mr,
>>>> ¬ifier->n);
>>>> + }
>> Is it safe to leave the notifier registered to an IOMMU whereas it gets
>> freed?
>
> Oh, this is a bug, left over from my first idea (which was to
> unregister the IOMMU notifier in the notifier unmap callback,
> in which case active == true would be the only case when we
> had a registered notifier).
>
> We should unconditionally unregister the notifier here.
>
>
>>>> /* Called from RCU critical section */
>>>> MemoryRegionSection *
>>>> address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
>>>> - hwaddr *xlat, hwaddr *plen)
>>>> + hwaddr *xlat, hwaddr *plen,
>>>> + MemTxAttrs attrs, int *prot)
>>>> {
>>>> MemoryRegionSection *section;
>>>> + IOMMUMemoryRegion *iommu_mr;
>>>> + IOMMUMemoryRegionClass *imrc;
>>>> + IOMMUTLBEntry iotlb;
>>>> + int iommu_idx;
>>>> AddressSpaceDispatch *d =
>>>> atomic_rcu_read(&cpu->cpu_ases[asidx].memory_dispatch);
>>>>
>>>> - section = address_space_translate_internal(d, addr, xlat, plen,
>>>> false);
>>>> + for (;;) {
>>>> + section = address_space_translate_internal(d, addr, &addr, plen,
>>>> false);
>>>> +
>>>> + iommu_mr = memory_region_get_iommu(section->mr);
>>>> + if (!iommu_mr) {
>>>> + break;
>>>> + }
>>>> +
>>>> + imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
>>>> +
>>>> + iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
>>>> + tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx);
>>>> + /* We need all the permissions, so pass IOMMU_NONE so the IOMMU
>>>> + * doesn't short-cut its translation table walk.
>> it is not clear to me why you don't use the access flag as you seem to
>> handle the perm fault after the translate() call.
>
> We need to know all the permissions (because we'll cache the result
> in the TCG TLB and later use them for future read and write accesses),
> so we pass IOMMU_NONE.
>
> My understanding from previous discussion is that the only
> reason to pass in some other access flag value is if you
> only care about one of read or write and want to allow the
> IOMMU to stop walking the page table early as soon as it decides
> it doesn't have permissions.
agreed. So you need to fetch the whole set of table permissions to
update the TLB. By the way where is the TLB updated?
Thanks
Eric
>
> thanks
> -- PMM
>
