linux-kernel - Re: [PATCH v2 3/5] iommu/exynos: Add iommu driver for Exynos4 Platforms

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAHQjnONrPRynW5_C3+CqOAM095m+FK7P5jJMzN-ohwV_Y6jzGQ@mail.gmail.com>
Date:	Sat, 1 Oct 2011 08:46:51 +0900
From:	KyongHo Cho <pullip.cho@...sung.com>
To:	"Roedel, Joerg" <Joerg.Roedel@....com>
Cc:	Kukjin Kim <kgene.kim@...sung.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>,
	"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Ohad Ben-Cohen <ohad@...ery.com>,
	Sanghyun Lee <sanghyun75.lee@...sung.com>,
	"younglak1004.kim@...sung.com" <younglak1004.kim@...sung.com>
Subject: Re: [PATCH v2 3/5] iommu/exynos: Add iommu driver for Exynos4 Platforms

On Fri, Sep 30, 2011 at 9:06 PM, Roedel, Joerg <Joerg.Roedel@....com> wrote:
> First comment: Pleas remove the 'inline' annotations in this patch. It
> is better to let the compiler decide what to inline and what not.
Ok. Thanks :)

>
> Hmm, may it make sense to store data directly in dev->arch.iommu? This
> will save you the list traversals to get the information.
>
That looks better.
However, adding features to dev_archdata of ARM is actively discussed
in Linaro-mm-sig and the features also include iommu_domain.

On the other hand,
I was not able to determine what will be accepted to the mainline kernel
between Linaro's one that is suggested by Marek and Ohad's one.

>> +
>> +static inline bool set_sysmmu_active(struct sysmmu_drvdata *data)
>> +{
>> +       /* return true if the System MMU was not active previously
>> +          and it needs to be initialized */
>> +
>> +       data->activations++;
>
> Is this variable only accessed under a lock? If not it should be an
> atomic.
>
All 'sysmmu' functions must be called under a lock because a System MMU
(Samsung's IOMMU) is a shared resource.
I the first patch, I declared the 'data->activations' as atomic_t but
Russell King pointed
that atomic operations on data->activations is not helpful for
preventing to become
a System MMU enabled when its data->activations is 0.

>> +       return data->activations == 1;
>
> Is that right? Shouldn't it be 'data->activations > 0'?
>
The value returned by set_sysmmu_active() means "The System MMU must be
initialized".
System MMU is initialized when its state is changed from
'inactive(disabled)' to active(enabled).
If data->activations++ becomes 2 or more, the System MMU is already enabled and
must not be initialized again.

>> +static inline void __set_fault_handler(struct sysmmu_drvdata *data,
>> +                       int (*handler)(enum S5P_SYSMMU_INTERRUPT_TYPE itype,
>> +                                       unsigned long pgtable_base,
>> +                                       unsigned long fault_addr))
>
> Please typedef the function signature.
>
Ok. thanks.

>> +static int exynos_iommu_domain_init(struct iommu_domain *domain)
>> +{
>> +       struct exynos_iommu_domain *priv;
>> +
>> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> +       if (!priv)
>> +               return -ENOMEM;
>> +
>> +       priv->pgtable = (unsigned long *)__get_free_pages(GFP_KERNEL,
>> +               (S5P_LV1TABLE_ENTRIES * sizeof(unsigned long)) >> PAGE_SHIFT);
>
> For __get_free_pages you can't just pass in the number of pages, you
> need the order. Please use get_order.
>
Oh! Thank you.
I didn't notice the problem.
It allocated too large memory so far.

>> +       if (!priv->pgtable) {
>> +               kfree(priv);
>> +               return -ENOMEM;
>> +       }
>> +
>> +       memset(priv->pgtable, 0, S5P_LV1TABLE_ENTRIES * sizeof(unsigned long));
>> +       pgtable_flush(priv->pgtable, priv->pgtable + S5P_LV1TABLE_ENTRIES);
>> +
>> +       spin_lock_init(&priv->lock);
>
> No init for the page-table lock?
>
Thank you. I forgot that.


>> +static void exynos_iommu_domain_destroy(struct iommu_domain *domain)
>> +{
>> +       struct exynos_iommu_domain *priv = domain->priv;
>> +
>> +       free_pages((unsigned long)priv->pgtable,
>> +               (S5P_LV1TABLE_ENTRIES * sizeof(unsigned long)) >> PAGE_SHIFT);
>
> Same here, please use get_order.
>
True. I will fix that immediately.

>> +static int __init exynos_iommu_init(void)
>> +{
>> +       l2table_cachep = kmem_cache_create("SysMMU Lv2 Tables",
>> +                               S5P_LV2TABLE_SIZE, S5P_LV2TABLE_SIZE, 0, NULL);
>
> Any reason you allocate a seperate slab-cache? It doesn't have a
> constructor so it would be merged into the kmalloc-slabs anyway (at
> least with SLUB).
>
I found that kmalloc(SZ_1K) returned an address that is not aligned by SZ_1K.
I did not understand why it happened and I changed it to the slab cache.
I will try it again with kmalloc().


Regards,
KyongHo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/