On 2021-03-12 12:47 p.m., Robin Murphy wrote:

quoted

quoted

quoted

   {
       struct scatterlist *s, *cur = sg;
       unsigned long seg_mask = dma_get_seg_boundary(dev);

@@ -864,6 +865,20 @@ static int __finalise_sg(struct device *dev,

struct scatterlist *sg, int nents,
           sg_dma_address(s) = DMA_MAPPING_ERROR;
           sg_dma_len(s) = 0;
   +        if (is_pci_p2pdma_page(sg_page(s)) && !s_iova_len) {
+            if (i > 0)
+                cur = sg_next(cur);
+
+            sg_dma_address(cur) = sg_phys(s) + s->offset -

Are you sure about that? ;)

Do you see a bug? I don't follow you...

sg_phys() already accounts for the offset, so you're adding it twice.

Ah, oops. Nice catch. I missed that.

quoted

quoted

quoted

+                pci_p2pdma_bus_offset(sg_page(s));

Can the bus offset make P2P addresses overlap with regions of mem space
that we might use for regular IOVA allocation? That would be very bad...

No. IOMMU drivers already disallow all PCI addresses from being used as
IOVA addresses. See, for example,  dmar_init_reserved_ranges(). It would
be a huge problem for a whole lot of other reasons if it didn't.

I know we reserve the outbound windows (largely *because* some host 
bridges will consider those addresses as attempts at unsupported P2P and 
prevent them working), I just wanted to confirm that this bus offset is 
always something small that stays within the relevant window, rather 
than something that might make a BAR appear in a completely different 
place for P2P purposes. If so, that's good.

Yes, well if an IOVA overlaps with any PCI bus address there's going to 
be some difficult brokenness because when the IOVA is used it might be 
directed to the a PCI device and not the IOMMU. I fixed a bug like that 
once.

quoted

quoted

I'm not really thrilled about the idea of passing zero-length segments
to iommu_map_sg(). Yes, it happens to trick the concatenation logic in
the current implementation into doing what you want, but it feels 
fragile.

We're not passing zero length segments to iommu_map_sg() (or any
function). This loop is just scanning to calculate the length of the
required IOVA. __finalise_sg() (which is intimately tied to this loop)
then needs a way to determine which segments were P2P segments. The
existing code already overwrites s->length with an aligned length and
stores the original length in sg_dma_len. So we're not relying on
tricking any logic here.

Yes, we temporarily shuffle in page-aligned quantities to satisfy the 
needs of the iommu_map_sg() call, before unpacking things again in 
__finalise_sg(). It's some disgusting trickery that I'm particularly 
proud of. My point is that if you have a mix of both p2p and normal 
segments - which seems to be a case you want to support - then the 
length of 0 that you set to flag p2p segments here will be seen by 
iommu_map_sg() (as it walks the list to map the other segments) before 
you then use it as a key to override the DMA address in the final step. 
It's not a concern if you have a p2p-only list and short-circuit 
straight to that step (in which case all the shuffling was wasted effort 
anyway), but since it's not entirely clear what a segment with zero 
length would mean in general, it seems like a good idea to avoid passing 
the list across a public boundary in that state, if possible.

Ok, well, I mean the iommu_map_sg() does the right thing as is without 
changing it and IMO sg->length set to zero does make sense. Supporting 
mixed P2P and normal segments is really the whole point of this series 
(the current kernel supports homogeneous SGLs with a specialized path -- 
see pci_p2pdma_unmap_sg_attrs()). But do you have an alternate solution 
for sg->length?

Logan