Re: [net-next PATCH 03/27] swiotlb: Add support for DMA_ATTR_SKIP_CPU_SYNC

[Alexander Duyck]

On Fri, Oct 28, 2016 at 10:34 AM, Konrad Rzeszutek Wilk
 wrote:
> On Tue, Oct 25, 2016 at 11:37:03AM -0400, Alexander Duyck wrote:
>> As a first step to making DMA_ATTR_SKIP_CPU_SYNC apply to architectures
>> beyond just ARM I need to make it so that the swiotlb will respect the
>> flag.  In order to do that I also need to update the swiotlb-xen since it
>> heavily makes use of the functionality.
>>
>> Cc: Konrad Rzeszutek Wilk 
>
> I am pretty sure I acked it the RFC. Was there a particular
> reason (this is very different from the RFC?) you dropped my ACk?
>
> Thanks.

If I recall you had acked patch 1, but for 2 you had some review
comments on and suggested I change a few things.  What was patch 2 in
the RFC was split out into patches 2 and 3.  That is why I didn't
include an Ack from you for those patches.

Patch 2 is a fix for Xen to address the fact that you could return
either 0 or ~0.  It was part of patch 2 originally and I pulled it out
into a separate patch.

Patch 3 does most of what patch 2 in the RFC was doing before with
fixes to address the fact that I was moving some code to avoid going
over 80 characters.  I found a different way to fix that by just
updating attrs before using it instead of ORing in the value when
passing it as a parameter.

