Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
On Wed, 2015-09-16 at 13:55 +0100, Will Deacon wrote:
> Hello Yong,
>
> On Mon, Sep 14, 2015 at 01:25:00PM +0100, Yong Wu wrote:
> > On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > > > + ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot,
> > > > large);
> > > > +
> > > > + tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
> > > > + tlb->tlb_sync(data->iop.cookie);
> > >
> > > In _arm_short_map, it looks like you can only go from invalid -> valid,
> > > so why do you need to flush the TLB here?
> >
> > Hi Will,
> >Here is about flush-tlb after map iova, I have deleted it in v4
> > following this suggestion. But We meet a problem about it.
>
> Ok.
>
> > Take a example with JPEG. the test steps is:
> > a).JPEG HW decode a picture with the source iova,like 0xfd78.
> > b).JPEG HW decode done, It will unmap the iova(write 0 in pagetable and
> > flush tlb).
> > c).JPEG HW decode the second picture, whose source iova is also
> > 0xfd78.
> >Then our HW maybe fail due to it will auto prefetch, It may prefecth
> > between the step b) and c). then the HW may fetch the pagetable content
> > which has been unmapped in step b). then the HW will get the iova's
> > physical address is 0, It will translation fault!
>
> Oh no! So-called "negative caching" is certainly prohibited by the ARM
> architecture, but if you've built it then we can probably work around it
> as an additional quirk. I assume the prefetcher stops prefetching when
> it sees an invalid descriptor?
Yes, If it's a invalid descriptor, the HW will stop prefetch.
>
> > So I think our HW need flush-tlb after map iova. Could we add a
> > QUIRK like "IO_PGTABLE_QUIRK_AUTO_PREFETCH_ENABLE" for it?
> > If it's not allowed, we will have to add this in our internal function
> > mtk_iommu_map of mtk_iommu.c.
>
> Actually, this type of quirk is ringing bells with me (I think another
> IOMMU needed something similar in the past), so maybe just add
> IO_PGTABLE_QUIRK_TLBI_ON_MAP?
Thanks. I will add it like:
//=
ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
if (data->iop.cfg.quirk & IO_PGTABLE_QUIRK_TLBI_ON_MAP) {
tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
tlb->tlb_sync(data->iop.cookie);
}
//==
It will flush-tlb every time after map-iova. then the HW will fetch the
new PA from the dram.
>
> Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
Hello Yong, On Mon, Sep 14, 2015 at 01:25:00PM +0100, Yong Wu wrote: > On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote: > > > + ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large); > > > + > > > + tlb->tlb_add_flush(iova, size, true, data->iop.cookie); > > > + tlb->tlb_sync(data->iop.cookie); > > > > In _arm_short_map, it looks like you can only go from invalid -> valid, > > so why do you need to flush the TLB here? > > Hi Will, >Here is about flush-tlb after map iova, I have deleted it in v4 > following this suggestion. But We meet a problem about it. Ok. > Take a example with JPEG. the test steps is: > a).JPEG HW decode a picture with the source iova,like 0xfd78. > b).JPEG HW decode done, It will unmap the iova(write 0 in pagetable and > flush tlb). > c).JPEG HW decode the second picture, whose source iova is also > 0xfd78. >Then our HW maybe fail due to it will auto prefetch, It may prefecth > between the step b) and c). then the HW may fetch the pagetable content > which has been unmapped in step b). then the HW will get the iova's > physical address is 0, It will translation fault! Oh no! So-called "negative caching" is certainly prohibited by the ARM architecture, but if you've built it then we can probably work around it as an additional quirk. I assume the prefetcher stops prefetching when it sees an invalid descriptor? > So I think our HW need flush-tlb after map iova. Could we add a > QUIRK like "IO_PGTABLE_QUIRK_AUTO_PREFETCH_ENABLE" for it? > If it's not allowed, we will have to add this in our internal function > mtk_iommu_map of mtk_iommu.c. Actually, this type of quirk is ringing bells with me (I think another IOMMU needed something similar in the past), so maybe just add IO_PGTABLE_QUIRK_TLBI_ON_MAP? Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
[...]
> > +static int arm_short_map(struct io_pgtable_ops *ops, unsigned long iova,
> > +phys_addr_t paddr, size_t size, int prot)
> > +{
> > + struct arm_short_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > + const struct iommu_gather_ops *tlb = data->iop.cfg.tlb;
> > + int ret;
> > + arm_short_iopte pgdprot = 0, pteprot = 0;
> > + bool large;
> > +
> > + /* If no access, then nothing to do */
> > + if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
> > + return 0;
> > +
> > + switch (size) {
> > + case SZ_4K:
> > + case SZ_64K:
> > + large = (size == SZ_64K) ? true : false;
> > + pteprot = __arm_short_pte_prot(data, prot, large);
> > + pgdprot = __arm_short_pgtable_prot(data, prot &
> > IOMMU_NOEXEC);
> > + break;
> > +
> > + case SZ_1M:
> > + case SZ_16M:
> > + large = (size == SZ_16M) ? true : false;
> > + pgdprot = __arm_short_pgd_prot(data, prot, large);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (WARN_ON((iova | paddr) & (size - 1)))
> > + return -EINVAL;
> > +
> > + ret = _arm_short_map(data, iova, paddr, pgdprot, pteprot, large);
> > +
> > + tlb->tlb_add_flush(iova, size, true, data->iop.cookie);
> > + tlb->tlb_sync(data->iop.cookie);
>
> In _arm_short_map, it looks like you can only go from invalid -> valid,
> so why do you need to flush the TLB here?
Hi Will,
Here is about flush-tlb after map iova, I have deleted it in v4
following this suggestion. But We meet a problem about it.
Take a example with JPEG. the test steps is:
a).JPEG HW decode a picture with the source iova,like 0xfd78.
b).JPEG HW decode done, It will unmap the iova(write 0 in pagetable and
flush tlb).
c).JPEG HW decode the second picture, whose source iova is also
0xfd78.
Then our HW maybe fail due to it will auto prefetch, It may prefecth
between the step b) and c). then the HW may fetch the pagetable content
which has been unmapped in step b). then the HW will get the iova's
physical address is 0, It will translation fault!
So I think our HW need flush-tlb after map iova. Could we add a
QUIRK like "IO_PGTABLE_QUIRK_AUTO_PREFETCH_ENABLE" for it?
If it's not allowed, we will have to add this in our internal function
mtk_iommu_map of mtk_iommu.c.
Thanks.
>
> > + return ret;
> > +}
> > +
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
On Fri, Jul 31, 2015 at 08:55:37AM +0100, Yong Wu wrote: > 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. Aha, now *thats* a case of page-table abuse! > 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 ? Given that you don't have XN either, maybe IO_PGTABLE_QUIRK_NO_PERMS? When set, IOMMU_READ/WRITE/EXEC are ignored and the mapping will never generate a permission fault. Will -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
On Tue, Jul 28, 2015 at 02:37:43PM +0100, Yong Wu wrote:
> On Tue, 2015-07-28 at 12:00 +0100, Will Deacon wrote:
> > 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.
>
> Thanks for your confirm.
> Then I delete all the PXN bit in here?
>
> Take a example,
> #define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4))
> I will change it to "BIT(4)".
Yes. Then the PXN bit can be added later as a quirk when we have an
implementation that supports it.
Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
On Tue, 2015-07-28 at 12:00 +0100, Will Deacon wrote:
> 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.
Thanks for your confirm.
Then I delete all the PXN bit in here?
Take a example,
#define ARM_SHORT_PGD_SECTION_XN(BIT(0) | BIT(4))
I will change it to "BIT(4)".
>
> > 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
Thanks, I will add it.
>
> > (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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
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.)
Then I write this code according to ARM-v8 spec defaultly, and add a
ARM-v7 quirk?
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.
(MTK don't implement AP bits too, but HW don't use them, it is ok even
though AP bits is wrote)
In the end, I will add two quirk like this, is it OK?
//===
#define ARM_PGTABLE_QUIRK_SHORT_ARM_V7 BIT(2) /* for ARM-v7 while
default is the ARM-v8 spec */
#define ARM_PGTABLE_QUIRK_SHORT_MTK BIT(3) /* MTK special */
//===
In the ARM_V7 quirk, I only disable PXN bit in section and supersection,
In the MTK quirk, I only disbable XN in small page.
The other bits seems the same. I'm not sure I write clearly and It seems
we could not copy a image of mtk pagetable here..If any question, please
help tell me.
Thanks very much.
> Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
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_BBIT(2)
> > > > +#define ARM_SHORT_PTE_CBIT(3)
> > > > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
> > > > +#define ARM_SHORT_PTE_IMPLEBIT(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
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
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_BBIT(2)
> > > +#define ARM_SHORT_PTE_CBIT(3)
> > > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
> > > +#define ARM_SHORT_PTE_IMPLEBIT(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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
Hi Will, Thanks for your review so detail. When you are free, please help me check whether it's ok if it's changed like below. Thanks very much. On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote: > Hello, > > This is looking better, but I still have some concerns. > > On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote: > > This patch is for ARM Short Descriptor Format. > > > > Signed-off-by: Yong Wu > > --- > > drivers/iommu/Kconfig| 18 + > > drivers/iommu/Makefile |1 + > > drivers/iommu/io-pgtable-arm-short.c | 742 > > ++ > > drivers/iommu/io-pgtable-arm.c |3 - > > drivers/iommu/io-pgtable.c |4 + > > drivers/iommu/io-pgtable.h | 13 + > > 6 files changed, 778 insertions(+), 3 deletions(-) > > create mode 100644 drivers/iommu/io-pgtable-arm-short.c > > [...] > > > +#define ARM_SHORT_PGDIR_SHIFT 20 > > +#define ARM_SHORT_PAGE_SHIFT 12 > > +#define ARM_SHORT_PTRS_PER_PTE \ > > + (1 << (ARM_SHORT_PGDIR_SHIFT - ARM_SHORT_PAGE_SHIFT)) > > +#define ARM_SHORT_BYTES_PER_PTE\ > > + (ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte)) > > + > > +/* level 1 pagetable */ > > +#define ARM_SHORT_PGD_TYPE_PGTABLE BIT(0) > > +#define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4)) > > +#define ARM_SHORT_PGD_TYPE_SECTION BIT(1) > > +#define ARM_SHORT_PGD_PGTABLE_XN BIT(2) > > This should be PXN, but I'm not even sure why we care for a table at > level 1. I will delete it. > > > +#define ARM_SHORT_PGD_BBIT(2) > > +#define ARM_SHORT_PGD_CBIT(3) > > +#define ARM_SHORT_PGD_PGTABLE_NS BIT(3) > > +#define ARM_SHORT_PGD_IMPLEBIT(9) > > +#define ARM_SHORT_PGD_TEX0 BIT(12) > > +#define ARM_SHORT_PGD_SBIT(16) > > +#define ARM_SHORT_PGD_nG BIT(17) > > +#define ARM_SHORT_PGD_SUPERSECTION BIT(18) > > +#define ARM_SHORT_PGD_SECTION_NS BIT(19) > > + > > +#define ARM_SHORT_PGD_TYPE_SUPERSECTION\ > > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION) > > +#define ARM_SHORT_PGD_SECTION_TYPE_MSK \ > > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION) > > +#define ARM_SHORT_PGD_PGTABLE_TYPE_MSK \ > > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_TYPE_PGTABLE) > > +#define ARM_SHORT_PGD_TYPE_IS_PGTABLE(pgd) \ > > + (((pgd) & ARM_SHORT_PGD_PGTABLE_TYPE_MSK) == > > ARM_SHORT_PGD_TYPE_PGTABLE) > > +#define ARM_SHORT_PGD_TYPE_IS_SECTION(pgd) \ > > + (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == > > ARM_SHORT_PGD_TYPE_SECTION) > > +#define ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(pgd)\ > > + (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == \ > > + ARM_SHORT_PGD_TYPE_SUPERSECTION) > > +#define ARM_SHORT_PGD_PGTABLE_MSK 0xfc00 > > +#define ARM_SHORT_PGD_SECTION_MSK (~(SZ_1M - 1)) > > +#define ARM_SHORT_PGD_SUPERSECTION_MSK (~(SZ_16M - 1)) > > + > > +/* 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_BBIT(2) > > +#define ARM_SHORT_PTE_CBIT(3) > > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6) > > +#define ARM_SHORT_PTE_IMPLEBIT(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. > > > +#define ARM_SHORT_PTE_SBIT(10) > > +#define ARM_SHORT_PTE_nG BIT(11) > > +#define ARM_SHORT_PTE_LARGE_TEX0 BIT(12) > > +#define ARM_SHORT_PTE_LARGE_XN BIT(15) > > +#define ARM_SHORT_PTE_LARGE_MSK(~(SZ_64K - 1)) > > +#define ARM_SHORT_PTE_SMALL_MSK(~(SZ_4K - 1)) > > +#define ARM_SHORT_PTE_TYPE_MSK \ > > + (ARM_SHORT_PTE_TYPE_LARGE | ARM_SHORT_PTE_TYPE_SMALL) > > +#define ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(pte) \ > > + (pte) & ARM_SHORT_PTE_TYPE_MSK) >> 1) << 1)\ > > + == ARM_SHORT_PTE_TYPE_SMALL) > > I see what you're trying to do here, but the shifting is confusing. I > think it's better doing something like: > > ((pte) & ARM_SHORT_PTE_TYPE_SMALL) == ARM_SHORT_PTE_TYPE_SMALL > > or even just: > > (pte) & ARM_SHORT_PTE_TYPE_SMALL Thanks. I will use this: ((pte) & ARM_SHORT_PTE_TYPE_SMALL) == ARM_SHORT_PTE_TYPE_S
Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
Hello, This is looking better, but I still have some concerns. On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote: > This patch is for ARM Short Descriptor Format. > > Signed-off-by: Yong Wu > --- > drivers/iommu/Kconfig| 18 + > drivers/iommu/Makefile |1 + > drivers/iommu/io-pgtable-arm-short.c | 742 > ++ > drivers/iommu/io-pgtable-arm.c |3 - > drivers/iommu/io-pgtable.c |4 + > drivers/iommu/io-pgtable.h | 13 + > 6 files changed, 778 insertions(+), 3 deletions(-) > create mode 100644 drivers/iommu/io-pgtable-arm-short.c [...] > +#define ARM_SHORT_PGDIR_SHIFT 20 > +#define ARM_SHORT_PAGE_SHIFT 12 > +#define ARM_SHORT_PTRS_PER_PTE \ > + (1 << (ARM_SHORT_PGDIR_SHIFT - ARM_SHORT_PAGE_SHIFT)) > +#define ARM_SHORT_BYTES_PER_PTE\ > + (ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte)) > + > +/* level 1 pagetable */ > +#define ARM_SHORT_PGD_TYPE_PGTABLE BIT(0) > +#define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4)) > +#define ARM_SHORT_PGD_TYPE_SECTION BIT(1) > +#define ARM_SHORT_PGD_PGTABLE_XN BIT(2) This should be PXN, but I'm not even sure why we care for a table at level 1. > +#define ARM_SHORT_PGD_BBIT(2) > +#define ARM_SHORT_PGD_CBIT(3) > +#define ARM_SHORT_PGD_PGTABLE_NS BIT(3) > +#define ARM_SHORT_PGD_IMPLEBIT(9) > +#define ARM_SHORT_PGD_TEX0 BIT(12) > +#define ARM_SHORT_PGD_SBIT(16) > +#define ARM_SHORT_PGD_nG BIT(17) > +#define ARM_SHORT_PGD_SUPERSECTION BIT(18) > +#define ARM_SHORT_PGD_SECTION_NS BIT(19) > + > +#define ARM_SHORT_PGD_TYPE_SUPERSECTION\ > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION) > +#define ARM_SHORT_PGD_SECTION_TYPE_MSK \ > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION) > +#define ARM_SHORT_PGD_PGTABLE_TYPE_MSK \ > + (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_TYPE_PGTABLE) > +#define ARM_SHORT_PGD_TYPE_IS_PGTABLE(pgd) \ > + (((pgd) & ARM_SHORT_PGD_PGTABLE_TYPE_MSK) == > ARM_SHORT_PGD_TYPE_PGTABLE) > +#define ARM_SHORT_PGD_TYPE_IS_SECTION(pgd) \ > + (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == > ARM_SHORT_PGD_TYPE_SECTION) > +#define ARM_SHORT_PGD_TYPE_IS_SUPERSECTION(pgd)\ > + (((pgd) & ARM_SHORT_PGD_SECTION_TYPE_MSK) == \ > + ARM_SHORT_PGD_TYPE_SUPERSECTION) > +#define ARM_SHORT_PGD_PGTABLE_MSK 0xfc00 > +#define ARM_SHORT_PGD_SECTION_MSK (~(SZ_1M - 1)) > +#define ARM_SHORT_PGD_SUPERSECTION_MSK (~(SZ_16M - 1)) > + > +/* 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_BBIT(2) > +#define ARM_SHORT_PTE_CBIT(3) > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6) > +#define ARM_SHORT_PTE_IMPLEBIT(9) This is AP[2] for small pages. > +#define ARM_SHORT_PTE_SBIT(10) > +#define ARM_SHORT_PTE_nG BIT(11) > +#define ARM_SHORT_PTE_LARGE_TEX0 BIT(12) > +#define ARM_SHORT_PTE_LARGE_XN BIT(15) > +#define ARM_SHORT_PTE_LARGE_MSK(~(SZ_64K - 1)) > +#define ARM_SHORT_PTE_SMALL_MSK(~(SZ_4K - 1)) > +#define ARM_SHORT_PTE_TYPE_MSK \ > + (ARM_SHORT_PTE_TYPE_LARGE | ARM_SHORT_PTE_TYPE_SMALL) > +#define ARM_SHORT_PTE_TYPE_IS_SMALLPAGE(pte) \ > + (pte) & ARM_SHORT_PTE_TYPE_MSK) >> 1) << 1)\ > + == ARM_SHORT_PTE_TYPE_SMALL) I see what you're trying to do here, but the shifting is confusing. I think it's better doing something like: ((pte) & ARM_SHORT_PTE_TYPE_SMALL) == ARM_SHORT_PTE_TYPE_SMALL or even just: (pte) & ARM_SHORT_PTE_TYPE_SMALL > +#define ARM_SHORT_PTE_TYPE_IS_LARGEPAGE(pte) \ > + (((pte) & ARM_SHORT_PTE_TYPE_MSK) == ARM_SHORT_PTE_TYPE_LARGE) > + > +#define ARM_SHORT_PGD_IDX(a) ((a) >> ARM_SHORT_PGDIR_SHIFT) > +#define ARM_SHORT_PTE_IDX(a) \ > + (((a) >> ARM_SHORT_PAGE_SHIFT) & (ARM_SHORT_PTRS_PER_PTE - 1)) > + > +#define ARM_SHORT_GET_PTE_VA(pgd) \ > + (phys_to_virt((unsigned long)pgd & ARM_SHORT_PGD_PGTABLE_MSK)) Maybe better named as ARM_SHORT_GET_PGTABLE_VA ? > + > +#define ARM_SHORT_PTE_LARGE_GET_PROT(pte) \ > + (((pte) & (~ARM_SHORT_PTE_LARGE_MSK)) & ~ARM_SHORT_PTE_TYPE_MSK) > + > +#define ARM_SHORT_PGD_GET_PROT(pgd)\ > + (((pgd) & (~ARM_SHORT_
[PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.
This patch is for ARM Short Descriptor Format.
Signed-off-by: Yong Wu
---
drivers/iommu/Kconfig| 18 +
drivers/iommu/Makefile |1 +
drivers/iommu/io-pgtable-arm-short.c | 742 ++
drivers/iommu/io-pgtable-arm.c |3 -
drivers/iommu/io-pgtable.c |4 +
drivers/iommu/io-pgtable.h | 13 +
6 files changed, 778 insertions(+), 3 deletions(-)
create mode 100644 drivers/iommu/io-pgtable-arm-short.c
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f1fb1d3..f50dbf3 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -39,6 +39,24 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
If unsure, say N here.
+config IOMMU_IO_PGTABLE_SHORT
+ bool "ARMv7/v8 Short Descriptor Format"
+ select IOMMU_IO_PGTABLE
+ depends on ARM || ARM64 || COMPILE_TEST
+ help
+ Enable support for the ARM Short descriptor pagetable format.
+ This allocator supports 2 levels translation tables which supports
+ a memory map based on memory sections or pages.
+
+config IOMMU_IO_PGTABLE_SHORT_SELFTEST
+ bool "Short Descriptor selftests"
+ depends on IOMMU_IO_PGTABLE_SHORT
+ help
+ Enable self-tests for Short Descriptor page table allocator.
+ This performs a series of page-table consistency checks during boot.
+
+ If unsure, say N here.
+
endmenu
config IOMMU_IOVA
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index c6dcc51..06df3e6 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_IOMMU_API) += iommu-traces.o
obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
+obj-$(CONFIG_IOMMU_IO_PGTABLE_SHORT) += io-pgtable-arm-short.o
obj-$(CONFIG_IOMMU_IOVA) += iova.o
obj-$(CONFIG_OF_IOMMU) += of_iommu.o
obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
diff --git a/drivers/iommu/io-pgtable-arm-short.c
b/drivers/iommu/io-pgtable-arm-short.c
new file mode 100644
index 000..340d590
--- /dev/null
+++ b/drivers/iommu/io-pgtable-arm-short.c
@@ -0,0 +1,742 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#define pr_fmt(fmt)"arm-short-desc io-pgtable: "fmt
+
+#include
+#include
+#include
+#include
+
+#include "io-pgtable.h"
+
+typedef u32 arm_short_iopte;
+
+struct arm_short_io_pgtable {
+ struct io_pgtable iop;
+ struct kmem_cache *ptekmem;
+ size_t pgd_size;
+ void*pgd;
+};
+
+#define io_pgtable_to_data(x) \
+ container_of((x), struct arm_short_io_pgtable, iop)
+
+#define io_pgtable_ops_to_data(x) \
+ io_pgtable_to_data(io_pgtable_ops_to_pgtable(x))
+
+#define ARM_SHORT_PGDIR_SHIFT 20
+#define ARM_SHORT_PAGE_SHIFT 12
+#define ARM_SHORT_PTRS_PER_PTE \
+ (1 << (ARM_SHORT_PGDIR_SHIFT - ARM_SHORT_PAGE_SHIFT))
+#define ARM_SHORT_BYTES_PER_PTE\
+ (ARM_SHORT_PTRS_PER_PTE * sizeof(arm_short_iopte))
+
+/* level 1 pagetable */
+#define ARM_SHORT_PGD_TYPE_PGTABLE BIT(0)
+#define ARM_SHORT_PGD_SECTION_XN (BIT(0) | BIT(4))
+#define ARM_SHORT_PGD_TYPE_SECTION BIT(1)
+#define ARM_SHORT_PGD_PGTABLE_XN BIT(2)
+#define ARM_SHORT_PGD_BBIT(2)
+#define ARM_SHORT_PGD_CBIT(3)
+#define ARM_SHORT_PGD_PGTABLE_NS BIT(3)
+#define ARM_SHORT_PGD_IMPLEBIT(9)
+#define ARM_SHORT_PGD_TEX0 BIT(12)
+#define ARM_SHORT_PGD_SBIT(16)
+#define ARM_SHORT_PGD_nG BIT(17)
+#define ARM_SHORT_PGD_SUPERSECTION BIT(18)
+#define ARM_SHORT_PGD_SECTION_NS BIT(19)
+
+#define ARM_SHORT_PGD_TYPE_SUPERSECTION\
+ (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
+#define ARM_SHORT_PGD_SECTION_TYPE_MSK \
+ (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_SUPERSECTION)
+#define ARM_SHORT_PGD_PGTABLE_TYPE_MSK \
+ (ARM_SHORT_PGD_TYPE_SECTION | ARM_SHORT_PGD_TYPE_PGTABLE)
+#define ARM_SHORT_PGD_TYPE_IS_PGTABLE(pgd) \
+ (((pgd) & ARM_SHORT_PGD_PGTABLE_TYPE_MSK) == ARM_SHORT_PGD_TYPE_PGTABLE)
+#define ARM_SHORT_PGD_TYPE_IS_SECTION(pgd) \
+ (((pgd
