* [PATCH 0/3] xen/swiotlb: fix an issue and improve swiotlb-xen @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: konrad.wilk, boris.ostrovsky, sstabellini, Juergen Gross While hunting an issue in swiotlb-xen I stumbled over a wrong test and found some areas for improvement. Juergen Gross (3): xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() xen/swiotlb: simplify range_straddles_page_boundary() xen/swiotlb: remember having called xen_create_contiguous_region() drivers/xen/swiotlb-xen.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) -- 2.16.4 ^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH 0/3] xen/swiotlb: fix an issue and improve swiotlb-xen @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, konrad.wilk While hunting an issue in swiotlb-xen I stumbled over a wrong test and found some areas for improvement. Juergen Gross (3): xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() xen/swiotlb: simplify range_straddles_page_boundary() xen/swiotlb: remember having called xen_create_contiguous_region() drivers/xen/swiotlb-xen.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) -- 2.16.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH 0/3] xen/swiotlb: fix an issue and improve swiotlb-xen @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, konrad.wilk While hunting an issue in swiotlb-xen I stumbled over a wrong test and found some areas for improvement. Juergen Gross (3): xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() xen/swiotlb: simplify range_straddles_page_boundary() xen/swiotlb: remember having called xen_create_contiguous_region() drivers/xen/swiotlb-xen.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-) -- 2.16.4 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: konrad.wilk, boris.ostrovsky, sstabellini, Juergen Gross, stable The condition in xen_swiotlb_free_coherent() for deciding whether to call xen_destroy_contiguous_region() is wrong: in case the region to be freed is not contiguous calling xen_destroy_contiguous_region() is the wrong thing to do: it would result in inconsistent mappings of multiple PFNs to the same MFN. This will lead to various strange crashes or data corruption. Instead of calling xen_destroy_contiguous_region() in that case a warning should be issued as that situation should never occur. Cc: stable@vger.kernel.org Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 877baf2a94f4..42a3924e6d91 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if (((dev_addr + size - 1 <= dma_mask)) || - range_straddles_page_boundary(phys, size)) + if ((dev_addr + size - 1 <= dma_mask) && + !WARN_ON(range_straddles_page_boundary(phys, size))) xen_destroy_contiguous_region(phys, order); xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); -- 2.16.4 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, stable, konrad.wilk The condition in xen_swiotlb_free_coherent() for deciding whether to call xen_destroy_contiguous_region() is wrong: in case the region to be freed is not contiguous calling xen_destroy_contiguous_region() is the wrong thing to do: it would result in inconsistent mappings of multiple PFNs to the same MFN. This will lead to various strange crashes or data corruption. Instead of calling xen_destroy_contiguous_region() in that case a warning should be issued as that situation should never occur. Cc: stable@vger.kernel.org Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 877baf2a94f4..42a3924e6d91 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if (((dev_addr + size - 1 <= dma_mask)) || - range_straddles_page_boundary(phys, size)) + if ((dev_addr + size - 1 <= dma_mask) && + !WARN_ON(range_straddles_page_boundary(phys, size))) xen_destroy_contiguous_region(phys, order); xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); -- 2.16.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, stable, konrad.wilk The condition in xen_swiotlb_free_coherent() for deciding whether to call xen_destroy_contiguous_region() is wrong: in case the region to be freed is not contiguous calling xen_destroy_contiguous_region() is the wrong thing to do: it would result in inconsistent mappings of multiple PFNs to the same MFN. This will lead to various strange crashes or data corruption. Instead of calling xen_destroy_contiguous_region() in that case a warning should be issued as that situation should never occur. Cc: stable@vger.kernel.org Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 877baf2a94f4..42a3924e6d91 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if (((dev_addr + size - 1 <= dma_mask)) || - range_straddles_page_boundary(phys, size)) + if ((dev_addr + size - 1 <= dma_mask) && + !WARN_ON(range_straddles_page_boundary(phys, size))) xen_destroy_contiguous_region(phys, order); xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); -- 2.16.4 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-23 14:20 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:20 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu Cc: konrad.wilk, sstabellini, stable On 4/23/19 6:54 AM, Juergen Gross wrote: > The condition in xen_swiotlb_free_coherent() for deciding whether to > call xen_destroy_contiguous_region() is wrong: in case the region to > be freed is not contiguous calling xen_destroy_contiguous_region() is > the wrong thing to do: it would result in inconsistent mappings of > multiple PFNs to the same MFN. This will lead to various strange > crashes or data corruption. > > Instead of calling xen_destroy_contiguous_region() in that case a > warning should be issued as that situation should never occur. > > Cc: stable@vger.kernel.org > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-23 14:20 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:20 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu Cc: sstabellini, stable, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > The condition in xen_swiotlb_free_coherent() for deciding whether to > call xen_destroy_contiguous_region() is wrong: in case the region to > be freed is not contiguous calling xen_destroy_contiguous_region() is > the wrong thing to do: it would result in inconsistent mappings of > multiple PFNs to the same MFN. This will lead to various strange > crashes or data corruption. > > Instead of calling xen_destroy_contiguous_region() in that case a > warning should be issued as that situation should never occur. > > Cc: stable@vger.kernel.org > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-23 14:20 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:20 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu Cc: sstabellini, stable, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > The condition in xen_swiotlb_free_coherent() for deciding whether to > call xen_destroy_contiguous_region() is wrong: in case the region to > be freed is not contiguous calling xen_destroy_contiguous_region() is > the wrong thing to do: it would result in inconsistent mappings of > multiple PFNs to the same MFN. This will lead to various strange > crashes or data corruption. > > Instead of calling xen_destroy_contiguous_region() in that case a > warning should be issued as that situation should never occur. > > Cc: stable@vger.kernel.org > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-23 14:20 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:20 UTC (permalink / raw) To: Juergen Gross, xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: sstabellini-DgEjT+Ai2ygdnm+yROfE0A, stable-u79uwXL29TY76Z2rM5mHXA, konrad.wilk-QHcLZuEGTsvQT0dZR+AlfA On 4/23/19 6:54 AM, Juergen Gross wrote: > The condition in xen_swiotlb_free_coherent() for deciding whether to > call xen_destroy_contiguous_region() is wrong: in case the region to > be freed is not contiguous calling xen_destroy_contiguous_region() is > the wrong thing to do: it would result in inconsistent mappings of > multiple PFNs to the same MFN. This will lead to various strange > crashes or data corruption. > > Instead of calling xen_destroy_contiguous_region() in that case a > warning should be issued as that situation should never occur. > > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Signed-off-by: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org> Reviewed-by: Boris Ostrovsky <boris.ostrovsky-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() 2019-04-23 10:54 ` Juergen Gross ` (2 preceding siblings ...) (?) @ 2019-04-23 14:20 ` Boris Ostrovsky -1 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:20 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu Cc: sstabellini, stable, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > The condition in xen_swiotlb_free_coherent() for deciding whether to > call xen_destroy_contiguous_region() is wrong: in case the region to > be freed is not contiguous calling xen_destroy_contiguous_region() is > the wrong thing to do: it would result in inconsistent mappings of > multiple PFNs to the same MFN. This will lead to various strange > crashes or data corruption. > > Instead of calling xen_destroy_contiguous_region() in that case a > warning should be issued as that situation should never occur. > > Cc: stable@vger.kernel.org > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() 2019-04-23 10:54 ` Juergen Gross (?) (?) @ 2019-04-25 8:53 ` Jan Beulich -1 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 8:53 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, iommu, xen-devel, Boris Ostrovsky, Konrad Rzeszutek Wilk, linux-kernel, stable >>> On 23.04.19 at 12:54, <jgross@suse.com> wrote: > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (((dev_addr + size - 1 <= dma_mask)) || > - range_straddles_page_boundary(phys, size)) > + if ((dev_addr + size - 1 <= dma_mask) && > + !WARN_ON(range_straddles_page_boundary(phys, size))) > xen_destroy_contiguous_region(phys, order); On the allocation side we have if (((dev_addr + size - 1 <= dma_mask)) && !range_straddles_page_boundary(phys, size)) *dma_handle = dev_addr; else { if (xen_create_contiguous_region(phys, order, fls64(dma_mask), dma_handle) != 0) { xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } } which is (as far as the function call is concerned) if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) xen_create_contiguous_region(...); So I don't think your transformation is correct. Even worse, both parts of the condition in xen_swiotlb_free_coherent() act on an address that is the _result_ of the prior xen_create_contiguous_region(), i.e. the address should always match _both_ criteria anyway. Whereas what you really want is undo the xen_create_contiguous_region() only when it actually was called. Otherwise you also shatter contiguous allocations that were contiguous already for other reasons (perhaps just luck). Jan ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-25 8:53 ` Jan Beulich 0 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 8:53 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, linux-kernel, stable, iommu, xen-devel, Boris Ostrovsky >>> On 23.04.19 at 12:54, <jgross@suse.com> wrote: > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (((dev_addr + size - 1 <= dma_mask)) || > - range_straddles_page_boundary(phys, size)) > + if ((dev_addr + size - 1 <= dma_mask) && > + !WARN_ON(range_straddles_page_boundary(phys, size))) > xen_destroy_contiguous_region(phys, order); On the allocation side we have if (((dev_addr + size - 1 <= dma_mask)) && !range_straddles_page_boundary(phys, size)) *dma_handle = dev_addr; else { if (xen_create_contiguous_region(phys, order, fls64(dma_mask), dma_handle) != 0) { xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } } which is (as far as the function call is concerned) if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) xen_create_contiguous_region(...); So I don't think your transformation is correct. Even worse, both parts of the condition in xen_swiotlb_free_coherent() act on an address that is the _result_ of the prior xen_create_contiguous_region(), i.e. the address should always match _both_ criteria anyway. Whereas what you really want is undo the xen_create_contiguous_region() only when it actually was called. Otherwise you also shatter contiguous allocations that were contiguous already for other reasons (perhaps just luck). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-25 8:53 ` Jan Beulich 0 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 8:53 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, linux-kernel, stable, iommu, xen-devel, Boris Ostrovsky >>> On 23.04.19 at 12:54, <jgross@suse.com> wrote: > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (((dev_addr + size - 1 <= dma_mask)) || > - range_straddles_page_boundary(phys, size)) > + if ((dev_addr + size - 1 <= dma_mask) && > + !WARN_ON(range_straddles_page_boundary(phys, size))) > xen_destroy_contiguous_region(phys, order); On the allocation side we have if (((dev_addr + size - 1 <= dma_mask)) && !range_straddles_page_boundary(phys, size)) *dma_handle = dev_addr; else { if (xen_create_contiguous_region(phys, order, fls64(dma_mask), dma_handle) != 0) { xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } } which is (as far as the function call is concerned) if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) xen_create_contiguous_region(...); So I don't think your transformation is correct. Even worse, both parts of the condition in xen_swiotlb_free_coherent() act on an address that is the _result_ of the prior xen_create_contiguous_region(), i.e. the address should always match _both_ criteria anyway. Whereas what you really want is undo the xen_create_contiguous_region() only when it actually was called. Otherwise you also shatter contiguous allocations that were contiguous already for other reasons (perhaps just luck). Jan _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() @ 2019-04-25 8:53 ` Jan Beulich 0 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 8:53 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, iommu, xen-devel, Boris Ostrovsky, Konrad Rzeszutek Wilk, linux-kernel, stable >>> On 23.04.19 at 12:54, <jgross@suse.com> wrote: > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (((dev_addr + size - 1 <= dma_mask)) || > - range_straddles_page_boundary(phys, size)) > + if ((dev_addr + size - 1 <= dma_mask) && > + !WARN_ON(range_straddles_page_boundary(phys, size))) > xen_destroy_contiguous_region(phys, order); On the allocation side we have if (((dev_addr + size - 1 <= dma_mask)) && !range_straddles_page_boundary(phys, size)) *dma_handle = dev_addr; else { if (xen_create_contiguous_region(phys, order, fls64(dma_mask), dma_handle) != 0) { xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } } which is (as far as the function call is concerned) if ((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) xen_create_contiguous_region(...); So I don't think your transformation is correct. Even worse, both parts of the condition in xen_swiotlb_free_coherent() act on an address that is the _result_ of the prior xen_create_contiguous_region(), i.e. the address should always match _both_ criteria anyway. Whereas what you really want is undo the xen_create_contiguous_region() only when it actually was called. Otherwise you also shatter contiguous allocations that were contiguous already for other reasons (perhaps just luck). Jan ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() 2019-04-23 10:54 ` Juergen Gross ` (2 preceding siblings ...) (?) @ 2019-04-23 10:54 ` Juergen Gross -1 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, stable, konrad.wilk The condition in xen_swiotlb_free_coherent() for deciding whether to call xen_destroy_contiguous_region() is wrong: in case the region to be freed is not contiguous calling xen_destroy_contiguous_region() is the wrong thing to do: it would result in inconsistent mappings of multiple PFNs to the same MFN. This will lead to various strange crashes or data corruption. Instead of calling xen_destroy_contiguous_region() in that case a warning should be issued as that situation should never occur. Cc: stable@vger.kernel.org Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 877baf2a94f4..42a3924e6d91 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -360,8 +360,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if (((dev_addr + size - 1 <= dma_mask)) || - range_straddles_page_boundary(phys, size)) + if ((dev_addr + size - 1 <= dma_mask) && + !WARN_ON(range_straddles_page_boundary(phys, size))) xen_destroy_contiguous_region(phys, order); xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); -- 2.16.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() 2019-04-23 10:54 ` Juergen Gross ` (3 preceding siblings ...) (?) @ 2019-04-23 10:54 ` Juergen Gross -1 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, konrad.wilk range_straddles_page_boundary() is open coding several macros from include/xen/page.h. Use those instead. Additionally there is no need to have check_pages_physically_contiguous() as a separate function as it is used only once, so merge it into range_straddles_page_boundary(). Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 42a3924e6d91..43b6e65ae256 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -92,34 +92,18 @@ static inline dma_addr_t xen_virt_to_bus(void *address) return xen_phys_to_bus(virt_to_phys(address)); } -static int check_pages_physically_contiguous(unsigned long xen_pfn, - unsigned int offset, - size_t length) +static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) { - unsigned long next_bfn; - int i; - int nr_pages; + unsigned long next_bfn, xen_pfn = XEN_PFN_DOWN(p); + unsigned int i, nr_pages = XEN_PFN_UP(xen_offset_in_page(p) + size); next_bfn = pfn_to_bfn(xen_pfn); - nr_pages = (offset + length + XEN_PAGE_SIZE-1) >> XEN_PAGE_SHIFT; - for (i = 1; i < nr_pages; i++) { + for (i = 1; i < nr_pages; i++) if (pfn_to_bfn(++xen_pfn) != ++next_bfn) - return 0; - } - return 1; -} + return 1; -static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) -{ - unsigned long xen_pfn = XEN_PFN_DOWN(p); - unsigned int offset = p & ~XEN_PAGE_MASK; - - if (offset + size <= XEN_PAGE_SIZE) - return 0; - if (check_pages_physically_contiguous(xen_pfn, offset, size)) - return 0; - return 1; + return 0; } static int is_xen_swiotlb_buffer(dma_addr_t dma_addr) -- 2.16.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: konrad.wilk, boris.ostrovsky, sstabellini, Juergen Gross range_straddles_page_boundary() is open coding several macros from include/xen/page.h. Use those instead. Additionally there is no need to have check_pages_physically_contiguous() as a separate function as it is used only once, so merge it into range_straddles_page_boundary(). Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 42a3924e6d91..43b6e65ae256 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -92,34 +92,18 @@ static inline dma_addr_t xen_virt_to_bus(void *address) return xen_phys_to_bus(virt_to_phys(address)); } -static int check_pages_physically_contiguous(unsigned long xen_pfn, - unsigned int offset, - size_t length) +static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) { - unsigned long next_bfn; - int i; - int nr_pages; + unsigned long next_bfn, xen_pfn = XEN_PFN_DOWN(p); + unsigned int i, nr_pages = XEN_PFN_UP(xen_offset_in_page(p) + size); next_bfn = pfn_to_bfn(xen_pfn); - nr_pages = (offset + length + XEN_PAGE_SIZE-1) >> XEN_PAGE_SHIFT; - for (i = 1; i < nr_pages; i++) { + for (i = 1; i < nr_pages; i++) if (pfn_to_bfn(++xen_pfn) != ++next_bfn) - return 0; - } - return 1; -} + return 1; -static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) -{ - unsigned long xen_pfn = XEN_PFN_DOWN(p); - unsigned int offset = p & ~XEN_PAGE_MASK; - - if (offset + size <= XEN_PAGE_SIZE) - return 0; - if (check_pages_physically_contiguous(xen_pfn, offset, size)) - return 0; - return 1; + return 0; } static int is_xen_swiotlb_buffer(dma_addr_t dma_addr) -- 2.16.4 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, konrad.wilk range_straddles_page_boundary() is open coding several macros from include/xen/page.h. Use those instead. Additionally there is no need to have check_pages_physically_contiguous() as a separate function as it is used only once, so merge it into range_straddles_page_boundary(). Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 42a3924e6d91..43b6e65ae256 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -92,34 +92,18 @@ static inline dma_addr_t xen_virt_to_bus(void *address) return xen_phys_to_bus(virt_to_phys(address)); } -static int check_pages_physically_contiguous(unsigned long xen_pfn, - unsigned int offset, - size_t length) +static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) { - unsigned long next_bfn; - int i; - int nr_pages; + unsigned long next_bfn, xen_pfn = XEN_PFN_DOWN(p); + unsigned int i, nr_pages = XEN_PFN_UP(xen_offset_in_page(p) + size); next_bfn = pfn_to_bfn(xen_pfn); - nr_pages = (offset + length + XEN_PAGE_SIZE-1) >> XEN_PAGE_SHIFT; - for (i = 1; i < nr_pages; i++) { + for (i = 1; i < nr_pages; i++) if (pfn_to_bfn(++xen_pfn) != ++next_bfn) - return 0; - } - return 1; -} + return 1; -static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) -{ - unsigned long xen_pfn = XEN_PFN_DOWN(p); - unsigned int offset = p & ~XEN_PAGE_MASK; - - if (offset + size <= XEN_PAGE_SIZE) - return 0; - if (check_pages_physically_contiguous(xen_pfn, offset, size)) - return 0; - return 1; + return 0; } static int is_xen_swiotlb_buffer(dma_addr_t dma_addr) -- 2.16.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, konrad.wilk range_straddles_page_boundary() is open coding several macros from include/xen/page.h. Use those instead. Additionally there is no need to have check_pages_physically_contiguous() as a separate function as it is used only once, so merge it into range_straddles_page_boundary(). Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 42a3924e6d91..43b6e65ae256 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -92,34 +92,18 @@ static inline dma_addr_t xen_virt_to_bus(void *address) return xen_phys_to_bus(virt_to_phys(address)); } -static int check_pages_physically_contiguous(unsigned long xen_pfn, - unsigned int offset, - size_t length) +static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) { - unsigned long next_bfn; - int i; - int nr_pages; + unsigned long next_bfn, xen_pfn = XEN_PFN_DOWN(p); + unsigned int i, nr_pages = XEN_PFN_UP(xen_offset_in_page(p) + size); next_bfn = pfn_to_bfn(xen_pfn); - nr_pages = (offset + length + XEN_PAGE_SIZE-1) >> XEN_PAGE_SHIFT; - for (i = 1; i < nr_pages; i++) { + for (i = 1; i < nr_pages; i++) if (pfn_to_bfn(++xen_pfn) != ++next_bfn) - return 0; - } - return 1; -} + return 1; -static inline int range_straddles_page_boundary(phys_addr_t p, size_t size) -{ - unsigned long xen_pfn = XEN_PFN_DOWN(p); - unsigned int offset = p & ~XEN_PAGE_MASK; - - if (offset + size <= XEN_PAGE_SIZE) - return 0; - if (check_pages_physically_contiguous(xen_pfn, offset, size)) - return 0; - return 1; + return 0; } static int is_xen_swiotlb_buffer(dma_addr_t dma_addr) -- 2.16.4 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() @ 2019-04-23 14:25 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:25 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu; +Cc: konrad.wilk, sstabellini On 4/23/19 6:54 AM, Juergen Gross wrote: > range_straddles_page_boundary() is open coding several macros from > include/xen/page.h. Use those instead. Additionally there is no need > to have check_pages_physically_contiguous() as a separate function as > it is used only once, so merge it into range_straddles_page_boundary(). > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() @ 2019-04-23 14:25 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:25 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu; +Cc: sstabellini, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > range_straddles_page_boundary() is open coding several macros from > include/xen/page.h. Use those instead. Additionally there is no need > to have check_pages_physically_contiguous() as a separate function as > it is used only once, so merge it into range_straddles_page_boundary(). > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() @ 2019-04-23 14:25 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:25 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu; +Cc: sstabellini, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > range_straddles_page_boundary() is open coding several macros from > include/xen/page.h. Use those instead. Additionally there is no need > to have check_pages_physically_contiguous() as a separate function as > it is used only once, so merge it into range_straddles_page_boundary(). > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() @ 2019-04-23 14:25 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:25 UTC (permalink / raw) To: Juergen Gross, xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: sstabellini-DgEjT+Ai2ygdnm+yROfE0A, konrad.wilk-QHcLZuEGTsvQT0dZR+AlfA On 4/23/19 6:54 AM, Juergen Gross wrote: > range_straddles_page_boundary() is open coding several macros from > include/xen/page.h. Use those instead. Additionally there is no need > to have check_pages_physically_contiguous() as a separate function as > it is used only once, so merge it into range_straddles_page_boundary(). > > Signed-off-by: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org> Reviewed-by: Boris Ostrovsky <boris.ostrovsky-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() 2019-04-23 10:54 ` Juergen Gross ` (2 preceding siblings ...) (?) @ 2019-04-23 14:25 ` Boris Ostrovsky -1 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:25 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu; +Cc: sstabellini, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > range_straddles_page_boundary() is open coding several macros from > include/xen/page.h. Use those instead. Additionally there is no need > to have check_pages_physically_contiguous() as a separate function as > it is used only once, so merge it into range_straddles_page_boundary(). > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() 2019-04-23 10:54 ` Juergen Gross ` (5 preceding siblings ...) (?) @ 2019-04-23 10:54 ` Juergen Gross -1 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, konrad.wilk Instead of always calling xen_destroy_contiguous_region() in case the memory is DMA-able for the used device, do so only in case it has been made DMA-able via xen_create_contiguous_region() before. This will avoid a lot of xen_destroy_contiguous_region() calls for 64-bit capable devices. As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 flag of the first allocated page can be used for remembering. Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 43b6e65ae256..a72f181d8e20 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } + SetPageOwnerPriv1(virt_to_page(ret)); } memset(ret, 0, size); return ret; @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if ((dev_addr + size - 1 <= dma_mask) && - !WARN_ON(range_straddles_page_boundary(phys, size))) - xen_destroy_contiguous_region(phys, order); + if (PageOwnerPriv1(virt_to_page(vaddr))) { + if (!WARN_ON(range_straddles_page_boundary(phys, size))) + xen_destroy_contiguous_region(phys, order); + ClearPageOwnerPriv1(virt_to_page(vaddr)); + } xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); } -- 2.16.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: konrad.wilk, boris.ostrovsky, sstabellini, Juergen Gross Instead of always calling xen_destroy_contiguous_region() in case the memory is DMA-able for the used device, do so only in case it has been made DMA-able via xen_create_contiguous_region() before. This will avoid a lot of xen_destroy_contiguous_region() calls for 64-bit capable devices. As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 flag of the first allocated page can be used for remembering. Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 43b6e65ae256..a72f181d8e20 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } + SetPageOwnerPriv1(virt_to_page(ret)); } memset(ret, 0, size); return ret; @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if ((dev_addr + size - 1 <= dma_mask) && - !WARN_ON(range_straddles_page_boundary(phys, size))) - xen_destroy_contiguous_region(phys, order); + if (PageOwnerPriv1(virt_to_page(vaddr))) { + if (!WARN_ON(range_straddles_page_boundary(phys, size))) + xen_destroy_contiguous_region(phys, order); + ClearPageOwnerPriv1(virt_to_page(vaddr)); + } xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); } -- 2.16.4 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, konrad.wilk Instead of always calling xen_destroy_contiguous_region() in case the memory is DMA-able for the used device, do so only in case it has been made DMA-able via xen_create_contiguous_region() before. This will avoid a lot of xen_destroy_contiguous_region() calls for 64-bit capable devices. As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 flag of the first allocated page can be used for remembering. Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 43b6e65ae256..a72f181d8e20 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } + SetPageOwnerPriv1(virt_to_page(ret)); } memset(ret, 0, size); return ret; @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if ((dev_addr + size - 1 <= dma_mask) && - !WARN_ON(range_straddles_page_boundary(phys, size))) - xen_destroy_contiguous_region(phys, order); + if (PageOwnerPriv1(virt_to_page(vaddr))) { + if (!WARN_ON(range_straddles_page_boundary(phys, size))) + xen_destroy_contiguous_region(phys, order); + ClearPageOwnerPriv1(virt_to_page(vaddr)); + } xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); } -- 2.16.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel, linux-kernel, iommu Cc: Juergen Gross, boris.ostrovsky, sstabellini, konrad.wilk Instead of always calling xen_destroy_contiguous_region() in case the memory is DMA-able for the used device, do so only in case it has been made DMA-able via xen_create_contiguous_region() before. This will avoid a lot of xen_destroy_contiguous_region() calls for 64-bit capable devices. As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 flag of the first allocated page can be used for remembering. Signed-off-by: Juergen Gross <jgross@suse.com> --- drivers/xen/swiotlb-xen.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 43b6e65ae256..a72f181d8e20 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } + SetPageOwnerPriv1(virt_to_page(ret)); } memset(ret, 0, size); return ret; @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if ((dev_addr + size - 1 <= dma_mask) && - !WARN_ON(range_straddles_page_boundary(phys, size))) - xen_destroy_contiguous_region(phys, order); + if (PageOwnerPriv1(virt_to_page(vaddr))) { + if (!WARN_ON(range_straddles_page_boundary(phys, size))) + xen_destroy_contiguous_region(phys, order); + ClearPageOwnerPriv1(virt_to_page(vaddr)); + } xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); } -- 2.16.4 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 10:54 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 10:54 UTC (permalink / raw) To: xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: Juergen Gross, boris.ostrovsky-QHcLZuEGTsvQT0dZR+AlfA, sstabellini-DgEjT+Ai2ygdnm+yROfE0A, konrad.wilk-QHcLZuEGTsvQT0dZR+AlfA Instead of always calling xen_destroy_contiguous_region() in case the memory is DMA-able for the used device, do so only in case it has been made DMA-able via xen_create_contiguous_region() before. This will avoid a lot of xen_destroy_contiguous_region() calls for 64-bit capable devices. As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 flag of the first allocated page can be used for remembering. Signed-off-by: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org> --- drivers/xen/swiotlb-xen.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 43b6e65ae256..a72f181d8e20 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); return NULL; } + SetPageOwnerPriv1(virt_to_page(ret)); } memset(ret, 0, size); return ret; @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); - if ((dev_addr + size - 1 <= dma_mask) && - !WARN_ON(range_straddles_page_boundary(phys, size))) - xen_destroy_contiguous_region(phys, order); + if (PageOwnerPriv1(virt_to_page(vaddr))) { + if (!WARN_ON(range_straddles_page_boundary(phys, size))) + xen_destroy_contiguous_region(phys, order); + ClearPageOwnerPriv1(virt_to_page(vaddr)); + } xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); } -- 2.16.4 ^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 14:31 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:31 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu; +Cc: konrad.wilk, sstabellini On 4/23/19 6:54 AM, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. I think a new enum in pageflags would be useful, and be consistent with other flag uses. -boris > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 14:31 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:31 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu; +Cc: sstabellini, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. I think a new enum in pageflags would be useful, and be consistent with other flag uses. -boris > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 14:31 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:31 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu; +Cc: sstabellini, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. I think a new enum in pageflags would be useful, and be consistent with other flag uses. -boris > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 14:31 ` Boris Ostrovsky 0 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:31 UTC (permalink / raw) To: Juergen Gross, xen-devel-GuqFBffKawtpuQazS67q72D2FQJk+8+b, linux-kernel-u79uwXL29TY76Z2rM5mHXA, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: sstabellini-DgEjT+Ai2ygdnm+yROfE0A, konrad.wilk-QHcLZuEGTsvQT0dZR+AlfA On 4/23/19 6:54 AM, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. I think a new enum in pageflags would be useful, and be consistent with other flag uses. -boris > > Signed-off-by: Juergen Gross <jgross-IBi9RG/b67k@public.gmane.org> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() 2019-04-23 10:54 ` Juergen Gross ` (3 preceding siblings ...) (?) @ 2019-04-23 14:31 ` Boris Ostrovsky -1 siblings, 0 replies; 49+ messages in thread From: Boris Ostrovsky @ 2019-04-23 14:31 UTC (permalink / raw) To: Juergen Gross, xen-devel, linux-kernel, iommu; +Cc: sstabellini, konrad.wilk On 4/23/19 6:54 AM, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. I think a new enum in pageflags would be useful, and be consistent with other flag uses. -boris > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() 2019-04-23 10:54 ` Juergen Gross ` (4 preceding siblings ...) (?) @ 2019-04-23 17:05 ` Stefano Stabellini -1 siblings, 0 replies; 49+ messages in thread From: Stefano Stabellini @ 2019-04-23 17:05 UTC (permalink / raw) To: Juergen Gross Cc: sstabellini, konrad.wilk, linux-kernel, iommu, xen-devel, boris.ostrovsky On Tue, 23 Apr 2019, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. Although the patch looks OK, this sentence puzzles me. Why do you say that the memory in question is owned by swiotlb-xen? Because it was returned by xen_alloc_coherent_pages? Both the x86 and the Arm implementation return fresh new memory, hence, it should be safe to set the PageOwnerPriv1 flag? My concern with this approach is with the semantics of PG_owner_priv_1. Is a page marked with PG_owner_priv_1 only supposed to be used by the owner? > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 17:05 ` Stefano Stabellini 0 siblings, 0 replies; 49+ messages in thread From: Stefano Stabellini @ 2019-04-23 17:05 UTC (permalink / raw) To: Juergen Gross Cc: xen-devel, linux-kernel, iommu, konrad.wilk, boris.ostrovsky, sstabellini On Tue, 23 Apr 2019, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. Although the patch looks OK, this sentence puzzles me. Why do you say that the memory in question is owned by swiotlb-xen? Because it was returned by xen_alloc_coherent_pages? Both the x86 and the Arm implementation return fresh new memory, hence, it should be safe to set the PageOwnerPriv1 flag? My concern with this approach is with the semantics of PG_owner_priv_1. Is a page marked with PG_owner_priv_1 only supposed to be used by the owner? > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 17:05 ` Stefano Stabellini 0 siblings, 0 replies; 49+ messages in thread From: Stefano Stabellini @ 2019-04-23 17:05 UTC (permalink / raw) To: Juergen Gross Cc: sstabellini, konrad.wilk, linux-kernel, iommu, xen-devel, boris.ostrovsky On Tue, 23 Apr 2019, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. Although the patch looks OK, this sentence puzzles me. Why do you say that the memory in question is owned by swiotlb-xen? Because it was returned by xen_alloc_coherent_pages? Both the x86 and the Arm implementation return fresh new memory, hence, it should be safe to set the PageOwnerPriv1 flag? My concern with this approach is with the semantics of PG_owner_priv_1. Is a page marked with PG_owner_priv_1 only supposed to be used by the owner? > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 17:05 ` Stefano Stabellini 0 siblings, 0 replies; 49+ messages in thread From: Stefano Stabellini @ 2019-04-23 17:05 UTC (permalink / raw) To: Juergen Gross Cc: sstabellini, konrad.wilk, linux-kernel, iommu, xen-devel, boris.ostrovsky On Tue, 23 Apr 2019, Juergen Gross wrote: > Instead of always calling xen_destroy_contiguous_region() in case the > memory is DMA-able for the used device, do so only in case it has been > made DMA-able via xen_create_contiguous_region() before. > > This will avoid a lot of xen_destroy_contiguous_region() calls for > 64-bit capable devices. > > As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 > flag of the first allocated page can be used for remembering. Although the patch looks OK, this sentence puzzles me. Why do you say that the memory in question is owned by swiotlb-xen? Because it was returned by xen_alloc_coherent_pages? Both the x86 and the Arm implementation return fresh new memory, hence, it should be safe to set the PageOwnerPriv1 flag? My concern with this approach is with the semantics of PG_owner_priv_1. Is a page marked with PG_owner_priv_1 only supposed to be used by the owner? > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > drivers/xen/swiotlb-xen.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 43b6e65ae256..a72f181d8e20 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -321,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t size, > xen_free_coherent_pages(hwdev, size, ret, (dma_addr_t)phys, attrs); > return NULL; > } > + SetPageOwnerPriv1(virt_to_page(ret)); > } > memset(ret, 0, size); > return ret; > @@ -344,9 +345,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, > /* Convert the size to actually allocated. */ > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if ((dev_addr + size - 1 <= dma_mask) && > - !WARN_ON(range_straddles_page_boundary(phys, size))) > - xen_destroy_contiguous_region(phys, order); > + if (PageOwnerPriv1(virt_to_page(vaddr))) { > + if (!WARN_ON(range_straddles_page_boundary(phys, size))) > + xen_destroy_contiguous_region(phys, order); > + ClearPageOwnerPriv1(virt_to_page(vaddr)); > + } > > xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); > } _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 18:36 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 18:36 UTC (permalink / raw) To: Stefano Stabellini Cc: xen-devel, linux-kernel, iommu, konrad.wilk, boris.ostrovsky On 23/04/2019 19:05, Stefano Stabellini wrote: > On Tue, 23 Apr 2019, Juergen Gross wrote: >> Instead of always calling xen_destroy_contiguous_region() in case the >> memory is DMA-able for the used device, do so only in case it has been >> made DMA-able via xen_create_contiguous_region() before. >> >> This will avoid a lot of xen_destroy_contiguous_region() calls for >> 64-bit capable devices. >> >> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >> flag of the first allocated page can be used for remembering. > > Although the patch looks OK, this sentence puzzles me. Why do you say > that the memory in question is owned by swiotlb-xen? Because it was > returned by xen_alloc_coherent_pages? Both the x86 and the Arm > implementation return fresh new memory, hence, it should be safe to set > the PageOwnerPriv1 flag? > > My concern with this approach is with the semantics of PG_owner_priv_1. > Is a page marked with PG_owner_priv_1 only supposed to be used by the > owner? The owner of the page is free to use the flag. Like Grant pages are marked by the grant driver using this flag. And Xen page tables are using it in PV-guests for indicating a "Pinned" page table. Juergen ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 18:36 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 18:36 UTC (permalink / raw) To: Stefano Stabellini Cc: boris.ostrovsky, xen-devel, iommu, linux-kernel, konrad.wilk On 23/04/2019 19:05, Stefano Stabellini wrote: > On Tue, 23 Apr 2019, Juergen Gross wrote: >> Instead of always calling xen_destroy_contiguous_region() in case the >> memory is DMA-able for the used device, do so only in case it has been >> made DMA-able via xen_create_contiguous_region() before. >> >> This will avoid a lot of xen_destroy_contiguous_region() calls for >> 64-bit capable devices. >> >> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >> flag of the first allocated page can be used for remembering. > > Although the patch looks OK, this sentence puzzles me. Why do you say > that the memory in question is owned by swiotlb-xen? Because it was > returned by xen_alloc_coherent_pages? Both the x86 and the Arm > implementation return fresh new memory, hence, it should be safe to set > the PageOwnerPriv1 flag? > > My concern with this approach is with the semantics of PG_owner_priv_1. > Is a page marked with PG_owner_priv_1 only supposed to be used by the > owner? The owner of the page is free to use the flag. Like Grant pages are marked by the grant driver using this flag. And Xen page tables are using it in PV-guests for indicating a "Pinned" page table. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-23 18:36 ` Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 18:36 UTC (permalink / raw) To: Stefano Stabellini Cc: boris.ostrovsky, xen-devel, iommu, linux-kernel, konrad.wilk On 23/04/2019 19:05, Stefano Stabellini wrote: > On Tue, 23 Apr 2019, Juergen Gross wrote: >> Instead of always calling xen_destroy_contiguous_region() in case the >> memory is DMA-able for the used device, do so only in case it has been >> made DMA-able via xen_create_contiguous_region() before. >> >> This will avoid a lot of xen_destroy_contiguous_region() calls for >> 64-bit capable devices. >> >> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >> flag of the first allocated page can be used for remembering. > > Although the patch looks OK, this sentence puzzles me. Why do you say > that the memory in question is owned by swiotlb-xen? Because it was > returned by xen_alloc_coherent_pages? Both the x86 and the Arm > implementation return fresh new memory, hence, it should be safe to set > the PageOwnerPriv1 flag? > > My concern with this approach is with the semantics of PG_owner_priv_1. > Is a page marked with PG_owner_priv_1 only supposed to be used by the > owner? The owner of the page is free to use the flag. Like Grant pages are marked by the grant driver using this flag. And Xen page tables are using it in PV-guests for indicating a "Pinned" page table. Juergen _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() 2019-04-23 18:36 ` Juergen Gross (?) (?) @ 2019-04-25 9:01 ` Jan Beulich -1 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 9:01 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, linux-kernel, iommu, xen-devel, Boris Ostrovsky >>> On 23.04.19 at 20:36, <jgross@suse.com> wrote: > On 23/04/2019 19:05, Stefano Stabellini wrote: >> On Tue, 23 Apr 2019, Juergen Gross wrote: >>> Instead of always calling xen_destroy_contiguous_region() in case the >>> memory is DMA-able for the used device, do so only in case it has been >>> made DMA-able via xen_create_contiguous_region() before. >>> >>> This will avoid a lot of xen_destroy_contiguous_region() calls for >>> 64-bit capable devices. >>> >>> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >>> flag of the first allocated page can be used for remembering. >> >> Although the patch looks OK, this sentence puzzles me. Why do you say >> that the memory in question is owned by swiotlb-xen? Because it was >> returned by xen_alloc_coherent_pages? Both the x86 and the Arm >> implementation return fresh new memory, hence, it should be safe to set >> the PageOwnerPriv1 flag? >> >> My concern with this approach is with the semantics of PG_owner_priv_1. >> Is a page marked with PG_owner_priv_1 only supposed to be used by the >> owner? > > The owner of the page is free to use the flag. > > Like Grant pages are marked by the grant driver using this flag. And > Xen page tables are using it in PV-guests for indicating a "Pinned" > page table. Considering the background of the series, isn't such multi-purpose use of the flag a possible problem? You're already suspecting a wrong call into here. The function finding the flag set (but for another reason) might add to the confusion. But I realize there are only so many page flags available. Perhaps the freeing function should, first thing, check the handed space actually matches the criteria (within dma_mask and contiguous)? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() 2019-04-23 18:36 ` Juergen Gross (?) (?) @ 2019-04-25 9:01 ` Jan Beulich -1 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 9:01 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, iommu, xen-devel, Boris Ostrovsky, Konrad Rzeszutek Wilk, linux-kernel >>> On 23.04.19 at 20:36, <jgross@suse.com> wrote: > On 23/04/2019 19:05, Stefano Stabellini wrote: >> On Tue, 23 Apr 2019, Juergen Gross wrote: >>> Instead of always calling xen_destroy_contiguous_region() in case the >>> memory is DMA-able for the used device, do so only in case it has been >>> made DMA-able via xen_create_contiguous_region() before. >>> >>> This will avoid a lot of xen_destroy_contiguous_region() calls for >>> 64-bit capable devices. >>> >>> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >>> flag of the first allocated page can be used for remembering. >> >> Although the patch looks OK, this sentence puzzles me. Why do you say >> that the memory in question is owned by swiotlb-xen? Because it was >> returned by xen_alloc_coherent_pages? Both the x86 and the Arm >> implementation return fresh new memory, hence, it should be safe to set >> the PageOwnerPriv1 flag? >> >> My concern with this approach is with the semantics of PG_owner_priv_1. >> Is a page marked with PG_owner_priv_1 only supposed to be used by the >> owner? > > The owner of the page is free to use the flag. > > Like Grant pages are marked by the grant driver using this flag. And > Xen page tables are using it in PV-guests for indicating a "Pinned" > page table. Considering the background of the series, isn't such multi-purpose use of the flag a possible problem? You're already suspecting a wrong call into here. The function finding the flag set (but for another reason) might add to the confusion. But I realize there are only so many page flags available. Perhaps the freeing function should, first thing, check the handed space actually matches the criteria (within dma_mask and contiguous)? Jan ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-25 9:01 ` Jan Beulich 0 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 9:01 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, linux-kernel, iommu, xen-devel, Boris Ostrovsky >>> On 23.04.19 at 20:36, <jgross@suse.com> wrote: > On 23/04/2019 19:05, Stefano Stabellini wrote: >> On Tue, 23 Apr 2019, Juergen Gross wrote: >>> Instead of always calling xen_destroy_contiguous_region() in case the >>> memory is DMA-able for the used device, do so only in case it has been >>> made DMA-able via xen_create_contiguous_region() before. >>> >>> This will avoid a lot of xen_destroy_contiguous_region() calls for >>> 64-bit capable devices. >>> >>> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >>> flag of the first allocated page can be used for remembering. >> >> Although the patch looks OK, this sentence puzzles me. Why do you say >> that the memory in question is owned by swiotlb-xen? Because it was >> returned by xen_alloc_coherent_pages? Both the x86 and the Arm >> implementation return fresh new memory, hence, it should be safe to set >> the PageOwnerPriv1 flag? >> >> My concern with this approach is with the semantics of PG_owner_priv_1. >> Is a page marked with PG_owner_priv_1 only supposed to be used by the >> owner? > > The owner of the page is free to use the flag. > > Like Grant pages are marked by the grant driver using this flag. And > Xen page tables are using it in PV-guests for indicating a "Pinned" > page table. Considering the background of the series, isn't such multi-purpose use of the flag a possible problem? You're already suspecting a wrong call into here. The function finding the flag set (but for another reason) might add to the confusion. But I realize there are only so many page flags available. Perhaps the freeing function should, first thing, check the handed space actually matches the criteria (within dma_mask and contiguous)? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-25 9:01 ` Jan Beulich 0 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 9:01 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, linux-kernel, iommu, xen-devel, Boris Ostrovsky >>> On 23.04.19 at 20:36, <jgross@suse.com> wrote: > On 23/04/2019 19:05, Stefano Stabellini wrote: >> On Tue, 23 Apr 2019, Juergen Gross wrote: >>> Instead of always calling xen_destroy_contiguous_region() in case the >>> memory is DMA-able for the used device, do so only in case it has been >>> made DMA-able via xen_create_contiguous_region() before. >>> >>> This will avoid a lot of xen_destroy_contiguous_region() calls for >>> 64-bit capable devices. >>> >>> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >>> flag of the first allocated page can be used for remembering. >> >> Although the patch looks OK, this sentence puzzles me. Why do you say >> that the memory in question is owned by swiotlb-xen? Because it was >> returned by xen_alloc_coherent_pages? Both the x86 and the Arm >> implementation return fresh new memory, hence, it should be safe to set >> the PageOwnerPriv1 flag? >> >> My concern with this approach is with the semantics of PG_owner_priv_1. >> Is a page marked with PG_owner_priv_1 only supposed to be used by the >> owner? > > The owner of the page is free to use the flag. > > Like Grant pages are marked by the grant driver using this flag. And > Xen page tables are using it in PV-guests for indicating a "Pinned" > page table. Considering the background of the series, isn't such multi-purpose use of the flag a possible problem? You're already suspecting a wrong call into here. The function finding the flag set (but for another reason) might add to the confusion. But I realize there are only so many page flags available. Perhaps the freeing function should, first thing, check the handed space actually matches the criteria (within dma_mask and contiguous)? Jan _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-25 9:01 ` Jan Beulich 0 siblings, 0 replies; 49+ messages in thread From: Jan Beulich @ 2019-04-25 9:01 UTC (permalink / raw) To: Juergen Gross Cc: Stefano Stabellini, iommu, xen-devel, Boris Ostrovsky, Konrad Rzeszutek Wilk, linux-kernel >>> On 23.04.19 at 20:36, <jgross@suse.com> wrote: > On 23/04/2019 19:05, Stefano Stabellini wrote: >> On Tue, 23 Apr 2019, Juergen Gross wrote: >>> Instead of always calling xen_destroy_contiguous_region() in case the >>> memory is DMA-able for the used device, do so only in case it has been >>> made DMA-able via xen_create_contiguous_region() before. >>> >>> This will avoid a lot of xen_destroy_contiguous_region() calls for >>> 64-bit capable devices. >>> >>> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >>> flag of the first allocated page can be used for remembering. >> >> Although the patch looks OK, this sentence puzzles me. Why do you say >> that the memory in question is owned by swiotlb-xen? Because it was >> returned by xen_alloc_coherent_pages? Both the x86 and the Arm >> implementation return fresh new memory, hence, it should be safe to set >> the PageOwnerPriv1 flag? >> >> My concern with this approach is with the semantics of PG_owner_priv_1. >> Is a page marked with PG_owner_priv_1 only supposed to be used by the >> owner? > > The owner of the page is free to use the flag. > > Like Grant pages are marked by the grant driver using this flag. And > Xen page tables are using it in PV-guests for indicating a "Pinned" > page table. Considering the background of the series, isn't such multi-purpose use of the flag a possible problem? You're already suspecting a wrong call into here. The function finding the flag set (but for another reason) might add to the confusion. But I realize there are only so many page flags available. Perhaps the freeing function should, first thing, check the handed space actually matches the criteria (within dma_mask and contiguous)? Jan ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() 2019-04-23 17:05 ` Stefano Stabellini ` (2 preceding siblings ...) (?) @ 2019-04-23 18:36 ` Juergen Gross -1 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-23 18:36 UTC (permalink / raw) To: Stefano Stabellini Cc: boris.ostrovsky, xen-devel, iommu, linux-kernel, konrad.wilk On 23/04/2019 19:05, Stefano Stabellini wrote: > On Tue, 23 Apr 2019, Juergen Gross wrote: >> Instead of always calling xen_destroy_contiguous_region() in case the >> memory is DMA-able for the used device, do so only in case it has been >> made DMA-able via xen_create_contiguous_region() before. >> >> This will avoid a lot of xen_destroy_contiguous_region() calls for >> 64-bit capable devices. >> >> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >> flag of the first allocated page can be used for remembering. > > Although the patch looks OK, this sentence puzzles me. Why do you say > that the memory in question is owned by swiotlb-xen? Because it was > returned by xen_alloc_coherent_pages? Both the x86 and the Arm > implementation return fresh new memory, hence, it should be safe to set > the PageOwnerPriv1 flag? > > My concern with this approach is with the semantics of PG_owner_priv_1. > Is a page marked with PG_owner_priv_1 only supposed to be used by the > owner? The owner of the page is free to use the flag. Like Grant pages are marked by the grant driver using this flag. And Xen page tables are using it in PV-guests for indicating a "Pinned" page table. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() @ 2019-04-25 9:07 Juergen Gross 0 siblings, 0 replies; 49+ messages in thread From: Juergen Gross @ 2019-04-25 9:07 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, lkml, iommu, xen-devel, Boris Ostrovsky On 25/04/2019 11:01, Jan Beulich wrote: >>>> On 23.04.19 at 20:36, <jgross@suse.com> wrote: >> On 23/04/2019 19:05, Stefano Stabellini wrote: >>> On Tue, 23 Apr 2019, Juergen Gross wrote: >>>> Instead of always calling xen_destroy_contiguous_region() in case the >>>> memory is DMA-able for the used device, do so only in case it has been >>>> made DMA-able via xen_create_contiguous_region() before. >>>> >>>> This will avoid a lot of xen_destroy_contiguous_region() calls for >>>> 64-bit capable devices. >>>> >>>> As the memory in question is owned by swiotlb-xen the PG_owner_priv_1 >>>> flag of the first allocated page can be used for remembering. >>> >>> Although the patch looks OK, this sentence puzzles me. Why do you say >>> that the memory in question is owned by swiotlb-xen? Because it was >>> returned by xen_alloc_coherent_pages? Both the x86 and the Arm >>> implementation return fresh new memory, hence, it should be safe to set >>> the PageOwnerPriv1 flag? >>> >>> My concern with this approach is with the semantics of PG_owner_priv_1. >>> Is a page marked with PG_owner_priv_1 only supposed to be used by the >>> owner? >> >> The owner of the page is free to use the flag. >> >> Like Grant pages are marked by the grant driver using this flag. And >> Xen page tables are using it in PV-guests for indicating a "Pinned" >> page table. > > Considering the background of the series, isn't such multi-purpose use > of the flag a possible problem? You're already suspecting a wrong call > into here. The function finding the flag set (but for another reason) > might add to the confusion. But I realize there are only so many page > flags available. Right. > Perhaps the freeing function should, first thing, check the handed > space actually matches the criteria (within dma_mask and contiguous)? Yes, I think this is a good idea. Juergen _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 49+ messages in thread
end of thread, other threads:[~2019-04-25 9:14 UTC | newest] Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-23 10:54 [PATCH 0/3] xen/swiotlb: fix an issue and improve swiotlb-xen Juergen Gross 2019-04-23 10:54 ` [Xen-devel] " Juergen Gross 2019-04-23 10:54 ` Juergen Gross 2019-04-23 10:54 ` [PATCH 1/3] xen/swiotlb: fix condition for calling xen_destroy_contiguous_region() Juergen Gross 2019-04-23 10:54 ` [Xen-devel] " Juergen Gross 2019-04-23 10:54 ` Juergen Gross 2019-04-23 14:20 ` Boris Ostrovsky 2019-04-23 14:20 ` [Xen-devel] " Boris Ostrovsky 2019-04-23 14:20 ` Boris Ostrovsky 2019-04-23 14:20 ` Boris Ostrovsky 2019-04-23 14:20 ` Boris Ostrovsky 2019-04-25 8:53 ` [Xen-devel] " Jan Beulich 2019-04-25 8:53 ` Jan Beulich 2019-04-25 8:53 ` Jan Beulich 2019-04-25 8:53 ` Jan Beulich 2019-04-23 10:54 ` Juergen Gross 2019-04-23 10:54 ` [PATCH 2/3] xen/swiotlb: simplify range_straddles_page_boundary() Juergen Gross 2019-04-23 10:54 ` Juergen Gross 2019-04-23 10:54 ` [Xen-devel] " Juergen Gross 2019-04-23 10:54 ` Juergen Gross 2019-04-23 14:25 ` Boris Ostrovsky 2019-04-23 14:25 ` [Xen-devel] " Boris Ostrovsky 2019-04-23 14:25 ` Boris Ostrovsky 2019-04-23 14:25 ` Boris Ostrovsky 2019-04-23 14:25 ` Boris Ostrovsky 2019-04-23 10:54 ` [PATCH 3/3] xen/swiotlb: remember having called xen_create_contiguous_region() Juergen Gross 2019-04-23 10:54 ` Juergen Gross 2019-04-23 10:54 ` [Xen-devel] " Juergen Gross 2019-04-23 10:54 ` Juergen Gross 2019-04-23 10:54 ` Juergen Gross 2019-04-23 14:31 ` Boris Ostrovsky 2019-04-23 14:31 ` [Xen-devel] " Boris Ostrovsky 2019-04-23 14:31 ` Boris Ostrovsky 2019-04-23 14:31 ` Boris Ostrovsky 2019-04-23 14:31 ` Boris Ostrovsky 2019-04-23 17:05 ` Stefano Stabellini 2019-04-23 17:05 ` Stefano Stabellini 2019-04-23 17:05 ` [Xen-devel] " Stefano Stabellini 2019-04-23 17:05 ` Stefano Stabellini 2019-04-23 18:36 ` Juergen Gross 2019-04-23 18:36 ` [Xen-devel] " Juergen Gross 2019-04-23 18:36 ` Juergen Gross 2019-04-25 9:01 ` Jan Beulich 2019-04-25 9:01 ` [Xen-devel] " Jan Beulich 2019-04-25 9:01 ` Jan Beulich 2019-04-25 9:01 ` Jan Beulich 2019-04-25 9:01 ` Jan Beulich 2019-04-23 18:36 ` Juergen Gross 2019-04-25 9:07 Juergen Gross
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.