Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4244 bytes --]

On Mon, 2015-07-27 at 12:21 +0800, Yong Wu wrote:

> On Fri, 2015-07-24 at 17:53 +0100, Will Deacon wrote:
> > 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.
> 
> I will add the comment alongside the #define.
> 
> > 
> > > > > +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.
> 
> From the spec, AP[2] differentiate the read/write and readonly.
> How about this?: 
> //===============
>   #define ARM_SHORT_PGD_FULL_ACCESS  (3 << 10) 
>   #define ARM_SHORT_PGD_RDONLY       BIT(15)
> 
>   pgdprot |= ARM_SHORT_PGD_FULL_ACCESS;/* or other names? */
>   if(!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
>      pgdprot |= ARM_SHORT_PGD_RDONLY;
> //===============
> pte is the same. 
> 
> Sorry, Our HW don't meet the standard spec fully. it don't implement the
> AP bits.



Hi Will, 
    About the AP bits, I may have to add a new quirk for it...

  Current I add AP in pte like this:
 #define ARM_SHORT_PTE_RD_WR        (3 << 4) 
 #define ARM_SHORT_PTE_RDONLY       BIT(9)

 pteprot |=  ARM_SHORT_PTE_RD_WR;

 If(!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))

      pteprot |= ARM_SHORT_PTE_RDONLY;

The problem is that the BIT(9) in the level1 and level2 pagetable of our
HW has been used for PA[32] that is for the dram size over 4G.

so I had to add a quirk to disable bit9 while RDONLY case.
(If BIT9 isn't disabled, the HW treat it as the PA[32] case then it will
translation fault..)

like: IO_PGTABLE_QUIRK_SHORT_MTK ?


> > 
> > Will
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek



[-- Attachment #1.2: Type: text/html, Size: 5265 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]