Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1224080 > unrolled thread

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

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.

Contents

  Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table  allocator. Yong Wu <yong.wu@mediatek.com> - 2015-09-14 14:30 +0200
    Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table  allocator. Will Deacon <will.deacon@arm.com> - 2015-09-16 15:00 +0200
      Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table  allocator. Yong Wu <yong.wu@mediatek.com> - 2015-09-17 04:40 +0200

#1224080 — 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 0xfd780000.
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
0xfd780000.
   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 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]

#1226066

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 0xfd780000.
> 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
> 0xfd780000.
>    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 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]

#1226633

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 0xfd780000.
> > 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
> > 0xfd780000.
> >    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 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