>> @@ -558,11 +560,12 @@ void xen_swiotlb_unmap_page(struct device *hwdev, 
>> dma_addr_t dev_addr,
>>
>> start_dma_addr,
>>sg_phys(sg),
>>sg->length,
>> -  dir);
>> +  dir, attrs);
>>   if (map == SWIOTLB_MAP_ERROR) {
>>   dev_warn(hwdev, "swiotlb buffer is full\n");
>>   /* Don't panic here, we expect map_sg users
>>  to do proper error handling. */
>> + attrs |= DMA_ATTR_SKIP_CPU_SYNC;
>>   xen_swiotlb_unmap_sg_attrs(hwdev, sgl, i, dir,
>>  attrs);
>>   sg_dma_len(sgl) = 0;

The biggest difference from patch 2 in the RFC is right here.  This
code before was moving this off to the end of the function and adding
a label which I then jumped to.  I just ORed the
DMA_ATTR_SKIP_CPU_SYNC into attrs and skipped the problem entirely.
It should be harmless to do this way since attrs isn't used anywhere
else once we have had the error.

I hope that helps to clear it up.  So if you want I will add your
Acked-by for patches 2 and 3, but I just wanted to make sure this
worked with the changes you suggested.

Thanks.

- Alex

Re: [net-next PATCH 03/27] swiotlb: Add support for DMA_ATTR_SKIP_CPU_SYNC

[Alexander Duyck]

On Fri, Oct 28, 2016 at 10:34 AM, Konrad Rzeszutek Wilk
 wrote:
> On Tue, Oct 25, 2016 at 11:37:03AM -0400, Alexander Duyck wrote:
>> As a first step to making DMA_ATTR_SKIP_CPU_SYNC apply to architectures
>> beyond just ARM I need to make it so that the swiotlb will respect the
>> flag.  In order to do that I also need to update the swiotlb-xen since it
>> heavily makes use of the functionality.
>>
>> Cc: Konrad Rzeszutek Wilk 
>
> I am pretty sure I acked it the RFC. Was there a particular
> reason (this is very different from the RFC?) you dropped my ACk?
>
> Thanks.

If I recall you had acked patch 1, but for 2 you had some review
comments on and suggested I change a few things.  What was patch 2 in
the RFC was split out into patches 2 and 3.  That is why I didn't
include an Ack from you for those patches.

Patch 2 is a fix for Xen to address the fact that you could return
either 0 or ~0.  It was part of patch 2 originally and I pulled it out
into a separate patch.

Patch 3 does most of what patch 2 in the RFC was doing before with
fixes to address the fact that I was moving some code to avoid going
over 80 characters.  I found a different way to fix that by just
updating attrs before using it instead of ORing in the value when
passing it as a parameter.

>> @@ -558,11 +560,12 @@ void xen_swiotlb_unmap_page(struct device *hwdev, 
>> dma_addr_t dev_addr,
>>
>> start_dma_addr,
>>sg_phys(sg),
>>sg->length,
>> -  dir);
>> +  dir, attrs);
>>   if (map == SWIOTLB_MAP_ERROR) {
>>   dev_warn(hwdev, "swiotlb buffer is full\n");
>>   /* Don't panic here, we expect map_sg users
>>  to do proper error handling. */
>> + attrs |= DMA_ATTR_SKIP_CPU_SYNC;
>>   xen_swiotlb_unmap_sg_attrs(hwdev, sgl, i, dir,
>>  attrs);
>>   sg_dma_len(sgl) = 0;

The biggest difference from patch 2 in the RFC is right here.  This
code before was moving this off to the end of the function and adding
a label which I then jumped to.  I just ORed the
DMA_ATTR_SKIP_CPU_SYNC into attrs and skipped the problem entirely.
It should be harmless to do this way since attrs isn't used anywhere
else once we have had the error.

I hope that helps to clear it up.  So if you want I will add your
Acked-by for patches 2 and 3, but I just wanted to make sure this
worked with the changes you suggested.

Thanks.

- Alex

Re: [net-next PATCH 03/27] swiotlb: Add support for DMA_ATTR_SKIP_CPU_SYNC

[Konrad Rzeszutek Wilk]

On Tue, Oct 25, 2016 at 11:37:03AM -0400, Alexander Duyck wrote:
> As a first step to making DMA_ATTR_SKIP_CPU_SYNC apply to architectures
> beyond just ARM I need to make it so that the swiotlb will respect the
> flag.  In order to do that I also need to update the swiotlb-xen since it
> heavily makes use of the functionality.
> 
> Cc: Konrad Rzeszutek Wilk 

I am pretty sure I acked it the RFC. Was there a particular
reason (this is very different from the RFC?) you dropped my ACk?

Thanks.

> Signed-off-by: Alexander Duyck 
> ---
>  drivers/xen/swiotlb-xen.c |   11 +++---
>  include/linux/swiotlb.h   |6 --
>  lib/swiotlb.c |   48 
> +++--
>  3 files changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index b8014bf..3d048af 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -405,7 +405,8 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, 
> struct page *page,
>*/
>   trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
>  
> - map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir);
> + map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir,
> +  attrs);
>   if (map == SWIOTLB_MAP_ERROR)
>   return DMA_ERROR_CODE;
>  
> @@ -419,7 +420,8 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, 
> struct page *page,
>   if (dma_capable(dev, dev_addr, size))
>   return dev_addr;
>  
> - swiotlb_tbl_unmap_single(dev, map, size, dir);
> + swiotlb_tbl_unmap_single(dev, map, size, dir,
> +  attrs | DMA_ATTR_SKIP_CPU_SYNC);
>  
>   return DMA_ERROR_CODE;
>  }
> @@ -445,7 +447,7 @@ static void xen_unmap_single(struct device *hwdev, 
> dma_addr_t dev_addr,
>  
>   /* NOTE: We use dev_addr here, not paddr! */
>   if (is_xen_swiotlb_buffer(dev_addr)) {
> - swiotlb_tbl_unmap_single(hwdev, paddr, size, dir);
> + swiotlb_tbl_unmap_single(hwdev, paddr, size, dir, attrs);
>   return;
>   }
>  
> @@ -558,11 +560,12 @@ void xen_swiotlb_unmap_page(struct device *hwdev, 
> dma_addr_t dev_addr,
>start_dma_addr,
>sg_phys(sg),
>sg->length,
> -  dir);
> +  dir, attrs);
>   if (map == SWIOTLB_MAP_ERROR) {
>   dev_warn(hwdev, "swiotlb buffer is full\n");
>   /* Don't panic here, we expect map_sg users
>  to do proper error handling. */
> + attrs |= DMA_ATTR_SKIP_CPU_SYNC;
>   xen_swiotlb_unmap_sg_attrs(hwdev, sgl, i, dir,
>  attrs);
>   sg_dma_len(sgl) = 0;
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index e237b6f..4517be9 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -44,11 +44,13 @@ enum dma_sync_target {
>  extern phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> dma_addr_t tbl_dma_addr,
> phys_addr_t phys, size_t size,
> -   enum dma_data_direction dir);
> +   enum dma_data_direction dir,
> +   unsigned long attrs);
>  
>  extern void swiotlb_tbl_unmap_single(struct device *hwdev,
>phys_addr_t tlb_addr,
> -  size_t size, enum dma_data_direction dir);
> +  size_t size, enum dma_data_direction dir,
> +  unsigned long attrs);
>  
>  extern void swiotlb_tbl_sync_single(struct device *hwdev,
>   phys_addr_t tlb_addr,
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 47aad37..b538d39 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -425,7 +425,8 @@ static void swiotlb_bounce(phys_addr_t orig_addr, 
> phys_addr_t tlb_addr,
>  phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>  dma_addr_t tbl_dma_addr,
>  phys_addr_t orig_addr, size_t size,
> -enum dma_data_direction dir)
> +enum dma_data_direction dir,
> +unsigned long attrs)
>  {
>   unsigned long flags;
>   phys_addr_t tlb_addr;
> @@ 

Re: [net-next PATCH 03/27] swiotlb: Add support for DMA_ATTR_SKIP_CPU_SYNC

[Konrad Rzeszutek Wilk]

On Tue, Oct 25, 2016 at 11:37:03AM -0400, Alexander Duyck wrote:
> As a first step to making DMA_ATTR_SKIP_CPU_SYNC apply to architectures
> beyond just ARM I need to make it so that the swiotlb will respect the
> flag.  In order to do that I also need to update the swiotlb-xen since it
> heavily makes use of the functionality.
> 
> Cc: Konrad Rzeszutek Wilk 

I am pretty sure I acked it the RFC. Was there a particular
reason (this is very different from the RFC?) you dropped my ACk?

Thanks.

> Signed-off-by: Alexander Duyck 
> ---
>  drivers/xen/swiotlb-xen.c |   11 +++---
>  include/linux/swiotlb.h   |6 --
>  lib/swiotlb.c |   48 
> +++--
>  3 files changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index b8014bf..3d048af 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -405,7 +405,8 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, 
> struct page *page,
>*/
>   trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
>  
> - map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir);
> + map = swiotlb_tbl_map_single(dev, start_dma_addr, phys, size, dir,
> +  attrs);
>   if (map == SWIOTLB_MAP_ERROR)
>   return DMA_ERROR_CODE;
>  
> @@ -419,7 +420,8 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, 
> struct page *page,
>   if (dma_capable(dev, dev_addr, size))
>   return dev_addr;
>  
> - swiotlb_tbl_unmap_single(dev, map, size, dir);
> + swiotlb_tbl_unmap_single(dev, map, size, dir,
> +  attrs | DMA_ATTR_SKIP_CPU_SYNC);
>  
>   return DMA_ERROR_CODE;
>  }
> @@ -445,7 +447,7 @@ static void xen_unmap_single(struct device *hwdev, 
> dma_addr_t dev_addr,
>  
>   /* NOTE: We use dev_addr here, not paddr! */
>   if (is_xen_swiotlb_buffer(dev_addr)) {
> - swiotlb_tbl_unmap_single(hwdev, paddr, size, dir);
> + swiotlb_tbl_unmap_single(hwdev, paddr, size, dir, attrs);
>   return;
>   }
>  
> @@ -558,11 +560,12 @@ void xen_swiotlb_unmap_page(struct device *hwdev, 
> dma_addr_t dev_addr,
>start_dma_addr,
>sg_phys(sg),
>sg->length,
> -  dir);
> +  dir, attrs);
>   if (map == SWIOTLB_MAP_ERROR) {
>   dev_warn(hwdev, "swiotlb buffer is full\n");
>   /* Don't panic here, we expect map_sg users
>  to do proper error handling. */
> + attrs |= DMA_ATTR_SKIP_CPU_SYNC;
>   xen_swiotlb_unmap_sg_attrs(hwdev, sgl, i, dir,
>  attrs);
>   sg_dma_len(sgl) = 0;
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index e237b6f..4517be9 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -44,11 +44,13 @@ enum dma_sync_target {
>  extern phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
> dma_addr_t tbl_dma_addr,
> phys_addr_t phys, size_t size,
> -   enum dma_data_direction dir);
> +   enum dma_data_direction dir,
> +   unsigned long attrs);
>  
>  extern void swiotlb_tbl_unmap_single(struct device *hwdev,
>phys_addr_t tlb_addr,
> -  size_t size, enum dma_data_direction dir);
> +  size_t size, enum dma_data_direction dir,
> +  unsigned long attrs);
>  
>  extern void swiotlb_tbl_sync_single(struct device *hwdev,
>   phys_addr_t tlb_addr,
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 47aad37..b538d39 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -425,7 +425,8 @@ static void swiotlb_bounce(phys_addr_t orig_addr, 
> phys_addr_t tlb_addr,
>  phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>  dma_addr_t tbl_dma_addr,
>  phys_addr_t orig_addr, size_t size,
> -enum dma_data_direction dir)
> +enum dma_data_direction dir,
> +unsigned long attrs)
>  {
>   unsigned long flags;
>   phys_addr_t tlb_addr;
> @@ -526,7 +527,8 @@ phys_addr_t