Re: [PATCH 2/5] iommu/arm-smmu: Emulate bypass by using context banks

On Thu 09 Jul 11:55 PDT 2020, Rob Clark wrote:

On Thu, Jul 9, 2020 at 9:56 AM Rob Clark [off-list ref] wrote:

quoted

On Thu, Jul 9, 2020 at 9:48 AM Bjorn Andersson
[off-list ref] wrote:

quoted

On Thu 09 Jul 09:17 PDT 2020, Rob Clark wrote:

quoted

On Wed, Jul 8, 2020 at 10:01 PM Bjorn Andersson
[off-list ref] wrote:

[..]

quoted

quoted

@@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
        if (smmu_domain->smmu)
                goto out_unlock;

-       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+       /*
+        * Nothing to do for IDENTITY domains,unless disabled context banks are
+        * used to emulate bypass mappings on Qualcomm platforms.
+        */
+       if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {

maybe I'm overlooking something, but I think this would put us back to
allocating pgtables (and making iommu->map/unmap() no longer no-ops),
which I don't think we want

You're right, we are allocating page tables for these contexts and
map/unmap would modify the page tables. But afaict traversal is never
performed, given that the banks are never enabled.

But as drivers probe properly, or the direct mapped drivers sets up
their iommu domains explicitly with translation this would not be used.

So afaict we're just wasting some memory - for the gain of not
overcomplicating this function.

the problem is that it makes dma_map/unmap less of a no-op than it
should be (for the case where the driver is explicitly managing it's
own domain)..  I was hoping to get rid of the hacks to use dma_sync go
back to dma_map/unmap for cache cleaning

That said, it seems to cause less explosions than before.. either that
or I'm not trying hard enough.  Still, I think it would probably
result in unnecessary tlb inv's.

Previously, *somehow* we seemed to end up with dma_map/unmap trying to
modify the domain that we had attached.

I might need some help to really understand the plumbing here, but this
is what I read from the code and can see in my debugging...


The display device will upon creation be allocated a default_domain, of
type IOMMU_DOMAIN_IDENTITY - per the qcom-impl. Then you then allocate a
new iommu domain on the platform bus in the display driver and attach
this to the device. This will result in the group's domain being of type
IOMMU_DOMAIN_UNMANAGED.

But when you then invoke dma_map_single() on the display device, the map
operation will acquire the iommu_group's default_domain (not domain) and
as such attempt to map stuff on the IDENTITY domain.

__iommu_map() will however reject this, given that the type does not
have __IOMMU_DOMAIN_PAGING set. Afaict, this would happen regardless of
this patch actually setting up a page table for the default_domain or
not.


So, afaict, you can't use dma_map_single()/dma_unmap() to operate your
page table on a lately attached iommu domain.

This would also imply that the display device consumes two context
banks, which worries me more than the waste of page tables etc.

Regards,
Bjorn

BR,
-R

quoted

BR,
-R

quoted

Regards,
Bjorn

quoted

BR,
-R

quoted

                smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
                smmu_domain->smmu = smmu;
                goto out_unlock;

@@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
        domain->geometry.aperture_end = (1UL << ias) - 1;
        domain->geometry.force_aperture = true;

+       /* Enable translation for non-identity context banks */
+       if (domain->type != IOMMU_DOMAIN_IDENTITY)
+               cfg->m = true;
+
        /* Initialise the context bank with our page table cfg */
        arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
        arm_smmu_write_context_bank(smmu, cfg->cbndx);

diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index d172c024be61..a71d193073e4 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h

@@ -305,6 +305,8 @@ struct arm_smmu_device {

        /* IOMMU core code handle */
        struct iommu_device             iommu;
+
+       bool                            qcom_bypass_quirk;
 };

 enum arm_smmu_context_fmt {

@@ -323,6 +325,7 @@ struct arm_smmu_cfg {
        };
        enum arm_smmu_cbar_type         cbar;
        enum arm_smmu_context_fmt       fmt;
+       bool                            m;
 };
 #define ARM_SMMU_INVALID_IRPTNDX       0xff

--

2.26.2

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel