diff mbox

[dpdk-dev,v2] vfio/ppc64/spapr: Warn if DMA window was created at unexpected offset

Message ID 20170426080908.24026-1-aik@ozlabs.ru (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Alexey Kardashevskiy April 26, 2017, 8:09 a.m. UTC 
  VFIO_IOMMU_SPAPR_TCE_CREATE ioctl() returns the actual bus address for
just created DMA window. It happens to start from zero because the default
window is removed (leaving no windows) and new window starts from zero.
However this is not guaranteed and a new window may start from another
address, this adds a check and an error message.


Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* this just prints warning and fails instead of incorrectly changing
IOVA addresses
---
 lib/librte_eal/linuxapp/eal/eal_vfio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Burakov, Anatoly April 28, 2017, 11:19 a.m. UTC | #1 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Alexey
> Kardashevskiy
> Sent: Wednesday, April 26, 2017 9:09 AM
> To: dev@dpdk.org
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Subject: [dpdk-dev] [PATCH dpdk v2] vfio/ppc64/spapr: Warn if DMA
> window was created at unexpected offset
> 
> VFIO_IOMMU_SPAPR_TCE_CREATE ioctl() returns the actual bus address for
> just created DMA window. It happens to start from zero because the default
> window is removed (leaving no windows) and new window starts from zero.
> However this is not guaranteed and a new window may start from another
> address, this adds a check and an error message.
> 
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v2:
> * this just prints warning and fails instead of incorrectly changing IOVA
> addresses
> ---
>  lib/librte_eal/linuxapp/eal/eal_vfio.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> index 46f951f4d..530815790 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> @@ -702,6 +702,13 @@ vfio_spapr_dma_map(int vfio_container_fd)
>  		return -1;
>  	}
> 
> +	if (create.start_addr) {
> +		RTE_LOG(ERR, EAL,
> +			"  DMA offsets other than zero is not supported, "
> +			"new window is created at %lx\n",
> create.start_addr);
> +		return -1;
> +	}
> +
>  	/* map all DPDK segments for DMA. use 1:1 PA to IOVA mapping */
>  	for (i = 0; i < RTE_MAX_MEMSEG; i++) {
>  		struct vfio_iommu_type1_dma_map dma_map; @@ -734,7
> +741,6 @@ vfio_spapr_dma_map(int vfio_container_fd)
>  				"error %i (%s)\n", errno, strerror(errno));
>  			return -1;
>  		}
> -
>  	}
> 
>  	return 0;
> --
> 2.11.0

Hi Alexey,

There are some compile failures with this patch. You'll probably want to replace %lx with PRIx64 or similar :)

I would argue that just saying "DMA offsets other than zero are not supported" will be sufficient and will make error message less confusing. Up to you though.

Thanks,
Anatoly
Thomas Monjalon Oct. 17, 2017, 4:13 p.m. UTC | #2 
28/04/2017 13:19, Burakov, Anatoly:
> From: Alexey Kardashevskiy
> > VFIO_IOMMU_SPAPR_TCE_CREATE ioctl() returns the actual bus address for
> > just created DMA window. It happens to start from zero because the default
> > window is removed (leaving no windows) and new window starts from zero.
> > However this is not guaranteed and a new window may start from another
> > address, this adds a check and an error message.
> > 
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > Changes:
> > v2:
> > * this just prints warning and fails instead of incorrectly changing IOVA
> > addresses
> > ---
> 
> Hi Alexey,
> 
> There are some compile failures with this patch. You'll probably want to replace %lx with PRIx64 or similar :)
> 
> I would argue that just saying "DMA offsets other than zero are not supported" will be sufficient and will make error message less confusing. Up to you though.

Please, can we progress on this patch?
Is it still a relevant patch?
If yes, someone to make a v3, please?
Alexey Kardashevskiy Oct. 18, 2017, 5:11 a.m. UTC | #3 
On 18/10/17 03:13, Thomas Monjalon wrote:
> 28/04/2017 13:19, Burakov, Anatoly:
>> From: Alexey Kardashevskiy
>>> VFIO_IOMMU_SPAPR_TCE_CREATE ioctl() returns the actual bus address for
>>> just created DMA window. It happens to start from zero because the default
>>> window is removed (leaving no windows) and new window starts from zero.
>>> However this is not guaranteed and a new window may start from another
>>> address, this adds a check and an error message.
>>>
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v2:
>>> * this just prints warning and fails instead of incorrectly changing IOVA
>>> addresses
>>> ---
>>
>> Hi Alexey,
>>
>> There are some compile failures with this patch. You'll probably want to replace %lx with PRIx64 or similar :)
>>
>> I would argue that just saying "DMA offsets other than zero are not supported" will be sufficient and will make error message less confusing. Up to you though.
> 
> Please, can we progress on this patch?
> Is it still a relevant patch?

Nope, http://dpdk.org/browse/dpdk/commit/?id=ed1e7e576be219e1e has the
warning, this is enough really.


> If yes, someone to make a v3, please?
>
Thomas Monjalon Oct. 18, 2017, 6:24 a.m. UTC | #4 
18/10/2017 07:11, Alexey Kardashevskiy:
> On 18/10/17 03:13, Thomas Monjalon wrote:
> > 28/04/2017 13:19, Burakov, Anatoly:
> >> From: Alexey Kardashevskiy
> >>> VFIO_IOMMU_SPAPR_TCE_CREATE ioctl() returns the actual bus address for
> >>> just created DMA window. It happens to start from zero because the default
> >>> window is removed (leaving no windows) and new window starts from zero.
> >>> However this is not guaranteed and a new window may start from another
> >>> address, this adds a check and an error message.
> >>>
> >>>
> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>> ---
> >>> Changes:
> >>> v2:
> >>> * this just prints warning and fails instead of incorrectly changing IOVA
> >>> addresses
> >>> ---
> >>
> >> Hi Alexey,
> >>
> >> There are some compile failures with this patch. You'll probably want to replace %lx with PRIx64 or similar :)
> >>
> >> I would argue that just saying "DMA offsets other than zero are not supported" will be sufficient and will make error message less confusing. Up to you though.
> > 
> > Please, can we progress on this patch?
> > Is it still a relevant patch?
> 
> Nope, http://dpdk.org/browse/dpdk/commit/?id=ed1e7e576be219e1e has the
> warning, this is enough really.

OK, I've marked it as superseded in patchwork.
Thanks
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
index 46f951f4d..530815790 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
@@ -702,6 +702,13 @@  vfio_spapr_dma_map(int vfio_container_fd)
 		return -1;
 	}
 
+	if (create.start_addr) {
+		RTE_LOG(ERR, EAL,
+			"  DMA offsets other than zero is not supported, "
+			"new window is created at %lx\n", create.start_addr);
+		return -1;
+	}
+
 	/* map all DPDK segments for DMA. use 1:1 PA to IOVA mapping */
 	for (i = 0; i < RTE_MAX_MEMSEG; i++) {
 		struct vfio_iommu_type1_dma_map dma_map;
@@ -734,7 +741,6 @@  vfio_spapr_dma_map(int vfio_container_fd)
 				"error %i (%s)\n", errno, strerror(errno));
 			return -1;
 		}
-
 	}
 
 	return 0;