Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
I looked over your swiotlb modification and I don't think we really need
them. The only thing we really need is to split the size parameter to
swiotlb_tbl_map_single and swiotlb_tbl_unmap_single into an alloc_size
and a mapping_size paramter, where the latter one is rounded up to the
iommu page size. Below is an untested patch on top of your series to
show what I mean. That being said - both the current series and the one
with my patch will still leak the content of the swiotlb buffer
allocated but not used to the untrusted external device. Is that
acceptable? If not we need to clear that part, at which point you don't
need swiotlb changes. Another implication is that for untrusted devices
the size of the dma coherent allocations needs to be rounded up to the
iommu page size (if that can ever be larger than the host page size).
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8c4a078fb041..eb5c32ad4443 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2151,10 +2151,13 @@ static int bounce_map(struct device *dev, struct
iommu_domain *domain,
void *data)
{
const struct iommu_ops *ops = domain->ops;
+ unsigned long page_size = domain_minimal_pgsize(domain);
phys_addr_t tlb_addr;
int prot = 0;
int ret;
+ if (WARN_ON_ONCE(size > page_size))
+ return -EINVAL;
if (unlikely(!ops->map || domain->pgsize_bitmap == 0UL))
return -ENODEV;
@@ -2164,16 +2167,16 @@ static int bounce_map(struct device *dev, struct
iommu_domain *domain,
if (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL)
prot |= IOMMU_WRITE;
- tlb_addr = phys_to_dma(dev, paddr);
- if (!swiotlb_map(dev, &paddr, &tlb_addr, size,
-dir, attrs | DMA_ATTR_BOUNCE_PAGE))
+ tlb_addr = swiotlb_tbl_map_single(dev, __phys_to_dma(dev, io_tlb_start),
+ paddr, size, page_size, dir, attrs);
+ if (tlb_addr == DMA_MAPPING_ERROR)
return -ENOMEM;
ret = ops->map(domain, addr, tlb_addr, size, prot);
- if (ret)
- swiotlb_tbl_unmap_single(dev, tlb_addr, size,
-dir, attrs | DMA_ATTR_BOUNCE_PAGE);
-
+ if (ret) {
+ swiotlb_tbl_unmap_single(dev, tlb_addr, size, page_size,
+dir, attrs);
+ }
return ret;
}
@@ -2194,11 +2197,12 @@ static int bounce_unmap(struct device *dev, struct
iommu_domain *domain,
if (unlikely(!ops->unmap))
return -ENODEV;
- ops->unmap(domain, ALIGN_DOWN(addr, page_size), page_size);
+ ops->unmap(domain, addr, page_size);
- if (tlb_addr)
- swiotlb_tbl_unmap_single(dev, tlb_addr, size,
-dir, attrs | DMA_ATTR_BOUNCE_PAGE);
+ if (tlb_addr) {
+ swiotlb_tbl_unmap_single(dev, tlb_addr, size, page_size,
+dir, attrs);
+ }
return 0;
}
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 877baf2a94f4..3b6ce643bffa 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -404,7 +404,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
*/
trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
- map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir,
+ map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, size, dir,
attrs);
if (map == DMA_MAPPING_ERROR)
return DMA_MAPPING_ERROR;
@@ -420,7 +420,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
return dev_addr;
attrs |= DMA_ATTR_SKIP_CPU_SYNC;
- swiotlb_tbl_unmap_single(dev, map, size, dir, attrs);
+ swiotlb_tbl_unmap_single(dev, map, size, size, dir, attrs);
return DMA_MAPPING_ERROR;
}
@@ -445,7 +445,7 @@ static void xen_unmap_single(struct device *hwdev,
dma_addr_t dev_addr,
/* NOTE: We use dev_addr here, not paddr! */
if (is_xen_swiotlb_buffer(dev_addr))
- swiotlb_tbl_unmap_single(hwdev, paddr, size, dir, attrs);
+ swiotlb_tbl_unmap_single(hwdev, paddr, size, size, dir, attrs);
}
static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
@@ -556,6 +556,7 @@ xen_swiotlb_map_sg_attrs(struct device *hwdev, struct
scatterlist *sgl,
start_dma_addr,
sg_phys(sg),
sg->length,
+sg->length,
dir, attrs);
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
Hi Christoph,
Thanks for reviewing my patches.
On 4/23/19 12:45 AM, Christoph Hellwig wrote:
I looked over your swiotlb modification and I don't think we really need
them. The only thing we really need is to split the size parameter to
swiotlb_tbl_map_single and swiotlb_tbl_unmap_single into an alloc_size
and a mapping_size paramter, where the latter one is rounded up to the
iommu page size. Below is an untested patch on top of your series to
show what I mean.
Good suggestion. The only problem as far as I can see is:
442 phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
443 dma_addr_t tbl_dma_addr, phys_addr_t orig_addr,
444 size_t mapping_size, size_t alloc_size,
445 enum dma_data_direction dir, unsigned long attrs)
446 {
447 unsigned long flags;
448 phys_addr_t tlb_addr;
449 unsigned int nslots, stride, index, wrap;
450 int i;
451 unsigned long mask;
452 unsigned long offset_slots;
453 unsigned long max_slots;
[--->o<]
545 found:
546 io_tlb_used += nslots;
547 spin_unlock_irqrestore(&io_tlb_lock, flags);
548
549 /*
550 * Save away the mapping from the original address to the
DMA address.
551 * This is needed when we sync the memory. Then we sync the
buffer if
552 * needed.
553 */
554 for (i = 0; i < nslots; i++)
555 io_tlb_orig_addr[index+i] = orig_addr + (i <<
IO_TLB_SHIFT);
Could the tlb orig address set to PAGE_ALIGN_DOWN(orig_addr)? We
couldn't assume the bounce buffer just starts from the beginning of the
slot. Or anything I missed?
556 if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
557 (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL))
558 swiotlb_bounce(orig_addr, tlb_addr, mapping_size,
559 DMA_TO_DEVICE);
Same here. We should sync from the place where the bounce buffer starts
from.
560
561 return tlb_addr;
562 }
That being said - both the current series and the one
with my patch will still leak the content of the swiotlb buffer
allocated but not used to the untrusted external device. Is that
acceptable? If not we need to clear that part, at which point you don't
need swiotlb changes.
Good catch. I think the allocated buffer should be cleared, otherwise,
the untrusted device still has a chance to access the data belonging to
other swiotlb consumers.
Another implication is that for untrusted devices
the size of the dma coherent allocations needs to be rounded up to the
iommu page size (if that can ever be larger than the host page size).
Agreed.
Intel iommu driver has already aligned the dma coherent allocation size
to PAGE_SIZE. And alloc_coherent is equal to alloc plus map. Hence, it
eventually goes into bounce_map().
Best regards,
Lu Baolu
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8c4a078fb041..eb5c32ad4443 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2151,10 +2151,13 @@ static int bounce_map(struct device *dev, struct
iommu_domain *domain,
void *data)
{
const struct iommu_ops *ops = domain->ops;
+ unsigned long page_size = domain_minimal_pgsize(domain);
phys_addr_t tlb_addr;
int prot = 0;
int ret;
+ if (WARN_ON_ONCE(size > page_size))
+ return -EINVAL;
if (unlikely(!ops->map || domain->pgsize_bitmap == 0UL))
return -ENODEV;
@@ -2164,16 +2167,16 @@ static int bounce_map(struct device *dev, struct iommu_domain *domain,
if (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL)
prot |= IOMMU_WRITE;
- tlb_addr = phys_to_dma(dev, paddr);
- if (!swiotlb_map(dev, &paddr, &tlb_addr, size,
-dir, attrs | DMA_ATTR_BOUNCE_PAGE))
+ tlb_addr = swiotlb_tbl_map_single(dev, __phys_to_dma(dev, io_tlb_start),
+ paddr, size, page_size, dir, attrs);
+ if (tlb_addr == DMA_MAPPING_ERROR)
return -ENOMEM;
ret = ops->map(domain, addr, tlb_addr, size, prot);
- if (ret)
- swiotlb_tbl_unmap_single(dev, tlb_addr, size,
-dir, attrs | DMA_ATTR_BOUNCE_PAGE);
-
+ if (ret) {
+ swiotlb_tbl_unmap_single(dev, tlb_addr, size, page_size,
+dir, attrs);
+ }
return ret;
}
@@ -2194,11 +2197,12 @@ static int bounce_unmap(struct device *dev, struct iommu_domain *domain,
if (unlikely(!ops->unmap))
return -ENODEV;
- ops->unmap(domain, ALIGN_DOWN(addr, page_size), page_size);
+ ops->unmap(domain, addr, page_size);
- if (tlb_addr)
- swiotlb_tbl_unmap_single(dev, tlb_addr, size,
-dir, attrs | DMA_ATTR_BOUNCE_PAGE);
+ if (tlb_addr) {
+
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
On Tue, Apr 23, 2019 at 09:58:19AM +0800, Lu Baolu wrote: > 554 for (i = 0; i < nslots; i++) > 555 io_tlb_orig_addr[index+i] = orig_addr + (i << > IO_TLB_SHIFT); > > Could the tlb orig address set to PAGE_ALIGN_DOWN(orig_addr)? We > couldn't assume the bounce buffer just starts from the beginning of the > slot. Or anything I missed? I don't see why we need to align the orig_addr. We only use io_tlb_orig_addr to find the address(es) for the swiotlb_bounce calls, and I don't see a good reason why we'd need to align those. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
Hi Christoph, On 4/23/19 2:12 PM, Christoph Hellwig wrote: On Tue, Apr 23, 2019 at 09:58:19AM +0800, Lu Baolu wrote: 554 for (i = 0; i < nslots; i++) 555 io_tlb_orig_addr[index+i] = orig_addr + (i << IO_TLB_SHIFT); Could the tlb orig address set to PAGE_ALIGN_DOWN(orig_addr)? We couldn't assume the bounce buffer just starts from the beginning of the slot. Or anything I missed? I don't see why we need to align the orig_addr. We only use io_tlb_orig_addr to find the address(es) for the swiotlb_bounce calls, and I don't see a good reason why we'd need to align those. Let me show you an example. Normally, if IOMMU is on, the device DMAs with an iova. IOMMU takes the responsibility to translate the iova to the physical address in paging mode. Physical IOVA Buffer .-. .-. | IOMMU | | IOMMU | | PAGE| | PAGE| .-. ---> .-. | Buffer | | Buffer | '-' '-' | | | | | | | | '-' '-' .---. | IOMMU | '---' When we add the bounce buffer between IOVA and physical buffer, the bounced buffer must starts from the same offset in a page, otherwise, IOMMU can't work here. Bouce Physical IOVA Buffer Buffer .-. .-. .-. | | .-> | Buffer | <---. | | | | | '-' | | | .-. | | | | .-. | Buffer |NO | | YES | Buffer | '-' | | '-' | | | | | | | | | | | | '-' '-' '-' .---. .-. | IOMMU | | swiotlb | '---' '-' A workable buffer location looks like below. Bouce Physical IOVA Buffer Buffer .-. .-. .-. | | | | | | | | | | | | .-. --->.-.<- .-. | Buffer |YES | Buffer | YES | Buffer | '-' '-' '-' | | | | | | | | | | | | '-' '-' '-' .---. .-. | IOMMU | | swiotlb | '---' '-' Best regards, Lu Baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
On Tue, Apr 23, 2019 at 03:32:16PM +0800, Lu Baolu wrote: > When we add the bounce buffer between IOVA and physical buffer, the > bounced buffer must starts from the same offset in a page, otherwise, > IOMMU can't work here. Why? Even with the odd hardware descriptors typical in Intel land that only allow offsets in the first page, not the following ones, having a zero offset where we previously had one should be fine. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
Hi, On 4/24/19 10:45 PM, Christoph Hellwig wrote: On Tue, Apr 23, 2019 at 03:32:16PM +0800, Lu Baolu wrote: When we add the bounce buffer between IOVA and physical buffer, the bounced buffer must starts from the same offset in a page, otherwise, IOMMU can't work here. Why? Even with the odd hardware descriptors typical in Intel land that only allow offsets in the first page, not the following ones, having a zero offset where we previously had one should be fine. This is not VT-d specific. It's just how generic IOMMU works. Normally, IOMMU works in paging mode. So if a driver issues DMA with IOVA 0x0123, IOMMU can remap it with a physical address 0x0123. But we should never expect IOMMU to remap 0x0123 with physical address of 0x. That's the reason why I said that IOMMU will not work there. swiotlb System IOVA bounce pageMemory .-. .-. .-. | | | | | | | | | | | | buffer_start .-. .-.buffer_start .-. | |->| | | | | | | |>| | | | | | swiotlb | | IOMMU Page '-' '-' mapping '-' Boundary | | | | | | | | | | | | | |->| | | |IOMMU mapping | | | | | | IOMMU Page .-. .-. Boundary | | | | | | | | | |->| | | | IOMMU mapping| | | | | | | | | | IOMMU Page .-. .-. .-. Boundary | | | | | | | | | |>| | | |->| | swiotlb | | buffer_end '-' '-' mapping '-' | | | | | | | | | | | | '-' '-' '-' This is the whole view of iommu bounce page. I expect the buffer_start returned by swiotlb_tbl_map_single() starts from the same page_offset as the buffer_start in IOVA. Best regards, Lu Baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
On Thu, Apr 25, 2019 at 10:07:19AM +0800, Lu Baolu wrote: > This is not VT-d specific. It's just how generic IOMMU works. > > Normally, IOMMU works in paging mode. So if a driver issues DMA with > IOVA 0x0123, IOMMU can remap it with a physical address 0x0123. > But we should never expect IOMMU to remap 0x0123 with physical > address of 0x. That's the reason why I said that IOMMU will not > work there. Well, with the iommu it doesn't happen. With swiotlb it obviosuly can happen, so drivers are fine with it. Why would that suddenly become an issue when swiotlb is called from the iommu code? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
Hi Christoph, On 4/26/19 11:04 PM, Christoph Hellwig wrote: On Thu, Apr 25, 2019 at 10:07:19AM +0800, Lu Baolu wrote: This is not VT-d specific. It's just how generic IOMMU works. Normally, IOMMU works in paging mode. So if a driver issues DMA with IOVA 0x0123, IOMMU can remap it with a physical address 0x0123. But we should never expect IOMMU to remap 0x0123 with physical address of 0x. That's the reason why I said that IOMMU will not work there. Well, with the iommu it doesn't happen. With swiotlb it obviosuly can happen, so drivers are fine with it. Why would that suddenly become an issue when swiotlb is called from the iommu code? I would say IOMMU is DMA remapping, not DMA engine. :-) Best regards, Lu Baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
On 29/04/2019 06:10, Lu Baolu wrote: Hi Christoph, On 4/26/19 11:04 PM, Christoph Hellwig wrote: On Thu, Apr 25, 2019 at 10:07:19AM +0800, Lu Baolu wrote: This is not VT-d specific. It's just how generic IOMMU works. Normally, IOMMU works in paging mode. So if a driver issues DMA with IOVA 0x0123, IOMMU can remap it with a physical address 0x0123. But we should never expect IOMMU to remap 0x0123 with physical address of 0x. That's the reason why I said that IOMMU will not work there. Well, with the iommu it doesn't happen. With swiotlb it obviosuly can happen, so drivers are fine with it. Why would that suddenly become an issue when swiotlb is called from the iommu code? I would say IOMMU is DMA remapping, not DMA engine. :-) I'm not sure I really follow the issue here - if we're copying the buffer to the bounce page(s) there's no conceptual difference from copying it to SWIOTLB slot(s), so there should be no need to worry about the original in-page offset. From the reply up-thread I guess you're trying to include an optimisation to only copy the head and tail of the buffer if it spans multiple pages, and directly map the ones in the middle, but AFAICS that's going to tie you to also using strict mode for TLB maintenance, which may not be a win overall depending on the balance between invalidation bandwidth vs. memcpy bandwidth. At least if we use standard SWIOTLB logic to always copy the whole thing, we should be able to release the bounce pages via the flush queue to allow 'safe' lazy unmaps. Either way I think it would be worth just implementing the straightforward version first, then coming back to consider optimisations later. Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
On Mon, Apr 29, 2019 at 12:06:52PM +0100, Robin Murphy wrote: > > From the reply up-thread I guess you're trying to include an optimisation > to only copy the head and tail of the buffer if it spans multiple pages, > and directly map the ones in the middle, but AFAICS that's going to tie you > to also using strict mode for TLB maintenance, which may not be a win > overall depending on the balance between invalidation bandwidth vs. memcpy > bandwidth. At least if we use standard SWIOTLB logic to always copy the > whole thing, we should be able to release the bounce pages via the flush > queue to allow 'safe' lazy unmaps. Oh. The head and tail optimization is what I missed. Yes, for that we'd need the offset. > Either way I think it would be worth just implementing the straightforward > version first, then coming back to consider optimisations later. Agreed, let's start simple. Especially as large DMA mappings or allocations should usually be properly aligned anyway, and if not we should fix that for multiple reasons. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
Hi Robin, On 4/29/19 7:06 PM, Robin Murphy wrote: On 29/04/2019 06:10, Lu Baolu wrote: Hi Christoph, On 4/26/19 11:04 PM, Christoph Hellwig wrote: On Thu, Apr 25, 2019 at 10:07:19AM +0800, Lu Baolu wrote: This is not VT-d specific. It's just how generic IOMMU works. Normally, IOMMU works in paging mode. So if a driver issues DMA with IOVA 0x0123, IOMMU can remap it with a physical address 0x0123. But we should never expect IOMMU to remap 0x0123 with physical address of 0x. That's the reason why I said that IOMMU will not work there. Well, with the iommu it doesn't happen. With swiotlb it obviosuly can happen, so drivers are fine with it. Why would that suddenly become an issue when swiotlb is called from the iommu code? I would say IOMMU is DMA remapping, not DMA engine. :-) I'm not sure I really follow the issue here - if we're copying the buffer to the bounce page(s) there's no conceptual difference from copying it to SWIOTLB slot(s), so there should be no need to worry about the original in-page offset. From the reply up-thread I guess you're trying to include an optimisation to only copy the head and tail of the buffer if it spans multiple pages, and directly map the ones in the middle, but AFAICS that's going to tie you to also using strict mode for TLB maintenance, which may not be a win overall depending on the balance between invalidation bandwidth vs. memcpy bandwidth. At least if we use standard SWIOTLB logic to always copy the whole thing, we should be able to release the bounce pages via the flush queue to allow 'safe' lazy unmaps. With respect, even we use the standard SWIOTLB logic, we need to use the strict mode for TLB maintenance. Say, some swiotbl slots are used by untrusted device for bounce page purpose. When the device driver unmaps the IOVA, the slots are freed but the mapping is still cached in IOTLB, hence the untrusted device is still able to access the slots. Then the slots are allocated to other devices. This makes it possible for the untrusted device to access the data buffer of other devices. Best regards, Lu Baolu Either way I think it would be worth just implementing the straightforward version first, then coming back to consider optimisations later. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
On 30/04/2019 03:02, Lu Baolu wrote: Hi Robin, On 4/29/19 7:06 PM, Robin Murphy wrote: On 29/04/2019 06:10, Lu Baolu wrote: Hi Christoph, On 4/26/19 11:04 PM, Christoph Hellwig wrote: On Thu, Apr 25, 2019 at 10:07:19AM +0800, Lu Baolu wrote: This is not VT-d specific. It's just how generic IOMMU works. Normally, IOMMU works in paging mode. So if a driver issues DMA with IOVA 0x0123, IOMMU can remap it with a physical address 0x0123. But we should never expect IOMMU to remap 0x0123 with physical address of 0x. That's the reason why I said that IOMMU will not work there. Well, with the iommu it doesn't happen. With swiotlb it obviosuly can happen, so drivers are fine with it. Why would that suddenly become an issue when swiotlb is called from the iommu code? I would say IOMMU is DMA remapping, not DMA engine. :-) I'm not sure I really follow the issue here - if we're copying the buffer to the bounce page(s) there's no conceptual difference from copying it to SWIOTLB slot(s), so there should be no need to worry about the original in-page offset. From the reply up-thread I guess you're trying to include an optimisation to only copy the head and tail of the buffer if it spans multiple pages, and directly map the ones in the middle, but AFAICS that's going to tie you to also using strict mode for TLB maintenance, which may not be a win overall depending on the balance between invalidation bandwidth vs. memcpy bandwidth. At least if we use standard SWIOTLB logic to always copy the whole thing, we should be able to release the bounce pages via the flush queue to allow 'safe' lazy unmaps. With respect, even we use the standard SWIOTLB logic, we need to use the strict mode for TLB maintenance. Say, some swiotbl slots are used by untrusted device for bounce page purpose. When the device driver unmaps the IOVA, the slots are freed but the mapping is still cached in IOTLB, hence the untrusted device is still able to access the slots. Then the slots are allocated to other devices. This makes it possible for the untrusted device to access the data buffer of other devices. Sure, that's indeed how it would work right now - however since the bounce pages will be freed and reused by the DMA API layer itself (at the same level as the IOVAs) I see no technical reason why we couldn't investigate deferred freeing as a future optimisation. Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
Hi Robin, On 4/30/19 5:53 PM, Robin Murphy wrote: On 30/04/2019 03:02, Lu Baolu wrote: Hi Robin, On 4/29/19 7:06 PM, Robin Murphy wrote: On 29/04/2019 06:10, Lu Baolu wrote: Hi Christoph, On 4/26/19 11:04 PM, Christoph Hellwig wrote: On Thu, Apr 25, 2019 at 10:07:19AM +0800, Lu Baolu wrote: This is not VT-d specific. It's just how generic IOMMU works. Normally, IOMMU works in paging mode. So if a driver issues DMA with IOVA 0x0123, IOMMU can remap it with a physical address 0x0123. But we should never expect IOMMU to remap 0x0123 with physical address of 0x. That's the reason why I said that IOMMU will not work there. Well, with the iommu it doesn't happen. With swiotlb it obviosuly can happen, so drivers are fine with it. Why would that suddenly become an issue when swiotlb is called from the iommu code? I would say IOMMU is DMA remapping, not DMA engine. :-) I'm not sure I really follow the issue here - if we're copying the buffer to the bounce page(s) there's no conceptual difference from copying it to SWIOTLB slot(s), so there should be no need to worry about the original in-page offset. From the reply up-thread I guess you're trying to include an optimisation to only copy the head and tail of the buffer if it spans multiple pages, and directly map the ones in the middle, but AFAICS that's going to tie you to also using strict mode for TLB maintenance, which may not be a win overall depending on the balance between invalidation bandwidth vs. memcpy bandwidth. At least if we use standard SWIOTLB logic to always copy the whole thing, we should be able to release the bounce pages via the flush queue to allow 'safe' lazy unmaps. With respect, even we use the standard SWIOTLB logic, we need to use the strict mode for TLB maintenance. Say, some swiotbl slots are used by untrusted device for bounce page purpose. When the device driver unmaps the IOVA, the slots are freed but the mapping is still cached in IOTLB, hence the untrusted device is still able to access the slots. Then the slots are allocated to other devices. This makes it possible for the untrusted device to access the data buffer of other devices. Sure, that's indeed how it would work right now - however since the bounce pages will be freed and reused by the DMA API layer itself (at the same level as the IOVAs) I see no technical reason why we couldn't investigate deferred freeing as a future optimisation. Yes, agreed. Best regards, Lu Baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
Hi Christoph, On 4/29/19 7:44 PM, Christoph Hellwig wrote: On Mon, Apr 29, 2019 at 12:06:52PM +0100, Robin Murphy wrote: From the reply up-thread I guess you're trying to include an optimisation to only copy the head and tail of the buffer if it spans multiple pages, and directly map the ones in the middle, but AFAICS that's going to tie you to also using strict mode for TLB maintenance, which may not be a win overall depending on the balance between invalidation bandwidth vs. memcpy bandwidth. At least if we use standard SWIOTLB logic to always copy the whole thing, we should be able to release the bounce pages via the flush queue to allow 'safe' lazy unmaps. Oh. The head and tail optimization is what I missed. Yes, for that we'd need the offset. Yes. Either way I think it would be worth just implementing the straightforward version first, then coming back to consider optimisations later. Agreed, let's start simple. Especially as large DMA mappings or allocations should usually be properly aligned anyway, and if not we should fix that for multiple reasons. Agreed. I will prepare the next version simply without the optimization, so the offset is not required. For your changes in swiotlb, will you formalize them in patches or want me to do this? Best regards, Lu Baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
On Mon, May 06, 2019 at 09:54:30AM +0800, Lu Baolu wrote: > Agreed. I will prepare the next version simply without the optimization, so > the offset is not required. > > For your changes in swiotlb, will you formalize them in patches or want > me to do this? Please do it yourself given that you still need the offset and thus a rework of the patches anyway.
Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free
Hi, On 5/13/19 3:05 PM, Christoph Hellwig wrote: On Mon, May 06, 2019 at 09:54:30AM +0800, Lu Baolu wrote: Agreed. I will prepare the next version simply without the optimization, so the offset is not required. For your changes in swiotlb, will you formalize them in patches or want me to do this? Please do it yourself given that you still need the offset and thus a rework of the patches anyway. Okay. Best regards, Lu Baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
