On Thu, Sep 05, 2024 at 09:07:36PM +0200, Jan Kiszka wrote:
> On 05.09.24 18:33, Bjorn Helgaas wrote:
> > [+cc Kishon, just in case you have time/interest ;)]
> >
> > On Wed, Sep 04, 2024 at 12:00:13PM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> >>
> >> The AM654 lacks an IOMMU, thus does not support isolating DMA requests
> >> from untrusted PCI devices to selected memory regions this way. Use
> >> static PVU-based protection instead.
> >>
> >> For this, we use the availability of restricted-dma-pool memory regions
> >> as trigger and register those as valid DMA targets with the PVU.
> >
> > I guess the implication is that DMA *outside* the restricted-dma-pool
> > just gets dropped, and the Requester would see Completion Timeouts or
> > something for reads?
>
> I cannot tell what happens on the PCI bus in that case, maybe someone
> from TI can help out.
>
> On the host side, the PVU will record an error and raise an interrupt
> which will make the driver report that to the kernel log. That's quite
> similar to what IOMMU drivers do on translation faults.
The main thing is that the DMA doesn't complete, as you mentioned
below.
> > Since there's no explicit use of "restricted-dma-pool" elsewhere in
> > this patch, I assume the setup above causes the controller to drop any
> > DMA accesses outside that pool? I think a comment about how the
> > controller behavior is being changed would be useful. Basically the
> > same comment as for the commit log.
>
> Right, this is what will happen. Will add some comment.
>
> > Would there be any value in a dmesg note about a restriction being
> > enforced? Seems like it's dependent on both CONFIG_TI_PVU and some DT
> > properties, and since those are invisible in the log, maybe a note
> > would help understand/debug any issues?
>
> This is what you will see when there are reserved region and PVU in
> play:
>
> keystone-pcie 5600000.pcie: assigned reserved memory node restricted-dma@c0000000
> ti-pvu 30f80000.iommu: created TLB entry 0.2: 0xc0000000, psize 4 (0x02000000)
> ti-pvu 30f80000.iommu: created TLB entry 0.3: 0xc2000000, psize 4 (0x02000000)
> ...
> ath9k 0000:01:00.0: assigned reserved memory node restricted-dma@c0000000
Looks reasonable and solves my concern.
> >> + of_for_each_phandle(&it, err, pdev->dev.of_node, "memory-region",
> >> + NULL, 0) {
> >> + if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
> >> + of_address_to_resource(it.node, 0, &phys) == 0)
> >> + ti_pvu_remove_region(KS_PCI_VIRTID, &phys);
> >
> > I guess it's not important to undo the PCIE_VMAP_xP_CTRL_EN and
> > related setup that was done by ks_init_restricted_dma()?
> >
>
> Right, I didn't find a reason to do that.
OK, as long as you considered it :)
Bjorn
