| Message ID | 1547671814-30088-1-git-send-email-navneetk@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [1/5] iommu/tegra-smmu: Fix domain_alloc | expand |
16.01.2019 23:50, Navneet Kumar пишет: > * Allocate dma iova cookie for a domain while adding dma iommu > devices. > * Perform a stricter check for domain type parameter. > Commit message should tell what exactly is getting "fixed". Apparently you're trying to support T132 ARM64 here. > Signed-off-by: Navneet Kumar <navneetk@nvidia.com> > --- > drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++---------------- > 1 file changed, 27 insertions(+), 16 deletions(-) > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > index 543f7c9..ee4d8a8 100644 > --- a/drivers/iommu/tegra-smmu.c > +++ b/drivers/iommu/tegra-smmu.c > @@ -16,6 +16,7 @@ > #include <linux/platform_device.h> > #include <linux/slab.h> > #include <linux/dma-mapping.h> > +#include <linux/dma-iommu.h> > > #include <soc/tegra/ahb.h> > #include <soc/tegra/mc.h> > @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap) > static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) > { > struct tegra_smmu_as *as; > + int ret; > > - if (type != IOMMU_DOMAIN_UNMANAGED) > + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA && > + type != IOMMU_DOMAIN_IDENTITY) > return NULL; Should be better to write these lines like this for the sake of readability: if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_IDENTITY) return NULL; > > as = kzalloc(sizeof(*as), GFP_KERNEL); > @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) > > as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE; > > + ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) : > + -ENODEV; This makes to fail allocation of domain that has type other than IOMMU_DOMAIN_DMA. Should be: if (type == IOMMU_DOMAIN_DMA) { err = iommu_get_dma_cookie(&as->domain); if (err) goto free_as; } > + if (ret) > + goto free_as; > + > as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO); > - if (!as->pd) { > - kfree(as); > - return NULL; > - } > + if (!as->pd) > + goto put_dma_cookie; > > as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL); > - if (!as->count) { > - __free_page(as->pd); > - kfree(as); > - return NULL; > - } > + if (!as->count) > + goto free_pd_range; > > as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL); > - if (!as->pts) { > - kfree(as->count); > - __free_page(as->pd); > - kfree(as); > - return NULL; > - } > + if (!as->pts) > + goto free_pts; > > /* setup aperture */ > as->domain.geometry.aperture_start = 0; > @@ -308,6 +307,18 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) > as->domain.geometry.force_aperture = true; > > return &as->domain; > + > +free_pts: > + kfree(as->pts); > +free_pd_range: > + __free_page(as->pd); > +put_dma_cookie: > + if (type == IOMMU_DOMAIN_DMA) > + iommu_put_dma_cookie(&as->domain); > +free_as: > + kfree(as); > + > + return NULL; > } > > static void tegra_smmu_domain_free(struct iommu_domain *domain) >
On 17/01/2019 15:13, Dmitry Osipenko wrote: > 16.01.2019 23:50, Navneet Kumar пишет: >> * Allocate dma iova cookie for a domain while adding dma iommu >> devices. >> * Perform a stricter check for domain type parameter. >> > > Commit message should tell what exactly is getting "fixed". Apparently you're trying to support T132 ARM64 here. > >> Signed-off-by: Navneet Kumar <navneetk@nvidia.com> >> --- >> drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++---------------- >> 1 file changed, 27 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c >> index 543f7c9..ee4d8a8 100644 >> --- a/drivers/iommu/tegra-smmu.c >> +++ b/drivers/iommu/tegra-smmu.c >> @@ -16,6 +16,7 @@ >> #include <linux/platform_device.h> >> #include <linux/slab.h> >> #include <linux/dma-mapping.h> >> +#include <linux/dma-iommu.h> >> >> #include <soc/tegra/ahb.h> >> #include <soc/tegra/mc.h> >> @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap) >> static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) >> { >> struct tegra_smmu_as *as; >> + int ret; >> >> - if (type != IOMMU_DOMAIN_UNMANAGED) >> + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA && >> + type != IOMMU_DOMAIN_IDENTITY) >> return NULL; > > Should be better to write these lines like this for the sake of readability: > > if (type != IOMMU_DOMAIN_UNMANAGED && > type != IOMMU_DOMAIN_DMA && > type != IOMMU_DOMAIN_IDENTITY) > return NULL; Furthermore, AFAICS there's no special handling being added for identity domains - don't pretend to support them by giving back a regular translation domain, that's just misleading and broken. > > >> >> as = kzalloc(sizeof(*as), GFP_KERNEL); >> @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) >> >> as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE; >> >> + ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) : >> + -ENODEV; > > This makes to fail allocation of domain that has type other than IOMMU_DOMAIN_DMA. > > Should be: > > if (type == IOMMU_DOMAIN_DMA) { > err = iommu_get_dma_cookie(&as->domain); > if (err) > goto free_as; > } > > >> + if (ret) >> + goto free_as; >> + >> as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO); >> - if (!as->pd) { >> - kfree(as); >> - return NULL; >> - } >> + if (!as->pd) >> + goto put_dma_cookie; >> >> as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL); >> - if (!as->count) { >> - __free_page(as->pd); >> - kfree(as); >> - return NULL; >> - } >> + if (!as->count) >> + goto free_pd_range; >> >> as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL); >> - if (!as->pts) { >> - kfree(as->count); >> - __free_page(as->pd); >> - kfree(as); >> - return NULL; >> - } >> + if (!as->pts) >> + goto free_pts; >> >> /* setup aperture */ >> as->domain.geometry.aperture_start = 0; >> @@ -308,6 +307,18 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) >> as->domain.geometry.force_aperture = true; >> >> return &as->domain; >> + >> +free_pts: >> + kfree(as->pts); >> +free_pd_range: >> + __free_page(as->pd); >> +put_dma_cookie: >> + if (type == IOMMU_DOMAIN_DMA) FWIW you don't strictly need that check - since domain->iova_cookie won't be set for other domain types anyway, iommu_put_dma_cookie() will simply ignore them. >> + iommu_put_dma_cookie(&as->domain); >> +free_as: >> + kfree(as); >> + >> + return NULL; >> } >> >> static void tegra_smmu_domain_free(struct iommu_domain *domain) How about not leaking the cookie when you free a DMA ops domain? Robin.
Thanks for the feedback, Robin, and, Dmitry. I mostly agree with all your comments, pls see my responses inline. I'll make these fixes in V2. On 1/17/19 8:50 AM, Robin Murphy wrote: > On 17/01/2019 15:13, Dmitry Osipenko wrote: >> 16.01.2019 23:50, Navneet Kumar пишет: >>> * Allocate dma iova cookie for a domain while adding dma iommu >>> devices. >>> * Perform a stricter check for domain type parameter. >>> >> >> Commit message should tell what exactly is getting "fixed". Apparently you're trying to support T132 ARM64 here. I'll fix it. >> >>> Signed-off-by: Navneet Kumar <navneetk@nvidia.com> >>> --- >>> drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++---------------- >>> 1 file changed, 27 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c >>> index 543f7c9..ee4d8a8 100644 >>> --- a/drivers/iommu/tegra-smmu.c >>> +++ b/drivers/iommu/tegra-smmu.c >>> @@ -16,6 +16,7 @@ >>> #include <linux/platform_device.h> >>> #include <linux/slab.h> >>> #include <linux/dma-mapping.h> >>> +#include <linux/dma-iommu.h> >>> #include <soc/tegra/ahb.h> >>> #include <soc/tegra/mc.h> >>> @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap) >>> static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) >>> { >>> struct tegra_smmu_as *as; >>> + int ret; >>> - if (type != IOMMU_DOMAIN_UNMANAGED) >>> + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA && >>> + type != IOMMU_DOMAIN_IDENTITY) >>> return NULL; >> >> Should be better to write these lines like this for the sake of readability: >> >> if (type != IOMMU_DOMAIN_UNMANAGED && >> type != IOMMU_DOMAIN_DMA && >> type != IOMMU_DOMAIN_IDENTITY) >> return NULL; > > Furthermore, AFAICS there's no special handling being added for identity domains - don't pretend to support them by giving back a regular translation domain, that's just misleading and broken. Agreed. > >> >> >>> as = kzalloc(sizeof(*as), GFP_KERNEL); >>> @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) >>> as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE; >>> + ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) : >>> + -ENODEV; >> >> This makes to fail allocation of domain that has type other than IOMMU_DOMAIN_DMA. >> >> Should be: >> >> if (type == IOMMU_DOMAIN_DMA) { >> err = iommu_get_dma_cookie(&as->domain); >> if (err) >> goto free_as; >> } >> Agreed. >> >>> + if (ret) >>> + goto free_as; >>> + >>> as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO); >>> - if (!as->pd) { >>> - kfree(as); >>> - return NULL; >>> - } >>> + if (!as->pd) >>> + goto put_dma_cookie; >>> as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL); >>> - if (!as->count) { >>> - __free_page(as->pd); >>> - kfree(as); >>> - return NULL; >>> - } >>> + if (!as->count) >>> + goto free_pd_range; >>> as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL); >>> - if (!as->pts) { >>> - kfree(as->count); >>> - __free_page(as->pd); >>> - kfree(as); >>> - return NULL; >>> - } >>> + if (!as->pts) >>> + goto free_pts; >>> /* setup aperture */ >>> as->domain.geometry.aperture_start = 0; >>> @@ -308,6 +307,18 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) >>> as->domain.geometry.force_aperture = true; >>> return &as->domain; >>> + >>> +free_pts: >>> + kfree(as->pts); >>> +free_pd_range: >>> + __free_page(as->pd); >>> +put_dma_cookie: >>> + if (type == IOMMU_DOMAIN_DMA) > > FWIW you don't strictly need that check - since domain->iova_cookie won't be set for other domain types anyway, iommu_put_dma_cookie() will simply ignore them. > I'll still keep that check and not rely solely on the put_dma_cookie API to handle this case. This will keep the code robust even if the put_dma_cookie API changes in future (for whatever reason). It also looks like the canonical usage in other drivers. >>> + iommu_put_dma_cookie(&as->domain); >>> +free_as: >>> + kfree(as); >>> + >>> + return NULL; >>> } >>> static void tegra_smmu_domain_free(struct iommu_domain *domain) > > How about not leaking the cookie when you free a DMA ops domain? > Agreed. > Robin.
On Wed, Jan 16, 2019 at 12:50:10PM -0800, Navneet Kumar wrote: > * Allocate dma iova cookie for a domain while adding dma iommu > devices. > * Perform a stricter check for domain type parameter. > > Signed-off-by: Navneet Kumar <navneetk@nvidia.com> > --- > drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++---------------- > 1 file changed, 27 insertions(+), 16 deletions(-) I just gave this a quick spin because I was investigating how we could potentially make use of the DMA API instead of the IOMMU API directly in Tegra DRM. We currently rely on the fact that the Tegra SMMU driver only supports unmanaged domains. Once we start supporting DMA domains all the automatic machinery kicks in and there's lots of SMMU faults. I think at least partially those faults point out bugs we currently have in the code. From the looks of it, the display controller is running during boot and happily fetching from whatever address it was configured with in the bootloader, and when we enable the ASID for the display controller as part of the DMA/IOMMU setup, the fetches from the display controller will be accessing IOV addresses that don't have a mapping. One one hand that's a good thing because it points out existing weaknesses, but then it also means that we can't merge this series because it causes bad regressions. I also see failures from the GPU with this applied, which I think stem from the fact that we're now transparently mapping allocations through the SMMU without the Nouveau driver knowing that and setting the appropriate bit when addressing memory. Or it could come from the SMMU code in Nouveau trying to map an already mapped buffer, so effectively creating an IOVA mapping to an address that is already a IOV address rather than a physical address. So I think before we can go ahead with this series we have a lot of janitorial work to do first so that this won't cause any regressions when applied. Thierry
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 543f7c9..ee4d8a8 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -16,6 +16,7 @@ #include <linux/platform_device.h> #include <linux/slab.h> #include <linux/dma-mapping.h> +#include <linux/dma-iommu.h> #include <soc/tegra/ahb.h> #include <soc/tegra/mc.h> @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap) static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) { struct tegra_smmu_as *as; + int ret; - if (type != IOMMU_DOMAIN_UNMANAGED) + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA && + type != IOMMU_DOMAIN_IDENTITY) return NULL; as = kzalloc(sizeof(*as), GFP_KERNEL); @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE; + ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) : + -ENODEV; + if (ret) + goto free_as; + as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO); - if (!as->pd) { - kfree(as); - return NULL; - } + if (!as->pd) + goto put_dma_cookie; as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL); - if (!as->count) { - __free_page(as->pd); - kfree(as); - return NULL; - } + if (!as->count) + goto free_pd_range; as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL); - if (!as->pts) { - kfree(as->count); - __free_page(as->pd); - kfree(as); - return NULL; - } + if (!as->pts) + goto free_pts; /* setup aperture */ as->domain.geometry.aperture_start = 0; @@ -308,6 +307,18 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type) as->domain.geometry.force_aperture = true; return &as->domain; + +free_pts: + kfree(as->pts); +free_pd_range: + __free_page(as->pd); +put_dma_cookie: + if (type == IOMMU_DOMAIN_DMA) + iommu_put_dma_cookie(&as->domain); +free_as: + kfree(as); + + return NULL; } static void tegra_smmu_domain_free(struct iommu_domain *domain)
* Allocate dma iova cookie for a domain while adding dma iommu devices. * Perform a stricter check for domain type parameter. Signed-off-by: Navneet Kumar <navneetk@nvidia.com> --- drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 16 deletions(-)