Hi Tomasz,
Thanks very much for you suggestion and explain so detail.
please help check below.
On Fri, 2015-03-27 at 18:41 +0900, Tomasz Figa wrote:
> Hi Yong Wu,
>
> Sorry for long delay, I had to figure out some time to look at this again.
>
> On Wed, Mar 18, 2015 at 8:22 PM, Yong Wu <[email protected]> wrote:
> >>
> >> > + imudev = piommu->dev;
> >> > +
> >> > + spin_lock_irqsave(&priv->portlock, flags);
> >>
> >> What is protected by this spinlock?
> > We will write a register of the local arbiter while config port. If
> > some modules are in the same local arbiter, it may be overwrite. so I
> > add it here.
> >>
>
> OK. Maybe it could be called larb_lock then? It would be good to have
> structures or code that should be running under this spinlock
> annotated with proper comments. And purpose of the lock documented in
> a comment as well (probably in a kerneldoc-style documentation of
> priv).
Thanks. I have move the spinlock into the smi driver, it will lock
for writing the local arbiter regsiter only.
>
> >> > +static void mtk_iommu_detach_device(struct iommu_domain *domain,
> >> > + struct device *dev)
> >> > +{
> >>
> >> No hardware (de)configuration or clean-up necessary?
> > I will add it. Actually we design like this:If a device have attached to
> > iommu domain, it won't detach from it.
>
> Isn't proper clean-up required for module removal? Some drivers might
> be required to be loadable modules, which should be unloadable.
>
> >>
> >> > +
> >> > + piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
> >>
> >> style: Operators like * should have space on both sides.
> >>
> >> > + GFP_KERNEL);
> >>
> >> Shouldn't dma_alloc_coherent() be used for this?
> > We don't care the data in it. I think they are the same. Could you
> > help tell me why dma_alloc_coherent may be better.
>
> Can you guarantee that at the time you allocate the memory using
> devm_kmalloc() the memory is not dirty (i.e. some write back data are
> stored in CPU cache) and is not going to be written back in some time,
> overwriting data put there by IOMMU hardware?
>
As I noted in the function "mtk_iommu_hw_init":
/* protect memory,HW will write here while translation fault */
protectpa = __virt_to_phys(piommu->protect_va);
We don’t care the content of this buffer, It is ok even though its
data is dirty.
It seem to be a the protect memory. While a translation fault
happened, The iommu HW will overwrite here instead of writing to the
fault physical address which may be 0 or some random address.
> >> > +
> >> > + iommu_set_fault_handler(domain, mtk_iommu_fault_handler, piommu);
> >>
> >> I don't see any other drivers doing this. Isn't this for upper layers,
> >> so that they can set their own generic fault handlers?
> > I think that this function is related with the iommu domain, we
> > have only one multimedia iommu domain. so I add it after the iommu
> > domain are created.
>
> No, this function is for drivers of IOMMU clients (i.e. master IP
> blocks) which want to subscribe to page fault to do things like paging
> on demand and so on. It shouldn't be called by IOMMU driver. Please
> see other IOMMU drivers, for example rockchip-iommmu.c.
Thanks. I have read it. I will delete it and print the error info
in the ISR. Also call the report_iommu_fault in the ISR.
> Best regards,
> Tomasz
On Tue, Apr 14, 2015 at 3:31 PM, Yong Wu <[email protected]> wrote:
>> >>
>> >> > +
>> >> > + piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
>> >>
>> >> style: Operators like * should have space on both sides.
>> >>
>> >> > + GFP_KERNEL);
>> >>
>> >> Shouldn't dma_alloc_coherent() be used for this?
>> > We don't care the data in it. I think they are the same. Could you
>> > help tell me why dma_alloc_coherent may be better.
>>
>> Can you guarantee that at the time you allocate the memory using
>> devm_kmalloc() the memory is not dirty (i.e. some write back data are
>> stored in CPU cache) and is not going to be written back in some time,
>> overwriting data put there by IOMMU hardware?
>>
> As I noted in the function "mtk_iommu_hw_init":
>
> /* protect memory,HW will write here while translation fault */
> protectpa = __virt_to_phys(piommu->protect_va);
>
> We don’t care the content of this buffer, It is ok even though its
> data is dirty.
> It seem to be a the protect memory. While a translation fault
> happened, The iommu HW will overwrite here instead of writing to the
> fault physical address which may be 0 or some random address.
>
Do you mean that it's just a dummy page for hardware behind the IOMMU
to access when the mapping is not available? How would that work with
potential on demand paging when the hardware needs to be blocked until
the mapping is created?
Best regards,
Tomasz
On Wed, 2015-04-15 at 11:20 +0900, Tomasz Figa wrote:
> On Tue, Apr 14, 2015 at 3:31 PM, Yong Wu <[email protected]> wrote:
> >> >>
> >> >> > +
> >> >> > + piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
> >> >>
> >> >> style: Operators like * should have space on both sides.
> >> >>
> >> >> > + GFP_KERNEL);
> >> >>
> >> >> Shouldn't dma_alloc_coherent() be used for this?
> >> > We don't care the data in it. I think they are the same. Could you
> >> > help tell me why dma_alloc_coherent may be better.
> >>
> >> Can you guarantee that at the time you allocate the memory using
> >> devm_kmalloc() the memory is not dirty (i.e. some write back data are
> >> stored in CPU cache) and is not going to be written back in some time,
> >> overwriting data put there by IOMMU hardware?
> >>
> > As I noted in the function "mtk_iommu_hw_init":
> >
> > /* protect memory,HW will write here while translation fault */
> > protectpa = __virt_to_phys(piommu->protect_va);
> >
> > We don’t care the content of this buffer, It is ok even though its
> > data is dirty.
> > It seem to be a the protect memory. While a translation fault
> > happened, The iommu HW will overwrite here instead of writing to the
> > fault physical address which may be 0 or some random address.
> >
>
> Do you mean that it's just a dummy page for hardware behind the IOMMU
> to access when the mapping is not available? How would that work with
> potential on demand paging when the hardware needs to be blocked until
> the mapping is created?
>
> Best regards,
> Tomasz
1. YES
2. Sorry. Our iommu HW can not support this right now. The HW can not
be blocked until the mapping is created.
If the page is not ready, we can not get the physical address, then
How to fill the pagetable for that memory. I think the dma&iommu may
guaranty it?
On Wed, Apr 15, 2015 at 4:06 PM, Yong Wu <[email protected]> wrote:
> On Wed, 2015-04-15 at 11:20 +0900, Tomasz Figa wrote:
>> On Tue, Apr 14, 2015 at 3:31 PM, Yong Wu <[email protected]> wrote:
>> >> >>
>> >> >> > +
>> >> >> > + piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
>> >> >>
>> >> >> style: Operators like * should have space on both sides.
>> >> >>
>> >> >> > + GFP_KERNEL);
>> >> >>
>> >> >> Shouldn't dma_alloc_coherent() be used for this?
>> >> > We don't care the data in it. I think they are the same. Could you
>> >> > help tell me why dma_alloc_coherent may be better.
>> >>
>> >> Can you guarantee that at the time you allocate the memory using
>> >> devm_kmalloc() the memory is not dirty (i.e. some write back data are
>> >> stored in CPU cache) and is not going to be written back in some time,
>> >> overwriting data put there by IOMMU hardware?
>> >>
>> > As I noted in the function "mtk_iommu_hw_init":
>> >
>> > /* protect memory,HW will write here while translation fault */
>> > protectpa = __virt_to_phys(piommu->protect_va);
>> >
>> > We don’t care the content of this buffer, It is ok even though its
>> > data is dirty.
>> > It seem to be a the protect memory. While a translation fault
>> > happened, The iommu HW will overwrite here instead of writing to the
>> > fault physical address which may be 0 or some random address.
>> >
>>
>> Do you mean that it's just a dummy page for hardware behind the IOMMU
>> to access when the mapping is not available? How would that work with
>> potential on demand paging when the hardware needs to be blocked until
>> the mapping is created?
>>
>> Best regards,
>> Tomasz
> 1. YES
> 2. Sorry. Our iommu HW can not support this right now. The HW can not
> be blocked until the mapping is created.
OK, that explains it. Well, then I guess this is necessary and
contents of that memory don't matter that much. (Although, this might
be a minor security issue, because the faulting hardware would get
access to some data previously stored by kernel code. Not sure how
much of a threat would that be, though.)
> If the page is not ready, we can not get the physical address, then
> How to fill the pagetable for that memory. I think the dma&iommu may
> guaranty it?
If your hardware can't block until the mapping is created then what
you do currently seems to be the only option. (+/- the missing cache
maintenance at initialization)
Best regards,
Tomasz