[dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[tone.zhang]

With a larger PAGE_SIZE it is possible for the MSI table to very
close to the end of the BAR s.t. when we align the MSI table to
the PAGE_SIZE, the end offset of the MSI table is out the PCI BAR
boundary.

This patch addresses the issue by comparing both the start and the
end offset of the MSI table with the BAR size.

The patch fixes the debug log as below:
EAL: Skipping BAR0

Signed-off-by: tone.zhang <tone.zhang@arm.com>
Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Steve Capper <Steve.Capper@arm.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index b1f0683..1373345 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 	struct pci_msix_table *msix_table = &vfio_res->msix_table;
 	struct pci_map *bar = &vfio_res->maps[bar_index];
 
-	if (bar->size == 0)
+	if (bar->size == 0) {
 		/* Skip this BAR */
+		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
 		return 0;
+	}
 
 	if (msix_table->bar_index == bar_index) {
 		/*
@@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 		uint32_t table_start = msix_table->offset;
 		uint32_t table_end = table_start + msix_table->size;
 		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
-		table_start &= PAGE_MASK;
+		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;
+		/* after rounding to PAGE_SIZE, it is over the bar->size,
+		 * fall back to the MSI-X table offset in the bar.
+		*/
+		if (table_start >= bar->size)
+			table_start = msix_table->offset;
 
 		if (table_start == 0 && table_end >= bar->size) {
 			/* Cannot map this BAR */
@@ -469,8 +476,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 
 		memreg[0].offset = bar->offset;
 		memreg[0].size = table_start;
-		memreg[1].offset = bar->offset + table_end;
-		memreg[1].size = bar->size - table_end;
+		if (bar->size < table_end) {
+			/*
+			 * after rounding to PAGE_SIZE we don't have any space
+			 * left after the MSI table, so don't try and map it.
+			*/
+			memreg[1].offset = 0;
+			memreg[1].size = 0;
+		}
+		else {
+			memreg[1].offset = bar->offset + table_end;
+			memreg[1].size = bar->size - table_end;
+		}
 
 		RTE_LOG(DEBUG, EAL,
 			"Trying to map BAR%d that contains the MSI-X "
-- 
2.7.4

Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 24-Oct-18 3:20 AM, tone.zhang wrote:
> With a larger PAGE_SIZE it is possible for the MSI table to very
> close to the end of the BAR s.t. when we align the MSI table to
> the PAGE_SIZE, the end offset of the MSI table is out the PCI BAR
> boundary.
> 
> This patch addresses the issue by comparing both the start and the
> end offset of the MSI table with the BAR size.
> 
> The patch fixes the debug log as below:
> EAL: Skipping BAR0
> 
> Signed-off-by: tone.zhang <tone.zhang@arm.com>
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index b1f0683..1373345 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>   	struct pci_map *bar = &vfio_res->maps[bar_index];
>   
> -	if (bar->size == 0)
> +	if (bar->size == 0) {
>   		/* Skip this BAR */
> +		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
>   		return 0;

I feel like "this" is unnecessary here - just "Skipping BAR%d" should be 
enough :)

> +	}
>   
>   	if (msix_table->bar_index == bar_index) {
>   		/*
> @@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   		uint32_t table_start = msix_table->offset;
>   		uint32_t table_end = table_start + msix_table->size;
>   		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> -		table_start &= PAGE_MASK;
> +		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;

IMO these two additions should be replaced by RTE_ALIGN by page size. 
Makes the purpose of the code much clearer.

> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
> +		 * fall back to the MSI-X table offset in the bar.
> +		*/
> +		if (table_start >= bar->size)
> +			table_start = msix_table->offset;

If i understand things correctly, msix_table->offset value here may be 
unaligned, so falling back to this value may cause mapping failure, 
because we later use this value as a size of mapping (which needs to be 
page aligned). Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?

>   
>   		if (table_start == 0 && table_end >= bar->size) {
>   			/* Cannot map this BAR */
> @@ -469,8 +476,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   
>   		memreg[0].offset = bar->offset;
>   		memreg[0].size = table_start;
> -		memreg[1].offset = bar->offset + table_end;
> -		memreg[1].size = bar->size - table_end;
> +		if (bar->size < table_end) {
> +			/*
> +			 * after rounding to PAGE_SIZE we don't have any space
> +			 * left after the MSI table, so don't try and map it.
> +			*/
> +			memreg[1].offset = 0;
> +			memreg[1].size = 0;
> +		}
> +		else {
> +			memreg[1].offset = bar->offset + table_end;
> +			memreg[1].size = bar->size - table_end;
> +		}
>   
>   		RTE_LOG(DEBUG, EAL,
>   			"Trying to map BAR%d that contains the MSI-X "
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi Burakov,

I'm sorry for the late response. 

Thanks a lot for your comments. Please find my response below (marked with "Tone:"). 😊

Br,
Tone

-----Original Message-----
From: Burakov, Anatoly <anatoly.burakov@intel.com> 
Sent: Wednesday, October 24, 2018 5:09 PM
To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; dev@dpdk.org
Cc: nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

On 24-Oct-18 3:20 AM, tone.zhang wrote:
> With a larger PAGE_SIZE it is possible for the MSI table to very close 
> to the end of the BAR s.t. when we align the MSI table to the 
> PAGE_SIZE, the end offset of the MSI table is out the PCI BAR 
> boundary.
> 
> This patch addresses the issue by comparing both the start and the end 
> offset of the MSI table with the BAR size.
> 
> The patch fixes the debug log as below:
> EAL: Skipping BAR0
> 
> Signed-off-by: tone.zhang <tone.zhang@arm.com>
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c 
> b/drivers/bus/pci/linux/pci_vfio.c
> index b1f0683..1373345 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>   	struct pci_map *bar = &vfio_res->maps[bar_index];
>   
> -	if (bar->size == 0)
> +	if (bar->size == 0) {
>   		/* Skip this BAR */
> +		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
>   		return 0;

I feel like "this" is unnecessary here - just "Skipping BAR%d" should be enough :)

Tone: Will update code and remove "this" in next version.

> +	}
>   
>   	if (msix_table->bar_index == bar_index) {
>   		/*
> @@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   		uint32_t table_start = msix_table->offset;
>   		uint32_t table_end = table_start + msix_table->size;
>   		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> -		table_start &= PAGE_MASK;
> +		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;

IMO these two additions should be replaced by RTE_ALIGN by page size. 
Makes the purpose of the code much clearer.

Tone: Sure, it is better! Will update code in next version. Thanks!

> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
> +		 * fall back to the MSI-X table offset in the bar.
> +		*/
> +		if (table_start >= bar->size)
> +			table_start = msix_table->offset;

If i understand things correctly, msix_table->offset value here may be unaligned, so falling back to this value may cause mapping failure, because we later use this value as a size of mapping (which needs to be page aligned). Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?

Tone: It is a little tricky. Align msix_table->offset with RTE_ALIGN_FLOOR maybe get 0 if the offset is less than page size in the PCI bar. It will trigger mmap() error. IIRC the input parameter "size" in mmap() is not required to be aligned with page size, system will do it. But it is better if we can do it. If I was wrong, please correct me. Thanks a lot.
>   
>   		if (table_start == 0 && table_end >= bar->size) {
>   			/* Cannot map this BAR */
> @@ -469,8 +476,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct 
> mapped_pci_resource *vfio_res,
>   
>   		memreg[0].offset = bar->offset;
>   		memreg[0].size = table_start;
> -		memreg[1].offset = bar->offset + table_end;
> -		memreg[1].size = bar->size - table_end;
> +		if (bar->size < table_end) {
> +			/*
> +			 * after rounding to PAGE_SIZE we don't have any space
> +			 * left after the MSI table, so don't try and map it.
> +			*/
> +			memreg[1].offset = 0;
> +			memreg[1].size = 0;
> +		}
> +		else {
> +			memreg[1].offset = bar->offset + table_end;
> +			memreg[1].size = bar->size - table_end;
> +		}
>   
>   		RTE_LOG(DEBUG, EAL,
>   			"Trying to map BAR%d that contains the MSI-X "
> 


--
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 01-Nov-18 2:33 AM, Tone Zhang (Arm Technology China) wrote:
> Hi Burakov,
> 
> I'm sorry for the late response.
> 
> Thanks a lot for your comments. Please find my response below (marked with "Tone:"). 😊
> 
> Br,
> Tone
> 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Wednesday, October 24, 2018 5:09 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver
> 
> On 24-Oct-18 3:20 AM, tone.zhang wrote:
>> With a larger PAGE_SIZE it is possible for the MSI table to very close
>> to the end of the BAR s.t. when we align the MSI table to the
>> PAGE_SIZE, the end offset of the MSI table is out the PCI BAR
>> boundary.
>>
>> This patch addresses the issue by comparing both the start and the end
>> offset of the MSI table with the BAR size.
>>
>> The patch fixes the debug log as below:
>> EAL: Skipping BAR0
>>
>> Signed-off-by: tone.zhang <tone.zhang@arm.com>
>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
>> ---
>>    drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
>>    1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bus/pci/linux/pci_vfio.c
>> b/drivers/bus/pci/linux/pci_vfio.c
>> index b1f0683..1373345 100644
>> --- a/drivers/bus/pci/linux/pci_vfio.c
>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>    	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>>    	struct pci_map *bar = &vfio_res->maps[bar_index];
>>    
>> -	if (bar->size == 0)
>> +	if (bar->size == 0) {
>>    		/* Skip this BAR */
>> +		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
>>    		return 0;
> 
> I feel like "this" is unnecessary here - just "Skipping BAR%d" should be enough :)
> 
> Tone: Will update code and remove "this" in next version.
> 
>> +	}
>>    
>>    	if (msix_table->bar_index == bar_index) {
>>    		/*
>> @@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>    		uint32_t table_start = msix_table->offset;
>>    		uint32_t table_end = table_start + msix_table->size;
>>    		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
>> -		table_start &= PAGE_MASK;
>> +		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;
> 
> IMO these two additions should be replaced by RTE_ALIGN by page size.
> Makes the purpose of the code much clearer.
> 
> Tone: Sure, it is better! Will update code in next version. Thanks!
> 
>> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
>> +		 * fall back to the MSI-X table offset in the bar.
>> +		*/
>> +		if (table_start >= bar->size)
>> +			table_start = msix_table->offset;
> 
> If i understand things correctly, msix_table->offset value here may be unaligned, so falling back to this value may cause mapping failure, because we later use this value as a size of mapping (which needs to be page aligned). Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?
> 
> Tone: It is a little tricky. Align msix_table->offset with RTE_ALIGN_FLOOR maybe get 0 if the offset is less than page size in the PCI bar. It will trigger mmap() error. IIRC the input parameter "size" in mmap() is not required to be aligned with page size, system will do it. But it is better if we can do it. If I was wrong, please correct me. Thanks a lot.

Apologies, you're correct - length can be misaligned (just tested it).

However, i think it's still worth aligning (and putting in an additional 
check), because we want to make sure we *don't* attempt to map the MSI-X 
BAR, and kernel might do that by adjusting length automatically and 
return mmap failure that way.

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi Burakov,

Thanks!
Please check my feedback below.

Br,
Tone

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
Sent: Thursday, November 1, 2018 6:01 PM
To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; dev@dpdk.org
Cc: nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

On 01-Nov-18 2:33 AM, Tone Zhang (Arm Technology China) wrote:
> Hi Burakov,
> 
> I'm sorry for the late response.
> 
> Thanks a lot for your comments. Please find my response below (marked 
> with "Tone:"). 😊
> 
> Br,
> Tone
> 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Wednesday, October 24, 2018 5:09 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; 
> dev@dpdk.org
> Cc: nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel 
> page_size with vfio-pci driver
> 
> On 24-Oct-18 3:20 AM, tone.zhang wrote:
>> With a larger PAGE_SIZE it is possible for the MSI table to very 
>> close to the end of the BAR s.t. when we align the MSI table to the 
>> PAGE_SIZE, the end offset of the MSI table is out the PCI BAR 
>> boundary.
>>
>> This patch addresses the issue by comparing both the start and the 
>> end offset of the MSI table with the BAR size.
>>
>> The patch fixes the debug log as below:
>> EAL: Skipping BAR0
>>
>> Signed-off-by: tone.zhang <tone.zhang@arm.com>
>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
>> ---
>>    drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
>>    1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bus/pci/linux/pci_vfio.c
>> b/drivers/bus/pci/linux/pci_vfio.c
>> index b1f0683..1373345 100644
>> --- a/drivers/bus/pci/linux/pci_vfio.c
>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>    	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>>    	struct pci_map *bar = &vfio_res->maps[bar_index];
>>    
>> -	if (bar->size == 0)
>> +	if (bar->size == 0) {
>>    		/* Skip this BAR */
>> +		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
>>    		return 0;
> 
> I feel like "this" is unnecessary here - just "Skipping BAR%d" should 
> be enough :)
> 
> Tone: Will update code and remove "this" in next version.
> 
>> +	}
>>    
>>    	if (msix_table->bar_index == bar_index) {
>>    		/*
>> @@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>    		uint32_t table_start = msix_table->offset;
>>    		uint32_t table_end = table_start + msix_table->size;
>>    		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
>> -		table_start &= PAGE_MASK;
>> +		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;
> 
> IMO these two additions should be replaced by RTE_ALIGN by page size.
> Makes the purpose of the code much clearer.
> 
> Tone: Sure, it is better! Will update code in next version. Thanks!
> 
>> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
>> +		 * fall back to the MSI-X table offset in the bar.
>> +		*/
>> +		if (table_start >= bar->size)
>> +			table_start = msix_table->offset;
> 
> If i understand things correctly, msix_table->offset value here may be unaligned, so falling back to this value may cause mapping failure, because we later use this value as a size of mapping (which needs to be page aligned). Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?
> 
> Tone: It is a little tricky. Align msix_table->offset with RTE_ALIGN_FLOOR maybe get 0 if the offset is less than page size in the PCI bar. It will trigger mmap() error. IIRC the input parameter "size" in mmap() is not required to be aligned with page size, system will do it. But it is better if we can do it. If I was wrong, please correct me. Thanks a lot.

Apologies, you're correct - length can be misaligned (just tested it).

However, i think it's still worth aligning (and putting in an additional check), because we want to make sure we *don't* attempt to map the MSI-X BAR, and kernel might do that by adjusting length automatically and return mmap failure that way.

Tone: Thanks a lot! I agree with you. It worth aligning the size. I will update code (RTE_ALIGN_FLOOR by page size) in next version. 
I'd like to discuss one case with you. In the case, base->size is 16384, msix_table->offset is 8192, page_size is 65536. After align "msix_table->offset" with page_size (RTE_ALIGN_FLOOR), the value of "table_start" is 0, mmap() will report error, and the memory mapping is failed. 
For the case (table_start is 0 after the aglinment), may I continue falling back the "table_start" to " msix_table->offset" (not aligned with page size), and left system adjust the length automatically? Thanks!

--
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 03-Nov-18 5:46 AM, Tone Zhang (Arm Technology China) wrote:
> Hi Burakov,
> 
> Thanks!
> Please check my feedback below.
> 
> Br,
> Tone
> 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
> Sent: Thursday, November 1, 2018 6:01 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver
> 
> On 01-Nov-18 2:33 AM, Tone Zhang (Arm Technology China) wrote:
>> Hi Burakov,
>>
>> I'm sorry for the late response.
>>
>> Thanks a lot for your comments. Please find my response below (marked
>> with "Tone:"). 😊
>>
>> Br,
>> Tone
>>
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Wednesday, October 24, 2018 5:09 PM
>> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
>> dev@dpdk.org
>> Cc: nd <nd@arm.com>
>> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel
>> page_size with vfio-pci driver
>>
>> On 24-Oct-18 3:20 AM, tone.zhang wrote:
>>> With a larger PAGE_SIZE it is possible for the MSI table to very
>>> close to the end of the BAR s.t. when we align the MSI table to the
>>> PAGE_SIZE, the end offset of the MSI table is out the PCI BAR
>>> boundary.
>>>
>>> This patch addresses the issue by comparing both the start and the
>>> end offset of the MSI table with the BAR size.
>>>
>>> The patch fixes the debug log as below:
>>> EAL: Skipping BAR0
>>>
>>> Signed-off-by: tone.zhang <tone.zhang@arm.com>
>>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
>>> ---
>>>     drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
>>>     1 file changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/bus/pci/linux/pci_vfio.c
>>> b/drivers/bus/pci/linux/pci_vfio.c
>>> index b1f0683..1373345 100644
>>> --- a/drivers/bus/pci/linux/pci_vfio.c
>>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>>> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>>     	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>>>     	struct pci_map *bar = &vfio_res->maps[bar_index];
>>>     
>>> -	if (bar->size == 0)
>>> +	if (bar->size == 0) {
>>>     		/* Skip this BAR */
>>> +		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
>>>     		return 0;
>>
>> I feel like "this" is unnecessary here - just "Skipping BAR%d" should
>> be enough :)
>>
>> Tone: Will update code and remove "this" in next version.
>>
>>> +	}
>>>     
>>>     	if (msix_table->bar_index == bar_index) {
>>>     		/*
>>> @@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>>     		uint32_t table_start = msix_table->offset;
>>>     		uint32_t table_end = table_start + msix_table->size;
>>>     		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
>>> -		table_start &= PAGE_MASK;
>>> +		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;
>>
>> IMO these two additions should be replaced by RTE_ALIGN by page size.
>> Makes the purpose of the code much clearer.
>>
>> Tone: Sure, it is better! Will update code in next version. Thanks!
>>
>>> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
>>> +		 * fall back to the MSI-X table offset in the bar.
>>> +		*/
>>> +		if (table_start >= bar->size)
>>> +			table_start = msix_table->offset;
>>
>> If i understand things correctly, msix_table->offset value here may be unaligned, so falling back to this value may cause mapping failure, because we later use this value as a size of mapping (which needs to be page aligned). Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?
>>
>> Tone: It is a little tricky. Align msix_table->offset with RTE_ALIGN_FLOOR maybe get 0 if the offset is less than page size in the PCI bar. It will trigger mmap() error. IIRC the input parameter "size" in mmap() is not required to be aligned with page size, system will do it. But it is better if we can do it. If I was wrong, please correct me. Thanks a lot.
> 
> Apologies, you're correct - length can be misaligned (just tested it).
> 
> However, i think it's still worth aligning (and putting in an additional check), because we want to make sure we *don't* attempt to map the MSI-X BAR, and kernel might do that by adjusting length automatically and return mmap failure that way.
> 
> Tone: Thanks a lot! I agree with you. It worth aligning the size. I will update code (RTE_ALIGN_FLOOR by page size) in next version.
> I'd like to discuss one case with you. In the case, base->size is 16384, msix_table->offset is 8192, page_size is 65536. After align "msix_table->offset" with page_size (RTE_ALIGN_FLOOR), the value of "table_start" is 0, mmap() will report error, and the memory mapping is failed.
> For the case (table_start is 0 after the aglinment), may I continue falling back the "table_start" to " msix_table->offset" (not aligned with page size), and left system adjust the length automatically? Thanks!

Please correct me if i'm wrong, but this is a code path for when we're 
trying to mmap around the MSI-X BAR. Kernel will not allow us to do 
that, period, so whatever start/end addresses you get, they *must not* 
include a single byte of MSI-X BAR. So, in case like you described, i 
think we should just straight up refuse the map the entire BAR.

However, as i do not have a system with such properties to test on, so 
please correct me if i'm wrong here :)

> 
> --
> Thanks,
> Anatoly
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi Burakov,

Please find my test case below. Thanks!

Br,
Tone

-----Original Message-----
From: Burakov, Anatoly <anatoly.burakov@intel.com> 
Sent: Tuesday, November 6, 2018 7:03 PM
To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; dev@dpdk.org
Cc: nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

On 03-Nov-18 5:46 AM, Tone Zhang (Arm Technology China) wrote:
> Hi Burakov,
> 
> Thanks!
> Please check my feedback below.
> 
> Br,
> Tone
> 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
> Sent: Thursday, November 1, 2018 6:01 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; 
> dev@dpdk.org
> Cc: nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel 
> page_size with vfio-pci driver
> 
> On 01-Nov-18 2:33 AM, Tone Zhang (Arm Technology China) wrote:
>> Hi Burakov,
>>
>> I'm sorry for the late response.
>>
>> Thanks a lot for your comments. Please find my response below (marked 
>> with "Tone:"). 😊
>>
>> Br,
>> Tone
>>
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Sent: Wednesday, October 24, 2018 5:09 PM
>> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; 
>> dev@dpdk.org
>> Cc: nd <nd@arm.com>
>> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel 
>> page_size with vfio-pci driver
>>
>> On 24-Oct-18 3:20 AM, tone.zhang wrote:
>>> With a larger PAGE_SIZE it is possible for the MSI table to very 
>>> close to the end of the BAR s.t. when we align the MSI table to the 
>>> PAGE_SIZE, the end offset of the MSI table is out the PCI BAR 
>>> boundary.
>>>
>>> This patch addresses the issue by comparing both the start and the 
>>> end offset of the MSI table with the BAR size.
>>>
>>> The patch fixes the debug log as below:
>>> EAL: Skipping BAR0
>>>
>>> Signed-off-by: tone.zhang <tone.zhang@arm.com>
>>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
>>> ---
>>>     drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
>>>     1 file changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/bus/pci/linux/pci_vfio.c
>>> b/drivers/bus/pci/linux/pci_vfio.c
>>> index b1f0683..1373345 100644
>>> --- a/drivers/bus/pci/linux/pci_vfio.c
>>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>>> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>>     	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>>>     	struct pci_map *bar = &vfio_res->maps[bar_index];
>>>     
>>> -	if (bar->size == 0)
>>> +	if (bar->size == 0) {
>>>     		/* Skip this BAR */
>>> +		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
>>>     		return 0;
>>
>> I feel like "this" is unnecessary here - just "Skipping BAR%d" should 
>> be enough :)
>>
>> Tone: Will update code and remove "this" in next version.
>>
>>> +	}
>>>     
>>>     	if (msix_table->bar_index == bar_index) {
>>>     		/*
>>> @@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>>     		uint32_t table_start = msix_table->offset;
>>>     		uint32_t table_end = table_start + msix_table->size;
>>>     		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
>>> -		table_start &= PAGE_MASK;
>>> +		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;
>>
>> IMO these two additions should be replaced by RTE_ALIGN by page size.
>> Makes the purpose of the code much clearer.
>>
>> Tone: Sure, it is better! Will update code in next version. Thanks!
>>
>>> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
>>> +		 * fall back to the MSI-X table offset in the bar.
>>> +		*/
>>> +		if (table_start >= bar->size)
>>> +			table_start = msix_table->offset;
>>
>> If i understand things correctly, msix_table->offset value here may be unaligned, so falling back to this value may cause mapping failure, because we later use this value as a size of mapping (which needs to be page aligned). Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?
>>
>> Tone: It is a little tricky. Align msix_table->offset with RTE_ALIGN_FLOOR maybe get 0 if the offset is less than page size in the PCI bar. It will trigger mmap() error. IIRC the input parameter "size" in mmap() is not required to be aligned with page size, system will do it. But it is better if we can do it. If I was wrong, please correct me. Thanks a lot.
> 
> Apologies, you're correct - length can be misaligned (just tested it).
> 
> However, i think it's still worth aligning (and putting in an additional check), because we want to make sure we *don't* attempt to map the MSI-X BAR, and kernel might do that by adjusting length automatically and return mmap failure that way.
> 
> Tone: Thanks a lot! I agree with you. It worth aligning the size. I will update code (RTE_ALIGN_FLOOR by page size) in next version.
> I'd like to discuss one case with you. In the case, base->size is 16384, msix_table->offset is 8192, page_size is 65536. After align "msix_table->offset" with page_size (RTE_ALIGN_FLOOR), the value of "table_start" is 0, mmap() will report error, and the memory mapping is failed.
> For the case (table_start is 0 after the aglinment), may I continue falling back the "table_start" to " msix_table->offset" (not aligned with page size), and left system adjust the length automatically? Thanks!

Please correct me if i'm wrong, but this is a code path for when we're trying to mmap around the MSI-X BAR. Kernel will not allow us to do that, period, so whatever start/end addresses you get, they *must not* include a single byte of MSI-X BAR. So, in case like you described, i think we should just straight up refuse the map the entire BAR.

However, as i do not have a system with such properties to test on, so please correct me if i'm wrong here :)


Tone: I understand and agree with you.  😊

Please have a look at my test case. In my case, I tried to bind NVMe device with VFIO driver and the kernel page size is 64KB. Without the change, the test is failed. 

From the debug information, I observed that "bar->size" is 16384, "msix_table->offset" is 8192 and "msix_table->size" is 512. Regarding the page size is much bigger than the "bar->size", in the change, the code maps the first 8192 bytes ahead of MSI-X table. After align with the page size boundary, the "start" offset after the MSI-X table is over "bar->size", mmap() reports error. In this case, I can only map the memory before the MSI-X table. After fall back "table_start" to " msix_table->offset " (i.e. 8192 bytes), and NOT mapping the memory behind MSI-X table, the NVMe device can be bound to VFIO driver, and the test is passed. The kernel version in my test environment is 4.16. 

So in the change, I do not map any byte of MSI-X table, unfortunately I cannot align the memory "size" in mmap() to page size boundary. From the test result, the change fixes the error. The case looks a little tricky. If we refuse the memory map here, it means we cannot bind VFIO driver with some PCI devices with 64KB kernel page size. I hope we can support such case in DPDK. 😊

> 
> --
> Thanks,
> Anatoly
> 


--
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 07-Nov-18 4:55 AM, Tone Zhang (Arm Technology China) wrote:
> Hi Burakov,
> 
> Please find my test case below. Thanks!
> 
> Br,
> Tone
> 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Tuesday, November 6, 2018 7:03 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver
> 
> On 03-Nov-18 5:46 AM, Tone Zhang (Arm Technology China) wrote:
>> Hi Burakov,
>>
>> Thanks!
>> Please check my feedback below.
>>
>> Br,
>> Tone
>>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
>> Sent: Thursday, November 1, 2018 6:01 PM
>> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
>> dev@dpdk.org
>> Cc: nd <nd@arm.com>
>> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel
>> page_size with vfio-pci driver
>>
>> On 01-Nov-18 2:33 AM, Tone Zhang (Arm Technology China) wrote:
>>> Hi Burakov,
>>>
>>> I'm sorry for the late response.
>>>
>>> Thanks a lot for your comments. Please find my response below (marked
>>> with "Tone:"). 😊
>>>
>>> Br,
>>> Tone
>>>
>>> -----Original Message-----
>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>>> Sent: Wednesday, October 24, 2018 5:09 PM
>>> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
>>> dev@dpdk.org
>>> Cc: nd <nd@arm.com>
>>> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel
>>> page_size with vfio-pci driver
>>>
>>> On 24-Oct-18 3:20 AM, tone.zhang wrote:
>>>> With a larger PAGE_SIZE it is possible for the MSI table to very
>>>> close to the end of the BAR s.t. when we align the MSI table to the
>>>> PAGE_SIZE, the end offset of the MSI table is out the PCI BAR
>>>> boundary.
>>>>
>>>> This patch addresses the issue by comparing both the start and the
>>>> end offset of the MSI table with the BAR size.
>>>>
>>>> The patch fixes the debug log as below:
>>>> EAL: Skipping BAR0
>>>>
>>>> Signed-off-by: tone.zhang <tone.zhang@arm.com>
>>>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
>>>> ---
>>>>      drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
>>>>      1 file changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/pci/linux/pci_vfio.c
>>>> b/drivers/bus/pci/linux/pci_vfio.c
>>>> index b1f0683..1373345 100644
>>>> --- a/drivers/bus/pci/linux/pci_vfio.c
>>>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>>>> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>>>      	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>>>>      	struct pci_map *bar = &vfio_res->maps[bar_index];
>>>>      
>>>> -	if (bar->size == 0)
>>>> +	if (bar->size == 0) {
>>>>      		/* Skip this BAR */
>>>> +		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
>>>>      		return 0;
>>>
>>> I feel like "this" is unnecessary here - just "Skipping BAR%d" should
>>> be enough :)
>>>
>>> Tone: Will update code and remove "this" in next version.
>>>
>>>> +	}
>>>>      
>>>>      	if (msix_table->bar_index == bar_index) {
>>>>      		/*
>>>> @@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>>>      		uint32_t table_start = msix_table->offset;
>>>>      		uint32_t table_end = table_start + msix_table->size;
>>>>      		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
>>>> -		table_start &= PAGE_MASK;
>>>> +		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;
>>>
>>> IMO these two additions should be replaced by RTE_ALIGN by page size.
>>> Makes the purpose of the code much clearer.
>>>
>>> Tone: Sure, it is better! Will update code in next version. Thanks!
>>>
>>>> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
>>>> +		 * fall back to the MSI-X table offset in the bar.
>>>> +		*/
>>>> +		if (table_start >= bar->size)
>>>> +			table_start = msix_table->offset;
>>>
>>> If i understand things correctly, msix_table->offset value here may be unaligned, so falling back to this value may cause mapping failure, because we later use this value as a size of mapping (which needs to be page aligned). Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?
>>>
>>> Tone: It is a little tricky. Align msix_table->offset with RTE_ALIGN_FLOOR maybe get 0 if the offset is less than page size in the PCI bar. It will trigger mmap() error. IIRC the input parameter "size" in mmap() is not required to be aligned with page size, system will do it. But it is better if we can do it. If I was wrong, please correct me. Thanks a lot.
>>
>> Apologies, you're correct - length can be misaligned (just tested it).
>>
>> However, i think it's still worth aligning (and putting in an additional check), because we want to make sure we *don't* attempt to map the MSI-X BAR, and kernel might do that by adjusting length automatically and return mmap failure that way.
>>
>> Tone: Thanks a lot! I agree with you. It worth aligning the size. I will update code (RTE_ALIGN_FLOOR by page size) in next version.
>> I'd like to discuss one case with you. In the case, base->size is 16384, msix_table->offset is 8192, page_size is 65536. After align "msix_table->offset" with page_size (RTE_ALIGN_FLOOR), the value of "table_start" is 0, mmap() will report error, and the memory mapping is failed.
>> For the case (table_start is 0 after the aglinment), may I continue falling back the "table_start" to " msix_table->offset" (not aligned with page size), and left system adjust the length automatically? Thanks!
> 
> Please correct me if i'm wrong, but this is a code path for when we're trying to mmap around the MSI-X BAR. Kernel will not allow us to do that, period, so whatever start/end addresses you get, they *must not* include a single byte of MSI-X BAR. So, in case like you described, i think we should just straight up refuse the map the entire BAR.
> 
> However, as i do not have a system with such properties to test on, so please correct me if i'm wrong here :)
> 
> 
> Tone: I understand and agree with you.  😊
> 
> Please have a look at my test case. In my case, I tried to bind NVMe device with VFIO driver and the kernel page size is 64KB. Without the change, the test is failed.
> 
>  From the debug information, I observed that "bar->size" is 16384, "msix_table->offset" is 8192 and "msix_table->size" is 512. Regarding the page size is much bigger than the "bar->size", in the change, the code maps the first 8192 bytes ahead of MSI-X table. After align with the page size boundary, the "start" offset after the MSI-X table is over "bar->size", mmap() reports error. In this case, I can only map the memory before the MSI-X table. After fall back "table_start" to " msix_table->offset " (i.e. 8192 bytes), and NOT mapping the memory behind MSI-X table, the NVMe device can be bound to VFIO driver, and the test is passed. The kernel version in my test environment is 4.16.
> 
> So in the change, I do not map any byte of MSI-X table, unfortunately I cannot align the memory "size" in mmap() to page size boundary. From the test result, the change fixes the error. The case looks a little tricky. If we refuse the memory map here, it means we cannot bind VFIO driver with some PCI devices with 64KB kernel page size. I hope we can support such case in DPDK. 😊

Hi Tone,

If it works and doesn't impact any other cases, i'm happy to include the 
above change (i.e. fall back to unaligned offset if aligning it results 
in zero offset). I'm curious what would happen if there's something else 
after the MSI-X table as well, and what alignments would be required for 
that... But i would rather wait for someone to come to us with an actual 
test case for that as well :)

> 
>>
>> --
>> Thanks,
>> Anatoly
>>
> 
> 
> --
> Thanks,
> Anatoly
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi Burakov,

Thanks a lot for you review. I will update the change and push the next version ASAP.

Thanks!

Br,
Tone

-----Original Message-----
From: Burakov, Anatoly <anatoly.burakov@intel.com> 
Sent: Wednesday, November 7, 2018 6:13 PM
To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; dev@dpdk.org
Cc: nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

On 07-Nov-18 4:55 AM, Tone Zhang (Arm Technology China) wrote:
> Hi Burakov,
> 
> Please find my test case below. Thanks!
> 
> Br,
> Tone
> 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Tuesday, November 6, 2018 7:03 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; 
> dev@dpdk.org
> Cc: nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel 
> page_size with vfio-pci driver
> 
> On 03-Nov-18 5:46 AM, Tone Zhang (Arm Technology China) wrote:
>> Hi Burakov,
>>
>> Thanks!
>> Please check my feedback below.
>>
>> Br,
>> Tone
>>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
>> Sent: Thursday, November 1, 2018 6:01 PM
>> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; 
>> dev@dpdk.org
>> Cc: nd <nd@arm.com>
>> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel 
>> page_size with vfio-pci driver
>>
>> On 01-Nov-18 2:33 AM, Tone Zhang (Arm Technology China) wrote:
>>> Hi Burakov,
>>>
>>> I'm sorry for the late response.
>>>
>>> Thanks a lot for your comments. Please find my response below 
>>> (marked with "Tone:"). 😊
>>>
>>> Br,
>>> Tone
>>>
>>> -----Original Message-----
>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>>> Sent: Wednesday, October 24, 2018 5:09 PM
>>> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>; 
>>> dev@dpdk.org
>>> Cc: nd <nd@arm.com>
>>> Subject: Re: [dpdk-dev] [PATCH] pci_vfio: Support 64KB kernel 
>>> page_size with vfio-pci driver
>>>
>>> On 24-Oct-18 3:20 AM, tone.zhang wrote:
>>>> With a larger PAGE_SIZE it is possible for the MSI table to very 
>>>> close to the end of the BAR s.t. when we align the MSI table to the 
>>>> PAGE_SIZE, the end offset of the MSI table is out the PCI BAR 
>>>> boundary.
>>>>
>>>> This patch addresses the issue by comparing both the start and the 
>>>> end offset of the MSI table with the BAR size.
>>>>
>>>> The patch fixes the debug log as below:
>>>> EAL: Skipping BAR0
>>>>
>>>> Signed-off-by: tone.zhang <tone.zhang@arm.com>
>>>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
>>>> ---
>>>>      drivers/bus/pci/linux/pci_vfio.c | 25 +++++++++++++++++++++----
>>>>      1 file changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/pci/linux/pci_vfio.c
>>>> b/drivers/bus/pci/linux/pci_vfio.c
>>>> index b1f0683..1373345 100644
>>>> --- a/drivers/bus/pci/linux/pci_vfio.c
>>>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>>>> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>>>      	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>>>>      	struct pci_map *bar = &vfio_res->maps[bar_index];
>>>>      
>>>> -	if (bar->size == 0)
>>>> +	if (bar->size == 0) {
>>>>      		/* Skip this BAR */
>>>> +		RTE_LOG(INFO, EAL, "Skipping this BAR%d\n", bar_index);
>>>>      		return 0;
>>>
>>> I feel like "this" is unnecessary here - just "Skipping BAR%d" 
>>> should be enough :)
>>>
>>> Tone: Will update code and remove "this" in next version.
>>>
>>>> +	}
>>>>      
>>>>      	if (msix_table->bar_index == bar_index) {
>>>>      		/*
>>>> @@ -457,7 +459,12 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>>>>      		uint32_t table_start = msix_table->offset;
>>>>      		uint32_t table_end = table_start + msix_table->size;
>>>>      		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
>>>> -		table_start &= PAGE_MASK;
>>>> +		table_start = (table_start + ~PAGE_MASK) & PAGE_MASK;
>>>
>>> IMO these two additions should be replaced by RTE_ALIGN by page size.
>>> Makes the purpose of the code much clearer.
>>>
>>> Tone: Sure, it is better! Will update code in next version. Thanks!
>>>
>>>> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
>>>> +		 * fall back to the MSI-X table offset in the bar.
>>>> +		*/
>>>> +		if (table_start >= bar->size)
>>>> +			table_start = msix_table->offset;
>>>
>>> If i understand things correctly, msix_table->offset value here may be unaligned, so falling back to this value may cause mapping failure, because we later use this value as a size of mapping (which needs to be page aligned). Shouldn't this be aligned using RTE_ALIGN_FLOOR by page size?
>>>
>>> Tone: It is a little tricky. Align msix_table->offset with RTE_ALIGN_FLOOR maybe get 0 if the offset is less than page size in the PCI bar. It will trigger mmap() error. IIRC the input parameter "size" in mmap() is not required to be aligned with page size, system will do it. But it is better if we can do it. If I was wrong, please correct me. Thanks a lot.
>>
>> Apologies, you're correct - length can be misaligned (just tested it).
>>
>> However, i think it's still worth aligning (and putting in an additional check), because we want to make sure we *don't* attempt to map the MSI-X BAR, and kernel might do that by adjusting length automatically and return mmap failure that way.
>>
>> Tone: Thanks a lot! I agree with you. It worth aligning the size. I will update code (RTE_ALIGN_FLOOR by page size) in next version.
>> I'd like to discuss one case with you. In the case, base->size is 16384, msix_table->offset is 8192, page_size is 65536. After align "msix_table->offset" with page_size (RTE_ALIGN_FLOOR), the value of "table_start" is 0, mmap() will report error, and the memory mapping is failed.
>> For the case (table_start is 0 after the aglinment), may I continue falling back the "table_start" to " msix_table->offset" (not aligned with page size), and left system adjust the length automatically? Thanks!
> 
> Please correct me if i'm wrong, but this is a code path for when we're trying to mmap around the MSI-X BAR. Kernel will not allow us to do that, period, so whatever start/end addresses you get, they *must not* include a single byte of MSI-X BAR. So, in case like you described, i think we should just straight up refuse the map the entire BAR.
> 
> However, as i do not have a system with such properties to test on, so 
> please correct me if i'm wrong here :)
> 
> 
> Tone: I understand and agree with you.  😊
> 
> Please have a look at my test case. In my case, I tried to bind NVMe device with VFIO driver and the kernel page size is 64KB. Without the change, the test is failed.
> 
>  From the debug information, I observed that "bar->size" is 16384, "msix_table->offset" is 8192 and "msix_table->size" is 512. Regarding the page size is much bigger than the "bar->size", in the change, the code maps the first 8192 bytes ahead of MSI-X table. After align with the page size boundary, the "start" offset after the MSI-X table is over "bar->size", mmap() reports error. In this case, I can only map the memory before the MSI-X table. After fall back "table_start" to " msix_table->offset " (i.e. 8192 bytes), and NOT mapping the memory behind MSI-X table, the NVMe device can be bound to VFIO driver, and the test is passed. The kernel version in my test environment is 4.16.
> 
> So in the change, I do not map any byte of MSI-X table, unfortunately 
> I cannot align the memory "size" in mmap() to page size boundary. From 
> the test result, the change fixes the error. The case looks a little 
> tricky. If we refuse the memory map here, it means we cannot bind VFIO 
> driver with some PCI devices with 64KB kernel page size. I hope we can 
> support such case in DPDK. 😊

Hi Tone,

If it works and doesn't impact any other cases, i'm happy to include the above change (i.e. fall back to unaligned offset if aligning it results in zero offset). I'm curious what would happen if there's something else after the MSI-X table as well, and what alignments would be required for that... But i would rather wait for someone to come to us with an actual test case for that as well :)

> 
>>
>> --
>> Thanks,
>> Anatoly
>>
> 
> 
> --
> Thanks,
> Anatoly
> 


--
Thanks,
Anatoly

[dpdk-dev] [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[tone.zhang]

With a larger PAGE_SIZE it is possible for the MSI table to very
close to the end of the BAR s.t. when we align the start and end
of the MSI table to the PAGE_SIZE, the end offset of the MSI
table is out of the PCI BAR boundary.

This patch addresses the issue by comparing both the start and the
end offset of the MSI table with the BAR size, and skip the mapping
if it is out of Bar scope.

The patch fixes the debug log as below:
EAL: Skipping BAR0

Signed-off-by: tone.zhang <tone.zhang@arm.com>
Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Steve Capper <Steve.Capper@arm.com>
Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index 305cc06..9a0affe 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 	struct pci_msix_table *msix_table = &vfio_res->msix_table;
 	struct pci_map *bar = &vfio_res->maps[bar_index];
 
-	if (bar->size == 0)
+	if (bar->size == 0) {
 		/* Skip this BAR */
+		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
 		return 0;
+	}
 
 	if (msix_table->bar_index == bar_index) {
 		/*
@@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 		 */
 		uint32_t table_start = msix_table->offset;
 		uint32_t table_end = table_start + msix_table->size;
-		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
-		table_start &= PAGE_MASK;
+		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
+		table_start = RTE_ALIGN(table_start, PAGE_SIZE);
+		/* after rounding to PAGE_SIZE, it is over the bar->size,
+		 * fall back to the MSI-X table offset in the bar and
+		 * align with PAGE_SIZE.
+		 */
+		if (table_start >= bar->size) {
+			table_start = RTE_ALIGN_FLOOR(msix_table->offset,
+							PAGE_SIZE);
+			/* after aligning with PAGE_SIZE, if it is less than
+			 * the MSI-X table offset, continue falling back to
+			 * the actual MSI-X table offset in the bar. 
+			 */
+			if (table_start < msix_table->offset)
+				table_start = msix_table->offset;
+		}
 
 		if (table_start == 0 && table_end >= bar->size) {
 			/* Cannot map this BAR */
@@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 
 		memreg[0].offset = bar->offset;
 		memreg[0].size = table_start;
-		memreg[1].offset = bar->offset + table_end;
-		memreg[1].size = bar->size - table_end;
+		if (bar->size < table_end) {
+			/*
+			 * after rounding to PAGE_SIZE we don't have any space
+			 * left after the MSI table, so don't try and map it.
+			 */
+			memreg[1].offset = 0;
+			memreg[1].size = 0;
+		}
+		else {
+			memreg[1].offset = bar->offset + table_end;
+			memreg[1].size = bar->size - table_end;
+		}
 
 		RTE_LOG(DEBUG, EAL,
 			"Trying to map BAR%d that contains the MSI-X "
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 09-Nov-18 5:57 AM, tone.zhang wrote:
> With a larger PAGE_SIZE it is possible for the MSI table to very
> close to the end of the BAR s.t. when we align the start and end
> of the MSI table to the PAGE_SIZE, the end offset of the MSI
> table is out of the PCI BAR boundary.
> 
> This patch addresses the issue by comparing both the start and the
> end offset of the MSI table with the BAR size, and skip the mapping
> if it is out of Bar scope.
> 
> The patch fixes the debug log as below:
> EAL: Skipping BAR0
> 
> Signed-off-by: tone.zhang <tone.zhang@arm.com>
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>

In the future, please don't include my Reviewed tag unless i actually 
sent one :)

> ---
>   drivers/bus/pci/linux/pci_vfio.c | 36 +++++++++++++++++++++++++++++++-----
>   1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index 305cc06..9a0affe 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>   	struct pci_map *bar = &vfio_res->maps[bar_index];
>   
> -	if (bar->size == 0)
> +	if (bar->size == 0) {
>   		/* Skip this BAR */
> +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
>   		return 0;
> +	}
>   
>   	if (msix_table->bar_index == bar_index) {
>   		/*
> @@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   		 */
>   		uint32_t table_start = msix_table->offset;
>   		uint32_t table_end = table_start + msix_table->size;
> -		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> -		table_start &= PAGE_MASK;
> +		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> +		table_start = RTE_ALIGN(table_start, PAGE_SIZE);
> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
> +		 * fall back to the MSI-X table offset in the bar and
> +		 * align with PAGE_SIZE.
> +		 */

Minor nitpick - wording of comment could be better, for example:

if page-aligned start of MSI-X table is beyond BAR size, shrink the 
mapping size to MSI-X table start address.

Also, probably needs newline before comment.

> +		if (table_start >= bar->size) {
> +			table_start = RTE_ALIGN_FLOOR(msix_table->offset,
> +							PAGE_SIZE);
> +			/* after aligning with PAGE_SIZE, if it is less than
> +			 * the MSI-X table offset, continue falling back to
> +			 * the actual MSI-X table offset in the bar.
> +			 */

Same here, wording could probably be improved. Suggested rewording:

If MSI-X table address, floor-aligned by page size, is lower than actual 
MSI-X table offset, fall back to using MSI-X table offset as table start.

Now that i think of it, this could really be expressed like this:

uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
table_start = RTE_MAX(aligned, msix_table_offset);

I believe this would be much clearer.

> +			if (table_start < msix_table->offset)
> +				table_start = msix_table->offset;
> +		}
>   
>   		if (table_start == 0 && table_end >= bar->size) {
>   			/* Cannot map this BAR */
> @@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   
>   		memreg[0].offset = bar->offset;
>   		memreg[0].size = table_start;
> -		memreg[1].offset = bar->offset + table_end;
> -		memreg[1].size = bar->size - table_end;
> +		if (bar->size < table_end) {
> +			/*
> +			 * after rounding to PAGE_SIZE we don't have any space
> +			 * left after the MSI table, so don't try and map it.
> +			 */

Suggested rewording:

If MSI-X table end is beyond BAR end, don't attempt to perform second 
mapping.

> +			memreg[1].offset = 0;
> +			memreg[1].size = 0;
> +		}
> +		else {
> +			memreg[1].offset = bar->offset + table_end;
> +			memreg[1].size = bar->size - table_end;
> +		}
>   
>   		RTE_LOG(DEBUG, EAL,
>   			"Trying to map BAR%d that contains the MSI-X "
> 

However, the patch can go in as is if needed, so

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi Anatoly,

Sorry for the late response.

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Friday, November 9, 2018 8:15 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
> dev@dpdk.org
> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci
> driver
> 
> On 09-Nov-18 5:57 AM, tone.zhang wrote:
> > With a larger PAGE_SIZE it is possible for the MSI table to very close
> > to the end of the BAR s.t. when we align the start and end of the MSI
> > table to the PAGE_SIZE, the end offset of the MSI table is out of the
> > PCI BAR boundary.
> >
> > This patch addresses the issue by comparing both the start and the end
> > offset of the MSI table with the BAR size, and skip the mapping if it
> > is out of Bar scope.
> >
> > The patch fixes the debug log as below:
> > EAL: Skipping BAR0
> >
> > Signed-off-by: tone.zhang <tone.zhang@arm.com>
> > Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> > Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
> 
> In the future, please don't include my Reviewed tag unless i actually sent one :)

Thanks a lot! Will keep in mind. 😊

> 
> > ---
> >   drivers/bus/pci/linux/pci_vfio.c | 36 +++++++++++++++++++++++++++++++---
> --
> >   1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index 305cc06..9a0affe 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
> >   	struct pci_msix_table *msix_table = &vfio_res->msix_table;
> >   	struct pci_map *bar = &vfio_res->maps[bar_index];
> >
> > -	if (bar->size == 0)
> > +	if (bar->size == 0) {
> >   		/* Skip this BAR */
> > +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
> >   		return 0;
> > +	}
> >
> >   	if (msix_table->bar_index == bar_index) {
> >   		/*
> > @@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
> >   		 */
> >   		uint32_t table_start = msix_table->offset;
> >   		uint32_t table_end = table_start + msix_table->size;
> > -		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> > -		table_start &= PAGE_MASK;
> > +		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> > +		table_start = RTE_ALIGN(table_start, PAGE_SIZE);
> > +		/* after rounding to PAGE_SIZE, it is over the bar->size,
> > +		 * fall back to the MSI-X table offset in the bar and
> > +		 * align with PAGE_SIZE.
> > +		 */
> 
> Minor nitpick - wording of comment could be better, for example:
> 
> if page-aligned start of MSI-X table is beyond BAR size, shrink the mapping size
> to MSI-X table start address.
> 
> Also, probably needs newline before comment.
>

Will update the code in next version. Thanks!

> > +		if (table_start >= bar->size) {
> > +			table_start = RTE_ALIGN_FLOOR(msix_table->offset,
> > +							PAGE_SIZE);
> > +			/* after aligning with PAGE_SIZE, if it is less than
> > +			 * the MSI-X table offset, continue falling back to
> > +			 * the actual MSI-X table offset in the bar.
> > +			 */
> 
> Same here, wording could probably be improved. Suggested rewording:
> 
> If MSI-X table address, floor-aligned by page size, is lower than actual MSI-X
> table offset, fall back to using MSI-X table offset as table start.
> 
> Now that i think of it, this could really be expressed like this:
> 
> uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
> table_start = RTE_MAX(aligned, msix_table_offset);
> 
> I believe this would be much clearer.
>

Will update the patch.
 
> > +			if (table_start < msix_table->offset)
> > +				table_start = msix_table->offset;
> > +		}
> >
> >   		if (table_start == 0 && table_end >= bar->size) {
> >   			/* Cannot map this BAR */
> > @@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > mapped_pci_resource *vfio_res,
> >
> >   		memreg[0].offset = bar->offset;
> >   		memreg[0].size = table_start;
> > -		memreg[1].offset = bar->offset + table_end;
> > -		memreg[1].size = bar->size - table_end;
> > +		if (bar->size < table_end) {
> > +			/*
> > +			 * after rounding to PAGE_SIZE we don't have any space
> > +			 * left after the MSI table, so don't try and map it.
> > +			 */
> 
> Suggested rewording:
> 
> If MSI-X table end is beyond BAR end, don't attempt to perform second mapping.
> 

Thanks a lot. Will update.

> > +			memreg[1].offset = 0;
> > +			memreg[1].size = 0;
> > +		}
> > +		else {
> > +			memreg[1].offset = bar->offset + table_end;
> > +			memreg[1].size = bar->size - table_end;
> > +		}
> >
> >   		RTE_LOG(DEBUG, EAL,
> >   			"Trying to map BAR%d that contains the MSI-X "
> >
> 
> However, the patch can go in as is if needed, so
> 
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> 

Thanks! 😉

> --
> Thanks,
> Anatoly

Br,
Tone

Re: [dpdk-dev] [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi Anatoly,

I have some comments.

> -----Original Message-----
> From: Tone Zhang (Arm Technology China)
> Sent: Thursday, November 15, 2018 8:49 AM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org
> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>; nd <nd@arm.com>
> Subject: RE: [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci
> driver
> 
> Hi Anatoly,
> 
> Sorry for the late response.
> 
> > -----Original Message-----
> > From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > Sent: Friday, November 9, 2018 8:15 PM
> > To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
> > dev@dpdk.org
> > Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
> > Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> > <Steve.Capper@arm.com>; nd <nd@arm.com>
> > Subject: Re: [PATCH v2] pci_vfio: Support 64KB kernel page_size with
> > vfio-pci driver
> >
> > On 09-Nov-18 5:57 AM, tone.zhang wrote:
> > > With a larger PAGE_SIZE it is possible for the MSI table to very
> > > close to the end of the BAR s.t. when we align the start and end of
> > > the MSI table to the PAGE_SIZE, the end offset of the MSI table is
> > > out of the PCI BAR boundary.
> > >
> > > This patch addresses the issue by comparing both the start and the
> > > end offset of the MSI table with the BAR size, and skip the mapping
> > > if it is out of Bar scope.
> > >
> > > The patch fixes the debug log as below:
> > > EAL: Skipping BAR0
> > >
> > > Signed-off-by: tone.zhang <tone.zhang@arm.com>
> > > Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> > > Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
> >
> > In the future, please don't include my Reviewed tag unless i actually
> > sent one :)
> 
> Thanks a lot! Will keep in mind. 😊
> 
> >
> > > ---
> > >   drivers/bus/pci/linux/pci_vfio.c | 36
> > > +++++++++++++++++++++++++++++++---
> > --
> > >   1 file changed, 31 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > > b/drivers/bus/pci/linux/pci_vfio.c
> > > index 305cc06..9a0affe 100644
> > > --- a/drivers/bus/pci/linux/pci_vfio.c
> > > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > > @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > mapped_pci_resource *vfio_res,
> > >   	struct pci_msix_table *msix_table = &vfio_res->msix_table;
> > >   	struct pci_map *bar = &vfio_res->maps[bar_index];
> > >
> > > -	if (bar->size == 0)
> > > +	if (bar->size == 0) {
> > >   		/* Skip this BAR */
> > > +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
> > >   		return 0;
> > > +	}
> > >
> > >   	if (msix_table->bar_index == bar_index) {
> > >   		/*
> > > @@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > mapped_pci_resource *vfio_res,
> > >   		 */
> > >   		uint32_t table_start = msix_table->offset;
> > >   		uint32_t table_end = table_start + msix_table->size;
> > > -		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> > > -		table_start &= PAGE_MASK;
> > > +		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> > > +		table_start = RTE_ALIGN(table_start, PAGE_SIZE);
> > > +		/* after rounding to PAGE_SIZE, it is over the bar->size,
> > > +		 * fall back to the MSI-X table offset in the bar and
> > > +		 * align with PAGE_SIZE.
> > > +		 */
> >
> > Minor nitpick - wording of comment could be better, for example:
> >
> > if page-aligned start of MSI-X table is beyond BAR size, shrink the
> > mapping size to MSI-X table start address.
> >
> > Also, probably needs newline before comment.
> >
> 
> Will update the code in next version. Thanks!
> 
> > > +		if (table_start >= bar->size) {
> > > +			table_start = RTE_ALIGN_FLOOR(msix_table->offset,
> > > +							PAGE_SIZE);
> > > +			/* after aligning with PAGE_SIZE, if it is less than
> > > +			 * the MSI-X table offset, continue falling back to
> > > +			 * the actual MSI-X table offset in the bar.
> > > +			 */
> >
> > Same here, wording could probably be improved. Suggested rewording:
> >
> > If MSI-X table address, floor-aligned by page size, is lower than
> > actual MSI-X table offset, fall back to using MSI-X table offset as table start.
> >
> > Now that i think of it, this could really be expressed like this:
> >
> > uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
> > table_start = RTE_MAX(aligned, msix_table_offset);
> >
> > I believe this would be much clearer.
> >
> 
> Will update the patch.
> 

When enter the judgement, it implies the "msix_table->offset" is NOT page size aligned, I think we can replace the code in the judgement with one line: table_start = msix_table->offset;
It looks more simple. What's your opinion? Thanks!

> > > +			if (table_start < msix_table->offset)
> > > +				table_start = msix_table->offset;
> > > +		}
> > >
> > >   		if (table_start == 0 && table_end >= bar->size) {
> > >   			/* Cannot map this BAR */
> > > @@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > > mapped_pci_resource *vfio_res,
> > >
> > >   		memreg[0].offset = bar->offset;
> > >   		memreg[0].size = table_start;
> > > -		memreg[1].offset = bar->offset + table_end;
> > > -		memreg[1].size = bar->size - table_end;
> > > +		if (bar->size < table_end) {
> > > +			/*
> > > +			 * after rounding to PAGE_SIZE we don't have any space
> > > +			 * left after the MSI table, so don't try and map it.
> > > +			 */
> >
> > Suggested rewording:
> >
> > If MSI-X table end is beyond BAR end, don't attempt to perform second
> mapping.
> >
> 
> Thanks a lot. Will update.
> 
> > > +			memreg[1].offset = 0;
> > > +			memreg[1].size = 0;
> > > +		}
> > > +		else {
> > > +			memreg[1].offset = bar->offset + table_end;
> > > +			memreg[1].size = bar->size - table_end;
> > > +		}
> > >
> > >   		RTE_LOG(DEBUG, EAL,
> > >   			"Trying to map BAR%d that contains the MSI-X "
> > >
> >
> > However, the patch can go in as is if needed, so
> >
> > Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >
> 
> Thanks! 😉
> 
> > --
> > Thanks,
> > Anatoly
> 
> Br,
> Tone

Br,
Tone

Re: [dpdk-dev] [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 16-Nov-18 2:34 AM, Tone Zhang (Arm Technology China) wrote:
> Hi Anatoly,
> 
> I have some comments.
> 
>> -----Original Message-----
>> From: Tone Zhang (Arm Technology China)
>> Sent: Thursday, November 15, 2018 8:49 AM
>> To: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org
>> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
>> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
>> <Steve.Capper@arm.com>; nd <nd@arm.com>
>> Subject: RE: [PATCH v2] pci_vfio: Support 64KB kernel page_size with vfio-pci
>> driver
>>
>> Hi Anatoly,
>>
>> Sorry for the late response.
>>
>>> -----Original Message-----
>>> From: Burakov, Anatoly <anatoly.burakov@intel.com>
>>> Sent: Friday, November 9, 2018 8:15 PM
>>> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
>>> dev@dpdk.org
>>> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
>>> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
>>> <Steve.Capper@arm.com>; nd <nd@arm.com>
>>> Subject: Re: [PATCH v2] pci_vfio: Support 64KB kernel page_size with
>>> vfio-pci driver
>>>
>>> On 09-Nov-18 5:57 AM, tone.zhang wrote:
>>>> With a larger PAGE_SIZE it is possible for the MSI table to very
>>>> close to the end of the BAR s.t. when we align the start and end of
>>>> the MSI table to the PAGE_SIZE, the end offset of the MSI table is
>>>> out of the PCI BAR boundary.
>>>>
>>>> This patch addresses the issue by comparing both the start and the
>>>> end offset of the MSI table with the BAR size, and skip the mapping
>>>> if it is out of Bar scope.
>>>>
>>>> The patch fixes the debug log as below:
>>>> EAL: Skipping BAR0
>>>>
>>>> Signed-off-by: tone.zhang <tone.zhang@arm.com>
>>>> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>>> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
>>>> Reviewed-by: Burakov Anatoly <anatoly.burakov@intel.com>
>>>
>>> In the future, please don't include my Reviewed tag unless i actually
>>> sent one :)
>>
>> Thanks a lot! Will keep in mind. 😊
>>
>>>
>>>> ---
>>>>    drivers/bus/pci/linux/pci_vfio.c | 36
>>>> +++++++++++++++++++++++++++++++---
>>> --
>>>>    1 file changed, 31 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/bus/pci/linux/pci_vfio.c
>>>> b/drivers/bus/pci/linux/pci_vfio.c
>>>> index 305cc06..9a0affe 100644
>>>> --- a/drivers/bus/pci/linux/pci_vfio.c
>>>> +++ b/drivers/bus/pci/linux/pci_vfio.c
>>>> @@ -445,9 +445,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
>>> mapped_pci_resource *vfio_res,
>>>>    	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>>>>    	struct pci_map *bar = &vfio_res->maps[bar_index];
>>>>
>>>> -	if (bar->size == 0)
>>>> +	if (bar->size == 0) {
>>>>    		/* Skip this BAR */
>>>> +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
>>>>    		return 0;
>>>> +	}
>>>>
>>>>    	if (msix_table->bar_index == bar_index) {
>>>>    		/*
>>>> @@ -456,8 +458,22 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
>>> mapped_pci_resource *vfio_res,
>>>>    		 */
>>>>    		uint32_t table_start = msix_table->offset;
>>>>    		uint32_t table_end = table_start + msix_table->size;
>>>> -		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
>>>> -		table_start &= PAGE_MASK;
>>>> +		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
>>>> +		table_start = RTE_ALIGN(table_start, PAGE_SIZE);
>>>> +		/* after rounding to PAGE_SIZE, it is over the bar->size,
>>>> +		 * fall back to the MSI-X table offset in the bar and
>>>> +		 * align with PAGE_SIZE.
>>>> +		 */
>>>
>>> Minor nitpick - wording of comment could be better, for example:
>>>
>>> if page-aligned start of MSI-X table is beyond BAR size, shrink the
>>> mapping size to MSI-X table start address.
>>>
>>> Also, probably needs newline before comment.
>>>
>>
>> Will update the code in next version. Thanks!
>>
>>>> +		if (table_start >= bar->size) {
>>>> +			table_start = RTE_ALIGN_FLOOR(msix_table->offset,
>>>> +							PAGE_SIZE);
>>>> +			/* after aligning with PAGE_SIZE, if it is less than
>>>> +			 * the MSI-X table offset, continue falling back to
>>>> +			 * the actual MSI-X table offset in the bar.
>>>> +			 */
>>>
>>> Same here, wording could probably be improved. Suggested rewording:
>>>
>>> If MSI-X table address, floor-aligned by page size, is lower than
>>> actual MSI-X table offset, fall back to using MSI-X table offset as table start.
>>>
>>> Now that i think of it, this could really be expressed like this:
>>>
>>> uint32_t aligned = RTE_ALIGN_FLOOR(msix_table->offset, PAGE_SIZE);
>>> table_start = RTE_MAX(aligned, msix_table_offset);
>>>
>>> I believe this would be much clearer.
>>>
>>
>> Will update the patch.
>>
> 
> When enter the judgement, it implies the "msix_table->offset" is NOT page size aligned, I think we can replace the code in the judgement with one line: table_start = msix_table->offset;
> It looks more simple. What's your opinion? Thanks!

Agree, what was i thinking :D

> 
>>>> +			if (table_start < msix_table->offset)
>>>> +				table_start = msix_table->offset;
>>>> +		}
>>>>
>>>>    		if (table_start == 0 && table_end >= bar->size) {
>>>>    			/* Cannot map this BAR */
>>>> @@ -469,8 +485,18 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
>>>> mapped_pci_resource *vfio_res,
>>>>
>>>>    		memreg[0].offset = bar->offset;
>>>>    		memreg[0].size = table_start;
>>>> -		memreg[1].offset = bar->offset + table_end;
>>>> -		memreg[1].size = bar->size - table_end;
>>>> +		if (bar->size < table_end) {
>>>> +			/*
>>>> +			 * after rounding to PAGE_SIZE we don't have any space
>>>> +			 * left after the MSI table, so don't try and map it.
>>>> +			 */
>>>
>>> Suggested rewording:
>>>
>>> If MSI-X table end is beyond BAR end, don't attempt to perform second
>> mapping.
>>>
>>
>> Thanks a lot. Will update.
>>
>>>> +			memreg[1].offset = 0;
>>>> +			memreg[1].size = 0;
>>>> +		}
>>>> +		else {
>>>> +			memreg[1].offset = bar->offset + table_end;
>>>> +			memreg[1].size = bar->size - table_end;
>>>> +		}
>>>>
>>>>    		RTE_LOG(DEBUG, EAL,
>>>>    			"Trying to map BAR%d that contains the MSI-X "
>>>>
>>>
>>> However, the patch can go in as is if needed, so
>>>
>>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>>
>>
>> Thanks! 😉
>>
>>> --
>>> Thanks,
>>> Anatoly
>>
>> Br,
>> Tone
> 
> Br,
> Tone
> 


-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[tone.zhang]

With a larger PAGE_SIZE it is possible for the MSI table to very
close to the end of the BAR s.t. when we align the start and end
of the MSI table to the PAGE_SIZE, the end offset of the MSI
table is out of the PCI BAR boundary.

This patch addresses the issue by comparing both the start and the
end offset of the MSI table with the BAR size, and skip the mapping
if it is out of Bar scope.

The patch fixes the debug log as below:
EAL: Skipping BAR0

Signed-off-by: tone.zhang <tone.zhang@arm.com>
Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Steve Capper <Steve.Capper@arm.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index ffd26f1..0e7bfd7 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -457,9 +457,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 	struct pci_msix_table *msix_table = &vfio_res->msix_table;
 	struct pci_map *bar = &vfio_res->maps[bar_index];
 
-	if (bar->size == 0)
+	if (bar->size == 0) {
 		/* Skip this BAR */
+		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
 		return 0;
+	}
 
 	if (msix_table->bar_index == bar_index) {
 		/*
@@ -468,8 +470,14 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 		 */
 		uint32_t table_start = msix_table->offset;
 		uint32_t table_end = table_start + msix_table->size;
-		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
-		table_start &= PAGE_MASK;
+		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
+		table_start = RTE_ALIGN(table_start, PAGE_SIZE);
+
+		/* If page-aligned start of MSI-X table is beyond BAR size,
+		 * shrink the mapping size to MSI-X table start address.
+		 */
+		if (table_start >= bar->size)
+			table_start = msix_table->offset;
 
 		if (table_start == 0 && table_end >= bar->size) {
 			/* Cannot map this BAR */
@@ -481,8 +489,17 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 
 		memreg[0].offset = bar->offset;
 		memreg[0].size = table_start;
-		memreg[1].offset = bar->offset + table_end;
-		memreg[1].size = bar->size - table_end;
+		if (bar->size < table_end) {
+			/*
+			 * If MSI-X table end is beyond BAR end, don't attempt
+			 * to perform second mapping.
+			 */
+			memreg[1].offset = 0;
+			memreg[1].size = 0;
+		} else {
+			memreg[1].offset = bar->offset + table_end;
+			memreg[1].size = bar->size - table_end;
+		}
 
 		RTE_LOG(DEBUG, EAL,
 			"Trying to map BAR%d that contains the MSI-X "
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi all,

Could anyone please have a review and re-test the change?

Thanks a lot!

Br,
Tone

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of tone.zhang
> Sent: Monday, November 19, 2018 10:37 AM
> To: dev@dpdk.org
> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>; anatoly.burakov@intel.com; nd <nd@arm.com>
> Subject: [dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with
> vfio-pci driver
> 
> With a larger PAGE_SIZE it is possible for the MSI table to very close to the end
> of the BAR s.t. when we align the start and end of the MSI table to the
> PAGE_SIZE, the end offset of the MSI table is out of the PCI BAR boundary.
> 
> This patch addresses the issue by comparing both the start and the end offset of
> the MSI table with the BAR size, and skip the mapping if it is out of Bar scope.
> 
> The patch fixes the debug log as below:
> EAL: Skipping BAR0
> 
> Signed-off-by: tone.zhang <tone.zhang@arm.com>
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  drivers/bus/pci/linux/pci_vfio.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index ffd26f1..0e7bfd7 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -457,9 +457,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
>  	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>  	struct pci_map *bar = &vfio_res->maps[bar_index];
> 
> -	if (bar->size == 0)
> +	if (bar->size == 0) {
>  		/* Skip this BAR */
> +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
>  		return 0;
> +	}
> 
>  	if (msix_table->bar_index == bar_index) {
>  		/*
> @@ -468,8 +470,14 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
>  		 */
>  		uint32_t table_start = msix_table->offset;
>  		uint32_t table_end = table_start + msix_table->size;
> -		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> -		table_start &= PAGE_MASK;
> +		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> +		table_start = RTE_ALIGN(table_start, PAGE_SIZE);
> +
> +		/* If page-aligned start of MSI-X table is beyond BAR size,
> +		 * shrink the mapping size to MSI-X table start address.
> +		 */
> +		if (table_start >= bar->size)
> +			table_start = msix_table->offset;
> 
>  		if (table_start == 0 && table_end >= bar->size) {
>  			/* Cannot map this BAR */
> @@ -481,8 +489,17 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
> 
>  		memreg[0].offset = bar->offset;
>  		memreg[0].size = table_start;
> -		memreg[1].offset = bar->offset + table_end;
> -		memreg[1].size = bar->size - table_end;
> +		if (bar->size < table_end) {
> +			/*
> +			 * If MSI-X table end is beyond BAR end, don't attempt
> +			 * to perform second mapping.
> +			 */
> +			memreg[1].offset = 0;
> +			memreg[1].size = 0;
> +		} else {
> +			memreg[1].offset = bar->offset + table_end;
> +			memreg[1].size = bar->size - table_end;
> +		}
> 
>  		RTE_LOG(DEBUG, EAL,
>  			"Trying to map BAR%d that contains the MSI-X "
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 03-Dec-18 7:25 AM, Tone Zhang (Arm Technology China) wrote:
> Hi all,
> 
> Could anyone please have a review and re-test the change?
> 
> Thanks a lot!
> 
> Br,
> Tone

Tested with latest 18.11, all seems to work fine.

Tested-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 19-Nov-18 2:37 AM, tone.zhang wrote:
> With a larger PAGE_SIZE it is possible for the MSI table to very
> close to the end of the BAR s.t. when we align the start and end
> of the MSI table to the PAGE_SIZE, the end offset of the MSI
> table is out of the PCI BAR boundary.
> 
> This patch addresses the issue by comparing both the start and the
> end offset of the MSI table with the BAR size, and skip the mapping
> if it is out of Bar scope.
> 
> The patch fixes the debug log as below:
> EAL: Skipping BAR0
> 
> Signed-off-by: tone.zhang <tone.zhang@arm.com>
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   drivers/bus/pci/linux/pci_vfio.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
> index ffd26f1..0e7bfd7 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -457,9 +457,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   	struct pci_msix_table *msix_table = &vfio_res->msix_table;
>   	struct pci_map *bar = &vfio_res->maps[bar_index];
>   
> -	if (bar->size == 0)
> +	if (bar->size == 0) {
>   		/* Skip this BAR */
> +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
>   		return 0;

I would perhaps make it a DEBUG rather than INFO.

> +	}
>   
>   	if (msix_table->bar_index == bar_index) {
>   		/*
> @@ -468,8 +470,14 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   		 */
>   		uint32_t table_start = msix_table->offset;
>   		uint32_t table_end = table_start + msix_table->size;
> -		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> -		table_start &= PAGE_MASK;
> +		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> +		table_start = RTE_ALIGN(table_start, PAGE_SIZE);

I believe table_start &= PAGE_MASK is equivalent to table_start = 
RTE_ALIGN_FLOOR(), not RTE_ALIGN() (which is an alias for RTE_ALIGN_CEIL()).

> +
> +		/* If page-aligned start of MSI-X table is beyond BAR size,
> +		 * shrink the mapping size to MSI-X table start address.
> +		 */
> +		if (table_start >= bar->size)
> +			table_start = msix_table->offset;
>   
>   		if (table_start == 0 && table_end >= bar->size) {
>   			/* Cannot map this BAR */
> @@ -481,8 +489,17 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
>   
>   		memreg[0].offset = bar->offset;
>   		memreg[0].size = table_start;
> -		memreg[1].offset = bar->offset + table_end;
> -		memreg[1].size = bar->size - table_end;
> +		if (bar->size < table_end) {
> +			/*
> +			 * If MSI-X table end is beyond BAR end, don't attempt
> +			 * to perform second mapping.
> +			 */
> +			memreg[1].offset = 0;
> +			memreg[1].size = 0;
> +		} else {
> +			memreg[1].offset = bar->offset + table_end;
> +			memreg[1].size = bar->size - table_end;
> +		}
>   
>   		RTE_LOG(DEBUG, EAL,
>   			"Trying to map BAR%d that contains the MSI-X "
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Stephen Hemminger]

On Mon, 10 Dec 2018 11:45:37 +0000
"Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:

> >   		/* Skip this BAR */
> > +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
> >   		return 0;  
> 
> I would perhaps make it a DEBUG rather than INFO.

And drop the comment...

Re: [dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi Stephen,

Thanks for the comments.

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, December 10, 2018 11:56 PM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
> dev@dpdk.org; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with
> vfio-pci driver
> 
> On Mon, 10 Dec 2018 11:45:37 +0000
> "Burakov, Anatoly" <anatoly.burakov@intel.com> wrote:
> 
> > >   		/* Skip this BAR */
> > > +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
> > >   		return 0;
> >
> > I would perhaps make it a DEBUG rather than INFO.
> 
> And drop the comment...

Will drop the comment in v4.

Thanks.

Br,
Tone

Re: [dpdk-dev] [PATCH v3] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Tone Zhang (Arm Technology China)]

Hi Anatoly,

Thanks for the comments.

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Monday, December 10, 2018 7:46 PM
> To: Tone Zhang (Arm Technology China) <Tone.Zhang@arm.com>;
> dev@dpdk.org
> Cc: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v3] pci_vfio: Support 64KB kernel page_size with vfio-pci
> driver
> 
> On 19-Nov-18 2:37 AM, tone.zhang wrote:
> > With a larger PAGE_SIZE it is possible for the MSI table to very close
> > to the end of the BAR s.t. when we align the start and end of the MSI
> > table to the PAGE_SIZE, the end offset of the MSI table is out of the
> > PCI BAR boundary.
> >
> > This patch addresses the issue by comparing both the start and the end
> > offset of the MSI table with the BAR size, and skip the mapping if it
> > is out of Bar scope.
> >
> > The patch fixes the debug log as below:
> > EAL: Skipping BAR0
> >
> > Signed-off-by: tone.zhang <tone.zhang@arm.com>
> > Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> > Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > ---
> >   drivers/bus/pci/linux/pci_vfio.c | 27 ++++++++++++++++++++++-----
> >   1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index ffd26f1..0e7bfd7 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -457,9 +457,11 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
> >   	struct pci_msix_table *msix_table = &vfio_res->msix_table;
> >   	struct pci_map *bar = &vfio_res->maps[bar_index];
> >
> > -	if (bar->size == 0)
> > +	if (bar->size == 0) {
> >   		/* Skip this BAR */
> > +		RTE_LOG(INFO, EAL, "Skipping BAR%d\n", bar_index);
> >   		return 0;
> 
> I would perhaps make it a DEBUG rather than INFO.

Will update in v4 version soon.

> 
> > +	}
> >
> >   	if (msix_table->bar_index == bar_index) {
> >   		/*
> > @@ -468,8 +470,14 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> mapped_pci_resource *vfio_res,
> >   		 */
> >   		uint32_t table_start = msix_table->offset;
> >   		uint32_t table_end = table_start + msix_table->size;
> > -		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
> > -		table_start &= PAGE_MASK;
> > +		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
> > +		table_start = RTE_ALIGN(table_start, PAGE_SIZE);
> 
> I believe table_start &= PAGE_MASK is equivalent to table_start =
> RTE_ALIGN_FLOOR(), not RTE_ALIGN() (which is an alias for RTE_ALIGN_CEIL()).

Will change to RTE_ALIGN_FLOOR. Then table_start will be less than bar->size, the below code will be updated further in v4.

> 
> > +
> > +		/* If page-aligned start of MSI-X table is beyond BAR size,
> > +		 * shrink the mapping size to MSI-X table start address.
> > +		 */
> > +		if (table_start >= bar->size)
> > +			table_start = msix_table->offset;
> >
> >   		if (table_start == 0 && table_end >= bar->size) {
> >   			/* Cannot map this BAR */
> > @@ -481,8 +489,17 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct
> > mapped_pci_resource *vfio_res,
> >
> >   		memreg[0].offset = bar->offset;
> >   		memreg[0].size = table_start;
> > -		memreg[1].offset = bar->offset + table_end;
> > -		memreg[1].size = bar->size - table_end;
> > +		if (bar->size < table_end) {
> > +			/*
> > +			 * If MSI-X table end is beyond BAR end, don't attempt
> > +			 * to perform second mapping.
> > +			 */
> > +			memreg[1].offset = 0;
> > +			memreg[1].size = 0;
> > +		} else {
> > +			memreg[1].offset = bar->offset + table_end;
> > +			memreg[1].size = bar->size - table_end;
> > +		}
> >
> >   		RTE_LOG(DEBUG, EAL,
> >   			"Trying to map BAR%d that contains the MSI-X "
> >
> 
> 
> --
> Thanks,
> Anatoly

Thnaks!

Br,
Tone

[dpdk-dev] [PATCH v4] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[tone.zhang]

With a larger PAGE_SIZE it is possible for the MSI table to very
close to the end of the BAR s.t. when we align the start and end
of the MSI table to the PAGE_SIZE, the end offset of the MSI
table is out of the PCI BAR boundary.

This patch addresses the issue by comparing both the start and the
end offset of the MSI table with the BAR size, and skip the mapping
if it is out of Bar scope.

The patch fixes the debug log as below:
EAL: Skipping BAR0

Signed-off-by: tone.zhang <tone.zhang@arm.com>
Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Steve Capper <Steve.Capper@arm.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 drivers/bus/pci/linux/pci_vfio.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c
index ffd26f1..22e9de0 100644
--- a/drivers/bus/pci/linux/pci_vfio.c
+++ b/drivers/bus/pci/linux/pci_vfio.c
@@ -457,9 +457,10 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 	struct pci_msix_table *msix_table = &vfio_res->msix_table;
 	struct pci_map *bar = &vfio_res->maps[bar_index];
 
-	if (bar->size == 0)
-		/* Skip this BAR */
+	if (bar->size == 0) {
+		RTE_LOG(DEBUG, EAL, "Bar size is 0, skip BAR%d\n", bar_index);
 		return 0;
+	}
 
 	if (msix_table->bar_index == bar_index) {
 		/*
@@ -468,8 +469,15 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 		 */
 		uint32_t table_start = msix_table->offset;
 		uint32_t table_end = table_start + msix_table->size;
-		table_end = (table_end + ~PAGE_MASK) & PAGE_MASK;
-		table_start &= PAGE_MASK;
+		table_end = RTE_ALIGN(table_end, PAGE_SIZE);
+		table_start = RTE_ALIGN_FLOOR(table_start, PAGE_SIZE);
+
+		/* If page-aligned start of MSI-X table is less than the
+		 * actual MSI-X table start address, reassign to the actual
+		 * start address.
+		 */
+		if (table_start < msix_table->offset)
+			table_start = msix_table->offset;
 
 		if (table_start == 0 && table_end >= bar->size) {
 			/* Cannot map this BAR */
@@ -481,8 +489,17 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res,
 
 		memreg[0].offset = bar->offset;
 		memreg[0].size = table_start;
-		memreg[1].offset = bar->offset + table_end;
-		memreg[1].size = bar->size - table_end;
+		if (bar->size < table_end) {
+			/*
+			 * If MSI-X table end is beyond BAR end, don't attempt
+			 * to perform second mapping.
+			 */
+			memreg[1].offset = 0;
+			memreg[1].size = 0;
+		} else {
+			memreg[1].offset = bar->offset + table_end;
+			memreg[1].size = bar->size - table_end;
+		}
 
 		RTE_LOG(DEBUG, EAL,
 			"Trying to map BAR%d that contains the MSI-X "
-- 
2.7.4

Re: [dpdk-dev] [PATCH v4] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Burakov, Anatoly]

On 12-Dec-18 11:25 AM, tone.zhang wrote:
> With a larger PAGE_SIZE it is possible for the MSI table to very
> close to the end of the BAR s.t. when we align the start and end
> of the MSI table to the PAGE_SIZE, the end offset of the MSI
> table is out of the PCI BAR boundary.
> 
> This patch addresses the issue by comparing both the start and the
> end offset of the MSI table with the BAR size, and skip the mapping
> if it is out of Bar scope.
> 
> The patch fixes the debug log as below:
> EAL: Skipping BAR0
> 
> Signed-off-by: tone.zhang <tone.zhang@arm.com>
> Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v4] pci_vfio: Support 64KB kernel page_size with vfio-pci driver

[Thomas Monjalon]

12/12/2018 12:27, Burakov, Anatoly:
> On 12-Dec-18 11:25 AM, tone.zhang wrote:
> > With a larger PAGE_SIZE it is possible for the MSI table to very
> > close to the end of the BAR s.t. when we align the start and end
> > of the MSI table to the PAGE_SIZE, the end offset of the MSI
> > table is out of the PCI BAR boundary.
> > 
> > This patch addresses the issue by comparing both the start and the
> > end offset of the MSI table with the BAR size, and skip the mapping
> > if it is out of Bar scope.
> > 
> > The patch fixes the debug log as below:
> > EAL: Skipping BAR0
> > 
> > Signed-off-by: tone.zhang <tone.zhang@arm.com>
> > Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Steve Capper <Steve.Capper@arm.com>
> > Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > ---
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks