linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
Date: Fri, 24 Jul 2015 17:53:25 +0100	[thread overview]
Message-ID: <20150724165325.GC21177@arm.com> (raw)
In-Reply-To: <1437715466.23932.68.camel@mhfsdcap03>

On Fri, Jul 24, 2015 at 06:24:26AM +0100, Yong Wu wrote:
> On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> > > +/* level 2 pagetable */
> > > +#define ARM_SHORT_PTE_TYPE_LARGE               BIT(0)
> > > +#define ARM_SHORT_PTE_SMALL_XN                 BIT(0)
> > > +#define ARM_SHORT_PTE_TYPE_SMALL               BIT(1)
> > > +#define ARM_SHORT_PTE_B                                BIT(2)
> > > +#define ARM_SHORT_PTE_C                                BIT(3)
> > > +#define ARM_SHORT_PTE_SMALL_TEX0               BIT(6)
> > > +#define ARM_SHORT_PTE_IMPLE                    BIT(9)
> >
> > This is AP[2] for small pages.
> 
> Sorry, In our pagetable bit9 in PGD and PTE is PA[32] that is for  the
> dram size over 4G. I didn't care it is different in PTE of the standard
> spec.
> And I don't use the AP[2] currently, so I only delete this line in next
> time.

Is this related to the "special bit". What would be good is a comment
next to the #define for the quirk describing *exactly* that differs in
your implementation. Without that, it's very difficult to know what is
intentional and what is actually broken.

> > > +static arm_short_iopte
> > > +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
> > > +{
> > > +       arm_short_iopte pteprot;
> > > +
> > > +       pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> > > +       pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> > > +                               ARM_SHORT_PTE_TYPE_SMALL;
> > > +       if (prot & IOMMU_CACHE)
> > > +               pteprot |=  ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
> > > +       if (prot & IOMMU_WRITE)
> > > +               pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> > > +                               ARM_SHORT_PTE_SMALL_TEX0;
> >
> > This doesn't make any sense. TEX[2:0] is all about memory attributes, not
> > permissions, so you're making the mapping write-back, write-allocate but
> > that's not what the IOMMU_* values are about.
> 
>      I will delete it.

Well, can you not control mapping permissions with the AP bits? The idea
of the IOMMU flags are:

  IOMMU_CACHE : Install a normal, cacheable mapping (you've got this right)
  IOMMU_READ : Allow read access for the device
  IOMMU_WRITE : Allow write access for the device
  IOMMU_NOEXEC : Disallow execute access for the device

so the caller to iommu_map passes in a bitmap of these, which you need to
encode in the page-table entry.

> > > +static int
> > > +_arm_short_map(struct arm_short_io_pgtable *data,
> > > +              unsigned int iova, phys_addr_t paddr,
> > > +              arm_short_iopte pgdprot, arm_short_iopte pteprot,
> > > +              bool large)
> > > +{
> > > +       const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > > +       arm_short_iopte *pgd = data->pgd, *pte;
> > > +       void *cookie = data->iop.cookie, *pte_va;
> > > +       unsigned int ptenr = large ? 16 : 1;
> > > +       int i, quirk = data->iop.cfg.quirks;
> > > +       bool ptenew = false;
> > > +
> > > +       pgd += ARM_SHORT_PGD_IDX(iova);
> > > +
> > > +       if (!pteprot) { /* section or supersection */
> > > +               if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK)
> > > +                       pgdprot &= ~ARM_SHORT_PGD_SECTION_XN;
> > > +               pte = pgd;
> > > +               pteprot = pgdprot;
> > > +       } else {        /* page or largepage */
> > > +               if (quirk & IO_PGTABLE_QUIRK_SHORT_MTK) {
> > > +                       if (large) { /* special Bit */
> >
> > This definitely needs a better comment! What exactly are you doing here
> > and what is that quirk all about?
> 
> I use this quirk is for MTK Special Bit as we don't have the XN bit in
> pagetable.

I'm still not really clear about what this is.

> > > +               if (!(*pgd)) {
> > > +                       pte_va = kmem_cache_zalloc(data->ptekmem, GFP_ATOMIC);
> > > +                       if (unlikely(!pte_va))
> > > +                               return -ENOMEM;
> > > +                       ptenew = true;
> > > +                       *pgd = virt_to_phys(pte_va) | pgdprot;
> > > +                       kmemleak_ignore(pte_va);
> > > +                       tlb->flush_pgtable(pgd, sizeof(*pgd), cookie);
> >
> > I think you need to flush this before it becomes visible to the walker.
> 
> I have flushed pgtable here, Do you meaning flush tlb here?

No. afaict, you allocate the pte table using kmem_cache_zalloc but you never
flush it. However, you update the pgd to point at this table, so the walker
can potentially see garbage instead of the zeroed entries.

Will

  reply	other threads:[~2015-07-24 16:53 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-16  9:04 [PATCH v3 0/6] MT8173 IOMMU SUPPORT Yong Wu
2015-07-16  9:04 ` [PATCH v3 1/6] dt-bindings: iommu: Add binding for mediatek IOMMU Yong Wu
2015-07-16  9:04 ` [PATCH v3 2/6] dt-bindings: mediatek: Add smi dts binding Yong Wu
2015-07-16  9:04 ` [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator Yong Wu
2015-07-21 17:11   ` Will Deacon
2015-07-24  5:24     ` Yong Wu
2015-07-24 16:53       ` Will Deacon [this message]
2015-07-27  4:21         ` Yong Wu
2015-07-27 14:05           ` Robin Murphy
2015-07-27 14:11             ` Will Deacon
2015-07-28  5:08               ` Yong Wu
2015-07-28 11:00                 ` Will Deacon
2015-07-28 13:37                   ` Yong Wu
2015-07-28 13:47                     ` Will Deacon
     [not found]           ` <1438329337.25925.147.camel@mhfsdcap03>
2015-07-31 11:32             ` Will Deacon
2015-09-14 12:25     ` Yong Wu
2015-09-16 12:55       ` Will Deacon
2015-09-17  2:38         ` Yong Wu
2015-07-16  9:04 ` [PATCH v3 4/6] memory: mediatek: Add SMI driver Yong Wu
2015-07-16  9:04 ` [PATCH v3 5/6] iommu/mediatek: Add mt8173 IOMMU driver Yong Wu
2015-07-21 14:59   ` Will Deacon
2015-07-24  5:43     ` Yong Wu
2015-07-24 16:55       ` Will Deacon
2015-07-27  4:24         ` Yong Wu
2015-07-27 15:48           ` Will Deacon
2015-07-27 13:23   ` Robin Murphy
2015-07-27 15:31     ` Russell King - ARM Linux
2015-07-27 15:49       ` Robin Murphy
2015-07-29  5:41         ` Yong Wu
2015-07-29 10:31           ` Will Deacon
2015-07-29  6:32     ` Yong Wu
2015-07-16  9:04 ` [PATCH v3 6/6] dts: mt8173: Add iommu/smi nodes for mt8173 Yong Wu
2015-07-23 14:40   ` Daniel Kurtz
2015-07-29  7:29     ` Yong Wu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150724165325.GC21177@arm.com \
    --to=will.deacon@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).