Message-ID: <45701356-ee41-1ad2-0e06-ca74af87b05a@deltatee.com>
Date:   Fri, 12 Mar 2021 10:03:38 -0700
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Robin Murphy <robin.murphy@....com>, linux-kernel@...r.kernel.org,
        linux-nvme@...ts.infradead.org, linux-block@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-mm@...ck.org,
        iommu@...ts.linux-foundation.org
Cc:     Minturn Dave B <dave.b.minturn@...el.com>,
        John Hubbard <jhubbard@...dia.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Ira Weiny <iweiny@...el.com>,
        Matthew Wilcox <willy@...radead.org>,
        Christian König <christian.koenig@....com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Jason Ekstrand <jason@...kstrand.net>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Dan Williams <dan.j.williams@...el.com>,
        Stephen Bates <sbates@...thlin.com>,
        Jakowski Andrzej <andrzej.jakowski@...el.com>,
        Christoph Hellwig <hch@....de>,
        Xiong Jianxin <jianxin.xiong@...el.com>
Subject: Re: [RFC PATCH v2 08/11] iommu/dma: Support PCI P2PDMA pages in
 dma-iommu map_sg



On 2021-03-12 8:52 a.m., Robin Murphy wrote:
> On 2021-03-11 23:31, Logan Gunthorpe wrote:
>> When a PCI P2PDMA page is seen, set the IOVA length of the segment
>> to zero so that it is not mapped into the IOVA. Then, in finalise_sg(),
>> apply the appropriate bus address to the segment. The IOVA is not
>> created if the scatterlist only consists of P2PDMA pages.
> 
> This misled me at first, but I see the implementation does actually
> appear to accomodate the case of working ACS where P2P *would* still
> need to be mapped at the IOMMU.

Yes, that's correct.
>>   static int __finalise_sg(struct device *dev, struct scatterlist *sg,
>> int nents,
>> -        dma_addr_t dma_addr)
>> +        dma_addr_t dma_addr, unsigned long attrs)
>>   {
>>       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...

>> +                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.


>> @@ -960,11 +975,12 @@ static int iommu_dma_map_sg(struct device *dev,
>> struct scatterlist *sg,
>>       struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>       struct iova_domain *iovad = &cookie->iovad;
>>       struct scatterlist *s, *prev = NULL;
>> +    struct dev_pagemap *pgmap = NULL;
>>       int prot = dma_info_to_prot(dir, dev_is_dma_coherent(dev), attrs);
>>       dma_addr_t iova;
>>       size_t iova_len = 0;
>>       unsigned long mask = dma_get_seg_boundary(dev);
>> -    int i;
>> +    int i, map = -1, ret = 0;
>>         if (static_branch_unlikely(&iommu_deferred_attach_enabled) &&
>>           iommu_deferred_attach(dev, domain))
>> @@ -993,6 +1009,23 @@ static int iommu_dma_map_sg(struct device *dev,
>> struct scatterlist *sg,
>>           s_length = iova_align(iovad, s_length + s_iova_off);
>>           s->length = s_length;
>>   +        if (is_pci_p2pdma_page(sg_page(s))) {
>> +            if (sg_page(s)->pgmap != pgmap) {
>> +                pgmap = sg_page(s)->pgmap;
>> +                map = pci_p2pdma_dma_map_type(dev, pgmap);
>> +            }
>> +
>> +            if (map < 0) {
> 
> It rather feels like it should be the job of whoever creates the list in
> the first place not to put unusable pages in it, especially since the
> p2pdma_map_type looks to be a fairly coarse-grained and static thing.
> The DMA API isn't responsible for validating normal memory pages, so
> what makes P2P special?

Yes, that would be ideal, but there's some difficulties there. For the
driver to check the pages, it would need to loop through the entire SG
one more time on every transaction, regardless of whether there are
P2PDMA pages, or not. So that will have a performance impact even when
the feature isn't being used. I don't think that'll be acceptable for
many drivers.

The other possibility is for GUP to do it when it gets the pages from
userspace. But GUP doesn't have all the information to do this at the
moment. We'd have to pass the struct device that will eventually map the
pages through all the nested functions in the GUP to do that test at
that time. This might not be a bad option (that I half looked into), but
I'm not sure how acceptable it would be to the GUP developers.

But even if we do verify the pages ahead of time, we still need the same
infrastructure in dma_map_sg(); it could only now be a BUG if the driver
sent invalid pages instead of an error return.

>> +                ret = -EREMOTEIO;
>> +                goto out_restore_sg;
>> +            }
>> +
>> +            if (map) {
>> +                s->length = 0;
> 
> 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.


>>   }
>>     static void iommu_dma_unmap_sg(struct device *dev, struct
>> scatterlist *sg,
>>           int nents, enum dma_data_direction dir, unsigned long attrs)
>>   {
>> -    dma_addr_t start, end;
>> +    dma_addr_t end, start = DMA_MAPPING_ERROR;
>>       struct scatterlist *tmp;
>>       int i;
>>   @@ -1054,14 +1090,20 @@ static void iommu_dma_unmap_sg(struct device
>> *dev, struct scatterlist *sg,
>>        * The scatterlist segments are mapped into a single
>>        * contiguous IOVA allocation, so this is incredibly easy.
>>        */
>> -    start = sg_dma_address(sg);
>> -    for_each_sg(sg_next(sg), tmp, nents - 1, i) {
>> +    for_each_sg(sg, tmp, nents, i) {
>> +        if (sg_is_pci_p2pdma(tmp))
> 
> Since the flag is associated with the DMA address which will no longer
> be valid, shouldn't it be cleared? The circumstances in which leaving it
> around could cause a problem are tenuous, but definitely possible.

Yes, that's a good idea.

Thanks for the review!

Logan

Powered by blists - more mailing lists