Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1192649 > unrolled thread
| Started by | Yong Wu <yong.wu@mediatek.com> |
|---|---|
| First post | 2015-07-27 06:30 +0200 |
| Last post | 2015-07-28 13:10 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator. Yong Wu <yong.wu@mediatek.com> - 2015-07-27 06:30 +0200
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator. Robin Murphy <robin.murphy@arm.com> - 2015-07-27 16:10 +0200
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator. Will Deacon <will.deacon@arm.com> - 2015-07-27 16:20 +0200
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator. Will Deacon <will.deacon@arm.com> - 2015-07-28 13:10 +0200
| From | Yong Wu <yong.wu@mediatek.com> |
|---|---|
| Date | 2015-07-27 06:30 +0200 |
| Subject | Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator. |
| Message-ID | <pQId3-7Ap-1@gated-at.bofh.it> |
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.
>
> > > > +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.
There is some difference between the standard spec and MTK HW,
Our hw don't implement some bits, like XN and AP.
So I add a quirk for MTK special.
>
> > > > + 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.
Thanks. I will add :
tlb->flush_pgtable(pte_va, ARM_SHORT_BYTES_PER_PTE, cookie);
>
> Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Robin Murphy <robin.murphy@arm.com> |
|---|---|
| Date | 2015-07-27 16:10 +0200 |
| Subject | Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator. |
| Message-ID | <pQRgo-46U-63@gated-at.bofh.it> |
| In reply to | #1192649 |
On 27/07/15 05:21, Yong Wu wrote:
[...]
>>>>> +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.
>
>>
>>>>> +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.
>
> There is some difference between the standard spec and MTK HW,
> Our hw don't implement some bits, like XN and AP.
> So I add a quirk for MTK special.
When you say it doesn't implement these bits, do you mean that having
them set will lead to Bad Things happening in the hardware, or that it
will simply ignore them and not enforce any of the protections they
imply? The former case would definitely want clearly documenting
somewhere, whereas for the latter case I'm not sure it's even worth the
complication of having a quirk - if the value doesn't matter there seems
little point in doing a special dance just for the sake of semantic
correctness of the in-memory PTEs, in my opinion.
Robin.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Will Deacon <will.deacon@arm.com> |
|---|---|
| Date | 2015-07-27 16:20 +0200 |
| Message-ID | <pQRq2-4iB-21@gated-at.bofh.it> |
| In reply to | #1193155 |
On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> On 27/07/15 05:21, Yong Wu wrote:
> >>>>> + } 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.
> >
> > There is some difference between the standard spec and MTK HW,
> > Our hw don't implement some bits, like XN and AP.
> > So I add a quirk for MTK special.
>
> When you say it doesn't implement these bits, do you mean that having
> them set will lead to Bad Things happening in the hardware, or that it
> will simply ignore them and not enforce any of the protections they
> imply? The former case would definitely want clearly documenting
> somewhere, whereas for the latter case I'm not sure it's even worth the
> complication of having a quirk - if the value doesn't matter there seems
> little point in doing a special dance just for the sake of semantic
> correctness of the in-memory PTEs, in my opinion.
Agreed. We should only use quirks if the current (architecturally
compliant) code causes real issues with the hardware. Then the quirk can
be used to either avoid the problematic routines or to take extra steps
to make things work as the architecture intended.
I've asked how this IOMMU differs from the architecture on a number of
occasions, but I'm still yet to receive a response other than "it's special".
Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Will Deacon <will.deacon@arm.com> |
|---|---|
| Date | 2015-07-28 13:10 +0200 |
| Message-ID | <pRaVI-7pC-21@gated-at.bofh.it> |
| In reply to | #1193164 |
On Tue, Jul 28, 2015 at 06:08:14AM +0100, Yong Wu wrote:
> On Mon, 2015-07-27 at 15:11 +0100, Will Deacon wrote:
> > On Mon, Jul 27, 2015 at 03:05:38PM +0100, Robin Murphy wrote:
> > > On 27/07/15 05:21, Yong Wu wrote:
> > > >>>>> + } 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.
> > > >
> > > > There is some difference between the standard spec and MTK HW,
> > > > Our hw don't implement some bits, like XN and AP.
> > > > So I add a quirk for MTK special.
> > >
> > > When you say it doesn't implement these bits, do you mean that having
> > > them set will lead to Bad Things happening in the hardware, or that it
> > > will simply ignore them and not enforce any of the protections they
> > > imply? The former case would definitely want clearly documenting
> > > somewhere, whereas for the latter case I'm not sure it's even worth the
> > > complication of having a quirk - if the value doesn't matter there seems
> > > little point in doing a special dance just for the sake of semantic
> > > correctness of the in-memory PTEs, in my opinion.
> >
> > Agreed. We should only use quirks if the current (architecturally
> > compliant) code causes real issues with the hardware. Then the quirk can
> > be used to either avoid the problematic routines or to take extra steps
> > to make things work as the architecture intended.
> >
> > I've asked how this IOMMU differs from the architecture on a number of
> > occasions, but I'm still yet to receive a response other than "it's special".
> >
>
> After check further with DE, Our pagetable is refer to ARM-v7's
> short-descriptor which is a little different from ARM-v8. like bit0(PXN)
> in section and supersection, I didn't read ARM-v7 spec before, so I add
> a MTK quirk to disable PXN bit in section and supersection.(if the PXN
> bit is wrote in ARM-v7 spec, HW will page fault.)
I've been reviewing this using the ARMv7 ARM (Rev.C of DDI0406C) the whole
time. PXN is there as an optional field in non-LPAE implementations. That's
fine and doesn't require any quirks.
> Then I write this code according to ARM-v8 spec defaultly, and add a
> ARM-v7 quirk?
No, I don't think you need this, as the v8 and v7 short-descriptor formats
look compatible to me. You should only need a quirk if architecturally
compliant code cannot work on your hardware.
> And there is a little different between ARM-v7 spec and MTK pagetable.
> It's the XN(bit0) in small page. MTK don't implement XN bit.
> The bit[1:0] in MTK's small page should be 2'b10, if it's 2'b11, HW will
> page fault.
Aha, thanks! *That* is worthy of a quirk. Something like:
IO_PGTABLE_QUIRK_ARM_NO_XN
> (MTK don't implement AP bits too, but HW don't use them, it is ok even
> though AP bits is wrote)
Yeah, I think that's fine. The pgtable code will honour the request but
the h/w will ignore it.
> In the end, I will add two quirk like this, is it OK?
I think you only need the one I mentioned above. I don't see the need
for PXN at all (as I said in the last review).
Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web